Fix TUI control-plane convergence regressions - #217
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughIntroduces ChangesOffline/stale state handling across dashboard, notifications, and control
Sequence Diagram(s)sequenceDiagram
participant NotifScreen as Notifications
participant DashActivate as DashboardInteraction
participant DashControl as DashboardControl
participant MetadataServer as MetadataServer
participant HttpClient as HttpClient
rect rgba(100, 100, 200, 0.5)
Note over NotifScreen,HttpClient: Notification target open: offline returns 409
NotifScreen->>MetadataServer: POST /control/open-notification-target
MetadataServer->>MetadataServer: resolveControlFocusClientSession(clientTty)
MetadataServer->>MetadataServer: Check service/agent running state
alt Offline/exited
MetadataServer-->>NotifScreen: 409 (service is offline / agent is offline)
end
end
rect rgba(100, 200, 100, 0.5)
Note over NotifScreen,DashControl: Dashboard activation: offline session flow
NotifScreen->>DashActivate: activateDashboardEntry(offlineEntry)
DashActivate->>DashActivate: resumeOfflineSessionWithFeedback()
DashActivate->>DashActivate: refreshDashboardModelFromService(true)
DashActivate->>DashControl: waitAndOpenLiveTmuxWindowForEntry(entry, 60000ms)
DashControl->>HttpClient: requestJson with retry loop
HttpClient-->>DashControl: HttpTimeoutError (GET) → retry without restart
HttpClient-->>DashControl: ECONNREFUSED → removeEndpoint + restart
HttpClient-->>DashControl: 503 → restart before retry
DashControl-->>DashActivate: "opened" | "missing"
DashActivate-->>NotifScreen: DashboardActivationResult
alt activationSucceeded(result)
NotifScreen->>NotifScreen: settle() (mark read, refresh coordination)
else activationFailed
NotifScreen->>NotifScreen: failOpen() (skip settle)
end
end
rect rgba(200, 100, 100, 0.5)
Note over NotifScreen,DashControl: Pending action settlement: model reconciliation
NotifScreen->>NotifScreen: withMetadataSessionPending(settle)
Note over NotifScreen: settle() resolves
DashActivate->>DashActivate: scheduleDashboardModelReconcile()
DashActivate->>DashActivate: refreshDashboardModelFromService(true)
DashActivate->>DashActivate: renderDashboard() if applied
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Sub-agent review finding resolved in 9ff9c27: |
|
Sub-agent review finding resolved in 9ff9c27: Coordination reachability treated |
|
@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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/multiplexer/notifications.ts (1)
179-189:⚠️ Potential issue | 🟠 MajorAvoid
.catchon an optional call result.At line 187,
host.refreshCoordinationFromService?.().catch(...)can still throw when the method is absent, because.catchis accessed onundefined. Guard the method before chaining to keep failure recovery non-throwing.Suggested fix
const settle = async () => { if (!unread) return; try { await markCoordinationItemRead(host, item); await host.refreshCoordinationFromService?.(); } catch { host.footerFlash = "Notification update failed"; host.footerFlashTicks = 3; - await host.refreshCoordinationFromService?.().catch(() => {}); + if (host.refreshCoordinationFromService) { + await host.refreshCoordinationFromService().catch(() => {}); + } } };🤖 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/notifications.ts` around lines 179 - 189, The issue is in the settle function where optional chaining on host.refreshCoordinationFromService?.() is followed by .catch(), which throws when the method is absent because .catch is being called on undefined. Guard the optional call by either applying optional chaining to .catch as well using .catch?.() to safely handle the case when the method doesn't exist, or conditionally call the catch handler only when the method is present.
🤖 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.
Outside diff comments:
In `@src/multiplexer/notifications.ts`:
- Around line 179-189: The issue is in the settle function where optional
chaining on host.refreshCoordinationFromService?.() is followed by .catch(),
which throws when the method is absent because .catch is being called on
undefined. Guard the optional call by either applying optional chaining to
.catch as well using .catch?.() to safely handle the case when the method
doesn't exist, or conditionally call the catch handler only when the method is
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74d14a6a-0d18-46ec-a33e-95ceec13b785
📒 Files selected for processing (6)
src/coordination-model.test.tssrc/coordination-model.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/notifications.test.tssrc/multiplexer/notifications.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/multiplexer/notifications.test.ts
- src/multiplexer/dashboard-control.ts
- src/multiplexer/dashboard-control.test.ts
|
Sub-agent round 2 finding resolved in 7cc3ad1: non-dashboard activation fallback restored only |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Resolved the CodeRabbit summary finding in 62f4524: guarded the optional coordination refresh before attaching .catch, and added regression coverage for the missing-refresh wiring path. |
|
Resolved independent reviewer finding in ecf2ce3: mutating project-service requests now validate the cached endpoint pid/manifest through a capped /health check before POST, while GETs keep the no-preflight fast path. Added regression coverage for stale manifest recovery before mutation. |
1 participant
Summary
Verification
Summary by CodeRabbit