Purchasing: PO create/list/detail, stock page (29/40 specs) - #65
Conversation
PoCreatePage (CompanyLookup + reference + save -> 201 redirect), PoListPage, PoDetailPage with PoSummaryCard (created-by input, reference autosave, status select), usePoLines over the single PATCH endpoint, and the missing 'po' concurrency invalidator so 412/428 recovery actually refetches. Ports createTestPurchaseOrder + waitForPoAutosave and the po-created-by spec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
PoLinesTable as the third grid on the useReactTable + useDraftRows pattern (phantom row, no add-line button, row-exit draft commit — unit-cost stays the row's last focusable cell so the spec's Tab exits the row). ItemSelect generalised for stock-only consumers (optional jobId/line, label + wrapper overrides; labour-rates query gated on jobId presence only, since textOnly labels need rate names). Inline JobSelect over purchasing_all_jobs_retrieve — the unfiltered endpoint v1's PO page uses, because fresh jobs are draft and the filtered sibling excludes them. Spec ported with the autosave waiter armed before the pick/status clicks (v2 saves immediately; v1's debounce is what made arm-after work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Spec-lean StockPage: full active-stock list on load, 300ms-debounced server FTS from 3 characters, enabled-gated so clearing the box renders the cached list with no /search/ request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
…ports The purchasing slice's PoLinesTable was about to become a third hand-rolled grid emitting the DataTable-row-N/data-grid-* contract inline. features/shared/DataTable.tsx is now the one owner of that contract; SmartTimesheetTable, CostLineGrid and PoLinesTable all render through it. Auditing further found the same pathology in the plain list pages: PoListPage and StockPage (both new this slice) had copied CompaniesListPage's table shell and loading/error/retry block verbatim, taking an existing duplicate (also in WipReportPage) from 2 instances to 4. features/shared/ListTable.tsx is the one owner of that block instead -- deliberately separate from DataTable, since it has no react-table dependency and forcing static lists through column-def machinery would be indirection, not rigor. A hand-rolled debounce-into-query-state pattern in CompaniesListPage and StockPage is now features/shared/useDebouncedValue.ts (KanbanSearchInput keeps its own URL-driven debounce -- a different concept). ItemSelect, the decimal helpers and the Save-failed badge move to features/shared/ because purchasing consumed them cross-domain -- a domain feature is not a library. ADR 0039 strengthened: unification is never deferred, shared concepts get shared homes, the bar is reference quality. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Adversarial review on the branch found the ListTable audit had stopped short: JobMovementReportPage and CompanyDetailPage hand-rolled the same loading/error/retry block ListTable was built to own (6 real instances, only 4 fixed). Split the block itself out as features/shared/QueryState.tsx -- the pending/error gate alone, no table -- so it fits pages that show something other than a table too. ListTable now composes QueryState instead of duplicating it. PoDetailPage, CostLineGrid, JobMovementReportPage and CompanyDetailPage all render through it. Fixed: PoLinesTable's item-picker label read `item_code ?? 'Select Item'` with no description fallback, so a bound stock item with a null code (nullable, v1 parity) misread as unbound. Now poLineItemLabel() in lines.ts, unit-tested. Moved features/company to features/shared/company: it was already cross-imported by features/job (JobCreatePage, JobSettingsTab) before this slice added a third importer (purchasing/PoSummaryCard) -- it never had a route of its own, a shared widget library in a domain-shaped box. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Reversed 2026-08-11. The 2026-08-10 record said SSE ships with the production-serving decision, filed under "Post-cutover -- decided, deliberately NOT before 15 August." That was overturned: racing bad architecture into production defeats the point of the rewrite (non-negotiable #3 in the Cutover section), and the interim polling shape plus the un-runnable apps/xero/sync_stream.py view are exactly that. Slice 3 -- live updates done properly (serving model fix + SSE ticker + discard the interim shortcuts) moves to its own MUST-tier section with a milestone checkbox, and every stale "deferred post-cutover" cross-reference in the file is corrected. The purchasing-slice PR is unaffected -- it is unrelated and merges as planned; this correction only fixes the durable record for the next session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
QueryState grew optional loadingNode/errorNode override props so a spinner-based caller keeps its visual shell instead of losing it to the plain-text default. JobFinishTab, DailyOverviewPage, and both gates in TimesheetEntryPage's EntryWorkspace now render through it. XeroQuoteCard, JobInvoiceCard and JobSettingsTab's pay-item field stay excluded -- richer branching than binary success/fail, not the page-level gate QueryState owns. TimesheetEntryPage's own outer gate stays as guard-clause `if` returns, matching CLAUDE.md's stated preference rather than converting to a shape that would abandon it. Also: rewrite-status.md's narration of "review round found X, we fixed Y" replaced with what the outcomes actually are -- which shared component owns which contract, which sites are deliberately excluded and why, which constraints a future change must respect. And the Cutover section now states the actual two-question go/no-go criteria (functional parity, proxied by MUST-tier E2E; materially better architecture, judged directly) rather than only the E2E proxy, with the honest fallback (abort and stay on v1) stated explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Warning Review limit reached
Next review available in:12 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 ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds authenticated purchasing pages for stock and purchase orders, including editable lines, autosave, optimistic updates, and E2E coverage. It also extracts shared query, table, company, decimal, and debounce components and updates architecture and cutover documentation. ChangesPurchasing frontend
Shared frontend consolidation
Architecture and cutover documentation
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 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (11)
docs/cutover-checklist.md (1)
115-123: 🩺 Stability & Availability | 🔵 TrivialDefine the ASGI acceptance criteria.
or the ASGI equivalentis not an executable serving requirement. Define the minimum concurrency, timeout, keep-alive, database-connection, and ORM-safety requirements. Link this checkbox to the deployment configuration that enforces them.🤖 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/cutover-checklist.md` around lines 115 - 123, Update the serving-model checklist item to define executable ASGI acceptance criteria: minimum concurrency, request timeout, keep-alive, database-connection capacity, and ORM-safety requirements. Link the checkbox to the deployment configuration or command that enforces these settings, while retaining the existing WSGI option and cutover context.frontend/src/features/purchasing/StockPage.tsx (2)
25-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSend the trimmed query.
searchActiveis computed fromquery.trim(), but the request sends the untrimmedquery. For an input of" abc"the gate opens andqis" abc". Each whitespace variation also produces a distinct query key, which adds duplicate cache entries and duplicate requests for the same search term.♻️ Proposed fix
- const searchActive = query.trim().length >= MIN_QUERY_LENGTH+ const trimmedQuery = query.trim()+ const searchActive = trimmedQuery.length >= MIN_QUERY_LENGTH const list = useQuery(purchasingStockListOptions()) const search = useQuery({ - ...purchasingStockSearchRetrieveOptions({ query: { q: query } }),+ ...purchasingStockSearchRetrieveOptions({ query: { q: trimmedQuery } }), enabled: searchActive, })🤖 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/purchasing/StockPage.tsx` around lines 25 - 31, Update the search query construction near searchActive in StockPage so the request and its query key use the trimmed query value, while preserving the existing minimum-length gating behavior.
27-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreserve the previous search results while fetching a new query.
Each active search term creates a new query key.
search.isPendingthen replaces the table withLoading stock items.... UseplaceholderData: keepPreviousDatato keep the current rows visible during the fetch.♻️ Proposed refactor
-import { useQuery } from '`@tanstack/react-query`'+import { keepPreviousData, useQuery } from '`@tanstack/react-query`'const search = useQuery({ ...purchasingStockSearchRetrieveOptions({ query: { q: query } }), enabled: searchActive, + placeholderData: keepPreviousData, })🤖 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/purchasing/StockPage.tsx` around lines 27 - 33, Update the useQuery configuration in StockPage around purchasingStockSearchRetrieveOptions to set placeholderData to keepPreviousData, preserving existing search rows while a new query is fetching and avoiding the pending loading state replacing the table.frontend/tests/e2e/helpers.ts (1)
444-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
Math.randomwith a collision-free identifier.
Math.floor(Math.random() * 100000)gives 100,000 possible values. Parallel Playwright workers and repeated local runs can produce the same supplier name. A duplicate[TEST] Supplier Ncreates a second live Xero contact push with the same name, which makes later lookups ambiguous.Use
crypto.randomUUID()or combine the timestamp with the worker index.♻️ Proposed change
- const randomSuffix = Math.floor(Math.random() * 100000)- const supplierName = `[TEST] Supplier ${randomSuffix}`+ const randomSuffix = crypto.randomUUID().slice(0, 8)+ const supplierName = `[TEST] Supplier ${randomSuffix}`🤖 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/helpers.ts` around lines 444 - 446, Update createTestPurchaseOrder so supplierName uses a collision-free identifier instead of Math.random(), preferably crypto.randomUUID() or a timestamp combined with the Playwright worker index, while preserving the existing “[TEST] Supplier” naming format.frontend/tests/e2e/purchasing/stock-search.spec.ts (1)
117-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe counter filter can miss a regression.
The listener ignores every
/search/response whose URL containsq=5mm. The exclusion exists to skip the first search, but it also hides the most likely regression: clearing the input re-fires the sameq=5mmrequest. That request is a real post-clear search call, and the assertion at Line 134 still passes.Gate on time instead of on the query value.
♻️ Proposed change
- // Track whether any further /search/ request fires when we clear the box.+ // Gate on the clear action, not on the query value: a re-fired `q=5mm`+ // request after clearing is still a post-clear search call. let postClearSearchCalls = 0 + let cleared = false page.on('response', (response) => { - if (- response.url().includes('/api/purchasing/stock/search/') &&- !response.url().includes('q=5mm')- ) {+ if (cleared && response.url().includes('/api/purchasing/stock/search/')) { postClearSearchCalls += 1 } }) + cleared = true await input.fill('')🤖 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/purchasing/stock-search.spec.ts` around lines 117 - 134, Update the response listener in the stock-search test to count search responses based on whether they occur after the input is cleared, rather than excluding URLs containing q=5mm. Start tracking post-clear responses only immediately before input.fill(''), so the initial search is ignored while any repeated q=5mm request after clearing is counted and fails the existing assertion.frontend/src/features/shared/ListTable.tsx (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
renderRowmust supply the React key.Line 66 calls
rows.map(renderRow)and applies no key. Every current caller setskeyon its own<tr>. That contract is not stated on the prop, so a future caller can omit the key and cause a React key warning plus incorrect row reconciliation.♻️ Proposed doc addition
+ /** Must set a stable React `key` on the returned row element; ListTable+ maps rows directly and applies no key of its own. */ renderRow: (row: TRow) => ReactNodeAlso applies to: 66-66
🤖 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/shared/ListTable.tsx` at line 21, Document the `renderRow` prop contract in `ListTable`: implementations must return each row with its own stable React `key`, since the `rows.map(renderRow)` call does not apply one. Add this requirement to the prop’s existing documentation without changing the rendering logic.frontend/src/features/shared/company/PersonSelectionModal.test.tsx (1)
19-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the create and update flows.
The single test covers accessibility only.
handleCreateandhandleUpdatehold the business rules: first person is always primary, blank optional fields are omitted from the request body, and an invalid email blocks submission. These rules are unit-testable with mocked mutations.🤖 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/shared/company/PersonSelectionModal.test.tsx` around lines 19 - 48, Extend the PersonSelectionModal tests beyond accessibility to cover handleCreate and handleUpdate with mocked mutations: verify the first person is always primary, blank optional fields are omitted from the request payload, and invalid email prevents submission. Assert the appropriate mutation callbacks and payloads for both create and update flows.frontend/src/features/shared/company/PersonSelectionModal.tsx (3)
294-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe person card is a clickable
div.The card carries
onClickbut norole,tabIndex, or key handler. Keyboard users reach the person through the hover-revealedSelectbutton, whichgroup-focus-withinexposes, so the flow is not blocked. Removing the card-levelonClickand relying on the explicitSelectbutton would remove the duplicated activation path and theevent.stopPropagation()calls at Lines 341, 354, and 367.🤖 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/shared/company/PersonSelectionModal.tsx` around lines 294 - 303, The person card in the person-selection rendering should no longer be clickable via its container. Remove the card-level onClick handler from the div around person.person_id, rely on the explicit Select button for activation, and remove the associated event.stopPropagation() calls in the button handlers.
218-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
invalidatePeoplehere.Lines 218-220 repeat the query-key invalidation that
invalidatePeopledefines at Lines 118-121.handleUpdateandhandleConfirmDeletealready call the helper. A future key change would need two edits.As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."♻️ Proposed fix
- await queryClient.invalidateQueries({- queryKey: companiesPeopleListQueryKey({ path: { company_id: companyId } }),- })+ await invalidatePeople()🤖 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/shared/company/PersonSelectionModal.tsx` around lines 218 - 220, Replace the direct companies-people query invalidation in the relevant update flow with the existing invalidatePeople helper, matching handleUpdate and handleConfirmDelete. Reuse that helper’s established arguments and remove the duplicated query-key invalidation.Source: Coding guidelines
534-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle the returned promise on the submit button.
handleUpdateandhandleCreateareasync. Passing them directly toonClickreturns a floating promise from the event handler. The delete button at Line 272 already wraps its call withvoid. Use the same form here for consistency and to satisfy@typescript-eslint/no-misused-promisesif that rule is enabled.♻️ Proposed fix
- onClick={editingPerson ? handleUpdate : handleCreate}+ onClick={() => {+ void (editingPerson ? handleUpdate() : handleCreate())+ }}🤖 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/shared/company/PersonSelectionModal.tsx` around lines 534 - 542, Update the submit button’s onClick handler to explicitly discard the promise returned by handleUpdate or handleCreate, matching the existing delete-button pattern. Preserve the editingPerson conditional selection and button behavior.frontend/src/features/shared/useDebouncedValue.test.tsx (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the fake-timer queue on unmount.
clearTimeoutis global, so another cleanup can satisfytoHaveBeenCalled(). Since this suite enables fake timers, assert thatvi.getTimerCount()is greater than zero afterrenderHook, then zero afterunmount().🤖 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/shared/useDebouncedValue.test.tsx` around lines 39 - 46, Update the “clears a pending timer on unmount” test to assert the fake-timer queue directly: verify vi.getTimerCount() is greater than zero after renderHook creates the debounced timer, then verify it is zero after unmount(). Remove the clearTimeout spy assertion.
🤖 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/rewrite-status.md`:
- Around line 57-61: Update the frontend rebuild table in the rewrite-status
document to replace references to features/company/PersonSelectionModal.tsx and
features/company/CompanyLookup.tsx with their canonical features/shared/company/
paths, leaving other entries unchanged.
In `@frontend/src/features/crm/CompaniesListPage.tsx`:
- Around line 113-119: Apply the first-load-only error rule at both QueryState
call sites: in frontend/src/features/crm/CompaniesListPage.tsx lines 113-119,
change the ListTable isError value to companies.isError && companies.data ===
undefined; in frontend/src/features/job/JobFinishTab.tsx lines 209-213, use a
loadError derived per query from isError && data === undefined so cached summary
and checklist content remains visible during failed refetches.
In `@frontend/src/features/purchasing/JobSelect.tsx`:
- Around line 113-150: Update JobSelect to support keyboard navigation by
tracking an active option index, moving it with ArrowUp/ArrowDown, and selecting
the active job on Enter. Add role="listbox" to the dropdown and role="option"
with aria-selected to each mapped job row, making options keyboard-accessible
and reusing the shared picker pattern if available.
- Around line 91-111: Update the closing paths in JobSelect’s onBlur timeout and
Escape handler to reset editing and clear the stale search term when the picker
closes without selection, so value returns to the bound job and filtering does
not reuse old input. Store the 150ms blur timeout in a ref and add a useEffect
cleanup that clears it on unmount, while preserving the existing select
behavior.
In `@frontend/src/features/purchasing/PoListPage.tsx`:
- Around line 57-74: Update the purchase-order row in PoListPage’s renderRow
callback to be keyboard accessible: make the tr focusable, expose it as an
interactive control, and handle keyboard activation by navigating to the same
po/$poId destination as onClick. Preserve the existing mouse navigation
behavior.
In `@frontend/src/features/shared/company/CompanyLookup.tsx`:
- Around line 238-242: Update the suggestions empty-state rendering near
companiesSearchRetrieveOptions to check search.isError first and display an
error state when the request fails. Gate the existing “No companies found”
message on a successful, non-error search so rejected requests never appear as
empty results.
- Around line 88-99: Associate quickCreateCompany with an active request token
and call handleCompanyCreated only when the response still matches the current
lookup interaction; otherwise ignore the completion. In
frontend/src/features/shared/company/CompanyLookup.tsx lines 88-99, update
quickCreateCompany accordingly. In
frontend/src/features/shared/company/CreateCompanyModal.tsx lines 49-64, either
prevent dismissal while creation is pending or ignore completion after the modal
closes. Add regression tests covering abandoned quick-create and modal-creation
interactions.
In `@frontend/src/features/shared/company/PersonSelectionModal.tsx`:
- Around line 242-281: Update the form footer submit state in the component’s
submitDisabled logic to also disable submission whenever deleteTarget is
non-null or removeLink.isPending is true. Keep the existing confirmation overlay
behavior and ensure create/edit actions cannot start while deletion is pending
or awaiting confirmation.
In `@frontend/src/features/shared/decimal.ts`:
- Around line 10-16: Update parseDecimalInput to validate cleaned input against
an explicit fixed-point decimal syntax before numeric finiteness checks,
rejecting hex, octal, binary, and other non-decimal forms. Normalize accepted
values such as “.5”, “+5”, and “5.” into the backend-supported decimal
representation, and ensure trimDecimal cannot return invalid syntax verbatim.
In `@frontend/src/features/shared/ItemSelect.tsx`:
- Around line 19-43: Rewrite or remove the comments at
frontend/src/features/shared/ItemSelect.tsx:19-43, replacing implementation,
E2E, and history narration with the rejected API alternative and factual
constraint, or remove them. Apply the same constraint-based approach at
frontend/src/features/shared/SaveFailedBadge.tsx:1-4,
frontend/src/features/shared/decimal.test.ts:33-34 (including the rejected
formatting behavior and exponent constraint),
frontend/src/features/shared/company/CompanyLookup.test.tsx:154-155 (including
the Sonner rendering constraint if retained),
frontend/src/features/shared/company/CompanyLookup.tsx:88-90, and
frontend/src/features/shared/company/CreateCompanyModal.tsx:16-26 and :39
(document only rejected alternatives and current product/state constraints,
otherwise remove the comments).
---
Nitpick comments:
In `@docs/cutover-checklist.md`:
- Around line 115-123: Update the serving-model checklist item to define
executable ASGI acceptance criteria: minimum concurrency, request timeout,
keep-alive, database-connection capacity, and ORM-safety requirements. Link the
checkbox to the deployment configuration or command that enforces these
settings, while retaining the existing WSGI option and cutover context.
In `@frontend/src/features/purchasing/StockPage.tsx`:
- Around line 25-31: Update the search query construction near searchActive in
StockPage so the request and its query key use the trimmed query value, while
preserving the existing minimum-length gating behavior.
- Around line 27-33: Update the useQuery configuration in StockPage around
purchasingStockSearchRetrieveOptions to set placeholderData to keepPreviousData,
preserving existing search rows while a new query is fetching and avoiding the
pending loading state replacing the table.
In `@frontend/src/features/shared/company/PersonSelectionModal.test.tsx`:
- Around line 19-48: Extend the PersonSelectionModal tests beyond accessibility
to cover handleCreate and handleUpdate with mocked mutations: verify the first
person is always primary, blank optional fields are omitted from the request
payload, and invalid email prevents submission. Assert the appropriate mutation
callbacks and payloads for both create and update flows.
In `@frontend/src/features/shared/company/PersonSelectionModal.tsx`:
- Around line 294-303: The person card in the person-selection rendering should
no longer be clickable via its container. Remove the card-level onClick handler
from the div around person.person_id, rely on the explicit Select button for
activation, and remove the associated event.stopPropagation() calls in the
button handlers.
- Around line 218-220: Replace the direct companies-people query invalidation in
the relevant update flow with the existing invalidatePeople helper, matching
handleUpdate and handleConfirmDelete. Reuse that helper’s established arguments
and remove the duplicated query-key invalidation.
- Around line 534-542: Update the submit button’s onClick handler to explicitly
discard the promise returned by handleUpdate or handleCreate, matching the
existing delete-button pattern. Preserve the editingPerson conditional selection
and button behavior.
In `@frontend/src/features/shared/ListTable.tsx`:
- Line 21: Document the `renderRow` prop contract in `ListTable`:
implementations must return each row with its own stable React `key`, since the
`rows.map(renderRow)` call does not apply one. Add this requirement to the
prop’s existing documentation without changing the rendering logic.
In `@frontend/src/features/shared/useDebouncedValue.test.tsx`:
- Around line 39-46: Update the “clears a pending timer on unmount” test to
assert the fake-timer queue directly: verify vi.getTimerCount() is greater than
zero after renderHook creates the debounced timer, then verify it is zero after
unmount(). Remove the clearTimeout spy assertion.
In `@frontend/tests/e2e/helpers.ts`:
- Around line 444-446: Update createTestPurchaseOrder so supplierName uses a
collision-free identifier instead of Math.random(), preferably
crypto.randomUUID() or a timestamp combined with the Playwright worker index,
while preserving the existing “[TEST] Supplier” naming format.
In `@frontend/tests/e2e/purchasing/stock-search.spec.ts`:
- Around line 117-134: Update the response listener in the stock-search test to
count search responses based on whether they occur after the input is cleared,
rather than excluding URLs containing q=5mm. Start tracking post-clear responses
only immediately before input.fill(''), so the initial search is ignored while
any repeated q=5mm request after clearing is counted and fails the existing
assertion.
🪄 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: 66ca2b61-202f-4ca9-a415-b773f6995a0a
📒 Files selected for processing (60)
docs/adr/0039-one-implementation-per-concept.mddocs/code-quality.mddocs/cutover-checklist.mddocs/rewrite-status.mdfrontend/src/api/index.tsfrontend/src/api/query-client.tsfrontend/src/features/crm/CompaniesListPage.tsxfrontend/src/features/crm/CompanyDetailPage.tsxfrontend/src/features/job/JobCreatePage.tsxfrontend/src/features/job/JobFinishTab.tsxfrontend/src/features/job/JobSettingsTab.tsxfrontend/src/features/job/costing/CostLineGrid.tsxfrontend/src/features/job/costing/calc.test.tsfrontend/src/features/job/costing/calc.tsfrontend/src/features/purchasing/JobSelect.test.tsfrontend/src/features/purchasing/JobSelect.tsxfrontend/src/features/purchasing/PoCreatePage.tsxfrontend/src/features/purchasing/PoDetailPage.tsxfrontend/src/features/purchasing/PoLinesTable.tsxfrontend/src/features/purchasing/PoListPage.tsxfrontend/src/features/purchasing/PoSummaryCard.tsxfrontend/src/features/purchasing/StockPage.tsxfrontend/src/features/purchasing/index.tsfrontend/src/features/purchasing/lines.test.tsfrontend/src/features/purchasing/lines.tsfrontend/src/features/purchasing/usePoLines.tsfrontend/src/features/reports/JobMovementReportPage.tsxfrontend/src/features/reports/WipReportPage.tsxfrontend/src/features/shared/DataTable.tsxfrontend/src/features/shared/ItemSelect.tsxfrontend/src/features/shared/ListTable.test.tsxfrontend/src/features/shared/ListTable.tsxfrontend/src/features/shared/QueryState.test.tsxfrontend/src/features/shared/QueryState.tsxfrontend/src/features/shared/SaveFailedBadge.tsxfrontend/src/features/shared/company/CompanyLookup.test.tsxfrontend/src/features/shared/company/CompanyLookup.tsxfrontend/src/features/shared/company/CreateCompanyModal.tsxfrontend/src/features/shared/company/PersonSelectionModal.test.tsxfrontend/src/features/shared/company/PersonSelectionModal.tsxfrontend/src/features/shared/company/PersonSelector.tsxfrontend/src/features/shared/company/create-company.tsfrontend/src/features/shared/company/index.tsfrontend/src/features/shared/company/xero-contact.tsfrontend/src/features/shared/decimal.test.tsfrontend/src/features/shared/decimal.tsfrontend/src/features/shared/useDebouncedValue.test.tsxfrontend/src/features/shared/useDebouncedValue.tsfrontend/src/features/timesheet/DailyOverviewPage.tsxfrontend/src/features/timesheet/SmartTimesheetTable.tsxfrontend/src/features/timesheet/TimesheetEntryPage.tsxfrontend/src/routeTree.gen.tsfrontend/src/routes/_authed/purchasing/po/$poId.tsxfrontend/src/routes/_authed/purchasing/po/create.tsxfrontend/src/routes/_authed/purchasing/po/index.tsxfrontend/src/routes/_authed/purchasing/stock.tsxfrontend/tests/e2e/helpers.tsfrontend/tests/e2e/purchasing/create-purchase-order.spec.tsfrontend/tests/e2e/purchasing/po-created-by.spec.tsfrontend/tests/e2e/purchasing/stock-search.spec.ts
💤 Files with no reviewable changes (2)
- frontend/src/features/job/costing/calc.ts
- frontend/src/features/job/costing/calc.test.ts
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.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🧹 Nitpick comments (11)
docs/cutover-checklist.md (1)
115-123: 🩺 Stability & Availability | 🔵 TrivialDefine the ASGI acceptance criteria.
or the ASGI equivalentis not an executable serving requirement. Define the minimum concurrency, timeout, keep-alive, database-connection, and ORM-safety requirements. Link this checkbox to the deployment configuration that enforces them.🤖 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/cutover-checklist.md` around lines 115 - 123, Update the serving-model checklist item to define executable ASGI acceptance criteria: minimum concurrency, request timeout, keep-alive, database-connection capacity, and ORM-safety requirements. Link the checkbox to the deployment configuration or command that enforces these settings, while retaining the existing WSGI option and cutover context.frontend/src/features/purchasing/StockPage.tsx (2)
25-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSend the trimmed query.
searchActiveis computed fromquery.trim(), but the request sends the untrimmedquery. For an input of" abc"the gate opens andqis" abc". Each whitespace variation also produces a distinct query key, which adds duplicate cache entries and duplicate requests for the same search term.♻️ Proposed fix
- const searchActive = query.trim().length >= MIN_QUERY_LENGTH+ const trimmedQuery = query.trim()+ const searchActive = trimmedQuery.length >= MIN_QUERY_LENGTH const list = useQuery(purchasingStockListOptions()) const search = useQuery({ - ...purchasingStockSearchRetrieveOptions({ query: { q: query } }),+ ...purchasingStockSearchRetrieveOptions({ query: { q: trimmedQuery } }), enabled: searchActive, })🤖 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/purchasing/StockPage.tsx` around lines 25 - 31, Update the search query construction near searchActive in StockPage so the request and its query key use the trimmed query value, while preserving the existing minimum-length gating behavior.
27-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreserve the previous search results while fetching a new query.
Each active search term creates a new query key.
search.isPendingthen replaces the table withLoading stock items.... UseplaceholderData: keepPreviousDatato keep the current rows visible during the fetch.♻️ Proposed refactor
-import { useQuery } from '`@tanstack/react-query`'+import { keepPreviousData, useQuery } from '`@tanstack/react-query`'const search = useQuery({ ...purchasingStockSearchRetrieveOptions({ query: { q: query } }), enabled: searchActive, + placeholderData: keepPreviousData, })🤖 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/purchasing/StockPage.tsx` around lines 27 - 33, Update the useQuery configuration in StockPage around purchasingStockSearchRetrieveOptions to set placeholderData to keepPreviousData, preserving existing search rows while a new query is fetching and avoiding the pending loading state replacing the table.frontend/tests/e2e/helpers.ts (1)
444-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
Math.randomwith a collision-free identifier.
Math.floor(Math.random() * 100000)gives 100,000 possible values. Parallel Playwright workers and repeated local runs can produce the same supplier name. A duplicate[TEST] Supplier Ncreates a second live Xero contact push with the same name, which makes later lookups ambiguous.Use
crypto.randomUUID()or combine the timestamp with the worker index.♻️ Proposed change
- const randomSuffix = Math.floor(Math.random() * 100000)- const supplierName = `[TEST] Supplier ${randomSuffix}`+ const randomSuffix = crypto.randomUUID().slice(0, 8)+ const supplierName = `[TEST] Supplier ${randomSuffix}`🤖 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/helpers.ts` around lines 444 - 446, Update createTestPurchaseOrder so supplierName uses a collision-free identifier instead of Math.random(), preferably crypto.randomUUID() or a timestamp combined with the Playwright worker index, while preserving the existing “[TEST] Supplier” naming format.frontend/tests/e2e/purchasing/stock-search.spec.ts (1)
117-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe counter filter can miss a regression.
The listener ignores every
/search/response whose URL containsq=5mm. The exclusion exists to skip the first search, but it also hides the most likely regression: clearing the input re-fires the sameq=5mmrequest. That request is a real post-clear search call, and the assertion at Line 134 still passes.Gate on time instead of on the query value.
♻️ Proposed change
- // Track whether any further /search/ request fires when we clear the box.+ // Gate on the clear action, not on the query value: a re-fired `q=5mm`+ // request after clearing is still a post-clear search call. let postClearSearchCalls = 0 + let cleared = false page.on('response', (response) => { - if (- response.url().includes('/api/purchasing/stock/search/') &&- !response.url().includes('q=5mm')- ) {+ if (cleared && response.url().includes('/api/purchasing/stock/search/')) { postClearSearchCalls += 1 } }) + cleared = true await input.fill('')🤖 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/purchasing/stock-search.spec.ts` around lines 117 - 134, Update the response listener in the stock-search test to count search responses based on whether they occur after the input is cleared, rather than excluding URLs containing q=5mm. Start tracking post-clear responses only immediately before input.fill(''), so the initial search is ignored while any repeated q=5mm request after clearing is counted and fails the existing assertion.frontend/src/features/shared/ListTable.tsx (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
renderRowmust supply the React key.Line 66 calls
rows.map(renderRow)and applies no key. Every current caller setskeyon its own<tr>. That contract is not stated on the prop, so a future caller can omit the key and cause a React key warning plus incorrect row reconciliation.♻️ Proposed doc addition
+ /** Must set a stable React `key` on the returned row element; ListTable+ maps rows directly and applies no key of its own. */ renderRow: (row: TRow) => ReactNodeAlso applies to: 66-66
🤖 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/shared/ListTable.tsx` at line 21, Document the `renderRow` prop contract in `ListTable`: implementations must return each row with its own stable React `key`, since the `rows.map(renderRow)` call does not apply one. Add this requirement to the prop’s existing documentation without changing the rendering logic.frontend/src/features/shared/company/PersonSelectionModal.test.tsx (1)
19-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the create and update flows.
The single test covers accessibility only.
handleCreateandhandleUpdatehold the business rules: first person is always primary, blank optional fields are omitted from the request body, and an invalid email blocks submission. These rules are unit-testable with mocked mutations.🤖 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/shared/company/PersonSelectionModal.test.tsx` around lines 19 - 48, Extend the PersonSelectionModal tests beyond accessibility to cover handleCreate and handleUpdate with mocked mutations: verify the first person is always primary, blank optional fields are omitted from the request payload, and invalid email prevents submission. Assert the appropriate mutation callbacks and payloads for both create and update flows.frontend/src/features/shared/company/PersonSelectionModal.tsx (3)
294-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe person card is a clickable
div.The card carries
onClickbut norole,tabIndex, or key handler. Keyboard users reach the person through the hover-revealedSelectbutton, whichgroup-focus-withinexposes, so the flow is not blocked. Removing the card-levelonClickand relying on the explicitSelectbutton would remove the duplicated activation path and theevent.stopPropagation()calls at Lines 341, 354, and 367.🤖 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/shared/company/PersonSelectionModal.tsx` around lines 294 - 303, The person card in the person-selection rendering should no longer be clickable via its container. Remove the card-level onClick handler from the div around person.person_id, rely on the explicit Select button for activation, and remove the associated event.stopPropagation() calls in the button handlers.
218-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
invalidatePeoplehere.Lines 218-220 repeat the query-key invalidation that
invalidatePeopledefines at Lines 118-121.handleUpdateandhandleConfirmDeletealready call the helper. A future key change would need two edits.As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."♻️ Proposed fix
- await queryClient.invalidateQueries({- queryKey: companiesPeopleListQueryKey({ path: { company_id: companyId } }),- })+ await invalidatePeople()🤖 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/shared/company/PersonSelectionModal.tsx` around lines 218 - 220, Replace the direct companies-people query invalidation in the relevant update flow with the existing invalidatePeople helper, matching handleUpdate and handleConfirmDelete. Reuse that helper’s established arguments and remove the duplicated query-key invalidation.Source: Coding guidelines
534-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle the returned promise on the submit button.
handleUpdateandhandleCreateareasync. Passing them directly toonClickreturns a floating promise from the event handler. The delete button at Line 272 already wraps its call withvoid. Use the same form here for consistency and to satisfy@typescript-eslint/no-misused-promisesif that rule is enabled.♻️ Proposed fix
- onClick={editingPerson ? handleUpdate : handleCreate}+ onClick={() => {+ void (editingPerson ? handleUpdate() : handleCreate())+ }}🤖 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/shared/company/PersonSelectionModal.tsx` around lines 534 - 542, Update the submit button’s onClick handler to explicitly discard the promise returned by handleUpdate or handleCreate, matching the existing delete-button pattern. Preserve the editingPerson conditional selection and button behavior.frontend/src/features/shared/useDebouncedValue.test.tsx (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the fake-timer queue on unmount.
clearTimeoutis global, so another cleanup can satisfytoHaveBeenCalled(). Since this suite enables fake timers, assert thatvi.getTimerCount()is greater than zero afterrenderHook, then zero afterunmount().🤖 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/shared/useDebouncedValue.test.tsx` around lines 39 - 46, Update the “clears a pending timer on unmount” test to assert the fake-timer queue directly: verify vi.getTimerCount() is greater than zero after renderHook creates the debounced timer, then verify it is zero after unmount(). Remove the clearTimeout spy assertion.
🤖 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/rewrite-status.md`:
- Around line 57-61: Update the frontend rebuild table in the rewrite-status
document to replace references to features/company/PersonSelectionModal.tsx and
features/company/CompanyLookup.tsx with their canonical features/shared/company/
paths, leaving other entries unchanged.
In `@frontend/src/features/crm/CompaniesListPage.tsx`:
- Around line 113-119: Apply the first-load-only error rule at both QueryState
call sites: in frontend/src/features/crm/CompaniesListPage.tsx lines 113-119,
change the ListTable isError value to companies.isError && companies.data ===
undefined; in frontend/src/features/job/JobFinishTab.tsx lines 209-213, use a
loadError derived per query from isError && data === undefined so cached summary
and checklist content remains visible during failed refetches.
In `@frontend/src/features/purchasing/JobSelect.tsx`:
- Around line 113-150: Update JobSelect to support keyboard navigation by
tracking an active option index, moving it with ArrowUp/ArrowDown, and selecting
the active job on Enter. Add role="listbox" to the dropdown and role="option"
with aria-selected to each mapped job row, making options keyboard-accessible
and reusing the shared picker pattern if available.
- Around line 91-111: Update the closing paths in JobSelect’s onBlur timeout and
Escape handler to reset editing and clear the stale search term when the picker
closes without selection, so value returns to the bound job and filtering does
not reuse old input. Store the 150ms blur timeout in a ref and add a useEffect
cleanup that clears it on unmount, while preserving the existing select
behavior.
In `@frontend/src/features/purchasing/PoListPage.tsx`:
- Around line 57-74: Update the purchase-order row in PoListPage’s renderRow
callback to be keyboard accessible: make the tr focusable, expose it as an
interactive control, and handle keyboard activation by navigating to the same
po/$poId destination as onClick. Preserve the existing mouse navigation
behavior.
In `@frontend/src/features/shared/company/CompanyLookup.tsx`:
- Around line 238-242: Update the suggestions empty-state rendering near
companiesSearchRetrieveOptions to check search.isError first and display an
error state when the request fails. Gate the existing “No companies found”
message on a successful, non-error search so rejected requests never appear as
empty results.
- Around line 88-99: Associate quickCreateCompany with an active request token
and call handleCompanyCreated only when the response still matches the current
lookup interaction; otherwise ignore the completion. In
frontend/src/features/shared/company/CompanyLookup.tsx lines 88-99, update
quickCreateCompany accordingly. In
frontend/src/features/shared/company/CreateCompanyModal.tsx lines 49-64, either
prevent dismissal while creation is pending or ignore completion after the modal
closes. Add regression tests covering abandoned quick-create and modal-creation
interactions.
In `@frontend/src/features/shared/company/PersonSelectionModal.tsx`:
- Around line 242-281: Update the form footer submit state in the component’s
submitDisabled logic to also disable submission whenever deleteTarget is
non-null or removeLink.isPending is true. Keep the existing confirmation overlay
behavior and ensure create/edit actions cannot start while deletion is pending
or awaiting confirmation.
In `@frontend/src/features/shared/decimal.ts`:
- Around line 10-16: Update parseDecimalInput to validate cleaned input against
an explicit fixed-point decimal syntax before numeric finiteness checks,
rejecting hex, octal, binary, and other non-decimal forms. Normalize accepted
values such as “.5”, “+5”, and “5.” into the backend-supported decimal
representation, and ensure trimDecimal cannot return invalid syntax verbatim.
In `@frontend/src/features/shared/ItemSelect.tsx`:
- Around line 19-43: Rewrite or remove the comments at
frontend/src/features/shared/ItemSelect.tsx:19-43, replacing implementation,
E2E, and history narration with the rejected API alternative and factual
constraint, or remove them. Apply the same constraint-based approach at
frontend/src/features/shared/SaveFailedBadge.tsx:1-4,
frontend/src/features/shared/decimal.test.ts:33-34 (including the rejected
formatting behavior and exponent constraint),
frontend/src/features/shared/company/CompanyLookup.test.tsx:154-155 (including
the Sonner rendering constraint if retained),
frontend/src/features/shared/company/CompanyLookup.tsx:88-90, and
frontend/src/features/shared/company/CreateCompanyModal.tsx:16-26 and :39
(document only rejected alternatives and current product/state constraints,
otherwise remove the comments).
---
Nitpick comments:
In `@docs/cutover-checklist.md`:
- Around line 115-123: Update the serving-model checklist item to define
executable ASGI acceptance criteria: minimum concurrency, request timeout,
keep-alive, database-connection capacity, and ORM-safety requirements. Link the
checkbox to the deployment configuration or command that enforces these
settings, while retaining the existing WSGI option and cutover context.
In `@frontend/src/features/purchasing/StockPage.tsx`:
- Around line 25-31: Update the search query construction near searchActive in
StockPage so the request and its query key use the trimmed query value, while
preserving the existing minimum-length gating behavior.
- Around line 27-33: Update the useQuery configuration in StockPage around
purchasingStockSearchRetrieveOptions to set placeholderData to keepPreviousData,
preserving existing search rows while a new query is fetching and avoiding the
pending loading state replacing the table.
In `@frontend/src/features/shared/company/PersonSelectionModal.test.tsx`:
- Around line 19-48: Extend the PersonSelectionModal tests beyond accessibility
to cover handleCreate and handleUpdate with mocked mutations: verify the first
person is always primary, blank optional fields are omitted from the request
payload, and invalid email prevents submission. Assert the appropriate mutation
callbacks and payloads for both create and update flows.
In `@frontend/src/features/shared/company/PersonSelectionModal.tsx`:
- Around line 294-303: The person card in the person-selection rendering should
no longer be clickable via its container. Remove the card-level onClick handler
from the div around person.person_id, rely on the explicit Select button for
activation, and remove the associated event.stopPropagation() calls in the
button handlers.
- Around line 218-220: Replace the direct companies-people query invalidation in
the relevant update flow with the existing invalidatePeople helper, matching
handleUpdate and handleConfirmDelete. Reuse that helper’s established arguments
and remove the duplicated query-key invalidation.
- Around line 534-542: Update the submit button’s onClick handler to explicitly
discard the promise returned by handleUpdate or handleCreate, matching the
existing delete-button pattern. Preserve the editingPerson conditional selection
and button behavior.
In `@frontend/src/features/shared/ListTable.tsx`:
- Line 21: Document the `renderRow` prop contract in `ListTable`:
implementations must return each row with its own stable React `key`, since the
`rows.map(renderRow)` call does not apply one. Add this requirement to the
prop’s existing documentation without changing the rendering logic.
In `@frontend/src/features/shared/useDebouncedValue.test.tsx`:
- Around line 39-46: Update the “clears a pending timer on unmount” test to
assert the fake-timer queue directly: verify vi.getTimerCount() is greater than
zero after renderHook creates the debounced timer, then verify it is zero after
unmount(). Remove the clearTimeout spy assertion.
In `@frontend/tests/e2e/helpers.ts`:
- Around line 444-446: Update createTestPurchaseOrder so supplierName uses a
collision-free identifier instead of Math.random(), preferably
crypto.randomUUID() or a timestamp combined with the Playwright worker index,
while preserving the existing “[TEST] Supplier” naming format.
In `@frontend/tests/e2e/purchasing/stock-search.spec.ts`:
- Around line 117-134: Update the response listener in the stock-search test to
count search responses based on whether they occur after the input is cleared,
rather than excluding URLs containing q=5mm. Start tracking post-clear responses
only immediately before input.fill(''), so the initial search is ignored while
any repeated q=5mm request after clearing is counted and fails the existing
assertion.
🪄 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: 66ca2b61-202f-4ca9-a415-b773f6995a0a
📒 Files selected for processing (60)
docs/adr/0039-one-implementation-per-concept.mddocs/code-quality.mddocs/cutover-checklist.mddocs/rewrite-status.mdfrontend/src/api/index.tsfrontend/src/api/query-client.tsfrontend/src/features/crm/CompaniesListPage.tsxfrontend/src/features/crm/CompanyDetailPage.tsxfrontend/src/features/job/JobCreatePage.tsxfrontend/src/features/job/JobFinishTab.tsxfrontend/src/features/job/JobSettingsTab.tsxfrontend/src/features/job/costing/CostLineGrid.tsxfrontend/src/features/job/costing/calc.test.tsfrontend/src/features/job/costing/calc.tsfrontend/src/features/purchasing/JobSelect.test.tsfrontend/src/features/purchasing/JobSelect.tsxfrontend/src/features/purchasing/PoCreatePage.tsxfrontend/src/features/purchasing/PoDetailPage.tsxfrontend/src/features/purchasing/PoLinesTable.tsxfrontend/src/features/purchasing/PoListPage.tsxfrontend/src/features/purchasing/PoSummaryCard.tsxfrontend/src/features/purchasing/StockPage.tsxfrontend/src/features/purchasing/index.tsfrontend/src/features/purchasing/lines.test.tsfrontend/src/features/purchasing/lines.tsfrontend/src/features/purchasing/usePoLines.tsfrontend/src/features/reports/JobMovementReportPage.tsxfrontend/src/features/reports/WipReportPage.tsxfrontend/src/features/shared/DataTable.tsxfrontend/src/features/shared/ItemSelect.tsxfrontend/src/features/shared/ListTable.test.tsxfrontend/src/features/shared/ListTable.tsxfrontend/src/features/shared/QueryState.test.tsxfrontend/src/features/shared/QueryState.tsxfrontend/src/features/shared/SaveFailedBadge.tsxfrontend/src/features/shared/company/CompanyLookup.test.tsxfrontend/src/features/shared/company/CompanyLookup.tsxfrontend/src/features/shared/company/CreateCompanyModal.tsxfrontend/src/features/shared/company/PersonSelectionModal.test.tsxfrontend/src/features/shared/company/PersonSelectionModal.tsxfrontend/src/features/shared/company/PersonSelector.tsxfrontend/src/features/shared/company/create-company.tsfrontend/src/features/shared/company/index.tsfrontend/src/features/shared/company/xero-contact.tsfrontend/src/features/shared/decimal.test.tsfrontend/src/features/shared/decimal.tsfrontend/src/features/shared/useDebouncedValue.test.tsxfrontend/src/features/shared/useDebouncedValue.tsfrontend/src/features/timesheet/DailyOverviewPage.tsxfrontend/src/features/timesheet/SmartTimesheetTable.tsxfrontend/src/features/timesheet/TimesheetEntryPage.tsxfrontend/src/routeTree.gen.tsfrontend/src/routes/_authed/purchasing/po/$poId.tsxfrontend/src/routes/_authed/purchasing/po/create.tsxfrontend/src/routes/_authed/purchasing/po/index.tsxfrontend/src/routes/_authed/purchasing/stock.tsxfrontend/tests/e2e/helpers.tsfrontend/tests/e2e/purchasing/create-purchase-order.spec.tsfrontend/tests/e2e/purchasing/po-created-by.spec.tsfrontend/tests/e2e/purchasing/stock-search.spec.ts
💤 Files with no reviewable changes (2)
- frontend/src/features/job/costing/calc.ts
- frontend/src/features/job/costing/calc.test.ts
🛑 Comments failed to post (3)
frontend/src/features/shared/company/CompanyLookup.tsx (2)
88-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignore completion from abandoned company-creation interactions.
Both creation paths apply a completed mutation after the initiating UI state can change. A quick-create response can select an old query after the user continues searching. A modal response can select a company after the user closes the dialog.
frontend/src/features/shared/company/CompanyLookup.tsx#L88-L99: associate quick creation with an active request token, and select the returned company only if the request still matches the current interaction.frontend/src/features/shared/company/CreateCompanyModal.tsx#L49-L64: prevent dismissal while creating or ignore a completion after the modal closes. Add regression tests for both paths.📍 Affects 2 files
frontend/src/features/shared/company/CompanyLookup.tsx#L88-L99(this comment)frontend/src/features/shared/company/CreateCompanyModal.tsx#L49-L64🤖 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/shared/company/CompanyLookup.tsx` around lines 88 - 99, Associate quickCreateCompany with an active request token and call handleCompanyCreated only when the response still matches the current lookup interaction; otherwise ignore the completion. In frontend/src/features/shared/company/CompanyLookup.tsx lines 88-99, update quickCreateCompany accordingly. In frontend/src/features/shared/company/CreateCompanyModal.tsx lines 49-64, either prevent dismissal while creation is pending or ignore completion after the modal closes. Add regression tests covering abandoned quick-create and modal-creation interactions.238-242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render a search failure state.
If
companiesSearchRetrieveOptionsrejects,search.isPendingis false and this branch rendersNo companies found.. The user receives a false empty-result message. Render an error state whensearch.isErroris true, and reserve the empty state for successful empty responses.🤖 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/shared/company/CompanyLookup.tsx` around lines 238 - 242, Update the suggestions empty-state rendering near companiesSearchRetrieveOptions to check search.isError first and display an error state when the request fails. Gate the existing “No companies found” message on a successful, non-error search so rejected requests never appear as empty results.frontend/src/features/shared/company/PersonSelectionModal.tsx (1)
242-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The delete overlay does not block the form column.
The confirmation panel is
absolute inset-0inside the people-list column only. The create/edit form and the submit button stay interactive while the confirmation is open.submitDisabledat Line 226 also ignoresremoveLink.isPending, so a user can start a create while a delete is in flight. Consider disabling the footer submit whiledeleteTarget !== nullorremoveLink.isPending.🤖 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/shared/company/PersonSelectionModal.tsx` around lines 242 - 281, Update the form footer submit state in the component’s submitDisabled logic to also disable submission whenever deleteTarget is non-null or removeLink.isPending is true. Keep the existing confirmation overlay behavior and ensure create/edit actions cannot start while deletion is pending or awaiting confirmation.
- CompaniesListPage/JobFinishTab: QueryState was passed the raw isError, breaking the first-load-only rule the other converted sites already follow -- a background refetch failure was unmounting already-rendered content instead of leaving it on screen. - JobSelect: closing the picker without a selection (blur or Escape) left `editing` true forever, so the input kept showing the abandoned search term instead of reverting to the bound job; the blur timer was untracked and could fire a state update after unmount. Also added keyboard selection (arrow keys + Enter, role=listbox/option, aria-activedescendant) -- the dropdown was mouse-only. - PoListPage: the row was reachable only by mouse click; added a real Link on the PO number cell, matching CompaniesListPage's existing pattern. - decimal.ts: parseDecimalInput accepted anything Number() parses, including hex/octal/binary literals and bare exponents, and sent them to the wire verbatim as garbage Decimal strings. Validates fixed-point syntax explicitly now. - Stale features/company/* paths in rewrite-status.md's build-order table, left behind by the features/shared/company move. - Comments narrating port history or test intent rewritten to state the rejected alternative and the constraint (ADR 0043) in ItemSelect, SaveFailedBadge, CreateCompanyModal, CompanyLookup; two flagged sites (the E2E-repair-loop comment, the sonner test comment) were already compliant and left as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
PR #54 (main) widened pyproject.toml's django specifier to >=6.0,<6.2 but never regenerated the matching uv.lock, which still recorded <6.1 in its requires-dist metadata. Every `uv run` this session silently self-corrected the lock locally; committing that fix rather than leaving a lockfile permanently out of sync with the manifest it's supposed to pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
frontend/test-results/ was covered; run_e2e.sh's Playwright invocation also writes artifacts relative to the repo root in some invocations, leaving an untracked test-results/ dangling after every E2E run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
…config playwright.config.ts resolved .env.test, testDir, outputDir and the html reporter's folder relative to process.cwd(). Any invocation whose cwd wasn't frontend/ (npm --prefix from the repo root, a bare npx playwright test) silently missed .env.test -- dropping E2E_TEST_USERNAME/PASSWORD with no error -- or wrote artifacts to the wrong location instead of erroring loudly, which is how a root-level test-results/ kept reappearing. Anchored every one of these to import.meta.dirname instead: verified from an unrelated cwd, .env/.env.test still resolve correctly and no stray directory gets created anywhere. The root-level gitignore entry from the previous commit stays as a defensive backstop, but this is the real fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Summary
Ports three of the five purchasing-cluster E2E specs (
po-created-by,create-purchase-order,stock-search) with a full purchasing UI: PO list/create/detail pages, an editable PO-lines grid, and a stock page.supplier-alias-searchandpickup-address(the two remaining, Google-Places-dependent specs) are next.Backend was already ported in a prior slice; this is frontend-only.
Architecture work beyond the target specs
Three rounds of adversarial review found this slice was about to leave (or was leaving) real structural debt behind, and each was fixed before merge rather than deferred:
features/shared/DataTable.tsx— the one owner of the editable-grid E2E contract (DataTable-row-N,data-grid-*). The newPoLinesTablewould otherwise have been a third hand-rolled copy alongsideSmartTimesheetTableandCostLineGrid; all three now render through it.features/shared/QueryState.tsx+features/shared/ListTable.tsx— the one owner of the pending/error gate every query-backed page or panel repeats, and the plain-rows-table shell layered over it.PoListPage/StockPagehad copiedCompaniesListPage's loading/error/retry block verbatim; an exhaustive sweep (three passes, each catching what the last missed) found and converted every genuine instance across the frontend —PoDetailPage,CostLineGrid,JobMovementReportPage,CompanyDetailPage,JobFinishTab,DailyOverviewPage,TimesheetEntryPage. A few sites stay deliberately unconverted (embedded card widgets with richer branching than binary success/fail; one guard-clause-shaped early return matching CLAUDE.md's own preference) — reasoning recorded inrewrite-status.md.features/company→features/shared/company— it had no route of its own and was already cross-imported byjob; this slice'sPoSummaryCardwould have been a third cross-domain import of what was really a shared widget library sitting in a domain-shaped directory.PoLinesTable's item-picker label had no description fallback, so a bound stock item with a nullitem_code(nullable, v1 parity) misread as unbound.ADR 0039 was strengthened with the underlying principle: architectural unification is never deferred to a later slice, and shared concepts get shared homes.
Also (unrelated to purchasing, fixed in the same branch at the user's direction):
docs/rewrite-status.mdanddocs/cutover-checklist.mdcorrected — the SSE/live-updates work (Slice 3) was misfiled as a post-cutover deferral and is actually MUST-before-cutover, and the release-gate section now states both go/no-go criteria (functional parity, architecture quality) rather than only the E2E proxy for the first.Test plan
npm run type-checkcleannpx vitest run src/features— 197/197 passing./scripts/ops/run_e2e.sh(full 88-case suite) green🤖 Generated with Claude Code
https://claude.ai/code/session_01WENcwU8rC6o7bxuHL6md2L
Summary by CodeRabbit