Open
Conversation
Add fs.watch on manifest.json (300ms debounce) and status polling (3s interval) that broadcast events to connected WebSocket clients. Tracks previous agent statuses to emit granular agent:status change events. Gracefully handles in-flight writes and tmux unavailability. Closes #74
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIntroduces a new WebSocket watcher module that monitors manifest.json for changes using fs.watch with 300ms debounce and periodically polls agent statuses at 3-second intervals, broadcasting manifest:updated and agent:status events. Includes comprehensive test suite validating debounce behavior, status change detection, error handling, and cleanup mechanisms. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FSWatch as fs.watch
participant Watcher as startManifestWatcher
participant ManifestFile as manifest.json
participant Tmux as tmux
participant Broadcast as broadcast()
rect rgba(100, 150, 200, 0.5)
Note over User,Broadcast: File Change Flow
User->>FSWatch: Modify manifest.json
activate FSWatch
FSWatch->>Watcher: file change event
Watcher->>Watcher: Debounce (300ms default)
deactivate FSWatch
Watcher->>ManifestFile: Read manifest
ManifestFile-->>Watcher: manifest content
Watcher->>Broadcast: manifest:updated
Broadcast-->>User: Event broadcasted
end
rect rgba(150, 200, 100, 0.5)
Note over User,Broadcast: Status Polling Flow
User->>Watcher: Poll interval (3000ms default)
activate Watcher
Watcher->>ManifestFile: Read manifest
ManifestFile-->>Watcher: manifest content
Watcher->>Tmux: List session panes
Tmux-->>Watcher: Pane list
Watcher->>Watcher: Check agent status
alt Status Changed
Watcher->>Broadcast: agent:status
Broadcast-->>User: Event broadcasted
else No Change
Watcher->>Watcher: Skip broadcast
end
deactivate Watcher
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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: 3
🧹 Nitpick comments (1)
src/server/ws/watcher.test.ts (1)
245-270: Add a regression test forstop()during an in-flight poll.Line 246 validates timer cleanup, but not a poll already awaiting
checkAgentStatus(). Add one deferred-promise case to ensure no lateagent:statusemits after stop.🧪 Suggested test addition
describe('cleanup', () => { test('stop should clear all timers and close watcher', async () => { @@ }); + + test('given in-flight poll, should not broadcast after stop', async () => { + const agent = makeAgent({ id: 'ag-aaa11111', status: 'running' }); + const wt = makeWorktree({ id: 'wt-abc123', agents: { [agent.id]: agent } }); + const manifest = makeManifest({ worktrees: { [wt.id]: wt } }); + mockedReadManifest.mockResolvedValue(manifest); + + let resolveStatus: ((v: { status: 'idle' }) => void) | null = null; + mockedCheckAgentStatus.mockReturnValueOnce( + new Promise((resolve) => { + resolveStatus = resolve as (v: { status: 'idle' }) => void; + }), + ); + + const events: WsEvent[] = []; + const watcher = startManifestWatcher(PROJECT_ROOT, (e) => events.push(e), { + pollIntervalMs: 1000, + }); + + await vi.advanceTimersByTimeAsync(1000); // starts poll + watcher.stop(); + resolveStatus?.({ status: 'idle' }); + await vi.advanceTimersByTimeAsync(0); + + expect(events).toHaveLength(0); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/ws/watcher.test.ts` around lines 245 - 270, Add a regression test to ensure watcher.stop() prevents late emits from an in-flight poll: in the existing describe('cleanup') test block that calls startManifestWatcher, stub/mock the module/function that performs the poll (checkAgentStatus or the internal function invoked during polling) to return a deferred promise (resolve it after calling watcher.stop()), trigger the polling path (e.g., via triggerFsWatch or advancing timers to start the poll), call watcher.stop() while the promise is still pending, then resolve the deferred promise and advance timers—assert that no new events (especially an `agent:status` event) are pushed to the events array and that fsWatcher.close was still called; use existing helpers like makeManifest, mockedReadManifest, mockedFsWatch and inspect mockedFsWatch.mock.results to locate the watcher instance.
🤖 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/server/ws/watcher.ts`:
- Around line 82-86: The watcher currently assumes
listSessionPanes(manifest.sessionName) will reject when tmux is unavailable, but
tmux.ts swallows errors and returns an empty Map, causing false "gone"
transitions; update watcher.ts to detect tmux unavailability by catching and/or
inspecting the error type from listSessionPanes (use the PpgError hierarchy) and
specifically handle PpgError with code TMUX_NOT_FOUND (or any TMUX-related error
code) to return/skip the cycle; locate the call to listSessionPanes and the
paneMap handling and change the try/catch to examine the thrown error (or the
returned Map result) and treat PpgError.code === 'TMUX_NOT_FOUND' as the skip
case while preserving normal empty-Map behavior for legitimate empty sessions.
- Around line 57-64: The onManifestFileChange handler can start while a prior
poll is running and can still broadcast after stop(); fix by serializing polls
and preventing late emits: add a module-scoped boolean or Promise lock (e.g.,
currentPoll) and a stopped flag; in onManifestFileChange check and await
currentPoll (or set/clear the running flag) so only one poll runs at a time, and
before calling broadcast or mutating previousStatuses verify stopped is false;
in stop() set stopped=true and await currentPoll to finish (or clear the lock)
so no in-flight cycle can emit after shutdown, and keep the existing try/catch
around readManifest to swallow transient read errors.
- Around line 39-55: The current watcher uses manifestPath(projectRoot) (mPath)
directly which breaks when the file is atomically replaced; change the watcher
to watch the manifest directory (use path.dirname(mPath)) instead of the file
inode and filter events by path.basename(mPath) inside the callback, keeping the
existing debounce logic (debounceTimer, debounceMs) and stopped guard, and
retain watcher.on('error') and the try/catch around creating watcher; update any
references to fs.watch(mPath, ...) to fs.watch(dir, ...) with a filename check
before calling onManifestFileChange().
---
Nitpick comments:
In `@src/server/ws/watcher.test.ts`:
- Around line 245-270: Add a regression test to ensure watcher.stop() prevents
late emits from an in-flight poll: in the existing describe('cleanup') test
block that calls startManifestWatcher, stub/mock the module/function that
performs the poll (checkAgentStatus or the internal function invoked during
polling) to return a deferred promise (resolve it after calling watcher.stop()),
trigger the polling path (e.g., via triggerFsWatch or advancing timers to start
the poll), call watcher.stop() while the promise is still pending, then resolve
the deferred promise and advance timers—assert that no new events (especially an
`agent:status` event) are pushed to the events array and that fsWatcher.close
was still called; use existing helpers like makeManifest, mockedReadManifest,
mockedFsWatch and inspect mockedFsWatch.mock.results to locate the watcher
instance.
src/server/ws/watcher.ts
Outdated
Comment on lines
82
to
86
| try { | ||
| paneMap = await listSessionPanes(manifest.sessionName); | ||
| } catch { | ||
| return; // tmux unavailable — skip | ||
| } |
There was a problem hiding this comment.
tmux unavailable handling currently relies on a rejection path that may not happen.
Line 83 expects listSessionPanes() to reject, but src/core/tmux.ts (Lines 202-232) catches failures and returns an empty Map. That can convert tmux outages into synthetic gone transitions instead of skipping the cycle.
As per coding guidelines: Use PpgError hierarchy with typed error codes: TMUX_NOT_FOUND, NOT_GIT_REPO, NOT_INITIALIZED, MANIFEST_LOCK, WORKTREE_NOT_FOUND, AGENT_NOT_FOUND, MERGE_FAILED, INVALID_ARGS, AGENTS_RUNNING, WAIT_TIMEOUT, AGENTS_FAILED, NO_SESSION_ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ws/watcher.ts` around lines 82 - 86, The watcher currently assumes
listSessionPanes(manifest.sessionName) will reject when tmux is unavailable, but
tmux.ts swallows errors and returns an empty Map, causing false "gone"
transitions; update watcher.ts to detect tmux unavailability by catching and/or
inspecting the error type from listSessionPanes (use the PpgError hierarchy) and
specifically handle PpgError with code TMUX_NOT_FOUND (or any TMUX-related error
code) to return/skip the cycle; locate the call to listSessionPanes and the
paneMap handling and change the try/catch to examine the thrown error (or the
returned Map result) and treat PpgError.code === 'TMUX_NOT_FOUND' as the skip
case while preserving normal empty-Map behavior for legitimate empty sessions.
P1: Replace inline import() type with top-level import type for PaneInfo P1: Add multi-agent/multi-worktree test covering nested loop + worktreeId mapping P2: Replace WsEvent payload: unknown with discriminated union for type-safe consumers P2: Add directory watcher fallback when manifest.json doesn't exist at startup P2: Add test for agent removed between polls (stale entry handling) P3: Use Promise.all for parallel agent status checks (consistent with refreshAllAgentStatuses) P3: Add polling overlap guard to prevent duplicate events from slow polls P3: Fix cleanup test silent if-guard — assert watchResults.length > 0 P3: Move makeManifest factory to shared test-fixtures.ts P3: Add optional onError callback for error observability P3: Document manifest:updated vs agent:status event gap in JSDoc
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
src/server/ws/watcher.tswith dual-source manifest change detectionfs.watchon manifest.json with 300ms debounce → broadcastsmanifest:updatedeventscheckAgentStatus()with batch pane fetching → broadcastsagent:statusevents on changestop()methodCloses #74
Test plan
npm run typecheckpasses (no new errors)npm test— all 226 tests pass (8 new watcher tests)Summary by CodeRabbit
Release Notes