feat: Command Console UI overhaul#1
Closed
chitcommit wants to merge 14 commits into
Closed
Conversation
CLAUDE.md: - Fix architecture: single worker, not 4 aspirational ones - Clarify Wave Accounting comes via ChittyFinance, not direct - Add auth middleware flow documentation - Add cron schedule with CT times - Document Hyperdrive binding for DB - Fix CORS origins to match actual config - Add key files: auth.ts, cron.ts, integrations.ts, bridge.ts, mcp.ts - Add secret management commands - Remove stale Active Disputes section (operational, not dev guidance) CHARTER.md: - Update compliance checklist: service now registered Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Install react-grid-layout and recharts for the dashboard layout and charting. Add Outfit (display) and JetBrains Mono (monospace) Google Fonts. Replace index.css with design token CSS variables for the new dark-chrome/light-card visual system, urgency colors, and brand palette. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…omponents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add FocusView (top 3 urgent items), FullView (metric cards + widget grid), ObligationsWidget, DisputesWidget, DeadlinesWidget, RecommendationsWidget. Dashboard now toggles between views based on Focus Mode context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bills: card list with urgency borders, loading states. Disputes: ProgressDots for lifecycle, priority badges, expandable panels. Accounts: grouped by type with credit utilization bars. CashFlow: Recharts area chart, scenario panel, MetricCard summaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate all remaining pages to Command Console design system: dark chrome shell, light card surfaces, urgency borders, semantic tokens. Replace raw HTML with Card/ActionButton/MetricCard components. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
StatusBar now self-fetches cash position, overdue count, due-this-week, and per-source sync freshness indicators from the API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a large UI redesign (sidebar layout, card-based components, Focus Mode), new design docs and implementation plans, theme and component libraries, several dashboard/dashboard-widget pages, validator schemas, schema-driven request validation across routes, and assorted build/test dependency updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/pages/CashFlow.tsx (1)
19-31:⚠️ Potential issue | 🟡 MinorHarden initial load error handling for non-Error rejections.
setError(e.message)can throw if the rejection isnull/undefined. The rest of the file already usesunknownguards—align this path for consistency and safety.✅ Suggested fix
- .catch((e) => setError(e.message)) + .catch((e: unknown) => setError(e instanceof Error ? e.message : 'Failed to load cash flow data'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 19 - 31, The catch in load() uses setError(e.message) which throws for non-Error rejections; change the handler to normalize unknown rejections before calling setError (e.g., extract message via e instanceof Error ? e.message : String(e) or use an existing getErrorMessage helper) so Promise.all(...).catch(...) always passes a safe string or Error-compatible value to setError; update the catch block in load() to compute a safe message and call setError(safeMessage).ui/src/pages/Recommendations.tsx (1)
46-54:⚠️ Potential issue | 🟡 MinorUse functional updates to avoid stale state when removing recs.
The async handlers can race and reintroduce items if multiple actions resolve out of order.🐛 Proposed fix
- setRecs(recs.filter(r => r.id !== id)); + setRecs(prev => prev.filter(r => r.id !== id)); ... - setRecs(recs.filter(r => r.id !== id)); + setRecs(prev => prev.filter(r => r.id !== id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Recommendations.tsx` around lines 46 - 54, The handlers act and dismiss update recs using the current closure value, which can be stale when multiple async ops complete out of order; change both setRecs calls in act and dismiss to use functional updates (e.g., setRecs(prev => prev.filter(r => r.id !== id))) so removal is applied against the latest state after the awaited api calls; keep awaiting api.actOnRecommendation and api.dismissRecommendation as-is and only replace the direct setRecs(recs.filter(...)) calls in the act and dismiss functions.
🧹 Nitpick comments (3)
ui/src/components/ui/MetricCard.tsx (1)
15-18: Add screen-reader text for trend indicators.The ▲/▼ glyphs are visual-only; add an SR label and mark the glyphs
aria-hiddenfor accessibility.Proposed fix
<div className="flex items-baseline gap-1 mt-1"> <p className={cn('text-2xl font-bold font-mono', valueClassName || 'text-card-text')}>{value}</p> - {trend === 'up' && <span className="text-urgency-green text-sm">▲</span>} - {trend === 'down' && <span className="text-urgency-red text-sm">▼</span>} + {trend && ( + <span className="sr-only"> + {trend === 'up' ? 'Trending up' : 'Trending down'} + </span> + )} + {trend === 'up' && <span aria-hidden="true" className="text-urgency-green text-sm">▲</span>} + {trend === 'down' && <span aria-hidden="true" className="text-urgency-red text-sm">▼</span>} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/ui/MetricCard.tsx` around lines 15 - 18, The trend glyphs in MetricCard are currently visual-only; update the two spans that render the ▲/▼ (the JSX that checks trend === 'up' and trend === 'down') to mark the glyph elements aria-hidden="true" and include an adjacent screen-reader-only text node (e.g., a span with an sr-only class) that reads an accessible label like "Increasing" for up and "Decreasing" for down so assistive tech receives the meaning while the glyph remains visual-only.ui/src/lib/focus-mode.tsx (1)
13-24: Harden localStorage access for non-browser or blocked storage.
localStorageaccess can throw in SSR/test environments or restricted storage modes. Guard and fail safe to default ON.Proposed fix
export function FocusModeProvider({ children }: { children: ReactNode }) { const [focusMode, setFocusMode] = useState(() => { - const saved = localStorage.getItem('chittycommand_focus_mode'); - return saved !== null ? saved === 'true' : true; // default ON + if (typeof window === 'undefined') return true; + try { + const saved = window.localStorage.getItem('chittycommand_focus_mode'); + return saved !== null ? saved === 'true' : true; // default ON + } catch { + return true; + } }); const toggleFocusMode = useCallback(() => { setFocusMode((prev) => { const next = !prev; - localStorage.setItem('chittycommand_focus_mode', String(next)); + try { + window.localStorage.setItem('chittycommand_focus_mode', String(next)); + } catch { + // ignore write failures (private mode / quota / disabled storage) + } return next; }); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/lib/focus-mode.tsx` around lines 13 - 24, FocusModeProvider currently accesses localStorage directly in the useState initializer and in toggleFocusMode which can throw in SSR/tests or when storage is blocked; wrap reads/writes in safe guards (check typeof window !== 'undefined' and wrap localStorage.getItem/localStorage.setItem calls in try/catch) and fall back to default true on error; update the useState initializer (FocusModeProvider) to catch exceptions and return true if storage is unavailable, and update toggleFocusMode to attempt to persist inside try/catch without throwing if setItem fails.ui/src/components/ui/ProgressDots.tsx (1)
9-23: Clamp progress values to avoid RangeError and overfill.
Array.from({ length: total })throws iftotalis negative, andcompleted > totaloverfills the visual state. Consider clamping defensively.Proposed fix
export function ProgressDots({ completed, total, className }: ProgressDotsProps) { + const safeTotal = Math.max(0, total); + const safeCompleted = Math.min(Math.max(0, completed), safeTotal); return ( <div className={cn('flex items-center gap-1', className)}> - {Array.from({ length: total }, (_, i) => ( + {Array.from({ length: safeTotal }, (_, i) => ( <span key={i} className={cn( 'w-2 h-2 rounded-full', - i < completed ? 'bg-urgency-green' : 'bg-card-border', + i < safeCompleted ? 'bg-urgency-green' : 'bg-card-border', )} /> ))} <span className="text-xs text-card-muted ml-1"> - {completed}/{total} + {safeCompleted}/{safeTotal} </span> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/ui/ProgressDots.tsx` around lines 9 - 23, The ProgressDots component should defensively clamp the input props so negative or non-finite totals and out-of-range completed values cannot cause a RangeError or visual overfill: inside ProgressDots compute a safeTotal (e.g., Math.max(0, Math.floor(total || 0)) or ensure Number.isFinite) and a safeCompleted clamped between 0 and safeTotal (e.g., Math.min(Math.max(0, Math.floor(completed || 0)), safeTotal)), then use safeTotal for Array.from length and safeCompleted for deciding the filled dot class and the displayed {completed}/{total} values (or display the clamped values) to prevent errors and overfill.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-02-23-ui-overhaul-design.md`:
- Around line 69-71: Update the text under the "Color-Coded Urgency Borders"
section to use a space in "48 hrs" for readability; locate the line that
currently reads "Red (4px left border): overdue or due within 48hrs" and change
"48hrs" to "48 hrs" so it becomes "Red (4px left border): overdue or due within
48 hrs".
In `@docs/plans/2026-02-23-ui-overhaul-plan.md`:
- Around line 15-16: The heading "### Task 1: Install Dependencies & Font Setup"
is skipping a level under the H1; change it from an H3 to an H2 by replacing
"### Task 1: Install Dependencies & Font Setup" with "## Task 1: Install
Dependencies & Font Setup" so the document uses a proper H1 -> H2 hierarchy
(locate the heading string in the file to update).
- Around line 1438-1441: The fenced block under "### Dependency Graph" is
missing a language tag (MD040); update the opening triple-backtick for the code
block that contains "Task 1 (fonts/deps) ──┐" to include a language identifier
(e.g., change ``` to ```text) so the block is recognized as text in the markdown
renderer and linter.
In `@ui/package.json`:
- Line 24: Remove the redundant type package entry "@types/react-grid-layout":
"^1.3.6" from package.json (where it appears under dependencies/devDependencies)
because react-grid-layout v2 bundles its own types; after removing that
dependency, update the lockfile by running your package manager install
(npm/yarn/pnpm) and verify there are no remaining references to
"@types/react-grid-layout" in package-lock.json/yarn.lock so the project uses
the built-in types from react-grid-layout.
- Around line 12-18: The devDependency `@types/react-grid-layout` is causing type
conflicts because react-grid-layout@2.2.2 includes built-in TypeScript types;
open ui/package.json and remove the `@types/react-grid-layout` entry from
devDependencies, then run your package manager install (npm/yarn/pnpm) to update
the lockfile; also check for any explicit import/type references to
`@types/react-grid-layout` in your repo (tsconfig, triple-slash refs) and remove
them so TypeScript uses the bundled types from react-grid-layout.
In `@ui/src/components/dashboard/FocusView.tsx`:
- Around line 77-90: The current code slices recommendations before computing
urgency which can drop higher-priority items if the list isn't pre-sorted; sort
the array by rec.priority (e.g., recommendations.sort((a,b) => a.priority -
b.priority) if lower numbers mean higher priority) before calling
recommendations.slice(0, 3) inside the loop that calls items.push, or
alternatively remove the early slice and apply the limit after mapping/urgency
calculation; update the block that references recommendations.slice, items.push,
rec.priority, onExecute and executingId accordingly so the top-3
highest-priority recommendations are used.
In `@ui/src/components/ui/ActionButton.tsx`:
- Around line 12-25: ActionButton currently lacks an explicit HTML button type
which can cause unintended form submissions; update the ActionButtonProps
interface to include an optional type?: "button" | "submit" | "reset" (default
"button"), ensure the component signature accepts the type prop (e.g., function
ActionButton({... , type = "button"}: ActionButtonProps)) and pass it into the
rendered <button> as type={type}; keep existing logic for disabled/loading and
className intact.
In `@ui/src/components/ui/Card.tsx`:
- Around line 20-29: The Card component's root <div> becomes interactive when
the onClick prop is provided but isn't keyboard-accessible; update the Card
(root element in Card.tsx that uses onClick and cn) to add keyboard semantics:
when onClick is present set tabIndex={0} and role="button", and add an onKeyDown
handler that calls the same onClick callback when Enter (key === "Enter") or
Space (key === " " or key === "Spacebar") is pressed (for Space preventDefault
to avoid scrolling); keep existing onClick behavior and className handling (cn,
borderColor, muted) intact and ensure any disabled-like state uses aria-disabled
if applicable.
In `@ui/src/components/ui/FreshnessDot.tsx`:
- Around line 20-25: The function freshnessFromDate currently treats an invalid
date string as 'failed' because new Date(dateStr).getTime() becomes NaN; update
freshnessFromDate to parse the date into a variable (e.g., const parsed = new
Date(dateStr)), check whether parsed.getTime() is a valid number (use
Number.isNaN or !isFinite(parsed.getTime())), and immediately return 'unknown'
for invalid timestamps before computing hours and returning
'fresh'/'stale'/'failed'.
In `@ui/src/components/ui/UrgencyBorder.tsx`:
- Around line 1-5: The function urgencyLevel currently treats NaN/Infinity as
valid numbers and they fall through to 'green'; update urgencyLevel to treat
non-finite values as null by checking Number.isFinite(score) (or an equivalent
isFinite guard) and returning null when the score is null/undefined or not
finite, then preserve the existing thresholds (>=70 => 'red', >=40 => 'amber',
else 'green').
In `@ui/src/index.css`:
- Line 36: Stylelint is flagging the quoted font family name 'Outfit'—remove the
quotes around Outfit in the font-family declaration so it reads without quotes
(i.e., use Outfit, -apple-system, BlinkMacSystemFont, sans-serif) to satisfy the
font-family-name-quotes rule; update the font-family declaration in the rule
that currently contains "font-family: 'Outfit', -apple-system,
BlinkMacSystemFont, sans-serif;" accordingly.
In `@ui/src/pages/Bills.tsx`:
- Around line 20-27: The handler handleMarkPaid currently swallows errors
because it uses try/finally; update it to use try/catch/finally so api.markPaid
failures are caught and surfaced via the component error state (call setError
with the caught error or a user-friendly message), still clear the loading
indicator in finally (setPayingId(null)), and only update obligations via
setObligations on success; reference handleMarkPaid, api.markPaid,
setObligations, setError, and setPayingId when making the change.
In `@ui/src/pages/Disputes.tsx`:
- Around line 31-46: The togglePanel function can briefly show stale data when
switching disputes; before awaiting api.getDispute(disputeId) clear the relevant
list so the UI doesn't render the previous dispute's items. Inside togglePanel
(reference function name togglePanel and state setters setCorrespondenceList and
setDocumentList), after setting setExpandedId and setActivePanel but before the
try/await, call setCorrespondenceList([]) when panel === 'correspondence' or
setDocumentList([]) when panel === 'documents' so the list is reset immediately,
then proceed with the fetch and repopulate on success/fallback to empty on
error.
In `@ui/src/pages/Legal.tsx`:
- Around line 16-27: The countdownColor function is inconsistent with
urgencyFromDays for the 8–30 day range (urgencyFromDays returns 'green' but
countdownColor returns amber); update countdownColor (the function named
countdownColor) so its conditional mapping matches urgencyFromDays—specifically
return 'text-urgency-green' for days >7 && days <=30 (and keep the same outputs
for <0 and <=7 and the default), ensuring the string CSS classes correspond to
the urgency keys used by urgencyFromDays.
In `@ui/src/pages/Upload.tsx`:
- Around line 89-94: The ActionButton's built-in loading state overrides the
label so "Uploading…" never appears; update Upload.tsx where ActionButton is
used (the instance with label={uploading ? 'Uploading...' : `Upload
${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} and props
onClick={handleUploadAll} loading={uploading} disabled={!pendingCount}) to
either remove the loading={uploading} prop and drive the disabled state with
uploading (e.g., disabled={!pendingCount || uploading}) so your explicit label
is shown, or extend ActionButton to accept a loadingLabel prop and pass
loadingLabel="Uploading..." while keeping loading={uploading}; adjust only the
ActionButton usage or the ActionButton component accordingly.
---
Outside diff comments:
In `@ui/src/pages/CashFlow.tsx`:
- Around line 19-31: The catch in load() uses setError(e.message) which throws
for non-Error rejections; change the handler to normalize unknown rejections
before calling setError (e.g., extract message via e instanceof Error ?
e.message : String(e) or use an existing getErrorMessage helper) so
Promise.all(...).catch(...) always passes a safe string or Error-compatible
value to setError; update the catch block in load() to compute a safe message
and call setError(safeMessage).
In `@ui/src/pages/Recommendations.tsx`:
- Around line 46-54: The handlers act and dismiss update recs using the current
closure value, which can be stale when multiple async ops complete out of order;
change both setRecs calls in act and dismiss to use functional updates (e.g.,
setRecs(prev => prev.filter(r => r.id !== id))) so removal is applied against
the latest state after the awaited api calls; keep awaiting
api.actOnRecommendation and api.dismissRecommendation as-is and only replace the
direct setRecs(recs.filter(...)) calls in the act and dismiss functions.
---
Nitpick comments:
In `@ui/src/components/ui/MetricCard.tsx`:
- Around line 15-18: The trend glyphs in MetricCard are currently visual-only;
update the two spans that render the ▲/▼ (the JSX that checks trend === 'up' and
trend === 'down') to mark the glyph elements aria-hidden="true" and include an
adjacent screen-reader-only text node (e.g., a span with an sr-only class) that
reads an accessible label like "Increasing" for up and "Decreasing" for down so
assistive tech receives the meaning while the glyph remains visual-only.
In `@ui/src/components/ui/ProgressDots.tsx`:
- Around line 9-23: The ProgressDots component should defensively clamp the
input props so negative or non-finite totals and out-of-range completed values
cannot cause a RangeError or visual overfill: inside ProgressDots compute a
safeTotal (e.g., Math.max(0, Math.floor(total || 0)) or ensure Number.isFinite)
and a safeCompleted clamped between 0 and safeTotal (e.g., Math.min(Math.max(0,
Math.floor(completed || 0)), safeTotal)), then use safeTotal for Array.from
length and safeCompleted for deciding the filled dot class and the displayed
{completed}/{total} values (or display the clamped values) to prevent errors and
overfill.
In `@ui/src/lib/focus-mode.tsx`:
- Around line 13-24: FocusModeProvider currently accesses localStorage directly
in the useState initializer and in toggleFocusMode which can throw in SSR/tests
or when storage is blocked; wrap reads/writes in safe guards (check typeof
window !== 'undefined' and wrap localStorage.getItem/localStorage.setItem calls
in try/catch) and fall back to default true on error; update the useState
initializer (FocusModeProvider) to catch exceptions and return true if storage
is unavailable, and update toggleFocusMode to attempt to persist inside
try/catch without throwing if setItem fails.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
CHARTER.mdCLAUDE.mddocs/plans/2026-02-23-ui-overhaul-design.mddocs/plans/2026-02-23-ui-overhaul-plan.mdui/index.htmlui/package.jsonui/src/components/Layout.tsxui/src/components/Sidebar.tsxui/src/components/StatusBar.tsxui/src/components/dashboard/DeadlinesWidget.tsxui/src/components/dashboard/DisputesWidget.tsxui/src/components/dashboard/FocusView.tsxui/src/components/dashboard/FullView.tsxui/src/components/dashboard/ObligationsWidget.tsxui/src/components/dashboard/RecommendationsWidget.tsxui/src/components/ui/ActionButton.tsxui/src/components/ui/Card.tsxui/src/components/ui/FreshnessDot.tsxui/src/components/ui/MetricCard.tsxui/src/components/ui/ProgressDots.tsxui/src/components/ui/UrgencyBorder.tsxui/src/index.cssui/src/lib/focus-mode.tsxui/src/main.tsxui/src/pages/Accounts.tsxui/src/pages/Bills.tsxui/src/pages/CashFlow.tsxui/src/pages/Dashboard.tsxui/src/pages/Disputes.tsxui/src/pages/Legal.tsxui/src/pages/Login.tsxui/src/pages/Recommendations.tsxui/src/pages/Settings.tsxui/src/pages/Upload.tsxui/tailwind.config.js
Comment on lines
+69
to
+71
| ### 4. Color-Coded Urgency Borders | ||
| - Red (4px left border): overdue or due within 48hrs | ||
| - Amber: due within 7 days or needs attention |
Contributor
There was a problem hiding this comment.
Use a space in “48 hrs” for readability.
The “48hrs” token is a minor style/grammar issue.
🧰 Tools
🪛 LanguageTool
[grammar] ~70-~70: Ensure spelling is correct
Context: ...4px left border): overdue or due within 48hrs - Amber: due within 7 days or needs attent...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-02-23-ui-overhaul-design.md` around lines 69 - 71, Update the
text under the "Color-Coded Urgency Borders" section to use a space in "48 hrs"
for readability; locate the line that currently reads "Red (4px left border):
overdue or due within 48hrs" and change "48hrs" to "48 hrs" so it becomes "Red
(4px left border): overdue or due within 48 hrs".
Comment on lines
+15
to
+16
| ### Task 1: Install Dependencies & Font Setup | ||
|
|
Contributor
There was a problem hiding this comment.
Fix heading level jump (MD001).
“### Task 1” should be an H2 under the H1 to avoid skipping a level.
✅ Suggested fix
-### Task 1: Install Dependencies & Font Setup
+## Task 1: Install Dependencies & Font Setup📝 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
| ### Task 1: Install Dependencies & Font Setup | |
| ## Task 1: Install Dependencies & Font Setup |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-02-23-ui-overhaul-plan.md` around lines 15 - 16, The heading
"### Task 1: Install Dependencies & Font Setup" is skipping a level under the
H1; change it from an H3 to an H2 by replacing "### Task 1: Install Dependencies
& Font Setup" with "## Task 1: Install Dependencies & Font Setup" so the
document uses a proper H1 -> H2 hierarchy (locate the heading string in the file
to update).
Comment on lines
+1438
to
+1441
| ### Dependency Graph | ||
|
|
||
| ``` | ||
| Task 1 (fonts/deps) ──┐ |
Contributor
There was a problem hiding this comment.
Add a language to the dependency-graph code fence (MD040).
Specify a language (e.g., text) for the fenced block.
✅ Suggested fix
-```
+```text
Task 1 (fonts/deps) ──┐
├── Task 3 (shared components)
Task 2 (tailwind) ────┘ │
...
-```
+```📝 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
| ### Dependency Graph | |
| ``` | |
| Task 1 (fonts/deps) ──┐ | |
| ### Dependency Graph | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1440-1440: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-02-23-ui-overhaul-plan.md` around lines 1438 - 1441, The
fenced block under "### Dependency Graph" is missing a language tag (MD040);
update the opening triple-backtick for the code block that contains "Task 1
(fonts/deps) ──┐" to include a language identifier (e.g., change ``` to ```text)
so the block is recognized as text in the markdown renderer and linter.
Comment on lines
+12
to
+18
| "clsx": "^2.1.0", | ||
| "lucide-react": "^0.400.0", | ||
| "react": "^18.3.0", | ||
| "react-dom": "^18.3.0", | ||
| "react-grid-layout": "^2.2.2", | ||
| "react-router-dom": "^6.23.0", | ||
| "lucide-react": "^0.400.0", | ||
| "clsx": "^2.1.0", | ||
| "recharts": "^3.7.0", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
clsx@2.1.0 npm package react compatibility
💡 Result:
clsx@2.1.0 has no React dependency and no React peerDependencies, so it’s compatible with any React version (it’s just a small JavaScript utility you can use to build className strings in React or any other framework). [1]
Other relevant constraints:
- Node.js engine:
>= 6(per the package metadata). [1] 2.1.0also introduced the optionalclsx/liteentrypoint (string-only variant), which is often convenient in React/Tailwind patterns. [2]
Sources: [1] [2]
🌐 Web query:
lucide-react@0.400.0 npm package react 18 compatibility
💡 Result:
lucide-react@0.400.0 is compatible with React 18 (its peer dependency range includes ^18.0.0, i.e., React 18 is supported). Evidence: nearby lucide-react versions declare react peer deps that include React 18 (e.g., ^16.5.1 || ^17.0.0 || ^18.0.0 ...). [1][2]
Sources:
[1] lucide-react peerDependencies snippet (0.403.0) showing React 18 in range: react: ^16.5.1 || ^17.0.0 || ^18.0.0 || ^19.0.0 https://git.hackclub.app/ahmadk953/tasko/commit/7625baf1ebcffeaf2edfe1e37093446ca68174fd
[2] lucide-react package.json (0.544.0) peerDependencies including React 18: react: ^16.5.1 || ^17.0.0 || ^18.0.0 || ^19.0.0 https://app.unpkg.com/lucide-react%400.544.0/files/package.json
🌐 Web query:
react-grid-layout@2.2.2 npm package peer dependencies react
💡 Result:
react-grid-layout@2.2.2 declares these peer dependencies:
react:>= 16.3.0react-dom:>= 16.3.0[1]
🌐 Web query:
recharts@3.7.0 npm package react 18 compatibility peer dependencies
💡 Result:
recharts@3.7.0 is compatible with React 18: its peerDependencies allow react ^18.0.0 (as well as React 16.8, 17, and 19). [1]
Peer dependencies (recharts@3.7.0):
react:^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0[1]react-dom:^16.0.0 || ^17.0.0 || ^18.0.0 || ^19.0.0[1]react-is:^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0[1]
Practical note: Recharts maintainers recommend keeping matching versions of react, react-dom, and react-is to avoid install/runtime issues. [2]
Sources
[1] TriggerFrontend package-lock.json showing recharts@3.7.0 peerDependencies
[2] Recharts GitHub Discussion #5701 (dependency/peerDependency guidance)
🌐 Web query:
@types/react-grid-layout npm package react-grid-layout version compatibility
💡 Result:
Compatibility summary
Your react-grid-layout version |
Should you install @types/react-grid-layout? |
What version of @types/react-grid-layout to use |
|---|---|---|
| v2.x (>= 2.0.0) | No — v2 ships its own TypeScript types (“no more @types/react-grid-layout”). |
If you install it, you’ll likely want to remove it to avoid conflicts. [1] |
| v1.x | Yes — v1 relies on DefinitelyTyped types. | Pin to the 1.3.x line (latest shown as 1.3.6). [2] |
Any version, but you installed @types/react-grid-layout@2.x (e.g., 2.1.0) |
Don’t — @types/react-grid-layout@2.x is a stub package that says the library provides its own types (intended for v2+). |
Use 1.3.x for react-grid-layout@1.x, or remove it for react-grid-layout@2.x. [3] |
Notes
react-grid-layoutitself documents version compatibility as: >= 2.0.0 (React 18+, TypeScript) and >= 0.17.0 (React 16 & 17). [1]
Sources: [1] [2] [3]
Remove @types/react-grid-layout devDependency—react-grid-layout v2.2.2 includes built-in TypeScript types.
All specified package versions are compatible with React 18.3.x: clsx@^2.1.0 is framework-agnostic; lucide-react@^0.400.0 supports React 18; react-grid-layout@^2.2.2 requires React >= 16.3.0; and recharts@^3.7.0 supports React 18. However, react-grid-layout v2.x ships its own types, so adding @types/react-grid-layout will cause type conflicts. Remove it from devDependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/package.json` around lines 12 - 18, The devDependency
`@types/react-grid-layout` is causing type conflicts because
react-grid-layout@2.2.2 includes built-in TypeScript types; open ui/package.json
and remove the `@types/react-grid-layout` entry from devDependencies, then run
your package manager install (npm/yarn/pnpm) to update the lockfile; also check
for any explicit import/type references to `@types/react-grid-layout` in your repo
(tsconfig, triple-slash refs) and remove them so TypeScript uses the bundled
types from react-grid-layout.
| "devDependencies": { | ||
| "@types/react": "^18.3.0", | ||
| "@types/react-dom": "^18.3.0", | ||
| "@types/react-grid-layout": "^1.3.6", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat ui/package.json | grep -E '(react-grid-layout|@types/react-grid-layout)'Repository: chittyos/chittycommand
Length of output: 142
🌐 Web query:
react-grid-layout v2 TypeScript types bundled
💡 Result:
react-grid-layout v2 bundles its own TypeScript declarations (“Full TypeScript support… no more @types/react-grid-layout”). (github.com)
So you generally should not install @types/react-grid-layout—the @types package is now just a stub that explicitly says the library provides its own types. (security.snyk.io)
Citations:
- 1: https://github.com/react-grid-layout/react-grid-layout?utm_source=openai
- 2: https://security.snyk.io/package/npm/%40types%2Freact-grid-layout/versions?utm_source=openai
Remove @types/react-grid-layout — react-grid-layout v2 bundles its own types.
Since react-grid-layout v2 ships with built-in TypeScript declarations, the @types/react-grid-layout package is now a stub that explicitly delegates to the library's own types. Installing both can create redundancy and potential type confusion. Remove @types/react-grid-layout from dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/package.json` at line 24, Remove the redundant type package entry
"@types/react-grid-layout": "^1.3.6" from package.json (where it appears under
dependencies/devDependencies) because react-grid-layout v2 bundles its own
types; after removing that dependency, update the lockfile by running your
package manager install (npm/yarn/pnpm) and verify there are no remaining
references to "@types/react-grid-layout" in package-lock.json/yarn.lock so the
project uses the built-in types from react-grid-layout.
Comment on lines
+1
to
+5
| export function urgencyLevel(score: number | null): 'red' | 'amber' | 'green' | null { | ||
| if (score === null || score === undefined) return null; | ||
| if (score >= 70) return 'red'; | ||
| if (score >= 40) return 'amber'; | ||
| return 'green'; |
Contributor
There was a problem hiding this comment.
Guard against NaN/Infinity scores.
NaN currently falls through to 'green'. Treat non-finite values as null to avoid misleading urgency.
Proposed fix
export function urgencyLevel(score: number | null): 'red' | 'amber' | 'green' | null {
if (score === null || score === undefined) return null;
+ if (!Number.isFinite(score)) return null;
if (score >= 70) return 'red';
if (score >= 40) return 'amber';
return 'green';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/ui/UrgencyBorder.tsx` around lines 1 - 5, The function
urgencyLevel currently treats NaN/Infinity as valid numbers and they fall
through to 'green'; update urgencyLevel to treat non-finite values as null by
checking Number.isFinite(score) (or an equivalent isFinite guard) and returning
null when the score is null/undefined or not finite, then preserve the existing
thresholds (>=70 => 'red', >=40 => 'amber', else 'green').
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Roboto', sans-serif; | ||
| background-color: #0f1117; | ||
| color: #e4e5e7; | ||
| font-family: 'Outfit', -apple-system, BlinkMacSystemFont, sans-serif; |
Contributor
There was a problem hiding this comment.
Fix Stylelint font-family quotes.
Stylelint flags quotes around Outfit; remove them to satisfy font-family-name-quotes.
Proposed fix
- font-family: 'Outfit', -apple-system, BlinkMacSystemFont, sans-serif;
+ font-family: Outfit, -apple-system, BlinkMacSystemFont, sans-serif;📝 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
| font-family: 'Outfit', -apple-system, BlinkMacSystemFont, sans-serif; | |
| font-family: Outfit, -apple-system, BlinkMacSystemFont, sans-serif; |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 36-36: Unexpected quotes around "Outfit" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/index.css` at line 36, Stylelint is flagging the quoted font family
name 'Outfit'—remove the quotes around Outfit in the font-family declaration so
it reads without quotes (i.e., use Outfit, -apple-system, BlinkMacSystemFont,
sans-serif) to satisfy the font-family-name-quotes rule; update the font-family
declaration in the rule that currently contains "font-family: 'Outfit',
-apple-system, BlinkMacSystemFont, sans-serif;" accordingly.
Comment on lines
+16
to
+27
| const urgencyFromDays = (days: number): 'red' | 'amber' | 'green' | null => { | ||
| if (days < 0) return 'red'; | ||
| if (days <= 7) return 'amber'; | ||
| if (days <= 30) return 'green'; | ||
| return null; | ||
| }; | ||
|
|
||
| const countdownColor = (days: number): string => { | ||
| if (days < 0) return 'text-urgency-red'; | ||
| if (days <= 7) return 'text-urgency-amber'; | ||
| if (days <= 30) return 'text-urgency-amber'; | ||
| return 'text-card-muted'; |
Contributor
There was a problem hiding this comment.
Countdown color is inconsistent with urgency rules.
For 8–30 days, urgencyFromDays returns green but countdownColor returns amber, causing mixed signals. Consider aligning them.
💡 Suggested fix
const countdownColor = (days: number): string => {
if (days < 0) return 'text-urgency-red';
if (days <= 7) return 'text-urgency-amber';
- if (days <= 30) return 'text-urgency-amber';
+ if (days <= 30) return 'text-urgency-green';
return 'text-card-muted';
};📝 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 urgencyFromDays = (days: number): 'red' | 'amber' | 'green' | null => { | |
| if (days < 0) return 'red'; | |
| if (days <= 7) return 'amber'; | |
| if (days <= 30) return 'green'; | |
| return null; | |
| }; | |
| const countdownColor = (days: number): string => { | |
| if (days < 0) return 'text-urgency-red'; | |
| if (days <= 7) return 'text-urgency-amber'; | |
| if (days <= 30) return 'text-urgency-amber'; | |
| return 'text-card-muted'; | |
| const urgencyFromDays = (days: number): 'red' | 'amber' | 'green' | null => { | |
| if (days < 0) return 'red'; | |
| if (days <= 7) return 'amber'; | |
| if (days <= 30) return 'green'; | |
| return null; | |
| }; | |
| const countdownColor = (days: number): string => { | |
| if (days < 0) return 'text-urgency-red'; | |
| if (days <= 7) return 'text-urgency-amber'; | |
| if (days <= 30) return 'text-urgency-green'; | |
| return 'text-card-muted'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Legal.tsx` around lines 16 - 27, The countdownColor function is
inconsistent with urgencyFromDays for the 8–30 day range (urgencyFromDays
returns 'green' but countdownColor returns amber); update countdownColor (the
function named countdownColor) so its conditional mapping matches
urgencyFromDays—specifically return 'text-urgency-green' for days >7 && days
<=30 (and keep the same outputs for <0 and <=7 and the default), ensuring the
string CSS classes correspond to the urgency keys used by urgencyFromDays.
Comment on lines
+89
to
+94
| <ActionButton | ||
| label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} | ||
| onClick={handleUploadAll} | ||
| disabled={uploading || !pendingCount} | ||
| className="px-4 py-1.5 text-sm bg-chitty-600 text-white rounded hover:bg-chitty-700 disabled:opacity-50 transition-colors" | ||
| > | ||
| {uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} | ||
| </button> | ||
| loading={uploading} | ||
| disabled={!pendingCount} | ||
| /> |
Contributor
There was a problem hiding this comment.
Loading label won’t show while loading is true.
ActionButton renders “Working…” when loading is true, so the “Uploading…” label never appears. If you want the explicit label, drive the disabled state instead of loading, or extend ActionButton with a loadingLabel.
💡 Suggested adjustment (keep explicit label)
<ActionButton
label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`}
onClick={handleUploadAll}
- loading={uploading}
- disabled={!pendingCount}
+ disabled={!pendingCount || uploading}
/>📝 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
| <ActionButton | |
| label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} | |
| onClick={handleUploadAll} | |
| disabled={uploading || !pendingCount} | |
| className="px-4 py-1.5 text-sm bg-chitty-600 text-white rounded hover:bg-chitty-700 disabled:opacity-50 transition-colors" | |
| > | |
| {uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} | |
| </button> | |
| loading={uploading} | |
| disabled={!pendingCount} | |
| /> | |
| <ActionButton | |
| label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} | |
| onClick={handleUploadAll} | |
| disabled={!pendingCount || uploading} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Upload.tsx` around lines 89 - 94, The ActionButton's built-in
loading state overrides the label so "Uploading…" never appears; update
Upload.tsx where ActionButton is used (the instance with label={uploading ?
'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`}
and props onClick={handleUploadAll} loading={uploading}
disabled={!pendingCount}) to either remove the loading={uploading} prop and
drive the disabled state with uploading (e.g., disabled={!pendingCount ||
uploading}) so your explicit label is shown, or extend ActionButton to accept a
loadingLabel prop and pass loadingLabel="Uploading..." while keeping
loading={uploading}; adjust only the ActionButton usage or the ActionButton
component accordingly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 908fdd7fa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
|
|
||
| if (error) { | ||
| if (error && !data) { |
There was a problem hiding this comment.
The new if (error && !data) guard suppresses errors once the first dashboard payload is present. When markPaid or actOnRecommendation fails, setError(...) is still called, but the page keeps rendering FocusView/FullView with no visible failure signal, so users can believe the action succeeded when it did not. Keep surfacing a visible error message even when data already exists.
Useful? React with 👍 / 👎.
Comment on lines
+12
to
+15
| useEffect(() => { | ||
| api.getDashboard().then(setData).catch(() => {}); | ||
| api.getSyncStatus().then(setSyncs).catch(() => {}); | ||
| }, []); |
There was a problem hiding this comment.
The status bar fetches dashboard/sync data only once via useEffect(..., []), so header values go stale during normal use. After actions like paying obligations on the dashboard or running syncs in settings, the cash/overdue counters and freshness indicators can remain outdated until a full reload, which makes the top bar disagree with the rest of the UI.
Useful? React with 👍 / 👎.
Backfill canonical governance sections to align with CHITTYFOUNDATION charter standards. Also updates last-updated date. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… tokens
- Replace window.location.href with useNavigate() in FocusView (SPA nav fix)
- Replace hardcoded hex colors with CSS custom properties in CashFlow chart
- Add try/catch to Recommendations act()/dismiss() (prevent silent data loss)
- Replace all catch(() => {}) with console.error in StatusBar, Settings, Upload
- Add catch block to Bills handleMarkPaid for financial operation safety
- Add panelError state to Disputes togglePanel (distinguish error vs empty)
- Fix StatusBar reduce null assertion, switch parseInt to Number()
- Add stable key={item.id} instead of array index in FocusView
- Add safety clamping (completed <= total) in ProgressDots
- Rename UrgencyBorder.tsx to .ts (exports functions, not JSX)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ui/src/pages/Settings.tsx (2)
38-49:⚠️ Potential issue | 🟡 MinorConcurrent row syncs corrupt each other's loading indicator
triggeringis a singlestring | null. If a user clicks "Sync Now" on two rows in quick succession, the second click overwrites the first value — the first row's spinner disappears even though its request is still in flight. When the first request'sfinallyblock runs, it setstriggering = null, hiding the second row's spinner prematurely.🛠️ Suggested fix — use a `Set`
-const [triggering, setTriggering] = useState<string | null>(null); +const [triggering, setTriggering] = useState<Set<string>>(new Set());const triggerSync = async (source: string) => { - setTriggering(source); + setTriggering((prev) => new Set(prev).add(source)); try { await api.triggerSync(source); const updated = await api.getSyncStatus(); setSyncStatuses(updated); } catch (e: unknown) { setError(e instanceof Error ? e.message : 'Sync failed'); } finally { - setTriggering(null); + setTriggering((prev) => { const next = new Set(prev); next.delete(source); return next; }); } };Update all
triggering === src.keychecks totriggering.has(src.key).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Settings.tsx` around lines 38 - 49, The current triggering state (used in triggerSync) is a single string and causes concurrent syncs to clobber each other's loading indicator; change triggering to a Set<string> (e.g., useState<Set<string>>(new Set())), update triggerSync to add the source key into the Set before awaiting (using a new Set copy and setTriggering) and remove that key in the finally block (again by copying and calling setTriggering), and update all UI checks that used `triggering === src.key` to use `triggering.has(src.key)` so multiple rows can show spinners concurrently; keep setTriggering updates immutable (create new Set copies) so React re-renders.
179-179:⚠️ Potential issue | 🟠 MajorBug: case-sensitive
.replace('chitty', '')generates wrong service URLs
'ChittyAuth'.replace('chitty', '')returns'ChittyAuth'unchanged (lowercase'chitty'≠'Chitty'), so the URL becomesChittyAuth.chitty.ccinstead ofauth.chitty.cc. Two additional problems:
- The remaining name isn't lowercased, so even a matching replace would yield
Auth.chitty.cc.- Non-Chitty services (e.g.,
Plaid) would incorrectly get.chitty.ccappended.🐛 Proposed fix
-url={`${svc.name.replace('chitty', '')}.chitty.cc`} +url={svc.name.toLowerCase().startsWith('chitty') + ? `${svc.name.slice('chitty'.length).toLowerCase()}.chitty.cc` + : svc.name.toLowerCase()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Settings.tsx` at line 179, The current URL generation uses svc.name.replace('chitty', '') which is case-sensitive and blindly appends .chitty.cc; instead detect Chitty services with a case-insensitive prefix match (e.g., /^chitty/i), strip that prefix case-insensitively, lower-case the remainder, and only append .chitty.cc for those matches; for non-Chitty services use the service name lowercased (or an alternative host) so services like Plaid aren't given a .chitty.cc suffix — update the expression that builds the URL (the svc.name.replace(...) usage in Settings.tsx) to follow this logic.ui/src/pages/Upload.tsx (1)
51-58:⚠️ Potential issue | 🟡 MinorUnmatched files silently marked as
'done'(line 57).If the filename sanitization on line 53 (
replace(/[^a-zA-Z0-9._-]/g, '_')) doesn't produce the same string the server uses,matchisundefinedand the file falls through to the default{ ...f, status: 'done' }. This silently hides a failed or unprocessed upload.A safer default would be
'error'with a descriptive message so the user knows something unexpected happened.Proposed fix
if (match?.status === 'ok') return { ...f, status: 'done' }; if (match?.status === 'skipped') return { ...f, status: 'skipped', error: 'Duplicate — already uploaded' }; if (match?.status === 'error') return { ...f, status: 'error', error: match.error }; - return { ...f, status: 'done' }; + return { ...f, status: 'error', error: 'No server response for this file' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Upload.tsx` around lines 51 - 58, The mapping in setFiles that updates upload statuses silently marks unmatched files as 'done'; modify the updater (the arrow function passed to setFiles) to treat match === undefined as an error instead of defaulting to done: when result.results.find(...) returns undefined, set the file to { ...f, status: 'error', error: `Upload mismatch: server returned no result for filename "${f.file.name}" (sanitized to "${sanitizedName}")` } (or similar) so users see a descriptive message; keep the existing branches for match.status === 'ok'|'skipped'|'error' and ensure you compute the sanitizedName once to reuse in the error text for clarity.
♻️ Duplicate comments (3)
ui/src/pages/Disputes.tsx (1)
40-46: Clear panel list immediately when switching disputes to avoid stale render.Same issue as noted before: without clearing the list before the fetch, the previous dispute’s items can flash until the new response arrives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Disputes.tsx` around lines 40 - 46, When switching disputes, clear the old panel lists immediately to avoid flashing stale items: after calling setExpandedId(disputeId), setActivePanel(panel) and setPanelError(null), call setCorrespondenceList([]) and setDocumentList([]) (or at least clear the one not needed) before awaiting api.getDispute(disputeId) so the UI renders an empty list while the new detail is fetched; update the logic around setCorrespondenceList and setDocumentList in the Disputes.tsx handler that calls api.getDispute to implement this.ui/src/components/ui/UrgencyBorder.ts (1)
1-6: NaN/Infinity values silently map to'green'.This was flagged in a prior review.
NaN >= 70andNaN >= 40are bothfalse, so non-finite scores fall through to'green', which is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/ui/UrgencyBorder.ts` around lines 1 - 6, The function urgencyLevel currently treats NaN/Infinity as valid and falls through to 'green'; update urgencyLevel to first verify the input is a finite number (e.g., typeof score === 'number' && Number.isFinite(score') or Number.isFinite(score)) and return null for non-finite values (including NaN and ±Infinity) before running the >= 70 / >= 40 checks so only finite numeric scores map to 'red', 'amber', or 'green'.ui/src/pages/Upload.tsx (1)
89-94:ActionButtonloadingprop overrides the custom "Uploading…" label.Same issue flagged in a prior review.
ActionButtonrenders"Working..."whenloading={true}, so your'Uploading...'label is dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Upload.tsx` around lines 89 - 94, The ActionButton's built-in loading behavior overrides your custom 'Uploading...' label; in Upload.tsx stop passing the loading prop and instead control disabled state so your custom label is shown: remove loading={uploading} from the <ActionButton ... /> call and change disabled={!pendingCount} to disabled={uploading || !pendingCount}; keep label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount !== 1 ? 's' : ''}`} and leave handleUploadAll as-is (or alternatively update ActionButton to accept a loadingLabel prop if you prefer keeping a spinner).
🧹 Nitpick comments (6)
ui/src/pages/Settings.tsx (2)
228-241:BridgeSyncButtonduplicatesActionButton— use the shared component insteadThe rest of the page already uses
ActionButton(lines 107-112) for interactive sync controls.BridgeSyncButtonre-implements a button with loading state manually, diverging from the design system.♻️ Proposed refactor
-function BridgeSyncButton({ label, syncing, onClick }: { label: string; syncing: boolean; onClick: () => void }) { - return ( - <button - onClick={onClick} - disabled={syncing} - className="flex items-center justify-between px-3 py-2 text-sm bg-card-hover border border-card-border rounded-lg hover:border-chitty-500 disabled:opacity-50 transition-colors" - > - <span className="text-card-text">{label}</span> - <span className={`text-xs px-2 py-0.5 rounded-full ${syncing ? 'bg-amber-100 text-amber-700' : 'bg-gray-100 text-gray-600'}`}> - {syncing ? 'Syncing...' : 'Sync'} - </span> - </button> - ); -}Replace each
<BridgeSyncButton ... />usage with:<ActionButton label={label} onClick={onClick} loading={syncing} className="w-full text-sm" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Settings.tsx` around lines 228 - 241, BridgeSyncButton duplicates the existing ActionButton design system; replace BridgeSyncButton usages with the shared ActionButton component and remove the duplicate component. Specifically, where BridgeSyncButton is used pass label -> label, onClick -> onClick, syncing -> loading to ActionButton (e.g., <ActionButton label={label} onClick={onClick} loading={syncing} ...>), update any className props as needed (for example className="w-full text-sm"), and delete the BridgeSyncButton function to avoid duplication.
218-226:SyncStatusBadgeuses hardcoded Tailwind color literals instead of design tokensThe new design system introduces urgency/card tokens, but
SyncStatusBadgemaps'completed','started', and'error'to rawbg-green-100 text-green-700etc. Consider aligning totext-urgency-green/text-urgency-redtokens for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Settings.tsx` around lines 218 - 226, SyncStatusBadge currently uses hardcoded Tailwind color classes; update the colors mapping inside the SyncStatusBadge function to use the new design tokens (e.g., replace 'bg-green-100 text-green-700' / 'bg-amber-100 text-amber-700' / 'bg-red-100 text-red-700' with the corresponding token classes such as 'bg-urgency-green text-urgency-green', 'bg-urgency-amber text-urgency-amber', 'bg-urgency-red text-urgency-red' or whatever token names your design system defines), ensure the fallback also uses the neutral design tokens (e.g., 'bg-utility-bg text-utility-fg'), and keep the rest of the JSX in function SyncStatusBadge unchanged so the badge styling is consistent with the new design system.ui/src/components/ui/UrgencyBorder.ts (1)
1-6: Naming collision withurgencyLevelinui/src/lib/urgency.ts.There's another
urgencyLevelexport inui/src/lib/urgency.ts(lines 58–63) with different thresholds (>=50→'high',>=30→'medium') and a different return type ('critical' | 'high' | 'medium' | 'low'). Having two identically named functions with divergent semantics is a maintenance trap—consumers can accidentally import the wrong one.Consider renaming one (e.g.,
urgencyColorhere since it returns color names) or consolidating into a single module.#!/bin/bash # Check which files import from each urgencyLevel source echo "=== Imports from components/ui/UrgencyBorder ===" rg -n "from.*UrgencyBorder" --type=ts --type=tsx -g '!node_modules' rg -n "from.*UrgencyBorder" -g '*.tsx' -g '*.ts' -g '!node_modules' echo "" echo "=== Imports from lib/urgency ===" rg -n "from.*lib/urgency" -g '*.tsx' -g '*.ts' -g '!node_modules' echo "" echo "=== All urgencyLevel references ===" rg -nP '\burgencyLevel\b' -g '*.tsx' -g '*.ts' -g '!node_modules'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/ui/UrgencyBorder.ts` around lines 1 - 6, There is a naming collision between the urgencyLevel exported from UrgencyBorder.ts and another urgencyLevel in ui/src/lib/urgency.ts; rename the function in UrgencyBorder.ts to a distinct, descriptive name (e.g., urgencyColor) and update its export, signature reference (export function urgencyColor(score: number | null): 'red'|'amber'|'green'|null), and all imports across the codebase that currently import urgencyLevel from components/ui/UrgencyBorder to import urgencyColor instead; alternatively, if you prefer consolidation, move the color-mapping logic into ui/src/lib/urgency.ts under a new export name (urgencyColor) and replace the UrgencyBorder.ts usage to import that single function so there's one canonical API.ui/src/pages/Recommendations.tsx (1)
70-86:priorityColorsandtypeColorsare constant dictionaries re-created every render.Move them to module scope to avoid unnecessary allocations.
Move to module level
+const priorityColors: Record<number, string> = { + 1: 'bg-red-100 text-red-700', + 2: 'bg-orange-100 text-orange-700', + 3: 'bg-amber-100 text-amber-700', + 4: 'bg-blue-100 text-blue-700', + 5: 'bg-gray-100 text-gray-700', +}; + +const typeColors: Record<string, string> = { + payment: 'bg-green-50 text-green-700 border-green-200', + negotiate: 'bg-purple-50 text-purple-700 border-purple-200', + defer: 'bg-gray-50 text-gray-600 border-gray-200', + dispute: 'bg-orange-50 text-orange-700 border-orange-200', + legal: 'bg-red-50 text-red-700 border-red-200', + warning: 'bg-amber-50 text-amber-700 border-amber-200', + strategy: 'bg-blue-50 text-blue-700 border-blue-200', +}; + +const actionLabel = (type: string | null): string => { + ... +}; + export function Recommendations() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Recommendations.tsx` around lines 70 - 86, priorityColors and typeColors are being re-created on every render inside the component; move both constant dictionaries to module scope (outside the component function) so they are only allocated once. Locate the definitions of priorityColors and typeColors in Recommendations.tsx, cut them from inside the component, and declare them at top-level with the same types (Record<number,string> and Record<string,string>) so the component can reference them without reallocation. Ensure imports/types used in the file still resolve after moving them.ui/src/pages/Bills.tsx (1)
14-18: No loading state shown while obligations are being fetched.Unlike
Recommendations.tsxwhich has aloadingstate and shows a loading message,Billsrenders an empty list during the initial fetch. Consider adding a loading indicator for better UX, especially on slower connections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/Bills.tsx` around lines 14 - 18, The Bills component's useEffect calls api.getObligations without a loading flag, causing the UI to show an empty list during fetch; add a loading state (e.g., const [loading, setLoading] = useState(false)) in the Bills component, setLoading(true) before calling api.getObligations in the useEffect, then setLoading(false) in a finally block (or after both setObligations and setError) to ensure it's cleared on success or error, and update the component render to show a loading indicator/message when loading is true (mirror the pattern used in Recommendations.tsx); key symbols: useEffect, api.getObligations, setObligations, setError, loading, setLoading.ui/src/pages/CashFlow.tsx (1)
146-165: Use a stable, unique key derived from the underlying data instead of array index.Index keys cause React to reuse DOM elements when the filtered list changes, leading to state/animation issues. The suggested composite key from date and values may work in practice, but consider exposing the database
idfield in theCashflowProjectioninterface and using that instead for guaranteed uniqueness and stability.♻️ Suggested change
- {chartData.filter((e) => e.outflow > 0).slice(0, 15).map((entry, i) => ( - <tr key={i} className="border-b border-card-border/50 last:border-0"> + {chartData.filter((e) => e.outflow > 0).slice(0, 15).map((entry) => ( + <tr key={`${entry.date}-${entry.outflow}-${entry.balance}`} className="border-b border-card-border/50 last:border-0">Or, add
idto theCashflowProjectioninterface inui/src/lib/api.tsand usekey={entry.id}for true stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 146 - 165, The current map in CashFlow.tsx uses the array index as the React key for chartData.map, which is unstable; replace key={i} with a stable unique identifier from the data (preferably entry.id) by adding an id to the CashflowProjection interface in ui/src/lib/api.ts and exposing it from the backend, then use key={entry.id}; if you cannot add an id immediately, use a deterministic composite key such as `${entry.date}-${entry.outflow}-${entry.balance}` for the map over chartData.filter(...).slice(...).map to avoid index-based reuse issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHARTER.md`:
- Line 138: The "Last Updated" date in the document header string "*Charter
Version: 1.0.0 | Last Updated: 2026-02-23*" was regressed; update that header to
the correct date "2026-02-24" so it reflects the commit that added the Three
Aspects and Document Triad sections (locate and edit the header line containing
"Charter Version: 1.0.0 | Last Updated:" and change 2026-02-23 to 2026-02-24).
- Line 130: The checklist line "Service registered in ChittyRegister
(03-1-USA-3846-T-2602-0-57, pending_cert)" uses a checked box "[x]" while also
tagging "pending_cert", which is contradictory; update that line by either
removing the check mark (change "[x]" to "[ ]") until certification is complete,
or keep "[x]" and append a clarifying note such as "(registered; certification
pending asynchronous step)" so the status is unambiguous—edit the exact string
containing "Service registered in ChittyRegister" to implement one of these two
options.
- Around line 113-126: Document Triad is missing the Endpoints and Dependencies
tables in CHITTY.md and CLAUDE.md; copy the canonical Endpoints and Dependencies
tables from CHARTER.md into those two docs in the matching sections: add the
Endpoints table into CHITTY.md’s Endpoints table area and into CLAUDE.md’s API
section (preserving headers/links), and add the Dependencies table into
CHITTY.md’s Dependencies table and into CLAUDE.md’s Architecture section; ensure
table content and field names (Canonical URI, Tier, Domain, Endpoints,
Dependencies) exactly match CHARTER.md so the triad remains synchronized.
In `@ui/src/components/ui/UrgencyBorder.ts`:
- Around line 8-12: The function urgencyFromDays currently treats NaN/non-finite
inputs as 'green' because both comparisons fail; add a guard at the top of
urgencyFromDays that checks Number.isFinite(days) (or !Number.isFinite(days) /
Number.isNaN(days)) and return a safe default (e.g., 'red' to fail
conservatively) or throw an error for invalid input before the existing
comparisons so bad parses don't silently produce a green indicator.
In `@ui/src/pages/Bills.tsx`:
- Line 34: The current early-return in Bills.tsx ("if (error) return <p...>")
removes the whole UI on any error (including markPaid failures); remove that
early return and instead render the error inline above the filter/list UI
(similar to Recommendations.tsx lines ~108–110), add a dismiss button that
clears the Bills component's error state, and change markPaid error handling to
set a per-item error or propagate details so only the affected item shows an
inline error/ retry control; update the Bills component render to always show
the filter bar and obligation list and include retry/dismiss handlers that call
the existing markPaid or reload functions.
In `@ui/src/pages/CashFlow.tsx`:
- Around line 70-76: The date parsing is shifting dates for US timezones because
new Date('YYYY-MM-DD') is treated as UTC midnight; update the locale formatting
to force UTC by adding { timeZone: 'UTC' } to the toLocaleDateString call in the
chartData mapping (the expression new
Date(p.projection_date).toLocaleDateString(...)) so dates don't roll back a day,
and make the identical change inside the formatDate() implementation in
ui/src/lib/utils.ts (update its toLocaleDateString options to include timeZone:
'UTC').
In `@ui/src/pages/Disputes.tsx`:
- Around line 33-53: The togglePanel function can apply stale getDispute
responses when users rapidly switch panels; add a per-request identity check
(request token or AbortController) to ensure only the latest request updates
state: generate a unique token (or use AbortController.signal) at the start of
togglePanel, store it in a ref like currentRequestRef, pass it to the async
request, and before setting state in both success and catch blocks verify the
token matches currentRequestRef (or that the request wasn't aborted); only then
call setCorrespondenceList/setDocumentList, setPanelError, setExpandedId or
setActivePanel so out‑of‑order responses cannot overwrite current panel data.
In `@ui/src/pages/Recommendations.tsx`:
- Around line 100-105: The ActionButton's built-in loading text ("Working...")
overrides the custom label so the generating ? 'Analyzing...' : 'Run Triage'
branch never appears; update Recommendations.tsx to either stop using the
ActionButton's loading prop and pass generating to disabled instead (call
ActionButton with disabled={generating} and remove loading={generating}), or add
a loadingLabel prop to ActionButton and pass loadingLabel="Analyzing..."
alongside loading={generating}; modify the ActionButton component
(ActionButton.tsx) to accept and display loadingLabel when provided (fallback to
its existing "Working..." otherwise) so the UI shows "Analyzing..." when
generate is running.
- Line 129: The urgency mapping on the Card component is inverted for P1; update
the conditional in the Card urgency prop (the JSX using Card key={rec.id}
urgency={...}) so rec.priority === 1 maps to 'red', rec.priority === 2 maps to
'amber', rec.priority === 3 maps to 'green', and any other priority yields null
(e.g., urgency={rec.priority === 1 ? 'red' : rec.priority === 2 ? 'amber' :
rec.priority === 3 ? 'green' : null}).
- Around line 46-55: The act and dismiss handlers close over the stale recs
snapshot and call setRecs(recs.filter(...)), which can drop concurrent updates;
update both functions (act and dismiss) to use the functional updater form
setRecs(prev => prev.filter(r => r.id !== id)) so they always operate on the
latest state, leaving API call and error handling unchanged.
In `@ui/src/pages/Settings.tsx`:
- Line 18: The getBridgeStatus call in Settings.tsx currently swallows failures
into console.error and leaves the Service Connections card displaying hardcoded
"connected" fallback; update the async handling around api.getBridgeStatus in
the Settings component to set an explicit error/availability state (e.g.,
bridgeStatusError or bridgeStatusUnavailable) when the promise rejects instead
of only logging, and use that state in the render path that uses
setServiceStatuses to show a non-blocking warning or mark the Service
Connections section as unavailable/ degraded; ensure you update both call sites
that invoke getBridgeStatus and the component rendering logic that reads
serviceStatuses so the UI reflects the unavailable state rather than silently
showing the fallback "connected" values.
- Line 30: The UI currently sets user-facing state with
setPlaidMessage(`${name}: ${JSON.stringify(result)}`), which leaks raw API
payloads and may expose sensitive or unreadable data; change this to set a
concise, user-friendly message instead (e.g., `${name}: ${status}` or a short
summary) and move the full JSON payload into a non-user-facing sink such as
console.debug or a logger for debugging/telemetry; ensure any sensitive fields
in result are redacted before logging and update the code that renders
plaidMessage to expect the new concise string (referencing setPlaidMessage and
plaidMessage).
In `@ui/src/pages/Upload.tsx`:
- Around line 150-170: The list uses array index as React key (files.map with
key={i}), which breaks when removeFile(i) shifts indices; add a stable id to the
FileEntry model when files are added (e.g., in the function that constructs new
FileEntry objects), use that id as the key (key={entry.id}) in the files.map
render, and change removeFile to accept an id and filter files by entry.id
instead of splicing by index so items retain stable identity; update any
types/interfaces for FileEntry and change call sites that add/remove files
accordingly.
---
Outside diff comments:
In `@ui/src/pages/Settings.tsx`:
- Around line 38-49: The current triggering state (used in triggerSync) is a
single string and causes concurrent syncs to clobber each other's loading
indicator; change triggering to a Set<string> (e.g., useState<Set<string>>(new
Set())), update triggerSync to add the source key into the Set before awaiting
(using a new Set copy and setTriggering) and remove that key in the finally
block (again by copying and calling setTriggering), and update all UI checks
that used `triggering === src.key` to use `triggering.has(src.key)` so multiple
rows can show spinners concurrently; keep setTriggering updates immutable
(create new Set copies) so React re-renders.
- Line 179: The current URL generation uses svc.name.replace('chitty', '') which
is case-sensitive and blindly appends .chitty.cc; instead detect Chitty services
with a case-insensitive prefix match (e.g., /^chitty/i), strip that prefix
case-insensitively, lower-case the remainder, and only append .chitty.cc for
those matches; for non-Chitty services use the service name lowercased (or an
alternative host) so services like Plaid aren't given a .chitty.cc suffix —
update the expression that builds the URL (the svc.name.replace(...) usage in
Settings.tsx) to follow this logic.
In `@ui/src/pages/Upload.tsx`:
- Around line 51-58: The mapping in setFiles that updates upload statuses
silently marks unmatched files as 'done'; modify the updater (the arrow function
passed to setFiles) to treat match === undefined as an error instead of
defaulting to done: when result.results.find(...) returns undefined, set the
file to { ...f, status: 'error', error: `Upload mismatch: server returned no
result for filename "${f.file.name}" (sanitized to "${sanitizedName}")` } (or
similar) so users see a descriptive message; keep the existing branches for
match.status === 'ok'|'skipped'|'error' and ensure you compute the sanitizedName
once to reuse in the error text for clarity.
---
Duplicate comments:
In `@ui/src/components/ui/UrgencyBorder.ts`:
- Around line 1-6: The function urgencyLevel currently treats NaN/Infinity as
valid and falls through to 'green'; update urgencyLevel to first verify the
input is a finite number (e.g., typeof score === 'number' &&
Number.isFinite(score') or Number.isFinite(score)) and return null for
non-finite values (including NaN and ±Infinity) before running the >= 70 / >= 40
checks so only finite numeric scores map to 'red', 'amber', or 'green'.
In `@ui/src/pages/Disputes.tsx`:
- Around line 40-46: When switching disputes, clear the old panel lists
immediately to avoid flashing stale items: after calling
setExpandedId(disputeId), setActivePanel(panel) and setPanelError(null), call
setCorrespondenceList([]) and setDocumentList([]) (or at least clear the one not
needed) before awaiting api.getDispute(disputeId) so the UI renders an empty
list while the new detail is fetched; update the logic around
setCorrespondenceList and setDocumentList in the Disputes.tsx handler that calls
api.getDispute to implement this.
In `@ui/src/pages/Upload.tsx`:
- Around line 89-94: The ActionButton's built-in loading behavior overrides your
custom 'Uploading...' label; in Upload.tsx stop passing the loading prop and
instead control disabled state so your custom label is shown: remove
loading={uploading} from the <ActionButton ... /> call and change
disabled={!pendingCount} to disabled={uploading || !pendingCount}; keep
label={uploading ? 'Uploading...' : `Upload ${pendingCount} File${pendingCount
!== 1 ? 's' : ''}`} and leave handleUploadAll as-is (or alternatively update
ActionButton to accept a loadingLabel prop if you prefer keeping a spinner).
---
Nitpick comments:
In `@ui/src/components/ui/UrgencyBorder.ts`:
- Around line 1-6: There is a naming collision between the urgencyLevel exported
from UrgencyBorder.ts and another urgencyLevel in ui/src/lib/urgency.ts; rename
the function in UrgencyBorder.ts to a distinct, descriptive name (e.g.,
urgencyColor) and update its export, signature reference (export function
urgencyColor(score: number | null): 'red'|'amber'|'green'|null), and all imports
across the codebase that currently import urgencyLevel from
components/ui/UrgencyBorder to import urgencyColor instead; alternatively, if
you prefer consolidation, move the color-mapping logic into
ui/src/lib/urgency.ts under a new export name (urgencyColor) and replace the
UrgencyBorder.ts usage to import that single function so there's one canonical
API.
In `@ui/src/pages/Bills.tsx`:
- Around line 14-18: The Bills component's useEffect calls api.getObligations
without a loading flag, causing the UI to show an empty list during fetch; add a
loading state (e.g., const [loading, setLoading] = useState(false)) in the Bills
component, setLoading(true) before calling api.getObligations in the useEffect,
then setLoading(false) in a finally block (or after both setObligations and
setError) to ensure it's cleared on success or error, and update the component
render to show a loading indicator/message when loading is true (mirror the
pattern used in Recommendations.tsx); key symbols: useEffect,
api.getObligations, setObligations, setError, loading, setLoading.
In `@ui/src/pages/CashFlow.tsx`:
- Around line 146-165: The current map in CashFlow.tsx uses the array index as
the React key for chartData.map, which is unstable; replace key={i} with a
stable unique identifier from the data (preferably entry.id) by adding an id to
the CashflowProjection interface in ui/src/lib/api.ts and exposing it from the
backend, then use key={entry.id}; if you cannot add an id immediately, use a
deterministic composite key such as
`${entry.date}-${entry.outflow}-${entry.balance}` for the map over
chartData.filter(...).slice(...).map to avoid index-based reuse issues.
In `@ui/src/pages/Recommendations.tsx`:
- Around line 70-86: priorityColors and typeColors are being re-created on every
render inside the component; move both constant dictionaries to module scope
(outside the component function) so they are only allocated once. Locate the
definitions of priorityColors and typeColors in Recommendations.tsx, cut them
from inside the component, and declare them at top-level with the same types
(Record<number,string> and Record<string,string>) so the component can reference
them without reallocation. Ensure imports/types used in the file still resolve
after moving them.
In `@ui/src/pages/Settings.tsx`:
- Around line 228-241: BridgeSyncButton duplicates the existing ActionButton
design system; replace BridgeSyncButton usages with the shared ActionButton
component and remove the duplicate component. Specifically, where
BridgeSyncButton is used pass label -> label, onClick -> onClick, syncing ->
loading to ActionButton (e.g., <ActionButton label={label} onClick={onClick}
loading={syncing} ...>), update any className props as needed (for example
className="w-full text-sm"), and delete the BridgeSyncButton function to avoid
duplication.
- Around line 218-226: SyncStatusBadge currently uses hardcoded Tailwind color
classes; update the colors mapping inside the SyncStatusBadge function to use
the new design tokens (e.g., replace 'bg-green-100 text-green-700' /
'bg-amber-100 text-amber-700' / 'bg-red-100 text-red-700' with the corresponding
token classes such as 'bg-urgency-green text-urgency-green', 'bg-urgency-amber
text-urgency-amber', 'bg-urgency-red text-urgency-red' or whatever token names
your design system defines), ensure the fallback also uses the neutral design
tokens (e.g., 'bg-utility-bg text-utility-fg'), and keep the rest of the JSX in
function SyncStatusBadge unchanged so the badge styling is consistent with the
new design system.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHARTER.mdui/src/components/StatusBar.tsxui/src/components/dashboard/FocusView.tsxui/src/components/ui/ProgressDots.tsxui/src/components/ui/UrgencyBorder.tsui/src/pages/Bills.tsxui/src/pages/CashFlow.tsxui/src/pages/Disputes.tsxui/src/pages/Recommendations.tsxui/src/pages/Settings.tsxui/src/pages/Upload.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src/components/dashboard/FocusView.tsx
- ui/src/components/StatusBar.tsx
- ui/src/components/ui/ProgressDots.tsx
Comment on lines
+113
to
+126
| ## Document Triad | ||
|
|
||
| This charter is part of a synchronized documentation triad. Changes to shared fields must propagate. | ||
|
|
||
| | Field | Canonical Source | Also In | | ||
| |-------|-----------------|---------| | ||
| | Canonical URI | CHARTER.md (Classification) | CHITTY.md (blockquote) | | ||
| | Tier | CHARTER.md (Classification) | CHITTY.md (blockquote) | | ||
| | Domain | CHARTER.md (Classification) | CHITTY.md (blockquote), CLAUDE.md (header) | | ||
| | Endpoints | CHARTER.md (API Contract) | CHITTY.md (Endpoints table), CLAUDE.md (API section) | | ||
| | Dependencies | CHARTER.md (Dependencies) | CHITTY.md (Dependencies table), CLAUDE.md (Architecture) | | ||
| | Certification badge | CHITTY.md (Certification) | CHARTER.md frontmatter `status` | | ||
|
|
||
| **Related docs**: [CHITTY.md](CHITTY.md) (badge/one-pager) | [CLAUDE.md](CLAUDE.md) (developer guide) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Document Triad propagation across CHARTER.md, CHITTY.md, and CLAUDE.md
echo "=== Checking if CHITTY.md and CLAUDE.md exist ==="
fd -e md | grep -E "(CHITTY|CLAUDE|CHARTER)" | sort
echo ""
echo "=== Canonical URI in CHITTY.md ==="
rg -n "chittycanon://core/services/chittycommand" --type md
echo ""
echo "=== Domain references (command.chitty.cc) across triad docs ==="
rg -n "command\.chitty\.cc" --type md
echo ""
echo "=== Tier references ==="
rg -n "Tier 5" --type md
echo ""
echo "=== Certification badge references ==="
fd -e md | xargs grep -l "03-1-USA-3846" 2>/dev/null || echo "Badge not found in any .md files"
echo ""
echo "=== Checking CHITTY.md for blockquote content about Canonical URI ==="
if [ -f "CHITTY.md" ]; then
head -50 CHITTY.md
else
echo "CHITTY.md not found"
fi
echo ""
echo "=== Checking CLAUDE.md for API section ==="
if [ -f "CLAUDE.md" ]; then
head -50 CLAUDE.md
else
echo "CLAUDE.md not found"
fiRepository: chittyos/chittycommand
Length of output: 6518
🏁 Script executed:
#!/bin/bash
# Deep verification of remaining Document Triad fields
echo "=== Endpoints in CHITTY.md (looking for Endpoints table) ==="
rg -n "Endpoints" CHITTY.md --type md -A 3
echo ""
echo "=== Endpoints in CHARTER.md ==="
rg -n "Endpoints" CHARTER.md --type md -B 2 -A 3
echo ""
echo "=== Endpoints in CLAUDE.md ==="
rg -n "Endpoints\|API section" CLAUDE.md --type md -B 2 -A 3
echo ""
echo "=== Dependencies table in CHITTY.md ==="
rg -n "Dependencies" CHITTY.md --type md -A 5
echo ""
echo "=== Dependencies section in CHARTER.md ==="
rg -n "Dependencies" CHARTER.md --type md -B 2 -A 8
echo ""
echo "=== Dependencies in CLAUDE.md ==="
rg -n "Dependencies" CLAUDE.md --type md -B 2 -A 5
echo ""
echo "=== Certification badge references (broader search) ==="
rg -n "status|Certification\|badge" CHITTY.md --type md | head -20
echo ""
echo "=== frontmatter status field in CHITTY.md ==="
head -20 CHITTY.md | rg -n "status"
echo ""
echo "=== frontmatter status field in CHARTER.md ==="
head -20 CHARTER.md | rg -n "status"Repository: chittyos/chittycommand
Length of output: 2255
Document Triad is incomplete — Endpoints and Dependencies tables have not been propagated.
The Document Triad's own language requires that changes to shared fields "must propagate" to all three docs. Verification confirms:
- ✓ Canonical URI (
chittycanon://core/services/chittycommand) — synced - ✓ Tier (Tier 5) — synced
- ✓ Domain (
command.chitty.cc) — synced - ✗ Endpoints table — present only in CHARTER.md; missing from CHITTY.md and CLAUDE.md
- ✗ Dependencies table — present only in CHARTER.md; missing from CHITTY.md and CLAUDE.md
- ⚠ Certification badge — frontmatter
status: DRAFTexists but badge table/visual not located
Create the Endpoints and Dependencies tables in CHITTY.md and CLAUDE.md to complete the triad as documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHARTER.md` around lines 113 - 126, Document Triad is missing the Endpoints
and Dependencies tables in CHITTY.md and CLAUDE.md; copy the canonical Endpoints
and Dependencies tables from CHARTER.md into those two docs in the matching
sections: add the Endpoints table into CHITTY.md’s Endpoints table area and into
CLAUDE.md’s API section (preserving headers/links), and add the Dependencies
table into CHITTY.md’s Dependencies table and into CLAUDE.md’s Architecture
section; ensure table content and field names (Canonical URI, Tier, Domain,
Endpoints, Dependencies) exactly match CHARTER.md so the triad remains
synchronized.
| ## Compliance | ||
|
|
||
| - [ ] Service registered in ChittyRegistry | ||
| - [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert) |
Contributor
There was a problem hiding this comment.
[x] and pending_cert are contradictory.
A checked item signals completion, but pending_cert signals it is still in progress. This will mislead anyone using the compliance checklist as a readiness gate.
📝 Suggested correction options
Option A — keep checked, clarify the cert is an async trailing step:
-- [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert)
+- [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57) *(certification pending)*Option B — leave unchecked until certification completes:
-- [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert)
+- [ ] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57) — certification in progress📝 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
| - [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert) | |
| - [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57) *(certification pending)* |
Suggested change
| - [x] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert) | |
| - [ ] Service registered in ChittyRegister (03-1-USA-3846-T-2602-0-57) — certification in progress |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHARTER.md` at line 130, The checklist line "Service registered in
ChittyRegister (03-1-USA-3846-T-2602-0-57, pending_cert)" uses a checked box
"[x]" while also tagging "pending_cert", which is contradictory; update that
line by either removing the check mark (change "[x]" to "[ ]") until
certification is complete, or keep "[x]" and append a clarifying note such as
"(registered; certification pending asynchronous step)" so the status is
unambiguous—edit the exact string containing "Service registered in
ChittyRegister" to implement one of these two options.
|
|
||
| --- | ||
| *Charter Version: 1.0.0 | Last Updated: 2026-02-24* | ||
| *Charter Version: 1.0.0 | Last Updated: 2026-02-23* |
Contributor
There was a problem hiding this comment.
Last Updated date regressed by one day.
The previous value was 2026-02-24; the new value is 2026-02-23, which predates the commit adding the Three Aspects and Document Triad sections. This will cause auditors to believe these sections existed before they were actually written.
📝 Proposed fix
-*Charter Version: 1.0.0 | Last Updated: 2026-02-23*
+*Charter Version: 1.0.0 | Last Updated: 2026-02-24*📝 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
| *Charter Version: 1.0.0 | Last Updated: 2026-02-23* | |
| *Charter Version: 1.0.0 | Last Updated: 2026-02-24* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHARTER.md` at line 138, The "Last Updated" date in the document header
string "*Charter Version: 1.0.0 | Last Updated: 2026-02-23*" was regressed;
update that header to the correct date "2026-02-24" so it reflects the commit
that added the Three Aspects and Document Triad sections (locate and edit the
header line containing "Charter Version: 1.0.0 | Last Updated:" and change
2026-02-23 to 2026-02-24).
Comment on lines
+8
to
+12
| export function urgencyFromDays(days: number): 'red' | 'amber' | 'green' { | ||
| if (days <= 2) return 'red'; | ||
| if (days <= 7) return 'amber'; | ||
| return 'green'; | ||
| } |
Contributor
There was a problem hiding this comment.
urgencyFromDays doesn't guard against NaN/non-finite input.
Same class of issue as urgencyLevel—if days is NaN, both comparisons are false and the function returns 'green'. Since the parameter type is number (not number | null), callers that pass a bad parse result would silently get a green indicator.
Proposed guard
export function urgencyFromDays(days: number): 'red' | 'amber' | 'green' {
+ if (!Number.isFinite(days)) return 'amber'; // safe fallback for bad input
if (days <= 2) return 'red';
if (days <= 7) return 'amber';
return 'green';
}📝 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 function urgencyFromDays(days: number): 'red' | 'amber' | 'green' { | |
| if (days <= 2) return 'red'; | |
| if (days <= 7) return 'amber'; | |
| return 'green'; | |
| } | |
| export function urgencyFromDays(days: number): 'red' | 'amber' | 'green' { | |
| if (!Number.isFinite(days)) return 'amber'; // safe fallback for bad input | |
| if (days <= 2) return 'red'; | |
| if (days <= 7) return 'amber'; | |
| return 'green'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/ui/UrgencyBorder.ts` around lines 8 - 12, The function
urgencyFromDays currently treats NaN/non-finite inputs as 'green' because both
comparisons fail; add a guard at the top of urgencyFromDays that checks
Number.isFinite(days) (or !Number.isFinite(days) / Number.isNaN(days)) and
return a safe default (e.g., 'red' to fail conservatively) or throw an error for
invalid input before the existing comparisons so bad parses don't silently
produce a green indicator.
| }; | ||
|
|
||
| if (error) return <p className="text-red-400">{error}</p>; | ||
| if (error) return <p className="text-urgency-red">{error}</p>; |
Contributor
There was a problem hiding this comment.
Error state replaces the entire page with no recovery path.
Once error is set (from a load failure or a markPaid failure), the component early-returns a bare <p> and the user loses the filter bar, obligation list, and any way to retry or dismiss. A markPaid error on a single item shouldn't nuke the whole page.
Consider showing the error inline (as done in Recommendations.tsx line 108–110) and keeping the rest of the UI visible, or at minimum add a dismiss/retry control.
Proposed fix: inline error with dismiss, remove early return
- if (error) return <p className="text-urgency-red">{error}</p>;
-
const filters = ['', 'pending', 'overdue', 'paid'];
return (
<div className="space-y-4">
+ {error && (
+ <div className="bg-red-50 border border-red-200 rounded-card p-3 text-urgency-red text-sm flex justify-between items-center">
+ <span>{error}</span>
+ <button onClick={() => setError(null)} className="text-urgency-red hover:underline text-xs ml-4">Dismiss</button>
+ </div>
+ )}
<div className="flex items-center justify-between">📝 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 (error) return <p className="text-urgency-red">{error}</p>; | |
| const filters = ['', 'pending', 'overdue', 'paid']; | |
| return ( | |
| <div className="space-y-4"> | |
| {error && ( | |
| <div className="bg-red-50 border border-red-200 rounded-card p-3 text-urgency-red text-sm flex justify-between items-center"> | |
| <span>{error}</span> | |
| <button onClick={() => setError(null)} className="text-urgency-red hover:underline text-xs ml-4">Dismiss</button> | |
| </div> | |
| )} | |
| <div className="flex items-center justify-between"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Bills.tsx` at line 34, The current early-return in Bills.tsx
("if (error) return <p...>") removes the whole UI on any error (including
markPaid failures); remove that early return and instead render the error inline
above the filter/list UI (similar to Recommendations.tsx lines ~108–110), add a
dismiss button that clears the Bills component's error state, and change
markPaid error handling to set a per-item error or propagate details so only the
affected item shows an inline error/ retry control; update the Bills component
render to always show the filter bar and obligation list and include
retry/dismiss handlers that call the existing markPaid or reload functions.
Comment on lines
+100
to
+105
| <h1 className="text-xl font-bold text-chrome-text">AI Recommendations</h1> | ||
| <ActionButton | ||
| label={generating ? 'Analyzing...' : 'Run Triage'} | ||
| onClick={generate} | ||
| disabled={generating} | ||
| className="px-4 py-2 text-sm bg-chitty-600 text-white rounded-lg hover:bg-chitty-700 disabled:opacity-50 transition-colors font-medium" | ||
| > | ||
| {generating ? 'Analyzing...' : 'Run Triage'} | ||
| </button> | ||
| loading={generating} | ||
| /> |
Contributor
There was a problem hiding this comment.
ActionButton's built-in loading text overrides the custom "Analyzing…" label.
ActionButton renders "Working..." when loading is true (see ActionButton.tsx line 24), so the 'Analyzing...' branch of your ternary is dead code. Either drive only disabled (as suggested in Upload.tsx's past review), or extend ActionButton to accept a loadingLabel prop.
Quick fix (use disabled instead of loading)
<ActionButton
- label={generating ? 'Analyzing...' : 'Run Triage'}
+ label={generating ? 'Analyzing...' : 'Run Triage'}
onClick={generate}
- loading={generating}
+ disabled={generating}
/>📝 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
| <h1 className="text-xl font-bold text-chrome-text">AI Recommendations</h1> | |
| <ActionButton | |
| label={generating ? 'Analyzing...' : 'Run Triage'} | |
| onClick={generate} | |
| disabled={generating} | |
| className="px-4 py-2 text-sm bg-chitty-600 text-white rounded-lg hover:bg-chitty-700 disabled:opacity-50 transition-colors font-medium" | |
| > | |
| {generating ? 'Analyzing...' : 'Run Triage'} | |
| </button> | |
| loading={generating} | |
| /> | |
| <h1 className="text-xl font-bold text-chrome-text">AI Recommendations</h1> | |
| <ActionButton | |
| label={generating ? 'Analyzing...' : 'Run Triage'} | |
| onClick={generate} | |
| disabled={generating} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Recommendations.tsx` around lines 100 - 105, The ActionButton's
built-in loading text ("Working...") overrides the custom label so the
generating ? 'Analyzing...' : 'Run Triage' branch never appears; update
Recommendations.tsx to either stop using the ActionButton's loading prop and
pass generating to disabled instead (call ActionButton with
disabled={generating} and remove loading={generating}), or add a loadingLabel
prop to ActionButton and pass loadingLabel="Analyzing..." alongside
loading={generating}; modify the ActionButton component (ActionButton.tsx) to
accept and display loadingLabel when provided (fallback to its existing
"Working..." otherwise) so the UI shows "Analyzing..." when generate is running.
| <div className="space-y-2"> | ||
| {recs.map((rec) => ( | ||
| <div key={rec.id} className="bg-[#161822] rounded-lg border border-gray-800 p-4"> | ||
| <Card key={rec.id} urgency={rec.priority <= 2 ? 'amber' : rec.priority <= 3 ? 'green' : null}> |
Contributor
There was a problem hiding this comment.
P1 (highest priority) maps to 'amber' urgency, not 'red'.
rec.priority <= 2 ? 'amber' : rec.priority <= 3 ? 'green' : null gives amber to P1 and P2, green to P3, and no urgency to P4+. Typically the most urgent items (P1) warrant 'red'. Was this intentional?
Suggested mapping
- <Card key={rec.id} urgency={rec.priority <= 2 ? 'amber' : rec.priority <= 3 ? 'green' : null}>
+ <Card key={rec.id} urgency={rec.priority === 1 ? 'red' : rec.priority === 2 ? 'amber' : rec.priority === 3 ? 'green' : 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
| <Card key={rec.id} urgency={rec.priority <= 2 ? 'amber' : rec.priority <= 3 ? 'green' : null}> | |
| <Card key={rec.id} urgency={rec.priority === 1 ? 'red' : rec.priority === 2 ? 'amber' : rec.priority === 3 ? 'green' : null}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Recommendations.tsx` at line 129, The urgency mapping on the
Card component is inverted for P1; update the conditional in the Card urgency
prop (the JSX using Card key={rec.id} urgency={...}) so rec.priority === 1 maps
to 'red', rec.priority === 2 maps to 'amber', rec.priority === 3 maps to
'green', and any other priority yields null (e.g., urgency={rec.priority === 1 ?
'red' : rec.priority === 2 ? 'amber' : rec.priority === 3 ? 'green' : null}).
| useEffect(() => { | ||
| api.getSyncStatus().then(setSyncStatuses).catch((e) => setError(e.message)); | ||
| api.getBridgeStatus().then((r) => setServiceStatuses(r.services)).catch(() => {}); | ||
| api.getBridgeStatus().then((r) => setServiceStatuses(r.services)).catch((e) => console.error('[Settings] bridge status failed:', e)); |
Contributor
There was a problem hiding this comment.
Silent bridge-status failures leave users with stale/misleading fallback data
Both getBridgeStatus call sites swallow errors into console.error, so if the request fails the Service Connections card silently renders the hardcoded fallback (all services shown as "connected"). Consider at least setting a non-blocking warning or marking the section as unavailable.
Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Settings.tsx` at line 18, The getBridgeStatus call in
Settings.tsx currently swallows failures into console.error and leaves the
Service Connections card displaying hardcoded "connected" fallback; update the
async handling around api.getBridgeStatus in the Settings component to set an
explicit error/availability state (e.g., bridgeStatusError or
bridgeStatusUnavailable) when the promise rejects instead of only logging, and
use that state in the render path that uses setServiceStatuses to show a
non-blocking warning or mark the Service Connections section as unavailable/
degraded; ensure you update both call sites that invoke getBridgeStatus and the
component rendering logic that reads serviceStatuses so the UI reflects the
unavailable state rather than silently showing the fallback "connected" values.
| setBridgeSyncing(name); | ||
| try { | ||
| const result = await fn(); | ||
| setPlaidMessage(`${name}: ${JSON.stringify(result)}`); |
Contributor
There was a problem hiding this comment.
JSON.stringify(result) exposes raw API payloads in a user-facing message
The plaidMessage state is rendered directly in the UI (line 127). Serialising an arbitrary unknown API response leaks implementation details and produces unreadable output for non-trivial payloads.
🛠️ Proposed fix
- setPlaidMessage(`${name}: ${JSON.stringify(result)}`);
+ setPlaidMessage(`${name}: sync completed`);📝 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
| setPlaidMessage(`${name}: ${JSON.stringify(result)}`); | |
| setPlaidMessage(`${name}: sync completed`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Settings.tsx` at line 30, The UI currently sets user-facing
state with setPlaidMessage(`${name}: ${JSON.stringify(result)}`), which leaks
raw API payloads and may expose sensitive or unreadable data; change this to set
a concise, user-friendly message instead (e.g., `${name}: ${status}` or a short
summary) and move the full JSON payload into a non-user-facing sink such as
console.debug or a logger for debugging/telemetry; ensure any sensitive fields
in result are redacted before logging and update the code that renders
plaidMessage to expect the new concise string (referencing setPlaidMessage and
plaidMessage).
Comment on lines
150
to
170
| <div className="space-y-2"> | ||
| {files.map((entry, i) => ( | ||
| <div key={i} className="flex items-center justify-between py-2 border-b border-gray-800 last:border-0"> | ||
| <div key={i} className="flex items-center justify-between py-2 border-b border-card-border last:border-0"> | ||
| <div className="flex items-center gap-3 min-w-0"> | ||
| <StatusIcon status={entry.status} /> | ||
| <div className="min-w-0"> | ||
| <p className="text-white text-sm truncate">{entry.file.name}</p> | ||
| <p className="text-gray-500 text-xs">{formatSize(entry.file.size)} — {entry.file.type || 'unknown type'}</p> | ||
| <p className="text-card-text text-sm truncate">{entry.file.name}</p> | ||
| <p className="text-card-muted text-xs">{formatSize(entry.file.size)} -- {entry.file.type || 'unknown type'}</p> | ||
| </div> | ||
| </div> | ||
| <div className="flex items-center gap-2 shrink-0"> | ||
| {entry.status === 'skipped' && <span className="text-yellow-400 text-xs">Duplicate</span>} | ||
| {entry.error && entry.status === 'error' && <span className="text-red-400 text-xs max-w-48 truncate">{entry.error}</span>} | ||
| {entry.status === 'skipped' && <span className="text-urgency-amber text-xs">Duplicate</span>} | ||
| {entry.error && entry.status === 'error' && <span className="text-urgency-red text-xs max-w-48 truncate">{entry.error}</span>} | ||
| {(entry.status === 'pending' || entry.status === 'error') && ( | ||
| <button onClick={() => removeFile(i)} className="text-gray-500 hover:text-red-400 text-sm px-1"> | ||
| <button onClick={() => removeFile(i)} className="text-card-muted hover:text-urgency-red text-sm px-1"> | ||
| × | ||
| </button> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Using array index as key for a removable list.
Line 152 uses key={i} for file entries. When removeFile(i) deletes an item, subsequent indices shift, causing React to mismatch DOM nodes with state. This can result in stale UI (wrong status icon next to a file name, etc.).
Generate a stable ID when files are added instead.
Proposed approach
Add an id to FileEntry and generate it on add:
interface FileEntry {
+ id: string;
file: File;
status: 'pending' | 'uploading' | 'done' | 'skipped' | 'error';
error?: string;
}+let nextId = 0;
const addFiles = useCallback((newFiles: FileList | File[]) => {
- const entries: FileEntry[] = Array.from(newFiles).map((file) => ({ file, status: 'pending' }));
+ const entries: FileEntry[] = Array.from(newFiles).map((file) => ({ id: String(nextId++), file, status: 'pending' }));
setFiles((prev) => [...prev, ...entries]);Then use key={entry.id} and update removeFile to filter by id.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/Upload.tsx` around lines 150 - 170, The list uses array index as
React key (files.map with key={i}), which breaks when removeFile(i) shifts
indices; add a stable id to the FileEntry model when files are added (e.g., in
the function that constructs new FileEntry objects), use that id as the key
(key={entry.id}) in the files.map render, and change removeFile to accept an id
and filter files by entry.id instead of splicing by index so items retain stable
identity; update any types/interfaces for FileEntry and change call sites that
add/remove files accordingly.
- Replace parseFloat() with Number() in StatusBar cash display for consistency with the same commit's parseInt→Number changes - Replace array index key with entry.date in CashFlow outflows table (same anti-pattern fixed in FocusView) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…edge cases - Add Zod schemas for auth, cashflow scenario, bridge books/assets/scrape, and GET query params (obligations, disputes, recommendations) - Wire safeParse validation into every POST/PUT/GET handler - Refactor urgency scoring: UTC-consistent date comparison, grace period support, deferred status modifier, NaN/invalid date guards - Fix recalculate-urgency endpoint premature overdue marking - Fix mcp.ts type error (unknown → string cast for tool name) - Add vitest with 59 comprehensive urgency scoring tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/pages/CashFlow.tsx (1)
60-68:⚠️ Potential issue | 🟠 Major
runScenarioand regeneration errors are silently swallowed when projections exist.
setErroris called on failure in bothrunScenario(line 66) andhandleGenerate(line 44), but the only error display is gated onprojections.length === 0(line 83). Once projections are loaded, any subsequent error is invisible to the user — clicking "Run Scenario" or "Regenerate" can fail with zero feedback.🐛 Proposed fix: inline error display in the Scenario panel and on the Regenerate button
const runScenario = async () => { if (deferIds.size === 0) return; + setError(null); try { const result = await api.runCashflowScenario(Array.from(deferIds)); setScenario(result); } catch (e: unknown) { setError(e instanceof Error ? e.message : 'Scenario failed'); } };Then surface the error inline, e.g. below the ActionButton cluster in the Scenario panel:
<div className="flex items-center gap-3"> <ActionButton ... /> {deferIds.size > 0 && ( <button ...>Clear</button> )} </div> + {error && projections.length > 0 && ( + <p className="mt-2 text-urgency-red text-sm">{error}</p> + )}Also applies to: 36-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 60 - 68, The error state set by runScenario and handleGenerate is currently only shown when projections.length === 0, so failures become invisible once projections exist; update the CashFlow UI to render the error message unconditionally in the Scenario panel (e.g., below the ActionButton cluster) and also show it near the Regenerate button so users see failures regardless of projections. Locate the runScenario and handleGenerate functions (they call setError), ensure they clear previous errors on successful runs, and move/remove the projections.length === 0 gating around the existing error display logic so the component renders the error whenever error state is non-empty.src/routes/obligations.ts (1)
85-92:⚠️ Potential issue | 🟡 MinorMissing
isFiniteguard onlate_fee— inconsistent with line 157.The recalculate-urgency endpoint now guards against
NaNfromparseFloat(line 157), but this update handler readsexisting.late_feefrom the DB with onlyparseFloat— the same corruption risk applies here.Proposed fix
late_fee: existing.late_fee ? parseFloat(existing.late_fee) : null, + // should match the guard in recalculate-urgency: - late_fee: existing.late_fee ? parseFloat(existing.late_fee) : null, + late_fee: existing.late_fee ? (isFinite(parseFloat(existing.late_fee)) ? parseFloat(existing.late_fee) : null) : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/obligations.ts` around lines 85 - 92, The code that builds the computeUrgencyScore call reads existing.late_fee and uses parseFloat without guarding against NaN; update the late_fee expression (used when constructing the computeUrgencyScore call) to mirror the recalculate-urgency handler by parsing the value and returning null if parseFloat produces NaN — e.g., use Number.isFinite(parseFloat(existing.late_fee)) (or isFinite) to decide between the parsed number and null so computeUrgencyScore receives either a finite number or null.
♻️ Duplicate comments (1)
ui/src/pages/CashFlow.tsx (1)
70-72:⚠️ Potential issue | 🟠 MajorUnresolved: date label rolls back one day in US timezones.
new Date('YYYY-MM-DD')parses as UTC midnight; withouttimeZone: 'UTC'intoLocaleDateString, US locales display the previous calendar day. The same label is then used as the table rowkeyat line 158, so a misformatted date propagates to both the chart axis and the table.🐛 Proposed fix
const chartData = projections.map((p) => ({ - date: new Date(p.projection_date).toLocaleDateString('en-US', { month: 'short', day: 'numeric' }), + date: new Date(p.projection_date).toLocaleDateString('en-US', { + month: 'short', + day: 'numeric', + timeZone: 'UTC', + }), + projectionDate: p.projection_date, // preserve for stable key usage balance: parseFloat(p.projected_balance),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 70 - 72, The date labels created in chartData via projections.map (using new Date(p.projection_date).toLocaleDateString(...)) roll back one day in US timezones; update the formatting to force UTC (e.g., toLocaleDateString('en-US', { month: 'short', day: 'numeric', timeZone: 'UTC' })) or alternatively parse with an explicit UTC timestamp (e.g., append 'T00:00:00Z' to p.projection_date) so the chart axis matches the intended calendar date; also ensure the same normalized label or the raw p.projection_date is used as the table row key (the code that sets the key for the table row should reference the same formatted value or use p.projection_date) to avoid mismatched keys between the chart and table.
🧹 Nitpick comments (10)
package.json (1)
26-26: Add atestscript to expose vitest vianpm test.The PR adds 59 urgency-scoring tests and wires vitest as a devDependency, but no
test(ortest:unit) script is defined inscripts. Without it, running the test suite requires knowing to invokenpx vitestdirectly rather than the conventionalnpm test.Version
4.0.18is confirmed as the current latest on npm, so the pin is valid. Vitest 4.0 includes breaking changes relative to prior major versions, but since this is a fresh addition rather than an upgrade, that's not a concern here.🛠️ Proposed addition of a `test` script
"ui:dev": "cd ui && vite dev", "ui:build": "cd ui && vite build", - "ui:preview": "cd ui && vite preview" + "ui:preview": "cd ui && vite preview", + "test": "vitest run", + "test:watch": "vitest"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 26, package.json lacks a "test" script exposing vitest; add a scripts entry (e.g., "test": "vitest") so running npm test invokes the new vitest devDependency. Update the "scripts" object in package.json to include the "test" key (or "test:unit" if you prefer) referencing the installed "vitest" so contributors can run tests via npm test rather than npx vitest.ui/src/pages/CashFlow.tsx (3)
73-75:parseFloatstill used here while the PR standardised onNumber()elsewhere.
parseFloat("12.5abc")silently returns12.5;Number("12.5abc")returnsNaN, which is the correct signal for a malformed API value. The same applies to line 188 (parseFloat(ob.amount_due || ...)).♻️ Proposed refactor
- balance: parseFloat(p.projected_balance), - inflow: parseFloat(p.projected_inflow), - outflow: parseFloat(p.projected_outflow), + balance: Number(p.projected_balance), + inflow: Number(p.projected_inflow), + outflow: Number(p.projected_outflow),And on line 188:
- <span ...>{formatCurrency(parseFloat(ob.amount_due || ob.amount_minimum || '0'))}</span> + <span ...>{formatCurrency(Number(ob.amount_due ?? ob.amount_minimum ?? '0'))}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 73 - 75, Replace the parseFloat calls in CashFlow.tsx with Number so malformed strings yield NaN instead of silently parsing; specifically change the conversions for the object properties balance, inflow, and outflow (currently using parseFloat(p.projected_balance / projected_inflow / projected_outflow)) to use Number(p.projected_balance) etc., and likewise replace parseFloat(ob.amount_due || ...) with Number(ob.amount_due || ...) where ob.amount_due is processed; leave surrounding logic unchanged so invalid API values are surfaced as NaN.
157-158: UseprojectionDate(the raw ISO date string) as the table row key instead of the formatted label.Formatted strings like
"Feb 10"are not globally unique (they repeat across years) and won't survive a dataset that spans year boundaries. The rawprojection_datevalue is already unique and was noted as a "stable keys" fix target in this PR's commits.♻️ Proposed fix
Expose the raw date in
chartData(also useful for the timezone fix above):const chartData = projections.map((p) => ({ date: new Date(p.projection_date).toLocaleDateString(...), + projectionDate: p.projection_date, balance: ...,Then use it as the key:
- {chartData.filter((e) => e.outflow > 0).slice(0, 15).map((entry) => ( - <tr key={entry.date} ...> + {chartData.filter((e) => e.outflow > 0).slice(0, 15).map((entry) => ( + <tr key={entry.projectionDate} ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` around lines 157 - 158, The table row key uses the formatted label (entry.date) which is not globally unique across years; update the chartData shape to include the raw ISO date (named projection_date or projectionDate) and change the row key in the map inside CashFlow.tsx to use that raw value (e.g., key={entry.projectionDate} or key={entry.projection_date}) instead of entry.date so keys remain stable across year boundaries; ensure wherever chartData is assembled you expose projectionDate/projection_date so the map callback has access to it.
132-132: YAxis formatter places$before the sign for negative values ($-5k) and rounds values under$500to$0k.Both are cosmetic but can mislead on a cash-flow chart where negative balances and sub-$500 granularity matter.
♻️ Proposed fix
- tickFormatter={(v) => `$${(v / 1000).toFixed(0)}k`} + tickFormatter={(v) => { + const abs = Math.abs(v); + const formatted = abs >= 1000 ? `$${(abs / 1000).toFixed(0)}k` : `$${abs}`; + return v < 0 ? `-${formatted}` : formatted; + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/CashFlow.tsx` at line 132, The YAxis tickFormatter currently formats values like `$-5k` and collapses values under $500 to `$0k`; update the tickFormatter used on the YAxis so the minus sign appears before the dollar sign and small amounts keep dollar granularity instead of rounding to `0k`. Specifically, in the YAxis tickFormatter function referenced in the file (tickFormatter on YAxis) compute a sign prefix ( '-' for negative else '' ), use Math.abs(v) for magnitude, and if abs(value) < 1000 return a dollar-formatted string with dollars (e.g., "-$500" or "$250") otherwise format in thousands with `k` (e.g., "-$5k" or "$12k") so negatives show as "-$X" and sub-$1000 values are not rounded to `0k`.src/lib/urgency.test.ts (1)
316-338: Weak assertions in invalid-input tests won't catch scoring regressions.The invalid date and empty string tests (lines 317–328) only assert that the score is within
[0, 100]. Any score in that range would pass, including one where time-pressure was incorrectly applied. Both cases should fall through to category-weight-only, giving a deterministic score of15. Consider pinning to that value.♻️ Proposed fix
it('invalid date string returns 0 (fails gracefully)', () => { const score = computeUrgencyScore({ ...base, due_date: 'not-a-date' }); - // Should handle NaN gracefully, not crash - expect(score).toBeGreaterThanOrEqual(0); - expect(score).toBeLessThanOrEqual(100); + // NaN date → skip time pressure → category weight only (utility = 15) + expect(score).toBe(15); }); it('empty date string returns category weight only (no time pressure)', () => { const score = computeUrgencyScore({ ...base, due_date: '' }); - expect(score).toBeGreaterThanOrEqual(0); - expect(score).toBeLessThanOrEqual(100); + expect(score).toBe(15); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/urgency.test.ts` around lines 316 - 338, The tests for invalid date inputs use weak range checks; update the two cases in the invalid-inputs block so that computeUrgencyScore returns the deterministic category-weight-only value (15) instead of merely asserting 0–100. Specifically, change the assertions for computeUrgencyScore called with due_date: 'not-a-date' and due_date: '' to expect( score ).toBe(15), keeping the existing NaN and negative late_fee cases as-is; locate these checks around the computeUrgencyScore calls in the failing test block to make the replacements.src/lib/urgency.ts (1)
56-59:lateFeecan hold a negative value despite the "guard against NaN/negative" comment.
isFinite(-10)istrue, so a negativelate_feepasses through as-is rather than being normalised to0. The subsequent> 0/> 25/> 50checks happen to produce the correct result (no bonus added), but the variable itself carries a misleading value that diverges from the stated intent.♻️ Proposed fix
- const lateFee = obligation.late_fee != null && isFinite(obligation.late_fee) ? obligation.late_fee : 0; + const rawFee = obligation.late_fee; + const lateFee = rawFee != null && isFinite(rawFee) && rawFee > 0 ? rawFee : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/urgency.ts` around lines 56 - 59, The lateFee variable currently allows negative numbers because isFinite only filters NaN/infinite; change the normalization of obligation.late_fee so negative values are clamped to 0 (e.g., set lateFee = Math.max(0, parsedValue)) before the scoring logic that modifies score (references: lateFee, obligation.late_fee, score in the urgency computation); ensure the replacement still guards against non-finite values and falls back to 0 when invalid.src/lib/validators.ts (2)
171-181:dateStringvalidates format only — semantically invalid dates like2024-02-30pass.The regex
/^\d{4}-\d{2}-\d{2}$/confirms shape but not calendar validity. For query params this is low-risk (an out-of-range date will simply match nothing), but a.refine()check would catch client bugs earlier.♻️ Optional semantic validation
-const dateString = z.string().regex(/^\d{4}-\d{2}-\d{2}$/, 'Must be YYYY-MM-DD'); +const dateString = z + .string() + .regex(/^\d{4}-\d{2}-\d{2}$/, 'Must be YYYY-MM-DD') + .refine((s) => !isNaN(Date.parse(s)), 'Must be a valid calendar date');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/validators.ts` around lines 171 - 181, The dateString regex currently only enforces YYYY-MM-DD shape but allows invalid calendar dates; update the dateString declaration to add a semantic .refine() that parses the captured year/month/day (from the string) and constructs a Date (or uses Date.UTC) then verifies the resulting date matches the original year, month, and day and is not NaN, returning a clear error like "Invalid calendar date"; keep the existing regex and error text for format, then reference dateString in obligationCalendarQuerySchema so start/end retain validation.
183-188:disputeQuerySchemaandrecommendationQuerySchemaaccept unconstrained status strings, unlikeobligationQuerySchema.
obligationQuerySchema(lines 173–176) constrains status to a typed enum, returning a400for unrecognised values. The dispute and recommendation schemas accept any string up to 50 chars, so a typo likestatus=opennpasses validation and silently returns an empty dataset rather than a400. Adopting enums here makes the API behaviour consistent and self-documenting.♻️ Proposed fix
export const disputeQuerySchema = z.object({ - status: z.string().max(50).optional(), + status: z.enum(['open', 'resolved', 'dismissed']).optional(), }); export const recommendationQuerySchema = z.object({ - status: z.string().max(50).optional(), + status: z.enum(['active', 'completed', 'dismissed']).optional(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/validators.ts` around lines 183 - 188, The disputeQuerySchema and recommendationQuerySchema currently accept any status string; change them to reuse the same enum/type used by obligationQuerySchema so invalid statuses fail validation. Replace the z.string().max(50).optional() for status in disputeQuerySchema and recommendationQuerySchema with the same Zod enum/nativeEnum used by obligationQuerySchema (import/reuse the enum symbol used there) and keep .optional(); ensure the schemas return a 400 on unrecognised enum values by using the identical enum type definition as obligationQuerySchema.src/routes/obligations.ts (2)
160-163: HoisttodayUtcabove the loop — it's date-constant and re-created per iteration.
nowandtodayUtcdon't depend onoband are recomputed every iteration. Move them before theforloop for clarity and to guarantee a single consistent "today" for the entire batch.Proposed fix
+ const now = new Date(); + const todayUtc = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate())); for (const ob of obligations) { const score = computeUrgencyScore({ ... }); const dueDate = new Date(ob.due_date + 'T00:00:00Z'); - const now = new Date(); - const todayUtc = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate())); const newStatus = dueDate < todayUtc && ob.status === 'pending' ? 'overdue' : ob.status;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/obligations.ts` around lines 160 - 163, The calculation of now and todayUtc is done inside the per-obligation loop (they're recomputed each iteration); move the Date construction for now and the Date.UTC-based todayUtc out of the loop so they are computed once before iterating over obligations, then keep using todayUtc inside the loop when computing dueDate and newStatus (the expressions involving dueDate, ob.status and newStatus remain the same). This guarantees a single consistent "today" for all obligations and avoids repeated allocations.
157-157: Avoid doubleparseFloat— extract to a local.Proposed fix
- late_fee: ob.late_fee ? (isFinite(parseFloat(ob.late_fee)) ? parseFloat(ob.late_fee) : null) : null, + late_fee: (() => { const v = ob.late_fee ? parseFloat(ob.late_fee) : NaN; return isFinite(v) ? v : null; })(),Or more readably, extract a small helper used here and in the PATCH handler:
const safeLateFee = (val: unknown): number | null => { if (!val) return null; const n = parseFloat(String(val)); return isFinite(n) ? n : null; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/obligations.ts` at line 157, The late_fee mapping currently calls parseFloat twice for ob.late_fee; extract a small helper (e.g., safeLateFee) that takes an unknown, coerces to string, parses once, and returns a finite number or null, then replace the inline expression in the late_fee property and the PATCH handler to call safeLateFee(ob.late_fee) to avoid double parsing and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/bridge.ts`:
- Around line 654-656: The code currently falls back to a hardcoded court number
when parsing with courtDocketScrapeSchema and assigning targetCase, which leaks
private data and is incorrect; update the implementation so caseNumber is
required by making courtDocketScrapeSchema require caseNumber (preferred) or, if
it must remain optional, replace the hardcoded fallback with a configurable
source (e.g., process.env.DEFAULT_CASE_NUMBER) and validate that the env var
exists before proceeding, then set targetCase = parsed.data.caseNumber ??
process.env.DEFAULT_CASE_NUMBER and return a 400 error if neither is present;
adjust any tests or callers accordingly and remove the literal '2024D007847'
from the code.
In `@src/routes/mcp.ts`:
- Around line 111-112: The code unsafely asserts params?.name into toolName and
uses || for arguments; change it to explicitly validate toolName before calling
executeTool and return a JSON-RPC -32602 Invalid params error when
missing/empty, and replace the arguments fallback with nullish coalescing.
Specifically, in the route handler where params is read (inspect variables
toolName and args), do: set args = params?.arguments ?? {}; check if !toolName
(or typeof toolName !== "string" or toolName.trim() === "") and if so construct
and return the proper JSON-RPC error response (code -32602, message "Invalid
params") instead of calling executeTool(sql, toolName, args); otherwise call
executeTool as before.
In `@ui/src/pages/CashFlow.tsx`:
- Around line 124-130: The SVG gradient uses a fixed id "balanceGradient" which
collides across multiple renders; generate a unique id with React's useId (or a
similar unique-id helper) inside the CashFlow component (e.g., const gradientId
= useId()), replace the literal id on the <linearGradient> with that unique id,
and update any references to the gradient (the AreaChart/Area fill prop that
currently uses url(`#balanceGradient`)) to use url(#${gradientId}); also ensure
useId is imported from React and used where the Area/defs are rendered so each
component instance gets a distinct gradient id.
---
Outside diff comments:
In `@src/routes/obligations.ts`:
- Around line 85-92: The code that builds the computeUrgencyScore call reads
existing.late_fee and uses parseFloat without guarding against NaN; update the
late_fee expression (used when constructing the computeUrgencyScore call) to
mirror the recalculate-urgency handler by parsing the value and returning null
if parseFloat produces NaN — e.g., use
Number.isFinite(parseFloat(existing.late_fee)) (or isFinite) to decide between
the parsed number and null so computeUrgencyScore receives either a finite
number or null.
In `@ui/src/pages/CashFlow.tsx`:
- Around line 60-68: The error state set by runScenario and handleGenerate is
currently only shown when projections.length === 0, so failures become invisible
once projections exist; update the CashFlow UI to render the error message
unconditionally in the Scenario panel (e.g., below the ActionButton cluster) and
also show it near the Regenerate button so users see failures regardless of
projections. Locate the runScenario and handleGenerate functions (they call
setError), ensure they clear previous errors on successful runs, and move/remove
the projections.length === 0 gating around the existing error display logic so
the component renders the error whenever error state is non-empty.
---
Duplicate comments:
In `@ui/src/pages/CashFlow.tsx`:
- Around line 70-72: The date labels created in chartData via projections.map
(using new Date(p.projection_date).toLocaleDateString(...)) roll back one day in
US timezones; update the formatting to force UTC (e.g.,
toLocaleDateString('en-US', { month: 'short', day: 'numeric', timeZone: 'UTC'
})) or alternatively parse with an explicit UTC timestamp (e.g., append
'T00:00:00Z' to p.projection_date) so the chart axis matches the intended
calendar date; also ensure the same normalized label or the raw
p.projection_date is used as the table row key (the code that sets the key for
the table row should reference the same formatted value or use
p.projection_date) to avoid mismatched keys between the chart and table.
---
Nitpick comments:
In `@package.json`:
- Line 26: package.json lacks a "test" script exposing vitest; add a scripts
entry (e.g., "test": "vitest") so running npm test invokes the new vitest
devDependency. Update the "scripts" object in package.json to include the "test"
key (or "test:unit" if you prefer) referencing the installed "vitest" so
contributors can run tests via npm test rather than npx vitest.
In `@src/lib/urgency.test.ts`:
- Around line 316-338: The tests for invalid date inputs use weak range checks;
update the two cases in the invalid-inputs block so that computeUrgencyScore
returns the deterministic category-weight-only value (15) instead of merely
asserting 0–100. Specifically, change the assertions for computeUrgencyScore
called with due_date: 'not-a-date' and due_date: '' to expect( score ).toBe(15),
keeping the existing NaN and negative late_fee cases as-is; locate these checks
around the computeUrgencyScore calls in the failing test block to make the
replacements.
In `@src/lib/urgency.ts`:
- Around line 56-59: The lateFee variable currently allows negative numbers
because isFinite only filters NaN/infinite; change the normalization of
obligation.late_fee so negative values are clamped to 0 (e.g., set lateFee =
Math.max(0, parsedValue)) before the scoring logic that modifies score
(references: lateFee, obligation.late_fee, score in the urgency computation);
ensure the replacement still guards against non-finite values and falls back to
0 when invalid.
In `@src/lib/validators.ts`:
- Around line 171-181: The dateString regex currently only enforces YYYY-MM-DD
shape but allows invalid calendar dates; update the dateString declaration to
add a semantic .refine() that parses the captured year/month/day (from the
string) and constructs a Date (or uses Date.UTC) then verifies the resulting
date matches the original year, month, and day and is not NaN, returning a clear
error like "Invalid calendar date"; keep the existing regex and error text for
format, then reference dateString in obligationCalendarQuerySchema so start/end
retain validation.
- Around line 183-188: The disputeQuerySchema and recommendationQuerySchema
currently accept any status string; change them to reuse the same enum/type used
by obligationQuerySchema so invalid statuses fail validation. Replace the
z.string().max(50).optional() for status in disputeQuerySchema and
recommendationQuerySchema with the same Zod enum/nativeEnum used by
obligationQuerySchema (import/reuse the enum symbol used there) and keep
.optional(); ensure the schemas return a 400 on unrecognised enum values by
using the identical enum type definition as obligationQuerySchema.
In `@src/routes/obligations.ts`:
- Around line 160-163: The calculation of now and todayUtc is done inside the
per-obligation loop (they're recomputed each iteration); move the Date
construction for now and the Date.UTC-based todayUtc out of the loop so they are
computed once before iterating over obligations, then keep using todayUtc inside
the loop when computing dueDate and newStatus (the expressions involving
dueDate, ob.status and newStatus remain the same). This guarantees a single
consistent "today" for all obligations and avoids repeated allocations.
- Line 157: The late_fee mapping currently calls parseFloat twice for
ob.late_fee; extract a small helper (e.g., safeLateFee) that takes an unknown,
coerces to string, parses once, and returns a finite number or null, then
replace the inline expression in the late_fee property and the PATCH handler to
call safeLateFee(ob.late_fee) to avoid double parsing and improve readability.
In `@ui/src/pages/CashFlow.tsx`:
- Around line 73-75: Replace the parseFloat calls in CashFlow.tsx with Number so
malformed strings yield NaN instead of silently parsing; specifically change the
conversions for the object properties balance, inflow, and outflow (currently
using parseFloat(p.projected_balance / projected_inflow / projected_outflow)) to
use Number(p.projected_balance) etc., and likewise replace
parseFloat(ob.amount_due || ...) with Number(ob.amount_due || ...) where
ob.amount_due is processed; leave surrounding logic unchanged so invalid API
values are surfaced as NaN.
- Around line 157-158: The table row key uses the formatted label (entry.date)
which is not globally unique across years; update the chartData shape to include
the raw ISO date (named projection_date or projectionDate) and change the row
key in the map inside CashFlow.tsx to use that raw value (e.g.,
key={entry.projectionDate} or key={entry.projection_date}) instead of entry.date
so keys remain stable across year boundaries; ensure wherever chartData is
assembled you expose projectionDate/projection_date so the map callback has
access to it.
- Line 132: The YAxis tickFormatter currently formats values like `$-5k` and
collapses values under $500 to `$0k`; update the tickFormatter used on the YAxis
so the minus sign appears before the dollar sign and small amounts keep dollar
granularity instead of rounding to `0k`. Specifically, in the YAxis
tickFormatter function referenced in the file (tickFormatter on YAxis) compute a
sign prefix ( '-' for negative else '' ), use Math.abs(v) for magnitude, and if
abs(value) < 1000 return a dollar-formatted string with dollars (e.g., "-$500"
or "$250") otherwise format in thousands with `k` (e.g., "-$5k" or "$12k") so
negatives show as "-$X" and sub-$1000 values are not rounded to `0k`.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonsrc/lib/urgency.test.tssrc/lib/urgency.tssrc/lib/validators.tssrc/routes/auth.tssrc/routes/bridge.tssrc/routes/cashflow.tssrc/routes/disputes.tssrc/routes/mcp.tssrc/routes/obligations.tssrc/routes/recommendations.tsui/src/components/StatusBar.tsxui/src/pages/CashFlow.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/StatusBar.tsx
Comment on lines
+654
to
+656
| const parsed = courtDocketScrapeSchema.safeParse(await c.req.json()); | ||
| if (!parsed.success) return c.json({ error: 'Validation failed', issues: parsed.error.issues }, 400); | ||
| const targetCase = parsed.data.caseNumber || '2024D007847'; |
Contributor
There was a problem hiding this comment.
Hardcoded personal court case number as a fallback is a privacy leak and a correctness bug.
'2024D007847' appears to be a real personal Cook County case number. Two problems:
- Privacy: It's now permanently visible in a public PR diff.
- Correctness: Any deployment that calls this endpoint without a
caseNumberbody will silently scrape and persist data for this specific case into its own database — completely wrong for users other than the original author.
The safe fix is to make caseNumber required in the schema, or — if truly optional — read a default from an env var rather than embedding it in source.
🔒 Proposed fix
Option A — make caseNumber required (cleanest):
// src/lib/validators.ts
export const courtDocketScrapeSchema = z.object({
- caseNumber: z.string().max(50).optional(),
+ caseNumber: z.string().min(1).max(50),
}); // src/routes/bridge.ts
- const targetCase = parsed.data.caseNumber || '2024D007847';
+ const targetCase = parsed.data.caseNumber;Option B — read from env if omitted (keeps backward compat):
- const targetCase = parsed.data.caseNumber || '2024D007847';
+ const targetCase = parsed.data.caseNumber || c.env.DEFAULT_CASE_NUMBER;
+ if (!targetCase) return c.json({ error: 'caseNumber is required' }, 400);📝 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 parsed = courtDocketScrapeSchema.safeParse(await c.req.json()); | |
| if (!parsed.success) return c.json({ error: 'Validation failed', issues: parsed.error.issues }, 400); | |
| const targetCase = parsed.data.caseNumber || '2024D007847'; | |
| const parsed = courtDocketScrapeSchema.safeParse(await c.req.json()); | |
| if (!parsed.success) return c.json({ error: 'Validation failed', issues: parsed.error.issues }, 400); | |
| const targetCase = parsed.data.caseNumber || c.env.DEFAULT_CASE_NUMBER; | |
| if (!targetCase) return c.json({ error: 'caseNumber is required' }, 400); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/bridge.ts` around lines 654 - 656, The code currently falls back
to a hardcoded court number when parsing with courtDocketScrapeSchema and
assigning targetCase, which leaks private data and is incorrect; update the
implementation so caseNumber is required by making courtDocketScrapeSchema
require caseNumber (preferred) or, if it must remain optional, replace the
hardcoded fallback with a configurable source (e.g.,
process.env.DEFAULT_CASE_NUMBER) and validate that the env var exists before
proceeding, then set targetCase = parsed.data.caseNumber ??
process.env.DEFAULT_CASE_NUMBER and return a 400 error if neither is present;
adjust any tests or callers accordingly and remove the literal '2024D007847'
from the code.
Comment on lines
+111
to
+112
| const toolName = params?.name as string; | ||
| const args = (params?.arguments || {}) as Record<string, unknown>; |
Contributor
There was a problem hiding this comment.
toolName can be undefined at runtime; add an explicit guard before calling executeTool.
params?.name as string is an unsound type assertion — when params is undefined or lacks a name field, toolName is undefined at runtime. The code still calls executeTool(sql, undefined, {}), which throws "Unknown tool: undefined" and returns it as a tool isError content block. Per JSON-RPC 2.0 / MCP spec, a missing required param should yield a -32602 Invalid params error response, not a tool-error content block.
Additionally, || {} on line 112 will replace any falsy arguments value (e.g., an empty string) with {}; ?? is the correct nullish guard here.
🛡️ Proposed fix
- const toolName = params?.name as string;
- const args = (params?.arguments || {}) as Record<string, unknown>;
+ const toolName = params?.name;
+ if (typeof toolName !== 'string' || !toolName) {
+ return c.json({
+ jsonrpc: '2.0',
+ id,
+ error: { code: -32602, message: 'Invalid params: missing or non-string tool name' },
+ });
+ }
+ const args = (params?.arguments ?? {}) as Record<string, unknown>;📝 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 toolName = params?.name as string; | |
| const args = (params?.arguments || {}) as Record<string, unknown>; | |
| const toolName = params?.name; | |
| if (typeof toolName !== 'string' || !toolName) { | |
| return c.json({ | |
| jsonrpc: '2.0', | |
| id, | |
| error: { code: -32602, message: 'Invalid params: missing or non-string tool name' }, | |
| }); | |
| } | |
| const args = (params?.arguments ?? {}) as Record<string, unknown>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/mcp.ts` around lines 111 - 112, The code unsafely asserts
params?.name into toolName and uses || for arguments; change it to explicitly
validate toolName before calling executeTool and return a JSON-RPC -32602
Invalid params error when missing/empty, and replace the arguments fallback with
nullish coalescing. Specifically, in the route handler where params is read
(inspect variables toolName and args), do: set args = params?.arguments ?? {};
check if !toolName (or typeof toolName !== "string" or toolName.trim() === "")
and if so construct and return the proper JSON-RPC error response (code -32602,
message "Invalid params") instead of calling executeTool(sql, toolName, args);
otherwise call executeTool as before.
Comment on lines
+124
to
+130
| <AreaChart data={chartData} margin={{ top: 10, right: 10, left: 0, bottom: 0 }}> | ||
| <defs> | ||
| <linearGradient id="balanceGradient" x1="0" y1="0" x2="0" y2="1"> | ||
| <stop offset="5%" stopColor="var(--chitty-500)" stopOpacity={0.3} /> | ||
| <stop offset="95%" stopColor="var(--chitty-500)" stopOpacity={0.02} /> | ||
| </linearGradient> | ||
| </defs> |
Contributor
There was a problem hiding this comment.
Static SVG id="balanceGradient" is a global DOM identifier — scope it to this instance.
If this component is ever rendered more than once in the same document (tests, portals, future feature reuse), both <linearGradient> elements share the same ID and only the last one "wins", silently breaking the gradient fill in the earlier chart.
🐛 Proposed fix using `useId`
+ import { useId } from 'react';
...
export function CashFlow() {
+ const gradientId = `balanceGradient-${useId()}`;
...
- <linearGradient id="balanceGradient" x1="0" y1="0" x2="0" y2="1">
+ <linearGradient id={gradientId} x1="0" y1="0" x2="0" y2="1">
...
- <Area ... fill="url(`#balanceGradient`)" ... />
+ <Area ... fill={`url(#${gradientId})`} ... />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/CashFlow.tsx` around lines 124 - 130, The SVG gradient uses a
fixed id "balanceGradient" which collides across multiple renders; generate a
unique id with React's useId (or a similar unique-id helper) inside the CashFlow
component (e.g., const gradientId = useId()), replace the literal id on the
<linearGradient> with that unique id, and update any references to the gradient
(the AreaChart/Area fill prop that currently uses url(`#balanceGradient`)) to use
url(#${gradientId}); also ensure useId is imported from React and used where the
Area/defs are rendered so each component instance gets a distinct gradient id.
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
34 files changed: 3,402 additions, 881 deletions
Test plan
npm run ui:devand verify all pages render with new designnpm run ui:build— should pass with zero errors🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Documentation
Bug Fixes