Estimate tab: the grid's second config — create-estimate-entry green (15 of 40) - #50
Conversation
A thin kind=estimate CostLineGrid page (renders for every pricing
methodology — T&M jobs estimate too), lazy-wired. Numeric inputs display
wire decimals trimmed ('3.000' → '3'): typed values must round-trip as
typed for the estimate spec's string-equality assertions. The spec's
Tab chain (desc → quantity → unit cost → unit rev) is asserted to hold in
natural DOM order — no custom Tab handler to drift out of sync.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Eager persist-on-completion swapped the draft row out from under the
focused unit-rev cell the moment a cost commit derived the revenue —
exactly the race v1's row-exit rule existed to prevent ('rapid edits to
Unit Revenue cannot be overwritten by an earlier POST response'). Field
commits now only update the draft; the tr's focusout posts when focus
leaves the whole row, and item picks still persist immediately.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Deviations from v1, recorded in the file: the shared job is created by the first serial test through the standard authenticated fixture (no hand-rolled beforeAll login); waitForAutosave replaces every sleep, armed before the action that triggers the write; the row-exit gesture is a click on the section heading (v1's custom Tab handler moved to the next row — ours follows natural DOM order within the row); post-pick row finds retry via toPass instead of v1's fixed sleeps, which papered over the draft-to-server row swap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ep prose The row-exit commit is deferred by a 0ms timer that any focus landing back in the row's React tree cancels — the ItemSelect popover portals outside the tr in the DOM, so the old relatedTarget containment check read opening YOUR OWN picker as leaving the row, POSTing a complete draft as an adjustment and silently discarding the pick. Draft deletion moves to pointerdown (Safari does not focus buttons on click, so the preceding null-relatedTarget blur would create the line being deleted). parseDecimalInput canonicalises like the display so the send-dedupe compares like with like (re-entering '25.00' over a shown '25' no longer PATCHes and wipes an overridden revenue). Labour picks keep a user-typed description (v1's rule) and the draft pick drops a stale stock binding. trimDecimal guards to fixed-point forms. Ledgered: UI-created estimate time lines store NULL xero_pay_item (load-bearing only for actual lines). Findings: estimate-slice adversarial pair (1 blocker, 4 should-fix, 2 nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warning Review limit reached
Next review available in:45 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 (2)
📝 WalkthroughWalkthroughThe PR adds an estimate tab, estimate cost-line interactions, deferred row-exit persistence, labour-rate description handling, decimal normalization, integration tests, E2E coverage, and rewrite-status documentation. ChangesEstimate costing workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JobDetailPage
participant JobEstimateTab
participant JobDataAPI
participant CostLineGrid
participant PlaywrightE2E
JobDetailPage->>JobEstimateTab: open estimate tab
JobEstimateTab->>JobDataAPI: load job data and defaults
JobDataAPI-->>JobEstimateTab: return estimate configuration
JobEstimateTab->>CostLineGrid: render estimate rows
PlaywrightE2E->>CostLineGrid: create or edit cost line
CostLineGrid->>JobDataAPI: persist on row exit
JobDataAPI-->>CostLineGrid: return saved row state
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: 2
🧹 Nitpick comments (4)
frontend/src/features/job/costing/JobEstimateTab.test.tsx (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test to match what it asserts.
Nothing in this test varies the pricing methodology, and
JobEstimateTabdoes not readjob.pricing_methodology. The "renders for every pricing methodology" rule lives in the tab bar andJobDetailPage, which decide whether the estimate tab is offered. As written, the name suggests coverage that this file does not provide.♻️ Proposed rename
- it('renders the estimate grid for any pricing methodology', async () => {+ it('renders the estimate cost-line grid from company defaults', async () => {🤖 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/JobEstimateTab.test.tsx` at line 36, Rename the test case around JobEstimateTab to describe only the estimate grid rendering behavior it actually asserts, removing the claim that it covers every pricing methodology. Leave the test implementation unchanged.frontend/src/features/job/costing/CostLineGrid.test.tsx (1)
616-620: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
?? document.bodyfallback weakens this test.
fireEvent.pointerDownremoves the draft synchronously, so React has already unmounted the unit-cost input by the time this line runs. ThequerySelectorreturnsnulland the fallback blursdocument.body. The rowtrno longer exists, so itsonBlurnever runs and the row-exit commit path is never exercised. The test then proves only that a removed row does not POST, which is weaker than the stated intent.Assert the unmount explicitly instead of hiding it behind a fallback, so the test states what it verifies.
💚 Proposed change
// pointerdown removes the draft before any blur-driven commit runs. fireEvent.pointerDown( document.querySelector<HTMLElement>('[data-automation-id="SmartCostLinesTable-delete-0"]')!, ) - fireEvent.blur(- document.querySelector<HTMLInputElement>(- '[data-automation-id="SmartCostLinesTable-unit-cost-0"]',- ) ?? document.body,- )+ // The row is already gone, so the blur that Safari fires next lands on+ // nothing: there is no tr left to schedule a row-exit commit.+ expect(+ document.querySelector('[data-automation-id="SmartCostLinesTable-unit-cost-0"]'),+ ).toBeNull()+ fireEvent.blur(document.body)🤖 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 616 - 620, Update the test around the unit-cost input query to assert that the input has been unmounted after fireEvent.pointerDown, rather than falling back to document.body for fireEvent.blur. Keep the test focused on verifying the intended post-removal behavior and ensure it does not imply that the row-exit onBlur commit path still executes.frontend/src/features/job/costing/calc.test.ts (1)
148-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion for the generic
'Labour'description.
labourPickDeschas three auto-fill branches: blank, the literal'Labour', and another subtype's name. This test covers blank (via line 132) and the other-subtype case. The'Labour'branch has no assertion, so a future reader can delete it without a failure.💚 Proposed addition
// Another subtype's auto-fill is replaced, not kept. expect( labourPickPatch(line({ desc: 'Office' }), { rate: labourRate(), wageRate: '38.00', allRates: rates, }).desc, ).toBe('Workshop') + // The generic v1 placeholder is auto-fill too, though no rate is named it.+ expect(+ labourPickPatch(line({ desc: 'Labour' }), {+ rate: labourRate(),+ wageRate: '38.00',+ allRates: rates,+ }).desc,+ ).toBe('Workshop') })🤖 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 148 - 172, Add an assertion in the “keeps a user-authored description (v1 rule)” test covering a line whose description is exactly “Labour”, and verify labourPickPatch replaces it with the expected selected subtype description (“Workshop”), preserving the existing blank and other-subtype cases.frontend/src/features/job/costing/CostLineGrid.tsx (1)
497-510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
labourPickPatchin the draft branch.This block re-implements
labourPickPatchinline: samekind, samelabour_subtype, samelabourPickDesccall, same wage/charge-out assignment, samestock_idstrip. The stock branch directly above already avoids the copy by callingstockPickPatch(draftAsLine, ...). Two copies of the labour rule will drift the next time the rule changes.
draftAsLinealready carriesdescandext_refs, so the helper produces the same values.♻️ Proposed refactor
onPickLabour={(rate: JobLabourRateOut, allRates: readonly JobLabourRateOut[]) => { - // Same rules as the server-row patch: keep a user-authored desc,- // and drop any stale stock binding from a failed material pick.- const { stock_id: _dropped, ...keptRefs } = gridRow.draft.ext_refs- context.updateDraft(gridRow.localId, {- kind: 'time',- labour_subtype: rate.labour_subtype,- desc: labourPickDesc(gridRow.draft.desc, rate, allRates),- unit_cost: context.wageRate,- unit_rev: rate.charge_out_rate,- ext_refs: keptRefs,- })+ // One rule for both row types: the helper keeps a user-authored+ // desc and drops a stale stock binding from a failed material pick.+ const patch = labourPickPatch(draftAsLine, {+ rate,+ wageRate: context.wageRate,+ allRates,+ })+ context.updateDraft(gridRow.localId, {+ kind: 'time',+ labour_subtype: rate.labour_subtype,+ desc: patch.desc ?? '',+ unit_cost: typeof patch.unit_cost === 'string' ? patch.unit_cost : null,+ unit_rev: typeof patch.unit_rev === 'string' ? patch.unit_rev : null,+ ext_refs: patch.ext_refs ?? {},+ }) context.commitDraftField(gridRow.localId) }}The
labourPickDescimport then becomes unused in this file.As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."
🤖 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 497 - 510, Replace the inline draft update in the onPickLabour handler with the existing labourPickPatch helper, passing draftAsLine and the selected rate data so it preserves the same description, labour subtype, costs, and stock_id removal. Keep the subsequent context.updateDraft and commit flow intact, and remove the now-unused labourPickDesc import.Source: Coding guidelines
🤖 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 `@frontend/src/features/job/costing/CostLineGrid.tsx`:
- Around line 566-576: Update the draft-delete onPointerDown handler for
gridRow.type === 'draft' to call context.removeDraft only when the pointer event
represents a primary-button press, preserving the existing Safari blur-race
behavior while preventing right/middle clicks and non-primary pointer
interactions from deleting the draft.
- Around line 113-135: Add an unmount cleanup effect near rowExitTimersRef,
using the existing timer map to clear every pending timeout and remove its
entries when CostLineGrid unmounts. Update the React import to include
useEffect, without changing the existing scheduling or cancellation behavior.
---
Nitpick comments:
In `@frontend/src/features/job/costing/calc.test.ts`:
- Around line 148-172: Add an assertion in the “keeps a user-authored
description (v1 rule)” test covering a line whose description is exactly
“Labour”, and verify labourPickPatch replaces it with the expected selected
subtype description (“Workshop”), preserving the existing blank and
other-subtype cases.
In `@frontend/src/features/job/costing/CostLineGrid.test.tsx`:
- Around line 616-620: Update the test around the unit-cost input query to
assert that the input has been unmounted after fireEvent.pointerDown, rather
than falling back to document.body for fireEvent.blur. Keep the test focused on
verifying the intended post-removal behavior and ensure it does not imply that
the row-exit onBlur commit path still executes.
In `@frontend/src/features/job/costing/CostLineGrid.tsx`:
- Around line 497-510: Replace the inline draft update in the onPickLabour
handler with the existing labourPickPatch helper, passing draftAsLine and the
selected rate data so it preserves the same description, labour subtype, costs,
and stock_id removal. Keep the subsequent context.updateDraft and commit flow
intact, and remove the now-unused labourPickDesc import.
In `@frontend/src/features/job/costing/JobEstimateTab.test.tsx`:
- Line 36: Rename the test case around JobEstimateTab to describe only the
estimate grid rendering behavior it actually asserts, removing the claim that it
covers every pricing methodology. Leave the test implementation 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: fc6032f3-2181-4960-aa12-5e03f633f38f
📒 Files selected for processing (11)
docs/accepted-api-differences.ymldocs/rewrite-status.mdfrontend/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/JobEstimateTab.test.tsxfrontend/src/features/job/costing/JobEstimateTab.tsxfrontend/src/features/job/costing/calc.test.tsfrontend/src/features/job/costing/calc.tsfrontend/tests/e2e/job/create-estimate-entry.spec.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.
…egen Draft deletion only acts on a primary-button pointerdown; pending row-exit timers clear on unmount so a mid-blur tab navigation cannot fire a POST into a gone grid; the status table row catches up with the new ledger entry (the regen was missed after appending it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
create-estimate-entry.spec.tsis green — all 8 serial tests, and the fullrun_e2e.shgate passed 47/47 across the 15 ported spec files on the final code.What shipped
JobEstimateTab: a thinkind=estimateconfig of the oneCostLineGrid(renders for every pricing methodology — T&M jobs estimate too), lazy-wired.3, not the wire's3.000— typed values round-trip as typed (the spec asserts string equality), andparseDecimalInputcanonicalises the same way so the send-dedupe compares like with like.Review round (adversarial pair, all applied)
nextLabourDescrule) and drop stale stock bindings.xero_pay_item(traced: load-bearing only for actual lines; job-creation-seeded lines still carry the default — recorded so neither side gets "fixed" unilaterally).Spec port deviations (recorded in the file)
Shared job created by the first serial test via the standard fixture;
waitForAutosavereplaces every sleep, armed before the triggering action; row exit via a heading click; post-pick row finds retry viatoPass(v1's sleeps papered over the draft→server row swap); a loud guard for greps of later serial tests.100 frontend unit tests; backend untouched.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements