Conversation
…isual polish
## TUI Features
- Detail screen: Enter on list item opens full subscription card with editing shortcuts
- Sort cycling: s key cycles sort field (name -> price -> cycle -> status -> id)
- Billing day column in list view with scroll position indicator (3/15)
- Bar charts in Reports tab (Summary, Payment, etc.) using unicode blocks
- Redesigned subscription form with step progress, bordered inputs, and values panel
- Numbered sidebar shortcuts (1-6) for instant navigation
- Help screen (?) with complete keybinding reference
## Keybindings Added
- ? - Show help
- c - Open config screen
- S - Toggle subscription status (active/paused/cancelled)
- R - Refresh data
- Ctrl+l - Clear current filter
## Visual Polish
- Zebra striping on list rows for readability
- Selection glow via inverse rendering
- Selection markers in sidebar
- Consistent cyan/yellow/green colour scheme
- Truncated long filters in status bar with ellipsis
- Empty-state guidance (Press a to add)
## Bug Fixes
- SORT_FIELDS now includes 'status' (status sort was silently falling back to id)
- ImportTab: replaced writeSubscription with direct db.run() to avoid nested transaction crash
- RestoreTab: encrypted restore now calls restoreDb() to reload in-memory database
- tools.tsx: fixed any[] type in EXPORTERS -> SharedArgs[]
- config.tsx: removed spurious resetConfig() after setConfig()
- app.tsx: typed executeCommand from any to (AppState, React.Dispatch<AppAction>)
- reports.tsx: removed duplicate OCCURRENCES_PER_YEAR (imported from types.ts)
- detail.tsx: added explicit { isActive: true } to useInput
- sidebar.tsx: edit/delete/detail now highlight the 'List' item
- app.tsx: gg/G replaced 999999 with Number.MAX_SAFE_INTEGER
- tools.tsx: UsageTab responds to refreshKey
## Tests
- 2 new tests for getSubscriptions sort by status (ascending/descending)
- 311 total tests pass (up from 309)
- TypeScript clean, build succeeds
Contributor
|
Warning Review limit reached
More reviews will be available in 40 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR consolidates the TUI into tabbed Reports and Tools screens, adds a subscription detail screen, rewrites shared navigation/state handling, updates list sorting to support ChangesTUI refactor and status sort
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (2)
subtrack/src/tui/screens/reports.tsx (1)
487-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the exported screen component.
ReportsScreenis a public API from this module. As per coding guidelines,subtrack/**/*.{js,jsx,ts,tsx}should document public APIs with JSDoc comments in JavaScript/TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subtrack/src/tui/screens/reports.tsx` at line 487, The exported ReportsScreen component is a public API and currently lacks the required JSDoc documentation. Add a JSDoc comment immediately above ReportsScreen in the reports screen module, following the style used for other exported UI components, so the component’s purpose is documented for consumers of the module.Source: Coding guidelines
subtrack/src/tui/screens/tools.tsx (1)
391-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the exported screen component.
ToolsScreenis a public API from this module. As per coding guidelines,subtrack/**/*.{js,jsx,ts,tsx}should document public APIs with JSDoc comments in JavaScript/TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subtrack/src/tui/screens/tools.tsx` at line 391, The exported public component ToolsScreen is missing required JSDoc documentation. Add a JSDoc comment directly above the ToolsScreen function in the tools.tsx module, describing it as the TUI screen component and noting its public API role so it complies with the subtrack documentation guidelines.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/tui.tsx`:
- Around line 10-18: Wrap the entire TUI lifecycle in the try/catch so any
failure from render(<App />) or instance.waitUntilExit() is handled
consistently, and make sure consola.level is restored in a finally block even
when render throws. In the catch for the App/render flow, do not silently ignore
the error; rethrow or propagate real TUI failures so they are not converted into
successful exits. Use the render and waitUntilExit logic in tui.tsx as the
anchor for the fix.
In `@subtrack/src/tui/app.tsx`:
- Around line 272-283: The shortcut handling in app.tsx for the "S" key calls
updateSubscription() without any error handling, so a transient persistence
failure can escape and crash the TUI. Wrap the updateSubscription(sub.id, {
status: newStatus }) call in the input-handler path with a try/catch, keep the
state rollback behavior from updateSubscription(), and surface the failure
through the TUI’s existing error/reporting mechanism instead of letting it
propagate. Use the selectedId/subscription lookup block and the status cycle
logic in the shortcut handler to locate the fix.
- Around line 357-358: The unknown-command fallback in app.tsx leaves the
command buffer stale after returning to NORMAL mode. Update the unknown-command
handling near the SET_MODE dispatch so that it also clears the command/filter
state used by the status bar, using the relevant app state update logic around
the command handling path in the app component. Ensure the existing mode reset
remains, but the text from :bogus does not persist as if it were an active
filter.
- Around line 141-169: The keyboard handling in app.tsx is matching list/config
movement too broadly, so it intercepts j/k and arrow keys even when the sidebar
has focus. Update the navigation logic in the input handler around the
state.screen check so that the list/config movement block only runs when content
is focused, and let the sidebar navigation block handle those keys otherwise.
Keep the existing behavior for SET_LIST_INDEX, but gate it by the active focus
state used elsewhere in the TUI handler.
In `@subtrack/src/tui/context/app-context.tsx`:
- Around line 77-89: The SET_SCREEN reducer in app-context.tsx always appends
the current screen to history, even when navigating to the same screen, which
creates duplicate entries. Update the SET_SCREEN branch in the app-context
reducer to skip pushing state.screen into history when action.screen already
matches the current screen, so same-screen navigation doesn’t consume a back
step. Keep the change localized to the SET_SCREEN case and preserve the existing
reset behavior for mode, focus, filterText, and formActive.
- Around line 140-147: The SET_SORT branch in app-context.tsx has a dead toggle
path because SORT_CYCLE always advances to a different field, so the next ===
state.sortField check in the reducer never fires. Update the sort handling logic
in the reducer case to make descending order reachable, either by explicitly
toggling sortDesc when the same field is selected again or by using a separate
action/state transition in the AppContext reducer instead of relying on the
unreachable branch.
In `@subtrack/src/tui/screens/config.tsx`:
- Around line 40-49: The config screen’s save feedback in `save()` and the
result display is always styled like success, even when `setConfig()` rejects
the value. Update the state used by the result box in `ConfigScreen` so failed
saves set an error/failed status as well as the message, and make the render
logic choose the appropriate color/border styling based on that status instead
of always using success styles.
In `@subtrack/src/tui/screens/delete.tsx`:
- Around line 33-37: In delete.tsx, the cancel path and the post-delete path are
clearing selectedId too early, which breaks returning from detail and
confirmation flow. Update the handler in the delete screen so the cancel branch
only dispatches GO_BACK, and move the SET_SELECTED_ID null reset to the
successful delete flow after navigation has returned to the list. Use the
existing input handling logic in the delete screen to keep the detail screen
selection intact on cancel and avoid routing back through detail after confirm.
In `@subtrack/src/tui/screens/edit.tsx`:
- Around line 28-37: The edit screen is clearing the current selection after
navigation, which breaks the history-based return flow. In `handleSubmit` and
`handleCancel` in the edit screen, keep `selectedId` intact when calling
`dispatch({ type: "GO_BACK" })` so returning from edit preserves the previous
record. Only reset `selectedId` in the explicit route-to-list flow, not when
going back from edit.
In `@subtrack/src/tui/screens/help.tsx`:
- Around line 53-57: The Add/Edit form help in the Help screen is incorrectly
listing the y/n shortcut, which belongs to delete confirmation rather than the
form flow. Update the help content in the help.tsx screen so the Form Screens
(Add/Edit) section only lists shortcuts handled by the form screens, and move
y/n to the delete confirmation/help section that documents the confirmation
handlers.
In `@subtrack/src/tui/screens/list.tsx`:
- Around line 83-90: The row layout in list.tsx is not reserving space for the
leading selection marker column, so the data columns are calculated too wide and
can misalign or overflow on narrow terminals. Update the width budgeting around
the availableWidth/calcWidths path and the table rendering in the list screen so
the marker column is included in the total width calculation, keeping headers,
separators, and row cells aligned.
- Around line 85-88: The list height calculation in the TUI screen still
subtracts `filterBarHeight` even though the filter text is now rendered inline
in the title row, causing `availableHeight` and `maxVisible` to shrink
unnecessarily. Update the sizing logic in `list.tsx` around `headerHeight`,
`footerHeight`, and `availableHeight` so filtering no longer deducts extra rows,
and make sure the scroll/window calculations in this screen use the corrected
visible height.
In `@subtrack/src/tui/screens/reports.tsx`:
- Around line 243-252: The upcoming bills calculation in useMemo for reports.tsx
is using computeNextBill() with only billingDay, so quarterly/annual
subscriptions can appear monthly. Fix this by either filtering the upcoming list
to monthly subscriptions only in the upcoming selector, or by extending the data
passed into computeNextBill()/the subscription mapping to include billing
cycle-aware due-date logic before sorting and displaying results.
- Around line 304-327: The analytics view in reports.tsx is mixing currencies
when computing and displaying the “Most Expensive” list. Update the
sortedByPrice logic in the reports screen to avoid comparing raw minor-unit
prices across different currencies, and make BarChart format items using each
subscription’s own currency or separate groups by currency. Use the existing
activeSubs, sortedByPrice, and BarChart symbols to keep the fix localized.
- Around line 107-113: The monthly totals in the reports calculations are
rounding each subscription before aggregation, which can skew currency totals
for non-monthly cycles. Update the logic in the monthly aggregation paths (the
useMemo blocks in reports.tsx, including the repeated loops referenced in the
review) to accumulate fractional minor units per currency first, then apply
Math.round only once per currency total, using the existing
monthlyByCurrency-style maps and subscription cycle helpers to keep prices
represented as integer minor units throughout.
- Around line 100-101: The Reports screen is holding stale subscription data
because SummaryTab memoizes getSubscriptions() with an empty dependency list.
Update SummaryTab (and any similar report tabs) to depend on refreshKey in the
useMemo/useEffect logic, or move the getSubscriptions() call into ReportsScreen
and pass the loaded subscriptions down so the report queries re-run when
refreshKey changes.
In `@subtrack/src/tui/screens/subscription-form.tsx`:
- Around line 154-158: The price preview in ValuesPanel is rendering the stored
smallest-unit integer directly, so users see an inflated amount instead of a
human-readable price. Update the preview formatting for the price field in
ValuesPanel (and the related confirm/sidebar preview at the referenced section)
to convert the stored integer into the proper display amount for the currency,
using the existing FormData fields such as data.price and data.currency. Keep
storage in smallest currency units, but format the displayed value so USD shows
cents as dollars and other currencies follow their expected display rules.
In `@subtrack/src/tui/screens/tools.tsx`:
- Around line 253-255: The result color check in the tools screen is too broad
because the step rendering logic treats any string starting with "Backup" as
success. Update the conditional in the step === "done" block in the tools screen
so it distinguishes successful backup messages from failure messages like
"Backup failed: ...", and only renders green for actual success while failures
render red.
- Around line 272-276: The restore picker in BackupTab is reading backup files
from the database directory instead of the backup directory, so newly created
backups are missed. Update the backup lookup in the RestoreTab/BackupTab flow to
use getDefaultBackupDir() for getBackupFiles(), then keep mapping the returned
files into the options list as before using the same option-building logic in
tools.tsx.
- Around line 136-145: The CSV import path in the tools screen is allowing blank
subscription names because tools.tsx inserts fields[0].trim() without validating
it first. Update the import loop in the CSV parsing logic to reject rows where
the trimmed name is empty before the INSERT on subscriptions, and record the row
as failed/error just like the existing invalid price and currency checks.
---
Nitpick comments:
In `@subtrack/src/tui/screens/reports.tsx`:
- Line 487: The exported ReportsScreen component is a public API and currently
lacks the required JSDoc documentation. Add a JSDoc comment immediately above
ReportsScreen in the reports screen module, following the style used for other
exported UI components, so the component’s purpose is documented for consumers
of the module.
In `@subtrack/src/tui/screens/tools.tsx`:
- Line 391: The exported public component ToolsScreen is missing required JSDoc
documentation. Add a JSDoc comment directly above the ToolsScreen function in
the tools.tsx module, describing it as the TUI screen component and noting its
public API role so it complies with the subtrack documentation guidelines.
🪄 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: 92c21415-60f2-454d-8ef7-e9797a7a16bb
📒 Files selected for processing (38)
subtrack/src/__tests__/db.test.tssubtrack/src/db.tssubtrack/src/tui.tsxsubtrack/src/tui/app.tsxsubtrack/src/tui/components/command-bar.tsxsubtrack/src/tui/components/sidebar.tsxsubtrack/src/tui/components/status-bar.tsxsubtrack/src/tui/context/app-context.tsxsubtrack/src/tui/hooks/use-mouse.tssubtrack/src/tui/screens/add.tsxsubtrack/src/tui/screens/analytics.tsxsubtrack/src/tui/screens/backup.tsxsubtrack/src/tui/screens/bulk.tsxsubtrack/src/tui/screens/compare.tsxsubtrack/src/tui/screens/config.tsxsubtrack/src/tui/screens/delete.tsxsubtrack/src/tui/screens/detail.tsxsubtrack/src/tui/screens/edit.tsxsubtrack/src/tui/screens/export.tsxsubtrack/src/tui/screens/forecast.tsxsubtrack/src/tui/screens/help.tsxsubtrack/src/tui/screens/import.tsxsubtrack/src/tui/screens/list.tsxsubtrack/src/tui/screens/payment.tsxsubtrack/src/tui/screens/reports.tsxsubtrack/src/tui/screens/restore.tsxsubtrack/src/tui/screens/search.tsxsubtrack/src/tui/screens/subscription-form.tsxsubtrack/src/tui/screens/summary.tsxsubtrack/src/tui/screens/tag-manage.tsxsubtrack/src/tui/screens/tags.tsxsubtrack/src/tui/screens/tools.tsxsubtrack/src/tui/screens/trial-add.tsxsubtrack/src/tui/screens/trial-expiring.tsxsubtrack/src/tui/screens/trials.tsxsubtrack/src/tui/screens/upcoming.tsxsubtrack/src/tui/screens/usage.tsxsubtrack/src/tui/types.ts
💤 Files with no reviewable changes (19)
- subtrack/src/tui/screens/tags.tsx
- subtrack/src/tui/hooks/use-mouse.ts
- subtrack/src/tui/screens/tag-manage.tsx
- subtrack/src/tui/screens/summary.tsx
- subtrack/src/tui/screens/trial-expiring.tsx
- subtrack/src/tui/screens/restore.tsx
- subtrack/src/tui/screens/analytics.tsx
- subtrack/src/tui/screens/payment.tsx
- subtrack/src/tui/screens/trial-add.tsx
- subtrack/src/tui/screens/usage.tsx
- subtrack/src/tui/screens/forecast.tsx
- subtrack/src/tui/screens/search.tsx
- subtrack/src/tui/screens/import.tsx
- subtrack/src/tui/screens/bulk.tsx
- subtrack/src/tui/screens/compare.tsx
- subtrack/src/tui/screens/trials.tsx
- subtrack/src/tui/screens/export.tsx
- subtrack/src/tui/screens/backup.tsx
- subtrack/src/tui/screens/upcoming.tsx
Comment on lines
10
to
18
| const instance = render(<App />, { | ||
| exitOnCtrlC: true, | ||
| patchConsole: true, | ||
| }) | ||
|
|
||
| try { | ||
| await waitUntilExit() | ||
| } catch (error) { | ||
| await instance.waitUntilExit() | ||
| } catch { | ||
| // App exited with an error — silently ignore for clean exit |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore logging even if render fails, and don’t swallow TUI errors.
render(<App />) happens before the try, so a render-time failure leaves consola.level muted. The broad catch also converts real TUI failures into successful exits.
Proposed fix
- const instance = render(<App />, {
- exitOnCtrlC: true,
- patchConsole: true,
- })
-
+ let instance: ReturnType<typeof render> | undefined
try {
+ instance = render(<App />, {
+ exitOnCtrlC: true,
+ patchConsole: true,
+ })
await instance.waitUntilExit()
- } catch {
- // App exited with an error — silently ignore for clean exit
} finally {
// Clear Ink's output and restore terminal
- instance.clear()
- consola.level = prevLevel
+ try {
+ instance?.clear()
+ } finally {
+ consola.level = prevLevel
+ }
}📝 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
| const instance = render(<App />, { | |
| exitOnCtrlC: true, | |
| patchConsole: true, | |
| }) | |
| try { | |
| await waitUntilExit() | |
| } catch (error) { | |
| await instance.waitUntilExit() | |
| } catch { | |
| // App exited with an error — silently ignore for clean exit | |
| let instance: ReturnType<typeof render> | undefined | |
| try { | |
| instance = render(<App />, { | |
| exitOnCtrlC: true, | |
| patchConsole: true, | |
| }) | |
| await instance.waitUntilExit() | |
| } finally { | |
| // Clear Ink's output and restore terminal | |
| try { | |
| instance?.clear() | |
| } finally { | |
| consola.level = prevLevel | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui.tsx` around lines 10 - 18, Wrap the entire TUI lifecycle in
the try/catch so any failure from render(<App />) or instance.waitUntilExit() is
handled consistently, and make sure consola.level is restored in a finally block
even when render throws. In the catch for the App/render flow, do not silently
ignore the error; rethrow or propagate real TUI failures so they are not
converted into successful exits. Use the render and waitUntilExit logic in
tui.tsx as the anchor for the fix.
Comment on lines
+304
to
+327
| const sortedByPrice = useMemo( | ||
| () => [...activeSubs].sort((a, b) => b.price - a.price).slice(0, 5), | ||
| [activeSubs], | ||
| ) | ||
|
|
||
| return ( | ||
| <Box flexDirection="column"> | ||
| <Box marginBottom={1}><Text bold underline>Analytics</Text></Box> | ||
| {activeSubs.length === 0 ? ( | ||
| <Text dimColor>No active subscriptions</Text> | ||
| ) : ( | ||
| <> | ||
| <Box flexDirection="column" marginBottom={1}> | ||
| <Text bold color="cyan">Most Expensive (top 5)</Text> | ||
| <Box marginTop={1}> | ||
| <BarChart | ||
| items={sortedByPrice.map((sub) => ({ | ||
| label: sub.name, | ||
| value: sub.price, | ||
| color: "yellow", | ||
| }))} | ||
| maxWidth={16} | ||
| currency={sortedByPrice[0]?.currency ?? "USD"} | ||
| /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t rank and format mixed currencies together.
sortedByPrice compares raw minor-unit values across currencies, and BarChart formats every item with sortedByPrice[0]?.currency. Group by currency or convert through FX rates before ranking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/screens/reports.tsx` around lines 304 - 327, The analytics
view in reports.tsx is mixing currencies when computing and displaying the “Most
Expensive” list. Update the sortedByPrice logic in the reports screen to avoid
comparing raw minor-unit prices across different currencies, and make BarChart
format items using each subscription’s own currency or separate groups by
currency. Use the existing activeSubs, sortedByPrice, and BarChart symbols to
keep the fix localized.
Comment on lines
+154
to
+158
| function ValuesPanel({ data }: { data: FormData }) { | ||
| const fields: [string, string][] = [ | ||
| ["Name", data.name || "—"], | ||
| ["Price", data.price ? `${data.price} ${data.currency}` : "—"], | ||
| ["Cycle", data.cycle], |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the price preview human-readable.
These new previews print the stored integer directly as 1490 USD. With prices stored in the smallest currency unit, that reads like $1,490 instead of $14.90, so the sidebar/confirm step can mislead users into saving the wrong amount.
As per coding guidelines, "Represent prices as integers in the smallest currency unit (for example, JPY without decimals and USD in cents)".
Also applies to: 450-462
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/screens/subscription-form.tsx` around lines 154 - 158, The
price preview in ValuesPanel is rendering the stored smallest-unit integer
directly, so users see an inflated amount instead of a human-readable price.
Update the preview formatting for the price field in ValuesPanel (and the
related confirm/sidebar preview at the referenced section) to convert the stored
integer into the proper display amount for the currency, using the existing
FormData fields such as data.price and data.currency. Keep storage in smallest
currency units, but format the displayed value so USD shows cents as dollars and
other currencies follow their expected display rules.
Source: Coding guidelines
Comment on lines
+136
to
+145
| for (let i = 1; i < lines.length; i++) { | ||
| const fields = parseCsvLine(lines[i]) | ||
| if (fields.length < 5) { failed++; continue } | ||
| if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue } | ||
| const price = Number(fields[3]) | ||
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | ||
| // Direct insert to avoid nested transaction from writeSubscription | ||
| db.run( | ||
| "INSERT INTO subscriptions (name, price, currency, cycle, status, created_at) VALUES (?, ?, ?, ?, 'active', date('now'))", | ||
| [fields[0].trim(), price, fields[4], fields[1]], |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject empty subscription names during CSV import.
fields[0].trim() can be an empty string and still gets inserted, creating unusable blank-name subscriptions.
🛡️ Proposed fix
const fields = parseCsvLine(lines[i])
if (fields.length < 5) { failed++; continue }
+ const name = fields[0].trim()
+ if (!name) { failed++; errors.push(`Line ${i + 1}: missing name`); continue }
if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue }
const price = Number(fields[3])
if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue }
@@
- [fields[0].trim(), price, fields[4], fields[1]],
+ [name, price, fields[4], fields[1]],📝 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
| for (let i = 1; i < lines.length; i++) { | |
| const fields = parseCsvLine(lines[i]) | |
| if (fields.length < 5) { failed++; continue } | |
| if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue } | |
| const price = Number(fields[3]) | |
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | |
| // Direct insert to avoid nested transaction from writeSubscription | |
| db.run( | |
| "INSERT INTO subscriptions (name, price, currency, cycle, status, created_at) VALUES (?, ?, ?, ?, 'active', date('now'))", | |
| [fields[0].trim(), price, fields[4], fields[1]], | |
| for (let i = 1; i < lines.length; i++) { | |
| const fields = parseCsvLine(lines[i]) | |
| if (fields.length < 5) { failed++; continue } | |
| const name = fields[0].trim() | |
| if (!name) { failed++; errors.push(`Line ${i + 1}: missing name`); continue } | |
| if (!isValidCurrency(fields[4]) || !isValidCycle(fields[1])) { failed++; continue } | |
| const price = Number(fields[3]) | |
| if (isNaN(price) || price < 0 || !Number.isInteger(price)) { failed++; errors.push(`Line ${i + 1}: invalid price "${fields[3]}"`); continue } | |
| // Direct insert to avoid nested transaction from writeSubscription | |
| db.run( | |
| "INSERT INTO subscriptions (name, price, currency, cycle, status, created_at) VALUES (?, ?, ?, ?, 'active', date('now'))", | |
| [name, price, fields[4], fields[1]], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/screens/tools.tsx` around lines 136 - 145, The CSV import
path in the tools screen is allowing blank subscription names because tools.tsx
inserts fields[0].trim() without validating it first. Update the import loop in
the CSV parsing logic to reject rows where the trimmed name is empty before the
INSERT on subscriptions, and record the row as failed/error just like the
existing invalid price and currency checks.
Comment on lines
+253
to
+255
| {step === "done" && ( | ||
| <Text color={result?.startsWith("Backup") ? "green" : "red"}>{result}</Text> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Color backup failures as errors.
"Backup failed: ..." also starts with "Backup", so failures render green.
🐛 Proposed fix
- <Text color={result?.startsWith("Backup") ? "green" : "red"}>{result}</Text>
+ <Text color={result?.startsWith("Backup saved") ? "green" : "red"}>{result}</Text>📝 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
| {step === "done" && ( | |
| <Text color={result?.startsWith("Backup") ? "green" : "red"}>{result}</Text> | |
| )} | |
| {step === "done" && ( | |
| <Text color={result?.startsWith("Backup saved") ? "green" : "red"}>{result}</Text> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/screens/tools.tsx` around lines 253 - 255, The result color
check in the tools screen is too broad because the step rendering logic treats
any string starting with "Backup" as success. Update the conditional in the step
=== "done" block in the tools screen so it distinguishes successful backup
messages from failure messages like "Backup failed: ...", and only renders green
for actual success while failures render red.
Comment on lines
+272
to
+276
| const backups = getBackupFiles(getDbDir()) | ||
| const options = backups.map((b) => ({ | ||
| label: `${b.name} (${b.mtime.toISOString().slice(0, 10)})`, | ||
| value: b.path, | ||
| })) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
List backups from the backup directory, not the DB directory.
BackupTab saves to getDefaultBackupDir() by default, but RestoreTab searches getDbDir(), so freshly-created backups won’t appear in the restore picker.
🐛 Proposed fix
- const backups = getBackupFiles(getDbDir())
+ const backups = getBackupFiles(getDefaultBackupDir())📝 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
| const backups = getBackupFiles(getDbDir()) | |
| const options = backups.map((b) => ({ | |
| label: `${b.name} (${b.mtime.toISOString().slice(0, 10)})`, | |
| value: b.path, | |
| })) | |
| const backups = getBackupFiles(getDefaultBackupDir()) | |
| const options = backups.map((b) => ({ | |
| label: `${b.name} (${b.mtime.toISOString().slice(0, 10)})`, | |
| value: b.path, | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/screens/tools.tsx` around lines 272 - 276, The restore
picker in BackupTab is reading backup files from the database directory instead
of the backup directory, so newly created backups are missed. Update the backup
lookup in the RestoreTab/BackupTab flow to use getDefaultBackupDir() for
getBackupFiles(), then keep mapping the returned files into the options list as
before using the same option-building logic in tools.tsx.
Owner
Author
Contributor
|
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
subtrack/src/tui/components/command-palette.tsx (1)
67-68: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the unused subscription query from palette construction.
getSubscriptions()runs on everyentriesrecompute, butsubsis never read. That makes palette open/search/navigation pay for a DB read that has no effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subtrack/src/tui/components/command-palette.tsx` around lines 67 - 68, The command palette recompute path is doing an unnecessary database read because the local `subs` value from `getSubscriptions()` is never used. Remove that query from the palette construction flow in `command-palette.tsx` (around the logic that builds the `entries` and checks `hasSelection`), and keep only the subscription-independent selection logic so opening/searching/navigation no longer pays for the unused read.
🤖 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/tui/components/command-palette.tsx`:
- Around line 182-203: The command palette key handling in command-palette.tsx
is treating the printable keys j and k as navigation before they can be appended
to the query, so type-to-search fails for those characters. Update the input
handling in the palette key handler to distinguish actual navigation intent from
plain text input, and ensure the SET_PALETTE_QUERY path in the command-palette
component still receives single-character j/k when the user is typing a search.
In `@subtrack/src/tui/components/toast.tsx`:
- Around line 21-27: The toast-clearing logic in toast.tsx is too aggressive
because it only preserves toasts on the list screen, which causes save success
messages to be cleared when EditScreen returns to detail via GO_BACK. Update the
useEffect around state.toast/state.screen so it only clears toasts on true
screen transitions that should dismiss them, and make sure the success toast set
after GO_BACK survives when the restored screen is detail. Use the existing
toast state handling and the dispatch of CLEAR_TOAST as the place to adjust the
condition.
---
Nitpick comments:
In `@subtrack/src/tui/components/command-palette.tsx`:
- Around line 67-68: The command palette recompute path is doing an unnecessary
database read because the local `subs` value from `getSubscriptions()` is never
used. Remove that query from the palette construction flow in
`command-palette.tsx` (around the logic that builds the `entries` and checks
`hasSelection`), and keep only the subscription-independent selection logic so
opening/searching/navigation no longer pays for the unused read.
🪄 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: 324d4f06-7f76-4eb0-b5c3-6be131d0cf59
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
subtrack/package.jsonsubtrack/src/tui/app.tsxsubtrack/src/tui/components/command-bar.tsxsubtrack/src/tui/components/command-palette.tsxsubtrack/src/tui/components/status-bar.tsxsubtrack/src/tui/components/toast.tsxsubtrack/src/tui/context/app-context.tsxsubtrack/src/tui/screens/add.tsxsubtrack/src/tui/screens/config.tsxsubtrack/src/tui/screens/delete.tsxsubtrack/src/tui/screens/detail.tsxsubtrack/src/tui/screens/edit.tsxsubtrack/src/tui/screens/help.tsxsubtrack/src/tui/screens/list.tsxsubtrack/src/tui/screens/reports.tsxsubtrack/src/tui/screens/tools.tsxtypos.toml
✅ Files skipped from review due to trivial changes (1)
- typos.toml
🚧 Files skipped from review as they are similar to previous changes (12)
- subtrack/src/tui/screens/add.tsx
- subtrack/src/tui/components/status-bar.tsx
- subtrack/src/tui/screens/delete.tsx
- subtrack/src/tui/screens/config.tsx
- subtrack/src/tui/screens/detail.tsx
- subtrack/src/tui/components/command-bar.tsx
- subtrack/src/tui/context/app-context.tsx
- subtrack/src/tui/app.tsx
- subtrack/src/tui/screens/reports.tsx
- subtrack/src/tui/screens/help.tsx
- subtrack/src/tui/screens/tools.tsx
- subtrack/src/tui/screens/list.tsx
Comment on lines
+182
to
+203
| if (key.upArrow || input === "k") { | ||
| dispatch({ | ||
| type: "SET_PALETTE_INDEX", | ||
| index: Math.max(0, clampedIndex - 1), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (key.downArrow || input === "j") { | ||
| dispatch({ | ||
| type: "SET_PALETTE_INDEX", | ||
| index: Math.min(filtered.length - 1, clampedIndex + 1), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (input.length === 1 && !key.ctrl && !key.meta) { | ||
| dispatch({ | ||
| type: "SET_PALETTE_QUERY", | ||
| query: state.paletteQuery + input, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
j and k can never be typed into the palette query.
These branches run before the printable-character handler, so typing either character always moves selection instead of updating state.paletteQuery. That breaks type-to-search for any query containing j or k.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/components/command-palette.tsx` around lines 182 - 203, The
command palette key handling in command-palette.tsx is treating the printable
keys j and k as navigation before they can be appended to the query, so
type-to-search fails for those characters. Update the input handling in the
palette key handler to distinguish actual navigation intent from plain text
input, and ensure the SET_PALETTE_QUERY path in the command-palette component
still receives single-character j/k when the user is typing a search.
Source: Coding guidelines
Comment on lines
+21
to
+27
| useEffect(() => { | ||
| if (state.toast && state.screen !== "list") { | ||
| // Clear on screen change if not list | ||
| dispatch({ type: "CLEAR_TOAST" }) | ||
| return | ||
| } | ||
| }, [state.screen, state.toast, dispatch]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This drops save toasts when edit returns to detail.
EditScreen now does GO_BACK and then SET_TOAST, and GO_BACK restores the previous screen from history. If the user opened edit from the new detail screen, this effect clears the success toast immediately because the screen is "detail" instead of "list".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subtrack/src/tui/components/toast.tsx` around lines 21 - 27, The
toast-clearing logic in toast.tsx is too aggressive because it only preserves
toasts on the list screen, which causes save success messages to be cleared when
EditScreen returns to detail via GO_BACK. Update the useEffect around
state.toast/state.screen so it only clears toasts on true screen transitions
that should dismiss them, and make sure the success toast set after GO_BACK
survives when the restored screen is detail. Use the existing toast state
handling and the dispatch of CLEAR_TOAST as the place to adjust the condition.
This was referenced Jun 28, 2026
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.
feat: enhance TUI with detail screen, sort cycling, bar charts, and visual polish
TUI Features
Keybindings Added
Visual Polish
Bug Fixes
Tests
Summary by CodeRabbit