Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors model color logic into dataset-scoped, family-aware palettes with version-graduated colors; pins and centralizes toktrack to 2.4.0 with exact-match local-binary validation and bunx/npx fallbacks; adds dashboard settings UI showing configured toktrack version and a lazy npm latest-version check; aligns report coloring with the dashboard palette. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser (Settings UI)
participant Settings as SettingsModal (Component)
participant API as Server (server.js)
participant Registry as npm Registry
Client->>Settings: open settings dialog
Settings->>Settings: set isLoading=true
Settings->>API: GET /api/toktrack/version-status
API->>API: lookupLatestToktrackVersion()
API->>Registry: npm view toktrack@latest version
Registry-->>API: "2.4.1"
API-->>Settings: {configuredVersion:"2.4.0", latestVersion:"2.4.1", isLatest:false, lookupStatus:"ok"}
Settings->>Settings: update UI (show update available)
sequenceDiagram
participant Dashboard as Dashboard (Component)
participant Controller as useDashboardController (Hook)
participant Provider as ModelColorPaletteProvider
participant Palette as shared/model-colors.js
participant Components as Charts/Tables
Dashboard->>Controller: controller provides allModelsFromData
Controller->>Provider: pass modelNames
Provider->>Palette: createModelColorPalette(modelNames)
Palette->>Palette: resolve families & compute assignments (version shading)
Palette-->>Provider: palette instance
Provider-->>Components: context supplies getModelColor helpers
Components->>Palette: getColor(name, options) on render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
shared/toktrack-version.js (1)
1-3: Prevent the pinned version from drifting.This duplicates
dependencies.toktrackfrompackage.json, so the next bump can leave the server advertising/running a different version than the one actually installed. Please derive it from one source or add a small test that asserts they stay equal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/toktrack-version.js` around lines 1 - 3, The hardcoded TOKTRACK_VERSION/TOKTRACK_PACKAGE_SPEC can drift from package.json; replace the literal TOKTRACK_VERSION with a single source: read dependencies.toktrack from package.json at module init and compute TOKTRACK_PACKAGE_SPEC from that value (keep TOKTRACK_PACKAGE_NAME constant), or alternatively add a small test that imports the module exposing TOKTRACK_VERSION (or TOKTRACK_PACKAGE_SPEC) and asserts it equals require('./package.json').dependencies.toktrack; update TOKTRACK_VERSION references to use the derived value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.js`:
- Around line 1852-1865: The npm lookup in lookupLatestToktrackVersion currently
awaits runCommand with no timeout; add a bounded timeout (e.g., 5–10s) to that
call (either by using runCommand's timeout option if available or by wrapping
the child-process promise in a Promise.race that kills the child on expiry) and
ensure any timeout or thrown error is caught and returned as the existing
structured failed response (e.g., return { status: 'failed', reason: 'timeout' }
or similar) so the /api/toktrack/version-status path never waits indefinitely;
keep references to TOKTRACK_PACKAGE_NAME and NPX_CACHE_DIR when invoking the
command so nothing else changes.
- Around line 1832-1838: The current logic stops at existence of the binary
(commandExists(getExecutableName('bun'))) and returns createBunxToktrackRunner()
without verifying it can actually run toktrack; update resolveToktrackRunner to
probe each candidate runner before returning: after detecting a binary,
instantiate the runner via createBunxToktrackRunner() or
createNpxToktrackRunner() and attempt a lightweight invocation (e.g., run the
runner with a safe flag like "--version" or a dry-run check) and only return the
runner if that probe succeeds; if the probe fails, log the failure and continue
to the next candidate so npx can be tried as a fallback.
In `@shared/model-colors.js`:
- Around line 43-67: The gpt-main resolver (resolve on the object with id
'gpt-main') is currently matching "GPT-4.1" via /^GPT-(\d+(?:\.\d+)*)$/i and
capturing it before the gpt-omni resolver can run; update the gpt-main.resolve
to exclude 4.1 (e.g., after matching, return null if match[1] === '4.1' or
change the regex to disallow 4.1 via a negative lookahead) or alternatively move
the gpt-omni object (id 'gpt-omni') above the gpt-main object so
gpt-omni.resolve runs first.
In `@src/lib/api.ts`:
- Around line 175-180: The fetchToktrackVersionStatus function currently throws
an error using the wrong fallback i18n key (api.fetchSettingsFailed) when
'/api/toktrack/version-status' fails; update the readErrorMessage call in
fetchToktrackVersionStatus to use a dedicated fallback key such as
'api.fetchToktrackVersionFailed' (or a neutral message key) so the toktrack
endpoint failure reports correctly in the UI.
In `@src/lib/model-utils.ts`:
- Around line 12-24: activeModelColorPalette is module-scoped mutable state
updated only via setActiveModelColorPalette, so updates inside useEffect in
use-dashboard-controller.ts do not trigger React re-renders and consumers read
stale/null palettes; refactor to derive the palette from React state or context
instead of module scope—e.g., compute/memoize the palette from allModelsFromData
in the dashboard controller (or lift it into a context provider) and pass that
state into color consumers rather than calling setActiveModelColorPalette,
keeping the palette creation logic (createModelColorPalette) but moving storage
to React state or context so updates schedule re-renders.
---
Nitpick comments:
In `@shared/toktrack-version.js`:
- Around line 1-3: The hardcoded TOKTRACK_VERSION/TOKTRACK_PACKAGE_SPEC can
drift from package.json; replace the literal TOKTRACK_VERSION with a single
source: read dependencies.toktrack from package.json at module init and compute
TOKTRACK_PACKAGE_SPEC from that value (keep TOKTRACK_PACKAGE_NAME constant), or
alternatively add a small test that imports the module exposing TOKTRACK_VERSION
(or TOKTRACK_PACKAGE_SPEC) and asserts it equals
require('./package.json').dependencies.toktrack; update TOKTRACK_VERSION
references to use the derived value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8413de6a-492f-483c-a910-9e18e7b2967b
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mdpackage.jsonserver.jsserver/report/utils.jsshared/model-colors.d.tsshared/model-colors.jsshared/toktrack-version.d.tsshared/toktrack-version.jssrc/components/features/settings/SettingsModal.tsxsrc/hooks/use-dashboard-controller.tssrc/lib/api.tssrc/lib/model-utils.tssrc/lib/toktrack-version.tssrc/locales/de/common.jsonsrc/locales/en/common.jsonsrc/types/index.tstests/frontend/settings-modal.test.tsxtests/unit/api.test.tstests/unit/auto-import.test.tstests/unit/model-colors.test.tstests/unit/report-utils.test.tstests/unit/server-helpers.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/frontend/model-color-context.test.tsx (1)
40-58: Consider adding an assertion for color format validity.The test verifies that colors change but doesn't confirm the values are valid HSL strings. This could help catch regressions in the color generation logic.
💡 Optional: Add format validation
+ const HSL_PATTERN = /^rgb\(\d+,\s*\d+,\s*\d+\)$/ + it('updates the palette when the dataset model set changes', () => { const { rerender } = render( <ModelColorPaletteProvider modelNames={['Claude Opus 4.5', 'Claude Opus 4.6']}> <ModelSwatches models={['Claude Opus 4.5']} /> </ModelColorPaletteProvider>, ) const shadedColor = getComputedStyle(screen.getByTestId('Claude Opus 4.5')).color + expect(shadedColor).toMatch(HSL_PATTERN) rerender( <ModelColorPaletteProvider modelNames={['Claude Opus 4.5']}> <ModelSwatches models={['Claude Opus 4.5']} /> </ModelColorPaletteProvider>, ) const baseColor = getComputedStyle(screen.getByTestId('Claude Opus 4.5')).color + expect(baseColor).toMatch(HSL_PATTERN) expect(shadedColor).not.toBe(baseColor) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/model-color-context.test.tsx` around lines 40 - 58, Add an assertion that the color strings retrieved from getComputedStyle for the ModelSwatches entries are valid HSL-format values; after computing shadedColor and baseColor in the test, assert each matches an HSL regex (e.g., /^hsl\(/ or a more complete HSL pattern) to ensure ModelColorPaletteProvider/ModelSwatches produce valid HSL color strings in addition to differing between dataset changes.src/components/Dashboard.tsx (1)
442-445: Consider extracting the inline callback for consistency.The
onClearDateRangehandler creates a new function reference on each render. While the performance impact is negligible here, extracting it to auseCallbackwould align with the pattern used for other handlers in this component.💅 Optional: Extract to useCallback
Add near the other handlers:
const handleClearDateRange = useCallback(() => { setStartDate(undefined) setEndDate(undefined) }, [setStartDate, setEndDate])Then use:
- onClearDateRange={() => { - setStartDate(undefined) - setEndDate(undefined) - }} + onClearDateRange={handleClearDateRange}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Dashboard.tsx` around lines 442 - 445, Extract the inline onClearDateRange callback into a stable handler using useCallback: create a handleClearDateRange function (using useCallback) that calls setStartDate(undefined) and setEndDate(undefined) and replace the inline arrow passed to onClearDateRange with handleClearDateRange; ensure the dependency array includes setStartDate and setEndDate so the handler remains consistent with other handlers in this component.src/locales/en/common.json (1)
27-27: Avoid hardcoding the pinnedtoktrackversion in localized command copy.This reintroduces a separate source of truth for the pin, so the next version bump has to touch both locale files manually and
tests/unit/toktrack-version.test.tsstill won't catch drift here. Prefer interpolating the shared package spec/version from code when rendering these examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/locales/en/common.json` at line 27, The localized string in the "description" key hardcodes a pinned toktrack version; replace the literal occurrences of "toktrack@2.4.0" in src/locales/en/common.json with a template placeholder (e.g., {{toktrackSpec}}) and update the rendering path that loads this locale to inject the shared package spec/version constant (the same constant used by tests/unit/toktrack-version.test.ts, e.g., TOKTRACK_PACKAGE_SPEC or TOKTRACK_PACKAGE_SPECIFIER) so the example commands are generated from the single source of truth at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.js`:
- Around line 1873-1885: The preflight
commandExists(getExecutableName('bun'/'npx')) calls can hang and bypass the
timed probe; remove the preflight checks and instead instantiate the runners
(createBunxToktrackRunner, createNpxToktrackRunner) and call
probeToktrackRunner(...) directly so the existing timeout in probeToktrackRunner
governs detection; i.e., replace the commandExists branches in
resolveToktrackRunner with direct creation+await probeToktrackRunner and return
the runner if probe succeeds.
In `@tests/unit/server-helpers.test.ts`:
- Around line 160-183: The test uses an external "sleep 1" binary which may not
exist in CI; update the fake npm script created via writeExecutableScript in
this test to use a portable shell-builtin that blocks (e.g., replace 'sleep 1'
with a POSIX shell loop like 'sh -c "while true; do :; done"') so
lookupLatestToktrackVersion(50) exercises the timeout path; modify the
writeExecutableScript call in the test to write that shell-builtin blocking
command.
---
Nitpick comments:
In `@src/components/Dashboard.tsx`:
- Around line 442-445: Extract the inline onClearDateRange callback into a
stable handler using useCallback: create a handleClearDateRange function (using
useCallback) that calls setStartDate(undefined) and setEndDate(undefined) and
replace the inline arrow passed to onClearDateRange with handleClearDateRange;
ensure the dependency array includes setStartDate and setEndDate so the handler
remains consistent with other handlers in this component.
In `@src/locales/en/common.json`:
- Line 27: The localized string in the "description" key hardcodes a pinned
toktrack version; replace the literal occurrences of "toktrack@2.4.0" in
src/locales/en/common.json with a template placeholder (e.g., {{toktrackSpec}})
and update the rendering path that loads this locale to inject the shared
package spec/version constant (the same constant used by
tests/unit/toktrack-version.test.ts, e.g., TOKTRACK_PACKAGE_SPEC or
TOKTRACK_PACKAGE_SPECIFIER) so the example commands are generated from the
single source of truth at runtime.
In `@tests/frontend/model-color-context.test.tsx`:
- Around line 40-58: Add an assertion that the color strings retrieved from
getComputedStyle for the ModelSwatches entries are valid HSL-format values;
after computing shadedColor and baseColor in the test, assert each matches an
HSL regex (e.g., /^hsl\(/ or a more complete HSL pattern) to ensure
ModelColorPaletteProvider/ModelSwatches produce valid HSL color strings in
addition to differing between dataset changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5b9953a-5dd4-4e43-943d-56c90e61b8ce
📒 Files selected for processing (23)
server.jsshared/model-colors.jssrc/components/Dashboard.tsxsrc/components/charts/CostByModel.tsxsrc/components/charts/CostByModelOverTime.tsxsrc/components/charts/ModelMix.tsxsrc/components/charts/RequestCacheHitRateByModel.tsxsrc/components/charts/RequestsOverTime.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/layout/FilterBar.tsxsrc/components/tables/ModelEfficiency.tsxsrc/components/tables/RecentDays.tsxsrc/hooks/use-dashboard-controller.tssrc/lib/api.tssrc/lib/model-color-context.tsxsrc/lib/model-utils.tssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/frontend/model-color-context.test.tsxtests/unit/api.test.tstests/unit/model-colors.test.tstests/unit/server-helpers.test.tstests/unit/toktrack-version.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/components/tables/RecentDays.tsx
- shared/model-colors.js
- src/hooks/use-dashboard-controller.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/api.ts
- tests/unit/api.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server.js (1)
1900-1934: Consider short-lived caching for version-status lookups.Right now each request can spawn a fresh
npm viewprocess. A small TTL cache (e.g., 1–5 min, with shorter TTL on failures) would reduce latency spikes and subprocess churn.Also applies to: 2317-2323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 1900 - 1934, Add a short-lived in-memory TTL cache around the npm lookup to avoid spawning a process on every request: in the lookupLatestToktrackVersion function, store the last result object plus a timestamp in a module-level variable (e.g., latestToktrackCache with {value, expiresAt}) and return the cached value when Date.now() < expiresAt; on cache miss call runCommand(getExecutableName('npm'), ['view', `${TOKTRACK_PACKAGE_NAME}@latest', 'version'], ...) as before, store the success result with a 1–5 minute TTL and store failures with a shorter TTL (e.g., 30–60s) to avoid hammering; ensure you reference and return the same fields (configuredVersion, latestVersion, isLatest, lookupStatus, message) so callers of lookupLatestToktrackVersion keep the same contract, and apply the same caching pattern to the other similar npm lookup in the file.
🤖 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/Dashboard.tsx`:
- Around line 351-372: The FilterBar shows options from availableModels while
selectedModels can contain entries that were filtered out, causing invisible
active filters; fix by either pruning selectedModels when availableModels
changes or by making the FilterBar receive the union of both lists so selected
items remain visible. Implement one of these in the Dashboard component: (A) on
updates to availableModels compute intersection = selectedModels ∩
availableModels and call setSelectedModels(intersection) to auto-prune; or (B)
change the prop passed to FilterBar from allModels={availableModels} to
allModels={Array.from(new Set([...availableModels, ...selectedModels]))}
(dedupe) so FilterBar renders all active selections; use the existing symbols
selectedModels, setSelectedModels, availableModels, and FilterBar/allModels to
locate and apply the change.
In `@tests/frontend/empty-state.test.tsx`:
- Around line 29-34: The test embeds TOKTRACK_PACKAGE_SPEC directly into RegExp
which treats regex metacharacters (like dots in versions) as wildcards; fix by
escaping TOKTRACK_PACKAGE_SPEC before constructing the RegExp used in the two
getByText assertions. Add an escaped variable (e.g., escape
TOKTRACK_PACKAGE_SPEC with a standard escape for regex metacharacters) and use
new RegExp(escapedSpec + " daily --json") for both occurrences so the literal
package spec is matched.
---
Nitpick comments:
In `@server.js`:
- Around line 1900-1934: Add a short-lived in-memory TTL cache around the npm
lookup to avoid spawning a process on every request: in the
lookupLatestToktrackVersion function, store the last result object plus a
timestamp in a module-level variable (e.g., latestToktrackCache with {value,
expiresAt}) and return the cached value when Date.now() < expiresAt; on cache
miss call runCommand(getExecutableName('npm'), ['view',
`${TOKTRACK_PACKAGE_NAME}@latest', 'version'], ...) as before, store the success
result with a 1–5 minute TTL and store failures with a shorter TTL (e.g.,
30–60s) to avoid hammering; ensure you reference and return the same fields
(configuredVersion, latestVersion, isLatest, lookupStatus, message) so callers
of lookupLatestToktrackVersion keep the same contract, and apply the same
caching pattern to the other similar npm lookup in the file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66eaa64e-1bb0-4157-8eed-37b6296ea78b
📒 Files selected for processing (8)
server.jssrc/components/Dashboard.tsxsrc/components/EmptyState.tsxsrc/locales/de/common.jsonsrc/locales/en/common.jsontests/frontend/empty-state.test.tsxtests/frontend/model-color-context.test.tsxtests/unit/server-helpers.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/locales/de/common.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/locales/en/common.json
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 by CodeRabbit
New Features
Improvements
Documentation