fix(cli): suppress MSP garbage before CLI welcome message#613
Conversation
When the CLI tab opens, buffered MSP response frames (e.g. repeated MSP_BOXNAMES polls from the previous tab) were displayed as binary garbage because every incoming byte was written to the output display while CONFIGURATOR.cliValid was false. Fix: accumulate bytes silently until 'CLI' is found in the welcome message, then display only from 'Entering CLI Mode' onwards. MSP frames queued before the welcome message are discarded from the display without affecting serial communication. More visible after recent firmware PRs because more enabled features → more/larger MSP responses buffered during initial connection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIn ChangesCLI pre-entry output suppression and welcome rendering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/tabs/cli.js (1)
465-490: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate
cliValidtransition on confirmed welcome markerAt Line 467,
CONFIGURATOR.cliValidflips totruebefore confirmingEntering CLIexists. If a chunk contains"CLI"but not the full welcome marker, state transitions early, welcome rendering is skipped, and autocomplete may start from an invalid state.Proposed fix
if (!CONFIGURATOR.cliValid && validateText.indexOf('CLI') !== -1) { - GUI.log(i18n.getMessage('cliEnter')); - CONFIGURATOR.cliValid = true; - // Display only from 'Entering CLI Mode' onwards; MSP garbage before it is discarded - const cliMsgStart = validateText.indexOf('Entering CLI'); - if (cliMsgStart >= 0) { + // Display only from 'Entering CLI Mode' onwards; MSP garbage before it is discarded + const cliMsgStart = validateText.indexOf('Entering CLI'); + if (cliMsgStart >= 0) { + GUI.log(i18n.getMessage('cliEnter')); + CONFIGURATOR.cliValid = true; const welcomeText = validateText.substring(cliMsgStart); const lines = welcomeText.split(/\r?\n|\r/); for (let j = 0; j < lines.length; j++) { if (j < lines.length - 1) { writeLineToOutput(lines[j]); } else if (lines[j].length > 0) { writeToOutput(lines[j]); // prompt line — no trailing <br> } } - } - // begin output history with the prompt (last line of welcome message) - // this is to match the content of the history with what the user sees on this tab - const lastLine = validateText.split("\n").pop(); - this.outputHistory = lastLine; - validateText = ""; - - if (CliAutoComplete.isEnabled() && !CliAutoComplete.isBuilding()) { - // start building autoComplete - CliAutoComplete.builderStart(); + // begin output history with the prompt (last line of welcome message) + // this is to match the content of the history with what the user sees on this tab + const lastLine = welcomeText.split(/\r?\n|\r/).pop(); + this.outputHistory = lastLine; + validateText = ""; + + if (CliAutoComplete.isEnabled() && !CliAutoComplete.isBuilding()) { + // start building autoComplete + CliAutoComplete.builderStart(); + } } }🤖 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 `@src/js/tabs/cli.js` around lines 465 - 490, The `CONFIGURATOR.cliValid` flag is set to true immediately after checking for "CLI" in the validateText, but before confirming that the full "Entering CLI" welcome marker actually exists. Move the `CONFIGURATOR.cliValid = true;` assignment to execute only after the condition `if (cliMsgStart >= 0)` is confirmed true, ensuring the cliValid state transitions only when the complete welcome marker is actually found and processed.
🤖 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.
Outside diff comments:
In `@src/js/tabs/cli.js`:
- Around line 465-490: The `CONFIGURATOR.cliValid` flag is set to true
immediately after checking for "CLI" in the validateText, but before confirming
that the full "Entering CLI" welcome marker actually exists. Move the
`CONFIGURATOR.cliValid = true;` assignment to execute only after the condition
`if (cliMsgStart >= 0)` is confirmed true, ensuring the cliValid state
transitions only when the complete welcome marker is actually found and
processed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d38b52c-11d6-484b-ab3e-3a442e2e718b
📒 Files selected for processing (1)
src/js/tabs/cli.js
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: build (ubuntu-24.04, linux-x64)
- GitHub Check: build (macos-26-intel, macos-x86_64)
- GitHub Check: build (windows-2022, windows-x64, x64)
- GitHub Check: build (macos-26, macos-arm64)
- GitHub Check: build (windows-2022, windows-ia32, ia32)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/js/tabs/cli.js (1)
406-409: LGTM!
|
@coderabbitai AI generated comment Declining as proposed — the fix would introduce a regression. The firmware has two welcome strings depending on compile flags: #ifndef MINIMAL_CLI
cliPrintLine("\r\nEntering CLI Mode, type 'exit' to return, or 'help'");
#else
cliPrintLine("\r\nCLI"); // only "CLI" — no "Entering CLI"
#endifMoving `cliValid = true` inside `if (cliMsgStart >= 0)` (where `cliMsgStart = indexOf('Entering CLI')`) means any MINIMAL_CLI firmware would never trigger the condition — `cliValid` stays `false` permanently, autocomplete never starts, CLI is broken for that variant. The `indexOf('CLI')` gate is intentional and pre-existing (unchanged from the prior release). It correctly covers both firmware forms. The `indexOf('Entering CLI')` check added in this PR is display-only — it controls what text to render, not whether CLI mode is active. These are deliberately separate concerns. |
|
Tip For best results, initiate chat on the files or code changes.
(ᓀ‸ᓂ) ✏️ Learnings added
|
1 participant
AI Generated pull-request
Summary
CONFIGURATOR.cliValidwasfalsewas written directly to the CLI output display viawriteToOutput(currentChar). Buffered MSP response frames (e.g. repeatedMSP_BOXNAMESpolls from a previous tab) appeared as binary garbage characters before "Entering CLI Mode" on every CLI tab open.'CLI'is found in the welcome message, then display only from'Entering CLI Mode'onwards. MSP frames queued before the welcome message are discarded from the display without affecting serial communication or protocol state.cliValidtransitions totrue.Test plan
#prompt display correctlyexitand reconnect cycle works correctlySummary by CodeRabbit