Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 25 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 refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (1)
📝 WalkthroughWalkthroughThe CLI is migrated from Changessubtrack CLI v2.2.0 Feature Expansion
Sequence Diagram(s)sequenceDiagram
participant User
participant gunshi as gunshi CLI
participant commands as commands.ts
participant db as db.ts
participant display as display.ts
rect rgba(100, 150, 200, 0.5)
Note over User,display: edit (interactive)
User->>gunshi: subtrack edit
gunshi->>commands: handleEdit()
commands->>db: getSubscriptions()
db-->>commands: SharedArgs[]
commands->>User: select prompt
User-->>commands: chosen subscription
commands->>User: checkbox prompt (fields)
User-->>commands: selected fields + values
commands->>db: updateSubscription(id, fields)
db-->>commands: boolean
commands->>display: consola.success / .error
end
rect rgba(150, 200, 100, 0.5)
Note over User,display: import --dry-run
User->>gunshi: subtrack import file.csv --dry-run
gunshi->>commands: handleImport(file, {dryRun:true})
commands->>commands: parseCsvLine (per row)
commands->>display: consola.info("would import …")
end
rect rgba(200, 150, 100, 0.5)
Note over User,display: summary
User->>gunshi: subtrack summary
gunshi->>commands: handleSummary()
commands->>db: getSubscriptions()
db-->>commands: SharedArgs[]
commands->>display: showSummary(subs)
display->>display: calcSummary(subs)
display-->>User: totals by currency/tag + most expensive
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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: 3
🧹 Nitpick comments (4)
subtrack/src/commands.ts (1)
319-323: ⚡ Quick winPrefer defensive check over non-null assertion.
Line 320 uses a non-null assertion (
!) whereas the interactive path (lines 400-404) defensively checks forundefined. For consistency and to guard against edge cases (e.g., concurrent deletion), apply the same pattern here.♻️ Suggested refactor
updateSubscription(sub.id, newData) - const updated = getSubscription(sub.id)! + const updated = getSubscription(sub.id) + if (!updated) { + consola.error("Failed to retrieve updated subscription") + return + } consola.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/commands.ts` around lines 319 - 323, Remove the non-null assertion operator (!) from the getSubscription call at line 320 in the updateSubscription function and replace it with a defensive check that validates the subscription exists before logging the success message. Instead of directly calling getSubscription(sub.id)!, store the result in a variable and add an if condition to verify it is not undefined before proceeding with the consola.success call. This approach should match the same defensive pattern used in the interactive path around lines 400-404 to consistently handle cases where the subscription might not exist.subtrack/src/commands.test.ts (2)
9-44: ⚡ Quick winPrefer
consola.mockTypes()for logging mocks in tests.Please switch from manual
vi.mock("consola", ...)scaffolding to the project-standardconsola.mockTypes()pattern for consistency and lower mock maintenance.As per coding guidelines,
subtrack/**/*.test.ts: "Mockconsolaviaconsola.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/commands.test.ts` around lines 9 - 44, Replace the manual vi.mock("consola", ...) implementation with consola.mockTypes() to follow the project-standard pattern for logging mocks in test files. Remove the entire vi.mock block that manually creates the logMessages, infoMessages, successMessages, errorMessages, failMessages, and warnMessages arrays along with the makeFn helper function, and instead use the consola.mockTypes() API which provides the same mocking functionality while maintaining consistency with the project's coding guidelines for subtrack test files.Source: Coding guidelines
95-98: ⚡ Quick winMake
process.exitmock terminate control flow in failure-path tests.The current no-op mock lets execution continue after
process.exit, which can hide regressions in paths that are supposed to stop immediately.Proposed fix
- exitSpy = vi.spyOn(process, "exit").mockImplementation((() => { - // prevent process.exit from killing the test runner - }) as () => never) + exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`__TEST_PROCESS_EXIT__:${code ?? ""}`) + }) as () => never)🤖 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.test.ts` around lines 95 - 98, The process.exit mock created by the exitSpy is currently a no-op that doesn't terminate control flow, allowing test execution to continue after process.exit is called and potentially hiding bugs. Modify the mock implementation to throw an error instead of just having an empty comment, so that when process.exit is invoked during tests, it actually stops execution and prevents code from continuing past that point. This ensures failure-path tests properly validate that process.exit is being called when expected.subtrack/src/index.ts (1)
2-2: ⚡ Quick winAlign
src/index.tsframework usage with repository rule.This file now uses Gunshi, but the repository guideline for
subtrack/src/index.tsstill requires Commander. Please either restore Commander here or update the guideline contract in the same PR to prevent policy drift.As per coding guidelines,
subtrack/src/index.ts: "Usecommanderlibrary for CLI definition and command routing insrc/index.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/index.ts` at line 2, The import statement in subtrack/src/index.ts is using Gunshi (importing cli and define from "gunshi") but this conflicts with the repository coding guideline that requires using the Commander library for this file. Either replace the current Gunshi import with the appropriate Commander import and refactor the CLI definition code to use Commander's API instead, or update the repository guideline contract documented for subtrack/src/index.ts to reflect that Gunshi is the approved framework. Choose one approach and ensure consistency between the actual code and the documented guidelines in the same PR.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/display.ts`:
- Around line 441-444: The display logic in the loop that iterates through
sorted monthlyByTag data hardcodes "USD" when formatting the monthly price total
using formatPrice, but monthlyByTag accumulates prices from subscriptions in
different currencies without conversion. This causes misleading output by mixing
currencies. To fix this, you need to either track the currency information
alongside the monthly totals in monthlyByTag so you can display it correctly for
each tag, or modify the aggregation logic to convert all prices to a single
currency before summing, or remove the currency symbol from the display and add
a note that the values are not currency-converted. Choose the approach that best
fits your application's data model and update the monthlyByTag structure and the
formatPrice call accordingly.
In `@subtrack/src/index.ts`:
- Around line 71-77: The run callback in the command handler is invoking the
async function handleTags without returning its result. Return the promise from
handleTags to ensure proper rejection propagation and prevent premature command
completion. Apply the same fix to the other occurrence mentioned at lines
166-169 where another async handler is called without returning its promise.
- Around line 163-168: The `run` function casts ctx.values.period to Cycle type
without validating that the input is actually a valid Cycle value. Before
casting the period value in the line where handlePayment is called, add
validation logic to check if the period value is one of the valid Cycle enum
values, and either throw an error or fall back to a default value if the input
is invalid. This ensures only valid Cycle values are passed to the handlePayment
function.
---
Nitpick comments:
In `@subtrack/src/commands.test.ts`:
- Around line 9-44: Replace the manual vi.mock("consola", ...) implementation
with consola.mockTypes() to follow the project-standard pattern for logging
mocks in test files. Remove the entire vi.mock block that manually creates the
logMessages, infoMessages, successMessages, errorMessages, failMessages, and
warnMessages arrays along with the makeFn helper function, and instead use the
consola.mockTypes() API which provides the same mocking functionality while
maintaining consistency with the project's coding guidelines for subtrack test
files.
- Around line 95-98: The process.exit mock created by the exitSpy is currently a
no-op that doesn't terminate control flow, allowing test execution to continue
after process.exit is called and potentially hiding bugs. Modify the mock
implementation to throw an error instead of just having an empty comment, so
that when process.exit is invoked during tests, it actually stops execution and
prevents code from continuing past that point. This ensures failure-path tests
properly validate that process.exit is being called when expected.
In `@subtrack/src/commands.ts`:
- Around line 319-323: Remove the non-null assertion operator (!) from the
getSubscription call at line 320 in the updateSubscription function and replace
it with a defensive check that validates the subscription exists before logging
the success message. Instead of directly calling getSubscription(sub.id)!, store
the result in a variable and add an if condition to verify it is not undefined
before proceeding with the consola.success call. This approach should match the
same defensive pattern used in the interactive path around lines 400-404 to
consistently handle cases where the subscription might not exist.
In `@subtrack/src/index.ts`:
- Line 2: The import statement in subtrack/src/index.ts is using Gunshi
(importing cli and define from "gunshi") but this conflicts with the repository
coding guideline that requires using the Commander library for this file. Either
replace the current Gunshi import with the appropriate Commander import and
refactor the CLI definition code to use Commander's API instead, or update the
repository guideline contract documented for subtrack/src/index.ts to reflect
that Gunshi is the approved framework. Choose one approach and ensure
consistency between the actual code and the documented guidelines in the same
PR.
🪄 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: 9f2966b3-14aa-4f1e-a6fd-e4015b261462
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
subtrack/package.jsonsubtrack/src/commands.test.tssubtrack/src/commands.tssubtrack/src/db.test.tssubtrack/src/db.tssubtrack/src/display.test.tssubtrack/src/display.tssubtrack/src/index.tssubtrack/src/prompts.ts
| for (const [tag, info] of sorted) { | ||
| consola.log( | ||
| ` ${tag.padEnd(16)} ${formatPrice(Math.round(info.monthly), "USD")}/month (${info.count} sub${info.count > 1 ? "s" : ""})`, | ||
| ) |
There was a problem hiding this comment.
Monthly-by-tag totals mix currencies without conversion.
monthlyByTag accumulates raw prices from subscriptions that may have different currencies (JPY, USD, EUR, etc.), but the display hardcodes "USD" for formatting. This produces misleading output—e.g., summing ¥1000 + $10 and displaying as "$1,010/month".
Consider either:
- Converting all prices to a common currency before aggregation (requires FX rates)
- Grouping by
(tag, currency)and displaying separate totals per currency - Removing the currency symbol and noting that values are not converted
🤖 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/display.ts` around lines 441 - 444, The display logic in the
loop that iterates through sorted monthlyByTag data hardcodes "USD" when
formatting the monthly price total using formatPrice, but monthlyByTag
accumulates prices from subscriptions in different currencies without
conversion. This causes misleading output by mixing currencies. To fix this, you
need to either track the currency information alongside the monthly totals in
monthlyByTag so you can display it correctly for each tag, or modify the
aggregation logic to convert all prices to a single currency before summing, or
remove the currency symbol from the display and add a note that the values are
not currency-converted. Choose the approach that best fits your application's
data model and update the monthlyByTag structure and the formatPrice call
accordingly.
| run: (ctx) => { | ||
| if (ctx.positionals.length === 0) { | ||
| consola.error("Please specify at least one tag") | ||
| return | ||
| } | ||
| handleTags(ctx.positionals) | ||
| }, |
There was a problem hiding this comment.
Return async handler promises from command run callbacks.
Line 76 and Line 168 invoke async handlers without returning their promises. That can lose rejection propagation and allow command completion to be observed too early.
Proposed fix
const tagsCommand = define({
name: "tags",
description: "Filter subscriptions by tags (AND logic)",
run: (ctx) => {
if (ctx.positionals.length === 0) {
consola.error("Please specify at least one tag")
return
}
- handleTags(ctx.positionals)
+ return handleTags(ctx.positionals)
},
})
@@
const paymentCommand = define({
@@
run: (ctx) => {
const period = (ctx.values.period || "monthly") as Cycle
- handlePayment(period, { currency: ctx.values.currency })
+ return handlePayment(period, { currency: ctx.values.currency })
},
})Also applies to: 166-169
🤖 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 71 - 77, The run callback in the command
handler is invoking the async function handleTags without returning its result.
Return the promise from handleTags to ensure proper rejection propagation and
prevent premature command completion. Apply the same fix to the other occurrence
mentioned at lines 166-169 where another async handler is called without
returning its promise.
| period: { type: "positional", description: "Billing period (default: monthly)" }, | ||
| currency: { type: "string", short: "c", description: "Convert all prices to target currency" }, | ||
| }, | ||
| run: (ctx) => { | ||
| const period = (ctx.values.period || "monthly") as Cycle | ||
| handlePayment(period, { currency: ctx.values.currency }) |
There was a problem hiding this comment.
Validate period input before casting to Cycle.
Line 167 casts arbitrary user input to Cycle without runtime validation. Invalid values can flow into payment math and produce incorrect totals.
Proposed fix
const paymentCommand = define({
@@
run: (ctx) => {
- const period = (ctx.values.period || "monthly") as Cycle
+ const rawPeriod = ctx.values.period
+ const validPeriods: Cycle[] = ["weekly", "bi-weekly", "monthly", "quarterly", "semi-annual", "yearly"]
+ if (rawPeriod && !validPeriods.includes(rawPeriod as Cycle)) {
+ consola.error(`Invalid period: "${rawPeriod}"`)
+ return
+ }
+ const period = (rawPeriod || "monthly") as Cycle
return handlePayment(period, { currency: ctx.values.currency })
},
})📝 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.
| period: { type: "positional", description: "Billing period (default: monthly)" }, | |
| currency: { type: "string", short: "c", description: "Convert all prices to target currency" }, | |
| }, | |
| run: (ctx) => { | |
| const period = (ctx.values.period || "monthly") as Cycle | |
| handlePayment(period, { currency: ctx.values.currency }) | |
| run: (ctx) => { | |
| const rawPeriod = ctx.values.period | |
| const validPeriods: Cycle[] = ["weekly", "bi-weekly", "monthly", "quarterly", "semi-annual", "yearly"] | |
| if (rawPeriod && !validPeriods.includes(rawPeriod as Cycle)) { | |
| consola.error(`Invalid period: "${rawPeriod}"`) | |
| return | |
| } | |
| const period = (rawPeriod || "monthly") as Cycle | |
| handlePayment(period, { currency: ctx.values.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/index.ts` around lines 163 - 168, The `run` function casts
ctx.values.period to Cycle type without validating that the input is actually a
valid Cycle value. Before casting the period value in the line where
handlePayment is called, add validation logic to check if the period value is
one of the valid Cycle enum values, and either throw an error or fall back to a
default value if the input is invalid. This ensures only valid Cycle values are
passed to the handlePayment function.
1 participant
Summary
Core Change
Migrate CLI framework from Commander to Gunshi. Add 6 new features.
New Features
subtrack tag list|rename|delete|pruneBug Fixes & Improvements
ON DELETE CASCADEonsubscription_tags.tag_idFK (prevent foreign key violations)mapTagsquery to eliminate N+1sortwithreduceincalcSummary(O(n log n) → O(n))!)parseCsvLinefor testinghandleImportTests
commands.test.ts(52 new tests): full coverage for tag management, export, list, edit, import, summary, add, delete, paymentdb.test.ts(+14): sort, getSubscription, updateSubscription, tag managementdisplay.test.ts(+13): exportJson, calcSummary, showSummaryBuild
Summary by CodeRabbit
Release Notes
New Features
editcommand to modify subscription details interactively or via flagsimportcommand to bulk import subscriptions from CSV with validation and dry-run supportsummarycommand to view subscription totals and breakdowns by currency and tagVersion