Conversation
|
Warning Review limit reached
More reviews will be available in 11 minutes and 49 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds status and billing-day support to subscription add/edit flows, export-to-file output support, and new upcoming, analytics, and config commands. It also updates CLI wiring, persistence, tests, README usage text, and version metadata to 6.0.0. ChangesCLI command and config expansion
Sequence Diagram(s)sequenceDiagram
participant subtrackIndex as "subtrack/src/index.ts"
participant handleConfigReset
participant nodeFs as "node:fs"
participant resetConfig
participant consola
subtrackIndex->>handleConfigReset: invoke config reset
handleConfigReset->>nodeFs: remove config file
handleConfigReset->>resetConfig: reset configuration
handleConfigReset->>consola: report success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subtrack/src/commands.ts (1)
571-582: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject invalid
--statusand--billingDaybefore persisting them.The non-interactive edit path bypasses the new validators entirely.
flags.statusis just cast toStatus, andflags.billingDaygoes throughNumber()unchecked, so values like--status typoor--billingDay abccan reachupdateSubscription()and corrupt fields thatupcoming/analyticsassume are valid.Suggested fix
if (hasFlags) { // Non-interactive: update only flagged fields const newData: Partial<AddSharedArgs> = {} if (flags.name !== undefined) newData.name = flags.name if (flags.price !== undefined) newData.price = Number(flags.price) if (flags.currency !== undefined) newData.currency = flags.currency if (flags.cycle !== undefined) newData.cycle = flags.cycle as Cycle - if (flags.status !== undefined) newData.status = flags.status as Status + if (flags.status !== undefined) { + if (!isValidStatus(flags.status)) { + consola.error(`Invalid "${flags.status}". Valid: active, paused, cancelled`) + return + } + newData.status = flags.status + } if (flags.billingDay !== undefined) { const trimmed = flags.billingDay.trim() - newData.billingDay = trimmed ? Number(trimmed) : null + const valid = validateBillingDay(trimmed) + if (valid !== true) { + consola.error(valid) + return + } + newData.billingDay = trimmed ? Number(trimmed) : null }🤖 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/commands.ts` around lines 571 - 582, The non-interactive edit path in commands.ts bypasses validation for flags.status and flags.billingDay, so invalid values can reach updateSubscription() unchanged. In the hasFlags block, validate flags.status against the allowed Status values and reject unknown inputs before assigning newData.status, and parse flags.billingDay defensively so non-numeric values are rejected instead of passing Number(trimmed) through. Use the existing AddSharedArgs, Status, and updateSubscription flow to keep the same edit path but enforce the new validators before persisting.
🧹 Nitpick comments (5)
subtrack/src/config.ts (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported helper.
getConfigPath()is now part of the module's public surface, so it should have a short JSDoc contract before being exported. 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` at line 26, The exported helper getConfigPath() is part of the public API and needs a short JSDoc contract. Add a brief JSDoc comment immediately above getConfigPath in config.ts that describes what path it returns and any relevant behavior, following the module’s public API documentation convention.Source: Coding guidelines
subtrack/src/__tests__/analytics.test.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate this suite with
analytics.ts.
subtrack/src/__tests__/analytics.test.tsbreaks the repo's test-placement convention; this should live alongsidesubtrack/src/analytics.tsasanalytics.test.ts. As per coding guidelines, "subtrack/**/*.test.{ts,tsx}: Co-locate test files as*.test.tsalongside source files".🤖 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__/analytics.test.ts` around lines 1 - 4, The analytics test suite is in the wrong location and should be co-located with the source it covers. Move the `analytics.test.ts` suite from the `__tests__` area to sit alongside `analytics.ts`, and keep the existing Vitest setup/imports in that file. Use the `analytics.ts` module and the `analytics.test.ts` suite name as the reference point when relocating it.Source: Coding guidelines
subtrack/src/__tests__/config.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate this suite with
config.ts.
subtrack/src/__tests__/config.test.tsshould live besidesubtrack/src/config.tsasconfig.test.tsto match the repo's test layout. As per coding guidelines, "subtrack/**/*.test.{ts,tsx}: Co-locate test files as*.test.tsalongside source files".🤖 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__/config.test.ts` around lines 1 - 2, The config test suite is in the wrong location and should be co-located with the source file to match the repo test layout. Move the `config.test.ts` suite so it sits beside `config.ts` and keep the same `config.test.ts` naming convention, updating any imports or relative paths in the `config` test file as needed.Source: Coding guidelines
subtrack/src/prompts.ts (1)
45-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for these new exported prompt helpers.
STATUS_CHOICES,isValidStatus(), andvalidateBillingDay()are new exported APIs, but none of them are documented. 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/prompts.ts` around lines 45 - 81, Add JSDoc comments for the new public prompt APIs in prompts.ts: document STATUS_CHOICES, isValidStatus(), and validateBillingDay() so their purpose and expected values are clear to consumers. Place the docs directly above each exported symbol and keep them concise, matching the existing style used for isValidCurrency() and other prompt helpers.Source: Coding guidelines
subtrack/src/__tests__/commands.test.ts (1)
511-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for the new edit flags.
These tests only lock in the happy paths. Please add cases for invalid
statusand invalidbillingDayinput as well, since those are critical paths in the new non-interactive edit flow and would catch the current validator bypass inhandleEdit(). As per coding guidelines, "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__/commands.test.ts` around lines 511 - 546, Add negative-path tests for the new edit flow in commands.test.ts alongside the existing handleEdit cases. Cover invalid status and invalid billingDay inputs by invoking handleEdit with bad values and asserting the edit is rejected and the subscription is not updated. Use the handleEdit symbol and the existing db/getSubscription setup so the tests verify the validator behavior in the non-interactive path.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/README.md`:
- Around line 441-445: Clarify the `monthlyBudget` example to use the smallest
unit of the configured/default currency instead of “USD cents,” since the CLI’s
`defaultCurrency` setting drives how `monthlyBudget` is interpreted and
displayed. Update the README example near `subtrack config set monthlyBudget` to
describe the value generically in active-currency units, keeping it consistent
with the nearby `defaultCurrency` example.
In `@subtrack/src/__tests__/analytics.test.ts`:
- Around line 107-121: The analytics test is asserting the wrong USD budget
display for the configured monthly budget value. Update the expectation in
showAnalytics-related test setup so that the monthlyBudget value set via
setConfig("monthlyBudget", "50000") is asserted as the smallest-unit USD display
(the formatted amount produced by analytics.ts/showAnalytics), not as $50,000.
Use the existing test and the showAnalytics output assertions to align the
expected string with the cents-based contract.
- Around line 11-39: The test suite setup in analytics.test.ts should sandbox
config storage before any later call to setConfig() can fall back to the default
homedir-based path. Update the beforeAll initialization to set SUBSC_CLI_DB_DIR
(or the equivalent config directory override used by config.ts) to a temporary
test directory before importing or initializing code that reads config paths, so
subtrack/src/config.ts does not resolve to the real user config location during
local runs. Use the existing beforeAll and setConfig-related test flow as the
entry point to keep the config path isolated for this suite.
In `@subtrack/src/__tests__/config.test.ts`:
- Around line 8-28: The config tests are using the real default config location,
so the setup should redirect config I/O to a temporary directory before
exercising handleConfigSet() and handleConfigReset(). Update the test setup
around beforeEach/afterEach to set and restore SUBSC_CLI_DB_DIR (or the
equivalent config override used by getConfigPath in config.ts) so getConfigPath
resolves into an isolated temp path and reset does not touch a real user config.
- Around line 51-57: The test for handleConfigSet in config.test.ts currently
has no assertion, so it can pass even if handleConfigSet is a no-op. Update the
test to verify the state change or success output after calling
handleConfigSet("defaultCurrency", "JPY"), using the handleConfigSet symbol from
commands.ts, and only then reset the value back to "USD" for cleanup.
In `@subtrack/src/commands.ts`:
- Around line 136-151: The `add` flow in `commands.ts` is still prompting for
`status` via `promptSelect`, which makes scripted non-interactive usage behave
as interactive. Update the `status` handling around `promptSelect`, `statusRes`,
and the `prompted` flag so that when `flags.status` is omitted it uses the
default `active` value directly instead of opening a prompt. Keep the validation
through `isValidStatus`, and only set `prompted` when an actual user prompt
occurs for other fields.
- Around line 787-795: The handleConfigReset flow reports success even when
unlinkSync fails and the config file remains on disk, so update
handleConfigReset to only call resetConfig() and consola.success after a
confirmed successful removal via existsSync/unlinkSync in the getConfigPath
path. If unlinkSync throws, stop the reset path and surface a failure instead of
clearing only the in-memory cache, since resetConfig alone is not enough to
persist the reset across processes.
In `@subtrack/src/index.ts`:
- Around line 221-229: The upcoming command currently forwards unchecked `days`
values from `define`/`upcomingCommand` into `handleUpcoming`, which allows NaN
and negative numbers to reach downstream date logic. Validate `ctx.values.days`
in the `run` handler before calling `handleUpcoming`: parse it safely, reject
non-numeric or negative input, and only pass a valid non-negative integer
onward, otherwise surface a user-facing error or default behavior consistent
with the command’s description.
---
Outside diff comments:
In `@subtrack/src/commands.ts`:
- Around line 571-582: The non-interactive edit path in commands.ts bypasses
validation for flags.status and flags.billingDay, so invalid values can reach
updateSubscription() unchanged. In the hasFlags block, validate flags.status
against the allowed Status values and reject unknown inputs before assigning
newData.status, and parse flags.billingDay defensively so non-numeric values are
rejected instead of passing Number(trimmed) through. Use the existing
AddSharedArgs, Status, and updateSubscription flow to keep the same edit path
but enforce the new validators before persisting.
---
Nitpick comments:
In `@subtrack/src/__tests__/analytics.test.ts`:
- Around line 1-4: The analytics test suite is in the wrong location and should
be co-located with the source it covers. Move the `analytics.test.ts` suite from
the `__tests__` area to sit alongside `analytics.ts`, and keep the existing
Vitest setup/imports in that file. Use the `analytics.ts` module and the
`analytics.test.ts` suite name as the reference point when relocating it.
In `@subtrack/src/__tests__/commands.test.ts`:
- Around line 511-546: Add negative-path tests for the new edit flow in
commands.test.ts alongside the existing handleEdit cases. Cover invalid status
and invalid billingDay inputs by invoking handleEdit with bad values and
asserting the edit is rejected and the subscription is not updated. Use the
handleEdit symbol and the existing db/getSubscription setup so the tests verify
the validator behavior in the non-interactive path.
In `@subtrack/src/__tests__/config.test.ts`:
- Around line 1-2: The config test suite is in the wrong location and should be
co-located with the source file to match the repo test layout. Move the
`config.test.ts` suite so it sits beside `config.ts` and keep the same
`config.test.ts` naming convention, updating any imports or relative paths in
the `config` test file as needed.
In `@subtrack/src/config.ts`:
- Line 26: The exported helper getConfigPath() is part of the public API and
needs a short JSDoc contract. Add a brief JSDoc comment immediately above
getConfigPath in config.ts that describes what path it returns and any relevant
behavior, following the module’s public API documentation convention.
In `@subtrack/src/prompts.ts`:
- Around line 45-81: Add JSDoc comments for the new public prompt APIs in
prompts.ts: document STATUS_CHOICES, isValidStatus(), and validateBillingDay()
so their purpose and expected values are clear to consumers. Place the docs
directly above each exported symbol and keep them concise, matching the existing
style used for isValidCurrency() and other prompt helpers.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6f54004-76f0-42e3-baf3-90c2e2588d7b
📒 Files selected for processing (10)
subtrack/README.mdsubtrack/package.jsonsubtrack/src/__tests__/analytics.test.tssubtrack/src/__tests__/commands.test.tssubtrack/src/__tests__/config.test.tssubtrack/src/commands.tssubtrack/src/config.tssubtrack/src/db.tssubtrack/src/index.tssubtrack/src/prompts.ts
| # Set default currency to JPY | ||
| subtrack config set defaultCurrency JPY | ||
|
|
||
| # Set monthly budget (in USD cents) | ||
| subtrack config set monthlyBudget 50000 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the monthlyBudget unit here.
A few lines above, the docs show defaultCurrency being configurable, but this example hardcodes "USD cents". The CLI reads and displays monthlyBudget against the configured currency, so this should describe the smallest unit of the active/default currency rather than USD-specific cents.
🤖 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/README.md` around lines 441 - 445, Clarify the `monthlyBudget`
example to use the smallest unit of the configured/default currency instead of
“USD cents,” since the CLI’s `defaultCurrency` setting drives how
`monthlyBudget` is interpreted and displayed. Update the README example near
`subtrack config set monthlyBudget` to describe the value generically in
active-currency units, keeping it consistent with the nearby `defaultCurrency`
example.
| beforeAll(async () => { | ||
| const SQL = await initSqlJs() | ||
| testDb = new SQL.Database() | ||
| testDb.run("PRAGMA foreign_keys = ON") | ||
| testDb.run(`CREATE TABLE IF NOT EXISTS subscriptions ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| name TEXT NOT NULL, | ||
| price INTEGER NOT NULL, | ||
| currency TEXT NOT NULL, | ||
| cycle TEXT NOT NULL, | ||
| status TEXT NOT NULL DEFAULT 'active', | ||
| billing_day INTEGER, | ||
| created_at TEXT NOT NULL DEFAULT (date('now')) | ||
| )`) | ||
| testDb.run(`CREATE TABLE IF NOT EXISTS tags ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| name TEXT NOT NULL UNIQUE | ||
| )`) | ||
| testDb.run(`CREATE TABLE IF NOT EXISTS subscription_tags ( | ||
| subscription_id INTEGER NOT NULL, | ||
| tag_id INTEGER NOT NULL, | ||
| PRIMARY KEY (subscription_id, tag_id), | ||
| FOREIGN KEY (subscription_id) REFERENCES subscriptions(id) ON DELETE CASCADE, | ||
| FOREIGN KEY (tag_id) REFERENCES tags(id) ON DELETE CASCADE | ||
| )`) | ||
|
|
||
| const db = await import("../db.ts") | ||
| db.__setDb(testDb) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Sandbox the config path in suite setup.
This suite later calls setConfig(), and subtrack/src/config.ts:22-27 falls back to homedir()/.config/subtrack/config.json when SUBSC_CLI_DB_DIR is unset. A local test run can therefore overwrite a developer's real config file unless this setup points config storage at a temporary directory first.
🤖 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__/analytics.test.ts` around lines 11 - 39, The test
suite setup in analytics.test.ts should sandbox config storage before any later
call to setConfig() can fall back to the default homedir-based path. Update the
beforeAll initialization to set SUBSC_CLI_DB_DIR (or the equivalent config
directory override used by config.ts) to a temporary test directory before
importing or initializing code that reads config paths, so
subtrack/src/config.ts does not resolve to the real user config location during
local runs. Use the existing beforeAll and setConfig-related test flow as the
entry point to keep the config path isolated for this suite.
| test("showAnalytics includes budget info when configured", async () => { | ||
| const { resetConfig } = await import("../config.ts") | ||
| resetConfig() | ||
| const { setConfig } = await import("../config.ts") | ||
| setConfig("monthlyBudget", "50000") | ||
|
|
||
| const db = await import("../db.ts") | ||
| db.writeSubscription({ name: "Netflix", price: 1500, currency: "USD", cycle: "monthly", tags: [] }) | ||
|
|
||
| const { showAnalytics } = await import("../analytics.ts") | ||
| showAnalytics() | ||
|
|
||
| const output = logMessages.join("\n") | ||
| expect(output).toContain("Budget:") | ||
| expect(output).toContain("$50,000") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the USD budget expectation.
Line 111 sets monthlyBudget to 50000, which is 50,000 cents. The matching assertion is $500.00, not $50,000; otherwise this test locks in the wrong unit contract for USD budgets. As per coding guidelines, "Represent prices as integers in the smallest unit (JPY without decimals, USD in cents)".
🤖 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__/analytics.test.ts` around lines 107 - 121, The
analytics test is asserting the wrong USD budget display for the configured
monthly budget value. Update the expectation in showAnalytics-related test setup
so that the monthlyBudget value set via setConfig("monthlyBudget", "50000") is
asserted as the smallest-unit USD display (the formatted amount produced by
analytics.ts/showAnalytics), not as $50,000. Use the existing test and the
showAnalytics output assertions to align the expected string with the
cents-based contract.
Source: Coding guidelines
| beforeEach(() => { | ||
| logMessages.length = 0 | ||
| errorMessages.length = 0 | ||
| successMessages.length = 0 | ||
|
|
||
| const stripAnsi = (s: string) => s.replace(/\x1b\[[0-9;]*m/g, "") | ||
|
|
||
| consola.mockTypes((_type: string, _defaults: object) => { | ||
| return (...args: unknown[]) => { | ||
| const str = args.map((a) => String(a)).join(" ") | ||
| const clean = stripAnsi(str) | ||
| if (_type === "log") logMessages.push(clean) | ||
| if (_type === "error") errorMessages.push(clean) | ||
| if (_type === "success") successMessages.push(clean) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| consola.mockTypes() | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Isolate config writes to a temp directory in test setup.
These tests exercise handleConfigSet() and handleConfigReset(), and handleConfigReset() deletes getConfigPath(). Since subtrack/src/config.ts:22-27 defaults that path under homedir(), running this suite locally can overwrite or delete a real user config file unless setup redirects SUBSC_CLI_DB_DIR to a temporary directory first.
🤖 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__/config.test.ts` around lines 8 - 28, The config tests
are using the real default config location, so the setup should redirect config
I/O to a temporary directory before exercising handleConfigSet() and
handleConfigReset(). Update the test setup around beforeEach/afterEach to set
and restore SUBSC_CLI_DB_DIR (or the equivalent config override used by
getConfigPath in config.ts) so getConfigPath resolves into an isolated temp path
and reset does not touch a real user config.
| test("handleConfigSet sets a config value", async () => { | ||
| const { handleConfigSet } = await import("../commands.ts") | ||
| handleConfigSet("defaultCurrency", "JPY") | ||
|
|
||
| // Reset for next test | ||
| handleConfigSet("defaultCurrency", "USD") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that handleConfigSet() actually changed state.
This test has no assertion, so it still passes if handleConfigSet() is a no-op. Verify the stored config or emitted success output before resetting it.
Possible fix
test("handleConfigSet sets a config value", async () => {
const { handleConfigSet } = await import("../commands.ts")
+ const { loadConfig } = await import("../config.ts")
handleConfigSet("defaultCurrency", "JPY")
+ expect(loadConfig().defaultCurrency).toBe("JPY")
// Reset for next test
handleConfigSet("defaultCurrency", "USD")
})📝 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.
| test("handleConfigSet sets a config value", async () => { | |
| const { handleConfigSet } = await import("../commands.ts") | |
| handleConfigSet("defaultCurrency", "JPY") | |
| // Reset for next test | |
| handleConfigSet("defaultCurrency", "USD") | |
| }) | |
| test("handleConfigSet sets a config value", async () => { | |
| const { handleConfigSet } = await import("../commands.ts") | |
| const { loadConfig } = await import("../config.ts") | |
| handleConfigSet("defaultCurrency", "JPY") | |
| expect(loadConfig().defaultCurrency).toBe("JPY") | |
| // Reset for next test | |
| handleConfigSet("defaultCurrency", "USD") | |
| }) |
🤖 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__/config.test.ts` around lines 51 - 57, The test for
handleConfigSet in config.test.ts currently has no assertion, so it can pass
even if handleConfigSet is a no-op. Update the test to verify the state change
or success output after calling handleConfigSet("defaultCurrency", "JPY"), using
the handleConfigSet symbol from commands.ts, and only then reset the value back
to "USD" for cleanup.
| export async function handleConfigReset(): Promise<void> { | ||
| const { unlinkSync, existsSync } = await import("node:fs") | ||
| const { getConfigPath } = await import("./config.ts") | ||
| const configPath = getConfigPath() | ||
| if (existsSync(configPath)) { | ||
| try { unlinkSync(configPath) } catch { /* best-effort */ } | ||
| } | ||
| resetConfig() | ||
| consola.success("Config reset to defaults") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Don't report success when the config file was not actually removed.
If unlinkSync() fails, this still calls resetConfig() and prints a success message. resetConfig() only clears the in-memory cache, so the next process will reload the unchanged file and the reset is effectively lost.
Suggested fix
const configPath = getConfigPath()
if (existsSync(configPath)) {
- try { unlinkSync(configPath) } catch { /* best-effort */ }
+ try {
+ unlinkSync(configPath)
+ } catch (error) {
+ consola.error(`Failed to reset config: ${String(error)}`)
+ return
+ }
}
resetConfig()
consola.success("Config reset to defaults")📝 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 async function handleConfigReset(): Promise<void> { | |
| const { unlinkSync, existsSync } = await import("node:fs") | |
| const { getConfigPath } = await import("./config.ts") | |
| const configPath = getConfigPath() | |
| if (existsSync(configPath)) { | |
| try { unlinkSync(configPath) } catch { /* best-effort */ } | |
| } | |
| resetConfig() | |
| consola.success("Config reset to defaults") | |
| export async function handleConfigReset(): Promise<void> { | |
| const { unlinkSync, existsSync } = await import("node:fs") | |
| const { getConfigPath } = await import("./config.ts") | |
| const configPath = getConfigPath() | |
| if (existsSync(configPath)) { | |
| try { | |
| unlinkSync(configPath) | |
| } catch (error) { | |
| consola.error(`Failed to reset config: ${String(error)}`) | |
| return | |
| } | |
| } | |
| resetConfig() | |
| consola.success("Config reset to defaults") | |
| } |
🤖 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/commands.ts` around lines 787 - 795, The handleConfigReset flow
reports success even when unlinkSync fails and the config file remains on disk,
so update handleConfigReset to only call resetConfig() and consola.success after
a confirmed successful removal via existsSync/unlinkSync in the getConfigPath
path. If unlinkSync throws, stop the reset path and surface a failure instead of
clearing only the in-memory cache, since resetConfig alone is not enough to
persist the reset across processes.
| const upcomingCommand = define({ | ||
| name: "upcoming", | ||
| description: "Show upcoming bills within a number of days", | ||
| args: { | ||
| days: { type: "positional", description: "Number of days (default: 7)", required: false }, | ||
| }, | ||
| run: (ctx) => { | ||
| const days = ctx.values.days ? Number(ctx.values.days) : undefined | ||
| handleUpcoming(days) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate upcoming [days] before passing it downstream.
Number(ctx.values.days) turns inputs like abc into NaN, and negative values are accepted too. showUpcoming() then does date math and user-facing messaging with that invalid value instead of rejecting it early.
Suggested fix
run: (ctx) => {
- const days = ctx.values.days ? Number(ctx.values.days) : undefined
- handleUpcoming(days)
+ if (ctx.values.days === undefined) {
+ handleUpcoming()
+ return
+ }
+
+ const days = Number(ctx.values.days)
+ if (!Number.isInteger(days) || days < 0) {
+ consola.error("days must be a non-negative integer")
+ return
+ }
+
+ handleUpcoming(days)
},📝 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 upcomingCommand = define({ | |
| name: "upcoming", | |
| description: "Show upcoming bills within a number of days", | |
| args: { | |
| days: { type: "positional", description: "Number of days (default: 7)", required: false }, | |
| }, | |
| run: (ctx) => { | |
| const days = ctx.values.days ? Number(ctx.values.days) : undefined | |
| handleUpcoming(days) | |
| const upcomingCommand = define({ | |
| name: "upcoming", | |
| description: "Show upcoming bills within a number of days", | |
| args: { | |
| days: { type: "positional", description: "Number of days (default: 7)", required: false }, | |
| }, | |
| run: (ctx) => { | |
| if (ctx.values.days === undefined) { | |
| handleUpcoming() | |
| return | |
| } | |
| const days = Number(ctx.values.days) | |
| if (!Number.isInteger(days) || days < 0) { | |
| consola.error("days must be a non-negative integer") | |
| return | |
| } | |
| handleUpcoming(days) |
🤖 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 221 - 229, The upcoming command currently
forwards unchecked `days` values from `define`/`upcomingCommand` into
`handleUpcoming`, which allows NaN and negative numbers to reach downstream date
logic. Validate `ctx.values.days` in the `run` handler before calling
`handleUpcoming`: parse it safely, reject non-numeric or negative input, and
only pass a valid non-negative integer onward, otherwise surface a user-facing
error or default behavior consistent with the command’s description.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 7 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
1 participant
Summary
New commands and enhancements for v6.0.0.
New Commands
subtrack upcoming [days]subtrack analyticssubtrack config list|get|set|resetEnhancements
add: Added--statusand--billingDayflagsedit: Added--statusand--billingDayflags (non-interactive and interactive)export: Added-o, --output <file>flag to write to file instead of stdoutusage refresh: Fixed README description to match implementationusage import: Added to README command tableDB Changes
updateSubscription()now supportsstatusandbilling_daycolumnsTests
analytics.test.ts(4 tests),config.test.ts(7 tests)commands.test.tsCloses: N/A
Summary by CodeRabbit
New Features
Bug Fixes
Documentation