Codex/exclude claude welcome page - #562
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesWelcome onboarding flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant InitializeHandler
participant WelcomeOnboarding
MCPClient->>InitializeHandler: send Initialize request
alt eligible client
InitializeHandler->>WelcomeOnboarding: handleWelcomePageOnboarding
else ineligible client
InitializeHandler->>WelcomeOnboarding: skipWelcomePageOnboarding
WelcomeOnboarding->>WelcomeOnboarding: set pendingWelcomeOnboarding to false
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 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 `@test/test-welcome-onboarding-legacy-config.js`:
- Around line 37-40: In the spawn call within the test setup, the piped stderr
stream is never consumed and can deadlock the child process. Update the stdio
configuration in the spawn invocation to ignore stderr or attach a drain
listener that continuously consumes it.
🪄 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: f410f852-68b3-4df1-adbc-54fc4a87c1bf
📒 Files selected for processing (4)
src/config-manager.tssrc/server.tssrc/utils/welcome-onboarding.tstest/test-welcome-onboarding-legacy-config.js
| const child = spawn('node', [DIST_INDEX], { | ||
| env: { ...process.env, HOME: this.home, USERPROFILE: this.home }, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Drain or ignore stderr to avoid a pipe-buffer deadlock.
stderr is piped but never consumed. If the spawned server writes more than the OS pipe buffer (~64KB) to stderr, it will block on the write and the test will hang until TIMEOUT_MS, producing a flaky timeout rather than a clean result. Either ignore stderr or drain it.
🔧 Proposed fix
const child = spawn('node', [DIST_INDEX], {
env: { ...process.env, HOME: this.home, USERPROFILE: this.home },
- stdio: ['pipe', 'pipe', 'pipe'],
+ stdio: ['pipe', 'pipe', 'ignore'],
});📝 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.
| const child = spawn('node', [DIST_INDEX], { | |
| env: { ...process.env, HOME: this.home, USERPROFILE: this.home }, | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); | |
| const child = spawn('node', [DIST_INDEX], { | |
| env: { ...process.env, HOME: this.home, USERPROFILE: this.home }, | |
| stdio: ['pipe', 'pipe', 'ignore'], | |
| }); |
🤖 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 `@test/test-welcome-onboarding-legacy-config.js` around lines 37 - 40, In the
spawn call within the test setup, the piped stderr stream is never consumed and
can deadlock the child process. Update the stdio configuration in the spawn
invocation to ignore stderr or attach a drain listener that continuously
consumes it.
… exclusions (#583) * feat(onboarding): flag-controlled welcome page kill switch and client exclusions Add two feature flags evaluated before the welcome-page A/B decision: - welcome_page_enabled: kill switch, defaults to true so older flag documents keep current behavior - welcome_page_excluded_clients: case-insensitive client-name list; matching clients have their pending onboarding consumed silently Extend the legacy-config regression test into scenario runner covering the disabled flag and case-insensitive exclusion paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(onboarding): fail open on malformed kill-switch values, pin skip-path test assertions Review fixes for #583: - welcome_page_enabled: only explicit false disables; absent or malformed values fail open so a flag-file typo cannot permanently consume pending onboarding for new installs. - Test scenarios also assert sawOnboardingPage stays unset, distinguishing genuine skip paths from the A/B control path that also consumes pending. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/test-welcome-onboarding-legacy-config.js (1)
47-47: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDrain or ignore
stderrto avoid a pipe-buffer deadlock.
stderris piped but never consumed. If the spawned server writes more than the OS pipe buffer (~64KB) tostderr, it will block on the write and the test will hang untilTIMEOUT_MS, producing a flaky timeout rather than a clean result. Either ignorestderror drain it.🔧 Proposed fix
- stdio: ['pipe', 'pipe', 'pipe'], + stdio: ['pipe', 'pipe', 'ignore'],🤖 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 `@test/test-welcome-onboarding-legacy-config.js` at line 47, Update the spawned server configuration in test-welcome-onboarding-legacy-config.js to avoid piping an unconsumed stderr stream: either configure stderr to be ignored or consume it continuously. Preserve the existing stdout and stdin behavior while ensuring stderr cannot fill its pipe and block the test.
🤖 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.
Duplicate comments:
In `@test/test-welcome-onboarding-legacy-config.js`:
- Line 47: Update the spawned server configuration in
test-welcome-onboarding-legacy-config.js to avoid piping an unconsumed stderr
stream: either configure stderr to be ignored or consume it continuously.
Preserve the existing stdout and stdin behavior while ensuring stderr cannot
fill its pipe and block the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 045c1491-bf41-440d-a8ab-a583b02fbd1f
📒 Files selected for processing (2)
src/utils/welcome-onboarding.tstest/test-welcome-onboarding-legacy-config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/welcome-onboarding.ts
1 participant
Summary by CodeRabbit