Skip to content

Keep TUI recovery on sidecar transport#283

Merged
TraderSamwise merged 3 commits into
masterfrom
chore/core-sidecar-next-7
Jul 3, 2026
Merged

Keep TUI recovery on sidecar transport#283
TraderSamwise merged 3 commits into
masterfrom
chore/core-sidecar-next-7

Conversation

@TraderSamwise

@TraderSamwise TraderSamwise commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • move dashboard control-plane recovery from the daemon-starting core command client to the pure sidecar transport
  • add boundary coverage so multiplexer code cannot import daemon-starting core command wrappers
  • tighten startup/SSE refresh lifecycle behavior to avoid no-op stale renders
  • make last-used pruning tests avoid repeated persistence cycles and use fast atomic writes for recency hints

Verification

  • yarn typecheck
  • yarn lint
  • yarn vitest run
  • yarn build

Summary by CodeRabbit

  • Bug Fixes
    • Improved dashboard refresh behavior so the view only updates when new data was actually applied.
    • Prevented extra refresh/retry work when service discovery fails, making recovery more stable.
    • Avoided duplicate dashboard event-stream starts during session setup and repair flows.
    • Tightened persistence and pruning of recent usage data to keep stored history within limits.
    • Added an extra safeguard for module boundaries to keep internal command wiring separated.

@vercel

vercel Bot commented Jul 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
app Ready Ready Preview, Comment Jul 3, 2026 10:49pm

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 40 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 83f615c8-c93d-456e-be1a-f044dda1fedc

📥 Commits

Reviewing files that changed from the base of the PR and between 4d9127b and d1dfaf6.

📒 Files selected for processing (3)
  • src/last-used.test.ts
  • src/last-used.ts
  • src/multiplexer/session-launch.ts
📝 Walkthrough

Walkthrough

Migrates dashboard-control core command invocations from requestCoreCommand to sendCoreCommand across implementation and tests, adds a lifecycle guard and refresh/render logic fixes in dashboard-api-client and project-event-stream, introduces idempotent event-stream startup in session-launch, switches last-used persistence to atomicWriteFast, and adds a module boundary test.

Changes

Core command transport migration

Layer / File(s) Summary
Switch dashboard-control to sendCoreCommand transport
src/multiplexer/dashboard-control.ts
Import and recovery logic now call sendCoreCommand from core-command-transport.js instead of requestCoreCommand for projectStop/projectEnsure.
Update dashboard-control tests for sendCoreCommand
src/multiplexer/dashboard-control.test.ts
Mocks, assertion helpers, and module mocking updated to use sendCoreCommand and ../core-command-transport.js.
Add module boundary test
src/core-sidecar-boundary.test.ts
New test scans multiplexer files and asserts none reference core-command-client.js.

Dashboard refresh lifecycle and event-stream fixes

Layer / File(s) Summary
Post-refresh lifecycle guard
src/multiplexer/dashboard-api-client.ts
Adds a lifecycle-current re-check after the async refresh, returning false if stale before further checks.
Event stream retry and refresh-applied logic
src/multiplexer/project-event-stream.ts
Removes recover() before backoff sleep on endpoint failure; changes render-skip condition to require at least one successful refresh with a non-false value.
Event stream tests
src/multiplexer/project-event-stream.test.ts
Adds a test for skipping render on no-applied refresh and updates endpoint-failure test to expect no resync/render.
Idempotent event stream startup
src/multiplexer/session-launch.ts
Introduces startProjectEventStreamOnce to prevent duplicate stream starts across the repair and already-primed paths.

Last-used persistence and pruning test

Layer / File(s) Summary
Switch persistence to atomicWriteFast
src/last-used.ts, src/last-used.test.ts
persistLastUsedState now writes pretty-printed JSON via atomicWriteFast; pruning test seeds 69 items via JSON write before a single markLastUsed call.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • TraderSamwise/aimux#234: Both PRs modify project-event-stream.ts refresh/render decision logic and retry behavior.
  • TraderSamwise/aimux#273: Both PRs relate to dashboard-control.ts core command routing for projectStop/projectEnsure.
  • TraderSamwise/aimux#232: Both PRs add post-async lifecycle gating to prevent stale dashboard refresh/render paths.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: keeping TUI recovery on the sidecar transport instead of the core command path.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/core-sidecar-next-7

Comment @coderabbitai help to get the list of available commands.

@TraderSamwise

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/last-used.ts (1)

115-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant mkdirSync call.

atomicWriteFast already creates the target directory internally: "mkdirSync(dirname(path), { recursive: true });" before writing. The explicit mkdirSync(dir, ...) on Line 117 is now dead weight.

♻️ Proposed cleanup
 function persistLastUsedState(projectRoot: string, state: LastUsedState): void {
-  const dir = getProjectStateDirFor(projectRoot);
-  mkdirSync(dir, { recursive: true });
   atomicWriteFast(getLastUsedPath(projectRoot), `${JSON.stringify(state, null, 2)}\n`);
 }
🤖 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/last-used.ts` around lines 115 - 118, Remove the redundant directory
creation in persistLastUsedState: atomicWriteFast already ensures the target
directory exists, so the explicit mkdirSync(dir, { recursive: true }) call is
unnecessary. Update persistLastUsedState to rely on atomicWriteFast alone and
keep the JSON write path unchanged.
src/last-used.test.ts (1)

83-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hand-rolled JSON fixture duplicates the production wire schema.

The test now manually reconstructs the entire LastUsedState on-disk shape (version, items, clients.*.recentIds/items/updatedAt, projectRecentIds) via a literal object instead of driving it through markLastUsed/persistLastUsedState. This is faster (fewer persistence cycles, matching the PR's stated goal), but it creates a second, parallel definition of the state schema that must be kept in sync by hand — if the shape in src/last-used.ts changes (e.g., a field rename or new required field), this fixture can silently drift and either mask a bug or start failing for reasons unrelated to pruning logic.

Consider extracting a small buildSeededLastUsedState(...) test helper co-located with (or exported for) the last-used tests, or seeding through a lower-level exported state constructor instead of a raw literal, so schema changes only need to be updated in one place.

🤖 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/last-used.test.ts` around lines 83 - 109, The test is duplicating the
persisted LastUsedState schema with a raw JSON literal, which can drift from
src/last-used.ts. Refactor this setup to use a shared test helper or lower-level
state builder (for example, a buildSeededLastUsedState helper near
markLastUsed/persistLastUsedState) so the seeded fixture is generated from the
same schema definition instead of manually reconstructing version, items,
clients, and projectRecentIds.
🤖 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.

Nitpick comments:
In `@src/last-used.test.ts`:
- Around line 83-109: The test is duplicating the persisted LastUsedState schema
with a raw JSON literal, which can drift from src/last-used.ts. Refactor this
setup to use a shared test helper or lower-level state builder (for example, a
buildSeededLastUsedState helper near markLastUsed/persistLastUsedState) so the
seeded fixture is generated from the same schema definition instead of manually
reconstructing version, items, clients, and projectRecentIds.

In `@src/last-used.ts`:
- Around line 115-118: Remove the redundant directory creation in
persistLastUsedState: atomicWriteFast already ensures the target directory
exists, so the explicit mkdirSync(dir, { recursive: true }) call is unnecessary.
Update persistLastUsedState to rely on atomicWriteFast alone and keep the JSON
write path unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 546aea03-1619-42be-a836-72699edec315

📥 Commits

Reviewing files that changed from the base of the PR and between c6609a6 and 4d9127b.

📒 Files selected for processing (9)
  • src/core-sidecar-boundary.test.ts
  • src/last-used.test.ts
  • src/last-used.ts
  • src/multiplexer/dashboard-api-client.ts
  • src/multiplexer/dashboard-control.test.ts
  • src/multiplexer/dashboard-control.ts
  • src/multiplexer/project-event-stream.test.ts
  • src/multiplexer/project-event-stream.ts
  • src/multiplexer/session-launch.ts

@TraderSamwise

Copy link
Copy Markdown
Owner Author

Sub-agent review findings resolved in e9088ad:

  • src/multiplexer/session-launch.ts: startup repair failure could leave SSE disabled until dashboard restart. Fixed by starting the project event stream once startup repair reaches a current terminal outcome, including error outcomes.
  • src/last-used.ts: fast atomic writes for last-used.json needed explicit intent. Documented last-used as a recoverable recency hint on hot TUI paths, where avoiding fsync latency is intentional.

@TraderSamwise

Copy link
Copy Markdown
Owner Author

CodeRabbit nitpicks resolved in d1dfaf6:

  • Removed redundant directory creation in persistLastUsedState; atomicWriteFast owns parent directory creation.
  • Moved the large last-used seed fixture into seededLastUsedState so the prune test setup is explicit and less brittle.

@TraderSamwise
TraderSamwise merged commit 4b7ae83 into master Jul 3, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant