feat: add live watch mode for kdm health Add watch mode to health command#87
Merged
utkarsh232005 merged 3 commits intoMay 23, 2026
Merged
Conversation
Contributor
|
Note
|
| Layer / File(s) | Summary |
|---|---|
HealthOptions contract and data fetching helper src/commands/health.ts |
New HealthOptions interface with watch, interval, and signal. fetchHealthRows(target) centralizes Docker/Kubernetes fetches, maps health colors, and returns table-ready rows with warnings on fetch failures. |
Watch mode polling and signal lifecycle src/commands/health.ts |
showHealth(target, options) now supports watch mode: validates interval as positive integer, starts a polling loop that clears the terminal, logs a [KDM Health] timestamped header, re-fetches and re-renders rows, and supports early termination via options.signal with proper timer and listener cleanup. Single-run mode still uses a spinner then renders once. |
CLI command registration and options src/commands/health.ts |
registerHealthCommand adds --watch / -w and --interval / -i flags, creates an AbortController, maps SIGINT/SIGTERM to controller.abort() (and exits), passes the controller signal into showHealth, and removes handlers after completion. |
Watch mode test coverage src/__tests__/health.test.ts |
Tests added/updated: verify CLI options -w/-i are registered; invalid interval inputs (-1, 3.5, 3abc) produce “Invalid interval” errors; watch-mode test uses fake timers and spies on process.stdout.write to assert screen clearing and a second render after interval; a test calls showHealth with an AbortSignal and verifies the watch loop stops rendering after abort. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI Action
participant showHealth
participant fetchHealthRows
participant Timer as setInterval Loop
participant Terminal as Terminal/Table
participant SignalHandler as SIGINT/SIGTERM
CLI->>showHealth: showHealth(target, {watch: true, interval: "3"})
showHealth->>showHealth: Parse & validate interval
showHealth->>Timer: Start polling loop
Timer->>fetchHealthRows: Fetch current rows
fetchHealthRows-->>Timer: Return health rows
Timer->>Terminal: Clear screen & re-render table
Terminal-->>Timer: Display updated health
SignalHandler->>Timer: Stop interval on Ctrl+C
Timer->>showHealth: Cleanup
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- KDM-cli/kdm-cli#40: Prior health-related changes touching
src/commands/health.tsand tests that this watch-mode extension builds upon.
Poem
⏱️ Watch ticks, the table clears away,
Health rows bloom, then fade, then play,
Abort the watch when quiet's due—
Fresh snapshots served, in seconds few.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | The PR comprehensively implements all coding requirements from issue #44: watch/interval CLI flags, dynamic health polling with configurable intervals, terminal clearing/refresh, SIGINT/SIGTERM clean exit, and interval validation. |
| Out of Scope Changes check | ✅ Passed | All changes are directly scoped to implementing watch-mode functionality for the health command; no unrelated modifications detected. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Title Check | ✅ Passed | Title check skipped as CodeRabbit has written the PR title. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
📋 Issue Planner
Built with CodeRabbit's Coding Plans for faster development and fewer bugs.
View plan used: #44
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands and usage tips.
Member
Author
|
@Rishiraj-Pathak-27 review this |
utkarsh232005
changed the title
kdm healthkdm health @coderabbitai
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a live watch mode to kdm health with a configurable refresh interval, refactoring the existing one-shot logic into a reusable fetchHealthRows helper and adding tests for the new options.
Changes:
- Introduces
--watch/-wand--interval <number>/-iflags on thehealthcommand, with interval validation. - Implements a recursive
setTimeoutpoll loop that clears the screen, prints a header/timestamp, and re-renders the health table; intercepts SIGINT/SIGTERM (skipped whenNODE_ENV === 'test'). - Adds Vitest cases for option registration, invalid intervals, and fake-timer driven polling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/commands/health.ts | Adds HealthOptions, splits row-building into fetchHealthRows, and implements the watch loop, signal handling, and CLI flag wiring. |
| src/tests/health.test.ts | Adds tests verifying watch/interval option registration, invalid-interval handling, and periodic polling with fake timers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/health.ts (1)
50-88: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPerformance: Fetch containers and pods in parallel with
Promise.allSettled.Currently, Docker containers and Kubernetes pods are fetched sequentially. Since these are independent operations, fetching them in parallel would reduce latency, especially when one source is slow or timing out.
⚡ Proposed parallel fetch
const fetchHealthRows = async (target: string): Promise<(string | number)[][]> => { const rows: (string | number)[][] = []; + const fetchContainers = target === 'all' || target === 'containers'; + const fetchPods = target === 'all' || target === 'pods'; + + const [containerResult, podResult] = await Promise.allSettled([ + fetchContainers ? getRunningContainers() : Promise.resolve([]), + fetchPods ? getRunningPods() : Promise.resolve([]), + ]); - if (target === 'all' || target === 'containers') { - try { - const containers = await getRunningContainers(); + if (fetchContainers) { + if (containerResult.status === 'fulfilled') { + const containers = containerResult.value; rows.push( ...containers.map((container) => [ 'container', container.name, healthColor(container.state), container.status, ]), ); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); + } else { + const message = containerResult.reason instanceof Error + ? containerResult.reason.message + : String(containerResult.reason); logger.warn?.(`Docker unavailable: ${message}`); } } - if (target === 'all' || target === 'pods') { - try { - const pods = await getRunningPods(); + if (fetchPods) { + if (podResult.status === 'fulfilled') { + const pods = podResult.value; rows.push( ...pods.map((pod) => [ 'pod', pod.name, healthColor(pod.status), `namespace: ${pod.namespace}, restarts: ${pod.restarts}`, ]), ); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); + } else { + const message = podResult.reason instanceof Error + ? podResult.reason.message + : String(podResult.reason); logger.warn?.(`Kubernetes unavailable: ${message}`); } }🤖 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/commands/health.ts` around lines 50 - 88, fetchHealthRows currently awaits getRunningContainers and getRunningPods sequentially; run them in parallel using Promise.allSettled when target includes them so slow/failed sources don't block the other. Call Promise.allSettled on the relevant promises (getRunningContainers and getRunningPods), then for each settled result: on fulfilled map items to rows (as done now using healthColor, container.name/status and pod.name/status/namespace/restarts) and push to rows; on rejected extract the error message and call logger.warn with the same Docker/Kubernetes unavailable text. Preserve existing row shape and logging strings while removing the sequential awaits.
🤖 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/__tests__/health.test.ts`:
- Around line 211-246: Add a test that covers the watch-mode edge case where
both sources fail: mock getRunningContainers and getRunningPods to reject (e.g.,
new Error('Docker down') / 'K8s down'), use vi.useFakeTimers(), spy
process.stdout.write, call
program.parseAsync(['node','test','health','all','--watch','--interval','2'])
(or similar), advance timers as needed, then assert logger.warn was called with
messages containing 'Docker unavailable' and 'Kubernetes unavailable' and 'No
workloads found.' and assert tableUtils.renderTable was not called; finally
restore the stdout spy and reset timers/vi.useRealTimers(). Ensure you reference
the existing symbols getRunningContainers, getRunningPods, program.parseAsync,
tableUtils.renderTable, and logger.warn when adding this test.
- Around line 211-246: The test leaves the recursive watch polling timers
queued; after advancing timers and making assertions, explicitly clear pending
fake timers before restoring real timers to avoid test pollution — add
vi.clearAllTimers() (and if the module exposes a stop/cleanup function for the
watch/poll loop, call that, e.g., stopPolling() or health.stopWatch) after your
assertions and before vi.useRealTimers(); keep the writeSpy.mockRestore() as-is.
This targets the watch/poll behavior triggered by program.parseAsync(...) and
the poll() loop.
In `@src/commands/health.ts`:
- Around line 130-156: The watch-mode polling loop in showHealth (the inner
async function poll) never exposes a way to stop it, making tests unable to
cancel recursive setTimeout scheduling; modify showHealth to return a
cancel/cleanup function (or an object with cancel) when options.watch is true
that sets isRunning = false and clears the scheduled timer (clearTimeout(timer))
and any pending resources, and ensure poll references the same timer/isRunning
variables so calling the returned cleanup stops further scheduling; update tests
to call the returned cancel to terminate the loop.
- Around line 120-128: The SIGINT/SIGTERM handlers (sigintHandler) are left
attached and call process.exit(0), causing listener leaks and preventing
post-cleanup logic; update showHealth to (1) register handlers with process.once
or ensure they are removed in cleanup by calling process.removeListener for
'SIGINT' and 'SIGTERM' with sigintHandler, and (2) stop invoking process.exit(0)
unconditionally inside sigintHandler—instead call cleanup() and only exit from
the top-level caller when appropriate (or conditionally exit when NODE_ENV !==
'test') so tests and downstream teardown can run; reference sigintHandler and
cleanup when making the change.
---
Outside diff comments:
In `@src/commands/health.ts`:
- Around line 50-88: fetchHealthRows currently awaits getRunningContainers and
getRunningPods sequentially; run them in parallel using Promise.allSettled when
target includes them so slow/failed sources don't block the other. Call
Promise.allSettled on the relevant promises (getRunningContainers and
getRunningPods), then for each settled result: on fulfilled map items to rows
(as done now using healthColor, container.name/status and
pod.name/status/namespace/restarts) and push to rows; on rejected extract the
error message and call logger.warn with the same Docker/Kubernetes unavailable
text. Preserve existing row shape and logging strings while removing the
sequential awaits.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 341508bc-b8db-4d2a-94c3-621ebab33709
📒 Files selected for processing (2)
src/__tests__/health.test.tssrc/commands/health.ts
Comment on lines
+211
to
+246
| it('should poll health status periodically in watch mode', async () => { | ||
| vi.useFakeTimers(); | ||
| const writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); | ||
|
|
||
| vi.mocked(getRunningContainers).mockResolvedValue([ | ||
| { id: '1', name: 'web', image: 'nginx', state: 'running', status: 'Up 2 hours' }, | ||
| ]); | ||
| vi.mocked(getRunningPods).mockResolvedValue([]); | ||
|
|
||
| // Parse to run the watch mode command | ||
| await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '3']); | ||
|
|
||
| expect(tableUtils.renderTable).toHaveBeenCalledTimes(1); | ||
| expect(writeSpy).toHaveBeenCalledWith('\x1Bc'); | ||
|
|
||
| // Change mock status to verify the next iteration | ||
| vi.mocked(getRunningContainers).mockResolvedValue([ | ||
| { id: '1', name: 'web', image: 'nginx', state: 'exited', status: 'Exited' }, | ||
| ]); | ||
|
|
||
| // Advance timer by 3 seconds (3000ms) | ||
| await vi.advanceTimersByTimeAsync(3000); | ||
|
|
||
| expect(tableUtils.renderTable).toHaveBeenCalledTimes(2); | ||
| expect(tableUtils.renderTable).toHaveBeenLastCalledWith( | ||
| expect.objectContaining({ | ||
| rows: expect.arrayContaining([ | ||
| expect.arrayContaining(['container', 'web', expect.stringContaining('exited')]), | ||
| ]), | ||
| }), | ||
| ); | ||
|
|
||
| // Clean up | ||
| writeSpy.mockRestore(); | ||
| vi.useRealTimers(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Missing test coverage: both Docker and Kubernetes fail in watch mode.
You have good coverage for individual failures, but no test verifies watch mode behavior when both sources fail simultaneously. This edge case should show warnings for both and render no table (or an empty state).
🧪 Suggested additional test
it('should warn for both sources and render empty state in watch mode when both fail', async () => {
vi.useFakeTimers();
const writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true);
vi.mocked(getRunningContainers).mockRejectedValue(new Error('Docker down'));
vi.mocked(getRunningPods).mockRejectedValue(new Error('K8s down'));
await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '2']);
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Docker unavailable'));
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Kubernetes unavailable'));
expect(logger.warn).toHaveBeenCalledWith('No workloads found.');
expect(tableUtils.renderTable).not.toHaveBeenCalled();
writeSpy.mockRestore();
vi.clearAllTimers();
vi.useRealTimers();
});As per coding guidelines: "Verify that tests cover both happy paths and error conditions."
🤖 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/__tests__/health.test.ts` around lines 211 - 246, Add a test that covers
the watch-mode edge case where both sources fail: mock getRunningContainers and
getRunningPods to reject (e.g., new Error('Docker down') / 'K8s down'), use
vi.useFakeTimers(), spy process.stdout.write, call
program.parseAsync(['node','test','health','all','--watch','--interval','2'])
(or similar), advance timers as needed, then assert logger.warn was called with
messages containing 'Docker unavailable' and 'Kubernetes unavailable' and 'No
workloads found.' and assert tableUtils.renderTable was not called; finally
restore the stdout spy and reset timers/vi.useRealTimers(). Ensure you reference
the existing symbols getRunningContainers, getRunningPods, program.parseAsync,
tableUtils.renderTable, and logger.warn when adding this test.
Watch mode test doesn't stop the polling loop — timer continues after test.
The recursive setTimeout in poll() keeps scheduling new timers even after your assertions. While vi.useRealTimers() is called, any pending fake timers may still be queued. This can cause test pollution or unexpected behavior in subsequent tests.
Consider explicitly clearing all pending timers before restoring real timers:
🧹 Proposed cleanup
expect(tableUtils.renderTable).toHaveBeenLastCalledWith(
expect.objectContaining({
rows: expect.arrayContaining([
expect.arrayContaining(['container', 'web', expect.stringContaining('exited')]),
]),
}),
);
// Clean up
writeSpy.mockRestore();
+ vi.clearAllTimers();
vi.useRealTimers();
});🤖 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/__tests__/health.test.ts` around lines 211 - 246, The test leaves the
recursive watch polling timers queued; after advancing timers and making
assertions, explicitly clear pending fake timers before restoring real timers to
avoid test pollution — add vi.clearAllTimers() (and if the module exposes a
stop/cleanup function for the watch/poll loop, call that, e.g., stopPolling() or
health.stopWatch) after your assertions and before vi.useRealTimers(); keep the
writeSpy.mockRestore() as-is. This targets the watch/poll behavior triggered by
program.parseAsync(...) and the poll() loop.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
coderabbitai
Bot
changed the title
kdm health @coderabbitaikdm health Add watch mode to health command
Contributor
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/commands/health.ts (1)
50-88: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winParallelize container and pod fetching for
target='all'usingPromise.allSettled.When both containers and pods need to be fetched, these are independent I/O operations executed sequentially. In watch mode this runs repeatedly, doubling the latency on each poll cycle unnecessarily.
Using
Promise.allSettledallows parallel fetching while preserving the current graceful error handling for each source independently.⚡ Proposed refactor for parallel fetching
const fetchHealthRows = async (target: string): Promise<(string | number)[][]> => { const rows: (string | number)[][] = []; + const shouldFetchContainers = target === 'all' || target === 'containers'; + const shouldFetchPods = target === 'all' || target === 'pods'; - if (target === 'all' || target === 'containers') { - try { - const containers = await getRunningContainers(); - rows.push( - ...containers.map((container) => [ - 'container', - container.name, - healthColor(container.state), - container.status, - ]), - ); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - logger.warn?.(`Docker unavailable: ${message}`); - } - } + const [containerResult, podResult] = await Promise.allSettled([ + shouldFetchContainers ? getRunningContainers() : Promise.resolve([]), + shouldFetchPods ? getRunningPods() : Promise.resolve([]), + ]); - if (target === 'all' || target === 'pods') { - try { - const pods = await getRunningPods(); - rows.push( - ...pods.map((pod) => [ - 'pod', - pod.name, - healthColor(pod.status), - `namespace: ${pod.namespace}, restarts: ${pod.restarts}`, - ]), - ); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - logger.warn?.(`Kubernetes unavailable: ${message}`); - } + if (containerResult.status === 'fulfilled') { + rows.push( + ...containerResult.value.map((container) => [ + 'container', + container.name, + healthColor(container.state), + container.status, + ]), + ); + } else if (shouldFetchContainers) { + const message = containerResult.reason instanceof Error + ? containerResult.reason.message + : String(containerResult.reason); + logger.warn?.(`Docker unavailable: ${message}`); + } + + if (podResult.status === 'fulfilled') { + rows.push( + ...podResult.value.map((pod) => [ + 'pod', + pod.name, + healthColor(pod.status), + `namespace: ${pod.namespace}, restarts: ${pod.restarts}`, + ]), + ); + } else if (shouldFetchPods) { + const message = podResult.reason instanceof Error + ? podResult.reason.message + : String(podResult.reason); + logger.warn?.(`Kubernetes unavailable: ${message}`); } return rows; };🤖 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/commands/health.ts` around lines 50 - 88, In fetchHealthRows, parallelize independent I/O by calling getRunningContainers() and getRunningPods() concurrently with Promise.allSettled instead of awaiting them sequentially; when target is 'all' or includes each source, start both promises, await Promise.allSettled, then for each result inspect status and on fulfilled map and push rows (using the same mapping logic as the current containers.map/pods.map), and on rejected extract the error message (error instanceof Error ? error.message : String(error)) and call logger.warn? with the same Docker/Kubernetes-unavailable messages so each source retains its original graceful error handling; ensure you still only kick off the pod/container call when the corresponding target branch applies (i.e., start a promise only if target requires it) and keep the returned rows accumulation and healthColor usage unchanged.
🤖 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/commands/health.ts`:
- Around line 50-88: In fetchHealthRows, parallelize independent I/O by calling
getRunningContainers() and getRunningPods() concurrently with Promise.allSettled
instead of awaiting them sequentially; when target is 'all' or includes each
source, start both promises, await Promise.allSettled, then for each result
inspect status and on fulfilled map and push rows (using the same mapping logic
as the current containers.map/pods.map), and on rejected extract the error
message (error instanceof Error ? error.message : String(error)) and call
logger.warn? with the same Docker/Kubernetes-unavailable messages so each source
retains its original graceful error handling; ensure you still only kick off the
pod/container call when the corresponding target branch applies (i.e., start a
promise only if target requires it) and keep the returned rows accumulation and
healthColor usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b88d8f60-8de7-4b37-9dd1-97f49c07b58f
📒 Files selected for processing (1)
src/commands/health.ts
…ncellation, strict validation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/__tests__/health.test.ts (3)
221-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing test coverage: both Docker and Kubernetes fail in watch mode.
You have solid coverage for individual source failures, but no test verifies watch-mode behavior when both sources fail simultaneously. This edge case should log warnings for both and render no table (or an empty state).
🧪 Suggested additional test
it('should warn for both sources and render empty state in watch mode when both fail', async () => { vi.useFakeTimers(); const writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); vi.mocked(getRunningContainers).mockRejectedValue(new Error('Docker down')); vi.mocked(getRunningPods).mockRejectedValue(new Error('K8s down')); await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '2']); expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Docker unavailable')); expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Kubernetes unavailable')); expect(logger.warn).toHaveBeenCalledWith('No workloads found.'); expect(tableUtils.renderTable).not.toHaveBeenCalled(); writeSpy.mockRestore(); vi.clearAllTimers(); vi.useRealTimers(); });As per coding guidelines: "Verify that tests cover both happy paths and error conditions."
🤖 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/__tests__/health.test.ts` around lines 221 - 256, Add a new test that simulates both Docker and Kubernetes failing in watch mode by mocking getRunningContainers and getRunningPods to reject (e.g., new Error('Docker down') / 'K8s down'), then call program.parseAsync(['node','test','health','all','--watch','--interval','2']) and assert that logger.warn was called with messages indicating Docker and Kubernetes are unavailable and that a "No workloads found." warning was logged, and assert tableUtils.renderTable was not called; ensure you use vi.useFakeTimers(), spy on process.stdout.write, advance/clear timers and restore spies (matching the existing watch-mode test patterns).
221-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWatch mode test doesn't stop the polling loop — timer continues after test.
The recursive
setTimeoutin the watch loop keeps scheduling new timers even after your assertions complete. Pending fake timers remain queued and can pollute subsequent tests or cause unexpected behavior.🧹 Proposed cleanup
// Clean up writeSpy.mockRestore(); + vi.clearAllTimers(); vi.useRealTimers(); });🤖 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/__tests__/health.test.ts` around lines 221 - 256, The watch-mode test leaves the recursive setTimeout polling running; spy on global.setTimeout before calling program.parseAsync to capture any timer IDs created by the health watch loop, then clear those timers during cleanup (call clearTimeout on each captured ID), restore the setTimeout spy, and call vi.useRealTimers(); specifically, in this test wrap program.parseAsync(['node','test','health','all','--watch','--interval','3']) with a vi.spyOn(global, 'setTimeout') to collect returned timer IDs and ensure you call clearTimeout(timerId) for each and restore the spy after assertions so the polling loop is stopped and no fake timers remain.
258-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear pending timers before restoring real timers.
After aborting the watch loop, pending fake timers may still be queued. Explicitly clear them to ensure clean test isolation.
🧹 Proposed fix
// Clean up writeSpy.mockRestore(); + vi.clearAllTimers(); vi.useRealTimers(); });As per coding guidelines: "Ensure mocks are clean and shared correctly."
🤖 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/__tests__/health.test.ts` around lines 258 - 286, The test leaves fake timers pending before switching back to real timers; after aborting the AbortController and advancing timers, call vi.clearAllTimers() (or vi.clearAllMocks() if preferred) before vi.useRealTimers() and before restoring the stdout write spy to ensure no fake timers remain queued; update the test that calls showHealth('all', { watch: true, interval: '3', signal: controller.signal }) to clear timers with vi.clearAllTimers() right after await vi.advanceTimersByTimeAsync(3000) and before writeSpy.mockRestore() and vi.useRealTimers().
🤖 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/__tests__/health.test.ts`:
- Around line 204-219: The test calls program.parseAsync multiple times letting
logger.error mocks accumulate; add mock clearing between each invocation to
avoid cross-call pollution—either call jest.clearAllMocks() or specifically
logger.error.mockClear() after each await program.parseAsync(...) (references:
program.parseAsync and logger.error in the 'should reject non-integer and
malformed intervals strictly' test) so each assertion checks only the error
produced by the immediately preceding parseAsync.
---
Duplicate comments:
In `@src/__tests__/health.test.ts`:
- Around line 221-256: Add a new test that simulates both Docker and Kubernetes
failing in watch mode by mocking getRunningContainers and getRunningPods to
reject (e.g., new Error('Docker down') / 'K8s down'), then call
program.parseAsync(['node','test','health','all','--watch','--interval','2'])
and assert that logger.warn was called with messages indicating Docker and
Kubernetes are unavailable and that a "No workloads found." warning was logged,
and assert tableUtils.renderTable was not called; ensure you use
vi.useFakeTimers(), spy on process.stdout.write, advance/clear timers and
restore spies (matching the existing watch-mode test patterns).
- Around line 221-256: The watch-mode test leaves the recursive setTimeout
polling running; spy on global.setTimeout before calling program.parseAsync to
capture any timer IDs created by the health watch loop, then clear those timers
during cleanup (call clearTimeout on each captured ID), restore the setTimeout
spy, and call vi.useRealTimers(); specifically, in this test wrap
program.parseAsync(['node','test','health','all','--watch','--interval','3'])
with a vi.spyOn(global, 'setTimeout') to collect returned timer IDs and ensure
you call clearTimeout(timerId) for each and restore the spy after assertions so
the polling loop is stopped and no fake timers remain.
- Around line 258-286: The test leaves fake timers pending before switching back
to real timers; after aborting the AbortController and advancing timers, call
vi.clearAllTimers() (or vi.clearAllMocks() if preferred) before
vi.useRealTimers() and before restoring the stdout write spy to ensure no fake
timers remain queued; update the test that calls showHealth('all', { watch:
true, interval: '3', signal: controller.signal }) to clear timers with
vi.clearAllTimers() right after await vi.advanceTimersByTimeAsync(3000) and
before writeSpy.mockRestore() and vi.useRealTimers().
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8597044b-3c25-462b-ae28-71cc03159b12
📒 Files selected for processing (2)
src/__tests__/health.test.tssrc/commands/health.ts
Comment on lines
+204
to
+219
| it('should reject non-integer and malformed intervals strictly', async () => { | ||
| await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '-1']); | ||
| expect(logger.error).toHaveBeenLastCalledWith( | ||
| expect.stringContaining('Invalid interval'), | ||
| ); | ||
|
|
||
| await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '3.5']); | ||
| expect(logger.error).toHaveBeenLastCalledWith( | ||
| expect.stringContaining('Invalid interval'), | ||
| ); | ||
|
|
||
| await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '3abc']); | ||
| expect(logger.error).toHaveBeenLastCalledWith( | ||
| expect.stringContaining('Invalid interval'), | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Clear mocks between sequential parseAsync calls to prevent test pollution.
Running three parseAsync invocations back-to-back without clearing mocks causes logger.error calls to accumulate. While toHaveBeenLastCalledWith checks only the most recent call, the test is fragile: if any call fails to trigger an error, the assertion may pass using a stale call from a previous iteration.
♻️ Recommended fix: clear mocks between calls
it('should reject non-integer and malformed intervals strictly', async () => {
await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '-1']);
expect(logger.error).toHaveBeenLastCalledWith(
expect.stringContaining('Invalid interval'),
);
+ vi.clearAllMocks();
await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '3.5']);
expect(logger.error).toHaveBeenLastCalledWith(
expect.stringContaining('Invalid interval'),
);
+ vi.clearAllMocks();
await program.parseAsync(['node', 'test', 'health', 'all', '--watch', '--interval', '3abc']);
expect(logger.error).toHaveBeenLastCalledWith(
expect.stringContaining('Invalid interval'),
);
});As per coding guidelines: "Ensure mocks are clean and shared correctly."
🤖 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/__tests__/health.test.ts` around lines 204 - 219, The test calls
program.parseAsync multiple times letting logger.error mocks accumulate; add
mock clearing between each invocation to avoid cross-call pollution—either call
jest.clearAllMocks() or specifically logger.error.mockClear() after each await
program.parseAsync(...) (references: program.parseAsync and logger.error in the
'should reject non-integer and malformed intervals strictly' test) so each
assertion checks only the error produced by the immediately preceding
parseAsync.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Rishiraj-Pathak-27
approved these changes
May 23, 2026
Contributor
Rishiraj-Pathak-27
left a comment
Rishiraj-Pathak-27
left a comment
There was a problem hiding this comment.
I review this PR
The PR is heading in a good direction and the feature is genuinely useful for a CLI tool. The implementation appears functional, but there are still a few quality, maintainability, and robustness improvements needed before merge.
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
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.
Resolves #44
This PR adds a continuous watch mode to
kdm health:--watch/-wflag.--interval <seconds>/-iflag (defaults to 5s).setTimeoutwatch loop to fetch status dynamically.SIGINT/SIGTERMfor clean exit.Summary by CodeRabbit
New Features
Tests