Stabilize API-backed TUI connection#236
Merged
TraderSamwise merged 4 commits intoJun 23, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR hardens aimux service reliability across multiple subsystems: daemon startup gains a filesystem lock and readiness-verified spawn; dashboard-control endpoint health moves from boolean to a ChangesTmux Session Identity and Inline Metadata
Service Reliability and Runtime State Hardening
Sequence Diagram(s)sequenceDiagram
participant caller as ensureProjectService
participant sRPS as spawnReadyProjectService
participant wPSR as waitForProjectServiceReady
participant fs as metadata-api.json
participant health as ProjectService /health
caller->>sRPS: request new project-service spawn
sRPS->>sRPS: spawn child process (pid=N)
loop every PROJECT_SERVICE_READY_POLL_MS
wPSR->>fs: read metadata endpoint pid
fs-->>wPSR: pid or absent
end
wPSR->>health: GET /health (with PROJECT_SERVICE_HEALTH_TIMEOUT_MS)
health-->>wPSR: { pid, manifest, projectStateDir }
wPSR->>wPSR: validate pid match + manifest match
alt readiness confirmed
sRPS-->>caller: ProjectServiceState
else timeout or validation failure
sRPS->>sRPS: terminate child, remove stale state
sRPS-->>caller: throw error
end
sequenceDiagram
participant client as Dashboard Client
participant dc as dashboard-control requestProjectService
participant esfr as endpointStateForRequest
participant emcps as endpointMatchesCurrentProjectService
participant cp as ensureDashboardControlPlane
alt POST request
client->>dc: POST /some-route
dc->>esfr: check endpoint state (30s cache)
esfr->>emcps: GET /health
emcps-->>esfr: "current" | "stale" | "unknown"
alt stale + within deadline
esfr-->>dc: "stale"
dc->>cp: restart project service
dc->>dc: retry POST
else current
esfr-->>dc: "current"
dc->>dc: execute POST
dc->>dc: markProjectServiceEndpointCurrent
end
else GET request
client->>dc: GET /some-route
dc->>dc: direct route, no health preflight
dc->>dc: markProjectServiceEndpointCurrent on 2xx
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/multiplexer/dashboard-control.ts (1)
918-927: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t cache endpoint health after an unverified GET.
Line 926 marks any successful request as
"current", but GET routes now bypass the pid/projectStateDir/manifest check. A successful GET to a stale endpoint can therefore seeddashboardProjectServiceEndpointHealth, and the next POST will trust the cache at Lines 982-984 instead of runningendpointMatchesCurrentProjectService.Suggested fix
if (status >= 200 && status < 300 && json?.ok !== false) { - markProjectServiceEndpointCurrent(host, endpoint); return json; }Also applies to: 975-986
🤖 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/multiplexer/dashboard-control.ts` around lines 918 - 927, The markProjectServiceEndpointCurrent function is being called unconditionally after any successful HTTP response, but GET requests bypass the pid/projectStateDir/manifest verification checks. This allows stale endpoints to be marked as "current" based on unverified GET responses, causing subsequent POST requests to incorrectly trust the cached endpoint state instead of re-verifying with endpointMatchesCurrentProjectService. Only invoke markProjectServiceEndpointCurrent when the request has been fully verified (for POST requests), not after all successful responses. Additionally, review the related caching logic in the lines mentioned (975-986) to ensure similar protection is applied where dashboardProjectServiceEndpointHealth is accessed.
🧹 Nitpick comments (1)
src/metadata-server.test.ts (1)
205-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse condition-based waits instead of fixed sleeps to reduce CI flake.
The
20ms/250mssleeps can be timing-sensitive in busy environments. Prefervi.waitFor(...)on observable state (e.g.,getStatecall count / refreshedseq) to keep this deterministic.Refactor sketch
- await new Promise((resolve) => setTimeout(resolve, 20)); + await vi.waitFor(() => expect(getState).toHaveBeenCalledTimes(2));Also applies to: 241-241
🤖 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/metadata-server.test.ts` around lines 205 - 217, Replace the fixed setTimeout calls (250ms and 20ms) with vi.waitFor() conditions that wait for observable state changes instead of relying on timing. After server.notifyChange() is called, use vi.waitFor() to wait for getState to be called exactly 2 times before making the third fetch request. Similarly, replace the initial 250ms sleep with vi.waitFor() waiting for getState to be called exactly 1 time before fetching the cached state. This ensures the test waits for actual state changes rather than arbitrary delays, making it deterministic in CI environments.
🤖 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/daemon.ts`:
- Around line 229-231: The probeDefaultDaemon function calls requestJson without
specifying a timeoutMs parameter, which allows it to hang indefinitely if a
listener accepts the connection but never responds. Add a timeoutMs parameter to
the requestJson call within the probeDefaultDaemon function to ensure the health
probe times out appropriately. Also apply the same timeout fix to the additional
health probe call mentioned at lines 477-488 that has the same issue.
In `@src/metadata-server.ts`:
- Around line 1250-1257: The setTimeout callback for desktopStateRefreshTimer is
missing exception handling for the refreshDesktopStateCache() call. The current
try-finally block only ensures cleanup via the finally clause but does not catch
exceptions thrown by refreshDesktopStateCache(). Add a catch block after the try
block (before the finally) to catch and log any exceptions thrown during the
desktop state cache refresh, preventing them from escaping the timer callback
and crashing the process.
In `@src/multiplexer/dashboard-model.ts`:
- Around line 1029-1033: The paneDead property check at line 1031 is optional
and treating undefined as live can cause false positives in the
contradictory-empty rejection path. Replace the condition checking
entry?.target?.paneDead === true with a proper call to isWindowAlive function to
accurately determine if the window is alive. Return false when the window is not
alive, ensuring that undefined paneDead values do not incorrectly bypass the
guard logic that applies to the same check at lines 1090-1092.
---
Outside diff comments:
In `@src/multiplexer/dashboard-control.ts`:
- Around line 918-927: The markProjectServiceEndpointCurrent function is being
called unconditionally after any successful HTTP response, but GET requests
bypass the pid/projectStateDir/manifest verification checks. This allows stale
endpoints to be marked as "current" based on unverified GET responses, causing
subsequent POST requests to incorrectly trust the cached endpoint state instead
of re-verifying with endpointMatchesCurrentProjectService. Only invoke
markProjectServiceEndpointCurrent when the request has been fully verified (for
POST requests), not after all successful responses. Additionally, review the
related caching logic in the lines mentioned (975-986) to ensure similar
protection is applied where dashboardProjectServiceEndpointHealth is accessed.
---
Nitpick comments:
In `@src/metadata-server.test.ts`:
- Around line 205-217: Replace the fixed setTimeout calls (250ms and 20ms) with
vi.waitFor() conditions that wait for observable state changes instead of
relying on timing. After server.notifyChange() is called, use vi.waitFor() to
wait for getState to be called exactly 2 times before making the third fetch
request. Similarly, replace the initial 250ms sleep with vi.waitFor() waiting
for getState to be called exactly 1 time before fetching the cached state. This
ensures the test waits for actual state changes rather than arbitrary delays,
making it deterministic in CI environments.
🪄 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: 82835eed-0691-4653-8791-c09546e16ed8
📒 Files selected for processing (17)
src/builtin-metadata-watchers.test.tssrc/builtin-metadata-watchers.tssrc/daemon.test.tssrc/daemon.tssrc/main.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-model-service.test.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/project-scanner.test.tssrc/tmux/doctor.test.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metadata-server.ts (1)
687-687: 📐 Maintainability & Code Quality | 🟠 MajorInclude generated
dist/artifacts in the PR.The changes to
src/metadata-server.tsmodify runtime behavior (cache TTL and desktop state refresh logic). Per the coding guideline forsrc/**/*.ts, these changes requireyarn buildto update thedist/directory. The PR currently includes only the source file changes without the corresponding generated artifacts.🤖 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/metadata-server.ts` at line 687, The modifications to the DESKTOP_STATE_CACHE_TTL_MS constant and related runtime behavior in src/metadata-server.ts require regenerating the dist directory artifacts. Run yarn build to compile the TypeScript source files and generate the updated dist artifacts, then include these generated files in the PR commit to ensure the changes are properly reflected in the distributable code.Source: Coding guidelines
🤖 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/tmux/runtime-manager.ts`:
- Around line 794-807: Rebuild the session scan list after calling
repairLegacyProjectSessionNames() in runtime-manager.ts, because the current
sessionNames mapping/filtering still uses the stale allSessionNames snapshot.
Update this logic around getLegacyProjectSessionName(), listSessionNames(), and
repairLegacyProjectSessionNames() so the post-repair names are re-read before
filtering, ensuring renamed legacy client sessions are included immediately even
when the canonical host session already exists.
---
Outside diff comments:
In `@src/metadata-server.ts`:
- Line 687: The modifications to the DESKTOP_STATE_CACHE_TTL_MS constant and
related runtime behavior in src/metadata-server.ts require regenerating the dist
directory artifacts. Run yarn build to compile the TypeScript source files and
generate the updated dist artifacts, then include these generated files in the
PR commit to ensure the changes are properly reflected in the distributable
code.
🪄 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: 8de8dfb2-50ef-43e7-a969-39ada5ad0813
📒 Files selected for processing (8)
src/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/multiplexer/session-launch.test.ts
- src/multiplexer/session-launch.ts
- src/metadata-server.test.ts
- src/multiplexer/dashboard-control.ts
Owner
Author
|
Follow-up review fixes landed in
Validation: |
This was referenced Jun 26, 2026
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
1 participant
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
Verification
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Tests