fix(onboard): display port override hint in dashboard port conflict error#2498
Open
makeittotop wants to merge 4 commits intoNVIDIA:mainfrom
Open
fix(onboard): display port override hint in dashboard port conflict error#2498makeittotop wants to merge 4 commits intoNVIDIA:mainfrom
makeittotop wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
…rror Signed-off-by: Abhishek Pareek <makeittotop@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe preflight port-conflict error now adds a dashboard-specific remediation hint when the blocked port equals Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 3346-3351: The suggested message is misleading because the
port-availability check still uses DASHBOARD_PORT; either update the hint text
or make preflight honor CHAT_UI_URL. Fix by parsing CHAT_UI_URL (extract
hostname:port) and setting the port variable used in the port-availability loop
(the same variable currently compared to DASHBOARD_PORT/port) before the loop
runs, or change the console.error lines to clarify that only
NEMOCLAW_DASHBOARD_PORT controls the port used by preflight while CHAT_UI_URL
only points the UI—adjust the code that uses port/DASHBOARD_PORT and the printed
guidance accordingly (look for the port variable, DASHBOARD_PORT constant and
the port-availability loop in onboard.ts).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1cb8eaee-79f1-4f8c-a5c7-dfb2bcb6be86
📒 Files selected for processing (1)
src/lib/onboard.ts
CHAT_UI_URL is not read until sandbox creation (step 6/8) and does not affect the DASHBOARD_PORT constant checked during preflight. Only NEMOCLAW_DASHBOARD_PORT overrides the preflight check. Remove the misleading alternative to avoid blocking users who follow it.
wscurran
added
bug
Contributor
✨ Thanks for submitting this pull request that proposes a way to fix a bug and enhance the UI by displaying port override hints in the dashboard port conflict error. This identifies a bug and proposes a change to improve the user experience by providing a clearer solution to the issue.Related open issues: Related open issues: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When preflight fails because port 18789 is already bound by an unrelated process, the error message now surfaces both env var overrides so users can pick an alternative port without having to stop the conflicting process. Previously the error only suggested killing the blocker, with no mention of the documented override mechanism.
Related Issue
Fixes #2497
Changes
src/lib/onboard.ts: after the port conflict detail line, print a(pick one)block withNEMOCLAW_DASHBOARD_PORT=<N>andCHAT_UI_URL=http://127.0.0.1:<N>alternatives — guarded byport === DASHBOARD_PORTso it only appears for dashboard port conflicts, not gateway port conflicts.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Notes:
npm test— 139/139 onboard tests pass. Two pre-existing failures inpreflight.test.tsare environment-specific (anncprocess is bound to port 18789 on the test machine, which is the exact scenario reported in [Onboard] Preflight fails on fresh install when port 18789 is held by an unrelated process; no port override available #2497).AI Disclosure
Signed-off-by: Abhishek Pareek makeittotop@users.noreply.github.com
Summary by CodeRabbit