fix(sync): validate labelRules on read and fix two mappings-dashboard bugs - #168
Conversation
Closes the five non-blocking findings collected in #165. Three are the labelRules half of guards statusMappings and priorityMappings already had. labelRules is now validated on read. readPersistedConfig validated the other two sections per-section but passed labelRules through raw, so a malformed value was served as-is, omitted from invalidSections, and reached LabelRulesPanel which calls ruleList.map() on it. Absence is still valid -- the section is optional -- but a present value is checked like its siblings and falls back to defaults with the section named in the log. isValidLabelRulesShape now rejects an empty per-plugin array, matching isValidMappingShape. loadLabelMapper treats [] as "nothing persisted, use defaults", so saving { linear: [] } showed an empty rule list in the dashboard while the worker kept applying built-in rules -- the UI/engine divergence the sibling check exists to block. A PUT that omits labelRules no longer wipes persisted rules. The upsert replaces the whole row, so omitting the key dropped it -- the same silent wipe the {} rejection prevents, through a different door. Omitting the key now means "leave label rules alone" and carries forward what is stored; clearing requires an explicit valid value. Dashboard: PriorityMappingEntry gained a display-only "label" field, and the priority panel renders it beside the raw key. #95 moved the human text out of the persisted key (keys are now the adapter's real '0'-'4' values), which fixed the real bug but left the tab showing bare numbers -- the persistence half landed, the rendering half did not. And handleSave now clears provenance, so the "no saved mapping configuration is in effect" notice stops sitting on screen next to "Mappings saved successfully". Tests: three added, red-green verified -- with the source reverted exactly those three fail (labelRules read validation, omit-preservation, empty-array rejection) and all 13 pass with it. The two dashboard items are render-only. Verified: typecheck 10/10, tests 10/10, build 10/10.
Round-1 CR found eight issues, all in this branch's own code. Four were second-order effects of the labelRules carry-forward. The carry-forward was a read-then-write across two statements, so a concurrent PUT could land between them and lose its rules -- the same wipe the fix exists to prevent, through a narrower door. Read and write now happen inside one prisma.$transaction. An existing row whose labelRules are unusable, or whose JSON will not parse, still cannot be carried forward -- but it now says so. Previously the rules vanished with nothing in the logs, which is the failure shape this endpoint exists to remove. The client stored the request body after a successful save. With the server carrying labelRules forward, echoing the request back dropped rules that were actually persisted -- they disappeared from the editor until the next page load. The PUT response is now the source of truth, falling back to the request body if the response cannot be parsed. The display-only `label` field added for the priority panel was accepted unvalidated; a non-string reached the editor and rendered as garbage. It is now checked when present. The mock fixture still used pre-#95 cosmetic priority keys ('0 (None)') with no label -- the unmatchable-key shape #95 fixed on the persistence side. Leaving it re-canonicalized the bug in the fixture and left the new label rendering uncovered. Keys are now the adapter's real '0'..'4' values with the human text in `label`, which also resolves the docstring that contradicted it. Two weaknesses in the tests added last commit: `not.toEqual` also passes when the value is undefined, so the labelRules assertion is now positive against the defaults the endpoint serves; and console.error spies were restored at the end of each test body, so a failed assertion left console stubbed for the rest of the file -- restoration moved to afterEach. Tests: 18 across the two mapping suites, up from 13. Red-green verified -- reverting the transaction fails the atomicity test, and removing the label check fails the validation test. Call-site enumeration: - prisma.$transaction (new dependency of the mappings PUT): four test files mock prisma for this route. sync-mappings-roundtrip and sync-mappings-config-read were updated first; the full suite then caught sync-api.test.ts, and api-route-auth.test.ts needed the same. All four now provide $transaction and the mocks it calls. This is the enumeration step working after I initially ran it too narrowly -- on the route's callers rather than on every prisma mock that reaches it. - isValidMappingShape: two callers (statusMappings, priorityMappings) plus the read path; the label check is additive and cannot reject a previously valid entry, since `label` was never part of the accepted shape. - readPersistedConfig: still used by GET; the PUT path now inlines its own transactional read rather than calling it, so its behaviour is unchanged. Verified: typecheck 10/10, tests 10/10, build 10/10.
CPK-7933 Merge PR #168 — validate labelRules on read + two mappings-dashboard bugs
PR #168 — Closes outpost#165. No dependency on the #187 split — shares one file with #167, nothing else. Open since 2026-08-07. Lands on top of the sync-mapping persistence that #95 delivered, so it is the natural follow-on rather than new surface. |
jerelvelarde
left a comment
There was a problem hiding this comment.
The validation gap is real and the fix is the right shape: labelRules was optional, so absence is valid, but a present value was passed through raw straight into LabelRulesPanel's ruleList.map(...), and it was missing from invalidSections so nothing anywhere reported the problem. Validating it like the other two sections and deleting it from usable on failure makes the three consistent.
The best change here is the one that isn't in the title. The old PUT built a fresh config object and upserted it, so a request that omitted labelRules silently wiped any persisted rules — the exact silent wipe the labelRules: {} rejection exists to prevent, reachable through a different door. Treating an omitted key as "leave alone" and doing the read+write in one transaction (so a concurrent PUT can't land between them and lose its rules to whichever write is second) is correct.
Two supporting details I liked:
- When existing rules are present but unusable, they are not carried forward and that is logged. Silently dropping them there would be the same class of bug one layer down.
- The client now stores what the server persisted rather than echoing the request body. Without that, a PUT omitting
labelRuleswould have them carried forward server-side but dropped from local state — rules that are genuinely saved would vanish from the editor until the next page load. Easy one to miss.
Rejecting entries.length === 0 is justified by the right argument — loadLabelMapper reads an empty array as "nothing persisted, use the defaults", so saving { linear: [] } would show an empty rule list in the dashboard while the worker kept applying built-in rules, and a save must never produce a state where the UI and the engine disagree about what is in effect.
Two non-blocking follow-ups:
- Between that rejection and the
labelRules: {}rejection, I don't think there is any longer a way to deliberately clear label rules through this API —{}is rejected,{ linear: [] }is rejected, and omitting the key now means "keep". That is the safe direction to fail and I would not hold the PR for it, but it is worth an explicit sentinel (or a DELETE) rather than leaving the operation impossible-by-accident; right now the only route is editing theSystemConfigrow by hand. setProvenance({})on a successful save is right for the common case, but a PUT that omitslabelRuleswhen none were persisted leaves the config genuinely still on built-in defaults for that section — and the notice saying so is now cleared anyway. Deriving provenance from the response body rather than blanking it would keep it honest in that corner.
The label-must-be-a-string check and the priority-row rendering fix are both straightforwardly right — display-only fields still reach the DOM, and a non-string rendering as garbage is worth a 400.
Approving.
Fixes the five non-blocking findings collected in #165 — three are the
labelRuleshalf of guardsstatusMappingsandpriorityMappingsalready had, two are mappings-dashboard bugs.What #165 asked for
labelRuleswas not validated on read.readPersistedConfigvalidated the other two sections per-section but passedlabelRulesthrough raw, so a malformed value was served as-is, omitted frominvalidSections, and reachedLabelRulesPanel, which callsruleList.map(...)on it. Absence is still valid — the section is optional — but a present value is now checked like its siblings and falls back to defaults with the section named in the log.isValidLabelRulesShapewas missing the empty-array rejection its sibling has.loadLabelMappertreats[]as "nothing persisted, use defaults", so saving{ linear: [] }showed an empty rule list in the dashboard while the worker kept applying built-in rules — the UI/engine divergence the sibling check exists to block.A PUT that omitted
labelRuleswiped persisted rules. The upsert replaces the whole row, so omitting the key dropped it. Omitting it now means "leave label rules alone".The priority
labelfield was never rendered. #95 moved the human text out of the persisted key (keys are the adapter's real'0'–'4'values), which fixed the real bug but left the priority tab showing bare numbers.PriorityMappingEntrygained a display-onlylabel; the panel renders it beside the key.handleSavenever clearedprovenance, so "no saved mapping configuration is in effect" sat on screen next to "Mappings saved successfully".What review then found in that fix
A cr-loop round found eight issues, all in this branch's own code — four of them second-order effects of the carry-forward:
prisma.$transaction.labelRulesare unusable, or whose JSON won't parse, still can't be carried — but it now logs, rather than having rules vanish with nothing anywhere.labelfield was accepted unvalidated — a non-string reached the editor.'0 (None)') with nolabel, re-canonicalizing the shape Finish sync mapping persistence + bulk force-sync, fix dead Linear adapter registration #95 fixed and leaving the new rendering uncovered.not.toEqualalso passes when the value isundefined, andconsole.errorspies were restored at the end of each body, so a failed assertion left console stubbed for the rest of the file.Verification
pnpm typecheck10/10 ·pnpm test10/10 ·pnpm build10/10 · 18 tests across the two mapping suites, up from 13.Red-green verified: reverting the transaction fails the atomicity test; removing the
labelcheck fails the validation test; and on the original three, reverting the source fails exactly those three.One process note worth recording: adding
prisma.$transactionbroke two test suites I hadn't looked at. I enumerated the route's callers but not every prisma mock that reaches it — the full suite caughtsync-api.test.ts, andapi-route-auth.test.tsneeded the same. All four suites now provide it.Not in this PR
The remaining #165 items are the
MAPPING_CONFIG_KEY+ sharedparseMappingConfig()consolidation, which was already deferred out of #95 and is a better fit alongside it than bolted here.Closes #165.