Split ExpenseTable.jsx into typed sub-components - #47
Merged
Conversation
Adds typescript + typescript-eslint, a project-references tsconfig setup (app + node configs, allowJs so untouched .jsx files keep working without conversion), and wires tsc -b into npm run build so type errors actually fail the build instead of being silently ignored. eslint.config.js now lints .ts/.tsx too. No source files converted yet - this is pure tooling setup, verified against the existing all-.jsx codebase (lint clean, build passes, and a throwaway type error was confirmed to fail the build before being removed). Adoption is incremental: components convert to .tsx as they're touched, starting with the ExpenseTable.jsx split.
The single 1056-line ExpenseTable.jsx handled the expense list, income list, category breakdown, recurring charges, the edit dialog, and the budget dialog all in one file - every review pass this session flagged its size as a real cost. Splits it into BudgetDialog, ExpenseEditDialog, ExpenseList, IncomeList, CategoryBreakdown, and RecurringSection, with ExpenseTable itself becoming a thin orchestrator. Converted to .tsx (TypeScript tooling added in the prior commit) since this is exactly the moment new component boundaries are being drawn - typing them now catches missed/wrong props at the call site instead of failing silently at runtime. Everything else in the app stays .jsx; adoption is incremental, not a repo-wide migration. This was treated as a pure refactor: behavior, DOM output, and styling are unchanged. Verified against the full existing e2e suite (68 specs) before and after with zero test file changes required. An independent three-angle review (behavior-preservation against the original file, TypeScript type-safety, cross-file prop wiring) caught three real regressions the e2e suite didn't cover, all now fixed: - Re-opening the same expense after Cancel could show the previous unsaved edit instead of current values, because the resync check used object-reference equality instead of open/closed transition. - showAllCategories (breakdown "view more") and showRecurring became local state in components that unmount on every Expenses/Income tab switch, silently collapsing a section the user had just expanded. Both moved back to the parent. Also fixed two type-safety gaps the same review found: Expense/Income's logged_by was typed as non-nullable despite the backend's LEFT JOIN being able to send null (existing runtime code already defensively guarded this - the type was the one that was wrong), and a save-diff loop used `as never` as a blanket escape hatch, replaced with an explicit unrolled diff that needs no cast at all. Added e2e coverage for the two regressions verifiable with existing seed data (the reopen-after-cancel case and the tab-switch persistence case for the breakdown section). The recurring-section fix uses the identical code pattern but has no dedicated regression test - the seeded data has no expense repeating >=3 times, which recurring detection requires, and restructuring seed data for that felt like disproportionate scope for a pure refactor. Verified: 150 backend tests, 72 e2e tests, lint, and build (now including tsc type-checking) all green.
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
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
Pure refactor, no feature or behavior change intended: splits the 1,056-line
ExpenseTable.jsx— which had grown to handle the expense list, income list, category breakdown, recurring charges, the edit dialog, and the budget dialog all in one file — into focused components, and adopts TypeScript for them.typescript+typescript-eslint, a project-referencestsconfigsetup (allowJs: trueso untouched.jsxfiles keep working without conversion),tsc -bwired intonpm run buildso type errors actually fail the build. No source files converted in this commit — verified against the existing all-.jsxcodebase first (lint clean, build passes, and a throwaway type error was confirmed to fail the build before being removed).BudgetDialog.tsx,ExpenseEditDialog.tsx,ExpenseList.tsx,IncomeList.tsx,CategoryBreakdown.tsx,RecurringSection.tsx, withExpenseTable.tsxbecoming a thin orchestrator. Converted to.tsxbecause this is exactly the moment new component boundaries are being drawn — typing them now catches a missed/wrong prop at the call site instead of failing silently at runtime. Everything else in the app (App.jsx,Chat.jsx,Login.jsx, the shadcncomponents/ui/*) stays.jsx— this is incremental adoption, not a repo-wide migration.Review
Ran an independent three-angle review (behavior-preservation against the original file via
git show, TypeScript type-safety, cross-file prop wiring) before committing. Cross-file wiring came back clean. The other two angles caught real issues, all fixed:Three behavior regressions (none caught by the 68-spec e2e baseline, since none of them exercise these specific interaction sequences):
showAllCategories(breakdown "view more") andshowRecurringhad become local state inside components that now unmount on every Expenses/Income tab switch, silently collapsing a section the user had just expanded. Moved both back to the parent.Two type-safety gaps:
Expense.logged_by/Income.logged_bywere typed as non-nullablestring, butget_expenses()/get_income()LEFT JOINagainst a nullableuser_idcolumn — existing runtime code already defensively guards againstnull(e.logged_by && ...), which was the tell that the type was the one lying, not the code.values[key] as neveras a blanket escape hatch (assignable to anything, at any key) to work around a known TS limitation with union-typed loop variables. Replaced with an explicit unrolled diff — five fields, needs no cast at all.Added e2e coverage for the two regressions verifiable with existing seed data (reopen-after-cancel, and tab-switch persistence for the breakdown section). The recurring-section fix uses the identical lift-to-parent pattern but has no dedicated regression test — the seeded data has no expense repeating ≥3 times, which recurring detection requires, and restructuring seed data for that felt like disproportionate scope for a pure refactor.
Test plan
uv run pytest tests/— 150 passed (backend untouched, sanity check only)npm run lint— cleannpm run build(nowtsc -b && vite build) — clean, zeroanyanywhere in the new filesnpm run test:e2e— 72 passed (68 original + 4 new, both viewports), zero changes to any pre-existing test file required by the split itself — the only test-file edits were the two new regression testsRoadmap / future work
Chat.jsx(778 lines) is the next-largest file and a reasonable candidate for the same treatment if it starts costing the same wayExpenseTable.jsxdid. No urgency noted yet.Generated by Claude Code