Conversation
📝 WalkthroughWalkthroughAdds price history, notify, and timeline features; expands MCP and TUI flows; updates tests and CLI wiring; and pins workflow security tooling. ChangesSubscription features and TUI updates
Supply-chain hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
subtrack/v8-plan.md (2)
99-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHistory write runs outside the subscription update transaction.
The plan notes that
updateSubscriptionexecutes inside a DB transaction whilewritePriceHistoryhappens after. If the history write fails, the subscription update is already committed, leaving updated prices without audit trail. Consider wrapping both operations in a single transaction or implementing a retry/recovery mechanism for history writes.🤖 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 `@subtrack/v8-plan.md` around lines 99 - 101, The plan for `updateSubscription` and `writePriceHistory` leaves the history write outside the same database transaction, so a failed audit write can leave committed subscription changes without matching history. Update the flow so the subscription update and price-history recording are handled together in one transactional unit, or add a robust retry/recovery path around `writePriceHistory` that is coordinated with `updateSubscription`. Use the `updateSubscription` and `writePriceHistory` steps in this plan as the anchors when revising the transaction boundary.
151-153: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDouble-tap sort reversal behavior is ambiguously specified.
The plan describes "現在のsort fieldで2回目
sを押したら方向反転" (pressingsa second time on the current field reverses direction), but theSET_SORTaction description says "現在のfieldでもう一度押したらsortDescを反転、異なるfieldならそっちに変更" (reverse on same field, change on different). Clarify whether this replaces or extends the current cyclic behavior to avoid confusing users who expectsto always cycle through fields.🤖 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 `@subtrack/v8-plan.md` around lines 151 - 153, Clarify the sort-toggle behavior so it is unambiguous: the current `SET_SORT` description and the “2回目 s を押したら方向反転” note conflict with the existing cyclic behavior. Update the plan text around `SET_SORT` and the sort-field key handling to explicitly state whether pressing `s` on the same field immediately flips `sortDesc` or only after cycling through fields, and make the wording consistent wherever this behavior is described.subtrack/src/db.ts (1)
1006-1036: 🚀 Performance & Scalability | 🔵 TrivialConsider an index on
price_history(subscription_id, changed_at).
getPriceHistoryfilters onsubscription_idand both helpers order bychanged_at. A composite index would keep these queries fast as history grows. Likely negligible for typical local DB sizes, so treat as optional.🤖 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 `@subtrack/src/db.ts` around lines 1006 - 1036, The history query helpers getPriceHistory and getAllPriceChanges currently scan and sort price_history by subscription_id and changed_at without supporting index coverage. Add an optional composite index on price_history(subscription_id, changed_at) in the schema/migration path used by db.ts so the WHERE and ORDER BY patterns in these helpers stay fast as data grows.subtrack/src/history.ts (1)
59-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new
handleHistorypaths.
handleHistory(json / per-id /--all/ usage routing) is a new critical path with no test coverage in this PR. Worth covering the per-id,--all, and JSON branches.As per coding guidelines: "Write unit tests for all functions and critical code paths".
Want me to draft tests using the in-memory DB setup (
__setDb()) andconsola.mockTypes()?🤖 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 `@subtrack/src/history.ts` around lines 59 - 97, Add unit tests for handleHistory to cover each routing branch: JSON output, per-id lookup, --all listing, and the usage fallback. Use the handleHistory function as the entry point and mock getPriceHistory, getAllPriceChanges, getSubscription, displayHistory, and stdout writes so you can assert the correct branch is taken without relying on real I/O. Include cases for a missing subscription in the per-id path and an empty result in the --all path, and if needed use the in-memory DB setup with __setDb() plus consola.mockTypes() to verify the emitted messages.Source: Coding guidelines
🤖 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 `@subtrack/package.json`:
- Around line 68-76: The dependency setup in package.json has a version mismatch
between node-notifier v10 and `@types/node-notifier` v8, so align the typings with
the v10 API surface. Update the `@types/node-notifier` entry in the
devDependencies section to the matching v10-compatible release when available,
or add/adjust local declarations so the types used by node-notifier stay in
sync.
In `@subtrack/src/index.ts`:
- Around line 884-885: The ID parsing in the command handler should rely on the
resolved Gunshi value rather than the raw positional fallback. Update the logic
in the index command flow where ctx.values.id and positionals are read so it
uses ctx.values.id directly, or if you still need a raw positional fallback,
switch to the first positional instead of the second. Keep the change localized
around the existing id assignment in this path.
In `@subtrack/src/tui/screens/list.tsx`:
- Around line 321-327: The title bar filter display is duplicated because the
new conditional block renders state.filterText a second time instead of only
adding the count. Update the existing filter-text rendering in list.tsx so the
same block that shows the truncated query (the one with the ▶ prefix and info
color) also appends ({subs.length}) when a filter is active and results exist,
and remove the separate dimColor block entirely.
---
Nitpick comments:
In `@subtrack/src/db.ts`:
- Around line 1006-1036: The history query helpers getPriceHistory and
getAllPriceChanges currently scan and sort price_history by subscription_id and
changed_at without supporting index coverage. Add an optional composite index on
price_history(subscription_id, changed_at) in the schema/migration path used by
db.ts so the WHERE and ORDER BY patterns in these helpers stay fast as data
grows.
In `@subtrack/src/history.ts`:
- Around line 59-97: Add unit tests for handleHistory to cover each routing
branch: JSON output, per-id lookup, --all listing, and the usage fallback. Use
the handleHistory function as the entry point and mock getPriceHistory,
getAllPriceChanges, getSubscription, displayHistory, and stdout writes so you
can assert the correct branch is taken without relying on real I/O. Include
cases for a missing subscription in the per-id path and an empty result in the
--all path, and if needed use the in-memory DB setup with __setDb() plus
consola.mockTypes() to verify the emitted messages.
In `@subtrack/v8-plan.md`:
- Around line 99-101: The plan for `updateSubscription` and `writePriceHistory`
leaves the history write outside the same database transaction, so a failed
audit write can leave committed subscription changes without matching history.
Update the flow so the subscription update and price-history recording are
handled together in one transactional unit, or add a robust retry/recovery path
around `writePriceHistory` that is coordinated with `updateSubscription`. Use
the `updateSubscription` and `writePriceHistory` steps in this plan as the
anchors when revising the transaction boundary.
- Around line 151-153: Clarify the sort-toggle behavior so it is unambiguous:
the current `SET_SORT` description and the “2回目 s を押したら方向反転” note conflict with
the existing cyclic behavior. Update the plan text around `SET_SORT` and the
sort-field key handling to explicitly state whether pressing `s` on the same
field immediately flips `sortDesc` or only after cycling through fields, and
make the wording consistent wherever this behavior is described.
🪄 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: 5af1d63c-f832-475c-a082-55fedce0a49d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
subtrack/package.jsonsubtrack/src/__tests__/commands.test.tssubtrack/src/commands.tssubtrack/src/config.tssubtrack/src/db.tssubtrack/src/history.tssubtrack/src/index.tssubtrack/src/notify.tssubtrack/src/subscription.tssubtrack/src/tui/components/command-bar.tsxsubtrack/src/tui/context/app-context.tsxsubtrack/src/tui/screen-router.tsxsubtrack/src/tui/screens/detail.tsxsubtrack/src/tui/screens/history-screen.tsxsubtrack/src/tui/screens/list.tsxsubtrack/src/tui/types.tssubtrack/src/types.tssubtrack/v8-plan.md
| "node-notifier": "^10.0.1", | ||
| "picocolors": "^1.1.1", | ||
| "react": "^19.2.7", | ||
| "sql.js": "^1.14.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@secretlint/secretlint-rule-preset-recommend": "^13.0.2", | ||
| "@types/node": "^26.0.0", | ||
| "@types/node-notifier": "^8.0.5", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @types/node-notifier 8.x cover node-notifier 10 API, or does node-notifier ship its own types?
💡 Result:
Node-notifier does not ship its own type definitions; it relies on the community-maintained @types/node-notifier package on DefinitelyTyped [1][2][3]. Regarding compatibility, @types/node-notifier 8.x was initially designed for older versions of the library. However, as of March 2026, the type definitions have been updated to align with the node-notifier v10 API surface (see pull request #74675 on DefinitelyTyped) [4]. If you are using node-notifier 10, ensure you are using the latest version of @types/node-notifier to access the correct API definitions, including additions such as the root Notification constructor alias, EventEmitter inheritance, and updated options for WindowsToaster and NotificationCenter [4].
Citations:
- 1: https://registry.npmjs.org/@types/node-notifier
- 2: https://registry.npmjs.org/node-notifier
- 3: https://www.npmjs.com/package/node-notifier
- 4: [node-notifier] Align type definitions with v10 API surface DefinitelyTyped/DefinitelyTyped#74675
🌐 Web query:
npm registry @types/node-notifier latest version and compatibility with node-notifier v10
💡 Result:
The latest version of the @types/node-notifier package on the npm registry is 8.0.5 [1]. Regarding compatibility with node-notifier v10: while the package version 8.0.5 has served as the standard type definition, it does not fully align with the API surface of node-notifier v10.0.1 [2]. As of early 2026, a pull request (DefinitelyTyped PR #74675) has been developed to specifically align these type definitions with the node-notifier v10 API, including additions such as the root Notification constructor alias, EventEmitter inheritance for notifier classes, and updated options for WindowsToaster and NotifySend [2]. If you are using node-notifier v10, you may encounter missing type definitions if you rely solely on version 8.0.5 [2]. Users requiring comprehensive support for v10 features should check the status of the aforementioned DefinitelyTyped updates or consider extending the existing types locally [2].
Citations:
- 1: https://registry.npmjs.org/@types/node-notifier
- 2: [node-notifier] Align type definitions with v10 API surface DefinitelyTyped/DefinitelyTyped#74675
Keep @types/node-notifier aligned with node-notifier v10. @types/node-notifier@8.0.5 still lags the v10 API surface, so this pairing can miss newer declarations. Update the typings once the v10-aligned release is available, or add the missing declarations locally.
🤖 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 `@subtrack/package.json` around lines 68 - 76, The dependency setup in
package.json has a version mismatch between node-notifier v10 and
`@types/node-notifier` v8, so align the typings with the v10 API surface. Update
the `@types/node-notifier` entry in the devDependencies section to the matching
v10-compatible release when available, or add/adjust local declarations so the
types used by node-notifier stay in sync.
| const positionals = ctx.positionals as string[] | ||
| const id = ctx.values.id !== undefined ? Number(ctx.values.id) : positionals[1] ? Number(positionals[1]) : undefined |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how other gunshi commands in this file read positionals to confirm convention.
rg -nP -C3 'ctx\.(positionals|values\.id)' subtrack/src/index.tsRepository: nazozokc/subtrack
Length of output: 1986
🌐 Web query:
gunshi ctx.values positional arguments ctx.positionals command handler documentation
💡 Result:
In the Gunshi library, the ctx (CommandContext) object provides two primary ways to access positional arguments: ctx.values and ctx.positionals [1]. ctx.values This is the recommended way to access both named options and positional arguments [1]. When you define positional arguments in your command configuration (by setting their type to 'positional'), they are assigned a key [1]. You can then access these resolved and validated values directly via ctx.values using that key [1]. This approach is preferred for its clarity, consistency, and type safety [1]. ctx.positionals This property is an array of strings that contains the raw, unparsed values of positional arguments in the exact order they appeared on the command line [1][2]. While it remains available for use when you need to retain the original order or process arguments that have not been explicitly defined in your schema, ctx.values is generally recommended for standard use cases [1]. Summary of Key Differences: - ctx.values: Provides resolved, validated, and typed values (e.g., strings, numbers, booleans) based on the argument definitions in your schema [1]. - ctx.positionals: Provides raw string values in their original order [1]. For developers, using ctx.values leverages Gunshi's type inference and validation capabilities, while ctx.positionals serves as a fallback or utility for scenarios requiring raw access to the input stream [1][3].
Citations:
- 1: https://gunshi.dev/guide/essentials/declarative
- 2: https://gunshi.dev/guide/essentials/getting-started
- 3: https://gunshi.dev/guide/essentials/type-safe
🌐 Web query:
gunshi positional arg access ctx.values.id positionals array
💡 Result:
In the Gunshi library, you can access positional arguments in two primary ways within the run function of your command [1]. 1. Using ctx.values (Recommended): When you define an argument with type: 'positional' in your args configuration, you assign it a key (e.g., 'id'). Gunshi resolves the value of that positional argument and makes it available via that key within the ctx.values object (e.g., ctx.values.id) [1][2]. This method is preferred because it provides type safety and better clarity [1][3]. 2. Using ctx.positionals: The ctx.positionals property is an array that contains the raw string values of all positional arguments in the order they were parsed [1]. You can access these values using array indices (e.g., ctx.positionals) [1]. While this remains available for use cases where the original order or raw strings are required, it is generally recommended to use ctx.values for structured access [1]. Example of declarative configuration: const command = define({ args: { id: { type: 'positional', description: 'The ID argument' } }, run: ctx => { // Access via resolved values console.log('ID:', ctx.values.id); // Or access via raw positionals array console.log('First positional:', ctx.positionals); } });
Citations:
- 1: https://gunshi.dev/guide/essentials/declarative
- 2: feat: resolve positional argument placeholder values with argument schema kazupon/gunshi#111
- 3: https://gunshi.dev/guide/essentials/type-safe
Use ctx.values.id directly
Gunshi already resolves positional args into ctx.values, so the positionals[1] fallback is unnecessary and would read the second raw positional if it ever runs. Remove the fallback, or use positionals[0] for the raw value.
🤖 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 `@subtrack/src/index.ts` around lines 884 - 885, The ID parsing in the command
handler should rely on the resolved Gunshi value rather than the raw positional
fallback. Update the logic in the index command flow where ctx.values.id and
positionals are read so it uses ctx.values.id directly, or if you still need a
raw positional fallback, switch to the first positional instead of the second.
Keep the change localized around the existing id assignment in this path.
| {state.filterText && subs.length > 0 && ( | ||
| <Text dimColor> | ||
| {" — "}{state.filterText.length > 15 | ||
| ? state.filterText.slice(0, 15) + "…" | ||
| : state.filterText}{" "}({subs.length}) | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter text now renders twice in the title bar.
The pre-existing block at Lines 308-315 already displays state.filterText (info color, ▶ prefix, truncated to 20). This new block renders the same filterText again (dim, — prefix, truncated to 15) just to append the match count. When a filter is active, both appear side by side, so the query shows duplicated. Fold the ({subs.length}) count into the existing block instead of adding a second one.
🔧 Suggested consolidation
- {state.filterText && (
- <Text color={colors.info}>
- {" ▶ "}
- {state.filterText.length > 20
- ? state.filterText.slice(0, 20) + "…"
- : state.filterText}
- </Text>
- )}
+ {state.filterText && (
+ <Text color={colors.info}>
+ {" ▶ "}
+ {state.filterText.length > 20
+ ? state.filterText.slice(0, 20) + "…"
+ : state.filterText}
+ {subs.length > 0 ? ` (${subs.length})` : ""}
+ </Text>
+ )}And drop the duplicate block:
- {state.filterText && subs.length > 0 && (
- <Text dimColor>
- {" — "}{state.filterText.length > 15
- ? state.filterText.slice(0, 15) + "…"
- : state.filterText}{" "}({subs.length})
- </Text>
- )}🤖 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 `@subtrack/src/tui/screens/list.tsx` around lines 321 - 327, The title bar
filter display is duplicated because the new conditional block renders
state.filterText a second time instead of only adding the count. Update the
existing filter-text rendering in list.tsx so the same block that shows the
truncated query (the one with the ▶ prefix and info color) also appends
({subs.length}) when a filter is active and results exist, and remove the
separate dimColor block entirely.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
subtrack/src/mcp.ts (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the newly exported helpers.
These functions just became part of the module's public surface, but they still have no API docs. Please add short JSDoc blocks before each export so the MCP contract stays discoverable and matches the repo rule for public TS APIs. As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript".
Also applies to: 43-46, 110-110, 144-145
🤖 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 `@subtrack/src/mcp.ts` at line 24, Add short JSDoc comments for each newly exported helper so the public MCP API is documented and discoverable. Update the exports in mcp.ts, including formatDateISO, the other exported helper around lines 43-46, and the exports near lines 110 and 144-145, with brief descriptions matching the repo’s public TS API guideline. Keep the docs immediately before each export and make sure the function/method names remain unchanged so the contract stays clear.Source: Coding guidelines
subtrack/src/__tests__/mcp.test.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate this test beside
mcp.ts.This new suite lives under
src/__tests__, but the repo requiressubtrack/**/*.test.tsfiles to be co-located. Please move it next tomcp.tsso the test layout stays consistent. As per coding guidelines, "subtrack/**/*.test.ts: Co-locate test files as*.test.ts."🤖 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 `@subtrack/src/__tests__/mcp.test.ts` around lines 1 - 3, The new mcp test suite is placed in the shared __tests__ folder instead of being co-located with the implementation. Move the test file next to mcp.ts so it follows the repo’s subtrack/**/*.test.ts convention, and keep the test content unchanged after relocating it. Update any import paths or references in mcp.test.ts if the move changes relative paths, using mcp.ts and mcp.test.ts as the key symbols to verify the placement.Source: Coding guidelines
subtrack/src/config.ts (1)
100-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the new config helpers.
TuiColumnSettings,loadTuiColumns(), andsaveTuiColumns()are newly exported surface area, but they are still undocumented. As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript".🤖 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 `@subtrack/src/config.ts` around lines 100 - 125, Add JSDoc comments to the newly exported TUI config API so the public surface is documented. Update TuiColumnSettings, loadTuiColumns(), and saveTuiColumns() with brief descriptions of their purpose and what the settings fields represent, matching the existing style used in config.ts. Keep the comments directly above each symbol so they remain attached if the declarations move.Source: Coding guidelines
subtrack/src/tui/context/app-context.tsx (1)
93-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported reducer surface.
initialStateandappReducer()are exported from this module now, but neither has JSDoc describing the state shape/contract. As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript".🤖 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 `@subtrack/src/tui/context/app-context.tsx` around lines 93 - 123, The exported app state API in app-context.tsx is undocumented: add JSDoc to both initialState and appReducer to describe the AppState contract and reducer behavior. Use the existing symbols initialState and appReducer so future consumers understand the intended shape, defaults, and action-driven state updates. Keep the comments brief but explicit since these are public exports.Source: Coding guidelines
subtrack/src/__tests__/tui-context.test.ts (1)
1-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate this test with
app-context.tsx.
subtrack/src/__tests__/tui-context.test.tsdoesn't follow the repo rule forsubtrack/**/*.test.ts. Moving it next to the source file keeps ownership and imports local. As per coding guidelines, "subtrack/**/*.test.ts: Co-locate test files as*.test.ts."🤖 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 `@subtrack/src/__tests__/tui-context.test.ts` around lines 1 - 293, This test file is misplaced relative to the repo’s co-location rule; move the `appReducer` tests so they live next to `app-context.tsx` as a sibling `*.test.ts` file. Keep the same test content and imports, but relocate it to the source module’s directory so `appReducer`, `AppState`, and `AppAction` remain locally owned and easier to maintain.Source: Coding guidelines
🤖 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 `@subtrack/src/__tests__/mcp.test.ts`:
- Around line 148-172: The searchSubscriptions tests only cover the default name
path and a no-match case, so add targeted cases for the notes and tags branches
and the explicit field-selection behavior in mcp.ts. Extend the existing
searchSubscriptions describe block with tests that seed subscriptions data
containing notes and tags, then assert the query path selects matches from notes
and tags as intended and returns the expected fields. Use the unique symbol
searchSubscriptions to locate the code path and cover the SQL builder branches
that are currently untested.
- Around line 121-129: Add a unit test in nextDateForCycle’s test suite to cover
the missing "semi-annual" branch. Extend the existing mcp.test.ts coverage
around nextDateForCycle by asserting the returned month/day for a semi-annual
input from a known anchor and from-date, so the dedicated "semi-annual" path is
exercised alongside the current quarterly case.
In `@subtrack/src/tui/context/app-context.tsx`:
- Around line 225-246: Keep persistence out of appReducer()’s TOGGLE_COLUMN
case: the reducer should only compute the next AppState and must not call
saveTuiColumns(). Move the saveTuiColumns(settings) write to the dispatch site
or a provider-level useEffect keyed on the column flags (showTagsCol,
showNotesCol, showMethodCol) so state updates remain pure and config writes
happen outside reducer execution.
In `@subtrack/src/tui/screens/list.tsx`:
- Around line 151-153: The bulk-confirm flow is disabling its own input handling
by dispatching SET_FORM_ACTIVE from the useEffect that watches bulkConfirm,
while useInput on the same screen only runs when !state.formActive. Update the
logic around bulkConfirm, useEffect, and useInput so the y/n/Esc handler stays
active during the confirmation prompt, either by not flipping formActive for
bulkConfirm or by using a separate state flag for the prompt.
- Around line 167-178: The bulk-delete loop in `List` is counting every
successful call to `deleteSubscription`, but that function can return false when
nothing was removed. Update the loop to inspect the boolean result from
`deleteSubscription(id)` and only increment `count` when it returns true, while
still skipping thrown failures; keep the toast message in sync with the actual
deletions.
---
Nitpick comments:
In `@subtrack/src/__tests__/mcp.test.ts`:
- Around line 1-3: The new mcp test suite is placed in the shared __tests__
folder instead of being co-located with the implementation. Move the test file
next to mcp.ts so it follows the repo’s subtrack/**/*.test.ts convention, and
keep the test content unchanged after relocating it. Update any import paths or
references in mcp.test.ts if the move changes relative paths, using mcp.ts and
mcp.test.ts as the key symbols to verify the placement.
In `@subtrack/src/__tests__/tui-context.test.ts`:
- Around line 1-293: This test file is misplaced relative to the repo’s
co-location rule; move the `appReducer` tests so they live next to
`app-context.tsx` as a sibling `*.test.ts` file. Keep the same test content and
imports, but relocate it to the source module’s directory so `appReducer`,
`AppState`, and `AppAction` remain locally owned and easier to maintain.
In `@subtrack/src/config.ts`:
- Around line 100-125: Add JSDoc comments to the newly exported TUI config API
so the public surface is documented. Update TuiColumnSettings, loadTuiColumns(),
and saveTuiColumns() with brief descriptions of their purpose and what the
settings fields represent, matching the existing style used in config.ts. Keep
the comments directly above each symbol so they remain attached if the
declarations move.
In `@subtrack/src/mcp.ts`:
- Line 24: Add short JSDoc comments for each newly exported helper so the public
MCP API is documented and discoverable. Update the exports in mcp.ts, including
formatDateISO, the other exported helper around lines 43-46, and the exports
near lines 110 and 144-145, with brief descriptions matching the repo’s public
TS API guideline. Keep the docs immediately before each export and make sure the
function/method names remain unchanged so the contract stays clear.
In `@subtrack/src/tui/context/app-context.tsx`:
- Around line 93-123: The exported app state API in app-context.tsx is
undocumented: add JSDoc to both initialState and appReducer to describe the
AppState contract and reducer behavior. Use the existing symbols initialState
and appReducer so future consumers understand the intended shape, defaults, and
action-driven state updates. Keep the comments brief but explicit since these
are public exports.
🪄 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: aead6c48-daf7-4feb-ba1f-24eb43f03831
📒 Files selected for processing (19)
subtrack/src/__tests__/mcp.test.tssubtrack/src/__tests__/scanner-providers.test.tssubtrack/src/__tests__/tui-context.test.tssubtrack/src/analytics.tssubtrack/src/config.tssubtrack/src/crypto.tssubtrack/src/mcp.tssubtrack/src/tui/components/sidebar.tsxsubtrack/src/tui/components/status-bar.tsxsubtrack/src/tui/components/toast.tsxsubtrack/src/tui/context/app-context.tsxsubtrack/src/tui/screens/calendar-screen.tsxsubtrack/src/tui/screens/config.tsxsubtrack/src/tui/screens/delete.tsxsubtrack/src/tui/screens/detail.tsxsubtrack/src/tui/screens/edit.tsxsubtrack/src/tui/screens/list.tsxsubtrack/src/tui/types.tssubtrack/src/types.ts
✅ Files skipped from review due to trivial changes (5)
- subtrack/src/analytics.ts
- subtrack/src/tui/screens/calendar-screen.tsx
- subtrack/src/tests/scanner-providers.test.ts
- subtrack/src/crypto.ts
- subtrack/src/tui/components/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- subtrack/src/types.ts
- subtrack/src/tui/screens/detail.tsx
- subtrack/src/tui/types.ts
| test("nextDateForCycle — quarterly returns next quarter", async () => { | ||
| const { nextDateForCycle } = await import("../mcp.ts") | ||
| const anchor = new Date(2026, 0, 15) // Jan 15 | ||
| const from = new Date(2026, 5, 1) // Jun 1 | ||
| const next = nextDateForCycle(15, anchor, "quarterly", from) | ||
| expect(next.getMonth()).toBe(6) // July (Q3) | ||
| expect(next.getDate()).toBe(15) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing semi-annual cycle case.
nextDateForCycle has a dedicated "semi-annual" branch, but this suite never exercises it. A single branch-specific assertion here would close that gap. As per coding guidelines, "**/*.test.{js,ts,jsx,tsx}: Write unit tests for all functions and critical code paths".
🤖 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 `@subtrack/src/__tests__/mcp.test.ts` around lines 121 - 129, Add a unit test
in nextDateForCycle’s test suite to cover the missing "semi-annual" branch.
Extend the existing mcp.test.ts coverage around nextDateForCycle by asserting
the returned month/day for a semi-annual input from a known anchor and
from-date, so the dedicated "semi-annual" path is exercised alongside the
current quarterly case.
Source: Coding guidelines
| describe("searchSubscriptions", () => { | ||
| test("searches by name pattern", async () => { | ||
| testDb.run( | ||
| `INSERT INTO subscriptions (id, name, price, currency, cycle, status, billing_day, created_at, notes) | ||
| VALUES (1, 'Netflix', 1990, 'JPY', 'monthly', 'active', 15, '2026-01-01', 'Family plan'), | ||
| (2, 'Spotify', 980, 'JPY', 'monthly', 'active', 1, '2026-01-10', NULL)`, | ||
| ) | ||
|
|
||
| const { searchSubscriptions } = await import("../mcp.ts") | ||
| const results = searchSubscriptions("net", {}) | ||
| expect(results.length).toBeGreaterThanOrEqual(1) | ||
| expect(results.some((r: { name: string }) => r.name === "Netflix")).toBe(true) | ||
| }) | ||
|
|
||
| test("returns empty array for no match", async () => { | ||
| testDb.run( | ||
| `INSERT INTO subscriptions (id, name, price, currency, cycle, status, billing_day, created_at) | ||
| VALUES (1, 'Netflix', 1990, 'JPY', 'monthly', 'active', 15, '2026-01-01')`, | ||
| ) | ||
|
|
||
| const { searchSubscriptions } = await import("../mcp.ts") | ||
| const results = searchSubscriptions("zzzzz", {}) | ||
| expect(results.length).toBe(0) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the notes and tags search branches too.
These tests only hit the default name path and the no-match case. The SQL builder has separate notes and tags branches plus explicit field-selection behavior, so add focused cases for those paths as well. As per coding guidelines, "**/*.test.{js,ts,jsx,tsx}: Write unit tests for all functions and critical code paths".
🤖 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 `@subtrack/src/__tests__/mcp.test.ts` around lines 148 - 172, The
searchSubscriptions tests only cover the default name path and a no-match case,
so add targeted cases for the notes and tags branches and the explicit
field-selection behavior in mcp.ts. Extend the existing searchSubscriptions
describe block with tests that seed subscriptions data containing notes and
tags, then assert the query path selects matches from notes and tags as intended
and returns the expected fields. Use the unique symbol searchSubscriptions to
locate the code path and cover the SQL builder branches that are currently
untested.
Source: Coding guidelines
| case "TOGGLE_COLUMN": { | ||
| let next: AppState | ||
| switch (action.column) { | ||
| case "tags": | ||
| next = { ...state, showTagsCol: !state.showTagsCol } | ||
| break | ||
| case "notes": | ||
| next = { ...state, showNotesCol: !state.showNotesCol } | ||
| break | ||
| case "method": | ||
| next = { ...state, showMethodCol: !state.showMethodCol } | ||
| break | ||
| default: | ||
| return state | ||
| } | ||
| // Persist to config | ||
| const settings: TuiColumnSettings = { | ||
| showTagsCol: next.showTagsCol, | ||
| showNotesCol: next.showNotesCol, | ||
| showMethodCol: next.showMethodCol, | ||
| } | ||
| try { saveTuiColumns(settings) } catch { /* best-effort */ } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant area.
ast-grep outline subtrack/src/tui/context/app-context.tsx --view expanded || true
printf '\n--- lines 180-280 ---\n'
sed -n '180,280p' subtrack/src/tui/context/app-context.tsx
printf '\n--- search for saveTuiColumns / reducer / provider ---\n'
rg -n "saveTuiColumns|appReducer|TOGGLE_COLUMN|useReducer|dispatch" subtrack/src/tui/context/app-context.tsx subtrack/src/tui -g '*.ts' -g '*.tsx'Repository: nazozokc/subtrack
Length of output: 19393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the persistence helper to understand whether it is synchronous and side-effectful.
fd -a ".*" subtrack/src/tui | sed -n '1,120p'
printf '\n--- saveTuiColumns definition search ---\n'
rg -n "function saveTuiColumns|const saveTuiColumns|export .*saveTuiColumns|saveTuiColumns\\(" subtrack/src/tui -g '*.ts' -g '*.tsx'Repository: nazozokc/subtrack
Length of output: 3046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate config file and inspect TUI column persistence helpers ---'
fd -a '^config\.ts$' subtrack/src
printf '\n'
ast-grep outline subtrack/src/config.ts --view expanded || true
printf '\n--- relevant lines from subtrack/src/config.ts ---\n'
sed -n '1,220p' subtrack/src/config.ts
printf '\n--- tests / reducer usages ---\n'
rg -n "appReducer|TOGGLE_COLUMN|saveTuiColumns|loadTuiColumns" subtrack -g '*test.*' -g '*spec.*' -g '*.ts' -g '*.tsx'Repository: nazozokc/subtrack
Length of output: 9873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,250p' subtrack/src/__tests__/tui-context.test.tsRepository: nazozokc/subtrack
Length of output: 1295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether reducer is expected to stay pure anywhere else, and whether TOGGLE_COLUMN is exercised directly.
rg -n "appReducer\\(|TOGGLE_COLUMN|saveTuiColumns|loadTuiColumns" subtrack/src/__tests__ subtrack/src/tui/context/app-context.tsx subtrack/src/config.tsRepository: nazozokc/subtrack
Length of output: 5012
Keep persistence out of the reducer. TOGGLE_COLUMN still writes config from appReducer(), so dispatching it can hit the filesystem during reducer tests and can repeat the write under React dev checks. Move saveTuiColumns() to the dispatch site or a provider-level effect keyed off the column flags.
🤖 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 `@subtrack/src/tui/context/app-context.tsx` around lines 225 - 246, Keep
persistence out of appReducer()’s TOGGLE_COLUMN case: the reducer should only
compute the next AppState and must not call saveTuiColumns(). Move the
saveTuiColumns(settings) write to the dispatch site or a provider-level
useEffect keyed on the column flags (showTagsCol, showNotesCol, showMethodCol)
so state updates remain pure and config writes happen outside reducer execution.
| useEffect(() => { | ||
| dispatch({ type: "SET_FORM_ACTIVE", active: bulkConfirm !== null }) | ||
| }, [bulkConfirm, dispatch]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't let bulk confirm disable its own input handler.
When bulkConfirm becomes non-null, this effect dispatches SET_FORM_ACTIVE, but the same screen's useInput is only active while !state.formActive. After the next render, the y/n/Esc branch stops receiving input, so the new bulk-delete prompt cannot be completed reliably.
Suggested fix
useInput(
(input: string, key) => {
@@
},
- { isActive: state.focus === "content" && !state.formActive && !state.paletteOpen },
+ {
+ isActive:
+ state.focus === "content" &&
+ !state.paletteOpen &&
+ (!state.formActive || bulkConfirm !== null),
+ },
)Also applies to: 157-188
🤖 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 `@subtrack/src/tui/screens/list.tsx` around lines 151 - 153, The bulk-confirm
flow is disabling its own input handling by dispatching SET_FORM_ACTIVE from the
useEffect that watches bulkConfirm, while useInput on the same screen only runs
when !state.formActive. Update the logic around bulkConfirm, useEffect, and
useInput so the y/n/Esc handler stays active during the confirmation prompt,
either by not flipping formActive for bulkConfirm or by using a separate state
flag for the prompt.
| for (const id of ids) { | ||
| try { | ||
| deleteSubscription(id) | ||
| count++ | ||
| } catch { /* skip failed */ } | ||
| } | ||
| dispatch({ type: "MULTI_SELECT_CLEAR" }) | ||
| dispatch({ type: "INCREMENT_REFRESH_KEY" }) | ||
| dispatch({ | ||
| type: "SET_TOAST", | ||
| toast: { message: `Deleted ${count} subscription${count !== 1 ? "s" : ""}`, type: "success" }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use deleteSubscription's boolean result when counting deletions.
deleteSubscription returns false when no row is removed, but this loop increments count after every call that doesn't throw. A stale selection will therefore show an inflated success count.
Suggested fix
for (const id of ids) {
try {
- deleteSubscription(id)
- count++
+ if (deleteSubscription(id)) {
+ count++
+ }
} catch { /* skip failed */ }
}📝 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.
| for (const id of ids) { | |
| try { | |
| deleteSubscription(id) | |
| count++ | |
| } catch { /* skip failed */ } | |
| } | |
| dispatch({ type: "MULTI_SELECT_CLEAR" }) | |
| dispatch({ type: "INCREMENT_REFRESH_KEY" }) | |
| dispatch({ | |
| type: "SET_TOAST", | |
| toast: { message: `Deleted ${count} subscription${count !== 1 ? "s" : ""}`, type: "success" }, | |
| }) | |
| for (const id of ids) { | |
| try { | |
| if (deleteSubscription(id)) { | |
| count++ | |
| } | |
| } catch { /* skip failed */ } | |
| } | |
| dispatch({ type: "MULTI_SELECT_CLEAR" }) | |
| dispatch({ type: "INCREMENT_REFRESH_KEY" }) | |
| dispatch({ | |
| type: "SET_TOAST", | |
| toast: { message: `Deleted ${count} subscription${count !== 1 ? "s" : ""}`, type: "success" }, | |
| }) |
🤖 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 `@subtrack/src/tui/screens/list.tsx` around lines 167 - 178, The bulk-delete
loop in `List` is counting every successful call to `deleteSubscription`, but
that function can return false when nothing was removed. Update the loop to
inspect the boolean result from `deleteSubscription(id)` and only increment
`count` when it returns true, while still skipping thrown failures; keep the
toast message in sync with the actual deletions.
nazozokc
changed the title
The @V3 tag does not exist in the step-security/harden-runner repository, causing all CI jobs to fail at the 'Set up job' stage with: 'Unable to resolve action step-security/harden-runner@v3' Pin to commit SHA f808768 with version comment for immutability and clarity.
Previous pin f808768 (v2.17.0) was only reachable via a tag, not from any branch. GitHub Actions requires the SHA to be reachable from a branch ref. v2.19.4 (9af89fc) is on the main branch and also tagged as v2.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
subtrack/src/index.ts (1)
892-908: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
historyinvocations that provide neither<id>nor--all.With the current wiring,
subtrack history --jsonorsubtrack history --days 30falls through togetAllPriceChanges(...)instead of showing the documented usage. Add a guard here before callinghandleHistory.🤖 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 `@subtrack/src/index.ts` around lines 892 - 908, Add a guard in the `run` handler for `history` before calling `handleHistory` so the command is rejected when neither an `<id>` positional/`--id` nor `--all` is provided. Use the existing `ctx.values` parsing in `subtrack/src/index.ts` to detect this case, print the documented usage or an error via `consola.error`, and return early so `handleHistory` is never invoked without a target.subtrack/src/subscription.ts (1)
340-341: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the subscription update and history insert atomic.
updateSubscription()commits beforewritePriceHistory()runs. If the second write fails or the process exits between them, the subscription change is persisted without the matching history row. Move both writes behind one transactional helper indb.ts. As per coding guidelines, "Usesql.jswithPRAGMA foreign_keys = ONand transactions for multi-step database writes."Also applies to: 447-448
🤖 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 `@subtrack/src/subscription.ts` around lines 340 - 341, The subscription save flow in subscription.ts is splitting the update and history insert into separate commits, so make them atomic by moving both operations behind a single transactional helper in db.ts. Refactor the call site around updateSubscription and writePriceHistory so they run inside one sql.js transaction with PRAGMA foreign_keys = ON, and ensure the helper commits only after both writes succeed or rolls back on failure.Source: Coding guidelines
🧹 Nitpick comments (1)
subtrack/src/__tests__/untested-commands.test.ts (1)
18-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
consola.mockTypes()here instead of a bespoke mock.The repository test rules require the built-in
consolamocking path, and this customvi.mock("consola")will drift from the real logger surface over time. As per coding guidelines, "Mockconsolawithconsola.mockTypes()in tests."🤖 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 `@subtrack/src/__tests__/untested-commands.test.ts` around lines 18 - 39, The test file is using a custom vi.mock for consola, which can drift from the real logger API. Replace the bespoke mock with the built-in consola.mockTypes() setup in the untested-commands test, and keep the existing assertions wired to the mocked logger instance so the test follows the repository’s standard mocking pattern.Source: Coding guidelines
🤖 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 @.github/workflows/scheduled-ci.yml:
- Around line 38-41: The scheduled CI workflow’s OSV-Scanner step is using a
floating google/osv-scanner-action@v2 reference; update that uses entry in the
workflow to a full commit SHA so the action version is fixed and cannot change
unexpectedly. Keep the step name and fail-on-vuln setting as-is, and only
replace the action reference in the Scan dependencies with OSV-Scanner block.
In `@subtrack/src/__tests__/untested-commands.test.ts`:
- Around line 216-223: These notify tests depend on the real current date and
will become flaky as the calendar changes. In the test cases around
handleNotify, freeze the clock with vi.setSystemTime(...) before calling the
handler so the upcoming-bills and dry-run assertions always run against a fixed
date; keep the setup close to the insertSub and await handleNotify calls so the
date-sensitive billingDay logic stays deterministic. Apply the same fix to the
other notify test block mentioned in the review.
In `@subtrack/src/compare.ts`:
- Around line 175-197: The previous-period baseline in compare.ts is using the
latest old price from getAllPriceChanges(), which can be wrong for the requested
previousRange. Update the comparison logic around calcSubTotal,
getAllPriceChanges, and the priceBefore map so it selects the price that was
actually effective during previousRange (for each subscription, pick the
relevant change by date rather than the first newest-first row), then build
previousSubs from that range-aware lookup before calling calcSubTotal.
In `@subtrack/src/mcp.ts`:
- Around line 216-237: `calcSubTotalHelper()` and `calcPreviousTotals()` are not
using the requested comparison period, so `compare` always ends up with
monthly-style totals and may pick the wrong historical baseline. Update the MCP
compare flow in `mcp.ts` so the period passed into the compare request is
preserved through `calcSubTotalHelper`, `calcPreviousTotals`, and the relevant
compare command handlers, and use that period to compute totals and choose the
matching previous price window. Make sure the logic in the compare path
references the existing helpers (`periodFactor`, `convertPrice`,
`calcSubTotalHelper`, `calcPreviousTotals`) but stops hardcoding monthly
normalization when the requested period is yearly or quarterly.
- Around line 641-661: The edit_subscription branch in mcp.ts bypasses the same
validation and history-writing flow used by CLI edits, so updateSubscription()
can silently succeed on missing IDs and skip price_history updates. Refactor
this path to load the existing subscription first and route the update through
the shared checked helper used by the CLI, ensuring zero-row updates fail and
price/currency changes record history consistently.
In `@subtrack/src/subscription.ts`:
- Around line 184-187: `handleList` drops the requested ordering when
`options.tags` is set, because the `tagsSubscription()` branch bypasses
`getSubscriptions(options.sort, options.desc)`. Update the `handleList` flow so
tagged results are still sorted using the existing `sort`/`desc` options, either
by passing those options through to `tagsSubscription` or by applying the same
ordering logic after fetching the tagged subscriptions.
In `@subtrack/src/tui/screens/tools/import-tab.tsx`:
- Around line 36-52: Normalize the parsed CSV values in import-tab’s
row-processing logic before validation, since parseCsvLine() preserves
whitespace and causes isValidCurrency()/isValidCycle() to reject otherwise valid
rows; trim the relevant fields used by writeSubscription, especially fields[0],
fields[1], fields[2], fields[3], and fields[4]. Also update the validation/error
handling in the same loop so each failed row records a clear reason and line
number instead of silently incrementing failed in the short branches or catch
block. Keep the fix localized to the CSV import loop in the import-tab screen.
---
Outside diff comments:
In `@subtrack/src/index.ts`:
- Around line 892-908: Add a guard in the `run` handler for `history` before
calling `handleHistory` so the command is rejected when neither an `<id>`
positional/`--id` nor `--all` is provided. Use the existing `ctx.values` parsing
in `subtrack/src/index.ts` to detect this case, print the documented usage or an
error via `consola.error`, and return early so `handleHistory` is never invoked
without a target.
In `@subtrack/src/subscription.ts`:
- Around line 340-341: The subscription save flow in subscription.ts is
splitting the update and history insert into separate commits, so make them
atomic by moving both operations behind a single transactional helper in db.ts.
Refactor the call site around updateSubscription and writePriceHistory so they
run inside one sql.js transaction with PRAGMA foreign_keys = ON, and ensure the
helper commits only after both writes succeed or rolls back on failure.
---
Nitpick comments:
In `@subtrack/src/__tests__/untested-commands.test.ts`:
- Around line 18-39: The test file is using a custom vi.mock for consola, which
can drift from the real logger API. Replace the bespoke mock with the built-in
consola.mockTypes() setup in the untested-commands test, and keep the existing
assertions wired to the mocked logger instance so the test follows the
repository’s standard mocking pattern.
🪄 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: fff2b021-90e2-4269-8fe2-0b55cd72ba24
📒 Files selected for processing (22)
.github/workflows/app-ci.yml.github/workflows/check.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/labeler.yml.github/workflows/pages.yml.github/workflows/release.yml.github/workflows/renovate-approve.yml.github/workflows/scheduled-ci.yml.github/workflows/scorecard.ymlSECURITY.mdsubtrack/package.jsonsubtrack/src/__tests__/untested-commands.test.tssubtrack/src/commands.tssubtrack/src/compare.tssubtrack/src/index.tssubtrack/src/mcp.tssubtrack/src/notify.tssubtrack/src/subscription.tssubtrack/src/tui/screens/delete.tsxsubtrack/src/tui/screens/list.tsxsubtrack/src/tui/screens/tools/import-tab.tsx
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/check.yml
- .github/workflows/renovate-approve.yml
- .github/workflows/pages.yml
- .github/workflows/scorecard.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- subtrack/src/tui/screens/delete.tsx
- subtrack/package.json
- subtrack/src/notify.ts
- subtrack/src/tui/screens/list.tsx
| - name: Scan dependencies with OSV-Scanner | ||
| uses: google/osv-scanner-action@v2 | ||
| with: | ||
| fail-on-vuln: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow action references:"
rg -n '^\s*-\s+uses:\s+' .github/workflows
echo
echo "Non-SHA third-party action refs:"
rg -nP '^\s*-\s+uses:\s+(?!\./)(?!docker://)[^@]+@(?![0-9a-f]{40}\b)[^ #]+' .github/workflowsRepository: nazozokc/subtrack
Length of output: 4810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "scheduled-ci.yml around the cited lines:"
sed -n '30,50p' .github/workflows/scheduled-ci.yml | cat -n
echo
echo "Full action refs in scheduled-ci.yml:"
rg -n '^\s*-\s+uses:\s+' .github/workflows/scheduled-ci.ymlRepository: nazozokc/subtrack
Length of output: 948
Pin google/osv-scanner-action to a full commit SHA. This is the only floating uses: entry in the workflow; a retagged @v2 can change scheduled CI behavior or execute different code at runtime.
🤖 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 @.github/workflows/scheduled-ci.yml around lines 38 - 41, The scheduled CI
workflow’s OSV-Scanner step is using a floating google/osv-scanner-action@v2
reference; update that uses entry in the workflow to a full commit SHA so the
action version is fixed and cannot change unexpectedly. Keep the step name and
fail-on-vuln setting as-is, and only replace the action reference in the Scan
dependencies with OSV-Scanner block.
| test("handleNotify dry-run shows upcoming bills", async () => { | ||
| // Billing on day 2 from a previous month — next billing will be July 2 (within 7 days from July 1) | ||
| insertSub({ name: "Netflix", price: 1500, billingDay: 2, createdAt: "2026-06-01" }) | ||
|
|
||
| const { handleNotify } = await import("../notify.ts") | ||
| await handleNotify({ days: 7, dryRun: true }) | ||
| expect(logMessages.length).toBeGreaterThan(0) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
These notify tests are tied to the real calendar and will start flaking.
Both assertions only hold around July 1, 2026. handleNotify() uses the current date, so the “billing day 2 is within 7 days” case stops being true once the suite runs later in July 2026, and the days: 0 case flips as soon as the test date reaches the 10th of any month. Freeze time with vi.setSystemTime(...) before invoking the handler.
Also applies to: 243-250
🤖 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 `@subtrack/src/__tests__/untested-commands.test.ts` around lines 216 - 223,
These notify tests depend on the real current date and will become flaky as the
calendar changes. In the test cases around handleNotify, freeze the clock with
vi.setSystemTime(...) before calling the handler so the upcoming-bills and
dry-run assertions always run against a fixed date; keep the setup close to the
insertSub and await handleNotify calls so the date-sensitive billingDay logic
stays deterministic. Apply the same fix to the other notify test block mentioned
in the review.
| // Current period uses current prices | ||
| const currentTotals = calcSubTotal(activeSubs, rates, targetCurrency) | ||
| // Same subscriptions for previous period (they were active then too) | ||
| const previousTotals = calcSubTotal(activeSubs, rates, targetCurrency) | ||
|
|
||
| // Previous period — estimate from price history when available | ||
| const priceChanges = getAllPriceChanges() | ||
| const priceBefore: Record<number, { price: number; currency: string }> = {} | ||
| for (const change of priceChanges) { | ||
| if (change.oldPrice !== null && !priceBefore[change.subscriptionId]) { | ||
| priceBefore[change.subscriptionId] = { | ||
| price: change.oldPrice, | ||
| currency: change.oldCurrency ?? change.newCurrency, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const previousSubs = activeSubs.map((s) => { | ||
| const prev = priceBefore[s.id] | ||
| if (prev) { | ||
| return { ...s, price: prev.price, currency: prev.currency } | ||
| } | ||
| return s | ||
| }) | ||
| const previousTotals = calcSubTotal(previousSubs, rates, targetCurrency) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Choose the price that was effective during previousRange, not just the latest old price.
getAllPriceChanges() is global and newest-first, and this map keeps the first row per subscription. That reconstructs the price immediately before the most recent edit, not the price that was active in the specific previous period being compared. Any subscription with multiple edits—or with a change after the previous window—will produce the wrong baseline.
🤖 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 `@subtrack/src/compare.ts` around lines 175 - 197, The previous-period baseline
in compare.ts is using the latest old price from getAllPriceChanges(), which can
be wrong for the requested previousRange. Update the comparison logic around
calcSubTotal, getAllPriceChanges, and the priceBefore map so it selects the
price that was actually effective during previousRange (for each subscription,
pick the relevant change by date rather than the first newest-first row), then
build previousSubs from that range-aware lookup before calling calcSubTotal.
| function calcSubTotalHelper( | ||
| subs: SharedArgs[], | ||
| rates: FxRates | null, | ||
| targetCurrency: Currency | undefined, | ||
| ): CcyTotals { | ||
| const totals: CcyTotals = {} | ||
| for (const sub of subs) { | ||
| if (sub.status === "cancelled") continue | ||
| const monthly = sub.price * periodFactor(sub.cycle, "monthly") | ||
| if (targetCurrency && rates) { | ||
| try { | ||
| const converted = convertPrice(monthly, sub.currency, targetCurrency, rates.rates) | ||
| totals[targetCurrency] = (totals[targetCurrency] ?? 0) + converted | ||
| } catch { | ||
| totals[sub.currency] = (totals[sub.currency] ?? 0) + monthly | ||
| } | ||
| } else { | ||
| totals[sub.currency] = (totals[sub.currency] ?? 0) + monthly | ||
| } | ||
| } | ||
| return totals | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The MCP compare result is not actually period-aware.
calcSubTotalHelper() always normalizes to monthly totals, and calcPreviousTotals() just takes the latest oldPrice per subscription without checking whether that change belonged to the previous month/quarter/year being compared. So period: "yearly" still reports monthly numbers and can compare against the wrong baseline.
Also applies to: 239-278, 745-792
🤖 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 `@subtrack/src/mcp.ts` around lines 216 - 237, `calcSubTotalHelper()` and
`calcPreviousTotals()` are not using the requested comparison period, so
`compare` always ends up with monthly-style totals and may pick the wrong
historical baseline. Update the MCP compare flow in `mcp.ts` so the period
passed into the compare request is preserved through `calcSubTotalHelper`,
`calcPreviousTotals`, and the relevant compare command handlers, and use that
period to compute totals and choose the matching previous price window. Make
sure the logic in the compare path references the existing helpers
(`periodFactor`, `convertPrice`, `calcSubTotalHelper`, `calcPreviousTotals`) but
stops hardcoding monthly normalization when the requested period is yearly or
quarterly.
| case "edit_subscription": { | ||
| if (args?.id === undefined) { | ||
| return { | ||
| content: [{ type: "text", text: "id is required" }], | ||
| isError: true, | ||
| } | ||
| } | ||
| const editFields: Partial<AddSharedArgs> = {} | ||
| if (args.name !== undefined) editFields.name = String(args.name) | ||
| if (args.price !== undefined) editFields.price = Number(args.price) | ||
| if (args.currency !== undefined) editFields.currency = String(args.currency) | ||
| if (args.cycle !== undefined) editFields.cycle = String(args.cycle) as Cycle | ||
| if (args.status !== undefined) editFields.status = String(args.status) as Status | ||
| if (args.billingDay !== undefined) editFields.billingDay = Number(args.billingDay) | ||
| if (args.paymentMethod !== undefined) editFields.paymentMethod = String(args.paymentMethod) | ||
| if (args.notes !== undefined) editFields.notes = String(args.notes) | ||
| if (args.tags !== undefined) { | ||
| editFields.tags = String(args.tags).split(",").map((t: string) => t.trim()).filter(Boolean) | ||
| } | ||
| const success = updateSubscription(Number(args.id), editFields) | ||
| return { content: [{ type: "text", text: JSON.stringify({ success }) }] } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Route MCP edits through the same checked, history-writing path as CLI edits.
This branch calls updateSubscription() directly, so price/currency changes never hit price_history, and a nonexistent ID can still come back as { "success": true } because updateSubscription() does not detect zero-row updates. Load the current row first and perform the update + history write through one shared helper.
🤖 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 `@subtrack/src/mcp.ts` around lines 641 - 661, The edit_subscription branch in
mcp.ts bypasses the same validation and history-writing flow used by CLI edits,
so updateSubscription() can silently succeed on missing IDs and skip
price_history updates. Refactor this path to load the existing subscription
first and route the update through the shared checked helper used by the CLI,
ensuring zero-row updates fail and price/currency changes record history
consistently.
| export async function handleList(options: { currency?: string; sort?: string; desc?: boolean; api?: boolean; notes?: boolean; method?: boolean; tags?: string }) { | ||
| const list = options.tags | ||
| ? tagsSubscription(options.tags.split(",").map((t) => t.trim())) | ||
| : getSubscriptions(options.sort, options.desc) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve sort/desc when --tags is used.
This branch skips getSubscriptions(options.sort, options.desc), and tagsSubscription() does not apply any ordering, so subtrack list --tags foo --sort price --desc returns an arbitrary order instead of the requested sort.
🤖 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 `@subtrack/src/subscription.ts` around lines 184 - 187, `handleList` drops the
requested ordering when `options.tags` is set, because the `tagsSubscription()`
branch bypasses `getSubscriptions(options.sort, options.desc)`. Update the
`handleList` flow so tagged results are still sorted using the existing
`sort`/`desc` options, either by passing those options through to
`tagsSubscription` or by applying the same ordering logic after fetching the
tagged subscriptions.
| const fields = parseCsvLine(lines[i]) | ||
| if (fields.length < 5) { failed++; continue } | ||
| if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue } | ||
| const price = Number(fields[3]) | ||
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | ||
| db.run( | ||
| "INSERT INTO subscriptions (name, price, currency, cycle, status, created_at) VALUES (?, ?, ?, ?, 'active', date('now'))", | ||
| [fields[0].trim(), price, fields[4], fields[1]], | ||
| ) | ||
| const idRow = db.exec("SELECT last_insert_rowid() AS id") | ||
| if (idRow.length > 0 && idRow[0].values.length > 0) { | ||
| const subId = Number(idRow[0].values[0][0]) | ||
| const tags = fields[2].split(";").map((t) => t.trim()).filter(Boolean) | ||
| for (const t of tags) { | ||
| db.run("INSERT OR IGNORE INTO tags (name) VALUES (?)", [t]) | ||
| const tagRow = db.exec("SELECT id FROM tags WHERE name = ?", [t]) | ||
| if (tagRow.length > 0 && tagRow[0].values.length > 0) { | ||
| db.run("INSERT INTO subscription_tags (subscription_id, tag_id) VALUES (?, ?)", [subId, Number(tagRow[0].values[0][0])]) | ||
| } | ||
| } | ||
| } | ||
| writeSubscription({ | ||
| name: fields[0].trim(), | ||
| price, | ||
| currency: fields[4], | ||
| cycle: fields[1], | ||
| tags: fields[2].split(";").map((t) => t.trim()).filter(Boolean), | ||
| status: "active", | ||
| }) | ||
| success++ | ||
| } catch { | ||
| failed++ | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize CSV fields and report every row failure.
parseCsvLine() preserves whitespace, while isValidCurrency() and isValidCycle() require exact values, so rows like monthly, ..., USD with delimiter spaces are rejected. The short validation branches and catch block also hide the failing line/reason.
Proposed fix
- const fields = parseCsvLine(lines[i])
- if (fields.length < 5) { failed++; continue }
- if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue }
- const price = Number(fields[3])
- if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue }
+ const fields = parseCsvLine(lines[i]).map((field) => field.trim())
+ if (fields.length < 5) { failed++; errors.push(`Line ${i + 1}: expected at least 5 fields`); continue }
+ if (!isValidCycle(fields[1])) { failed++; errors.push(`Line ${i + 1}: invalid cycle "${fields[1]}"`); continue }
+ if (!isValidCurrency(fields[4])) { failed++; errors.push(`Line ${i + 1}: invalid currency "${fields[4]}"`); continue }
+ const price = Number(fields[3])
+ if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue }
writeSubscription({
- name: fields[0].trim(),
+ name: fields[0],
price,
currency: fields[4],
cycle: fields[1],
tags: fields[2].split(";").map((t) => t.trim()).filter(Boolean),
status: "active",
})
success++
- } catch {
+ } catch (error) {
failed++
+ errors.push(`Line ${i + 1}: ${error instanceof Error ? error.message : String(error)}`)
}📝 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.
| const fields = parseCsvLine(lines[i]) | |
| if (fields.length < 5) { failed++; continue } | |
| if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue } | |
| const price = Number(fields[3]) | |
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | |
| db.run( | |
| "INSERT INTO subscriptions (name, price, currency, cycle, status, created_at) VALUES (?, ?, ?, ?, 'active', date('now'))", | |
| [fields[0].trim(), price, fields[4], fields[1]], | |
| ) | |
| const idRow = db.exec("SELECT last_insert_rowid() AS id") | |
| if (idRow.length > 0 && idRow[0].values.length > 0) { | |
| const subId = Number(idRow[0].values[0][0]) | |
| const tags = fields[2].split(";").map((t) => t.trim()).filter(Boolean) | |
| for (const t of tags) { | |
| db.run("INSERT OR IGNORE INTO tags (name) VALUES (?)", [t]) | |
| const tagRow = db.exec("SELECT id FROM tags WHERE name = ?", [t]) | |
| if (tagRow.length > 0 && tagRow[0].values.length > 0) { | |
| db.run("INSERT INTO subscription_tags (subscription_id, tag_id) VALUES (?, ?)", [subId, Number(tagRow[0].values[0][0])]) | |
| } | |
| } | |
| } | |
| writeSubscription({ | |
| name: fields[0].trim(), | |
| price, | |
| currency: fields[4], | |
| cycle: fields[1], | |
| tags: fields[2].split(";").map((t) => t.trim()).filter(Boolean), | |
| status: "active", | |
| }) | |
| success++ | |
| } catch { | |
| failed++ | |
| } | |
| const fields = parseCsvLine(lines[i]).map((field) => field.trim()) | |
| if (fields.length < 5) { failed++; errors.push(`Line ${i + 1}: expected at least 5 fields`); continue } | |
| if (!isValidCycle(fields[1])) { failed++; errors.push(`Line ${i + 1}: invalid cycle "${fields[1]}"`); continue } | |
| if (!isValidCurrency(fields[4])) { failed++; errors.push(`Line ${i + 1}: invalid currency "${fields[4]}"`); continue } | |
| const price = Number(fields[3]) | |
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | |
| writeSubscription({ | |
| name: fields[0], | |
| price, | |
| currency: fields[4], | |
| cycle: fields[1], | |
| tags: fields[2].split(";").map((t) => t.trim()).filter(Boolean), | |
| status: "active", | |
| }) | |
| success++ | |
| } catch (error) { | |
| failed++ | |
| errors.push(`Line ${i + 1}: ${error instanceof Error ? error.message : String(error)}`) | |
| } |
🤖 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 `@subtrack/src/tui/screens/tools/import-tab.tsx` around lines 36 - 52,
Normalize the parsed CSV values in import-tab’s row-processing logic before
validation, since parseCsvLine() preserves whitespace and causes
isValidCurrency()/isValidCycle() to reject otherwise valid rows; trim the
relevant fields used by writeSubscription, especially fields[0], fields[1],
fields[2], fields[3], and fields[4]. Also update the validation/error handling
in the same loop so each failed row records a clear reason and line number
instead of silently incrementing failed in the short branches or catch block.
Keep the fix localized to the CSV import loop in the import-tab screen.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
subtrack/src/timeline.ts (1)
67-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated month-window computation between
calcMonthlyTotalsandcalcMonthlyTotalsByCategory.Both functions independently derive the same set of month dates/labels using slightly different loop directions and offset math (Lines 39-44 vs 88-90). This works today, but any future change to the month-window logic (e.g., timezone handling, off-by-one) risks drifting between the two implementations.
Consider extracting a shared
getMonthWindows(months)helper returning{ year, month, label, monthEnd }[]and reusing it in both functions.🤖 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 `@subtrack/src/timeline.ts` around lines 67 - 109, The month-window logic is duplicated between calcMonthlyTotals and calcMonthlyTotalsByCategory, so the two implementations can drift over time. Extract a shared getMonthWindows(months) helper that builds the month sequence once (including year/month, label, and monthEnd) and use it in both calcMonthlyTotals and calcMonthlyTotalsByCategory. Keep the existing behavior by switching both loops to iterate the shared windows instead of recomputing dates inline.subtrack/src/__tests__/timeline.test.ts (1)
108-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions don't verify the behavior the test names claim.
"excludes cancelled subscriptions" and "handles yearly cycle subscriptions" both only assert
loggedLines.some(l => l.includes("Monthly spending")), which just confirms the chart header is printed — it doesn't check that the cancelled subscription's price was actually excluded from totals, or that the yearly-cycle math ($120/yr → $10/mo per the inline comment) produced the expected value.Consider asserting on the JSON output path (
{ json: true }) to check the actual computed totals (e.g.,data.total === 0for the cancelled-only case, or a specific per-month value for the yearly case) instead of just checking for header text.🤖 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 `@subtrack/src/__tests__/timeline.test.ts` around lines 108 - 146, The two timeline tests are only checking for the chart header and do not verify the behavior their names describe. Update the assertions in timelineModule.handleTimeline tests to inspect the actual computed output, preferably via the JSON path with { json: true }, so "excludes cancelled subscriptions" validates the totals are zero/omits cancelled entries and "handles yearly cycle subscriptions" validates the monthly conversion result from the Annual seedSub case. Use the existing timelineModule.handleTimeline and seedSub setup to assert on the returned data rather than loggedLines containing "Monthly spending".subtrack/src/index.ts (1)
173-180: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the semicolons in this block
namesis already declared as a positional array, so the cast is fine. The remaining nit is that this block is the odd one out on semicolon style; align it with the rest ofsubtrack/src.🤖 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 `@subtrack/src/index.ts` around lines 173 - 180, The run handler in the command setup is the odd one out on semicolon style; update the block in the `run` callback to match the rest of `subtrack/src` by removing the trailing semicolons from the local declarations and calls. Keep the existing `ctx.values.names` cast and the `consola.error`/`handleTags` flow intact, just make the syntax consistent with surrounding code.
🤖 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 `@subtrack/src/__tests__/timeline.test.ts`:
- Around line 79-207: The handleTimeline test suite is monkey-patching consola
methods directly instead of using the approved mocking helper. Update the tests
in handleTimeline to use consola.mockTypes() for info/log/error assertions, and
remove the manual reassignment/restoration of consola.info, consola.log, and
consola.error in each test case. Keep the existing expectations the same, but
route all console interception through the mockTypes API so the suite follows
the testing guideline consistently.
- Around line 173-195: The “respects createdAt date for subscription inclusion”
test only checks the number of timeline entries, so it never verifies the
createdAt cutoff behavior. Update the test in timeline.test.ts to assert the
actual per-month totals returned by handleTimeline, using the seeded “Old Sub”
and “New Sub” to confirm months before lastMonthStr include only the older
subscription and months at/after that month include both. Reference
handleTimeline and the JSON output shape when adding assertions so the test
validates exclusion based on createdAt rather than just entry count.
In `@subtrack/src/timeline.ts`:
- Around line 189-242: The exported public API handleTimeline in timeline.ts is
missing JSDoc, unlike the internal helpers calcMonthlyTotals and
calcMonthlyTotalsByCategory. Add a concise JSDoc block directly above
handleTimeline describing its purpose and the TimelineOptions parameter/behavior
so the public entrypoint is documented per guidelines.
- Around line 46-56: The monthly total in calcMonthlyTotals is aggregating raw
sub.price values across different currencies, while the downstream chart and
JSON output present the result as USD. Update the calcMonthlyTotals logic in
timeline.ts so each subscription is normalized to a common currency before being
added, or otherwise group totals by currency consistently; use the existing
calcMonthlyTotals and periodFactor flow to locate the change and ensure the
formatted output matches the underlying currency data.
---
Nitpick comments:
In `@subtrack/src/__tests__/timeline.test.ts`:
- Around line 108-146: The two timeline tests are only checking for the chart
header and do not verify the behavior their names describe. Update the
assertions in timelineModule.handleTimeline tests to inspect the actual computed
output, preferably via the JSON path with { json: true }, so "excludes cancelled
subscriptions" validates the totals are zero/omits cancelled entries and
"handles yearly cycle subscriptions" validates the monthly conversion result
from the Annual seedSub case. Use the existing timelineModule.handleTimeline and
seedSub setup to assert on the returned data rather than loggedLines containing
"Monthly spending".
In `@subtrack/src/index.ts`:
- Around line 173-180: The run handler in the command setup is the odd one out
on semicolon style; update the block in the `run` callback to match the rest of
`subtrack/src` by removing the trailing semicolons from the local declarations
and calls. Keep the existing `ctx.values.names` cast and the
`consola.error`/`handleTags` flow intact, just make the syntax consistent with
surrounding code.
In `@subtrack/src/timeline.ts`:
- Around line 67-109: The month-window logic is duplicated between
calcMonthlyTotals and calcMonthlyTotalsByCategory, so the two implementations
can drift over time. Extract a shared getMonthWindows(months) helper that builds
the month sequence once (including year/month, label, and monthEnd) and use it
in both calcMonthlyTotals and calcMonthlyTotalsByCategory. Keep the existing
behavior by switching both loops to iterate the shared windows instead of
recomputing dates inline.
🪄 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: 09b13860-3ef8-433f-a55b-9d74e6aca22b
📒 Files selected for processing (16)
.github/workflows/app-ci.yml.github/workflows/check.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/labeler.yml.github/workflows/pages.yml.github/workflows/release.yml.github/workflows/renovate-approve.yml.github/workflows/scheduled-ci.yml.github/workflows/scorecard.ymlSECURITY.mdsubtrack/package.jsonsubtrack/src/__tests__/timeline.test.tssubtrack/src/commands.tssubtrack/src/index.tssubtrack/src/timeline.ts
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/pages.yml
- SECURITY.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/dependency-review.yml
- .github/workflows/scorecard.yml
- .github/workflows/labeler.yml
- .github/workflows/scheduled-ci.yml
- subtrack/package.json
- subtrack/src/commands.ts
| describe("handleTimeline", () => { | ||
| test("shows info when no subscriptions exist", () => { | ||
| const infoLogs: string[] = [] | ||
| const origInfo = consola.info | ||
| consola.info = (msg: unknown) => infoLogs.push(String(msg)) | ||
|
|
||
| timelineModule.handleTimeline() | ||
|
|
||
| expect(infoLogs.length).toBeGreaterThan(0) | ||
| expect(infoLogs[0]).toContain("No subscriptions") | ||
| consola.info = origInfo | ||
| }) | ||
|
|
||
| test("returns chart output for active subscriptions", () => { | ||
| seedSub("Netflix", 1549, "monthly") | ||
| seedSub("Spotify", 999, "monthly") | ||
|
|
||
| const loggedLines: string[] = [] | ||
| const origLog = consola.log | ||
| consola.log = (msg: unknown) => loggedLines.push(String(msg)) | ||
|
|
||
| timelineModule.handleTimeline({ months: 3 }) | ||
|
|
||
| expect(loggedLines.length).toBeGreaterThan(0) | ||
| expect(loggedLines.some((l) => l.includes("Monthly spending"))).toBe(true) | ||
|
|
||
| consola.log = origLog | ||
| }) | ||
|
|
||
| test("excludes cancelled subscriptions", () => { | ||
| seedSub("Netflix", 1549, "monthly", "active") | ||
| seedSub("Cancelled Thing", 5000, "monthly", "cancelled") | ||
|
|
||
| // With only the cancelled sub, should show no meaningful data | ||
| const loggedLines: string[] = [] | ||
| const origLog = consola.log | ||
| const origInfo = consola.info | ||
| consola.log = (msg: unknown) => loggedLines.push(String(msg)) | ||
| consola.info = () => {} | ||
|
|
||
| // Clear and add only cancelled | ||
| testDb.run("DELETE FROM subscriptions") | ||
| seedSub("Cancelled Thing", 5000, "monthly", "cancelled") | ||
|
|
||
| timelineModule.handleTimeline({ months: 3 }) | ||
|
|
||
| // Should still produce chart output (activeSubs is just empty but that's handled) | ||
| // Actually: zero active subs, getSubscriptions returns the cancelled one, | ||
| // activeSubs filter means empty array, calcMonthlyTotals returns all zeros | ||
| expect(loggedLines.some((l) => l.includes("Monthly spending"))).toBe(true) | ||
|
|
||
| consola.log = origLog | ||
| consola.info = origInfo | ||
| }) | ||
|
|
||
| test("handles yearly cycle subscriptions", () => { | ||
| // $120/yr = $10/mo | ||
| seedSub("Annual", 12000, "yearly") | ||
|
|
||
| const loggedLines: string[] = [] | ||
| const origLog = consola.log | ||
| consola.log = (msg: unknown) => loggedLines.push(String(msg)) | ||
|
|
||
| timelineModule.handleTimeline({ months: 3 }) | ||
|
|
||
| expect(loggedLines.some((l) => l.includes("Monthly spending"))).toBe(true) | ||
| consola.log = origLog | ||
| }) | ||
|
|
||
| test("outputs JSON with --json flag", () => { | ||
| seedSub("Netflix", 1549, "monthly") | ||
|
|
||
| const jsonOutputs: string[] = [] | ||
| const origWrite = process.stdout.write.bind(process.stdout) | ||
| const mockWrite = ((chunk: unknown) => { | ||
| jsonOutputs.push(String(chunk)) | ||
| return true | ||
| }) as typeof process.stdout.write | ||
| process.stdout.write = mockWrite | ||
|
|
||
| timelineModule.handleTimeline({ json: true }) | ||
|
|
||
| process.stdout.write = origWrite | ||
|
|
||
| expect(jsonOutputs.length).toBeGreaterThan(0) | ||
| const data = JSON.parse(jsonOutputs[0]) | ||
| expect(data).toHaveProperty("months") | ||
| expect(data).toHaveProperty("entries") | ||
| expect(data.months).toBe(12) | ||
| expect(data.entries.length).toBe(12) | ||
| expect(data.entries[0]).toHaveProperty("month") | ||
| expect(data.entries[0]).toHaveProperty("total") | ||
| }) | ||
|
|
||
| test("respects createdAt date for subscription inclusion", () => { | ||
| const now = new Date() | ||
| const lastMonth = new Date(now.getFullYear(), now.getMonth() - 1, 15) | ||
| const lastMonthStr = `${lastMonth.getFullYear()}-${String(lastMonth.getMonth() + 1).padStart(2, "0")}-${String(lastMonth.getDate()).padStart(2, "0")}` | ||
|
|
||
| seedSub("Old Sub", 1000, "monthly", "active", "2020-01-01") | ||
| seedSub("New Sub", 2000, "monthly", "active", lastMonthStr) | ||
|
|
||
| const jsonOutputs: string[] = [] | ||
| const origWrite = process.stdout.write.bind(process.stdout) | ||
| const mockWrite = ((chunk: unknown) => { | ||
| jsonOutputs.push(String(chunk)) | ||
| return true | ||
| }) as typeof process.stdout.write | ||
| process.stdout.write = mockWrite | ||
|
|
||
| timelineModule.handleTimeline({ months: 12, json: true }) | ||
|
|
||
| process.stdout.write = origWrite | ||
|
|
||
| const data = JSON.parse(jsonOutputs[0]) | ||
| expect(data.entries.length).toBe(12) | ||
| }) | ||
|
|
||
| test("errors on invalid months", () => { | ||
| const errLogs: string[] = [] | ||
| const origError = consola.error | ||
| consola.error = (msg: unknown) => errLogs.push(String(msg)) | ||
|
|
||
| timelineModule.handleTimeline({ months: 0 }) | ||
|
|
||
| expect(errLogs.length).toBeGreaterThan(0) | ||
| expect(errLogs[0]).toContain("positive integer") | ||
| consola.error = origError | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Tests manually monkey-patch consola methods instead of using consola.mockTypes().
Every test in this suite reassigns consola.info/consola.log/consola.error directly and restores originals manually (Lines 82-89, 97-105, 113-131, 138-145, 199-206), rather than using consola.mockTypes().
As per coding guidelines, "Mock consola with consola.mockTypes() in tests."
🤖 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 `@subtrack/src/__tests__/timeline.test.ts` around lines 79 - 207, The
handleTimeline test suite is monkey-patching consola methods directly instead of
using the approved mocking helper. Update the tests in handleTimeline to use
consola.mockTypes() for info/log/error assertions, and remove the manual
reassignment/restoration of consola.info, consola.log, and consola.error in each
test case. Keep the existing expectations the same, but route all console
interception through the mockTypes API so the suite follows the testing
guideline consistently.
Source: Coding guidelines
| test("respects createdAt date for subscription inclusion", () => { | ||
| const now = new Date() | ||
| const lastMonth = new Date(now.getFullYear(), now.getMonth() - 1, 15) | ||
| const lastMonthStr = `${lastMonth.getFullYear()}-${String(lastMonth.getMonth() + 1).padStart(2, "0")}-${String(lastMonth.getDate()).padStart(2, "0")}` | ||
|
|
||
| seedSub("Old Sub", 1000, "monthly", "active", "2020-01-01") | ||
| seedSub("New Sub", 2000, "monthly", "active", lastMonthStr) | ||
|
|
||
| const jsonOutputs: string[] = [] | ||
| const origWrite = process.stdout.write.bind(process.stdout) | ||
| const mockWrite = ((chunk: unknown) => { | ||
| jsonOutputs.push(String(chunk)) | ||
| return true | ||
| }) as typeof process.stdout.write | ||
| process.stdout.write = mockWrite | ||
|
|
||
| timelineModule.handleTimeline({ months: 12, json: true }) | ||
|
|
||
| process.stdout.write = origWrite | ||
|
|
||
| const data = JSON.parse(jsonOutputs[0]) | ||
| expect(data.entries.length).toBe(12) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
"respects createdAt date" test doesn't actually verify createdAt-based exclusion.
The test seeds an "Old Sub" (created 2020) and a "New Sub" (created last month) and only asserts data.entries.length === 12 — that assertion passes unconditionally regardless of whether the createdAt cutoff logic works, since entries.length is always months for any valid input. The critical behavior described in the docstring (Lines 29-30 of timeline.ts: subscriptions excluded from months before they existed) is left unverified.
As per coding guidelines, "Write unit tests for all functions and critical code paths" — this test's assertions should validate the actual per-month totals (e.g., confirm months before lastMonthStr reflect only "Old Sub"'s contribution, and the cutoff month onward includes both).
🤖 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 `@subtrack/src/__tests__/timeline.test.ts` around lines 173 - 195, The
“respects createdAt date for subscription inclusion” test only checks the number
of timeline entries, so it never verifies the createdAt cutoff behavior. Update
the test in timeline.test.ts to assert the actual per-month totals returned by
handleTimeline, using the seeded “Old Sub” and “New Sub” to confirm months
before lastMonthStr include only the older subscription and months at/after that
month include both. Reference handleTimeline and the JSON output shape when
adding assertions so the test validates exclusion based on createdAt rather than
just entry count.
| let total = 0 | ||
| for (const sub of subs) { | ||
| // Skip cancelled subscriptions | ||
| if (sub.status === "cancelled") continue | ||
|
|
||
| // Skip if subscription was created after this month | ||
| const created = new Date(sub.createdAt) | ||
| if (created > monthEnd) continue | ||
|
|
||
| total += sub.price * periodFactor(sub.cycle, "monthly") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the codebase has an existing FX conversion helper that should be reused here.
rg -nP -i '(convert|fx|exchange).*(price|rate|currency)' subtrack/src --type=ts -C2Repository: nazozokc/subtrack
Length of output: 24108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect timeline implementation and related formatting around the reported lines.
wc -l subtrack/src/timeline.ts
sed -n '1,220p' subtrack/src/timeline.ts
# Inspect the FX helper to compare intended currency handling.
sed -n '1,220p' subtrack/src/fx.tsRepository: nazozokc/subtrack
Length of output: 8070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared subscription type and the timeline command/tests for currency handling.
sed -n '1,220p' subtrack/src/types.ts
sed -n '220,242p' subtrack/src/timeline.ts
rg -n "handleTimeline|calcMonthlyTotals|timeline" subtrack/src/__tests__ subtrack/src -g '*timeline*' -g '*.test.ts' -C2Repository: nazozokc/subtrack
Length of output: 14534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared subscription type and the timeline command/tests for currency handling.
sed -n '1,220p' subtrack/src/types.ts
sed -n '220,242p' subtrack/src/timeline.ts
rg -n "handleTimeline|calcMonthlyTotals|timeline" subtrack/src/__tests__ subtrack/src -g '*.test.ts' -C2Repository: nazozokc/subtrack
Length of output: 12191
Normalize timeline totals before formatting as USD in subtrack/src/timeline.ts
calcMonthlyTotals adds raw sub.price values across currencies, then the chart/JSON paths label the aggregate as USD. Mixed-currency timelines will report the wrong total; convert each subscription to a common currency first, or split the timeline by currency.
🤖 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 `@subtrack/src/timeline.ts` around lines 46 - 56, The monthly total in
calcMonthlyTotals is aggregating raw sub.price values across different
currencies, while the downstream chart and JSON output present the result as
USD. Update the calcMonthlyTotals logic in timeline.ts so each subscription is
normalized to a common currency before being added, or otherwise group totals by
currency consistently; use the existing calcMonthlyTotals and periodFactor flow
to locate the change and ensure the formatted output matches the underlying
currency data.
Source: Coding guidelines
| export function handleTimeline(options: TimelineOptions = {}): void { | ||
| const months = options.months ?? 12 | ||
|
|
||
| if (months < 1) { | ||
| consola.error("months must be a positive integer") | ||
| return | ||
| } | ||
|
|
||
| const subs = getSubscriptions() | ||
|
|
||
| if (subs.length === 0) { | ||
| consola.info("No subscriptions found") | ||
| return | ||
| } | ||
|
|
||
| // Filter to non-cancelled for active analysis | ||
| const activeSubs = subs.filter((s) => s.status !== "cancelled") | ||
|
|
||
| if (options.json) { | ||
| const totals = calcMonthlyTotals(activeSubs, months) | ||
| const data = { | ||
| months, | ||
| total: totals.reduce((s, t) => s + t.total, 0), | ||
| average: | ||
| totals.length > 0 | ||
| ? Math.round(totals.reduce((s, t) => s + t.total, 0) / totals.length) | ||
| : 0, | ||
| entries: totals.map((t) => ({ | ||
| month: t.label, | ||
| total: t.total, | ||
| })), | ||
| } | ||
| if (options.categories) { | ||
| const { categories } = calcMonthlyTotalsByCategory(activeSubs, months) | ||
| const catData: Record<string, number[]> = {} | ||
| for (const cd of categories) { | ||
| catData[cd.category] = cd.months | ||
| } | ||
| ;(data as Record<string, unknown>).categories = catData | ||
| } | ||
| process.stdout.write(JSON.stringify(data, null, 2) + "\n") | ||
| return | ||
| } | ||
|
|
||
| if (options.categories) { | ||
| const { totals, categories } = calcMonthlyTotalsByCategory(activeSubs, months) | ||
| consola.log(renderBarChart(totals)) | ||
| consola.log("") | ||
| consola.log(renderCategoryChart(totals, categories)) | ||
| } else { | ||
| const totals = calcMonthlyTotals(activeSubs, months) | ||
| consola.log(renderBarChart(totals)) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
handleTimeline (public API) lacks a JSDoc comment.
Internal helpers (calcMonthlyTotals, calcMonthlyTotalsByCategory) have JSDoc, but the exported entrypoint itself does not.
As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript."
📝 Suggested JSDoc
+/**
+ * Handle the `timeline` CLI command: render or emit monthly spending totals,
+ * optionally broken down by category and/or as JSON.
+ */
export function handleTimeline(options: TimelineOptions = {}): void {📝 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.
| export function handleTimeline(options: TimelineOptions = {}): void { | |
| const months = options.months ?? 12 | |
| if (months < 1) { | |
| consola.error("months must be a positive integer") | |
| return | |
| } | |
| const subs = getSubscriptions() | |
| if (subs.length === 0) { | |
| consola.info("No subscriptions found") | |
| return | |
| } | |
| // Filter to non-cancelled for active analysis | |
| const activeSubs = subs.filter((s) => s.status !== "cancelled") | |
| if (options.json) { | |
| const totals = calcMonthlyTotals(activeSubs, months) | |
| const data = { | |
| months, | |
| total: totals.reduce((s, t) => s + t.total, 0), | |
| average: | |
| totals.length > 0 | |
| ? Math.round(totals.reduce((s, t) => s + t.total, 0) / totals.length) | |
| : 0, | |
| entries: totals.map((t) => ({ | |
| month: t.label, | |
| total: t.total, | |
| })), | |
| } | |
| if (options.categories) { | |
| const { categories } = calcMonthlyTotalsByCategory(activeSubs, months) | |
| const catData: Record<string, number[]> = {} | |
| for (const cd of categories) { | |
| catData[cd.category] = cd.months | |
| } | |
| ;(data as Record<string, unknown>).categories = catData | |
| } | |
| process.stdout.write(JSON.stringify(data, null, 2) + "\n") | |
| return | |
| } | |
| if (options.categories) { | |
| const { totals, categories } = calcMonthlyTotalsByCategory(activeSubs, months) | |
| consola.log(renderBarChart(totals)) | |
| consola.log("") | |
| consola.log(renderCategoryChart(totals, categories)) | |
| } else { | |
| const totals = calcMonthlyTotals(activeSubs, months) | |
| consola.log(renderBarChart(totals)) | |
| } | |
| } | |
| /** | |
| * Handle the `timeline` CLI command: render or emit monthly spending totals, | |
| * optionally broken down by category and/or as JSON. | |
| */ | |
| export function handleTimeline(options: TimelineOptions = {}): void { | |
| const months = options.months ?? 12 | |
| if (months < 1) { | |
| consola.error("months must be a positive integer") | |
| return | |
| } | |
| const subs = getSubscriptions() | |
| if (subs.length === 0) { | |
| consola.info("No subscriptions found") | |
| return | |
| } | |
| // Filter to non-cancelled for active analysis | |
| const activeSubs = subs.filter((s) => s.status !== "cancelled") | |
| if (options.json) { | |
| const totals = calcMonthlyTotals(activeSubs, months) | |
| const data = { | |
| months, | |
| total: totals.reduce((s, t) => s + t.total, 0), | |
| average: | |
| totals.length > 0 | |
| ? Math.round(totals.reduce((s, t) => s + t.total, 0) / totals.length) | |
| : 0, | |
| entries: totals.map((t) => ({ | |
| month: t.label, | |
| total: t.total, | |
| })), | |
| } | |
| if (options.categories) { | |
| const { categories } = calcMonthlyTotalsByCategory(activeSubs, months) | |
| const catData: Record<string, number[]> = {} | |
| for (const cd of categories) { | |
| catData[cd.category] = cd.months | |
| } | |
| ;(data as Record<string, unknown>).categories = catData | |
| } | |
| process.stdout.write(JSON.stringify(data, null, 2) + "\n") | |
| return | |
| } | |
| if (options.categories) { | |
| const { totals, categories } = calcMonthlyTotalsByCategory(activeSubs, months) | |
| consola.log(renderBarChart(totals)) | |
| consola.log("") | |
| consola.log(renderCategoryChart(totals, categories)) | |
| } else { | |
| const totals = calcMonthlyTotals(activeSubs, months) | |
| consola.log(renderBarChart(totals)) | |
| } | |
| } |
🤖 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 `@subtrack/src/timeline.ts` around lines 189 - 242, The exported public API
handleTimeline in timeline.ts is missing JSDoc, unlike the internal helpers
calcMonthlyTotals and calcMonthlyTotalsByCategory. Add a concise JSDoc block
directly above handleTimeline describing its purpose and the TimelineOptions
parameter/behavior so the public entrypoint is documented per guidelines.
Source: Coding guidelines
nazozokc
changed the title
1 participant
Changes
subtrack timeline [months=N] [--categories] [--json]— monthly spending bar chart with Unicode blocks, optional category breakdownsubtrack optimize [--json] [--min-savings N]— cost optimization analysis (cycle optimization, duplicate detection, inactive detection, cancelled savings)subtrack profile save|switch|list|show|delete— CRUD for named filter profiles persisted in config.json, with TUI integration (profile filter applied to subscription list, profile name shown in status bar and list header)step-security/harden-runnerto SHA9af89fc(v2.19.4) across all 10 workflow filesTesting
tsc --noEmitcleanpnpm buildclean