Xero quote path: quote push + cost-line grid — job-xero-quote green (14 of 40) - #48
Merged
Conversation
…otocol QuotePayload carries required terms (Xero applies no default to API-created quotes) and an expiry date; QuotePdfDocument hands the caller a temp file it owns. Live provider mirrors the invoice shapes: constructed edit-URL, DELETED upsert after a contact/date pre-read, PDF download that raises rather than returning a partial result. Readonly fabricates QU-E2E-* results and refuses the PDF download outright — a fabricated file would satisfy the text assertion against nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… mirror Expected refusals (already quoted, T&M pricing, empty quote cost set, blank breakdown descriptions, missing theme/terms config) return typed 400 values with the provider never called; unexpected failures persist once and re-raise per the base contract. Total-only mode sends the cost-set summary revenue as a single line; breakdown mode one sanitised line per cost line. The job's updated_at bumps in-request so the tab refetch sees quoted=true. The duplicated _create_job_event hoisted from invoice.py into the base. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST /xero/create_quote/{job_id} (body: breakdown) and DELETE
/xero/delete_quote/{job_id} mirror the invoice handlers; delete takes no id
parameter because a job holds at most one quote. GET /job/jobs/{id}/quote/
serves the Xero quote header or null — a plain GET, not v1's conditional-GET:
nothing external holds the URL and 304-with-empty-body reads as no-quote to
an axios consumer (ledger entry to follow with the slice docs).
XeroDocumentSuccessResponse gains nullable quote_id beside invoice_id; the
two xero operation renames are recorded in the work-list ledger.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Ports the v1 inspection: pypdf text extraction over non-blank pages, space-normalised AND compact matching (Xero's text layer wraps mid-phrase and sometimes drops word spaces), blank render raises rather than reporting the marker absent, and the temp file survives every failure path for diagnosis. The command emits exactly one sorted-keys JSON line — the subprocess contract the E2E quote spec parses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-exports the cost-line/labour-rates/stock-search factories and the three new quote operations through the api boundary. Adds @tanstack/react-table and the shadcn popover + command primitives (installed, not hand-written). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TanStack Table v8 with module-constant column defs — rebuilding them per render changes every cell component's identity and remounts (blurring) all inputs, so cells reach live state through table meta instead. Day-one contract: .smart-costlines-table, exactly one trailing phantom tbody row, SmartCostLinesTable-*/DataTable-row-*/data-grid-* attributes derived from the visual index, ItemSelect with labour-first options. useAutosaveField DERIVES its display value (local buffer only while editing): an optimistic write and its failure rollback can land between two renders, so an effect keyed on the server value never fires — the unit net caught a synced copy keeping rejected input on screen. 600ms debounce, blur flushes and cancels, no If-Match on cost-line CRUD (deliberate, v1 parity). All failures toast; the tests fail on any console.error, matching the E2E guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The quote workspace: editable quote cost set, server-owned summary card
(ADR 0046), and the Xero card (ping-gated create, Export Quote to Xero
dialog with Send Total Only / Send Breakdown, open-in-Xero with noopener,
delete). Lazy-loaded from the job detail page.
The retrieve contract changed to {quote: QuoteOut | null}: the unit net
caught the generated axios client coercing a bare JSON null body to {},
which read as an existing quote and crashed the card — an envelope is the
only shape that round-trips absence.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>…check Deliberate deviations from the v1 spec, each with its reason in place: a dedicated job instead of the read-only sharedEditJobUrl fixture; no in-spec Xero ping (global setup fails the run closed); waitForAutosave instead of 800ms sleeps around the 600ms debounce; and the PDF inspector spawns via uv run python since nothing guarantees an activated interpreter under npm. The repair machinery (Select Item pick, desc/unit-rev fills) ports near-verbatim — it is the part of the spec that exercises the grid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st states Backend: the concurrent-push loser now voids its orphan Xero quote (under a savepoint, so the compensation can persist its AppError) and refuses with the readable 400; a quote deleted Xero-side returns a typed 404 from the provider pre-read and the manager treats it as cleanup-allowed instead of bricking the job; the local mirror stores the payload's date (midnight span); present-but-null totals get the crafted message in both quote and invoice managers; the not-found PDF keeps its file for diagnosis; delete_invoice passes error_type through like its quote sibling. Frontend: a failed draft POST clears the persisting guard so the row stays retryable (was permanently bricked); draft inputs disable while the create is in flight; the send-dedupe only skips a KNOWN-applied value so a rejected edit can be retried; the quote card renders pending as pending, not as create-state; error states stop masquerading as data (picker, ping, summary); background-refetch errors keep the working grid; null profit margin renders as a dash, not 0.0%; PATCH rollback reverts only the patched fields against the current cache, with in-flight refetches cancelled first. Spec: isEnabled guard before repairing a rev input; response schema validates quote_id/online_url. Declined: the compact-match tightening (the proposed regex is equivalent in power — its own counterexample defeats both) and the readonly totals type change (2b precedent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warning Review limit reached
Next review available in:23 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 (11)
📝 WalkthroughWalkthroughAdded end-to-end Xero quote support across accounting providers, quote APIs, PDF inspection, job retrieval, frontend costing, and quote management UI. Added tests, OpenAPI updates, compatibility mappings, and E2E validation. ChangesXero quote backend
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant JobQuoteTab
participant XeroQuoteCard
participant QuoteAPI
participant Xero
User->>JobQuoteTab: Open the quote tab
JobQuoteTab->>XeroQuoteCard: Load quote state
User->>XeroQuoteCard: Select quote mode and submit
XeroQuoteCard->>QuoteAPI: Create quote request
QuoteAPI->>Xero: Create quote
Xero-->>QuoteAPI: Return quote identifiers and totals
QuoteAPI-->>XeroQuoteCard: Return quote response
XeroQuoteCard-->>User: Display quote link and status
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: 8
🧹 Nitpick comments (10)
frontend/src/features/job/costing/useCostLines.ts (1)
121-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
deleteLinerestores the whole snapshot.
patchLinecarries a comment that rejects the wholesale snapshot restore and explains the interleaved-write constraint.deleteLinethen uses that exact rejected strategy at Line 134 with no comment. A failed delete restores the full pre-delete cost set, so it also reverts any successful interleaved patch on another line until theonSettledrefetch lands. Record the constraint that makes the wholesale restore correct here, for example that a removed row cannot be reconstructed field-by-field.As per coding guidelines: "Comments must document the rejected obvious alternative and the factual constraint that rejected it".
🤖 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/job/costing/useCostLines.ts` around lines 121 - 140, Update deleteLine’s onError rollback comment to document why restoring the entire snapshot is intentional: unlike patchLine, a deleted row cannot be reconstructed field-by-field, so a wholesale restore is required despite potentially reverting interleaved writes until invalidate refetches.Source: Coding guidelines
frontend/src/features/job/costing/calc.test.ts (1)
123-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the two unresolved-reference fallbacks in
itemLabel.The suite does not exercise the fallback at
calc.tsLine 79 (labour rate not in the list) or Line 86 (stock id not in the loaded page). The comment oncalc.tsLine 85 states that the 'Stock item' fallback must not read as 'Select Item', because the E2E repair loop counts buttons by that exact name. A regression that returns 'Select Item' for an unresolved stock id would pass this suite and fail only in Playwright. Add the two cases.🧪 Proposed additional cases
it('names the labour subtype for a time line', () => { expect(itemLabel(line({ kind: 'time', labour_subtype: 'workshop' }), stockById, rates)).toBe( 'Workshop', ) }) ++ it('falls back to the raw subtype when the rate is not loaded', () => {+ expect(itemLabel(line({ kind: 'time', labour_subtype: 'nightshift' }), stockById, rates)).toBe(+ 'nightshift',+ )+ })++ it('never reads as "Select Item" when bound to unloaded stock', () => {+ expect(itemLabel(line({ ext_refs: { stock_id: 'stock-unloaded' } }), stockById, rates)).toBe(+ 'Stock item',+ )+ }) })🤖 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/job/costing/calc.test.ts` around lines 123 - 140, Add two `itemLabel` tests covering unresolved references: verify a time line with a labour subtype absent from `rates` uses the labour-rate fallback, and verify a bound line whose stock ID is absent from `stockById` returns the distinct “Stock item” fallback rather than “Select Item”.frontend/src/features/job/costing/CostLineGrid.test.tsx (1)
67-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the
labourRatesandstockPagefixtures with the generated types.
materialLineandcostSetare annotated, so a wire-type change breaks this file at compile time.labourRatesandstockPageare bare literals, so the same change passes type-checking and fails only at runtime.calc.test.tsannotates the equivalent fixtures asJobLabourRateOutandStockItem. Addconst labourRates: JobLabourRateOut[]and the generated page type forstockPage.🤖 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/job/costing/CostLineGrid.test.tsx` around lines 67 - 108, Annotate the labourRates fixture as JobLabourRateOut[] and annotate stockPage with the generated page type used for StockItem results, matching the equivalent fixtures in calc.test.ts. Preserve the existing fixture values while ensuring wire-type changes are caught during compilation.frontend/src/features/job/costing/ItemSelect.tsx (1)
88-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider debouncing the stock search input.
CommandInputcallsonValueChange={setSearch}directly, andsearchis part of thepurchasingStockSearchRetrieveOptionsquery key. Every keystroke while the popover is open fires a new server-side stock search request. The component's own comment notes that queries under 3 characters list everything, so short inputs during typing can each trigger a full unfiltered fetch.Debounce the value passed to the query (for example with a small
useDeferredValueor timer-based hook) so the request fires once typing pauses.🤖 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/job/costing/ItemSelect.tsx` around lines 88 - 92, Debounce the search value used by the stock query in ItemSelect rather than passing the raw search state into purchasingStockSearchRetrieveOptions on every keystroke. Keep CommandInput responsive with the immediate value, and use a small deferred or timer-based value so requests occur after typing pauses while preserving the existing short-query behavior.frontend/src/features/job/costing/XeroQuoteCard.test.tsx (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
userreturned byrenderWithProviders.
renderWithProvidersalready callsuserEvent.setup()and returns the instance. Each test creates a second instance. Two sessions can hold separate pointer and keyboard state. Reuse the returneduserin all five tests.- const user = userEvent.setup()- renderWithProviders(<XeroQuoteCard jobId="job-1" />)+ const { user } = renderWithProviders(<XeroQuoteCard jobId="job-1" />)🤖 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/job/costing/XeroQuoteCard.test.tsx` around lines 72 - 73, Update all five tests in XeroQuoteCard.test.tsx to destructure and reuse the user instance returned by renderWithProviders, removing each redundant userEvent.setup() call while preserving the existing test interactions.frontend/src/features/job/costing/CostLineGrid.tsx (1)
192-199: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMove the create call out of the
setDraftsupdater.
commitDraftFieldrunspersistDraftIfReadyinside a state updater. That updater executes during render. It starts a network mutation and callssyncPersisting(anothersetState) from render. React requires updaters to be pure. ThepersistingRefguard hides the StrictMode double invocation today, but any future replay of the updater still re-enters impure code.Keep the latest drafts in a ref and run the persistence after the state update.
♻️ Proposed refactor: read drafts from a ref instead of a state updater
const [drafts, setDrafts] = useState<DraftRow[]>([freshPhantom()]) + // persistDraftIfReady must see the same render's updateDraft result, but a+ // network call inside a state updater is not a pure update; the ref carries+ // that latest value out of the updater instead.+ const draftsRef = useRef<DraftRow[]>(drafts)+ draftsRef.current = drafts @@ updateDraft: (localId, patch) => { setDrafts((current) => { ... - return next+ draftsRef.current = next+ return next }) }, commitDraftField: (localId) => { - setDrafts((current) => {- persistDraftIfReady(current, localId)- return current- })+ persistDraftIfReady(draftsRef.current, localId) },🤖 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/job/costing/CostLineGrid.tsx` around lines 192 - 199, Refactor commitDraftField so persistDraftIfReady is no longer called inside the setDrafts updater; keep the latest drafts synchronized in a ref and invoke persistence after the state update using that ref. Ensure the setDrafts updater remains pure while preserving the existing localId persistence behavior and syncPersisting flow.apps/xero/tests/test_provider_documents.py (2)
431-438: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the 404 status code, not only the error text.
XeroQuoteManager.delete_document(apps/xero/documents/quote.py, line 311) branches onresult.status_code == 404to clean up the local row when the quote is already gone from Xero. The error string is not part of that contract; the status code is. This test passes today even ifdelete_quotereturns status 500 for an absent quote, which would brick the recovery path.♻️ Proposed assertion
result = provider.delete_quote(str(uuid.uuid4())) assert not result.success assert result.error is not None and "no quote" in result.error + # The manager keys its local-cleanup branch on this code, not the text.+ assert result.status_code == 404🤖 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 `@apps/xero/tests/test_provider_documents.py` around lines 431 - 438, Update test_missing_quote_is_an_error_result to assert that the failed delete_quote result has status_code 404, preserving the existing unsuccessful-result check while treating the status code—not the error text—as the contract for a missing quote.
462-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the match, or drop "persists" from the test name.
match="quote"matches everyValueErrorthis method raises, including the "returned N quotes" guard. The name promises a persistence assertion that the body does not make.♻️ Proposed change
- with pytest.raises(ValueError, match="quote"):+ with pytest.raises(ValueError, match="for a request naming"): provider.download_quote_pdf(str(uuid.uuid4()))🤖 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 `@apps/xero/tests/test_provider_documents.py` around lines 462 - 467, Align test_quote_id_mismatch_raises_and_persists with its actual coverage: either assert the specific quote-ID mismatch error message rather than the broad “quote” match, and add the intended persistence assertion, or rename the test to remove “persists” if persistence is not being verified.apps/xero/tests/test_quote_manager.py (1)
170-197: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the unvoidable-orphan branch.
This test covers the race loser whose compensating void succeeds. The branch at apps/xero/documents/quote.py lines 216-224 is untested: when
delete_quotealso fails, the manager raises aValueErrorcarrying the orphan quote id. That id is the only record an operator has, because theAppErrorrow cannot carry it. A regression that swallowed this raise would leave a real Xero quote orphaned with no trace.♻️ Proposed companion test
+ def test_race_loser_raises_when_the_orphan_cannot_be_voided(+ self, company: Company, job: Job, office_staff: Staff+ ) -> None:+ """An unvoidable orphan must reach an operator with its Xero id."""+ provider = Mock()+ provider.get_account_code.return_value = "200"+ orphan = _success_result()++ def concurrent_winner_lands_first(payload: object) -> DocumentResult: # noqa: ARG001+ _existing_quote(job, company)+ return orphan++ provider.create_quote.side_effect = concurrent_winner_lands_first+ provider.delete_quote.return_value = DocumentResult(+ success=False, error="Quote is ACCEPTED", status_code=400+ )+ manager = _manager(company, job, office_staff, provider)++ with pytest.raises(ValueError, match=str(orphan.external_id)):+ manager.create_document(breakdown=False)++ assert Quote.objects.count() == 1 # only the winner's row🤖 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 `@apps/xero/tests/test_quote_manager.py` around lines 170 - 197, Add a companion test for the race-loser path in create_document where provider.delete_quote fails after Quote.objects.create is rejected; assert the manager raises ValueError containing the orphan Xero quote id, while preserving the existing assertions for the successful compensation case.stubs/xero_python/accounting/__init__.pyi (1)
80-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
Quotefields the provider sets.
apps/xero/provider.py(lines 271-272) passesline_amount_typesandcurrency_codeintoQuote(...). The stub does not declare either attribute. The call type-checks today only because__init__accepts**kwargs: Any. Any later attribute read of those fields would fail under strict mypy.♻️ Proposed stub additions
class Quote: quote_id: str | None quote_number: str | None contact: Contact | None date: Any expiry_date: Any status: str | None line_items: list[LineItem] | None + line_amount_types: str | None+ currency_code: str | None branding_theme_id: str | None terms: str | None reference: str | None updated_date_utc: Any🤖 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 `@stubs/xero_python/accounting/__init__.pyi` around lines 80 - 93, Update the Quote stub by declaring the line_amount_types and currency_code attributes alongside the existing quote fields, using types consistent with the values passed by apps/xero/provider.py. Keep the existing constructor and serialization declarations unchanged.
🤖 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 `@apps/accounting/management/commands/inspect_xero_quote_pdf.py`:
- Line 28: Replace the narration docstring near the command’s
validation/inspection flow in
apps/accounting/management/commands/inspect_xero_quote_pdf.py:28-28 with either
no comment or rationale explaining the rejected behavior and the subprocess
contract requiring the selected behavior. Update the comment in
frontend/tests/e2e/job/job-xero-quote.spec.ts:139-140 to explain that treating
the phantom row as a persisted cost line would create invalid repair work
because it has no persisted cost-line data.
In `@apps/accounting/services/quote_pdf.py`:
- Around line 32-69: Wrap the PdfReader usage in the inspection flow with a
context manager so its streams close before any cleanup. Ensure the reader is
closed on both successful validation and failure paths, while preserving the
diagnostic PDF when the expected text is absent and allowing unlink after the
reader scope ends.
In `@frontend/src/features/job/costing/calc.ts`:
- Around line 32-44: Update stockPickPatch to explicitly set labour_subtype to
null when constructing the material CostLineUpdateRequest, while preserving the
existing stock and cost field mappings.
In `@frontend/src/features/job/costing/CostLineGrid.tsx`:
- Around line 410-419: Update the onPickStock handler to preserve null for
missing patch.unit_cost and patch.unit_rev instead of converting absent values
to empty strings. Ensure the resulting DraftLine remains incomplete when either
price is unavailable, and omit null price fields when constructing the create
PATCH request.
In `@frontend/src/features/job/costing/useAutosaveField.ts`:
- Around line 52-64: Update dispatch to compare against a live serverValue ref
rather than the render-captured serverValue when the debounce callback runs.
Keep the existing knownApplied and untouched deduplication behavior, and ensure
the ref is synchronized with the latest server value before dispatch evaluates
parsed.
In `@frontend/src/features/job/costing/XeroQuoteCard.tsx`:
- Around line 170-190: Update the button label logic in the XeroQuoteCard
component to distinguish ping.isPending from a genuinely disconnected
xeroConnected state. While the Xero connection check is pending, show a
checking/loading label and prevent the logged-out “Login to Xero first” message;
retain the existing labels once the check resolves.
- Around line 81-95: The executeDelete handler currently deletes the quote
without confirmation. Update executeDelete to show a visible confirmation
dialog, such as window.confirm with a clear deletion message, and only call
deleteQuote.mutate when the user confirms; preserve the existing pending guard
and success/error callbacks.
In `@stubs/xero_python/accounting/__init__.pyi`:
- Line 173: Update the AccountingApi quote PDF call to use the method name
provided by xero-python==15.0.0 instead of get_quote_as_pdf, avoiding the
AttributeError. If a local get_quote_as_pdf implementation is retained, pass
headers={"Accept": "application/pdf"} and write its response to a temporary file
before applying the return-type check, rather than treating the raw response as
a path.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 431-438: Update test_missing_quote_is_an_error_result to assert
that the failed delete_quote result has status_code 404, preserving the existing
unsuccessful-result check while treating the status code—not the error text—as
the contract for a missing quote.
- Around line 462-467: Align test_quote_id_mismatch_raises_and_persists with its
actual coverage: either assert the specific quote-ID mismatch error message
rather than the broad “quote” match, and add the intended persistence assertion,
or rename the test to remove “persists” if persistence is not being verified.
In `@apps/xero/tests/test_quote_manager.py`:
- Around line 170-197: Add a companion test for the race-loser path in
create_document where provider.delete_quote fails after Quote.objects.create is
rejected; assert the manager raises ValueError containing the orphan Xero quote
id, while preserving the existing assertions for the successful compensation
case.
In `@frontend/src/features/job/costing/calc.test.ts`:
- Around line 123-140: Add two `itemLabel` tests covering unresolved references:
verify a time line with a labour subtype absent from `rates` uses the
labour-rate fallback, and verify a bound line whose stock ID is absent from
`stockById` returns the distinct “Stock item” fallback rather than “Select
Item”.
In `@frontend/src/features/job/costing/CostLineGrid.test.tsx`:
- Around line 67-108: Annotate the labourRates fixture as JobLabourRateOut[] and
annotate stockPage with the generated page type used for StockItem results,
matching the equivalent fixtures in calc.test.ts. Preserve the existing fixture
values while ensuring wire-type changes are caught during compilation.
In `@frontend/src/features/job/costing/CostLineGrid.tsx`:
- Around line 192-199: Refactor commitDraftField so persistDraftIfReady is no
longer called inside the setDrafts updater; keep the latest drafts synchronized
in a ref and invoke persistence after the state update using that ref. Ensure
the setDrafts updater remains pure while preserving the existing localId
persistence behavior and syncPersisting flow.
In `@frontend/src/features/job/costing/ItemSelect.tsx`:
- Around line 88-92: Debounce the search value used by the stock query in
ItemSelect rather than passing the raw search state into
purchasingStockSearchRetrieveOptions on every keystroke. Keep CommandInput
responsive with the immediate value, and use a small deferred or timer-based
value so requests occur after typing pauses while preserving the existing
short-query behavior.
In `@frontend/src/features/job/costing/useCostLines.ts`:
- Around line 121-140: Update deleteLine’s onError rollback comment to document
why restoring the entire snapshot is intentional: unlike patchLine, a deleted
row cannot be reconstructed field-by-field, so a wholesale restore is required
despite potentially reverting interleaved writes until invalidate refetches.
In `@frontend/src/features/job/costing/XeroQuoteCard.test.tsx`:
- Around line 72-73: Update all five tests in XeroQuoteCard.test.tsx to
destructure and reuse the user instance returned by renderWithProviders,
removing each redundant userEvent.setup() call while preserving the existing
test interactions.
In `@stubs/xero_python/accounting/__init__.pyi`:
- Around line 80-93: Update the Quote stub by declaring the line_amount_types
and currency_code attributes alongside the existing quote fields, using types
consistent with the values passed by apps/xero/provider.py. Keep the existing
constructor and serialization declarations unchanged.
🪄 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: 6120c3fb-75ad-489f-9410-9c2daf5d7eaa
⛔ Files ignored due to path filters (6)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/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 (45)
apps/accounting/management/__init__.pyapps/accounting/management/commands/__init__.pyapps/accounting/management/commands/inspect_xero_quote_pdf.pyapps/accounting/provider.pyapps/accounting/services/quote_pdf.pyapps/accounting/tests/test_quote_pdf.pyapps/accounting/types.pyapps/job/api.pyapps/job/schemas.pyapps/job/services/job_service.pyapps/job/tests/test_job_api.pyapps/xero/api.pyapps/xero/documents/base.pyapps/xero/documents/invoice.pyapps/xero/documents/quote.pyapps/xero/provider.pyapps/xero/readonly_provider.pyapps/xero/tests/test_document_api.pyapps/xero/tests/test_provider_documents.pyapps/xero/tests/test_quote_manager.pydocs/accepted-api-differences.ymldocs/rewrite-status.mdfrontend/package.jsonfrontend/schema.v2.ymlfrontend/src/api/index.tsfrontend/src/components/ui/command.tsxfrontend/src/components/ui/popover.tsxfrontend/src/features/job/JobDetailPage.tsxfrontend/src/features/job/costing/CostLineGrid.test.tsxfrontend/src/features/job/costing/CostLineGrid.tsxfrontend/src/features/job/costing/ItemSelect.tsxfrontend/src/features/job/costing/JobQuoteTab.test.tsxfrontend/src/features/job/costing/JobQuoteTab.tsxfrontend/src/features/job/costing/XeroQuoteCard.test.tsxfrontend/src/features/job/costing/XeroQuoteCard.tsxfrontend/src/features/job/costing/calc.test.tsfrontend/src/features/job/costing/calc.tsfrontend/src/features/job/costing/types.tsfrontend/src/features/job/costing/useAutosaveField.tsfrontend/src/features/job/costing/useCostLines.tsfrontend/src/lib/format.tsfrontend/src/test/setup.tsfrontend/tests/e2e/job/job-xero-quote.spec.tsscripts/v1-frontend-operations.ymlstubs/xero_python/accounting/__init__.pyi
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.
Movements all belong to this slice: two call-time manager imports and test fixtures (PLC0415), the provider's deliberate exception-to-result conversions (BLE001, returns-instead), the TableMeta module augmentation (eslint-disable), and the new manager/provider try shapes. Passthrough stays pinned at zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…est states Close the PdfReader before any unlink (a held stream fails deletion on Windows); a stock pick clears labour_subtype so a converted time line carries no stale subtype; the draft item-pick preserves absence as null instead of '' (an empty string would satisfy the persist-ready check); the debounce dispatch compares against the live server value via a ref; quote deletion asks for confirmation like a cost-line delete; a pending Xero ping reads 'Checking Xero…' instead of the logged-out label; two comments now state their rejected alternatives. Declined with evidence in the thread: the claim that xero-python 15.0.0 lacks get_quote_as_pdf. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
corrin added a commit
that referenced
this pull request
Aug 9, 2026
* Quote hardening B1-B5: explicit validation, full compensation, unbricked deletes Provider quote calls adopt the PO pattern (summarize_errors=False + element-level validation_errors checks) so a rejected status change — e.g. deleting an ACCEPTED quote — can never read as success. The post-create tail moves into _finalize_created_quote: EVERY failure after the remote write now compensates (totals validation, persist, the timestamp bump all void the orphan best-effort with the external id in the error), and the IntegrityError is discriminated by state — a same-xero_id row means the sync mirrored our own quote first and is ADOPTED (linking the job the transform never sets), never voided; only the job-constraint race voids. Deletion no longer requires a Xero-valid company (the quote row carries its own), an unsynced company refuses with a readable 400 instead of a 500, and a retained diagnostic PDF reports its path. Findings: ultrareview (2) + user review (1, 2, 5) over PR #48. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * One document-endpoint adapter: auth refusal, failure map, success build The 7x-copied scaffolding (token check, failure-to-payload mapping with the status clamp, success-invariant raise + response construction) collapses into _xero_auth_refusal/_document_failure/_document_success. Wire-identical by construction — ninja serializes every declared field, so explicit Nones equal the fields each endpoint used to omit — and the exported schema is unchanged. The error_type drift (missing only from delete_purchase_order) disappears with the copies. Full capability split of apps/xero stays a recorded backlog item, not a pre-cutover change. Findings: ultrareview sub-cap cleanup + user review (4, partial). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Grid hardening F1-F6 + assertive spec: drafts persist, retries retry Draft rows derive unit_rev from unit_cost like server rows (a filled phantom used to silently never POST); draft commits skip the send-dedupe so retyping the same value after a failed POST retries; the autosave buffer tracks dirtiness instead of copying the server value in at focus (a focus landing in the same tick as a sibling cell's state-updating blur copied a stale render's value); quantity edits make the phantom real; typed free-form rows infer adjust (v1 rule — material means a stock pick); the PATCH echo merges only its own fields so it cannot clobber an interleaved optimistic edit; a failed delete re-inserts only its line. The quote spec now hard-asserts the fresh job's line state before the repair pass and all-clear after it — a line-creation regression fails instead of being silently healed. Findings: ultrareview (3, 5, 6, 8, 9, 10) + user review (3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record the deferred structure work; regenerate metrics Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Persist the collision at its catch site; refresh derived rows The handler-contract gate rightly flagged the IntegrityError handler: its persist lived inside the delegate where the AST cannot see it. The persist moves to the catch, which is where it belonged anyway. 1716 tests, 88.52%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 9, 2026
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.
Slice 2c — the last of the three slice-2 Xero PRs. Done means the spec is green:
job-xero-quote.spec.tspassed writes-live against the demo tenant (real DRAFT quote created, native Xero PDF downloaded server-side and the configured terms text found in it), and the fullrun_e2e.shgate passed all 39 tests across the 14 ported spec files on the final code.Backend
apps/xero/documents/quote.py— expected refusals (already quoted, T&M pricing, empty quote cost set, blank breakdown descriptions, missing theme/terms config) return typed 400 values with the provider never called; unexpected failures persist once and re-raise. Total-only mode sends the cost-set summary revenue as one line; breakdown mode one sanitised line per cost line. The concurrent-push loser voids its orphan Xero quote (savepoint-guarded) before refusing; a quote deleted Xero-side comes back as a typed 404 from the provider pre-read and the manager cleans up the local row instead of bricking the job.create_quote,delete_quote(soft delete via pre-read + DELETED upsert),download_quote_pdf. Readonly fabricatesQU-E2E-*results and refuses the PDF download — a fabricated file would satisfy the text assertion against nothing.POST /api/xero/create_quote/{job_id}(body{breakdown}),DELETE /api/xero/delete_quote/{job_id}(no id parameter — one quote per job) at v1-parity URL fragments;GET /api/job/jobs/{id}/quote/serving{quote: QuoteOut | null}— enveloped because the generated axios client coerces a bare JSONnullbody to{}(ledgered, with the conditional-GET drop).apps/accounting/services/quote_pdf.py+ theinspect_xero_quote_pdfcommand emitting the single JSON line the spec parses; the file survives every diagnostic path.Frontend
features/job/costing/CostLineGrid.tsx— the one cost-line grid (estimate/actual arrive later as prop configs), on TanStack Table v8 with module-constant column defs (per-render defs would remount and blur every input). Full day-one selector contract:.smart-costlines-table, exactly one trailing phantom tbody row,SmartCostLinesTable-*/DataTable-row-*/data-grid-*from the visual index,ItemSelect-option-*, trigger namedSelect Itemonly when unbound.useAutosaveFieldderives its display value (local buffer only while editing) — an effect-synced copy provably kept rejected input on screen when an optimistic write and its rollback coalesced into one render. 600ms debounce, blur flushes and cancels, deliberately no If-Match on cost-line CRUD (v1 parity).Spec port
Recorded deviations from v1's spec: own job instead of the read-only
sharedEditJobUrlfixture; no in-spec ping (global setup fails the run closed);waitForAutosaveinstead of 800ms sleeps; PDF inspector spawned viauv run python. The repair machinery ports near-verbatim — it is what exercises the grid every run (the fresh job's material line lacks a stock binding).Review
Adversarial 2-subagent review pre-PR; all four backend should-fixes and the frontend blocker + should-fixes applied with regression tests (draft-POST failure recovery, same-value retry after rollback). Declined with reasons in the commit: the compact-match "tightening" (equivalent in power to the existing check) and the readonly totals type change (2b precedent).
Verification: 1708 unit tests, coverage 88.46%,
job-xero-quotegreen live, full E2E gate green twice (before and after review fixes).🤖 Generated with Claude Code
Summary by CodeRabbit