feat: add composition project lane and session export workflow#916
Closed
ChuxiJ wants to merge 2 commits intocodex/v2-908-lora-comparefrom
Closed
feat: add composition project lane and session export workflow#916ChuxiJ wants to merge 2 commits intocodex/v2-908-lora-comparefrom
ChuxiJ wants to merge 2 commits intocodex/v2-908-lora-comparefrom
Conversation
This was referenced Mar 20, 2026
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExpanded ACEStep VST3 with a V2 UI: new themed look-and-feel, larger editor layout, modular components (StatusStrip, SynthPanel, TapeTransport, CompositionLane, ResultDeck, PreviewDeck), LoRA backend controls/requests, compare-slot workflow, session export, and extended persisted PluginState/result-take data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as Plugin Editor (UI)
participant FileChooser as File Chooser
participant Processor as Audio Processor
participant FileSystem as File System
User->>Editor: Click "Export Session"
Editor->>FileChooser: Open save dialog (*.txt)
FileChooser->>User: Show dialog
User->>FileChooser: Confirm path
FileChooser->>Editor: Return file
Editor->>Editor: persistTextFields()
Editor->>Processor: exportSessionSummary(file)
Processor->>Processor: Gather PluginState & resultTakes
Processor->>FileSystem: Overwrite/create file
Processor->>FileSystem: Write session summary
Processor-->>Editor: Return success
Editor->>Editor: refreshStatusViews() (update export status)
Editor->>User: Show export path/status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/acestep_vst3/src/PluginEditorPreview.cpp (1)
52-78: Consider guarding against concurrent FileChooser invocations.If the user clicks the export button while the FileChooser dialog is already open,
exportChooser_is reassigned on line 59, which destroys the in-flight dialog. While this matches the existing pattern inchoosePreviewFile(), consider disabling the button when a chooser is active or checkingif (exportChooser_ != nullptr) return;at the start.💡 Optional guard to prevent concurrent dialogs
void ACEStepVST3AudioProcessorEditor::chooseSessionExportFile() { + if (exportChooser_ != nullptr) + { + return; + } + const auto projectName = processor_.getState().projectName.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/acestep_vst3/src/PluginEditorPreview.cpp` around lines 52 - 78, chooseSessionExportFile currently reassigns exportChooser_ without guarding against an existing active dialog; add an early guard in ACEStepVST3AudioProcessorEditor::chooseSessionExportFile (e.g. if (exportChooser_ != nullptr) return;) or disable the export button while exportChooser_ is set so a concurrent invocation cannot replace the live FileChooser, and keep the existing exportChooser_.reset() in the async callback to allow reopening later; this mirrors the protection pattern used in choosePreviewFile and prevents destroying an in-flight dialog.plugins/acestep_vst3/src/PluginProcessor.cpp (1)
249-253: Consider checking write success afterwriteText.The method assumes
writeTextsucceeds afteropenedOk()returns true. While rare, disk-full or I/O errors during write could still occur. For robustness, consider checkingoutput.failedToOpen()or the stream status after writing.💡 Optional enhancement to check write status
output.writeText(summary, false, false, "\n"); output.flush(); + if (output.getStatus().failed()) + { + state_.errorMessage = "Failed to write session summary."; + return false; + } state_.lastExportPath = file.getFullPathName(); state_.errorMessage = {}; return true;🤖 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 249 - 253, After writing the summary with FileOutputStream::writeText, check the stream status to detect I/O errors (e.g., call output.failedToOpen() and/or check output.getStatus().failed()) before treating the operation as successful; on failure, set state_.errorMessage to an appropriate message, avoid updating state_.lastExportPath, close/flush the stream, and return false from the function (instead of proceeding to set state_.lastExportPath and returning true). Ensure you reference the existing symbols writeText, flush, failedToOpen / getStatus, state_.lastExportPath, state_.errorMessage, and file when making the change.
🤖 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/PluginProcessor.cpp`:
- Around line 241-247: Remove the premature file.deleteFile() call and rely on
juce::FileOutputStream's default truncation behavior when creating the stream
for "file"; create juce::FileOutputStream output(file) without deleting first,
then check output.openedOk(), set state_.errorMessage = "Could not open the
export destination." and return false if it failed (the existing openedOk()
check and state_ usage should remain unchanged), ensuring no file is deleted
before confirming write capability.
---
Nitpick comments:
In `@plugins/acestep_vst3/src/PluginEditorPreview.cpp`:
- Around line 52-78: chooseSessionExportFile currently reassigns exportChooser_
without guarding against an existing active dialog; add an early guard in
ACEStepVST3AudioProcessorEditor::chooseSessionExportFile (e.g. if
(exportChooser_ != nullptr) return;) or disable the export button while
exportChooser_ is set so a concurrent invocation cannot replace the live
FileChooser, and keep the existing exportChooser_.reset() in the async callback
to allow reopening later; this mirrors the protection pattern used in
choosePreviewFile and prevents destroying an in-flight dialog.
In `@plugins/acestep_vst3/src/PluginProcessor.cpp`:
- Around line 249-253: After writing the summary with
FileOutputStream::writeText, check the stream status to detect I/O errors (e.g.,
call output.failedToOpen() and/or check output.getStatus().failed()) before
treating the operation as successful; on failure, set state_.errorMessage to an
appropriate message, avoid updating state_.lastExportPath, close/flush the
stream, and return false from the function (instead of proceeding to set
state_.lastExportPath and returning true). Ensure you reference the existing
symbols writeText, flush, failedToOpen / getStatus, state_.lastExportPath,
state_.errorMessage, and file when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64eab96c-3019-4f67-b8be-e9b44e4f41fb
📒 Files selected for processing (8)
plugins/acestep_vst3/src/PluginEditor.cppplugins/acestep_vst3/src/PluginEditor.hplugins/acestep_vst3/src/PluginEditorPreview.cppplugins/acestep_vst3/src/PluginEditorState.cppplugins/acestep_vst3/src/PluginProcessor.cppplugins/acestep_vst3/src/PluginProcessor.hplugins/acestep_vst3/src/PluginState.cppplugins/acestep_vst3/src/PluginState.h
Comment on lines
+241
to
+247
| file.deleteFile(); | ||
| juce::FileOutputStream output(file); | ||
| if (!output.openedOk()) | ||
| { | ||
| state_.errorMessage = "Could not open the export destination."; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid deleting the file before confirming write capability.
Calling file.deleteFile() before opening the output stream creates a window where the original file is lost but the new write may fail. JUCE's FileOutputStream already truncates existing content by default, making the explicit delete unnecessary and risky.
🛡️ Proposed fix to remove premature deletion
- file.deleteFile();
juce::FileOutputStream output(file);
if (!output.openedOk())
{
state_.errorMessage = "Could not open the export destination.";
return false;
}
+ output.setPosition(0);
+ output.truncate();🤖 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 241 - 247, Remove
the premature file.deleteFile() call and rely on juce::FileOutputStream's
default truncation behavior when creating the stream for "file"; create
juce::FileOutputStream output(file) without deleting first, then check
output.openedOk(), set state_.errorMessage = "Could not open the export
destination." and return false if it failed (the existing openedOk() check and
state_ usage should remain unchanged), ensuring no file is deleted before
confirming write capability.
ChuxiJ
force-pushed
the
codex/v2-909-composition
branch
from
4733de4 to
cecb990
Compare
* fix: make the V2 VST3 editor vertically scrollable * refine: polish the V2 tape-synth interface hierarchy
Contributor
Author
|
Superseded by consolidated V2 implementation PR #919, which now carries this stack forward against . Closing this split PR to keep the review surface clean. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Closes #909
Refs #903
Summary by CodeRabbit
New Features
Improvements