feat: add V2 workflow state backbone#912
feat: add V2 workflow state backbone#912ChuxiJ wants to merge 1 commit intocodex/v2-905-design-systemfrom
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughIntroduces a V2 workflow model: new enums (transport/workflow), per-session and per-take state, state version bump to 2, XML serialization/migration for legacy fields, processor lifecycle/state transitions, and new V2 UI components and look-and-feel for session/transport/result/preview panels. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/acestep_vst3/src/PluginEnums.cpp (1)
176-178: Consider adding"compareready"(no delimiter) variant for consistency.The
jobStatusFromStringfunction accepts variants like"queued/running"without delimiters, buttransportStateFromStringonly handles"compare ready","compare_ready", and"compare-ready". Adding"compareready"would improve robustness if users paste values without delimiters.♻️ Suggested addition
- if (key == "compare ready" || key == "compare_ready" || key == "compare-ready") + if (key == "compare ready" || key == "compare_ready" || key == "compare-ready" || key == "compareready") { return TransportState::compareReady; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/acestep_vst3/src/PluginEnums.cpp` around lines 176 - 178, In transportStateFromString, add the delimiter-less variant "compareready" to the set of accepted keys so TransportState::compareReady is returned for inputs without spaces or punctuation; update the branch that currently checks for "compare ready", "compare_ready", and "compare-ready" to also accept "compareready" to make parsing more robust.plugins/acestep_vst3/src/PluginProcessor.cpp (2)
20-27: Add bounds check tomirrorTakeIntoLegacyFieldsfor defensive coding.The function casts
slotIndextosize_twithout validating it's within[0, kResultSlotCount). While current callers appear to pass valid indices, a negative or out-of-range value would cause undefined behavior.🛡️ Proposed fix
void mirrorTakeIntoLegacyFields(PluginState& state, int slotIndex) { + if (slotIndex < 0 || slotIndex >= kResultSlotCount) + { + return; + } const auto index = static_cast<size_t>(slotIndex); const auto& take = state.resultTakes[index];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/acestep_vst3/src/PluginProcessor.cpp` around lines 20 - 27, The function mirrorTakeIntoLegacyFields currently casts slotIndex to size_t and indexes into arrays without validation; add a defensive bounds check at the start of mirrorTakeIntoLegacyFields to ensure slotIndex is >= 0 and less than the valid slot count (use kResultSlotCount or validate against state.resultTakes.size()/state.resultSlots.size()), and early-return (optionally log) if out-of-range to avoid undefined behavior when accessing state.resultTakes, state.resultSlots, state.resultFileUrls, or state.resultLocalPaths.
391-392:schedulePreviewDownloadreads from legacyresultFileUrlsinstead ofresultTakes.The V2 code populates
resultTakes[].remoteFileUrlinapplyCompletedTask, then mirrors to legacy arrays. However,schedulePreviewDownloadreads directly fromstate_.resultFileUrls. If called before mirroring completes (e.g., viapendingPreviewDownloadSlot_set at line 498), this could use stale data.Consider reading from
resultTakes[].remoteFileUrlfor consistency with the V2 model.♻️ Suggested fix
const auto baseUrl = state_.backendBaseUrl; - const auto remoteFileUrl = state_.resultFileUrls[static_cast<size_t>(slotIndex)]; + const auto& take = state_.resultTakes[static_cast<size_t>(slotIndex)]; + const auto remoteFileUrl = take.remoteFileUrl.isNotEmpty() + ? take.remoteFileUrl + : state_.resultFileUrls[static_cast<size_t>(slotIndex)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/acestep_vst3/src/PluginProcessor.cpp` around lines 391 - 392, schedulePreviewDownload currently reads legacy state_.resultFileUrls which can be stale; update it to read the V2 field state_.resultTakes[slotIndex].remoteFileUrl (use that as the primary source) and only fall back to state_.resultFileUrls[static_cast<size_t>(slotIndex)] if the take entry is empty/null. Locate schedulePreviewDownload and replace the remoteFileUrl lookup, ensuring behavior remains compatible with applyCompletedTask mirroring and the pendingPreviewDownloadSlot_ flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/acestep_vst3/src/PluginState.cpp`:
- Around line 179-197: The migration currently remaps state.transportState
whenever it equals TransportState::idle, which can incorrectly override
legitimate v2 idle states; update the migration in PluginState.cpp to only
perform this derivation for legacy states by gating it on schemaVersion < 2 (or
by checking that the transportState XML attribute was actually missing during
deserialization) before inspecting state.jobStatus, and leave transportState
unchanged for true v2 states; reference the transportState/jobStatus logic and
the schemaVersion check in the deserialization/migration area to implement this
conditional gating.
---
Nitpick comments:
In `@plugins/acestep_vst3/src/PluginEnums.cpp`:
- Around line 176-178: In transportStateFromString, add the delimiter-less
variant "compareready" to the set of accepted keys so
TransportState::compareReady is returned for inputs without spaces or
punctuation; update the branch that currently checks for "compare ready",
"compare_ready", and "compare-ready" to also accept "compareready" to make
parsing more robust.
In `@plugins/acestep_vst3/src/PluginProcessor.cpp`:
- Around line 20-27: The function mirrorTakeIntoLegacyFields currently casts
slotIndex to size_t and indexes into arrays without validation; add a defensive
bounds check at the start of mirrorTakeIntoLegacyFields to ensure slotIndex is
>= 0 and less than the valid slot count (use kResultSlotCount or validate
against state.resultTakes.size()/state.resultSlots.size()), and early-return
(optionally log) if out-of-range to avoid undefined behavior when accessing
state.resultTakes, state.resultSlots, state.resultFileUrls, or
state.resultLocalPaths.
- Around line 391-392: schedulePreviewDownload currently reads legacy
state_.resultFileUrls which can be stale; update it to read the V2 field
state_.resultTakes[slotIndex].remoteFileUrl (use that as the primary source) and
only fall back to state_.resultFileUrls[static_cast<size_t>(slotIndex)] if the
take entry is empty/null. Locate schedulePreviewDownload and replace the
remoteFileUrl lookup, ensuring behavior remains compatible with
applyCompletedTask mirroring and the pendingPreviewDownloadSlot_ flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aac52be8-9075-4900-82f0-34c6f6729b60
📒 Files selected for processing (6)
plugins/acestep_vst3/src/PluginConfig.hplugins/acestep_vst3/src/PluginEnums.cppplugins/acestep_vst3/src/PluginEnums.hplugins/acestep_vst3/src/PluginProcessor.cppplugins/acestep_vst3/src/PluginState.cppplugins/acestep_vst3/src/PluginState.h
| if (state.transportState == TransportState::idle) | ||
| { | ||
| if (state.jobStatus == JobStatus::submitting) | ||
| { | ||
| state.transportState = TransportState::submitting; | ||
| } | ||
| else if (state.jobStatus == JobStatus::queuedOrRunning) | ||
| { | ||
| state.transportState = TransportState::rendering; | ||
| } | ||
| else if (state.jobStatus == JobStatus::succeeded) | ||
| { | ||
| state.transportState = TransportState::succeeded; | ||
| } | ||
| else if (state.jobStatus == JobStatus::failed) | ||
| { | ||
| state.transportState = TransportState::failed; | ||
| } | ||
| } |
There was a problem hiding this comment.
Migration logic may incorrectly override v2 states where transportState is legitimately idle.
This derivation runs whenever transportState == idle, not only for v1 states (where the transportState attribute is absent). If a v2 state has transportState=idle but jobStatus is non-idle (e.g., after a partial state manipulation), this logic will incorrectly override transportState.
Consider gating this migration on schemaVersion < 2 or checking whether the transportState attribute was actually absent in the XML.
🛡️ Suggested fix
importLegacyResultFields(state);
syncLegacyResultFields(state);
- if (state.transportState == TransportState::idle)
+ // Only derive transportState from jobStatus when migrating v1 state
+ if (state.schemaVersion < 2 && state.transportState == TransportState::idle)
{
if (state.jobStatus == JobStatus::submitting)
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/acestep_vst3/src/PluginState.cpp` around lines 179 - 197, The
migration currently remaps state.transportState whenever it equals
TransportState::idle, which can incorrectly override legitimate v2 idle states;
update the migration in PluginState.cpp to only perform this derivation for
legacy states by gating it on schemaVersion < 2 (or by checking that the
transportState XML attribute was actually missing during deserialization) before
inspecting state.jobStatus, and leave transportState unchanged for true v2
states; reference the transportState/jobStatus logic and the schemaVersion check
in the deserialization/migration area to implement this conditional gating.
ChuxiJ
force-pushed
the
codex/v2-906-workflow-model
branch
from
265cf39 to
8a9af8a
Compare
|
Superseded by consolidated V2 implementation PR #919, which now carries this stack forward against . Closing this split PR to keep the review surface clean. |
1 participant
Summary
Validation
Closes #906
Refs #903
Summary by CodeRabbit
New Features
Chores