fix: prevent exit event from overwriting manual stop status#174
fix: prevent exit event from overwriting manual stop status#174durdana3105 wants to merge 3 commits into
Conversation
|
@durdana3105 is attempting to deploy a commit to the Dot_NotSam's projects Team on Vercel. A member of the Team first needs to authorize it. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a module-level ChangesIntentional Shutdown Tracking & NPM Scripts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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: 2
🧹 Nitpick comments (1)
audioBridge.js (1)
131-142: ⚡ Quick winConditional status update prevents misleading message.
The status update "Helper stopped." occurs even when
helperProcessis null (helper never started or already stopped). This could replace the initial "Helper not started yet." message with "Helper stopped.", which is misleading.Consider updating status only when a helper was actually running:
♻️ Proposed fix to make status update conditional
function stop() { + let wasRunning = false; if (helperProcess) { + wasRunning = true; intentionalShutdown = true; helperProcess.kill(); helperProcess = null; } - updateStatus({ - mode: "simulated", - reason: "Helper stopped." - }); + if (wasRunning) { + updateStatus({ + mode: "simulated", + reason: "Helper stopped." + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` around lines 131 - 142, The stop function unconditionally calls updateStatus("Helper stopped.") even when helperProcess is null; change stop() so the status update happens only when a helper was actually running: move or wrap the intentionalShutdown = true, helperProcess.kill(), helperProcess = null and the updateStatus({... mode:"simulated", reason:"Helper stopped." }) into the same conditional that checks helperProcess (i.e., only execute updateStatus when helperProcess was truthy), leaving the existing initial "Helper not started yet." status untouched when helperProcess is null; use the existing stop, helperProcess, intentionalShutdown, and updateStatus symbols to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audioBridge.js`:
- Around line 112-129: The exit handler leaves a stale helperProcess reference
after unexpected exits; inside the helperProcess.on("exit", ...) branch where
intentionalShutdown is false, set helperProcess = null (and any related state)
before calling updateStatus so later callers like stop() see no live process and
won't attempt helperProcess.kill(); ensure the intentionalShutdown path still
nulls helperProcess as before and keep updateStatus behavior unchanged.
- Line 6: The module-level boolean intentionalShutdown should be moved into the
createAudioBridge closure so each bridge instance has its own shutdown flag;
locate the top-level intentionalShutdown declaration and remove it, then add an
instance-scoped variable (e.g., let intentionalShutdown = false) inside
createAudioBridge and update uses inside stop() and any exit-event handlers in
that closure to reference the new instance variable instead of the global one so
multiple createAudioBridge instances don't share state.
---
Nitpick comments:
In `@audioBridge.js`:
- Around line 131-142: The stop function unconditionally calls
updateStatus("Helper stopped.") even when helperProcess is null; change stop()
so the status update happens only when a helper was actually running: move or
wrap the intentionalShutdown = true, helperProcess.kill(), helperProcess = null
and the updateStatus({... mode:"simulated", reason:"Helper stopped." }) into the
same conditional that checks helperProcess (i.e., only execute updateStatus when
helperProcess was truthy), leaving the existing initial "Helper not started
yet." status untouched when helperProcess is null; use the existing stop,
helperProcess, intentionalShutdown, and updateStatus symbols to locate and
modify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const fs = require("fs"); | ||
| const path = require("path"); | ||
|
|
||
| let intentionalShutdown = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider moving intentionalShutdown into the closure to avoid shared state.
The module-level flag creates a hidden dependency: if createAudioBridge is ever called multiple times, all instances would share this single flag. Calling stop() on instance A would suppress exit-event errors from instance B's helper.
Move the flag inside createAudioBridge so each instance tracks its own shutdown intent independently.
♻️ Proposed fix to move flag into closure
-let intentionalShutdown = false;
-
function createAudioBridge(sendLevel, onStatusChange = () => {}) {
+ let intentionalShutdown = false;
let helperProcess = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let intentionalShutdown = false; | |
| function createAudioBridge(sendLevel, onStatusChange = () => {}) { | |
| let intentionalShutdown = false; | |
| let helperProcess = null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audioBridge.js` at line 6, The module-level boolean intentionalShutdown
should be moved into the createAudioBridge closure so each bridge instance has
its own shutdown flag; locate the top-level intentionalShutdown declaration and
remove it, then add an instance-scoped variable (e.g., let intentionalShutdown =
false) inside createAudioBridge and update uses inside stop() and any exit-event
handlers in that closure to reference the new instance variable instead of the
global one so multiple createAudioBridge instances don't share state.
| helperProcess.on("exit", (code) => { | ||
| if (intentionalShutdown) { | ||
| intentionalShutdown = false; | ||
| return; | ||
| } | ||
|
|
||
| function stop() { | ||
| if (helperProcess) { | ||
| helperProcess.kill(); | ||
| helperProcess = null; | ||
| } | ||
| updateStatus({ | ||
| mode: "simulated", | ||
| reason: [ | ||
| `Audio helper stopped (exit code ${code}).`, | ||
| "\n", | ||
| "Troubleshooting:", | ||
| "\n- The audio capture process exited unexpectedly.", | ||
| "\n- Try restarting Paraline.", | ||
| "\n- If the problem persists, rebuild the helper binary." | ||
| ].join("") | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Reset helperProcess to null on unexpected exit.
When the helper exits unexpectedly, helperProcess still references the dead process. This causes issues if stop() is called afterward:
- Line 132 checks
if (helperProcess)→ true (stale reference) - Line 134 calls
helperProcess.kill()on an already-dead process - May throw errors or cause undefined behavior
The intentional-exit path (line 135) correctly nulls the reference. Apply the same cleanup for unexpected exits.
🐛 Proposed fix to reset helperProcess on unexpected exit
helperProcess.on("exit", (code) => {
if (intentionalShutdown) {
intentionalShutdown = false;
return;
}
+ helperProcess = null;
updateStatus({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audioBridge.js` around lines 112 - 129, The exit handler leaves a stale
helperProcess reference after unexpected exits; inside the
helperProcess.on("exit", ...) branch where intentionalShutdown is false, set
helperProcess = null (and any related state) before calling updateStatus so
later callers like stop() see no live process and won't attempt
helperProcess.kill(); ensure the intentionalShutdown path still nulls
helperProcess as before and keep updateStatus behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
audioBridge.js (2)
109-126:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Reset
helperProcessto null on unexpected exit.This concern was raised in a previous review and remains unaddressed. When the helper exits unexpectedly,
helperProcessstill references the dead process. Ifstop()is called afterward, line 129 checksif (helperProcess)→ true (stale reference), then line 131 callshelperProcess.kill()on an already-dead process, which may throw errors or cause undefined behavior.The intentional-exit path at line 132 correctly nulls the reference. Apply the same cleanup for unexpected exits.
🐛 Proposed fix to reset helperProcess on unexpected exit
helperProcess.on("exit", (code) => { if (intentionalShutdown) { intentionalShutdown = false; return; } + helperProcess = null; updateStatus({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` around lines 109 - 126, The handler for helperProcess.on("exit") must clear the stale helperProcess reference on unexpected exits: inside the exit callback (where you check intentionalShutdown) ensure that when intentionalShutdown is false you set helperProcess = null (same as the intentional-exit path) before calling updateStatus; this prevents stop() from seeing a dead process reference and attempting helperProcess.kill() later. Update the exit listener that currently references helperProcess and intentionalShutdown to explicitly null helperProcess on unexpected exit.
6-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
intentionalShutdowninto the closure to avoid shared state.This concern was raised in a previous review and remains unaddressed. The module-level flag creates a hidden dependency: if
createAudioBridgeis called multiple times, all instances share this single flag, so callingstop()on instance A would suppress exit-event errors from instance B's helper.♻️ Proposed fix to move flag into closure
-let intentionalShutdown = false; - function createAudioBridge(sendLevel, onStatusChange = () => {}) { + let intentionalShutdown = false; let helperProcess = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` at line 6, The module-level flag intentionalShutdown should be removed and declared inside the createAudioBridge closure so each bridge instance has its own flag; update createAudioBridge to declare let intentionalShutdown = false within its scope and change all references (e.g., in the stop() method and any helper exit/event handlers attached inside createAudioBridge) to use that closure variable instead of the removed module-level one, ensuring no other modules rely on the global variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audioBridge.js`:
- Line 92: The file has a syntax error: the event listener for
helperProcess.stdout.on("data", ...) is closed with "};" instead of the required
"});". Open the helperProcess.stdout.on("data", ...) callback (the function that
starts at the helperProcess.stdout.on("data", ...) call) and replace the
incorrect closing token "};" with the correct closing sequence "});" so the
on("data", ...) invocation properly closes the callback function and the method
call.
In `@package.json`:
- Around line 7-37: The package.json contains two "scripts" objects so the
second one overwrites the first; remove the duplicate (the second "scripts"
block) and keep a single "scripts" object that includes the intended entries
(e.g., "start","dev","landing:dev","landing:build","helper:info"/"helper:build"
or your renamed "audio:helper:info"/"build:helper",
"build","clean","pack:win","dist:win","lint","format") and ensure
"validate:helper" is only present if you intend to run it; update/merge any
renamed script keys (helper:build vs build:helper, helper:info vs
audio:helper:info, etc.) so there is no duplication and the desired scripts are
preserved.
---
Duplicate comments:
In `@audioBridge.js`:
- Around line 109-126: The handler for helperProcess.on("exit") must clear the
stale helperProcess reference on unexpected exits: inside the exit callback
(where you check intentionalShutdown) ensure that when intentionalShutdown is
false you set helperProcess = null (same as the intentional-exit path) before
calling updateStatus; this prevents stop() from seeing a dead process reference
and attempting helperProcess.kill() later. Update the exit listener that
currently references helperProcess and intentionalShutdown to explicitly null
helperProcess on unexpected exit.
- Line 6: The module-level flag intentionalShutdown should be removed and
declared inside the createAudioBridge closure so each bridge instance has its
own flag; update createAudioBridge to declare let intentionalShutdown = false
within its scope and change all references (e.g., in the stop() method and any
helper exit/event handlers attached inside createAudioBridge) to use that
closure variable instead of the removed module-level one, ensuring no other
modules rely on the global variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2b61293-625a-4507-b97a-a6e31f85c9f6
📒 Files selected for processing (2)
audioBridge.jspackage.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
audioBridge.js (2)
109-126:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Reset
helperProcessto null on unexpected exit.This concern was raised in a previous review and remains unaddressed. When the helper exits unexpectedly,
helperProcessstill references the dead process. Ifstop()is called afterward, line 129 checksif (helperProcess)→ true (stale reference), then line 131 callshelperProcess.kill()on an already-dead process, which may throw errors or cause undefined behavior.The intentional-exit path at line 132 correctly nulls the reference. Apply the same cleanup for unexpected exits.
🐛 Proposed fix to reset helperProcess on unexpected exit
helperProcess.on("exit", (code) => { if (intentionalShutdown) { intentionalShutdown = false; return; } + helperProcess = null; updateStatus({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` around lines 109 - 126, The handler for helperProcess.on("exit") must clear the stale helperProcess reference on unexpected exits: inside the exit callback (where you check intentionalShutdown) ensure that when intentionalShutdown is false you set helperProcess = null (same as the intentional-exit path) before calling updateStatus; this prevents stop() from seeing a dead process reference and attempting helperProcess.kill() later. Update the exit listener that currently references helperProcess and intentionalShutdown to explicitly null helperProcess on unexpected exit.
6-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
intentionalShutdowninto the closure to avoid shared state.This concern was raised in a previous review and remains unaddressed. The module-level flag creates a hidden dependency: if
createAudioBridgeis called multiple times, all instances share this single flag, so callingstop()on instance A would suppress exit-event errors from instance B's helper.♻️ Proposed fix to move flag into closure
-let intentionalShutdown = false; - function createAudioBridge(sendLevel, onStatusChange = () => {}) { + let intentionalShutdown = false; let helperProcess = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` at line 6, The module-level flag intentionalShutdown should be removed and declared inside the createAudioBridge closure so each bridge instance has its own flag; update createAudioBridge to declare let intentionalShutdown = false within its scope and change all references (e.g., in the stop() method and any helper exit/event handlers attached inside createAudioBridge) to use that closure variable instead of the removed module-level one, ensuring no other modules rely on the global variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audioBridge.js`:
- Line 92: The file has a syntax error: the event listener for
helperProcess.stdout.on("data", ...) is closed with "};" instead of the required
"});". Open the helperProcess.stdout.on("data", ...) callback (the function that
starts at the helperProcess.stdout.on("data", ...) call) and replace the
incorrect closing token "};" with the correct closing sequence "});" so the
on("data", ...) invocation properly closes the callback function and the method
call.
In `@package.json`:
- Around line 7-37: The package.json contains two "scripts" objects so the
second one overwrites the first; remove the duplicate (the second "scripts"
block) and keep a single "scripts" object that includes the intended entries
(e.g., "start","dev","landing:dev","landing:build","helper:info"/"helper:build"
or your renamed "audio:helper:info"/"build:helper",
"build","clean","pack:win","dist:win","lint","format") and ensure
"validate:helper" is only present if you intend to run it; update/merge any
renamed script keys (helper:build vs build:helper, helper:info vs
audio:helper:info, etc.) so there is no duplication and the desired scripts are
preserved.
---
Duplicate comments:
In `@audioBridge.js`:
- Around line 109-126: The handler for helperProcess.on("exit") must clear the
stale helperProcess reference on unexpected exits: inside the exit callback
(where you check intentionalShutdown) ensure that when intentionalShutdown is
false you set helperProcess = null (same as the intentional-exit path) before
calling updateStatus; this prevents stop() from seeing a dead process reference
and attempting helperProcess.kill() later. Update the exit listener that
currently references helperProcess and intentionalShutdown to explicitly null
helperProcess on unexpected exit.
- Line 6: The module-level flag intentionalShutdown should be removed and
declared inside the createAudioBridge closure so each bridge instance has its
own flag; update createAudioBridge to declare let intentionalShutdown = false
within its scope and change all references (e.g., in the stop() method and any
helper exit/event handlers attached inside createAudioBridge) to use that
closure variable instead of the removed module-level one, ensuring no other
modules rely on the global variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2b61293-625a-4507-b97a-a6e31f85c9f6
📒 Files selected for processing (2)
audioBridge.jspackage.json
🛑 Comments failed to post (2)
audioBridge.js (1)
92-92:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: Missing closing parenthesis.
Line 92 has
};but should be});to properly close thehelperProcess.stdout.on("data", ...)method call started at line 61. This will cause a parse error when the code runs.🐛 Proposed fix for syntax error
- }; + });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.});🧰 Tools
🪛 Biome (2.4.16)
[error] 92-92: expected
,but instead found}(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audioBridge.js` at line 92, The file has a syntax error: the event listener for helperProcess.stdout.on("data", ...) is closed with "};" instead of the required "});". Open the helperProcess.stdout.on("data", ...) callback (the function that starts at the helperProcess.stdout.on("data", ...) call) and replace the incorrect closing token "};" with the correct closing sequence "});" so the on("data", ...) invocation properly closes the callback function and the method call.package.json (1)
7-37:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Duplicate
scriptskey in JSON.Lines 7-25 define a
scriptssection, but lines 27-37 declarescriptsagain. In JSON, only one value per key is allowed—the second declaration overwrites the first. This means all intended script updates (renaming tohelper:*, addingbuild/clean/lint/format, removingvalidate:helper) are lost, and the old script names remain active.Remove the duplicate (old) scripts section at lines 27-37.
🐛 Proposed fix to remove duplicate scripts section
"format": "echo \"No formatter configured yet\"" }, - "scripts": { - "start": "electron .", - "dev": "electron .", - "landing:dev": "npm --prefix landing run dev", - "landing:build": "npm --prefix landing run build", - "audio:helper:info": "node -e \"console.log('Paraline is Windows-only. Build the helper with: dotnet build ./audio-helper/Paraline.AudioBridge.csproj')\"", - "build:helper": "dotnet publish .\\audio-helper\\Paraline.AudioBridge.csproj -c Release -r win-x64 --self-contained true -p:PublishSingleFile=true -p:IncludeNativeLibrariesForSelfExtract=true -o .\\build\\audio-helper", - "pack:win": "npm run build:helper && npm run validate:helper && electron-builder --dir --win nsis --x64", - "dist:win": "npm run build:helper && npm run validate:helper && electron-builder --win nsis --x64", - "validate:helper": "node scripts/validate-helper.js" - }, - "keywords": [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."scripts": { "start": "electron .", "dev": "electron .", "landing:dev": "npm --prefix landing run dev", "landing:build": "npm --prefix landing run build", "helper:info": "powershell -NoProfile -Command \"Write-Output 'Build the helper with: dotnet build .\\audio-helper\\Paraline.AudioBridge.csproj'\"", "helper:build": "dotnet publish .\\audio-helper\\Paraline.AudioBridge.csproj -c Release -r win-x64 --self-contained true -p:PublishSingleFile=true -p:IncludeNativeLibrariesForSelfExtract=true -o .\\build\\audio-helper", "build": "npm run helper:build", "clean": "powershell -NoProfile -Command \"if (Test-Path dist) { Remove-Item dist -Recurse -Force }; if (Test-Path build/audio-helper) { Remove-Item build/audio-helper -Recurse -Force }\"", "pack:win": "npm run helper:build && electron-builder --dir --win nsis --x64", "dist:win": "npm run helper:build && electron-builder --win nsis --x64", "lint": "echo \"No linter configured yet\"", "format": "echo \"No formatter configured yet\"" }, "keywords": [🧰 Tools
🪛 Biome (2.4.16)
[error] 7-7: The key scripts was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 7 - 37, The package.json contains two "scripts" objects so the second one overwrites the first; remove the duplicate (the second "scripts" block) and keep a single "scripts" object that includes the intended entries (e.g., "start","dev","landing:dev","landing:build","helper:info"/"helper:build" or your renamed "audio:helper:info"/"build:helper", "build","clean","pack:win","dist:win","lint","format") and ensure "validate:helper" is only present if you intend to run it; update/merge any renamed script keys (helper:build vs build:helper, helper:info vs audio:helper:info, etc.) so there is no duplication and the desired scripts are preserved.
SamXop123
left a comment
SamXop123
left a comment
There was a problem hiding this comment.
Thanks for the PR.
but we need to clean up a few issues before merging:
package.json: There is a duplicate"scripts"block. Please revert the changes here so it doesn't break JSON linters.audioBridge.js: DeclaringintentionalShutdownat the file-level leaks state between instances. You can implement this statelessly by simply checkingif (!helperProcess)at the beginning of theexitlistener instead, sincestop()sets it tonull.
Please apply these changes so we can merge this
SamXop123
added
gssoc26
2 participants
Related Issue
Fixes #133
Summary
This PR prevents the audio helper exit handler from overwriting status messages generated during intentional shutdowns.
Previously, when the helper was stopped manually, the process exit event immediately updated the status again, replacing the user-facing "Helper stopped." message with an exit-code message.
Changes Made
Benefits
Testing
Checklist
Summary by CodeRabbit
Bug Fixes
Chores