Timesheet entry: the daily/entry surface — 5 specs green (21 of 40) - #52
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
…router The entry grid reads any staff member's day as CostLine-shaped lines (it patches them through the cost-line endpoints), so the projection reuses job_service.cost_line_data plus a job-identity overlay per line; the queryset is the one list_entries already had, extracted as day_time_lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
All staff including departed (the admin screen and wage-loading checks read them) — deliberately not get_displayable_staff, which answers who can record time on a date. Create/patch/icon stay with the staff slice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
useAutosaveField moves as-is; useDraftRows lifts the phantom invariant, deferred row-exit commit, in-flight guard and failure badge out of CostLineGrid, parameterised by draft type and commit policy so the timesheet grid can share them. CostLineGrid.test.tsx is untouched and green — the extraction is behaviour-preserving by that net. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Popover + hand-rolled list rather than cmdk: the keyboard contract is a maintained highlight with Enter/Tab committing it, which fights cmdk's internal selection model rather than reusing it. Trigger auto-opens on focus for an empty row (the create flow's focus handoff lands there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
The useCostLines discipline rebound to the entries envelope. Pricing is server-owned (any meta/labour_subtype PATCH on a created_from_timesheet line reprices through the one rate pipeline), so the echo merge applies pricing outputs unconditionally; created lines are enriched with the picked job's identity because the job router's echo carries none. restoreDeletedRow extracted to features/shared for both hooks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
CostLineGrid's sibling: the job is a per-row editable field (the picker), hours humanise on commit (2h / 3h 30m), the urgent/bill precedence ports v1's setJob exactly, and pricing edits ship meta so the server rate pipeline reprices. TableMeta's global augmentation namespaces per grid (costGrid / timesheetGrid) so both tables can type their own context. shadcn select added for the rate/billRate/labourType columns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Entry page: parallel reference queries (the performance spec measures
exactly this), loud unknown-staff error, weekend-skipping date nav per
company defaults, live Daily Breakdown tiles; Current Jobs cards and
the modals are deferred with seams — no spec asserts them. Daily page:
read-only summary with the StaffRow-name-{staffId} contract the specs
extract the entry staffId from.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vvecreate-timesheet-entry (8 cases), keyboard-nav, urgent-job-defaults, performance, staff-wage-loading, plus shared support helpers, timesheet API fixtures and getPhantomRowIndex. One backend-contract fix surfaced: a blank create description now ships as null (desc_not_blank, ADR 0040). Port deviation: serial-first-test job creation via the standard fixture replaces v1's hand-rolled beforeAll logins. tests/e2e joins the no-console lint exemption (the performance spec's output IS its report). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Also records the pre-existing cost-line write-auth asymmetry as an open decision (management reads are superuser-gated; the writes the entry grid reuses are plain-authenticated with a caller-chosen staff UUID). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
…errors The wage select now changes only the wage multiplier — v1's mirror was unreachable once a job was picked (job pick writes the bill key, which its override check counted as explicit), so mirroring here clobbered the urgent 1.5x default and could flip a shop job billable (a 422). Also: an errored background refetch keeps the working entry page instead of unmounting the grid (destroying drafts), the draft discard button locks in flight, hour totals humanise (no float-tail renders), and garbage URL dates fail loudly instead of flowing through as NaN. Two inherited shapes recorded in the backlog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
'—' strips to Number('') === 0, so the pre-data render both false-passed
the zero assertion and false-failed the greater-than-zero one under
full-suite load.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7VveCo-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
The full-suite gate caught a rapid type-then-Tab under load arriving with the highlight reset: the popover stayed open and no job committed. With exactly one match there is no ambiguity, so the fallback cannot misfire; multi-match with no highlight still refuses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Warning Review limit reached
Next review available in:36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds superuser staff and management-timesheet APIs, typed frontend contracts, shared draft and autosave behavior, editable timesheet pages, authenticated routes, and backend, unit, component, and end-to-end tests. ChangesTimesheet management slice
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
…own) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
frontend/src/features/shell/AppNavbar.tsx (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
data-automation-idto the Timesheets link.The Create Job link at Line 36 carries
AppNavbar-create-job. This link has no automation id, so specs must select it by visible text. Text selectors break on a label change.♻️ Proposed refactor
- <Link to="/timesheets/daily" className="text-sm text-gray-700 hover:text-gray-900">+ <Link+ to="/timesheets/daily"+ data-automation-id="AppNavbar-timesheets"+ className="text-sm text-gray-700 hover:text-gray-900"+ >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/shell/AppNavbar.tsx` around lines 42 - 44, Add a stable data-automation-id attribute to the Timesheets Link in AppNavbar, using the existing naming convention and a unique identifier such as AppNavbar-timesheets. Keep the current destination, styling, and visible label unchanged.frontend/src/features/timesheet/SmartTimesheetTable.tsx (2)
717-725: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe phantom focus effect queries the whole document.
document.querySelectorAllmatches picker triggers in any mountedSmartTimesheetTable. If a second grid ever mounts (for example a split staff view), focus lands in the wrong grid. A ref on the rootdivscopes the query to this instance. The same applies tofocusAutomationIdat Line 194.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/SmartTimesheetTable.tsx` around lines 717 - 725, Scope both picker-trigger lookups to the current SmartTimesheetTable instance instead of the global document. Add or reuse a ref on the root div, query through that ref in the focusPhantomToken effect and in focusAutomationId, and preserve the existing trigger-selection and focus behavior within that table.
116-122: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a job lookup map instead of a linear scan per cell.
jobForRowrunscontext.jobs.find(...)for every server row. Five cell components call it (JobPickerCell,CompanyCell,JobNameCell,LabourTypeCell,BillRateCell), so each render costs O(rows × jobs × 5). AMapbuilt once in the meta object removes the scan.♻️ Proposed refactor
interface TimesheetCellContext { jobs: readonly TimesheetJobOut[] + jobsById: ReadonlyMap<string, TimesheetJobOut> payItems: readonly XeroPayItemOut[]function jobForRow( context: TimesheetCellContext, gridRow: TimesheetGridRow, ): TimesheetJobOut | null { if (gridRow.type === 'draft') return gridRow.draft.job - return context.jobs.find((job) => job.id === gridRow.line.job_id) ?? null+ return context.jobsById.get(gridRow.line.job_id) ?? null }Build the map with
useMemonext torowsand pass it inmeta.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/SmartTimesheetTable.tsx` around lines 116 - 122, Refactor jobForRow and the surrounding table setup to use a memoized Map of jobs keyed by job.id, built alongside rows and passed through the meta object. Replace the context.jobs.find lookup for non-draft rows with a Map lookup while preserving draft-row handling and the null fallback; update all cell callers to use the new meta-provided map.frontend/src/features/timesheet/TimesheetEntryPage.tsx (1)
114-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
SmartTimesheetTablePropsinstead of deriving fromParameters.
SmartTimesheetTable.tsxLine 49 exportsSmartTimesheetTableProps.SmartTimesheetTableProps['jobs']reads more directly thanParameters<typeof SmartTimesheetTable>[0]['jobs']and survives a signature change to the component.As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/TimesheetEntryPage.tsx` around lines 114 - 115, Update the types around the timesheet entry props to import and use the exported SmartTimesheetTableProps from SmartTimesheetTable.tsx, replacing the Parameters<typeof SmartTimesheetTable>[0] derivations with SmartTimesheetTableProps['jobs'] and SmartTimesheetTableProps['payItems']; leave the underlying component behavior unchanged.Source: Coding guidelines
frontend/src/features/timesheet/DailyOverviewPage.tsx (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the query options object for the invalidation key.
getDailyTimesheetSummaryByDateOptions({ path: { target_date: date } })is built twice for the same query. Hoist it into a variable and read.queryKeyfrom it. That keeps the key and the query provably identical.♻️ Proposed refactor
- const summaryQuery = useQuery(- getDailyTimesheetSummaryByDateOptions({ path: { target_date: date } }),- )+ const summaryOptions = getDailyTimesheetSummaryByDateOptions({ path: { target_date: date } })+ const summaryQuery = useQuery(summaryOptions)- void queryClient.invalidateQueries({- queryKey: getDailyTimesheetSummaryByDateOptions({ path: { target_date: date } })- .queryKey,- })+ void queryClient.invalidateQueries({ queryKey: summaryOptions.queryKey })Also applies to: 71-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/DailyOverviewPage.tsx` around lines 30 - 32, Hoist the result of getDailyTimesheetSummaryByDateOptions({ path: { target_date: date } }) into a shared query-options variable in the DailyOverviewPage query flow, pass that variable to useQuery, and use its queryKey for invalidation instead of constructing the options a second time.frontend/tests/e2e/staff/staff-wage-loading.spec.ts (1)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or complete the port-history comments.
These comments narrate the port and prior review context. They do not state the factual constraint that rejects the alternative setup. Keep a comment only when it records both the rejected alternative and that constraint.
frontend/tests/e2e/staff/staff-wage-loading.spec.ts#L11-L18: remove the port-history text or state why the standard fixture is required.frontend/tests/e2e/timesheet/create-timesheet-entry.spec.ts#L7-L17: remove the port-history text or state the constraint that rejects the alternative setup.frontend/tests/e2e/timesheet/keyboard-nav.spec.ts#L9-L18: remove the port-history text or state the constraint that rejects the alternative setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/tests/e2e/staff/staff-wage-loading.spec.ts` around lines 11 - 18, Remove the port-history and prior-review narration from the comments in frontend/tests/e2e/staff/staff-wage-loading.spec.ts lines 11-18, frontend/tests/e2e/timesheet/create-timesheet-entry.spec.ts lines 7-17, and frontend/tests/e2e/timesheet/keyboard-nav.spec.ts lines 9-18, or replace it with the concrete constraint that requires the standard fixture/setup; retain only factual behavioral or setup requirements.Source: Coding guidelines
frontend/src/features/timesheet/timesheetDraft.test.ts (1)
95-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the test name.
The test picks
urgentJob, but the name says "a normal repick". Rename it to match the case under test, for examplean explicit override survives an urgent repick.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/timesheetDraft.test.ts` around lines 95 - 103, Rename the test case around applyJobPick to describe the urgentJob scenario, changing “a normal repick” to “an urgent repick” while leaving the test behavior unchanged.frontend/src/features/timesheet/TimesheetJobPicker.tsx (1)
210-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the highlighted option to assistive technology.
The list uses plain
divelements. The highlight is visual only. A screen-reader user hears no active option while the arrow keys move the highlight. Keyboard operation still works, so this is not a blocker.Add
role="listbox"on the container,role="option"plusaria-selectedon each row, stable option ids, andaria-activedescendantplusaria-controlson the search input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/TimesheetJobPicker.tsx` around lines 210 - 250, Update the TimesheetJobPicker list and keyboard controls to expose the highlighted job to assistive technology: add listbox semantics to the results container, option roles with aria-selected and stable ids to each mapped job row, and connect the search input using aria-controls and aria-activedescendant that tracks the highlighted option.frontend/src/features/timesheet/useTimesheetEntries.test.tsx (1)
199-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the toast, or drop it from the test name.
The name says "and toasts", but the test only checks the rollback. Add an assertion on the
sonnertoast, or rename the test to describe the rollback only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/timesheet/useTimesheetEntries.test.tsx` around lines 199 - 210, Update the test named “a failed patch rolls back only its own fields and toasts” to assert the expected sonner toast after the failed patch, preserving its rollback assertion; alternatively, rename the test to describe rollback only if toast behavior is intentionally out of scope.frontend/src/lib/dates.ts (1)
10-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject out-of-range date components.
parseLocalaccepts any numeric components.Datenormalises them, so2026-13-45becomes2027-02-14without an error. The date arrives from a user-editable URL search param, so a silently shifted date is possible. Validate the round-trip instead.♻️ Proposed refactor
) { throw new Error(`Not a YYYY-MM-DD date: ${isoDate}`) } - return new Date(year, month - 1, day)+ const date = new Date(year, month - 1, day)+ // Date normalises out-of-range parts (month 13 → next January), so the+ // round-trip is the only check that rejects them.+ if (date.getFullYear() !== year || date.getMonth() !== month - 1 || date.getDate() !== day) {+ throw new Error(`Not a YYYY-MM-DD date: ${isoDate}`)+ }+ return date }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/dates.ts` around lines 10 - 25, Update parseLocal to reject out-of-range date components by validating the constructed Date round-trips to the original year, month, and day. Throw the existing invalid-date error when the normalized Date values do not match the parsed components, while preserving current handling of non-numeric input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/code-quality.md`:
- Around line 101-103: Update the optional-return metric in
docs/rewrite-status.md to match the measured report: 154 functions returning X |
None out of 1,712 non-test functions. Change only the corresponding
planning/status figures and preserve the document’s existing formatting.
In `@docs/rewrite-status.md`:
- Around line 1080-1087: When the draft job is changed in the job-repick flow,
clear its labour_subtype before the updated draft is rendered so rateForSubtype
cannot use a stale value. Add a regression test covering selection of two jobs
with different labour-rate sets and verify the repicked draft has no labour
subtype.
- Around line 171-184: Enforce authorization for every cost-line write: update
the create operation job_jobs_cost_sets_actual_cost_lines_create and the
cost-line PATCH and DELETE handlers to require office/superuser access or verify
ownership of the referenced staff member before mutating data. Add authorization
tests covering denied and permitted create, PATCH, and DELETE requests.
In `@docs/superpowers/plans/2026-08-10-timesheet-entry.md`:
- Around line 41-166: Run Ruff formatting on the affected plan document to apply
its required formatting changes, then verify the working tree contains only the
formatter’s updates and the CI formatting check passes.
- Around line 386-398: Add the `text` language identifier to the opening fenced
code block containing the timesheet behavior checklist, while leaving its
contents and closing fence unchanged.
In `@docs/superpowers/specs/2026-08-10-timesheet-entry-design.md`:
- Around line 3-6: Correct the future-dated status records: in
docs/superpowers/specs/2026-08-10-timesheet-entry-design.md lines 3-6, replace
the future approval and completed green-result wording with the actual approval
date or planned status; in docs/rewrite-status.md lines 16-22, replace the
future last-updated and landed-slice wording with the actual update date or
planned status.
In `@frontend/src/features/timesheet/DailyOverviewPage.tsx`:
- Around line 124-137: The staff-name trigger in the DailyOverviewPage row
currently uses a clickable div, so replace that div with a button that preserves
the existing onOpen behavior, automation identifier, and visual styling while
presenting the name as text. Ensure the button retains the flex layout and
cursor appearance without adding a separate key handler, and keep its child
initials, name, and entry-count content unchanged.
In `@frontend/src/features/timesheet/hours.ts`:
- Around line 14-36: Update parseHoursInput to recognize the humanized Nh Mm,
Nh, and Mm forms emitted by formatHoursDisplay before the generic numeric
parsing path, converting hours and minutes to a decimal hour value so displayed
values such as “3h 30m” round-trip without losing minutes. Preserve the existing
fraction parsing, validation, fallback, and capping behavior.
In `@frontend/src/features/timesheet/SmartTimesheetTable.test.tsx`:
- Around line 240-265: Update the test case around the existing user instance so
pickJob receives user instead of creating a second userEvent.setup() instance.
Keep all subsequent keyboard interactions and focus assertions unchanged.
In `@frontend/src/features/timesheet/TimesheetEntryPage.tsx`:
- Around line 288-291: The billableFlag reader duplicates lineIsBillable and can
diverge from the grid logic. Move the shared is_billable reader and true
fallback into one exported utility in features/timesheet/, then update
billableFlag and SmartTimesheetTable’s lineIsBillable usage to import that
implementation and remove the duplicate definitions.
In `@frontend/src/features/timesheet/TimesheetJobPicker.test.tsx`:
- Around line 132-135: Update the “never opens when disabled” test to focus the
picker trigger after rendering with disabled enabled, then assert the search
input is absent. Keep the existing disabled assertion and target the trigger and
search-input helpers already used by the test suite.
In `@frontend/src/features/timesheet/TimesheetJobPicker.tsx`:
- Around line 46-49: Update jobDisplayRate to handle an empty job.labour_rates
array by returning the picker’s existing placeholder value for that row before
calling rateForSubtype; preserve rateForSubtype’s throwing behavior for
non-empty rates with genuinely unknown subtypes.
- Around line 170-174: Update the picker state around the trigger’s onFocus
handler and onCloseAutoFocus to track Escape or outside-click dismissal, and
skip auto-opening once when focus is restored to an empty enabled row. Clear the
dismissal marker on normal create-flow focus or subsequent intentional focus so
the existing auto-open behavior remains intact.
In `@frontend/src/features/timesheet/useTimesheetEntries.ts`:
- Around line 118-127: Update createLine to call cancelInFlight() before
createMutation.mutate, matching patchLine and deleteLine, so in-flight reads
cannot overwrite the newly inserted row before the settled invalidation refetch.
In `@frontend/src/routes/_authed/timesheets/daily.tsx`:
- Around line 6-8: Add a shared YYYY-MM-DD date validator in
frontend/src/lib/dates.ts that returns the input only for matching values and
undefined otherwise. Update validateSearch in
frontend/src/routes/_authed/timesheets/daily.tsx (lines 6-8) and
frontend/src/routes/_authed/timesheets/entry.tsx (lines 6-9) to use it for
search.date, preserving the existing staffId handling in entry.tsx so invalid
dates fall back to localIsoDate().
In `@frontend/tests/e2e/timesheet/performance.spec.ts`:
- Around line 83-84: Update the request-timing probe around page.goto and the
.smart-timesheet-table visibility wait to also wait for all .animate-spin
elements to disappear before classifying requests, matching the first probe’s
loading-completion behavior.
In `@frontend/tests/e2e/timesheet/urgent-job-defaults.spec.ts`:
- Around line 111-115: Update the response validation around responseBody so the
test fails when meta is absent or malformed instead of skipping assertions.
Assert that responseBody and responseBody.meta are records before checking
wage_rate_multiplier and bill_rate_multiplier, preserving the expected values of
1.0 and 1.5.
---
Nitpick comments:
In `@frontend/src/features/shell/AppNavbar.tsx`:
- Around line 42-44: Add a stable data-automation-id attribute to the Timesheets
Link in AppNavbar, using the existing naming convention and a unique identifier
such as AppNavbar-timesheets. Keep the current destination, styling, and visible
label unchanged.
In `@frontend/src/features/timesheet/DailyOverviewPage.tsx`:
- Around line 30-32: Hoist the result of getDailyTimesheetSummaryByDateOptions({
path: { target_date: date } }) into a shared query-options variable in the
DailyOverviewPage query flow, pass that variable to useQuery, and use its
queryKey for invalidation instead of constructing the options a second time.
In `@frontend/src/features/timesheet/SmartTimesheetTable.tsx`:
- Around line 717-725: Scope both picker-trigger lookups to the current
SmartTimesheetTable instance instead of the global document. Add or reuse a ref
on the root div, query through that ref in the focusPhantomToken effect and in
focusAutomationId, and preserve the existing trigger-selection and focus
behavior within that table.
- Around line 116-122: Refactor jobForRow and the surrounding table setup to use
a memoized Map of jobs keyed by job.id, built alongside rows and passed through
the meta object. Replace the context.jobs.find lookup for non-draft rows with a
Map lookup while preserving draft-row handling and the null fallback; update all
cell callers to use the new meta-provided map.
In `@frontend/src/features/timesheet/timesheetDraft.test.ts`:
- Around line 95-103: Rename the test case around applyJobPick to describe the
urgentJob scenario, changing “a normal repick” to “an urgent repick” while
leaving the test behavior unchanged.
In `@frontend/src/features/timesheet/TimesheetEntryPage.tsx`:
- Around line 114-115: Update the types around the timesheet entry props to
import and use the exported SmartTimesheetTableProps from
SmartTimesheetTable.tsx, replacing the Parameters<typeof SmartTimesheetTable>[0]
derivations with SmartTimesheetTableProps['jobs'] and
SmartTimesheetTableProps['payItems']; leave the underlying component behavior
unchanged.
In `@frontend/src/features/timesheet/TimesheetJobPicker.tsx`:
- Around line 210-250: Update the TimesheetJobPicker list and keyboard controls
to expose the highlighted job to assistive technology: add listbox semantics to
the results container, option roles with aria-selected and stable ids to each
mapped job row, and connect the search input using aria-controls and
aria-activedescendant that tracks the highlighted option.
In `@frontend/src/features/timesheet/useTimesheetEntries.test.tsx`:
- Around line 199-210: Update the test named “a failed patch rolls back only its
own fields and toasts” to assert the expected sonner toast after the failed
patch, preserving its rollback assertion; alternatively, rename the test to
describe rollback only if toast behavior is intentionally out of scope.
In `@frontend/src/lib/dates.ts`:
- Around line 10-25: Update parseLocal to reject out-of-range date components by
validating the constructed Date round-trips to the original year, month, and
day. Throw the existing invalid-date error when the normalized Date values do
not match the parsed components, while preserving current handling of
non-numeric input.
In `@frontend/tests/e2e/staff/staff-wage-loading.spec.ts`:
- Around line 11-18: Remove the port-history and prior-review narration from the
comments in frontend/tests/e2e/staff/staff-wage-loading.spec.ts lines 11-18,
frontend/tests/e2e/timesheet/create-timesheet-entry.spec.ts lines 7-17, and
frontend/tests/e2e/timesheet/keyboard-nav.spec.ts lines 9-18, or replace it with
the concrete constraint that requires the standard fixture/setup; retain only
factual behavioral or setup requirements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03579f8a-e6cb-422e-aa4f-1a0b34bafa10
⛔ Files ignored due to path filters (5)
frontend/src/api/generated/@tanstack/react-query.gen.tsis excluded by!**/generated/**frontend/src/api/generated/index.tsis excluded by!**/generated/**frontend/src/api/generated/sdk.gen.tsis excluded by!**/generated/**frontend/src/api/generated/types.gen.tsis excluded by!**/generated/**frontend/src/api/generated/zod.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (52)
apps/accounts/api.pyapps/accounts/schemas.pyapps/accounts/staff_directory.pyapps/accounts/tests/test_staff_list_api.pyapps/timesheet/api.pyapps/timesheet/schemas.pyapps/timesheet/services/workshop_timesheet_service.pyapps/timesheet/tests/test_timesheet_entries_api.pydocs/code-quality.mddocs/rewrite-status.mddocs/superpowers/plans/2026-08-10-timesheet-entry.mddocs/superpowers/specs/2026-08-10-timesheet-entry-design.mdfrontend/.oxlintrc.jsonfrontend/schema.v2.ymlfrontend/src/api/index.tsfrontend/src/components/ui/select.tsxfrontend/src/features/job/costing/CostLineGrid.tsxfrontend/src/features/job/costing/useCostLines.tsfrontend/src/features/shared/optimistic.tsfrontend/src/features/shared/useAutosaveField.tsfrontend/src/features/shared/useDraftRows.test.tsxfrontend/src/features/shared/useDraftRows.tsfrontend/src/features/shell/AppNavbar.tsxfrontend/src/features/timesheet/DailyOverviewPage.test.tsxfrontend/src/features/timesheet/DailyOverviewPage.tsxfrontend/src/features/timesheet/SmartTimesheetTable.test.tsxfrontend/src/features/timesheet/SmartTimesheetTable.tsxfrontend/src/features/timesheet/TimesheetEntryPage.test.tsxfrontend/src/features/timesheet/TimesheetEntryPage.tsxfrontend/src/features/timesheet/TimesheetJobPicker.test.tsxfrontend/src/features/timesheet/TimesheetJobPicker.tsxfrontend/src/features/timesheet/hours.test.tsfrontend/src/features/timesheet/hours.tsfrontend/src/features/timesheet/index.tsfrontend/src/features/timesheet/labourRates.tsfrontend/src/features/timesheet/timesheetDraft.test.tsfrontend/src/features/timesheet/timesheetDraft.tsfrontend/src/features/timesheet/useTimesheetEntries.test.tsxfrontend/src/features/timesheet/useTimesheetEntries.tsfrontend/src/lib/dates.test.tsfrontend/src/lib/dates.tsfrontend/src/routeTree.gen.tsfrontend/src/routes/_authed/timesheets/daily.tsxfrontend/src/routes/_authed/timesheets/entry.tsxfrontend/tests/e2e/fixtures/api.tsfrontend/tests/e2e/helpers.tsfrontend/tests/e2e/staff/staff-wage-loading.spec.tsfrontend/tests/e2e/timesheet/create-timesheet-entry.spec.tsfrontend/tests/e2e/timesheet/keyboard-nav.spec.tsfrontend/tests/e2e/timesheet/performance.spec.tsfrontend/tests/e2e/timesheet/support.tsfrontend/tests/e2e/timesheet/urgent-job-defaults.spec.ts
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+171
to
+184
| 0. **Cost-line write auth is looser than the timesheet reads (found in the | ||
| timesheet-entry slice review; predates it).** The management reads | ||
| (`/api/timesheets/*`, `/api/job/timesheet/entries/`, `/api/accounts/staff/`) | ||
| are superuser-only because they expose wage data — but the write path the | ||
| entry grid (and the cost-entry slice before it) uses is plain | ||
| authenticated: `job_jobs_cost_sets_actual_cost_lines_create` accepts an | ||
| arbitrary `staff` UUID with no ownership check, and cost-line PATCH/DELETE | ||
| are likewise open, so any authenticated staff member can attribute, edit | ||
| or delete a colleague's time line — bypassing the ownership rule the | ||
| self-service workshop endpoints enforce. `job_jobs_cost_sets_retrieve` | ||
| also serves every time line's wage-loaded `unit_cost` to any staff. | ||
| Tightening mid-week risks the workshop flows, so nothing changed in the | ||
| slice; your call whether cost-line writes gate on office/superuser (or | ||
| ownership) before or after cutover. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce authorization on every cost-line write.
Any authenticated user can create, modify, or delete another staff member’s time line. This bypasses the superuser protection on wage-bearing timesheet reads.
Apply an office/superuser or ownership check to create, PATCH, and DELETE before release. Add authorization tests for each operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rewrite-status.md` around lines 171 - 184, Enforce authorization for
every cost-line write: update the create operation
job_jobs_cost_sets_actual_cost_lines_create and the cost-line PATCH and DELETE
handlers to require office/superuser access or verify ownership of the
referenced staff member before mutating data. Add authorization tests covering
denied and permitted create, PATCH, and DELETE requests.
OwnerAuthor
There was a problem hiding this comment.
Deliberately not changed in this PR, and now recorded as open decision 0 in docs/rewrite-status.md: the asymmetry predates this slice (the cost-entry slice already shipped on these write endpoints), and tightening cost-line write auth five days before cutover risks the workshop flows that legitimately use them. The decision — office/superuser gate vs ownership check, before or after cutover — is called out for the maintainer with your exact failure scenario.
There was a problem hiding this comment.
@corrin, acknowledged. The finding remains valid, but Open decision 0 now records the risk, the affected operations, and the required authorization-model decision.
I will leave this review finding unresolved because the write authorization has not changed. Would you like me to open a follow-up GitHub issue for the office/superuser-versus-ownership decision and its implementation after cutover?
🐇
You are interacting with an AI system.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Picker: pointer opens belong to the Radix toggle (focus auto-open is
keyboard/handoff-only — the old shape relied on a close-refocus-reopen
flicker and made Escape reopen the row); stale labour_subtype clears on
job repick (a rate the new job lacks would throw in the bill cell);
parseHoursInput round-trips the humanised display ('3h 30m' no longer
silently drops the minutes); one lineMeta module feeds both the grid
and the page tiles; create insert cancels in-flight reads like its
siblings; routes validate the date search param shape (garbage falls
back to today); the daily staff name is a real button; test/format
nits; the optional-returns figure in rewrite-status now points at the
generated report instead of restating it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7VveUh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
The timesheet-entry slice: the daily overview and entry pages, a sibling grid over the draft machinery extracted from the cost grid, and two read ops. Full E2E gate green on this branch (67/67, writes live); 1731 unit tests; coverage 88.56%.
Backend (two read ops)
Frontend
Review findings applied (2-subagent adversarial pass)
Specs & environment
Five specs ported (`timesheet/` ×4 + `staff/staff-wage-loading`) with two full-suite races fixed en route: the Actuals chip's `'—'` placeholder parsing as 0, and a type-then-Tab arriving with the picker highlight reset (sole-match fallback). New environment prerequisite recorded in rewrite-status: the E2E user must be a superuser (management surface); set in the dev DB.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J334H8wZj3ccTqHZQh7Vve
Summary by CodeRabbit