feat(cli): add --watch flag to view command #685
Conversation
📝 WalkthroughWalkthroughAdds a --watch/-w option to the CLI and extends ViewCommand.execute to support watch mode with AbortSignal. Watch mode renders the dashboard to a string, refreshes every 2s only on content change, clears the screen between renders, and supports graceful shutdown via SIGINT/AbortController. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant ViewCommand
participant Terminal
User->>CLI: run `openspec view --watch`
CLI->>ViewCommand: execute(path, { watch: true, signal })
ViewCommand->>ViewCommand: build dashboard string (getDashboardOutput)
ViewCommand->>Terminal: write initial dashboard
loop every 2s
ViewCommand->>ViewCommand: rebuild dashboard string
alt content changed
ViewCommand->>Terminal: clear screen (ESC)
ViewCommand->>Terminal: write updated dashboard
else no change
Note over ViewCommand,Terminal: skip reprint
end
end
User->>CLI: Ctrl+C (SIGINT)
CLI->>ViewCommand: signal.abort()
ViewCommand->>Terminal: stop loop and exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Greptile OverviewGreptile SummaryThis PR adds a Key areas touched:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant CLI as src/cli/index.ts (commander)
participant V as ViewCommand
participant FS as File system
participant T as Task progress utils
U->>CLI: openspec view --watch
CLI->>V: execute(targetPath, {watch:true})
loop every 2s
V->>FS: read openspec/changes + openspec/specs
V->>T: getTaskProgressForChange(...)
V-->>V: getDashboardOutput()
alt output changed
V->>CLI: stdout.write(clear + output)
else unchanged
V-->>CLI: no write
end
end
U->>CLI: Ctrl+C (SIGINT)
CLI->>V: SIGINT handler
V->>V: clearInterval + exit(0)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/core/view.ts
Outdated
| process.on('SIGINT', () => { | ||
| clearInterval(interval); | ||
| console.log('\nExiting watch mode...'); | ||
| process.exit(0); | ||
| }); |
There was a problem hiding this comment.
SIGINT handler leaks
process.on('SIGINT', ...) is registered inside execute() every time watch mode runs, but it’s never removed. In a long-lived process (or if ViewCommand.execute() is invoked multiple times in the same Node process, e.g. tests/embedders), this accumulates listeners and can trigger MaxListenersExceededWarning / multiple exit handlers firing. Prefer process.once('SIGINT', ...) or remove the listener when clearing the interval.
| // Draft section should contain empty and no-tasks changes | ||
| expect(output).toContain('Draft Changes'); | ||
| expect(output).toContain('empty-change'); | ||
| expect(output).toContain('no-tasks-change'); | ||
| expect(allOutput).toContain('Draft Changes'); | ||
| expect(allOutput).toContain('empty-change'); | ||
| expect(allOutput).toContain('no-tasks-change'); |
There was a problem hiding this comment.
ANSI stripping mismatch
These assertions use allOutput (raw, potentially ANSI-colored) while the new lines array is stripAnsi’d. If chalk color output is enabled in the test environment, expect(allOutput).toContain('Draft Changes') / similar can fail due to escape codes splitting the plain text. Use the stripped version (lines.join('\n') or stripAnsi(allOutput)) consistently for substring assertions.
![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
🤖 Fix all issues with AI agents
In `@src/core/view.ts`:
- Around line 29-31: The catch in the update loop (where getDashboardOutput is
called) only logs to stderr so the stale dashboard remains visible; change the
catch in src/core/view.ts to clear or overwrite the painted dashboard and print
the error prominently to stdout (use the same screen-clearing helper used
elsewhere or write \u001b[2J\u001b[0;0H to stdout) and include the error message
styled with chalk; additionally add a simple retry/failure counter (e.g.,
failedUpdateCount) inside the updater function to exit or stop retrying after N
consecutive failures to avoid spamming stderr when getDashboardOutput repeatedly
throws.
🧹 Nitpick comments (6)
src/core/view.ts (5)
39-43: SIGINT listener is never removed and could leak in tests.
process.on('SIGINT', ...)registers a permanent listener. Ifexecuteis ever called more than once (e.g., in a test harness), listeners accumulate. Consider usingprocess.once('SIGINT', ...)or storing the handler and callingprocess.removeListeneralongsideclearInterval.Proposed fix
- process.on('SIGINT', () => { + const onSigint = () => { clearInterval(interval); console.log('\nExiting watch mode...'); process.exit(0); - }); + }; + process.once('SIGINT', onSigint);
46-46:await new Promise(() => {})blocks forever — intentional but fragile.This is the standard "keep-alive" pattern for CLI watch loops, but it means
execute()never returns in watch mode. The only exit path isprocess.exit(0)inside the SIGINT handler. This makes unit-testing watch mode very difficult (no way to programmatically stop it). Consider accepting anAbortSignalor returning the interval handle for testability.
214-215:totalChangesis computed but never used.This variable is dead code. Remove it or wire it into the summary output.
Proposed fix
- const totalChanges = - changesData.draft.length + changesData.active.length + changesData.completed.length;
228-231: No-opforEachon completed changes.This loop body is empty. Either remove it or implement the intended logic (e.g., accumulating task counts from completed changes into
totalTasks/completedTasks).Proposed fix — remove the no-op loop
- changesData.completed.forEach(() => { - // Completed changes count as 100% done (we don't know exact task count) - // This is a simplification - });
53-121: Inconsistent output assembly:appendvs directoutput +=.Lines 118–119 concatenate directly to
output(skippingappend), so they don't get the trailing\nthatappendadds. Meanwhile,getSummaryOutput(Line 65) is also concatenated directly. The mixing is easy to misread and could introduce subtle double-newline or missing-newline issues during future edits. Consider usingappendconsistently, or document the intent.test/core/view.test.ts (1)
9-128: No tests cover watch mode.The watch mode path (
options.watch = true) is untestable with the current design sinceexecutenever returns. Consider adding a note/TODO, or refactoringexecuteto accept anAbortSignalso watch mode can be stopped programmatically in tests.
- Fix SIGINT listener leak by using process.once - Improve error visibility in watch mode by clearing screen and showing errors in red - Remove dead code (unused totalChanges and empty loop) - Standardize string construction for consistent output - Add AbortSignal support and unit tests for watch mode
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/view.ts`:
- Line 27: The code currently clears the terminal including scrollback by
writing '\x1B[3J' in the process.stdout.write calls; update both occurrences in
src/core/view.ts (the process.stdout.write(...) calls that build the clear
sequence) to remove the '\x1B[3J' token so the sequence becomes only
'\x1B[2J\x1B[H' + output, thereby clearing the visible screen and repositioning
the cursor while preserving terminal scrollback.
🧹 Nitpick comments (3)
src/core/view.ts (3)
39-39: Async callback insetIntervalcan cause overlapping executions.
setInterval(update, 2000)fires every 2 s regardless of whether the previousupdate()(which isasync) has finished. IfgetDashboardOutputever takes longer than 2 s (e.g., large project, slow disk), two invocations will race and interleaveprocess.stdout.writecalls, producing garbled output.A simple fix is to use a recursive
setTimeoutinstead:Proposed fix
- const interval = setInterval(update, 2000); + let timeout: ReturnType<typeof setTimeout>; + const scheduleNext = () => { + timeout = setTimeout(async () => { + await update(); + scheduleNext(); + }, 2000); + }; + scheduleNext();Adjust cleanup references from
clearInterval(interval)toclearTimeout(timeout)accordingly.
60-69: Never-resolving promise on Line 68 keeps the function from ever returning.When no
AbortSignalis provided,await new Promise(() => {})hangs forever — the only escape isprocess.exit(0)inside the SIGINT handler. This is acceptable for CLI entry points but makes the method impossible to use programmatically or in integration tests without anAbortSignal.Consider documenting this contract explicitly (e.g., a JSDoc note that callers must pass
signalif they need the function to return), or provide a fallback that resolves when SIGINT fires instead of callingprocess.exit:Sketch
} else { - await new Promise(() => {}); + await new Promise<void>((resolve) => { + const onSigint = () => { + clearInterval(interval); + console.log('\nExiting watch mode...'); + resolve(); + }; + process.once('SIGINT', onSigint); + }); }This would unify the exit path and let the function return cleanly without
process.exit.
41-47:process.exit(0)incleanupis a hard exit that bypasses any callers awaitingexecute.When SIGINT fires (without an
AbortSignal), the cleanup handler callsprocess.exit(0), which immediately terminates the process. Anyfinallyblocks or post-executelogic in the caller will never run. This is related to the never-resolving promise on Line 68 — if the promise were made resolvable on SIGINT (as suggested above), theprocess.exithere becomes unnecessary and cleanup becomes more composable.
| if (output !== lastOutput) { | ||
| lastOutput = output; | ||
| // Clear screen, scrollback and move cursor to home | ||
| process.stdout.write('\x1B[2J\x1B[3J\x1B[H' + output); |
There was a problem hiding this comment.
\x1B[3J clears terminal scrollback history.
The PR description states the goal is to "preserve terminal scrolling" and avoid forcing scroll, but \x1B[3J explicitly erases the scrollback buffer. If the intent is only to clear the visible screen and reposition the cursor, drop the \x1B[3J sequence and keep only \x1B[2J\x1B[H. Same applies to Line 32.
🤖 Prompt for AI Agents
In `@src/core/view.ts` at line 27, The code currently clears the terminal
including scrollback by writing '\x1B[3J' in the process.stdout.write calls;
update both occurrences in src/core/view.ts (the process.stdout.write(...) calls
that build the clear sequence) to remove the '\x1B[3J' token so the sequence
becomes only '\x1B[2J\x1B[H' + output, thereby clearing the visible screen and
repositioning the cursor while preserving terminal scrollback.
src/core/view.ts
Outdated
| clearInterval(interval); | ||
| if (!options.signal?.aborted) { | ||
| console.log('\nExiting watch mode...'); | ||
| process.exit(0); |
There was a problem hiding this comment.
Calling process.exit(0) inside ViewCommand couples core logic to process lifecycle and can terminate host processes unexpectedly when reused outside the CLI entrypoint. Suggested outcome: make execute() return cleanly from cleanup (clear interval/remove listeners), and let src/cli/index.ts own process-exit behavior for SIGINT.
test/core/view.test.ts
Outdated
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
|
|
||
| // Verify initial output | ||
| const initialOutput = logOutput.join('\n'); // Note: ViewCommand uses process.stdout.write which we haven't mocked here fully for this test setup, |
There was a problem hiding this comment.
This test currently verifies AbortSignal resolution, but it does not verify watch-mode rendering behavior. initialOutput is never asserted and process.stdout.write is not mocked, so UI regressions can pass silently. Suggested outcome: mock/spy process.stdout.write, assert the initial render includes watch-change, and (optionally) assert a second render after mutating tasks.md.
| .command('view') | ||
| .description('Display an interactive dashboard of specs and changes') | ||
| .action(async () => { | ||
| .option('-w, --watch', 'Watch for changes and update real-time') |
There was a problem hiding this comment.
Adding --watch here is good, but it currently creates a docs/completions drift: view still has no flags in src/core/completions/command-registry.ts, and docs/cli.md does not mention watch mode. Suggested outcome: update command-registry + docs in the same PR so runtime behavior, generated completions, and docs stay in sync.
- Remove process.exit from ViewCommand, return Promise<void> instead - Move signal handling (SIGINT) to CLI entry point - Add AbortSignal support to ViewCommand.execute - Improve watch mode tests with stdout mocking - Add --watch flag to command registry and docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
This PR introduces a
--watch(-w) flag to theviewcommand, significantly improving the Developer Experience (DX) by allowing real-time monitoring of specifications and changes without manual refreshes.Problem
Currently, users must repeatedly execute
openspec viewto check the status of their specs or track progress on changes. This breaks flow and makes it difficult to visualize updates as they happen.Solution
I implemented a robust watch mode that refreshes the dashboard automatically. Key implementation details include:
viewcommand are strictly preserved.SIGINT(Ctrl+C) correctly, ensuring the cursor is restored and the process exits cleanly.Test Plan
test/core/view.test.tsto verify the watch logic and ensure the draft/active/completed sorting remains deterministic.npm run dev:cli -- view --watchSummary by CodeRabbit
New Features
Tests
Documentation