fix: prevent status update flooding from stderr#172
Conversation
|
@anchallll02 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. |
📝 WalkthroughWalkthrough
ChangesHelper Communication & Error Handling
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 68-76: The handler attached to helperProcess.stdout.on("data")
currently sets helperReady and calls updateStatus prematurely on the first
chunk; change it so helperReady = true and updateStatus({mode: "helper", reason:
"C# helper process connected."}) are executed only after you successfully parse
a complete helper JSON message (use your existing buffer accumulation and
JSON.parse inside a try/catch), i.e., move the helperReady/updateStatus block to
the branch that runs after a successful parse of a valid helper message/type,
and apply the same change to the other stdout handling block that mirrors this
logic (the other occurrence around the helper message parsing code).
- Line 13: The stderr throttling in audioBridge.js is advancing
`lastStatusUpdate` even when `updateStatus(...)` drops a duplicate status, which
can suppress a different message that follows and can also carry stale throttle
state across helper restarts. Update the logic around `lastStatusUpdate` and
`updateStatus` so the timestamp is only recorded after a status actually
changes, and reset that throttle state whenever the helper lifecycle is reset or
restarted. Use the `lastStatusUpdate` variable and the `updateStatus` flow as
the main points to adjust.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| reason: "Helper not started yet." | ||
| }; | ||
|
|
||
| let lastStatusUpdate = 0; // FIX: used for throttling stderr updates |
There was a problem hiding this comment.
Don't advance the stderr throttle window for dropped updates.
lastStatusUpdate is set before updateStatus(...), so a duplicate helper-error still refreshes the throttle bucket even though updateStatus short-circuits. That means a new, different stderr message that arrives right after can be hidden for up to 1s. Because this timestamp also survives helper restarts, the first error from a new process can be suppressed too. Only stamp the time after a status actually changes, and reset it when the helper lifecycle resets.
Proposed fix
function updateStatus(nextStatus) {
// FIX: prevent identical state spam
if (
helperStatus.mode === nextStatus.mode &&
helperStatus.reason === nextStatus.reason
) {
- return;
+ return false;
}
helperStatus = nextStatus;
onStatusChange(helperStatus);
+ return true;
} function start() {
+ lastStatusUpdate = 0;
const helperBinary = findHelperBinary(); // FIX: throttle UI updates to avoid flood (max 1 per 1000ms)
if (now - lastStatusUpdate < 1000) {
return;
}
-
- lastStatusUpdate = now;
-
- updateStatus({
+ if (updateStatus({
mode: "helper-error",
reason: [
"Audio helper error: ",
errorMessage || "Helper reported an error.",
"\n",
"Troubleshooting:",
"\n- Check if your audio device is in use by another app.",
"\n- Try restarting Paraline or your computer.",
"\n- If this continues, rebuild the helper binary."
].join("")
- });
+ })) {
+ lastStatusUpdate = now;
+ } function stop() {
if (helperProcess) {
helperProcess.kill();
helperProcess = null;
}
+ lastStatusUpdate = 0;Also applies to: 105-125
🤖 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 13, The stderr throttling in audioBridge.js is
advancing `lastStatusUpdate` even when `updateStatus(...)` drops a duplicate
status, which can suppress a different message that follows and can also carry
stale throttle state across helper restarts. Update the logic around
`lastStatusUpdate` and `updateStatus` so the timestamp is only recorded after a
status actually changes, and reset that throttle state whenever the helper
lifecycle is reset or restarted. Use the `lastStatusUpdate` variable and the
`updateStatus` flow as the main points to adjust.
| helperProcess.stdout.on("data", (chunk) => { | ||
| if (!helperReady) { | ||
| helperReady = true; | ||
| helperReady = true; | ||
|
|
||
| updateStatus({ | ||
| mode: "helper", | ||
| reason: "C# helper process connected." | ||
| }); | ||
| } |
There was a problem hiding this comment.
Delay the ready transition until a valid helper message is parsed.
Right now any first stdout chunk flips the bridge to "helper", even if that chunk is only partial JSON or malformed output. That can leave the UI showing a connected helper before the protocol has actually produced a valid message. Move the helperReady/updateStatus(...) block to after the first successfully parsed helper message instead of before buffering/parsing.
Proposed fix
helperProcess.stdout.on("data", (chunk) => {
- if (!helperReady) {
- helperReady = true;
-
- updateStatus({
- mode: "helper",
- reason: "C# helper process connected."
- });
- }
-
stdoutBuffer += chunk.toString();
const lines = stdoutBuffer.split(/\r?\n/);
stdoutBuffer = lines.pop() || "";
for (const line of lines) {
if (!line.trim()) continue;
try {
const message = JSON.parse(line);
+
+ if (!helperReady) {
+ helperReady = true;
+ updateStatus({
+ mode: "helper",
+ reason: "C# helper process connected."
+ });
+ }
if (message.type === "level" && typeof message.value === "number") {
sendLevel(message.value);
}
} catch (_error) {Also applies to: 80-95
🤖 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 68 - 76, The handler attached to
helperProcess.stdout.on("data") currently sets helperReady and calls
updateStatus prematurely on the first chunk; change it so helperReady = true and
updateStatus({mode: "helper", reason: "C# helper process connected."}) are
executed only after you successfully parse a complete helper JSON message (use
your existing buffer accumulation and JSON.parse inside a try/catch), i.e., move
the helperReady/updateStatus block to the branch that runs after a successful
parse of a valid helper message/type, and apply the same change to the other
stdout handling block that mirrors this logic (the other occurrence around the
helper message parsing code).
|
Hey @anchallll02! Saw your work on GSSoC 2026. We are building TermUI, a TypeScript terminal UI framework with React-style hooks and JSX, rendered entirely in the terminal. We have 74 unassigned GSSoC issues open across widgets, hooks, adapters, and CLI tooling. Your JavaScript background transfers directly. Karanjot, TermUI maintainer |
|
@anchallll02 Please fix the coderabbit review comments as suggested |
3 participants
Fix: Prevent status update flooding from stderr
Changes
Impact
Summary by CodeRabbit
Bug Fixes
Improvements