[codex] Fix confirm prompt default regression#78
[codex] Fix confirm prompt default regression#78Oranquelui wants to merge 4 commits intogaureshpai:mainfrom
Conversation
The Docker prompt's default value is not properly configured, causing it to display incorrectly when the user accepts the default. The default should be 'N' (No) and should be displayed/selected properly. Fixes gaureshpai#74
The confirm prompts now display '(default: Yes)' or '(default: No)' text in addition to the (Y/n) hint, making the default option much clearer. Before: ? Do you want to use TypeScript? (Y/n) After: ? Do you want to use TypeScript? (default: Yes) (Y/n) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Make displayMessage use strict boolean check (defaultVal === true) to match confirmHint behavior and prevent contradictory hints when non-boolean truthy values are passed as defaultVal.
|
Important Review skippedDraft detected. 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:
✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Hi @Oranquelui!
Thanks for submitting your first Pull Request!
We’re thrilled to have your contribution and will review it shortly.
gaureshpai
left a comment
gaureshpai
left a comment
There was a problem hiding this comment.
Also can we please re-render the line once an option is selected irrespective of prompt based input or default selection so that the (Y/n) would be vanished once an option will be selected
Rest LGTM! Good work
| const displayMessage = `${message}${defaultVal !== undefined && !hasDefaultSuffix ? ` (default: ${defaultDisplay})` : ""} `; | ||
|
|
||
| const confirmHint = defaultVal ? "(Y/n)" : "(y/N)"; | ||
| const confirmHint = defaultVal === true ? "(Y/n)" : "(y/N)"; |
There was a problem hiding this comment.
? Do you want to use Shadcn UI? (default: Yes) (Y/n)
? Do you want to add Docker support? (default: No) (y/N)
(Y/n) ✅
(y/N) ❌
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/test.js (1)
58-67: Make RESULT extraction less regex-greedy for future-proofing.
/RESULT=(\{.*\})/scan over-capture when additional brace-containing output appears. Parsing the explicitRESULT=line is safer.Proposed hardening
- const resultMatch = cleanOutput.match(/RESULT=(\{.*\})/s); - - if (!resultMatch) { + const resultLine = cleanOutput + .split(/\r?\n/) + .find((line) => line.startsWith("RESULT=")); + + if (!resultLine) { reject(new Error(`Prompt runner did not emit result.\nstdout:\n${cleanOutput}`)); return; } resolve({ output: cleanOutput, - result: JSON.parse(resultMatch[1]), + result: JSON.parse(resultLine.slice("RESULT=".length)), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/test.js` around lines 58 - 67, The RESULT extraction uses a greedy regex (resultMatch computed from cleanOutput) which can over-capture; change the match to target the explicit RESULT= line and use a non-greedy, multiline-safe pattern so only the JSON payload on that line is captured (e.g. anchor to the line start and stop at the closing brace or use a non-greedy capture across newlines). Update the code that builds resultMatch and keep the subsequent JSON.parse(resultMatch[1]) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/test.js`:
- Around line 58-67: The RESULT extraction uses a greedy regex (resultMatch
computed from cleanOutput) which can over-capture; change the match to target
the explicit RESULT= line and use a non-greedy, multiline-safe pattern so only
the JSON payload on that line is captured (e.g. anchor to the line start and
stop at the closing brace or use a non-greedy capture across newlines). Update
the code that builds resultMatch and keep the subsequent
JSON.parse(resultMatch[1]) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91f0899a-8d94-4078-bce4-ca164ca56889
📒 Files selected for processing (2)
src/lib/prompts.jssrc/test/test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Node 20.x - yarn
- GitHub Check: Node 22.x - npm
- GitHub Check: Node 20.x - npm
- GitHub Check: Node 20.x - pnpm
- GitHub Check: Node 22.x - yarn
- GitHub Check: Node 22.x - pnpm
🔇 Additional comments (4)
src/lib/prompts.js (2)
24-26: Default-label dedup logic is clean and targeted.This avoids repeated
(default: ...)rendering while still preserving explicit defaults embedded in the message.
28-37: Confirm default normalization is now type-safe.Using strict boolean checks for hint rendering and empty-input fallback ensures confirm answers resolve to booleans consistently.
src/test/test.js (2)
17-73: Subprocess-based prompt harness is a solid regression-testing approach.Running prompts in a separate Node process is a good fit for validating interactive rendering behavior end-to-end.
74-102: Great regression coverage for both confirm-default bugs.These two tests directly validate the duplicate-default rendering fix and the empty-answer boolean normalization behavior.
✅ CI/CD Test ResultsOverall Status: success |
3 participants
Summary
(default: ...)in confirm prompts when the message already includes a default suffixRoot Cause
The previous fix switched confirm rendering from
messagetodisplayMessage, but some callers already embed(default: Yes)in the prompt text. That caused duplicated default text in the rendered confirm line. The confirm handler also returned the rawdefaultvalue on empty input, which could leak string defaults instead of a boolean.Validation
COREPACK_ENABLE_AUTO_PIN=0 pnpm exec biome check src/lib/prompts.js src/test/test.jsCOREPACK_ENABLE_AUTO_PIN=0 pnpm testSummary by CodeRabbit
Release Notes
Bug Fixes
Tests