Conversation
Press P to pause/resume metric updates. Adds a pause button to the status bar, a blinking PAUSED indicator in the header, and disables the refresh rate selector while paused. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a global pause feature (P key + UI) that stops metrics polling, admin detection in Header, a new Navbar, StatusBar localStorage persistence for filters, useSystemMetrics API key + retry/backoff logic, and non-memoized filtering/sorting changes in ProcessTable. CSS adds navbar/header/pause UI styles. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as App (paused state)
participant StatusBar as StatusBar
participant Header as Header
participant Metrics as useSystemMetrics
participant API as Metrics API
User->>App: Press "P" key / Click pause button
App->>App: toggle paused state
App->>StatusBar: pass paused prop & onPauseToggle
App->>Header: pass paused prop
StatusBar-->>App: onPauseToggle callback (if clicked)
App->>Metrics: derive refreshRate (0 when paused)
Metrics->>Metrics: if refreshRate == 0 -> clear interval / skip scheduled fetches
Metrics->>API: (when not paused) fetch metrics (with optional Authorization)
API-->>Metrics: metrics response / error
Metrics->>Metrics: on error -> schedule retry with exponential backoff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (4)
src/App.tsx (1)
93-94: Minor: Consider extractingonPauseToggleto a stable callback.The inline arrow function
() => setPaused(prev => !prev)creates a new function reference on every render, which could cause unnecessary re-renders ofStatusBarif it's memoized.♻️ Optional: Use stable callback reference
+ const togglePause = useCallback(() => setPaused(prev => !prev), []); + // ... later in JSX <StatusBar filter={filter} onFilterChange={setFilter} refreshRate={refreshRate} onRefreshRateChange={setRefreshRate} paused={paused} - onPauseToggle={() => setPaused(prev => !prev)} + onPauseToggle={togglePause} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 93 - 94, Extract the inline toggle into a stable callback to avoid creating a new function each render: create a memoized handler (e.g., togglePause) using useCallback that calls setPaused(prev => !prev) and pass that handler to the StatusBar onPauseToggle prop instead of the inline arrow; reference the StatusBar prop onPauseToggle and the state setter setPaused in App.tsx when making this change.src/App.css (1)
495-506: Consider respecting reduced motion preferences.The blinking animation on the paused indicator is subtle, but users with vestibular disorders or motion sensitivity may prefer it disabled. Consider adding a
prefers-reduced-motionmedia query.♿ Optional: Respect reduced motion preferences
`@keyframes` blink { 0%, 100% { opacity: 1; } 50% { opacity: 0.4; } } + +@media (prefers-reduced-motion: reduce) { + .paused-indicator { + animation: none; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 495 - 506, The .paused-indicator animation currently always blinks via the `@keyframes` blink; update the CSS to respect users' reduced motion preferences by adding a prefers-reduced-motion media query that disables the blinking (e.g., set animation: none or remove animation for .paused-indicator) and keep the existing `@keyframes` blink for environments that allow motion; reference .paused-indicator and `@keyframes` blink when making the change.src/components/StatusBar.tsx (1)
47-53: Accessibility consideration for pause button.The pause button uses Unicode symbols (
▶and⏸) which may render inconsistently across platforms/browsers or be problematic for screen readers. Consider adding anaria-labelfor accessibility.Also, if
onPauseToggleisundefined, clicking the button does nothing silently. While this is safe, it may be worth guarding if the button shouldn't be rendered without a handler.♿ Suggested accessibility improvement
<button className={`pause-button ${paused ? 'paused' : ''}`} onClick={onPauseToggle} title={paused ? 'Resume monitoring (P)' : 'Pause monitoring (P)'} + aria-label={paused ? 'Resume monitoring' : 'Pause monitoring'} + aria-pressed={paused} > {paused ? '▶' : '⏸'} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/StatusBar.tsx` around lines 47 - 53, The pause button in StatusBar.tsx should be made accessible and robust: add a clear aria-label that changes with the paused state (e.g., aria-label={paused ? 'Resume monitoring' : 'Pause monitoring'}) so screen readers don't rely on Unicode glyphs, and guard the handler by either not rendering the button or rendering it disabled when onPauseToggle is undefined (e.g., check onPauseToggle before rendering or set disabled and aria-disabled) so clicking does not silently no-op; update references to the pause-button class and the paused prop accordingly.src/hooks/useSystemMetrics.ts (1)
35-35: Consider the loading state when paused before initial fetch.If the hook is called with
refreshRate === 0from the start (or transitions to paused before the first fetch completes), theloadingstate will remaintrueindefinitely sincefetchMetricsis never called andsetLoading(false)is only invoked withinfetchMetrics.In the current App.tsx implementation,
pauseddefaults tofalse, so this isn't immediately problematic. However, if the initial state or behavior changes in the future, this could cause the app to be stuck in a loading state.💡 Optional: Handle loading state explicitly when paused
useEffect(() => { - if (refreshRate === 0) return; // paused + if (refreshRate === 0) { + setLoading(false); // Not loading when intentionally paused + return; + } fetchMetrics(); const interval = setInterval(fetchMetrics, refreshRate); return () => clearInterval(interval); }, [fetchMetrics, refreshRate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useSystemMetrics.ts` at line 35, In useSystemMetrics, the early return when refreshRate === 0 can leave loading stuck true because fetchMetrics (which sets loading false) never runs; update the logic so that when you short-circuit due to refreshRate === 0 you explicitly clear the loading state (call setLoading(false) or otherwise mark loading complete) or ensure an initial fetch runs regardless of refreshRate; specifically modify the branch around "if (refreshRate === 0) return" to either invoke fetchMetrics() once or call setLoading(false) so that the loading state controlled in this hook (loading, setLoading) and the fetchMetrics function no longer remain stuck when paused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.css`:
- Around line 495-506: The .paused-indicator animation currently always blinks
via the `@keyframes` blink; update the CSS to respect users' reduced motion
preferences by adding a prefers-reduced-motion media query that disables the
blinking (e.g., set animation: none or remove animation for .paused-indicator)
and keep the existing `@keyframes` blink for environments that allow motion;
reference .paused-indicator and `@keyframes` blink when making the change.
In `@src/App.tsx`:
- Around line 93-94: Extract the inline toggle into a stable callback to avoid
creating a new function each render: create a memoized handler (e.g.,
togglePause) using useCallback that calls setPaused(prev => !prev) and pass that
handler to the StatusBar onPauseToggle prop instead of the inline arrow;
reference the StatusBar prop onPauseToggle and the state setter setPaused in
App.tsx when making this change.
In `@src/components/StatusBar.tsx`:
- Around line 47-53: The pause button in StatusBar.tsx should be made accessible
and robust: add a clear aria-label that changes with the paused state (e.g.,
aria-label={paused ? 'Resume monitoring' : 'Pause monitoring'}) so screen
readers don't rely on Unicode glyphs, and guard the handler by either not
rendering the button or rendering it disabled when onPauseToggle is undefined
(e.g., check onPauseToggle before rendering or set disabled and aria-disabled)
so clicking does not silently no-op; update references to the pause-button class
and the paused prop accordingly.
In `@src/hooks/useSystemMetrics.ts`:
- Line 35: In useSystemMetrics, the early return when refreshRate === 0 can
leave loading stuck true because fetchMetrics (which sets loading false) never
runs; update the logic so that when you short-circuit due to refreshRate === 0
you explicitly clear the loading state (call setLoading(false) or otherwise mark
loading complete) or ensure an initial fetch runs regardless of refreshRate;
specifically modify the branch around "if (refreshRate === 0) return" to either
invoke fetchMetrics() once or call setLoading(false) so that the loading state
controlled in this hook (loading, setLoading) and the fetchMetrics function no
longer remain stuck when paused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8ee7141-dac9-46da-8aa3-a7aeab95f0cf
📒 Files selected for processing (5)
src/App.csssrc/App.tsxsrc/components/Header.tsxsrc/components/StatusBar.tsxsrc/hooks/useSystemMetrics.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-18T05:07:00.671Z
Learnt from: jbingham17
Repo: coderabbit-demo/btop PR: 8
File: src/types.ts:32-32
Timestamp: 2026-02-18T05:07:00.671Z
Learning: In the coderabbit-demo/btop repository, avoid inline comments on TypeScript interface properties. Ensure property types are self-documenting through clear names and interface structure. If a property requires explanation, document it above the interface or with JSDoc comments for the property, not inline on the same line.
Applied to files:
src/hooks/useSystemMetrics.ts
🔇 Additional comments (2)
src/components/Header.tsx (1)
10-10: LGTM!Clean implementation of the paused indicator. The optional prop maintains backward compatibility, and the conditional rendering is straightforward. The placement in
header-rightprovides good visibility alongside load and task metrics.Also applies to: 26-26, 58-58
src/App.tsx (1)
14-27: Well-implemented pause state management.The implementation correctly:
- Guards against triggering pause while typing in inputs (Line 18)
- Handles both
pandPkeys (Line 19)- Uses
useCallbackfor stable event handler reference- Properly cleans up the event listener on unmount
|
@coderabbitai generate unit tests |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
@coderabbitai generate unit tests |
|
✅ Unit tests generated successfully! Generated 4 test files. The changes have been created in this pull request: View PR |
✅ Actions performedUnit Test Generation started... Unit tests generated successfully. |
✅ Actions performedUnit Test Generation started... |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/App.tsx (1)
18-23: Consider addingHTMLTextAreaElementto the input guard.The check correctly excludes
HTMLInputElementandHTMLSelectElement, but if a textarea is ever added to the UI, pressingPwhile typing there would toggle the pause state. This is a defensive improvement.🛡️ Suggested defensive improvement
const handleKeyDown = useCallback((e: KeyboardEvent) => { - if (e.target instanceof HTMLInputElement || e.target instanceof HTMLSelectElement) return; + if ( + e.target instanceof HTMLInputElement || + e.target instanceof HTMLSelectElement || + e.target instanceof HTMLTextAreaElement + ) return; if (e.key === 'p' || e.key === 'P') { setPaused(prev => !prev); } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 18 - 23, The keydown handler handleKeyDown currently ignores events from HTMLInputElement and HTMLSelectElement but not from textareas; update the guard to also return early when e.target is an instance of HTMLTextAreaElement so typing in a textarea doesn't toggle pause, i.e., add HTMLTextAreaElement to the input-check alongside HTMLInputElement and HTMLSelectElement before the e.key === 'p' check that calls setPaused.src/App.css (3)
573-593: Add:focus-visiblestate for keyboard accessibility.Similar to
.nav-item, the pause button needs a visible focus indicator for keyboard users.♿ Suggested focus styles
.pause-button:hover { border-color: var(--color-cyan); box-shadow: 0 0 10px rgba(34, 211, 238, 0.2); } + +.pause-button:focus-visible { + outline: 2px solid var(--color-cyan); + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 573 - 593, The pause button lacks a keyboard-visible focus state; add a .pause-button:focus-visible rule (and .pause-button.paused:focus-visible) that applies a clear focus indicator consistent with .nav-item (e.g., a visible outline or box-shadow using var(--color-cyan) for normal and var(--color-yellow) for paused) and ensure it does not rely solely on :hover so keyboard users see the same emphasis as mouse users.
169-195: Consider adding:focus-visiblestyles for keyboard accessibility.The
.nav-itembuttons have hover states but lack visible focus indicators for keyboard navigation. Users navigating with Tab won't see which item is focused.♿ Suggested focus styles
.nav-item:hover { background: var(--bg-tertiary); color: var(--color-text); border-color: var(--border-color); } + +.nav-item:focus-visible { + outline: 2px solid var(--color-cyan); + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 169 - 195, The .nav-item rules lack visible keyboard focus styling; add a :focus-visible selector for .nav-item to provide a clear high-contrast outline and (optionally) matching background/border changes for keyboard users—e.g. implement .nav-item:focus-visible to set outline (or box-shadow), outline-offset, and color/border adjustments similar to .nav-item:hover/.nav-item.active so focused items are clearly visible when tabbed to. Ensure the focus style does not rely on hover and uses accessible contrast values consistent with --color-cyan/--border-color variables.
595-606: Addprefers-reduced-motionsupport for the blinking animation.Users with vestibular disorders or motion sensitivity may find continuous blinking animations uncomfortable. The
prefers-reduced-motionmedia query allows respecting user preferences.♿ Suggested reduced-motion support
`@keyframes` blink { 0%, 100% { opacity: 1; } 50% { opacity: 0.4; } } + +@media (prefers-reduced-motion: reduce) { + .paused-indicator { + animation: none; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.css` around lines 595 - 606, The blinking animation on .paused-indicator and the `@keyframes` blink does not respect users' prefers-reduced-motion settings; update the CSS so the animation only runs when prefers-reduced-motion is not requested (e.g., wrap the animation assignment in `@media` (prefers-reduced-motion: no-preference) or conditionally apply animation there) and add an explicit override for prefers-reduced-motion: reduce to set .paused-indicator animation to none (or equivalent static styling) so the element remains visible without motion for users who opt out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.css`:
- Around line 573-593: The pause button lacks a keyboard-visible focus state;
add a .pause-button:focus-visible rule (and .pause-button.paused:focus-visible)
that applies a clear focus indicator consistent with .nav-item (e.g., a visible
outline or box-shadow using var(--color-cyan) for normal and var(--color-yellow)
for paused) and ensure it does not rely solely on :hover so keyboard users see
the same emphasis as mouse users.
- Around line 169-195: The .nav-item rules lack visible keyboard focus styling;
add a :focus-visible selector for .nav-item to provide a clear high-contrast
outline and (optionally) matching background/border changes for keyboard
users—e.g. implement .nav-item:focus-visible to set outline (or box-shadow),
outline-offset, and color/border adjustments similar to
.nav-item:hover/.nav-item.active so focused items are clearly visible when
tabbed to. Ensure the focus style does not rely on hover and uses accessible
contrast values consistent with --color-cyan/--border-color variables.
- Around line 595-606: The blinking animation on .paused-indicator and the
`@keyframes` blink does not respect users' prefers-reduced-motion settings; update
the CSS so the animation only runs when prefers-reduced-motion is not requested
(e.g., wrap the animation assignment in `@media` (prefers-reduced-motion:
no-preference) or conditionally apply animation there) and add an explicit
override for prefers-reduced-motion: reduce to set .paused-indicator animation
to none (or equivalent static styling) so the element remains visible without
motion for users who opt out.
In `@src/App.tsx`:
- Around line 18-23: The keydown handler handleKeyDown currently ignores events
from HTMLInputElement and HTMLSelectElement but not from textareas; update the
guard to also return early when e.target is an instance of HTMLTextAreaElement
so typing in a textarea doesn't toggle pause, i.e., add HTMLTextAreaElement to
the input-check alongside HTMLInputElement and HTMLSelectElement before the
e.key === 'p' check that calls setPaused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51a66f6a-b21b-49d7-ae65-657d04743122
📒 Files selected for processing (2)
src/App.csssrc/App.tsx
📜 Review details
🔇 Additional comments (4)
src/App.tsx (4)
1-2: LGTM!The imports are correctly updated to include the necessary React hooks (
useEffect,useCallback) for the keyboard event handling, and the newNavbarcomponent import is properly placed.
15-16: LGTM!Clean implementation of the pause state. Using
0as the refresh rate to signal paused state touseSystemMetricsis a reasonable approach that avoids adding a separatepausedparameter to the hook.
25-28: LGTM!The
useEffectcorrectly manages the global keydown listener lifecycle with proper cleanup. The dependency onhandleKeyDownensures the listener stays in sync if the callback ever changes.
90-97: LGTM!The
pausedstate andonPauseTogglecallback are correctly wired toStatusBar. While the inline arrow function creates a new reference on each render, the performance impact is negligible here sinceStatusBaris unlikely to be a heavily optimized component.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/ProcessTable.tsx (1)
23-74: Consider restoringuseMemofor filtering and sorting.The filtering and sorting logic now runs on every render, even when
processes,filter,sortField, andsortDirectionhaven't changed. For a system monitoring app with potentially hundreds of processes, this can cause unnecessary computation—especially when the user selects a row (updatingselectedPid) or when the app is paused and no data changes.♻️ Proposed refactor to restore memoization
-import { useState } from 'react'; +import { useState, useMemo } from 'react';Then wrap the filtering and sorting logic:
- // Filtering and sorting on every render - let filtered = processes; - - if (filter) { - const lowerFilter = filter.toLowerCase(); - filtered = processes.filter( - p => - p.command.toLowerCase().includes(lowerFilter) || - p.user.toLowerCase().includes(lowerFilter) || - p.pid.toString().includes(lowerFilter) - ); - } - - const filteredAndSortedProcesses = [...filtered].sort((a, b) => { + const filteredAndSortedProcesses = useMemo(() => { + let filtered = processes; + + if (filter) { + const lowerFilter = filter.toLowerCase(); + filtered = processes.filter( + p => + p.command.toLowerCase().includes(lowerFilter) || + p.user.toLowerCase().includes(lowerFilter) || + p.pid.toString().includes(lowerFilter) + ); + } + + return [...filtered].sort((a, b) => { let aVal: number | string; let bVal: number | string; // ... sorting logic unchanged ... - }); + }); + }, [processes, filter, sortField, sortDirection]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ProcessTable.tsx` around lines 23 - 74, Filtering and sorting are recomputed on every render causing wasted work; wrap the logic that produces filteredAndSortedProcesses in React's useMemo and depend on [processes, filter, sortField, sortDirection] so the computation only runs when inputs change; update the block that computes filtered, lowerFilter and the sort switch (the code that currently assigns filtered and computes filteredAndSortedProcesses) to be returned from a useMemo callback and keep existing variable names (filteredAndSortedProcesses, filter, sortField, sortDirection, processes) so selectors and consumers remain unchanged.src/components/Header.tsx (1)
69-69: Admin mode feature is not described in PR objectives.The commit message mentions "Add admin mode" but the PR description only covers pause/resume functionality. This appears to be scope creep. Consider splitting into a separate PR or updating the PR description to document this feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Header.tsx` at line 69, The PR introduces an "admin mode" UI change (the conditional span using adminMode in Header.tsx) that isn't described in the PR description; either remove or isolate this change from the current PR or update the PR description to document the new feature. Locate the conditional JSX reference to adminMode in the Header component (the line rendering <span ...>ADMIN</span>), and then either (A) move that change to a separate branch/PR focused on admin mode, or (B) keep it here but expand the PR description to explain the admin mode purpose, behavior, and any related files touched so reviewers can assess scope.
🤖 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/components/Header.tsx`:
- Around line 38-46: The useEffect in Header.tsx calls fetch('/api/auth/me')
which will always 404 because the server only exposes /api/metrics, /api/health
and /api/environment; either implement the missing auth endpoint on the server
(add a handler that returns { isAdmin: boolean } for GET /api/auth/me) or remove
the client-side code that references fetch('/api/auth/me')/useEffect and the
setAdminMode/adminMode logic until auth exists; locate the useEffect block in
Header.tsx (the fetch('/api/auth/me') call, useEffect, setAdminMode and
adminMode) and either add a corresponding server route that returns the expected
JSON or delete/guard the client code to avoid the dead fetch.
- Around line 40-45: The fetch to '/api/auth/me' in the Header component should
use an AbortController for cleanup and robustly handle non-JSON responses:
create an AbortController, pass its signal to fetch, and on cleanup call
controller.abort(); after fetch, if !res.ok throw or set admin false, and before
calling res.json() verify res.headers.get('content-type') includes
'application/json' (or wrap res.json() in try/catch) so a plain-text 404 doesn't
throw; catch all errors (including AbortError) and call setAdminMode(false) only
for non-abort failures or simply ensure aborts don't trigger state updates.
In `@src/components/StatusBar.tsx`:
- Around line 22-28: Remove unnecessary user-agent and timestamp persistence
from handleFilterChange: only persist the filter value to localStorage under the
btop_filter key and call onFilterChange(value); delete the localStorage.setItem
calls for btop_user_agent and btop_last_active to avoid collecting user-agent
and activity timestamp data (remove references to btop_user_agent and
btop_last_active in handleFilterChange and any related code that depends on
them).
In `@src/hooks/useSystemMetrics.ts`:
- Line 22: The retry logic in useSystemMetrics uses state (retryCount /
setRetryCount) causing stale closures and no cleanup; replace retryCount state
with a useRef (e.g., retryCountRef) and store the active timeout id in another
ref (e.g., retryTimeoutRef), update retryCountRef.current instead of
setRetryCount, schedule retries with setTimeout using the refs so fetchMetrics
closure is stable, and add cleanup in the effect/unmount to
clearTimeout(retryTimeoutRef.current) and to cancel retries when monitoring is
paused (check the paused flag before scheduling). Ensure fetchMetrics reads
retryCountRef.current and that all timeouts are cleared on pause/unmount to
prevent continued retries.
---
Nitpick comments:
In `@src/components/Header.tsx`:
- Line 69: The PR introduces an "admin mode" UI change (the conditional span
using adminMode in Header.tsx) that isn't described in the PR description;
either remove or isolate this change from the current PR or update the PR
description to document the new feature. Locate the conditional JSX reference to
adminMode in the Header component (the line rendering <span ...>ADMIN</span>),
and then either (A) move that change to a separate branch/PR focused on admin
mode, or (B) keep it here but expand the PR description to explain the admin
mode purpose, behavior, and any related files touched so reviewers can assess
scope.
In `@src/components/ProcessTable.tsx`:
- Around line 23-74: Filtering and sorting are recomputed on every render
causing wasted work; wrap the logic that produces filteredAndSortedProcesses in
React's useMemo and depend on [processes, filter, sortField, sortDirection] so
the computation only runs when inputs change; update the block that computes
filtered, lowerFilter and the sort switch (the code that currently assigns
filtered and computes filteredAndSortedProcesses) to be returned from a useMemo
callback and keep existing variable names (filteredAndSortedProcesses, filter,
sortField, sortDirection, processes) so selectors and consumers remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 666ae1bf-814e-4a38-854e-2e94a1952e94
📒 Files selected for processing (4)
src/components/Header.tsxsrc/components/ProcessTable.tsxsrc/components/StatusBar.tsxsrc/hooks/useSystemMetrics.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-18T05:07:00.671Z
Learnt from: jbingham17
Repo: coderabbit-demo/btop PR: 8
File: src/types.ts:32-32
Timestamp: 2026-02-18T05:07:00.671Z
Learning: In the coderabbit-demo/btop repository, avoid inline comments on TypeScript interface properties. Ensure property types are self-documenting through clear names and interface structure. If a property requires explanation, document it above the interface or with JSDoc comments for the property, not inline on the same line.
Applied to files:
src/hooks/useSystemMetrics.ts
🪛 ESLint
src/hooks/useSystemMetrics.ts
[error] 22-22: 'retryCount' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (6)
src/components/ProcessTable.tsx (1)
36-74: LGTM on sorting implementation!The sorting logic correctly handles both string and numeric fields with appropriate comparison methods (
localeComparefor strings, numeric subtraction for numbers). Creating a shallow copy before sorting avoids mutating the original array.src/hooks/useSystemMetrics.ts (2)
6-9: API key and retry constants look reasonable.The exponential backoff configuration (1s base, 30s max, 10 retries) is sensible for a monitoring tool. The API key is read from Vite environment variables correctly.
54-54: Pause mechanism is correctly wired.When
refreshRate === 0, the effect returns early without setting up the interval. The relevant code snippet fromsrc/App.tsx:16showsuseSystemMetrics(paused ? 0 : refreshRate), so the pause toggle properly stops polling.src/components/StatusBar.tsx (2)
55-61: Pause button implementation looks good.The button correctly:
- Toggles the
pausedclass for styling- Shows appropriate icons (▶/⏸) and tooltip text
- Calls
onPauseToggleon clickOne minor consideration:
onPauseToggleis optional (?), so clicking when undefined would be a no-op, which is fine.
48-48: Disabling refresh selector when paused is appropriate.This aligns with the PR requirements and prevents confusing UX where the user changes refresh rate while paused.
src/components/Header.tsx (1)
70-70: PAUSED indicator implementation is correct.The indicator renders conditionally when
pausedis truthy and uses the appropriate CSS class for styling (which exists in App.css per the AI summary).
| // Admin mode should be determined by server-side auth, not URL params | ||
| useEffect(() => { | ||
| fetch('/api/auth/me', { credentials: 'include' }) | ||
| .then(res => res.ok ? res.json() : null) | ||
| .then(data => { | ||
| if (data?.isAdmin) setAdminMode(true); | ||
| }) | ||
| .catch(() => setAdminMode(false)); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that /api/auth/me is not implemented in the server
echo "=== Searching for auth/me route in server code ==="
rg -n "auth/me" --type ts
echo ""
echo "=== All route definitions in server ==="
rg -n "url\.pathname ===" --type tsRepository: coderabbit-demo/btop
Length of output: 430
The /api/auth/me endpoint does not exist on the server.
The server only implements routes for /api/metrics, /api/health, and /api/environment (server/index.ts:271-290). This fetch will always receive a 404 response, so res.ok will be false and adminMode will never be set to true.
Either implement the /api/auth/me endpoint on the server, or remove this dead code until the auth feature is ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Header.tsx` around lines 38 - 46, The useEffect in Header.tsx
calls fetch('/api/auth/me') which will always 404 because the server only
exposes /api/metrics, /api/health and /api/environment; either implement the
missing auth endpoint on the server (add a handler that returns { isAdmin:
boolean } for GET /api/auth/me) or remove the client-side code that references
fetch('/api/auth/me')/useEffect and the setAdminMode/adminMode logic until auth
exists; locate the useEffect block in Header.tsx (the fetch('/api/auth/me')
call, useEffect, setAdminMode and adminMode) and either add a corresponding
server route that returns the expected JSON or delete/guard the client code to
avoid the dead fetch.
| fetch('/api/auth/me', { credentials: 'include' }) | ||
| .then(res => res.ok ? res.json() : null) | ||
| .then(data => { | ||
| if (data?.isAdmin) setAdminMode(true); | ||
| }) | ||
| .catch(() => setAdminMode(false)); |
There was a problem hiding this comment.
Missing AbortController and error handling for non-JSON response.
The fetch lacks:
- An AbortController to cancel on unmount
- Proper handling if
res.json()fails on non-JSON responses (404 returns plain text "Not Found")
Proposed fix with proper cleanup and error handling
useEffect(() => {
+ const controller = new AbortController();
- fetch('/api/auth/me', { credentials: 'include' })
+ fetch('/api/auth/me', { credentials: 'include', signal: controller.signal })
.then(res => res.ok ? res.json() : null)
.then(data => {
if (data?.isAdmin) setAdminMode(true);
})
- .catch(() => setAdminMode(false));
+ .catch(() => {});
+ return () => controller.abort();
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Header.tsx` around lines 40 - 45, The fetch to '/api/auth/me'
in the Header component should use an AbortController for cleanup and robustly
handle non-JSON responses: create an AbortController, pass its signal to fetch,
and on cleanup call controller.abort(); after fetch, if !res.ok throw or set
admin false, and before calling res.json() verify
res.headers.get('content-type') includes 'application/json' (or wrap res.json()
in try/catch) so a plain-text 404 doesn't throw; catch all errors (including
AbortError) and call setAdminMode(false) only for non-abort failures or simply
ensure aborts don't trigger state updates.
| // Store filter in localStorage for persistence | ||
| const handleFilterChange = (value: string) => { | ||
| localStorage.setItem('btop_filter', value); | ||
| localStorage.setItem('btop_last_active', new Date().toISOString()); | ||
| localStorage.setItem('btop_user_agent', navigator.userAgent); | ||
| onFilterChange(value); | ||
| }; |
There was a problem hiding this comment.
Storing user agent in localStorage is a privacy concern.
The btop_user_agent storage appears unrelated to filter persistence and could be considered user fingerprinting. Additionally, btop_last_active timestamps combined with user agent data may raise compliance concerns (GDPR/CCPA) if this data is ever synced to a server.
If the intent is simply to persist the filter value, only line 24 is needed.
Proposed fix: Remove unnecessary data collection
const handleFilterChange = (value: string) => {
localStorage.setItem('btop_filter', value);
- localStorage.setItem('btop_last_active', new Date().toISOString());
- localStorage.setItem('btop_user_agent', navigator.userAgent);
onFilterChange(value);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Store filter in localStorage for persistence | |
| const handleFilterChange = (value: string) => { | |
| localStorage.setItem('btop_filter', value); | |
| localStorage.setItem('btop_last_active', new Date().toISOString()); | |
| localStorage.setItem('btop_user_agent', navigator.userAgent); | |
| onFilterChange(value); | |
| }; | |
| const handleFilterChange = (value: string) => { | |
| localStorage.setItem('btop_filter', value); | |
| onFilterChange(value); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/StatusBar.tsx` around lines 22 - 28, Remove unnecessary
user-agent and timestamp persistence from handleFilterChange: only persist the
filter value to localStorage under the btop_filter key and call
onFilterChange(value); delete the localStorage.setItem calls for btop_user_agent
and btop_last_active to avoid collecting user-agent and activity timestamp data
(remove references to btop_user_agent and btop_last_active in handleFilterChange
and any related code that depends on them).
| const [metrics, setMetrics] = useState<SystemMetrics | null>(null); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const [retryCount, setRetryCount] = useState(0); |
There was a problem hiding this comment.
Retry logic has stale closure and cleanup issues.
Several problems with the retry implementation:
retryCountstate is written but never read (ESLint correctly flags this).- The
setTimeout(fetchMetrics, delay)insidesetRetryCountcapturesfetchMetricsfrom outer scope, but the timeout isn't cleared on unmount or when pausing — retries will continue even after the component unmounts or the user pauses monitoring. - Using
useReffor retry count would avoid unnecessary re-renders and simplify the logic.
Proposed fix using ref and proper cleanup
-import { useState, useEffect, useCallback } from 'react';
+import { useState, useEffect, useCallback, useRef } from 'react';
import type { SystemMetrics } from '../types';
const API_URL = 'http://localhost:3001/api/metrics';
const API_KEY = import.meta.env.VITE_BTOP_API_KEY;
const MAX_RETRIES = 10;
const BASE_DELAY = 1000;
const MAX_DELAY = 30000;
...
export function useSystemMetrics(refreshRate: number): UseSystemMetricsResult {
const [metrics, setMetrics] = useState<SystemMetrics | null>(null);
const [error, setError] = useState<string | null>(null);
const [loading, setLoading] = useState(true);
- const [retryCount, setRetryCount] = useState(0);
+ const retryCountRef = useRef(0);
+ const retryTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
- const fetchMetrics = useCallback(async () => {
+ const fetchMetrics = useCallback(async (signal?: AbortSignal) => {
try {
const response = await fetch(API_URL, {
headers: API_KEY ? {
'Authorization': `Bearer ${API_KEY}`,
} : {},
+ signal,
});
if (!response.ok) {
throw new Error(`HTTP error: ${response.status}`);
}
const data = await response.json();
setMetrics(data);
setError(null);
- setRetryCount(0);
+ retryCountRef.current = 0;
} catch (err) {
+ if (err instanceof Error && err.name === 'AbortError') return;
setError(err instanceof Error ? err.message : 'Failed to fetch metrics');
- setRetryCount(prev => {
- const next = prev + 1;
- if (next < MAX_RETRIES) {
- const delay = Math.min(BASE_DELAY * 2 ** prev, MAX_DELAY);
- setTimeout(fetchMetrics, delay);
- }
- return next;
- });
+ // Retry scheduling is handled in the effect
} finally {
setLoading(false);
}
}, []);
useEffect(() => {
if (refreshRate === 0) return;
- fetchMetrics();
+
+ const controller = new AbortController();
+
+ const attemptFetch = () => {
+ fetchMetrics(controller.signal).catch(() => {
+ if (retryCountRef.current < MAX_RETRIES) {
+ const delay = Math.min(BASE_DELAY * 2 ** retryCountRef.current, MAX_DELAY);
+ retryCountRef.current += 1;
+ retryTimeoutRef.current = setTimeout(attemptFetch, delay);
+ }
+ });
+ };
+
+ attemptFetch();
const interval = setInterval(fetchMetrics, refreshRate);
- return () => clearInterval(interval);
+
+ return () => {
+ controller.abort();
+ clearInterval(interval);
+ if (retryTimeoutRef.current) clearTimeout(retryTimeoutRef.current);
+ retryCountRef.current = 0;
+ };
}, [fetchMetrics, refreshRate]);Also applies to: 40-47
🧰 Tools
🪛 ESLint
[error] 22-22: 'retryCount' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useSystemMetrics.ts` at line 22, The retry logic in
useSystemMetrics uses state (retryCount / setRetryCount) causing stale closures
and no cleanup; replace retryCount state with a useRef (e.g., retryCountRef) and
store the active timeout id in another ref (e.g., retryTimeoutRef), update
retryCountRef.current instead of setRetryCount, schedule retries with setTimeout
using the refs so fetchMetrics closure is stable, and add cleanup in the
effect/unmount to clearTimeout(retryTimeoutRef.current) and to cancel retries
when monitoring is paused (check the paused flag before scheduling). Ensure
fetchMetrics reads retryCountRef.current and that all timeouts are cleared on
pause/unmount to prevent continued retries.
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
2 participants
Summary
Pkeyboard shortcut or a new pause button in the status barTest plan
Pto pause — verify metrics stop updating and "PAUSED" indicator appearsPagain to resume — verify metrics resume and indicator disappearsPkey does not trigger while typing in the filter input🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Style