fix(cli): accurately display bound viewer port on splash screen#560
Conversation
- Expose viewerPort and viewerSkipped state in /agentmemory/livez endpoint. - Update CLI readiness check to poll until the viewer port is bound or explicitly skipped. - Prevents misleading default port (3113) display on splash screen when the viewer falls back to another port.
|
@Tanmay-008 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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)
📝 WalkthroughWalkthroughThe PR exposes the viewer server's bound port and skip state, returns them in health/livez endpoints, and updates the CLI to discover and cache the actual viewer port for URL construction and reachability checks. ChangesDynamic Viewer Port Discovery
Sequence DiagramsequenceDiagram
participant CLI
participant HealthLivez as /agentmemory/livez
participant Health as /agentmemory/health
participant ViewerServer as ViewerServer
ViewerServer->>ViewerServer: bind to port, set boundViewerPort or viewerSkipped
CLI->>HealthLivez: GET /agentmemory/livez
HealthLivez->>ViewerServer: read getBoundViewerPort / getViewerSkipped
HealthLivez->>CLI: respond with viewerPort and viewerSkipped
CLI->>CLI: discoverViewerPort() caches viewerPort
CLI->>CLI: getViewerUrl() uses discovered port
CLI->>ViewerServer: probe constructed viewer URL for reachability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 `@src/viewer/server.ts`:
- Around line 134-135: Reset exported viewer state at the top of
startViewerServer() by setting boundViewerPort = null and viewerSkipped = false,
and when binding succeeds assign boundViewerPort from the actual server socket
(e.g., use server.address().port or the equivalent actual bound port) instead of
currentPort; also ensure that on any bind failure or exception paths (the
branches that currently handle bind errors) you clear boundViewerPort (set it to
null) and leave viewerSkipped in the correct state so stale module-level state
cannot leak across in-process restarts.
🪄 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: 842ff891-efbd-4b84-b319-a2bb616fda44
📒 Files selected for processing (3)
src/cli.tssrc/triggers/api.tssrc/viewer/server.ts
|
Thanks @Tanmay-008 — exposing viewerPort + viewerSkipped on /livez is a clean way to make the splash truthful. Merged. Closes #521. |
…uard (#588) Three concerns surfaced when auditing PRs merged since v0.9.21: 1) Graph parser regex from #494 was correct for self-closing tags ONLY when they're the only entity in the document. With a self-closing entity followed by an explicit-close entity, the greedy `[^>]*` ate the trailing `/` of the self-close, the alternation fell through to the explicit-close branch, and `[\s\S]*?` ran ahead to the *next* `</entity>` — merging two entity declarations into one match and silently dropping a node. Switch to lazy `[^>]*?` so the self-closing alternation gets first chance. Test from #494 (which was failing on main but I didn't notice) now passes. 2) #386 shipped `${AGENTMEMORY_URL}` / `${AGENTMEMORY_SECRET}` placeholders in plugin/.mcp.json. Claude Code and Cursor expand those at MCP launch time; some hosts pass the literal string through. A truthy literal `"${AGENTMEMORY_URL}"` defeats the existing `|| DEFAULT_URL` fallback and would have the shim POST to `${AGENTMEMORY_URL}/agentmemory/...` (DNS failure). Add a defensive guard in src/mcp/rest-proxy.ts that strips any value of the form `${...}` so the fallback engages. Mirror in src/mcp/standalone.ts's mode-announce log line. 3) CHANGELOG entries for all PRs landed since v0.9.21 (#321, #325, #386, #454, #494, #526, #542, #560, #561, #562, #564, #567) plus the regex + env-guard hardening here. Validation: - npm test → 98 files, 1091 tests pass - npm pack → 142 files, fresh install clean, iii-sdk@0.11.2 pinned, plugin/ shipped with hooks/scripts/skills/opencode/ - New test file covers 8 placeholder cases (unset, empty, `${VAR}` literal, mismatched braces, real values with $, unclosed `${`, no-braces `$VAR`).
2 participants
Fixes #521
Summary by CodeRabbit