fix: resolve upcoming.ts mixed currency total, prevent SQL injection, and fix scanner table names - #29
Closed
nazozokc wants to merge 2 commits into
Closed
fix: resolve upcoming.ts mixed currency total, prevent SQL injection, and fix scanner table names#29nazozokc wants to merge 2 commits into
nazozokc wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a pnpm cookie override, new config persistence, analytics and upcoming-bill output, a subscription status column, scanner SQL parameterization and table selection changes, and tests for upcoming-bill output. ChangesWorkspace cookie override
Subtrack reporting and scanner hardening
Sequence Diagram(s)Analytics output sequenceDiagram
participant showAnalytics
participant getSubscriptions
participant loadConfig
participant calcSummary
participant consola
showAnalytics->>getSubscriptions: active subscriptions
showAnalytics->>loadConfig: config
showAnalytics->>calcSummary: summary data
showAnalytics->>consola: formatted counts, totals, and tags
Upcoming bills sequenceDiagram
participant showUpcoming
participant getSubscriptions
participant consola
showUpcoming->>getSubscriptions: non-cancelled subscriptions
showUpcoming->>consola: upcoming bill lines and totals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
subtrack/src/analytics.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for
showAnalytics().
showAnalyticsis a public export and should be documented with a short contract/behavior note.
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/analytics.ts` at line 10, The public export showAnalytics() in analytics.ts is missing JSDoc, so add a short documentation block directly above the function. Include a concise contract/behavior note describing what showAnalytics() does and any relevant usage expectations, following the project guideline to document public APIs with JSDoc comments.Source: Coding guidelines
subtrack/src/config.ts (1)
8-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc to exported config APIs.
CONFIG_KEYS,loadConfig,resetConfig, andsetConfigare public exports but currently undocumented. Please add concise JSDoc for contract/behavior (especially validation and persistence side effects).
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 8 - 87, Add concise JSDoc comments to the public config API exports in config.ts: CONFIG_KEYS, loadConfig, resetConfig, and setConfig. Document what each symbol returns or mutates, and call out the key behaviors in loadConfig (lazy load, cached defaults, corrupt-file fallback), resetConfig (clears the in-memory cache), and setConfig (validates values, persists via saveConfig, and returns success/failure). Keep the comments brief and colocated with the corresponding exports so the contract is clear to consumers.Source: Coding guidelines
subtrack/src/upcoming.ts (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the exported
showUpcomingAPI.
showUpcomingis exported but undocumented. Please add a short JSDoc describing thedayswindow and output side effects.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/upcoming.ts` at line 103, The exported showUpcoming API is missing JSDoc, so add a short documentation block directly above showUpcoming describing that it displays upcoming items for the given days window and that it produces output as a side effect; make sure the comment includes the days parameter and clearly marks the function’s public behavior.Source: Coding guidelines
subtrack/src/__tests__/upcoming.test.ts (1)
89-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the non-monthly and due-today billing paths.
The production code has separate logic for weekly, bi-weekly, quarterly, semi-annual, yearly, month-end overflow, and “today” labels, but the tests only assert a monthly happy path plus cancellation filtering. Please add cases for those critical branches.
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__/upcoming.test.ts` around lines 89 - 114, The current tests for showUpcoming only cover a monthly upcoming subscription and cancelled filtering, but miss the critical billing branches in the production logic. Add unit tests in upcoming.test.ts that exercise showUpcoming’s handling of non-monthly cycles (weekly, bi-weekly, quarterly, semi-annual, yearly), the month-end overflow case, and the due-today label path, asserting the rendered output for each branch. Use the existing showUpcoming entry point and db.writeSubscription test setup to create subscriptions that hit each 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/src/__tests__/upcoming.test.ts`:
- Around line 73-114: The upcoming subscription tests are time-sensitive because
they depend on new Date() and can flap around billing thresholds. In
upcoming.test.ts, add deterministic time control in the test setup by using
jest.useFakeTimers() with a fixed system date in beforeEach and restoring with
jest.useRealTimers() in afterEach, so showUpcoming and the monthly billing-day
calculations behave consistently across runs.
In `@subtrack/src/analytics.ts`:
- Around line 48-58: The budget/tag analytics in analytics.ts are mixing amounts
from different currencies and hardcoding USD in the display path. Update the
budget summary logic around the monthly aggregation and the tag totals so values
are either grouped per currency (for example using a monthlyByCurrency-style
breakdown) or explicitly converted to one reference currency before formatting.
Make sure the Budget, Spent, Remaining, and tag output all use the same currency
source instead of passing "USD" directly into formatPrice.
- Around line 11-33: The analytics flow in `getSubscriptions`/`calcSummary` is
reading `s.status`, but the current query result and `SharedArgs` contract do
not expose a `status` field, causing the typecheck failure. Update the shared
row/query typing so `status` is included in the returned subscription shape, or
refactor `analytics.ts` to avoid accessing `status` in the `list.filter(...)`
and `cancelledCount` logic; make sure the types for `getSubscriptions`,
`SharedArgs`, and any related subscription model stay consistent.
In `@subtrack/src/display.ts`:
- Line 4: The Status contract between display.ts and types.ts is incomplete:
display.ts imports Status and reads sub.status, but SharedArgs does not define a
status field and types.ts does not export Status. Update the upstream type
contract in types.ts by adding the Status type and the status property to
SharedArgs, or change display.ts to import Status from the correct module, so
the sub.status usage in display.ts is type-safe.
In `@subtrack/src/upcoming.ts`:
- Around line 104-118: showUpcoming() now depends on subscription status and
billing metadata, so update the getSubscriptions() query in db.ts to load the
fields it reads: status, createdAt, and billingDay in addition to the existing
columns. Then ensure showUpcoming() can safely filter cancelled subscriptions
and calculateNextBilling() has the data it needs without assuming billingDay was
injected elsewhere.
- Around line 34-45: The monthly and yearly branches in upcoming() currently
exclude bills due today by using a strict candidate > fromDate check. Update the
comparison logic so a candidate on the same day is treated as upcoming as well,
while keeping the existing range handling intact; adjust both the "monthly" and
"yearly" cases consistently around the candidate/return logic.
- Around line 57-66: The quarterly and semi-annual branches in upcoming.ts are
using anchorDate directly, so they ignore the normalized billingDay and can
return a due date that is not after fromDate. Update the logic in the quarterly
and semi-annual cases to follow the same pattern as the yearly path: compute the
correct interval month, construct the result from anchorDay rather than the
original anchorDate day, and make sure the returned date is strictly greater
than fromDate.
---
Nitpick comments:
In `@subtrack/src/__tests__/upcoming.test.ts`:
- Around line 89-114: The current tests for showUpcoming only cover a monthly
upcoming subscription and cancelled filtering, but miss the critical billing
branches in the production logic. Add unit tests in upcoming.test.ts that
exercise showUpcoming’s handling of non-monthly cycles (weekly, bi-weekly,
quarterly, semi-annual, yearly), the month-end overflow case, and the due-today
label path, asserting the rendered output for each branch. Use the existing
showUpcoming entry point and db.writeSubscription test setup to create
subscriptions that hit each path.
In `@subtrack/src/analytics.ts`:
- Line 10: The public export showAnalytics() in analytics.ts is missing JSDoc,
so add a short documentation block directly above the function. Include a
concise contract/behavior note describing what showAnalytics() does and any
relevant usage expectations, following the project guideline to document public
APIs with JSDoc comments.
In `@subtrack/src/config.ts`:
- Around line 8-87: Add concise JSDoc comments to the public config API exports
in config.ts: CONFIG_KEYS, loadConfig, resetConfig, and setConfig. Document what
each symbol returns or mutates, and call out the key behaviors in loadConfig
(lazy load, cached defaults, corrupt-file fallback), resetConfig (clears the
in-memory cache), and setConfig (validates values, persists via saveConfig, and
returns success/failure). Keep the comments brief and colocated with the
corresponding exports so the contract is clear to consumers.
In `@subtrack/src/upcoming.ts`:
- Line 103: The exported showUpcoming API is missing JSDoc, so add a short
documentation block directly above showUpcoming describing that it displays
upcoming items for the given days window and that it produces output as a side
effect; make sure the comment includes the days parameter and clearly marks the
function’s public behavior.
🪄 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: d532cff5-3aaf-4e87-89f6-56055ac81404
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
pnpm-workspace.yamlsubtrack/src/__tests__/upcoming.test.tssubtrack/src/analytics.tssubtrack/src/codex-scanner.tssubtrack/src/config.tssubtrack/src/cursor-scanner.tssubtrack/src/display.tssubtrack/src/opencode-scanner.tssubtrack/src/upcoming.tssubtrack/src/windsurf-scanner.ts
Comment on lines
+73
to
+114
| test("showUpcoming shows info when no subscriptions", async () => { | ||
| const { showUpcoming } = await import("../upcoming.ts") | ||
| showUpcoming(7) | ||
| expect(infoMessages.some((m) => m.includes("No active subscriptions"))).toBe(true) | ||
| }) | ||
|
|
||
| test("showUpcoming shows info when no upcoming bills", async () => { | ||
| const db = await import("../db.ts") | ||
| // Create a subscription with billing far in the future | ||
| db.writeSubscription({ name: "Yearly", price: 1000, currency: "USD", cycle: "yearly", tags: [], status: "active", createdAt: "2025-01-01", billingDay: 1 }) | ||
|
|
||
| const { showUpcoming } = await import("../upcoming.ts") | ||
| showUpcoming(7) | ||
| expect(infoMessages.some((m) => m.includes("No upcoming bills"))).toBe(true) | ||
| }) | ||
|
|
||
| test("showUpcoming shows upcoming monthly subscription", async () => { | ||
| const db = await import("../db.ts") | ||
| // Create a subscription with billing day = 25 (tomorrow-ish) | ||
| const today = new Date() | ||
| const billingDay = today.getDate() + 1 > 28 ? 28 : today.getDate() + 1 | ||
| db.writeSubscription({ name: "Netflix", price: 1500, currency: "JPY", cycle: "monthly", tags: ["video"], status: "active", billingDay, createdAt: "2026-01-15" }) | ||
|
|
||
| const { showUpcoming } = await import("../upcoming.ts") | ||
| showUpcoming(30) | ||
| expect(logMessages.length).toBeGreaterThan(0) | ||
| const output = logMessages.join("\n") | ||
| expect(output).toContain("Netflix") | ||
| expect(output).toContain("¥1,500") | ||
| }) | ||
|
|
||
| test("showUpcoming excludes cancelled subscriptions", async () => { | ||
| const db = await import("../db.ts") | ||
| db.writeSubscription({ name: "Active", price: 100, currency: "USD", cycle: "monthly", tags: [], status: "active", billingDay: 28, createdAt: "2026-01-01" }) | ||
| db.writeSubscription({ name: "Cancelled", price: 200, currency: "USD", cycle: "monthly", tags: [], status: "cancelled", billingDay: 28, createdAt: "2026-01-01" }) | ||
|
|
||
| const { showUpcoming } = await import("../upcoming.ts") | ||
| showUpcoming(30) | ||
| const output = logMessages.join("\n") | ||
| expect(output).toContain("Active") | ||
| expect(output).not.toContain("Cancelled") | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether upcoming tests control system time.
# Expectation after the fix: this should find fake-timer setup such as useFakeTimers/setSystemTime/useRealTimers.
rg -n 'useFakeTimers|setSystemTime|useRealTimers' subtrack/src/__tests__/upcoming.test.tsRepository: nazozokc/subtrack
Length of output: 155
Freeze time in these tests to prevent flakiness.
The assertions in "no upcoming bills" and "upcoming monthly subscription" rely on new Date(), causing instability near billing thresholds. Implement jest.useFakeTimers() with a fixed date in beforeEach and jest.useRealTimers() in afterEach to ensure deterministic execution.
🤖 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__/upcoming.test.ts` around lines 73 - 114, The upcoming
subscription tests are time-sensitive because they depend on new Date() and can
flap around billing thresholds. In upcoming.test.ts, add deterministic time
control in the test setup by using jest.useFakeTimers() with a fixed system date
in beforeEach and restoring with jest.useRealTimers() in afterEach, so
showUpcoming and the monthly billing-day calculations behave consistently across
runs.
Comment on lines
+11
to
+33
| const list = getSubscriptions().filter((s) => s.status !== "cancelled") | ||
| if (list.length === 0) { | ||
| consola.info("No active subscriptions found") | ||
| return | ||
| } | ||
|
|
||
| const config = loadConfig() | ||
| const data = calcSummary(list) | ||
|
|
||
| // Header | ||
| consola.log(pc.bold("📊 Subscription Analytics")) | ||
| consola.log("") | ||
|
|
||
| // Overview | ||
| consola.log(pc.bold("Overview:")) | ||
| consola.log(` Total subscriptions: ${pc.bold(String(data.totalCount))}`) | ||
| consola.log(` Status breakdown:`) | ||
| const activeCount = list.filter((s) => s.status === "active").length | ||
| const pausedCount = list.filter((s) => s.status === "paused").length | ||
| const cancelledCount = getSubscriptions().filter((s) => s.status === "cancelled").length | ||
| consola.log(` ${pc.green(`active: ${activeCount}`)}`) | ||
| if (pausedCount > 0) consola.log(` ${pc.yellow(`paused: ${pausedCount}`)}`) | ||
| if (cancelledCount > 0) consola.log(` ${pc.red(`cancelled: ${cancelledCount}`)}`) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the SharedArgs/query contract for status (currently breaks typecheck).
Line 11 (and Lines 28-30) reads s.status, but CI fails with TS2339 and getSubscriptions’s shown query shape does not include status. This is a blocker: add status to the returned row shape (and SharedArgs typing), or stop using status here.
🧰 Tools
🪛 GitHub Actions: app-ci / 1_check _ check.txt
[error] 11-11: TypeScript (tsc --noEmit) failed: TS2339 Property 'status' does not exist on type 'SharedArgs'.
🪛 GitHub Actions: app-ci / check _ check
[error] 11-11: TypeScript (tsc --noEmit) error TS2339: Property 'status' does not exist on type 'SharedArgs'.
🤖 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/analytics.ts` around lines 11 - 33, The analytics flow in
`getSubscriptions`/`calcSummary` is reading `s.status`, but the current query
result and `SharedArgs` contract do not expose a `status` field, causing the
typecheck failure. Update the shared row/query typing so `status` is included in
the returned subscription shape, or refactor `analytics.ts` to avoid accessing
`status` in the `list.filter(...)` and `cancelledCount` logic; make sure the
types for `getSubscriptions`, `SharedArgs`, and any related subscription model
stay consistent.
Source: Pipeline failures
Comment on lines
+48
to
+58
| if (config.monthlyBudget > 0) { | ||
| const defaultCurrency = config.defaultCurrency || "USD" | ||
| // Sum all monthly costs in their original currencies | ||
| const monthlyTotal = list.reduce((sum, sub) => sum + sub.price * periodFactor(sub.cycle, "monthly"), 0) | ||
| // For budget display, use default currency as reference | ||
| const budgetDisplay = formatPrice(config.monthlyBudget, defaultCurrency) | ||
| const spentDisplay = formatPrice(monthlyTotal, "USD") | ||
| consola.log(` ${pc.dim("─".repeat(30))}`) | ||
| consola.log(` Budget: ${pc.bold(budgetDisplay)}`) | ||
| const remaining = config.monthlyBudget - monthlyTotal | ||
| const remainingDisplay = formatPrice(remaining, "USD") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Budget/tag totals are mixing currencies and hardcoding USD output.
Line 51 aggregates monthly totals across all currencies, then Lines 54/58/73 format as "USD" regardless of actual currency. This yields incorrect analytics in multi-currency data. Group by currency (like monthlyByCurrency) or apply explicit FX conversion before producing a single-currency budget/tag figure.
Also applies to: 67-74
🤖 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/analytics.ts` around lines 48 - 58, The budget/tag analytics in
analytics.ts are mixing amounts from different currencies and hardcoding USD in
the display path. Update the budget summary logic around the monthly aggregation
and the tag totals so values are either grouped per currency (for example using
a monthlyByCurrency-style breakdown) or explicitly converted to one reference
currency before formatting. Make sure the Budget, Spent, Remaining, and tag
output all use the same currency source instead of passing "USD" directly into
formatPrice.
| import pc from "picocolors" | ||
| import CliTable3 from "cli-table3" | ||
| import type { SharedArgs, Currency, LlmUsageEntry } from "./types.ts" | ||
| import type { SharedArgs, Currency, LlmUsageEntry, Status } from "./types.ts" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Status type contract is broken between display.ts and types.ts.
display.ts now imports Status and reads sub.status, but the provided types.ts contract for SharedArgs has no status field and no exported Status. This will fail type-checking and block the build. Please add Status + status to SharedArgs (or import from the correct module) so the new column has a valid upstream type contract.
Also applies to: 26-33
🤖 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` at line 4, The Status contract between display.ts
and types.ts is incomplete: display.ts imports Status and reads sub.status, but
SharedArgs does not define a status field and types.ts does not export Status.
Update the upstream type contract in types.ts by adding the Status type and the
status property to SharedArgs, or change display.ts to import Status from the
correct module, so the sub.status usage in display.ts is type-safe.
Comment on lines
+34
to
+45
| case "monthly": { | ||
| // Calculate next billing date based on anchor day | ||
| const candidate = new Date(fromDate.getFullYear(), fromDate.getMonth(), anchorDay) | ||
| if (candidate > fromDate) return candidate | ||
| // Move to next month | ||
| return new Date(fromDate.getFullYear(), fromDate.getMonth() + 1, anchorDay) | ||
| } | ||
| case "yearly": { | ||
| const candidate = new Date(fromDate.getFullYear(), anchorDate.getMonth(), anchorDay) | ||
| if (candidate > fromDate) return candidate | ||
| return new Date(fromDate.getFullYear() + 1, anchorDate.getMonth(), anchorDay) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include bills due today for monthly and yearly cycles.
The candidate > fromDate checks skip subscriptions whose billing date is today, even though the output has a “today” label and the later range check includes next >= now.
🐛 Proposed fix
- if (candidate > fromDate) return candidate
+ if (candidate >= fromDate) return candidate
...
- if (candidate > fromDate) return candidate
+ if (candidate >= fromDate) return candidate📝 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.
Suggested change
| case "monthly": { | |
| // Calculate next billing date based on anchor day | |
| const candidate = new Date(fromDate.getFullYear(), fromDate.getMonth(), anchorDay) | |
| if (candidate > fromDate) return candidate | |
| // Move to next month | |
| return new Date(fromDate.getFullYear(), fromDate.getMonth() + 1, anchorDay) | |
| } | |
| case "yearly": { | |
| const candidate = new Date(fromDate.getFullYear(), anchorDate.getMonth(), anchorDay) | |
| if (candidate > fromDate) return candidate | |
| return new Date(fromDate.getFullYear() + 1, anchorDate.getMonth(), anchorDay) | |
| } | |
| case "monthly": { | |
| // Calculate next billing date based on anchor day | |
| const candidate = new Date(fromDate.getFullYear(), fromDate.getMonth(), anchorDay) | |
| if (candidate >= fromDate) return candidate | |
| // Move to next month | |
| return new Date(fromDate.getFullYear(), fromDate.getMonth() + 1, anchorDay) | |
| } | |
| case "yearly": { | |
| const candidate = new Date(fromDate.getFullYear(), anchorDate.getMonth(), anchorDay) | |
| if (candidate >= fromDate) return candidate | |
| return new Date(fromDate.getFullYear() + 1, anchorDate.getMonth(), anchorDay) | |
| } |
🤖 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/upcoming.ts` around lines 34 - 45, The monthly and yearly
branches in upcoming() currently exclude bills due today by using a strict
candidate > fromDate check. Update the comparison logic so a candidate on the
same day is treated as upcoming as well, while keeping the existing range
handling intact; adjust both the "monthly" and "yearly" cases consistently
around the candidate/return logic.
Comment on lines
+57
to
+66
| case "quarterly": { | ||
| // Every 3 months from anchor | ||
| const monthsSince = (fromDate.getFullYear() - anchorDate.getFullYear()) * 12 + (fromDate.getMonth() - anchorDate.getMonth()) | ||
| const quartersSince = Math.ceil(monthsSince / 3) | ||
| return addMonths(new Date(anchorDate), quartersSince * 3) | ||
| } | ||
| case "semi-annual": { | ||
| const monthsSince = (fromDate.getFullYear() - anchorDate.getFullYear()) * 12 + (fromDate.getMonth() - anchorDate.getMonth()) | ||
| const halvesSince = Math.ceil(monthsSince / 6) | ||
| return addMonths(new Date(anchorDate), halvesSince * 6) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find tests that exercise quarterly/semi-annual billing-day and post-anchor-day behavior.
# Expectation after the fix: matching tests should cover quarterly and semi-annual cycles with billingDay overrides.
rg -n 'quarterly|semi-annual|billingDay' subtrack/src/__tests__Repository: nazozokc/subtrack
Length of output: 1745
🏁 Script executed:
# Inspect the actual implementation of quarterly/semi-annual logic in upcoming.ts to understand the current approach and confirm the bug details.
cat -n subtrack/src/upcoming.ts | sed -n '40,80p'Repository: nazozokc/subtrack
Length of output: 2354
Fix quarterly and semi-annual logic to respect billingDay and prevent past dates
The quarterly and semi-annual cases currently calculate intervals based on the raw anchorDate instead of the normalized anchorDay. This causes two issues:
- Custom
billingDayis ignored: The logic preserves the original day of the month fromcreatedAtrather than the configured billing day. - Past dates returned: Using
Math.ceilwith an anchor date in the current month can return the anchor date itself. If the anchor day has already passed this month, the result is a date in the past.
Update these branches to align with the yearly logic: calculate the target month based on the interval but set the day to anchorDay. Ensure the result strictly exceeds fromDate.
Diff context
// Current (subtrack/src/upcoming.ts, lines 57-66)
case "quarterly": {
const monthsSince = (fromDate.getFullYear() - anchorDate.getFullYear()) * 12 + (fromDate.getMonth() - anchorDate.getMonth())
const quartersSince = Math.ceil(monthsSince / 3)
return addMonths(new Date(anchorDate), quartersSince * 3) // ❌ Uses anchorDate day, risks past date
}
case "semi-annual": {
const monthsSince = (fromDate.getFullYear() - anchorDate.getFullYear()) * 12 + (fromDate.getMonth() - anchorDate.getMonth())
const halvesSince = Math.ceil(monthsSince / 6)
return addMonths(new Date(anchorDate), halvesSince * 6) // ❌ Uses anchorDate day, risks past date
}🤖 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/upcoming.ts` around lines 57 - 66, The quarterly and semi-annual
branches in upcoming.ts are using anchorDate directly, so they ignore the
normalized billingDay and can return a due date that is not after fromDate.
Update the logic in the quarterly and semi-annual cases to follow the same
pattern as the yearly path: compute the correct interval month, construct the
result from anchorDay rather than the original anchorDate day, and make sure the
returned date is strictly greater than fromDate.
Comment on lines
+104
to
+118
| const list = getSubscriptions().filter((s) => s.status !== "cancelled") | ||
| if (list.length === 0) { | ||
| consola.info("No active subscriptions found") | ||
| return | ||
| } | ||
|
|
||
| const now = new Date() | ||
| now.setHours(0, 0, 0, 0) | ||
| const endDate = new Date(now) | ||
| endDate.setDate(endDate.getDate() + days) | ||
|
|
||
| const entries: UpcomingEntry[] = [] | ||
|
|
||
| for (const sub of list) { | ||
| const next = calculateNextBilling(sub, now) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Load the fields this code now depends on.
showUpcoming() filters by s.status and calculateNextBilling() reads createdAt/billingDay, but getSubscriptions() currently selects only id, name, price, currency, and cycle in subtrack/src/db.ts:395-404. That means cancelled rows are not filtered and subscriptions without an injected billingDay can crash at sub.createdAt.split(...).
🐛 Proposed upstream contract fix
-SELECT id, name, price, currency, cycle FROM subscriptions ORDER BY ${field} ${order}
+SELECT
+ id,
+ name,
+ price,
+ currency,
+ cycle,
+ status,
+ billing_day AS billingDay,
+ created_at AS createdAt
+FROM subscriptions ORDER BY ${field} ${order}🤖 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/upcoming.ts` around lines 104 - 118, showUpcoming() now depends
on subscription status and billing metadata, so update the getSubscriptions()
query in db.ts to load the fields it reads: status, createdAt, and billingDay in
addition to the existing columns. Then ensure showUpcoming() can safely filter
cancelled subscriptions and calculateNextBilling() has the data it needs without
assuming billingDay was injected elsewhere.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修正内容
BUG 1: upcoming.ts — 複数通貨合計
BUG 2: opencode-scanner.ts / codex-scanner.ts — SQLインジェクション
?プレースホルダ）に変更BUG 3: cursor-scanner.ts / windsurf-scanner.ts — 動的テーブル名
BUG 4: display.ts — TOTAL判定
endsWith("TOTAL")から空カラム判定に変更確認
Summary by CodeRabbit
New Features
Bug Fixes
Tests