Skip to content

Commit 0f20f60

Browse files
Rylan-cgiclaude
andcommitted
refactor: clear three SonarQube smells, and pin what one of them exposed
schedule1/index.tsx L414/L422 — two three-deep nested ternaries extracted into named helpers with early returns. Sonar is right that these were confusing, and in a specific way: they hid that `derived` has the OPPOSITE precedence in the two figures. For cost, the mirror changes exactly one of three branches (only 140); for the rate, it supersedes every row including 139 and 140. Same two codes, opposite answer — legitimately, because deriveSchedule1 computes a rate for 139 (its volume is user-entered) but no cost for it (the cost is a Schedule 3 pull). Now stated in a comment instead of implied by nesting depth. That refactor turned up a coverage gap: inverting the rate precedence left all 268 schedule1 tests green, so the rule was pinned by nothing. Added a test that asserts 139's rate moves with an entered volume while its cost stays the Schedule 3 pull — mutation-verified, it fails on the inversion. schedule5SubPage/derived.ts L71 — `stampedVolume ?? null` becomes a defaulted `volume` parameter. Equivalent: a default fires only on `undefined`, and every call site passing an explicit `null` already wanted `null`. No behaviour change intended in any of the three. Suite 1291/1293 (two timeout flakes, both pass in isolation), zero unhandled errors, build clean, and zero type errors in the touched files against an origin/main baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent e666ac8 commit 0f20f60

3 files changed

Lines changed: 58 additions & 20 deletions

File tree

frontend/src/components/schedule1/__tests__/Schedule1.test.tsx

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,34 @@ describe('Schedule1 editable page', () => {
171171
expect(costOf(SUBTOTAL)).toBe('674,000')
172172
})
173173

174+
test("139's RATE is mirrored while its COST stays the Schedule 3 pull (#291)", async () => {
175+
// Row 139's two halves come from different places, and the page treats them differently on
176+
// purpose: the cost is pulled from Schedule 3 and nothing on this page feeds it, but the VOLUME is
177+
// user-entered -- so `deriveSchedule1` computes a rate for 139 (derived.ts:98) even though it
178+
// computes no cost for it. The mirror therefore supersedes 139's rate and must NOT touch its cost.
179+
//
180+
// Written after a SonarQube refactor of a three-deep ternary (2026-08-21) exposed that this
181+
// opposite-precedence rule was pinned by no test: inverting it left all 268 green.
182+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
183+
render(<Schedule1 />)
184+
const user = userEvent.setup()
185+
186+
const label = 'Less Silviculture Admin Costs'
187+
// Served state: 150,000 pulled from Schedule 3 over the fixture's volume of 55.
188+
expect(await screen.findByLabelText(`${label} volume`)).toHaveValue('55')
189+
expect(rate(label)).toBe('2,727.27') // 150,000 / 55
190+
191+
const volume = screen.getByLabelText(`${label} volume`)
192+
await user.clear(volume)
193+
await user.type(volume, '60000')
194+
await user.tab()
195+
196+
// The rate moved off the served figure -- the mirror owns it.
197+
expect(rate(label)).toBe('2.50') // 150,000 / 60,000
198+
// ...while the cost is still the Schedule 3 pull, untouched by the mirror.
199+
expect(costOf(label)).toBe('150,000')
200+
})
201+
174202
test('the Other Costs $/m³ tracks the volume entered on this page (#291)', async () => {
175203
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
176204
render(<Schedule1 />)

frontend/src/components/schedule1/index.tsx

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -401,29 +401,37 @@ const Schedule1: FC = () => {
401401
)
402402
}
403403

404+
// 139's cost is pulled from Schedule 3 and stays served even mid-entry, because nothing on this
405+
// page is an input to it; only 140's cost has a mirror. Extracted from a nested ternary (SonarQube
406+
// 2026-08-21) -- worth doing, because the nesting hid that `derived` changes exactly ONE of these
407+
// three branches.
408+
const silvicultureCost = (code: number, item: LineItem | null): number | null | undefined => {
409+
if (code === 139) return data.lessSilvAdminCost
410+
if (code === 140) return derived ? derived.totalSilvicultureCost : data.totalSilvicultureCost
411+
return item?.cost
412+
}
413+
414+
// $/m³ = cost ÷ volume (139/140 fold in the Schedule 3 pulls). Mirrored while editable so it
415+
// tracks entry; the document's server-computed figure otherwise (#291).
416+
//
417+
// NOTE the precedence is deliberately the OPPOSITE of `silvicultureCost` above: the mirror
418+
// supersedes every row here, 139 and 140 included, because `deriveSchedule1` computes a rate for
419+
// both (derived.ts:98,115) even though it computes a cost only for 140. Same two codes, different
420+
// answer -- which is precisely what a three-deep ternary is bad at showing.
421+
const silviculturePerUnit = (code: number, item: LineItem | null): number | null | undefined => {
422+
if (derived) return derived.perUnit[code]
423+
if (code === 139) return data.lessSilvAdminPerUnit
424+
if (code === 140) return data.totalSilviculturePerUnit
425+
return item?.perUnit
426+
}
427+
404428
const silvicultureRow = (row: (typeof SILV_ROWS)[number]) => {
405429
const item = data.silviculture[row.key]
406430
// All four silviculture VOLUMES are user-entered; only 1 & 2 have an editable cost. 139's cost is
407431
// pulled from Schedule 3, 140's is derived — both read-only.
408432
const writableCost = row.code === 1 || row.code === 2
409-
// 139's cost is pulled from Schedule 3; 140's is the derived Total Silviculture cost (both read-only).
410-
const costValue =
411-
row.code === 139
412-
? data.lessSilvAdminCost
413-
: row.code === 140
414-
? derived
415-
? derived.totalSilvicultureCost
416-
: data.totalSilvicultureCost
417-
: item?.cost
418-
// $/m³ = cost ÷ volume (139/140 fold in the Schedule 3 pulls). Mirrored while editable so it
419-
// tracks entry; the document's server-computed figure otherwise (#291).
420-
const perUnitValue = derived
421-
? derived.perUnit[row.code]
422-
: row.code === 139
423-
? data.lessSilvAdminPerUnit
424-
: row.code === 140
425-
? data.totalSilviculturePerUnit
426-
: item?.perUnit
433+
const costValue = silvicultureCost(row.code, item)
434+
const perUnitValue = silviculturePerUnit(row.code, item)
427435
return (
428436
<TableRow key={row.code}>
429437
<TableCell>{row.label}</TableCell>

frontend/src/components/schedule5SubPage/derived.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,11 @@ export function rowCostPerVolume(
6666
export function deriveSubPageTotals(
6767
kind: SubPageKind,
6868
rows: readonly SubPageRowForm[],
69-
stampedVolume: number | null | undefined,
69+
// Defaulted rather than normalised in the body (SonarQube 2026-08-21). Equivalent: a default fires
70+
// only on `undefined`, and an explicitly-passed `null` already IS the value the body wants — so
71+
// both the omitted and the null call paths land on `null`, as `?? null` did.
72+
volume: number | null = null,
7073
): SubPageTotals {
71-
const volume = stampedVolume ?? null
7274
let cost = 0
7375
let contributed = false
7476
for (const row of rows) {

0 commit comments

Comments
 (0)