Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughThis PR extends subscriptions with contract, auto-renewal, vendor, plan tier, and discount metadata across schema, types, DB layer, display, export, CSV import, and CLI commands. It adds currency conversion to calendar/upcoming/timeline/optimize, multi-channel notifications (email/Slack/webhook), interactive tag/profile prompts, and expanded JSON output modes, alongside corresponding test updates. ChangesExtended subscription metadata, currency conversion, and CLI updates
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant handleUpcoming
participant calcUpcomingWithCurrency
participant fetchFxRates
CLI->>handleUpcoming: days, options.currency
handleUpcoming->>calcUpcomingWithCurrency: request converted entries
calcUpcomingWithCurrency->>fetchFxRates: get FX rates
fetchFxRates-->>calcUpcomingWithCurrency: rates or error
calcUpcomingWithCurrency-->>handleUpcoming: converted/original entries
handleUpcoming-->>CLI: display or JSON output
sequenceDiagram
participant CLI
participant handleNotify
participant sendEmailNotification
participant sendSlackNotification
participant sendWebhookNotification
CLI->>handleNotify: options.channel or config.notifyChannels
handleNotify->>sendEmailNotification: channel includes "email"
handleNotify->>sendSlackNotification: channel includes "slack"
handleNotify->>sendWebhookNotification: channel includes "webhook"
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…tus filters, and interactive modes
![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: 20
🧹 Nitpick comments (9)
subtrack/src/profile.ts (1)
146-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on
handleProfile.The function signature and behavior changed substantially (async, new interactive save/switch/delete flows) but remains undocumented.
As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript" applies to
subtrack/**/*.{js,jsx,ts,tsx}.🤖 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/profile.ts` at line 146, The public API handleProfile is missing JSDoc despite its behavior changes and new async save/switch/delete flows. Add a JSDoc comment directly above handleProfile describing its purpose, parameters (command, name, filter), and that it returns a Promise<void>, matching the documented public API convention used in subtrack.Source: Coding guidelines
subtrack/src/tag.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on modified public APIs.
handleTagList,handleTagRename, andhandleTagDeleteall gained new parameters/behavior (sorting, JSON output, interactive prompts, backward-compat quirks) but remain undocumented.As per coding guidelines, "Document public APIs with JSDoc comments in JavaScript/TypeScript" applies to
subtrack/**/*.{js,jsx,ts,tsx}.Also applies to: 37-37, 74-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/tag.ts` at line 6, The public tag command APIs are missing JSDoc after their behavior changes. Add concise JSDoc comments to handleTagList, handleTagRename, and handleTagDelete describing their purpose and the new flags/behavior (sorting, JSON output, interactive prompts, and any backward-compat quirks) so the exported functions in tag.ts are documented per the project guideline.Source: Coding guidelines
subtrack/src/commands.ts (1)
281-285: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the dynamic
periodFactorimport out of the loop.
await import("./date-utils.ts")inside theforloop re-resolves the module on every iteration. Import it once before the loop (or statically) and reuse.♻️ Proposed change
const activeSubs = subs.filter((s) => s.status !== "cancelled") const currentTotals: Record<string, number> = {} + const { periodFactor } = await import("./date-utils.ts") for (const sub of activeSubs) { - const { periodFactor } = await import("./date-utils.ts") const monthly = sub.price * periodFactor(sub.cycle, "monthly") currentTotals[sub.currency] = (currentTotals[sub.currency] ?? 0) + monthly }🤖 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 281 - 285, The dynamic import of periodFactor inside the activeSubs loop causes the module to be resolved on every iteration. Hoist the import in commands.ts out of the for-of block in the code that processes activeSubs, or switch to a static import, and then reuse the single periodFactor reference when calculating monthly for each sub.subtrack/src/__tests__/forecast.test.ts (1)
74-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting a shared test schema fixture.
This same 8-column block (
contract_start…discount_type) is now duplicated verbatim across at least 6 test files (forecast, mcp, optimize, search, timeline, trial). A sharedcreateTestSchema(db)helper or SQL fragment in a test-utils module would prevent future schema drift across these fixtures.🤖 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__/forecast.test.ts` around lines 74 - 82, The test schema block for the contract and discount columns is duplicated across multiple test suites, so extract it into a shared test helper or SQL fragment to keep the fixtures consistent. Add a reusable schema fixture such as a createTestSchema(db) helper in the test-utils area, and update the forecast test and the other affected suites to call that shared setup instead of inlining the same columns repeatedly.subtrack/src/search.ts (2)
46-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatus/price/limit filtering duplicated across
handleSearch,handleList, andhandleExport.The status-split-lowercase-filter, min/max price filter, and limit-slice logic here is essentially identical to what's in
handleList(subscription.ts) andhandleExport(commands.ts) per the supplied graph evidence. Extracting a shared helper (e.g.applyCommonFilters(list, { status, minPrice, maxPrice, limit })) would reduce drift risk as these filters evolve.🤖 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/search.ts` around lines 46 - 65, The status, price, and limit filtering logic in `handleSearch` is duplicated in `handleList` and `handleExport`, so extract it into a shared helper such as `applyCommonFilters(...)` and have all three paths call that helper. Keep the existing behavior for status normalization, min/max price checks, and limit slicing, but centralize the implementation in the relevant search/subscription command modules to avoid drift.
114-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual SharedArgs mapping duplicates
SUBSCRIPTION_COLS/toSharedArgs.This hand-written
SELECTcolumn list and field-by-fieldNumber()/String()mapping duplicates the same conceptual "row → SharedArgs" conversion thatdb/subscriptions.tsnow centralizes viaSUBSCRIPTION_COLSandtoSharedArgs. If a future column is added toSUBSCRIPTION_COLS, this query won't automatically pick it up (already true today — it's a hand-maintained separate list), risking silent drift betweengetSubscriptions()/getSubscription()results andsearchSubscriptions()results.🤖 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/search.ts` around lines 114 - 146, The searchSubscriptions row mapping is manually duplicating the SharedArgs conversion already centralized in SUBSCRIPTION_COLS/toSharedArgs. Update search.ts so the query and result shaping reuse the same shared subscription column list and mapping helper instead of a hand-written SELECT plus per-field String()/Number() conversion, keeping searchSubscriptions() aligned with db/subscriptions.ts.subtrack/src/import-csv.ts (1)
72-132: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional hardening: guard against
__proto__/constructorCSV headers.Static analysis flagged the
map[h] = headerToField[h]assignment as a prototype-pollution pattern (CWE-1321). In practice,headerToField["__proto__"]resolves to the built-inObject.prototypereference (not an attacker-supplied object), and assigning that back only reassignsmap's own prototype to itself — no new enumerable properties are added, andObject.entries()later won't surface it, so this isn't practically exploitable here. Still, adding an explicit skip for dangerous keys is a cheap defense-in-depth measure against future refactors of this pattern.🔒 Optional guard
for (const h of lowerHeaders) { - if (headerToField[h]) { + if (h === "__proto__" || h === "constructor" || h === "prototype") continue + if (headerToField[h]) { map[h] = headerToField[h] } }🤖 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/import-csv.ts` around lines 72 - 132, The autoDetectFieldMap function should defensively skip dangerous CSV header keys to avoid prototype-pollution patterns. Update the header-to-field mapping loop in autoDetectFieldMap so it ignores __proto__, constructor, and prototype before assigning into map, while keeping the existing exact-match behavior for normal headers.Source: Linters/SAST tools
subtrack/src/subscription.ts (1)
353-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate JSON-shaping logic between
handleListandhandleTags.The JSON output object construction (id/name/price/.../discountType) here and in
handleTags(lines 462-483) is identical. A future field addition risks updating only one of the two.♻️ Proposed refactor
+function toJsonSubscription(sub: SharedArgs) { + return { + id: sub.id, name: sub.name, price: sub.price, currency: sub.currency, + cycle: sub.cycle, status: sub.status, tags: sub.tags, billingDay: sub.billingDay, + notes: sub.notes, paymentMethod: sub.paymentMethod, + contractStart: sub.contractStart, contractEnd: sub.contractEnd, + autoRenewal: sub.autoRenewal, vendorName: sub.vendorName, vendorUrl: sub.vendorUrl, + planTier: sub.planTier, discountAmount: sub.discountAmount, discountType: sub.discountType, + createdAt: sub.createdAt, + } +}Then call
list.map(toJsonSubscription)in bothhandleListandhandleTags.🤖 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 353 - 378, The JSON object shaping in handleList is duplicated in handleTags, so extract the shared subscription-to-JSON mapping into a reusable helper such as toJsonSubscription and use it in both handlers. Keep the helper near the existing list output logic in subscription.ts, and replace both list.map blocks with list.map(toJsonSubscription) so future field changes only need one update.subtrack/src/db/schema.ts (1)
87-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap multi-statement migration in a transaction.
Eight sequential
ALTER TABLEcalls execute outside a transaction. If the process is interrupted mid-migration (crash, OOM), the schema is left partially migrated (e.g.,contract_startadded butdiscount_typemissing), andhasContractStartwould then incorrectly treat the table as already migrated on next boot since it only checks forcontract_start.🔧 Proposed fix
if (!hasContractStart) { - db.run("ALTER TABLE subscriptions ADD COLUMN contract_start TEXT") - db.run("ALTER TABLE subscriptions ADD COLUMN contract_end TEXT") - db.run("ALTER TABLE subscriptions ADD COLUMN auto_renewal INTEGER NOT NULL DEFAULT 1") - db.run("ALTER TABLE subscriptions ADD COLUMN vendor_name TEXT") - db.run("ALTER TABLE subscriptions ADD COLUMN vendor_url TEXT") - db.run("ALTER TABLE subscriptions ADD COLUMN plan_tier TEXT") - db.run("ALTER TABLE subscriptions ADD COLUMN discount_amount INTEGER") - db.run("ALTER TABLE subscriptions ADD COLUMN discount_type TEXT") + db.run("BEGIN TRANSACTION") + try { + db.run("ALTER TABLE subscriptions ADD COLUMN contract_start TEXT") + db.run("ALTER TABLE subscriptions ADD COLUMN contract_end TEXT") + db.run("ALTER TABLE subscriptions ADD COLUMN auto_renewal INTEGER NOT NULL DEFAULT 1") + db.run("ALTER TABLE subscriptions ADD COLUMN vendor_name TEXT") + db.run("ALTER TABLE subscriptions ADD COLUMN vendor_url TEXT") + db.run("ALTER TABLE subscriptions ADD COLUMN plan_tier TEXT") + db.run("ALTER TABLE subscriptions ADD COLUMN discount_amount INTEGER") + db.run("ALTER TABLE subscriptions ADD COLUMN discount_type TEXT") + db.run("COMMIT") + } catch (e) { + db.run("ROLLBACK") + throw e + } }As per coding guidelines, "Use
sql.jswithPRAGMA foreign_keys = ONand transactions for multi-step database 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/src/db/schema.ts` around lines 87 - 101, The contract-management migration in schema.ts runs several ALTER TABLE statements independently, so a failure can leave subscriptions partially migrated while hasContractStart still only checks for contract_start. Wrap the full migration block in a single transaction using the existing db object, and commit only after all column additions succeed so the schema is applied atomically. Keep the migration guard around hasContractStart, but make the multi-statement path in this schema initialization code transactional.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/commands.ts`:
- Around line 254-266: handleAnalytics currently ignores the
AnalyticsOptions.currency value, so the analytics output silently skips FX
conversion. Update handleAnalytics to pass and apply the requested currency in
both the JSON branch and showAnalytics path, using the same currency conversion
approach as handleOptimize and showCalendar, or remove currency from
AnalyticsOptions/index.ts if analytics should not support it. Make sure
calcSummary and any underlying analytics rendering receive the converted
subscription prices before producing totals.
- Around line 272-292: The JSON path in handleCompare is emitting two different
response shapes and is skipping the --currency conversion. Update the
options.json branch in commands.ts so the empty and non-empty cases return the
same stable keys as the compare contract, and make sure current totals are
computed through the same conversion logic used by the non-JSON showCompare flow
(including convertPrice when options.currency is set) rather than aggregating
raw sub.price by sub.currency.
In `@subtrack/src/config.ts`:
- Around line 108-111: The notifyEmail config assignment in the config switch
stores the value verbatim, which can later be injected into the To header used
by sendEmailNotification. Update the notifyEmail handling in config.ts to
validate that the provided value is a single well-formed email address and
reject anything with newlines or header-like content before assigning it to
config.notifyEmail. Use the notifyEmail case and the sendEmailNotification flow
in notify.ts as the key places to verify the fix.
In `@subtrack/src/display.ts`:
- Around line 230-245: The column config selection in the display logic only
covers a few fixed flag combinations, but buildRow adds columns independently
for each enabled flag, so some valid CLI mixes produce mismatched headers and
row widths. Update the config generation in display.ts to mirror buildRow’s
insertion order instead of using the current if/else chain, and make sure the
same symbols (buildRow and the config selection near
BASE_COLS/ALL_COLS/CONTRACT_COLS/etc.) stay aligned for every combination of
showNotes, showMethod, showContract, and showVendor.
In `@subtrack/src/import-csv.ts`:
- Around line 51-70: `--map` cannot match the billing-day field because
`parseFieldMap` lowercases the target but `KNOWN_FIELDS` still uses mixed-case
`billingDay`. Update the known field handling in `parseFieldMap` and related
lookup logic so billing-day is canonicalized consistently, and make sure the
`getField`/field-name checks accept the same normalized name everywhere. This
should let `--map 'col:billingDay'` (and equivalent casing) resolve correctly
without affecting `autoDetectFieldMap`.
- Around line 283-351: The optional vendor/plan/discount CSV fields are being
passed straight into writeSubscription without validation, so invalid values can
be persisted. In import-csv.ts, add validation for vendorName, vendorUrl,
planTier, discountAmountStr, and discountType alongside the existing validators
for notes/paymentMethod, using the same warning-and-continue pattern before the
writeSubscription call. Ensure vendorUrl is a valid URL, vendorName and planTier
respect any length/format limits, discountAmountStr parses to a finite number,
and discountType is restricted to the allowed values referenced in the parsing
logic.
In `@subtrack/src/index.ts`:
- Line 55: The import in index.ts uses a trailing semicolon, which violates the
project’s no-semicolons import/export style. Remove the semicolon from the
import statement and keep it consistent with the nearby import declarations; the
fix is in the top-level import block around the Cycle, Status, NotifyChannel,
UsageRefreshFlags, and ListFlags import.
- Around line 1336-1340: The timeline option definition for currency is using a
short alias that conflicts with categories, making one CLI flag unusable. Update
the `currency` option in `subtrack/src/index.ts` to use a different short alias
than `categories`, and keep the change localized to the option declarations so
each flag remains uniquely identifiable.
- Around line 122-146: The run handler in the command setup is invoking
handleList without returning its promise, so gunshi cannot await it and
errors/output ordering may be lost. Update this run callback to return the
handleList call, and apply the same pattern to the other run handlers that call
handleSearch, handleForecast, handleOptimize, handleTimeline, handleCalendar,
and handleUpcoming so each async operation is properly awaited by gunshi.
In `@subtrack/src/notify.ts`:
- Around line 160-168: Both webhook POSTs in sendSlackNotification and
sendWebhookNotification can hang indefinitely because globalThis.fetch has no
timeout. Update the fetch options in notify.ts to pass an
AbortSignal.timeout(...) signal for each POST, using the same pattern in the
webhookUrl request and the Slack request so both requests fail fast if the
endpoint is unresponsive.
- Around line 104-119: The email notification body is reading the wrong date
field, so `sendEmailNotification` should use the top-level `entry.nextDate`
provided by `calcUpcoming` instead of `e.sub.nextDate`. Update the
`entries.map(...)` logic in `sendEmailNotification` to reference the entry-level
date when formatting each line, keeping the existing fallback to "upcoming" only
when that top-level value is missing.
In `@subtrack/src/optimize.ts`:
- Around line 387-400: The FX conversion in optimize.ts currently converts all
subscriptions in a single subs.map pass, so one unsupported currency can fail
the whole savings report and leave prices inconsistent. Update the conversion
logic around fetchFxRates, convertPrice, and the subs mapping to handle each
subscription individually with a per-item fallback like calendar.ts/upcoming.ts,
so a bad rate or unsupported currency only affects that item and the rest of the
report still converts correctly.
In `@subtrack/src/profile.ts`:
- Around line 208-222: The interactive profile delete flow in
handleProfileCommand currently deletes immediately after selecting a profile,
unlike the sibling handleTagDelete path that confirms first. Update the "delete"
case to add a confirmation step after the name is chosen and before calling
deleteProfile(name), and only proceed when the user explicitly confirms; if they
cancel, return without deleting.
In `@subtrack/src/prompts.ts`:
- Around line 120-129: validateVendorUrl currently accepts any URL scheme
because it only calls new URL(v), but the error message implies http/https-only.
Update validateVendorUrl to explicitly allow only http: and https: after parsing
the URL, and return the existing invalid-URL message for any other scheme. Keep
the same function signature and the current length/empty-string behavior.
- Around line 107-113: `validateDateString` currently accepts impossible
calendar dates because `new Date(v + "T00:00:00")` normalizes them instead of
rejecting them. Update the date check in `validateDateString` to parse the
year/month/day from the input and verify the constructed Date round-trips to the
same values, so invalid inputs like 2023-02-30 are rejected before being stored
in contractStart/contractEnd.
In `@subtrack/src/subscription.ts`:
- Around line 315-338: The handleList list path treats --tags differently from
the normal subscription listing, so the default active+paused filter is skipped
when tags are used. Update handleList so the tagsSubscription branch still
applies the same default status filter as getSubscriptions unless options.status
or options.all is set, and keep the post-fetch filtering in sync with
statusFilter. Use the existing handleList, tagsSubscription, and
getSubscriptions flow to ensure subtrack list --tags matches the default
behavior of subtrack list.
- Around line 253-279: The non-interactive discount parsing in subscription.ts
ignores flags.discountType whenever flags.discountAmount is provided, leaving
discountType null. Update the discount parsing logic around the
discountAmount/discountType handling to also read and validate
flags.discountType in the CLI-flag path, using validateDiscountType and the
existing discountType assignment. Keep the prompted flow intact, but ensure both
flags work together in the add command without relying on the interactive
prompt.
- Around line 564-595: The non-interactive edit path in subscription.ts writes
the new flag fields directly into newData without reusing the existing
validators from prompts.ts. Update the flags handling for contractStart,
contractEnd, vendorName, vendorUrl, planTier, discountAmount, and discountType
to run through validateDateString, validateVendorUrl, validateDiscountValue, and
validateDiscountType before assignment, matching the interactive edit flow. In
particular, ensure flags.discountAmount cannot become NaN and that invalid
values are rejected or normalized to null. Also mirror the interactive discount
behavior by clearing discountType whenever discountAmount is emptied so newData
cannot end up with discountAmount null but a stale discountType.
In `@subtrack/src/tag.ts`:
- Around line 56-61: The interactive rename flow in tag.ts accepts a valid value
in the input validation but then passes the raw string to renameTag, so
whitespace-padded names can be persisted. Update the newName handling in the
input/renameTag path to trim the value before it is used, and ensure the trimmed
value is what gets passed into renameTag(oldName, newName) while keeping the
existing non-empty validation behavior.
In `@subtrack/src/timeline.ts`:
- Around line 213-226: The currency conversion in the activeSubs mapping is too
brittle because one missing FX rate can abort the entire conversion pass and
still leave displayCurrency reset to USD. Update the options.currency branch in
timeline.ts to convert each subscription individually inside the activeSubs.map
logic, using convertPrice and fetchFxRates results per item while preserving the
original subscription when conversion fails. Keep the original currency entry
for any subscription that cannot be converted, and make sure the display label
only changes when the conversions actually succeed, consistent with the other
currency-aware commands.
---
Nitpick comments:
In `@subtrack/src/__tests__/forecast.test.ts`:
- Around line 74-82: The test schema block for the contract and discount columns
is duplicated across multiple test suites, so extract it into a shared test
helper or SQL fragment to keep the fixtures consistent. Add a reusable schema
fixture such as a createTestSchema(db) helper in the test-utils area, and update
the forecast test and the other affected suites to call that shared setup
instead of inlining the same columns repeatedly.
In `@subtrack/src/commands.ts`:
- Around line 281-285: The dynamic import of periodFactor inside the activeSubs
loop causes the module to be resolved on every iteration. Hoist the import in
commands.ts out of the for-of block in the code that processes activeSubs, or
switch to a static import, and then reuse the single periodFactor reference when
calculating monthly for each sub.
In `@subtrack/src/db/schema.ts`:
- Around line 87-101: The contract-management migration in schema.ts runs
several ALTER TABLE statements independently, so a failure can leave
subscriptions partially migrated while hasContractStart still only checks for
contract_start. Wrap the full migration block in a single transaction using the
existing db object, and commit only after all column additions succeed so the
schema is applied atomically. Keep the migration guard around hasContractStart,
but make the multi-statement path in this schema initialization code
transactional.
In `@subtrack/src/import-csv.ts`:
- Around line 72-132: The autoDetectFieldMap function should defensively skip
dangerous CSV header keys to avoid prototype-pollution patterns. Update the
header-to-field mapping loop in autoDetectFieldMap so it ignores __proto__,
constructor, and prototype before assigning into map, while keeping the existing
exact-match behavior for normal headers.
In `@subtrack/src/profile.ts`:
- Line 146: The public API handleProfile is missing JSDoc despite its behavior
changes and new async save/switch/delete flows. Add a JSDoc comment directly
above handleProfile describing its purpose, parameters (command, name, filter),
and that it returns a Promise<void>, matching the documented public API
convention used in subtrack.
In `@subtrack/src/search.ts`:
- Around line 46-65: The status, price, and limit filtering logic in
`handleSearch` is duplicated in `handleList` and `handleExport`, so extract it
into a shared helper such as `applyCommonFilters(...)` and have all three paths
call that helper. Keep the existing behavior for status normalization, min/max
price checks, and limit slicing, but centralize the implementation in the
relevant search/subscription command modules to avoid drift.
- Around line 114-146: The searchSubscriptions row mapping is manually
duplicating the SharedArgs conversion already centralized in
SUBSCRIPTION_COLS/toSharedArgs. Update search.ts so the query and result shaping
reuse the same shared subscription column list and mapping helper instead of a
hand-written SELECT plus per-field String()/Number() conversion, keeping
searchSubscriptions() aligned with db/subscriptions.ts.
In `@subtrack/src/subscription.ts`:
- Around line 353-378: The JSON object shaping in handleList is duplicated in
handleTags, so extract the shared subscription-to-JSON mapping into a reusable
helper such as toJsonSubscription and use it in both handlers. Keep the helper
near the existing list output logic in subscription.ts, and replace both
list.map blocks with list.map(toJsonSubscription) so future field changes only
need one update.
In `@subtrack/src/tag.ts`:
- Line 6: The public tag command APIs are missing JSDoc after their behavior
changes. Add concise JSDoc comments to handleTagList, handleTagRename, and
handleTagDelete describing their purpose and the new flags/behavior (sorting,
JSON output, interactive prompts, and any backward-compat quirks) so the
exported functions in tag.ts are documented per the project guideline.
🪄 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: 2b4528d4-b349-4f9a-b70d-b58db590a981
📒 Files selected for processing (34)
subtrack/src/__tests__/analytics.test.tssubtrack/src/__tests__/bulk.test.tssubtrack/src/__tests__/commands.test.tssubtrack/src/__tests__/db.test.tssubtrack/src/__tests__/display.test.tssubtrack/src/__tests__/forecast.test.tssubtrack/src/__tests__/mcp.test.tssubtrack/src/__tests__/optimize.test.tssubtrack/src/__tests__/search.test.tssubtrack/src/__tests__/timeline.test.tssubtrack/src/__tests__/trial.test.tssubtrack/src/__tests__/untested-commands.test.tssubtrack/src/__tests__/upcoming.test.tssubtrack/src/calendar.tssubtrack/src/commands.tssubtrack/src/config.tssubtrack/src/db/schema.tssubtrack/src/db/subscriptions.tssubtrack/src/display.tssubtrack/src/export.tssubtrack/src/forecast.tssubtrack/src/import-csv.tssubtrack/src/index.tssubtrack/src/notify.tssubtrack/src/optimize.tssubtrack/src/profile.tssubtrack/src/prompts.tssubtrack/src/search.tssubtrack/src/subscription.tssubtrack/src/tag.tssubtrack/src/timeline.tssubtrack/src/trial.tssubtrack/src/types.tssubtrack/src/upcoming.ts
Comment on lines
+254
to
266
| export function handleAnalytics(options: AnalyticsOptions = {}): void { | ||
| if (options.json) { | ||
| const subs = getSubscriptions(undefined, undefined, "active,paused") | ||
| if (subs.length === 0) { | ||
| process.stdout.write(JSON.stringify({ totalCount: 0, monthlyByCurrency: {}, monthlyByTag: {} }, null, 2) + "\n") | ||
| return | ||
| } | ||
| const data = calcSummary(subs) | ||
| process.stdout.write(JSON.stringify(data, null, 2) + "\n") | ||
| return | ||
| } | ||
| showAnalytics() | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--currency is accepted for analytics but never applied.
AnalyticsOptions includes currency, and index.ts forwards it (handleAnalytics({ json, currency })), but neither the JSON path (calcSummary(subs)) nor showAnalytics() performs any FX conversion. The flag is silently ignored, unlike the other currency-aware commands in this PR.
Consider converting subs prices when options.currency is set (mirroring handleOptimize/showCalendar), or drop the flag to avoid a misleading option.
🤖 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 254 - 266, handleAnalytics currently
ignores the AnalyticsOptions.currency value, so the analytics output silently
skips FX conversion. Update handleAnalytics to pass and apply the requested
currency in both the JSON branch and showAnalytics path, using the same currency
conversion approach as handleOptimize and showCalendar, or remove currency from
AnalyticsOptions/index.ts if analytics should not support it. Make sure
calcSummary and any underlying analytics rendering receive the converted
subscription prices before producing totals.
Comment on lines
+272
to
+292
| if (options.json) { | ||
| const subs = getSubscriptions(undefined, undefined, "active,paused") | ||
| if (subs.length === 0) { | ||
| process.stdout.write(JSON.stringify({ period, current: {}, previous: {}, change: {} }, null, 2) + "\n") | ||
| return | ||
| } | ||
|
|
||
| const activeSubs = subs.filter((s) => s.status !== "cancelled") | ||
| const currentTotals: Record<string, number> = {} | ||
| for (const sub of activeSubs) { | ||
| const { periodFactor } = await import("./date-utils.ts") | ||
| const monthly = sub.price * periodFactor(sub.cycle, "monthly") | ||
| currentTotals[sub.currency] = (currentTotals[sub.currency] ?? 0) + monthly | ||
| } | ||
|
|
||
| process.stdout.write(JSON.stringify({ | ||
| period, | ||
| currentPeriod: currentTotals, | ||
| subscriptions: activeSubs.length, | ||
| }, null, 2) + "\n") | ||
| return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare JSON output is inconsistent and ignores --currency.
Two problems in the JSON branch:
- The empty-result shape (
{ period, current: {}, previous: {}, change: {} }) uses different keys than the populated shape ({ period, currentPeriod, subscriptions }), so consumers can't rely on a stable contract. options.currencyis not applied here —currentTotalsis built per original currency with noconvertPricecall — even though the non-JSONshowComparepath honors it.handleCompare("monthly", { currency: "USD", json: true })will not convert.
Align both branches on one shape and apply the currency conversion (or explicitly document that JSON compare is per-currency only).
🤖 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 272 - 292, The JSON path in
handleCompare is emitting two different response shapes and is skipping the
--currency conversion. Update the options.json branch in commands.ts so the
empty and non-empty cases return the same stable keys as the compare contract,
and make sure current totals are computed through the same conversion logic used
by the non-JSON showCompare flow (including convertPrice when options.currency
is set) rather than aggregating raw sub.price by sub.currency.
Comment on lines
+108
to
+111
| case "notifyEmail": { | ||
| config.notifyEmail = value | ||
| break | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Validate notifyEmail to prevent email header injection.
notifyEmail is stored verbatim and later interpolated into the To: header passed to sendmail -t (see sendEmailNotification in notify.ts, Line 129). A value containing a newline (e.g. you@example.com\nBcc: someone@evil.com) would inject additional headers. Reject values that aren't a single, well-formed address at the point of configuration.
🛡️ Proposed validation
case "notifyEmail": {
+ if (value && (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value))) {
+ consola.error(`Invalid email address: "${value}"`)
+ return false
+ }
config.notifyEmail = value
break
}📝 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 "notifyEmail": { | |
| config.notifyEmail = value | |
| break | |
| } | |
| case "notifyEmail": { | |
| if (value && (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value))) { | |
| consola.error(`Invalid email address: "${value}"`) | |
| return false | |
| } | |
| config.notifyEmail = value | |
| break | |
| } |
🤖 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 108 - 111, The notifyEmail config
assignment in the config switch stores the value verbatim, which can later be
injected into the To header used by sendEmailNotification. Update the
notifyEmail handling in config.ts to validate that the provided value is a
single well-formed email address and reject anything with newlines or
header-like content before assigning it to config.notifyEmail. Use the
notifyEmail case and the sendEmailNotification flow in notify.ts as the key
places to verify the fix.
Comment on lines
+51
to
+70
| const KNOWN_FIELDS = [ | ||
| "name", "price", "currency", "cycle", "tags", | ||
| "notes", "status", "payment_method", "billing_day", "billingDay", | ||
| "contract_start", "contract_end", "auto_renewal", | ||
| "vendor_name", "vendor_url", "plan_tier", | ||
| "discount_amount", "discount_type", | ||
| ] as const | ||
|
|
||
| type FieldMap = Record<string, string> // CSV header -> field name | ||
|
|
||
| function parseFieldMap(mapStr: string): FieldMap { | ||
| const map: FieldMap = {} | ||
| for (const pair of mapStr.split(",")) { | ||
| const [csvCol, field] = pair.split(":").map((s) => s.trim().toLowerCase()) | ||
| if (csvCol && field && (KNOWN_FIELDS as readonly string[]).includes(field)) { | ||
| map[csvCol] = field | ||
| } | ||
| } | ||
| return map | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--map can never target the billing-day field due to case mismatch.
parseFieldMap always lowercases the target field (line 64: .toLowerCase()), then checks membership against KNOWN_FIELDS. But KNOWN_FIELDS contains "billingDay" (mixed-case) rather than an all-lowercase variant. Since field is forced to "billingday", it will never strictly equal "billing_day" or "billingDay", so --map 'col:billingDay' (or any casing) silently fails to map, even though billingDay is a documented known field. Auto-detect (autoDetectFieldMap) is unaffected since it looks up the literal dictionary value directly.
🐛 Proposed fix
const KNOWN_FIELDS = [
"name", "price", "currency", "cycle", "tags",
- "notes", "status", "payment_method", "billing_day", "billingDay",
+ "notes", "status", "payment_method", "billing_day", "billingday",
"contract_start", "contract_end", "auto_renewal",
"vendor_name", "vendor_url", "plan_tier",
"discount_amount", "discount_type",
] as constand normalize the corresponding colIndex key lookup (getField("billingDay") ?? getField("billing_day") ?? getField("billingday")), or simplest: canonicalize to a single field name (billingDay) everywhere and always lowercase-compare.
🤖 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/import-csv.ts` around lines 51 - 70, `--map` cannot match the
billing-day field because `parseFieldMap` lowercases the target but
`KNOWN_FIELDS` still uses mixed-case `billingDay`. Update the known field
handling in `parseFieldMap` and related lookup logic so billing-day is
canonicalized consistently, and make sure the `getField`/field-name checks
accept the same normalized name everywhere. This should let `--map
'col:billingDay'` (and equivalent casing) resolve correctly without affecting
`autoDetectFieldMap`.
Comment on lines
+253
to
+279
| // discountAmount & discountType: optional | ||
| let discountAmount: number | null = null | ||
| let discountType: "percentage" | "fixed" | null = null | ||
| if (flags.discountAmount !== undefined) { | ||
| const trimmed = flags.discountAmount.trim() | ||
| if (trimmed) { | ||
| const valid = validateDiscountValue(trimmed) | ||
| if (valid !== true) { consola.error(valid); return null } | ||
| discountAmount = Number(trimmed) | ||
| } | ||
| } else if (prompted) { | ||
| const da = await input({ | ||
| message: "discount amount (optional, e.g. 20 for 20% or $20 off)", | ||
| validate: validateDiscountValue, | ||
| }) | ||
| if (da.trim()) { | ||
| discountAmount = Number(da) | ||
| // Ask for type if amount was entered | ||
| const dt = await input({ | ||
| message: "discount type (percentage or fixed, optional)", | ||
| validate: validateDiscountType, | ||
| }) | ||
| if (dt.trim() && (dt.trim() === "percentage" || dt.trim() === "fixed")) { | ||
| discountType = dt.trim() as "percentage" | "fixed" | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--discount-type flag is silently ignored on non-interactive add.
When flags.discountAmount !== undefined (i.e. discount amount supplied as a CLI flag, non-interactively), discountType is never read from flags.discountType — it stays null regardless of what the user passes for --discount-type. The prompt for discountType only happens in the else if (prompted) branch, which is skipped entirely whenever --discount-amount is provided as a flag. A user running subtrack add --discount-amount 20 --discount-type percentage ... (non-interactively) will silently lose the discount type.
🐛 Proposed fix
if (flags.discountAmount !== undefined) {
const trimmed = flags.discountAmount.trim()
if (trimmed) {
const valid = validateDiscountValue(trimmed)
if (valid !== true) { consola.error(valid); return null }
discountAmount = Number(trimmed)
+ if (flags.discountType !== undefined) {
+ const dtTrimmed = flags.discountType.trim()
+ const dtValid = validateDiscountType(dtTrimmed)
+ if (dtValid !== true) { consola.error(dtValid); return null }
+ if (dtTrimmed === "percentage" || dtTrimmed === "fixed") {
+ discountType = dtTrimmed
+ }
+ }
}
} else if (prompted) {📝 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
| // discountAmount & discountType: optional | |
| let discountAmount: number | null = null | |
| let discountType: "percentage" | "fixed" | null = null | |
| if (flags.discountAmount !== undefined) { | |
| const trimmed = flags.discountAmount.trim() | |
| if (trimmed) { | |
| const valid = validateDiscountValue(trimmed) | |
| if (valid !== true) { consola.error(valid); return null } | |
| discountAmount = Number(trimmed) | |
| } | |
| } else if (prompted) { | |
| const da = await input({ | |
| message: "discount amount (optional, e.g. 20 for 20% or $20 off)", | |
| validate: validateDiscountValue, | |
| }) | |
| if (da.trim()) { | |
| discountAmount = Number(da) | |
| // Ask for type if amount was entered | |
| const dt = await input({ | |
| message: "discount type (percentage or fixed, optional)", | |
| validate: validateDiscountType, | |
| }) | |
| if (dt.trim() && (dt.trim() === "percentage" || dt.trim() === "fixed")) { | |
| discountType = dt.trim() as "percentage" | "fixed" | |
| } | |
| } | |
| } | |
| // discountAmount & discountType: optional | |
| let discountAmount: number | null = null | |
| let discountType: "percentage" | "fixed" | null = null | |
| if (flags.discountAmount !== undefined) { | |
| const trimmed = flags.discountAmount.trim() | |
| if (trimmed) { | |
| const valid = validateDiscountValue(trimmed) | |
| if (valid !== true) { consola.error(valid); return null } | |
| discountAmount = Number(trimmed) | |
| if (flags.discountType !== undefined) { | |
| const dtTrimmed = flags.discountType.trim() | |
| const dtValid = validateDiscountType(dtTrimmed) | |
| if (dtValid !== true) { consola.error(dtValid); return null } | |
| if (dtTrimmed === "percentage" || dtTrimmed === "fixed") { | |
| discountType = dtTrimmed | |
| } | |
| } | |
| } | |
| } else if (prompted) { | |
| const da = await input({ | |
| message: "discount amount (optional, e.g. 20 for 20% or $20 off)", | |
| validate: validateDiscountValue, | |
| }) | |
| if (da.trim()) { | |
| discountAmount = Number(da) | |
| // Ask for type if amount was entered | |
| const dt = await input({ | |
| message: "discount type (percentage or fixed, optional)", | |
| validate: validateDiscountType, | |
| }) | |
| if (dt.trim() && (dt.trim() === "percentage" || dt.trim() === "fixed")) { | |
| discountType = dt.trim() as "percentage" | "fixed" | |
| } | |
| } | |
| } |
🤖 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 253 - 279, The non-interactive
discount parsing in subscription.ts ignores flags.discountType whenever
flags.discountAmount is provided, leaving discountType null. Update the discount
parsing logic around the discountAmount/discountType handling to also read and
validate flags.discountType in the CLI-flag path, using validateDiscountType and
the existing discountType assignment. Keep the prompted flow intact, but ensure
both flags work together in the add command without relying on the interactive
prompt.
Comment on lines
+315
to
+338
| export async function handleList(options: ListFlags) { | ||
| // Determine status filter | ||
| let statusFilter: string | undefined | ||
| if (options.all) { | ||
| statusFilter = "all" | ||
| } else if (options.status) { | ||
| statusFilter = options.status | ||
| } else if (options.tags) { | ||
| // tagsSubscription doesn't filter by status — we'll filter the result | ||
| statusFilter = undefined | ||
| } else { | ||
| // Default: active + paused only | ||
| statusFilter = "active,paused" | ||
| } | ||
|
|
||
| let list = options.tags | ||
| ? tagsSubscription(options.tags.split(",").map((t) => t.trim())) | ||
| : getSubscriptions(options.sort, options.desc) | ||
| await spreadSubscription(list, options.currency as Currency | undefined, options.notes, options.method) | ||
| : getSubscriptions(options.sort, options.desc, statusFilter) | ||
|
|
||
| // Apply status filter for tagsSubscription path (tagsSubscription doesn't support status) | ||
| if (options.tags && statusFilter && statusFilter !== "all") { | ||
| const statuses = statusFilter.split(",").map((s) => s.trim().toLowerCase()) | ||
| list = list.filter((s) => statuses.includes(s.status)) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
list --tags bypasses the new default active+paused filter.
When --tags is used without --status/--all, statusFilter is set to undefined (line 324), and the post-fetch filter is skipped (line 335 only applies when statusFilter is truthy). This means subtrack list --tags x returns subscriptions of all statuses (including cancelled) by default, while plain subtrack list defaults to active+paused only — an inconsistent default between the two code paths for the same command.
🔧 Proposed fix
} else if (options.tags) {
- // tagsSubscription doesn't filter by status — we'll filter the result
- statusFilter = undefined
+ // tagsSubscription doesn't filter by status — we'll filter the result;
+ // keep the same active+paused default as the non-tag path
+ statusFilter = "active,paused"
} else {📝 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
| export async function handleList(options: ListFlags) { | |
| // Determine status filter | |
| let statusFilter: string | undefined | |
| if (options.all) { | |
| statusFilter = "all" | |
| } else if (options.status) { | |
| statusFilter = options.status | |
| } else if (options.tags) { | |
| // tagsSubscription doesn't filter by status — we'll filter the result | |
| statusFilter = undefined | |
| } else { | |
| // Default: active + paused only | |
| statusFilter = "active,paused" | |
| } | |
| let list = options.tags | |
| ? tagsSubscription(options.tags.split(",").map((t) => t.trim())) | |
| : getSubscriptions(options.sort, options.desc) | |
| await spreadSubscription(list, options.currency as Currency | undefined, options.notes, options.method) | |
| : getSubscriptions(options.sort, options.desc, statusFilter) | |
| // Apply status filter for tagsSubscription path (tagsSubscription doesn't support status) | |
| if (options.tags && statusFilter && statusFilter !== "all") { | |
| const statuses = statusFilter.split(",").map((s) => s.trim().toLowerCase()) | |
| list = list.filter((s) => statuses.includes(s.status)) | |
| } | |
| export async function handleList(options: ListFlags) { | |
| // Determine status filter | |
| let statusFilter: string | undefined | |
| if (options.all) { | |
| statusFilter = "all" | |
| } else if (options.status) { | |
| statusFilter = options.status | |
| } else if (options.tags) { | |
| // tagsSubscription doesn't filter by status — we'll filter the result; | |
| // keep the same active+paused default as the non-tag path | |
| statusFilter = "active,paused" | |
| } else { | |
| // Default: active + paused only | |
| statusFilter = "active,paused" | |
| } | |
| let list = options.tags | |
| ? tagsSubscription(options.tags.split(",").map((t) => t.trim())) | |
| : getSubscriptions(options.sort, options.desc, statusFilter) | |
| // Apply status filter for tagsSubscription path (tagsSubscription doesn't support status) | |
| if (options.tags && statusFilter && statusFilter !== "all") { | |
| const statuses = statusFilter.split(",").map((s) => s.trim().toLowerCase()) | |
| list = list.filter((s) => statuses.includes(s.status)) | |
| } |
🤖 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 315 - 338, The handleList list
path treats --tags differently from the normal subscription listing, so the
default active+paused filter is skipped when tags are used. Update handleList so
the tagsSubscription branch still applies the same default status filter as
getSubscriptions unless options.status or options.all is set, and keep the
post-fetch filtering in sync with statusFilter. Use the existing handleList,
tagsSubscription, and getSubscriptions flow to ensure subtrack list --tags
matches the default behavior of subtrack list.
Comment on lines
+564
to
+595
| if (flags.contractStart !== undefined) { | ||
| const trimmed = flags.contractStart.trim() | ||
| newData.contractStart = trimmed || null | ||
| } | ||
| if (flags.contractEnd !== undefined) { | ||
| const trimmed = flags.contractEnd.trim() | ||
| newData.contractEnd = trimmed || null | ||
| } | ||
| if (flags.autoRenewal !== undefined) { | ||
| const val = flags.autoRenewal.toString().toLowerCase() | ||
| newData.autoRenewal = val === "false" || val === "0" || val === "no" ? false : true | ||
| } | ||
| if (flags.vendorName !== undefined) { | ||
| const trimmed = flags.vendorName.trim() | ||
| newData.vendorName = trimmed || null | ||
| } | ||
| if (flags.vendorUrl !== undefined) { | ||
| const trimmed = flags.vendorUrl.trim() | ||
| newData.vendorUrl = trimmed || null | ||
| } | ||
| if (flags.planTier !== undefined) { | ||
| const trimmed = flags.planTier.trim() | ||
| newData.planTier = trimmed || null | ||
| } | ||
| if (flags.discountAmount !== undefined) { | ||
| const trimmed = flags.discountAmount.trim() | ||
| newData.discountAmount = trimmed ? Number(trimmed) : null | ||
| } | ||
| if (flags.discountType !== undefined) { | ||
| const trimmed = flags.discountType.trim() | ||
| newData.discountType = (trimmed as "percentage" | "fixed") || null | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Non-interactive edit flags skip validation for new fields — discountAmount can persist NaN.
Unlike price/billingDay earlier in this same function, the new field flags (contractStart, contractEnd, vendorName, vendorUrl, planTier, discountAmount, discountType) are written to newData without calling their corresponding validators (validateDateString, validateVendorUrl, validateDiscountValue, validateDiscountType) from prompts.ts. Most notably, flags.discountAmount.trim() ? Number(trimmed) : null will store NaN into an INTEGER column if a non-numeric string is passed (e.g. subtrack edit 1 --discount-amount abc), since Number("abc") is NaN and the truthy check on the trimmed string doesn't catch it.
Additionally, clearing discountAmount (empty flag) doesn't null out a stale discountType, unlike the interactive edit path (lines 759-768) which explicitly does this — leaving inconsistent discountAmount: null, discountType: "percentage" state reachable via flags.
🔧 Proposed fix
if (flags.contractStart !== undefined) {
const trimmed = flags.contractStart.trim()
+ if (trimmed) {
+ const valid = validateDateString(trimmed)
+ if (valid !== true) { consola.error(`Invalid contract start: ${valid}`); return }
+ }
newData.contractStart = trimmed || null
}
if (flags.contractEnd !== undefined) {
const trimmed = flags.contractEnd.trim()
+ if (trimmed) {
+ const valid = validateDateString(trimmed)
+ if (valid !== true) { consola.error(`Invalid contract end: ${valid}`); return }
+ }
newData.contractEnd = trimmed || null
}
...
if (flags.vendorUrl !== undefined) {
const trimmed = flags.vendorUrl.trim()
+ if (trimmed) {
+ const valid = validateVendorUrl(trimmed)
+ if (valid !== true) { consola.error(`Invalid vendor URL: ${valid}`); return }
+ }
newData.vendorUrl = trimmed || null
}
...
if (flags.discountAmount !== undefined) {
const trimmed = flags.discountAmount.trim()
+ if (trimmed) {
+ const valid = validateDiscountValue(trimmed)
+ if (valid !== true) { consola.error(`Invalid discount amount: ${valid}`); return }
+ }
newData.discountAmount = trimmed ? Number(trimmed) : null
+ if (!trimmed) newData.discountType = null
}
if (flags.discountType !== undefined) {
const trimmed = flags.discountType.trim()
+ if (trimmed) {
+ const valid = validateDiscountType(trimmed)
+ if (valid !== true) { consola.error(`Invalid discount type: ${valid}`); return }
+ }
newData.discountType = (trimmed as "percentage" | "fixed") || null
}📝 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
| if (flags.contractStart !== undefined) { | |
| const trimmed = flags.contractStart.trim() | |
| newData.contractStart = trimmed || null | |
| } | |
| if (flags.contractEnd !== undefined) { | |
| const trimmed = flags.contractEnd.trim() | |
| newData.contractEnd = trimmed || null | |
| } | |
| if (flags.autoRenewal !== undefined) { | |
| const val = flags.autoRenewal.toString().toLowerCase() | |
| newData.autoRenewal = val === "false" || val === "0" || val === "no" ? false : true | |
| } | |
| if (flags.vendorName !== undefined) { | |
| const trimmed = flags.vendorName.trim() | |
| newData.vendorName = trimmed || null | |
| } | |
| if (flags.vendorUrl !== undefined) { | |
| const trimmed = flags.vendorUrl.trim() | |
| newData.vendorUrl = trimmed || null | |
| } | |
| if (flags.planTier !== undefined) { | |
| const trimmed = flags.planTier.trim() | |
| newData.planTier = trimmed || null | |
| } | |
| if (flags.discountAmount !== undefined) { | |
| const trimmed = flags.discountAmount.trim() | |
| newData.discountAmount = trimmed ? Number(trimmed) : null | |
| } | |
| if (flags.discountType !== undefined) { | |
| const trimmed = flags.discountType.trim() | |
| newData.discountType = (trimmed as "percentage" | "fixed") || null | |
| } | |
| if (flags.contractStart !== undefined) { | |
| const trimmed = flags.contractStart.trim() | |
| if (trimmed) { | |
| const valid = validateDateString(trimmed) | |
| if (valid !== true) { consola.error(`Invalid contract start: ${valid}`); return } | |
| } | |
| newData.contractStart = trimmed || null | |
| } | |
| if (flags.contractEnd !== undefined) { | |
| const trimmed = flags.contractEnd.trim() | |
| if (trimmed) { | |
| const valid = validateDateString(trimmed) | |
| if (valid !== true) { consola.error(`Invalid contract end: ${valid}`); return } | |
| } | |
| newData.contractEnd = trimmed || null | |
| } | |
| if (flags.autoRenewal !== undefined) { | |
| const val = flags.autoRenewal.toString().toLowerCase() | |
| newData.autoRenewal = val === "false" || val === "0" || val === "no" ? false : true | |
| } | |
| if (flags.vendorName !== undefined) { | |
| const trimmed = flags.vendorName.trim() | |
| newData.vendorName = trimmed || null | |
| } | |
| if (flags.vendorUrl !== undefined) { | |
| const trimmed = flags.vendorUrl.trim() | |
| if (trimmed) { | |
| const valid = validateVendorUrl(trimmed) | |
| if (valid !== true) { consola.error(`Invalid vendor URL: ${valid}`); return } | |
| } | |
| newData.vendorUrl = trimmed || null | |
| } | |
| if (flags.planTier !== undefined) { | |
| const trimmed = flags.planTier.trim() | |
| newData.planTier = trimmed || null | |
| } | |
| if (flags.discountAmount !== undefined) { | |
| const trimmed = flags.discountAmount.trim() | |
| if (trimmed) { | |
| const valid = validateDiscountValue(trimmed) | |
| if (valid !== true) { consola.error(`Invalid discount amount: ${valid}`); return } | |
| } | |
| newData.discountAmount = trimmed ? Number(trimmed) : null | |
| if (!trimmed) newData.discountType = null | |
| } | |
| if (flags.discountType !== undefined) { | |
| const trimmed = flags.discountType.trim() | |
| if (trimmed) { | |
| const valid = validateDiscountType(trimmed) | |
| if (valid !== true) { consola.error(`Invalid discount type: ${valid}`); return } | |
| } | |
| newData.discountType = (trimmed as "percentage" | "fixed") || 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/subscription.ts` around lines 564 - 595, The non-interactive
edit path in subscription.ts writes the new flag fields directly into newData
without reusing the existing validators from prompts.ts. Update the flags
handling for contractStart, contractEnd, vendorName, vendorUrl, planTier,
discountAmount, and discountType to run through validateDateString,
validateVendorUrl, validateDiscountValue, and validateDiscountType before
assignment, matching the interactive edit flow. In particular, ensure
flags.discountAmount cannot become NaN and that invalid values are rejected or
normalized to null. Also mirror the interactive discount behavior by clearing
discountType whenever discountAmount is emptied so newData cannot end up with
discountAmount null but a stale discountType.
Comment on lines
+56
to
+61
| if (!newName) { | ||
| newName = await input({ | ||
| message: `New name for "${oldName}":`, | ||
| validate: (v: string) => v.trim().length > 0 ? true : "Name cannot be empty", | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim the interactive newName before use.
Validation checks v.trim().length > 0, but the raw untrimmed value from input() is stored via renameTag(oldName, newName), so padded input (e.g. " tag ") can slip through and create inconsistent tag names.
🩹 Proposed fix
if (!newName) {
newName = await input({
message: `New name for "${oldName}":`,
validate: (v: string) => v.trim().length > 0 ? true : "Name cannot be empty",
- })
+ })
+ newName = newName.trim()
}📝 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
| if (!newName) { | |
| newName = await input({ | |
| message: `New name for "${oldName}":`, | |
| validate: (v: string) => v.trim().length > 0 ? true : "Name cannot be empty", | |
| }) | |
| } | |
| if (!newName) { | |
| newName = await input({ | |
| message: `New name for "${oldName}":`, | |
| validate: (v: string) => v.trim().length > 0 ? true : "Name cannot be empty", | |
| }) | |
| newName = newName.trim() | |
| } |
🤖 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/tag.ts` around lines 56 - 61, The interactive rename flow in
tag.ts accepts a valid value in the input validation but then passes the raw
string to renameTag, so whitespace-padded names can be persisted. Update the
newName handling in the input/renameTag path to trim the value before it is
used, and ensure the trimmed value is what gets passed into renameTag(oldName,
newName) while keeping the existing non-empty validation behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enhance all existing subtrack CLI commands with missing features requested by the user.
Cross-cutting changes
list: Default shows active+paused only (not cancelled); added--all,--status,--min-price,--max-price,--limit,--jsonsearch: Added--status,--min-price,--max-price,--limitpost-filtersforecast: Added--jsonoutput,--max-rows(was hardcoded 8)compare: Added--jsonoutputanalytics: Added--json,--currencytags/tag list: Added--json,--sort;--filterrespects--alltrial list/trial expiring: Added--jsonupcoming: Made async; added--currencyfor FX price conversiontimeline: Made async; added--currencycalendar: Made async; added--currencyoptimize: Made async; added--currency,--discount-rate(configurable),--exclude(by ID/tag)profile save: Interactive prompt when no args givenprofile switch/delete: Interactive selection when name omittedexport: Now exports all subscriptions (including cancelled)Bug fixes
handleSearchparameter type fixed for strict mode complianceVerification
Summary by CodeRabbit