137676bf - Guard every /compliance path, including ones no screen claims - #1307
Conversation
…uard The suite for this screen mocked useComplianceGuard away and never asserted the call, so removing the guard from the screen left all eight tests green. That is the blind spot #1306 suspected, and it survived review because nothing pointed at it. Verified by mutation: with the guard call commented out, this new test fails and only this test fails; restored, the suite is green again. Refs #1306
Review pointed out the quieter mutation the first version missed: the hook is useComplianceGuard(redirectPath = '/', isActive = true) and its effect navigates only while isActive, so useComplianceGuard(undefined, false) would keep the call in place and still leave the guard inert — exactly the "guard present but ineffective" shape #1306 started from. The mock now forwards its arguments and the test asserts an empty argument list. Verified by mutation on both shapes: removing the call and passing (undefined, false) each fail this test and only this test.
|
EN: Three review passes, the last two triggered by findings this branch fixed — a guard assertion that ignored its arguments, then the catch-all route that closes the underlying hole. DetailsPass 1 — both dimensions clean on the first commit, which added the missing guard assertion to the chargeback screen test. Pass 2 — the logic dimension pointed out that the guard mock discarded its arguments, so Pass 3 — covers the product fix: a guarded catch-all for |
A path below /compliance that matches no route falls through to the router's errorElement. That screen calls no guard, and an error boundary leaves the URL alone — so an unauthorised role stays parked on a /compliance address, which is what #1306 saw. The role check existed only where a screen existed. A guarded catch-all closes that: 'compliance/*' as the last compliance route, rendering a screen that runs useComplianceGuard() and otherwise says the page does not exist. React Router ranks static segments above a splat, so it matches only what no screen claims — verified per URL by the lazy chunk that loads. Paths outside /compliance keep their current behaviour.
Closes #1306.
The defect
A URL below
/compliancethat matches no route — a typo, a screen not deployed yet, a screen removed —makes React Router raise a 404
ErrorResponseand render the rooterrorElement(ErrorScreen). Thatscreen runs outside every guard, and an error boundary does not change the URL. An unauthorised role
therefore sits on a
/compliance/…address and is never sent away.That is what #1306 observed. The report named
/compliance/pending-chargebacks, and the guard on thatscreen turned out to be intact — the run had been made against a build without the route
(
feat/e2e-stackbranches off #1282, the screen landed with #1285, and the harness builds the bundle fromits own working tree). But the hole underneath is real and independent of that one route: the role check
for the compliance area only exists where a screen exists.
The fix
A guarded catch-all as the last compliance route:
src/screens/compliance-not-found.screen.tsx— callsuseComplianceGuard()like every other compliancescreen, then states plainly that the page does not exist and shows the requested path.
src/App.tsx—path: 'compliance/*', placed after all compliance routes. React Router ranks staticsegments above a splat, so it only ever matches what no screen claims.
src/__tests__/compliance-not-found.screen.test.tsx— pins both halves: the guard call (with an emptyargument list) and the rendered message plus requested path.
The role check for
/complianceno longer depends on whether a given screen exists, and an authoriseduser gets "this compliance page does not exist" instead of "something went wrong".
Behaviour outside
/complianceis untouched: an unknown path elsewhere still renders the generic errorscreen exactly as before.
Second change: the guard assertion the screen tests were missing
src/__tests__/compliance-chargeback-list.screen.test.tsxmockeduseComplianceGuardaway and neverasserted the call — deleting the guard from the screen left all eight tests green. It now asserts the call
with an empty argument list, because
useComplianceGuard(redirectPath = '/', isActive = true)navigates only while
isActive:useComplianceGuard(undefined, false)would keep the call and still leavethe guard inert.
Evidence
Measured against a running dev server, role taken from a JWT (the frontend decodes it client-side, so the
guard decision involves no backend), polling
location.pathname:/compliance/definitiv-nicht-existent(invented)//compliance/pending-chargebacks///compliance/recalls///voellig-unbekannt-ausserhalb(outside/compliance)The splat claims nothing that belongs to a real screen — verified by which lazy chunk each URL loads:
Mutation checks on the guard assertion, each restored afterwards: removing
useComplianceGuard();andchanging it to
useComplianceGuard(undefined, false);each fail that one test and only that test.Full suite: 81 suites, 950 tests, all green. ESLint clean on both new files.
Coverage
src/screens/compliance-not-found.screen.tsx— 100 % statements / 100 % branches / 100 % functions /100 % lines.
src/screens/compliance-chargeback-list.screen.tsx(untouched, measured with the new assertion inplace) — 100 % / 100 % / 100 % / 100 %.
src/App.tsxis touched by two insertions (a lazy import and a route entry). It is the routing table,has no test of its own in this repository, and is not brought to 100 % here — a declared deviation
from the coverage rule in CONTRIBUTING.md, and the reviewer's call. The two added lines are exercised in
the browser evidence above.
Note for #1288
Once that branch is rebased onto a
developcontaining #1285,/compliance/pending-chargebacksbelongs inALL_COMPLIANCE_PATHSas a normal assertion and bothtest.fail()markers can go: the redirect assertionnow passes through this catch-all even while a screen is missing, and the mount assertion passes once the
screen is present.