Skip to content

chore: address CodeRabbit findings from the v0.2.14 release review (#201) #205

@w7-mgfcode

Description

@w7-mgfcode

Context

CodeRabbit's review of release PR #201 (devmain, v0.2.14) surfaced 35 comments in the review body (no inline threads). Triage: 0 release-blockers#201 proceeded. 3 comments were dismissed (version bumps on .release-please-manifest.json / CHANGELOG.md / pyproject.toml — release-please owns those). The remaining 32 valid findings are robustness / a11y / test-hygiene improvements on already-merged code, tracked here for a follow-up fix/ PR into dev.

CodeRabbit review: #201 (review)

Themes

1. Unstable pagination — add a deterministic tie-breaker sort key (🟠 Major ×3)

  • app/features/dimensions/service.py:104-113 + 229-238 — append Store.code / Product.sku as secondary sort.
  • app/features/jobs/service.py:256-267 — append Job.created_at, Job.job_id.
  • app/features/registry/service.py:317-327 — append ModelRun.run_id.

2. Test isolation (🟠×2 🟡×2 🧹×2)

  • app/features/analytics/tests/conftest.py:128-136 — teardown wipes whole InventorySnapshotDaily / SalesDaily / Calendar; scope to TEST-% rows.
  • app/features/dimensions/tests/conftest.py:69-72 — teardown wipes whole SalesDaily; scope to test rows.
  • app/features/analytics/tests/conftest.py:157, app/features/jobs/tests/conftest.py:74, app/features/dimensions/tests/conftest.py:98 — replace app.dependency_overrides.clear() with pop(get_db, None).
  • app/features/config/tests/test_service.py:103-109 — restore mutated Settings singleton in a finally block.

3. URL query-param validation — reject NaN / unknown enum values (🟠×3 🟡×2)

  • frontend/src/pages/explorer/sales.tsx:24-29dimension, store_id, product_id.
  • frontend/src/pages/explorer/runs.tsx:93-111page, status, model_type, sort_by.
  • frontend/src/pages/explorer/stores.tsx:63-84page, region, store_type, sort_by.
  • frontend/src/pages/explorer/jobs.tsx:45-51 and products.tsx:65-71 — clamp page to a positive integer.

4. Unhandled promise rejection on job-cancel (🟠 + ⚠️)

  • frontend/src/pages/explorer/jobs.tsx:66-68 and job-detail.tsx:76-81 — wrap cancelJob.mutateAsync in try/catch.

5. Dynamic Tailwind height dropped at build time (🟠 Major ×2)

  • frontend/src/components/charts/revenue-bar-chart.tsx:44, time-series-chart.tsx:73 (and backtest-folds-chart.tsx) — h-[${height}px] is not statically discoverable; use an inline style instead.

6. Keyboard accessibility for clickable rows / sort headers (🟠 Major ×2)

  • frontend/src/components/data-table/data-table.tsx:115-117 — add tabIndex / role / onKeyDown to clickable rows.
  • frontend/src/pages/visualize/demand.tsx:83-86 + 304-307 — make sort headers and rows keyboard-operable.

7. Demand-planner logic (🟠 Major ×2)

  • frontend/src/lib/demand-utils.ts:46 — reorder qty uses Math.round; use Math.ceil to avoid under-ordering.
  • frontend/src/lib/demand-utils.ts:89-91 + 105 — inventory Map keeps last-in-array entry; keep the latest snapshot by date.

8. RFC 7807 error envelope (🟠 Major)

  • app/features/analytics/routes.py:333-335get_timeseries propagates raw HTTPException from validate_date_range; return application/problem+json via app/core/problem_details.py.

9. CSV formula injection (🟠 Major, security)

  • frontend/src/lib/csv-export.ts:8-13 — neutralize values starting with = / + / - / @ in quoteField.

Misc small (🟡 / 🧹)

  • app/features/analytics/routes.py:302-313 + 381-388 — add ge=1 to store_id / product_id query params.
  • frontend/src/components/data-table/data-table.tsx:101-107 — skeleton rows + empty-state colSpan should use getVisibleLeafColumns().
  • frontend/src/pages/guide.tsx:192-201 — show an "unavailable" state instead of a permanent skeleton when config errors.
  • frontend/src/pages/visualize/forecast.tsx:51-54 + 192-193Array.isArray guard before .some() on result.forecasts.
  • frontend/src/pages/visualize/backtest.tsx:88-104 — enforce n_splits >= 2 / test_size >= 1 before enabling Run.
  • app/features/seeder/service.py:324-326 and scripts/run_demo.py:416-418 — anchor date windows to a single shared default_seed_end_date() value.
  • frontend/src/hooks/index.ts — re-export use-inventory from the hooks barrel.

Acceptance

  • A fix/ PR into dev addresses the 9 themes (one commit per theme), or splits them across a few PRs.
  • Validation gates green: ruff / mypy / pyright / pytest; pnpm tsc --noEmit && pnpm lint && pnpm test.

Metadata

Metadata

Assignees

No one assigned

    Labels

    choreNon-functional / hygiene change (no version bump)

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions