fix(404): ship the app's own not-found boundaries, with focus that survives a soft nav - #78
Conversation
…rvives a soft nav
There was no `not-found.tsx` anywhere in `src/`, so `payouts/[id]/page.tsx`'s
`notFound()` fell through to Next's built-in `HTTPAccessErrorFallback`. That
page renders *inside* `RootLayout` and injects
`body{color:#000;background:#fff;margin:0}`, which wins on source order over
`background: var(--void)` — both of those colours banned by name in DESIGN.md.
Its dark branch keys off the OS preference rather than the `colorScheme:"dark"`
this app declares, so a member on a light-mode OS got a full-bleed white screen
from a dark-only app. It also sets `font-family: system-ui` inline, losing both
self-hosted faces, and contains zero links: the first Tab left the document and
the only exits were Back and the URL bar.
Shipping any file here means that fallback is never reached, under 15 or 16 —
which matters while `package.json` (^16.3.0) and the lockfile (15.5.22) still
disagree.
Two files, not one. The segment-scoped `payouts/[id]/not-found.tsx` is
reachable only through `page.tsx`, which calls `requirePayoutReader()` *before*
`notFound()` — so everyone who lands on it has already cleared the guard, and
it can offer `/payouts` in the nav and as its primary action without reading
the session. The root boundary has no such proof and sends everyone to
`/account`. That difference is the whole point: the path that makes this a
blocker is a member clicking a since-deleted row in the operations list, and
the segment file puts them back in the list rather than at their account page.
Neither file reads the session, deliberately — `/_not-found` builds as `○
(Static)`, and adding `cookies()` would change that. The navs are fixed and
minimal.
Focus is the other half. That row click is a *soft* navigation (`/payouts` is
the app's only `next/link` call site), so there is no document load, and the
pressed link unmounts out from under the focus ring, leaving focus on `<body>`.
The App Router's own `focus()` call targets the first element of the changed
segment — the `<header>` — which is not focusable, so it is a no-op. Hence
`FocusHeading`: an `h1` with `tabIndex={-1}` that focuses on mount, which both
lands the keyboard inside the new content and gets the page's name announced.
This is the mechanism `admin/sync/page.tsx` argues for in writing, applied to
the case where the inserted element *is* the whole page.
Two things measured rather than assumed, both recorded in comments:
- The root boundary's `metadata` export applies; the segment-scoped one's does
not. `page.tsx`'s own metadata survives its `notFound()`, so that tab still
reads "Payout operation". Correcting it needs `generateMetadata` and a second
lookup on a working page — left as follow-up, and cosmetic next to the
announcement, which focus carries either way.
- Next's route announcer was seen reading the `h1` rather than the stale title,
because React is mid-swap on the hoisted `<title>` when its effect fires.
That is a race that lands well, not a contract, so the suite does not pin it.
e2e covers the soft-nav arrival end to end (list open, row deleted behind it,
click, assert boundary + focus + exit), the 404 status surviving a custom
component, the ground and typeface not being the injected ones, the pasted
truncated uuid, and that the payouts boundary stays behind the payouts guard.
Refs: design sweep SYNTHESIS.md issue 3 (error-boundary-audit #1, blocker;
error-boundary-critique #1, major).
📝 WalkthroughWalkthroughAdded custom root and payout-operation 404 pages. Added focused heading behavior, navigation, authorization handling, and Playwright coverage for status, metadata, routing, focus, and error boundaries. Changes404 pages
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Member
participant PayoutOperationPage
participant PayoutOperationNotFound
Member->>PayoutOperationPage: open payout operation
PayoutOperationPage->>PayoutOperationNotFound: render missing operation boundary
PayoutOperationNotFound->>Member: show recovery link and focused heading
Member->>PayoutOperationNotFound: select All operations
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/not-found.spec.ts`:
- Around line 143-151: Update the not-found E2E coverage to use a genuinely
truncated UUID and add a separate assertion for that invalid identifier,
ensuring it produces the “No such operation” response rather than reaching the
database or rendering “Something broke.” Preserve the existing complete-UUID
missing-record case separately.
- Around line 121-126: Update the focus assertion in the not-found test to use
the retrying toBeFocused() matcher on the heading, rather than reading
document.activeElement once via page.evaluate. Keep the existing “No such
operation” text assertion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90b6d4c7-f684-4e34-9bbe-be8c89f8f1fa
📒 Files selected for processing (4)
e2e/not-found.spec.tssrc/app/_components/focus-heading.tsxsrc/app/not-found.tsxsrc/app/payouts/[id]/not-found.tsx
| const focused = await page.evaluate(() => ({ | ||
| tag: document.activeElement?.tagName, | ||
| text: document.activeElement?.textContent, | ||
| })); | ||
| expect(focused.tag).toBe("H1"); | ||
| expect(focused.text).toBe("No such operation"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Playwright Test version 1.62.1 support the auto-retrying assertion await expect(locator).toBeFocused()?
💡 Result:
Yes, Playwright Test version 1.62.1 supports the auto-retrying assertion await expect(locator).toBeFocused [1][2][3]. As a standard "web-first" assertion, toBeFocused is designed to automatically retry until the condition is met or the specified timeout is reached [4][3]. This behavior has been a core feature of Playwright's locator assertions since it was introduced in version 1.20 [2]. The assertion functions by periodically re-testing the element's state, making it highly effective for handling asynchronous UI updates without requiring manual waits [4]. Playwright version 1.62.1 was released on July 30, 2026, primarily to address critical regressions [5][6]. It maintains full compatibility with the established API, including all existing auto-retrying assertions [6][1][2].
Citations:
- 1: https://playwright.dev/docs/test-assertions
- 2: https://playwright.dev/docs/api/class-locatorassertions
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/test-assertions-js.md
- 4: https://playwright.dev/docs/api/class-playwrightassertions
- 5: https://github.com/microsoft/playwright/releases/tag/v1.62.1
- 6: https://www.skakarh.com/blog/playwright-1-62-1-released
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
cat -n e2e/not-found.spec.ts | sed -n '90,160p'
printf '%s\n' '--- focus implementation and references ---'
rg -n -S 'FocusHeading|No such operation|notFound\\(|toBeFocused|not-found' . \
-g '!node_modules' -g '!dist' -g '!build' | head -200
printf '%s\n' '--- Playwright version declarations ---'
rg -n -S '"`@playwright/test`"|"playwright"' package.json package-lock.json pnpm-lock.yaml yarn.lock .github 2>/dev/null | head -100Repository: guarzo/authGD
Length of output: 4537
🏁 Script executed:
#!/bin/bash
set -eu
cat -n e2e/not-found.spec.ts | sed -n '90,160p'
rg -n -S 'FocusHeading|No such operation|notFound\\(|toBeFocused|not-found' . \
-g '!node_modules' -g '!dist' -g '!build' | head -200
rg -n -S '"`@playwright/test`"|"playwright"' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -100Repository: guarzo/authGD
Length of output: 4376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focus symbols ---'
rg -n -F 'FocusHeading' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'No such operation' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'notFound(' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'toBeFocused' . -g '!node_modules' -g '!dist' -g '!build' || true
printf '%s\n' '--- relevant TypeScript and package metadata files ---'
git ls-files | rg '(^|/)(package\.json|.*not-found.*|.*Focus.*|.*focus.*)\.(json|ts|tsx|js|jsx)$' | head -100
printf '%s\n' '--- exact Playwright lockfile entry ---'
sed -n '2045,2075p' package-lock.jsonRepository: guarzo/authGD
Length of output: 3806
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -F 'FocusHeading' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'No such operation' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'notFound(' . -g '!node_modules' -g '!dist' -g '!build' || true
rg -n -F 'toBeFocused' . -g '!node_modules' -g '!dist' -g '!build' || true
git ls-files | rg '(^|/)(package\.json|.*not-found.*|.*Focus.*|.*focus.*)\.(json|ts|tsx|js|jsx)$' | head -100
sed -n '2045,2075p' package-lock.jsonRepository: guarzo/authGD
Length of output: 3689
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FocusHeading implementation ---'
cat -n src/app/_components/focus-heading.tsx
printf '%s\n' '--- payouts not-found boundary ---'
cat -n src/app/payouts/'[id]'/not-found.tsx
printf '%s\n' '--- existing retrying focus assertions ---'
cat -n e2e/admin.spec.ts | sed -n '225,260p'Repository: guarzo/authGD
Length of output: 9208
🏁 Script executed:
#!/bin/bash
set -eu
cat -n src/app/_components/focus-heading.tsx
cat -n src/app/payouts/'[id]'/not-found.tsx
cat -n e2e/admin.spec.ts | sed -n '225,260p'Repository: guarzo/authGD
Length of output: 9094
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
component = Path("src/app/_components/focus-heading.tsx").read_text()
test = Path("e2e/not-found.spec.ts").read_text()
admin = Path("e2e/admin.spec.ts").read_text()
package_lock = Path("package-lock.json").read_text()
focus_effect = bool(re.search(r"useEffect\s*\(\s*\(\)\s*=>\s*\{.*?\.focus\(\)", component, re.S))
visible_pos = test.find('toBeVisible();', test.find('name: "No such operation"'))
evaluate_pos = test.find('page.evaluate', visible_pos)
one_shot_assertions = bool(re.search(r'page\.evaluate\(\(\)\s*=>\s*\(\{', test))
proposed_api_used = '.toBeFocused()' in admin
locked_version = bool(re.search(r'"node_modules/@playwright/test":\s*\{\s*"version":\s*"1\.62\.1"', package_lock))
print(f"FocusHeading focuses in useEffect: {focus_effect}")
print(f"Visibility assertion precedes page.evaluate: {0 <= visible_pos < evaluate_pos}")
print(f"Current focus check uses one-shot page.evaluate: {one_shot_assertions}")
print(f"Repository already uses Locator.toBeFocused(): {proposed_api_used}")
print(f"Lockfile contains `@playwright/test` 1.62.1: {locked_version}")
PYRepository: guarzo/authGD
Length of output: 395
Use a retrying focus assertion.
FocusHeading applies focus in useEffect, so the visibility assertion can pass before the heading receives focus. Replace the one-shot check with toBeFocused().
Proposed test change
- const focused = await page.evaluate(() => ({
- tag: document.activeElement?.tagName,
- text: document.activeElement?.textContent,
- }));
- expect(focused.tag).toBe("H1");
- expect(focused.text).toBe("No such operation");
+ await expect(
+ page.getByRole("heading", { name: "No such operation" }),
+ ).toBeFocused();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const focused = await page.evaluate(() => ({ | |
| tag: document.activeElement?.tagName, | |
| text: document.activeElement?.textContent, | |
| })); | |
| expect(focused.tag).toBe("H1"); | |
| expect(focused.text).toBe("No such operation"); | |
| await expect( | |
| page.getByRole("heading", { name: "No such operation" }), | |
| ).toBeFocused(); |
🤖 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 `@e2e/not-found.spec.ts` around lines 121 - 126, Update the focus assertion in
the not-found test to use the retrying toBeFocused() matcher on the heading,
rather than reading document.activeElement once via page.evaluate. Keep the
existing “No such operation” text assertion.
| // A well-formed uuid that resolves to nothing — the shape a link preview | ||
| // leaves behind. `getPayoutOperationDetail` returns null and `page.tsx` | ||
| // calls `notFound()`. | ||
| const res = await page.goto("/payouts/00000000-0000-4000-8000-000000000000"); | ||
|
|
||
| expect(res?.status()).toBe(404); | ||
| await expect(page.getByRole("heading", { name: "No such operation" })).toBeVisible(); | ||
| // Not "Something broke": a mistyped id is not a fault on this end. | ||
| await expect(page.getByText("Something broke")).toHaveCount(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the dynamic route before inspecting malformed-ID handling.
ast-grep outline 'src/app/payouts/[id]/page.tsx' --items all --view expanded
# Expect invalid IDs to reach notFound() before a UUID query can fail.
rg -n -C 8 --glob '*.ts' --glob '*.tsx' \
'getPayoutOperationDetail|notFound\(|requirePayoutReader|safeParse|uuid' srcRepository: guarzo/authGD
Length of output: 49068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- e2e/not-found.spec.ts ---'
cat -n e2e/not-found.spec.ts | sed -n '1,190p'
printf '%s\n' '--- payout route boundaries ---'
for f in 'src/app/payouts/[id]/page.tsx' 'src/app/payouts/[id]/not-found.tsx' 'src/app/payouts/[id]/error.tsx'; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
cat -n "$f" | sed -n '1,180p'
fi
done
printf '%s\n' '--- relevant test and configuration references ---'
rg -n -C 4 --glob 'e2e/**' --glob 'tests/**' --glob 'package.json' \
'not-found|No such operation|Something broke|payouts/.+id|error\.tsx|baseURL|webServer' .Repository: guarzo/authGD
Length of output: 34835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("e2e/not-found.spec.ts").read_text()
route = Path("src/app/payouts/[id]/page.tsx").read_text()
service = Path("src/services/payout-view.ts").read_text()
paths = re.findall(r'page\.goto\("([^"]+)"\)', test)
for path in paths:
if path.startswith("/payouts/"):
value = path.removeprefix("/payouts/")
print(f"{path}: id_length={len(value)}, uuid_shape={bool(re.fullmatch(r'[0-9a-fA-F]{{8}}-[0-9a-fA-F]{{4}}-[0-9a-fA-F]{{4}}-[0-9a-fA-F]{{4}}-[0-9a-fA-F]{{12}}', value))}")
print("route validates id before database lookup:",
bool(re.search(r"(uuid|safeParse|parse|validate|schema)", route, re.I)))
print("route passes id directly to detail lookup:",
bool(re.search(r"getPayoutOperationDetail\s*\(\s*getDb\(\)\s*,\s*id\s*\)", route)))
print("detail lookup compares database UUID column with operationId:",
bool(re.search(r"eq\s*\(\s*payoutOperation\.id\s*,\s*operationId\s*\)", service)))
PY
printf '%s\n' '--- error boundaries ---'
find src -type f \( -name 'error.tsx' -o -name 'global-error.tsx' \) -print
rg -n -C 5 'Something broke|error boundary|ErrorBoundary' src e2eRepository: guarzo/authGD
Length of output: 17361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact detail lookup implementation ---'
cat -n src/services/payout-view.ts | sed -n '100,145p'
printf '%s\n' '--- database driver and UUID handling references ---'
rg -n -C 4 --glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'package-lock.json' --glob 'yarn.lock' --glob '*.ts' \
'postgres|pg\b|drizzle|uuid\(' .Repository: guarzo/authGD
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("e2e/not-found.spec.ts").read_text()
route = Path("src/app/payouts/[id]/page.tsx").read_text()
lookup = route[route.index("export default async function"):route.index("const nav =")]
for path in re.findall(r'page\.goto\("([^"]+)"\)', test):
if path.startswith("/payouts/"):
value = path.removeprefix("/payouts/")
valid_uuid = bool(re.fullmatch(
r'[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-'
r'[0-9a-fA-F]{4}-[0-9a-fA-F]{12}', value
))
print(f"{path}: length={len(value)}, valid_uuid={valid_uuid}")
print("route validation statements before lookup:",
bool(re.search(r'\b(?:uuid|safeParse|parse|validate)\b', lookup, re.I)))
print("direct lookup with raw id:",
bool(re.search(
r'getPayoutOperationDetail\s*\(\s*getDb\(\)\s*,\s*id\s*\)', lookup
)))
PYRepository: guarzo/authGD
Length of output: 380
Use a genuinely truncated UUID and validate it before the database lookup.
The current value is a complete, valid UUID, so it only covers a missing record. The route passes id directly to the UUID query; invalid input can reach error.tsx. Add a separate case that asserts No such operation.
🤖 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 `@e2e/not-found.spec.ts` around lines 143 - 151, Update the not-found E2E
coverage to use a genuinely truncated UUID and add a separate assertion for that
invalid identifier, ensuring it produces the “No such operation” response rather
than reaching the database or rendering “Something broke.” Preserve the existing
complete-UUID missing-record case separately.
) Two ways `/payouts/[id]` told the member something untrue. The tab read "Payout operation" for an operation that isn't there. The segment-scoped `not-found.tsx` shipped in #78 recorded why it couldn't fix that from its own end: a not-found boundary doesn't get to set the title, and `page.tsx`'s static `metadata` is applied even when the page throws. So the title moves to `generateMetadata`, which resolves the same lookup and names the operation — or says it isn't there. That was left as follow-up on the grounds it would cost a second lookup of the same row. It doesn't: both callers go through one `cache()`d loader, and Next resolves metadata and render in the same request. Measured with a counter on the miss path — one lookup per page load, not two. And `/payouts/<not-a-uuid>` returned a 500. The id reached the `uuid` column as a parameter, postgres rejected the cast with 22P02, and the member got "Something broke" — an apology for a server fault, for a mistyped or truncated URL. This is the defect class #74 spent itself fixing across `/payouts/*`. A shape check next to the existing `notFound()` sends it down the 404 path the well-formed-but-missing id already took. The regex is deliberately narrower than postgres's own parser, which was measured accepting braces, hyphenless and oddly-hyphenated forms. None of those can come from this app or a link it renders, and being narrower than the database fails toward the 404 rather than the 500.
Design sweep
SYNTHESIS.mdissue 3 — the last open blocker (error-boundary-audit#1 blocker,error-boundary-critique#1 major).The problem
There was no
not-found.tsxanywhere insrc/, sopayouts/[id]/page.tsx:91'snotFound()fell through to Next's built-inHTTPAccessErrorFallback. That page renders insideRootLayoutand injectsbody{color:#000;background:#fff;margin:0}, which wins on source order overbackground: var(--void)— both colours banned by name in DESIGN.md:22. Its dark branch keys off the OS preference rather than thecolorScheme: "dark"declared atlayout.tsx:39, so a member on a light-mode OS got a full-bleed white screen from a dark-only app. It also setsfont-family: system-uiinline (losing both self-hosted faces) and contains zero links — first Tab leaves the document.It is reached as a soft navigation:
/payoutsis the app's onlynext/linkcall site, so clicking a since-deleted operation produced no document load, no browser spinner, and focus reset to<body>.What shipped
Two files, not one.
src/app/payouts/[id]/not-found.tsxis reachable only throughpage.tsx, which callsrequirePayoutReader()beforenotFound()— so everyone who lands there has already cleared the guard, and it can offer/payoutsin the nav and as its primary action without reading the session. The root boundary has no such proof and sends everyone to/account. That difference is the point: the blocker path is a member clicking a row in the operations list, and the segment file puts them back in the list.Neither file reads the session.
/_not-foundbuilds as○ (Static); addingcookies()would change that. Navs are fixed and minimal —/payoutsand/admin/*are both gated, and a link that bounces the member straight back out is worse than no link.FocusHeading— anh1withtabIndex={-1}that focuses on mount. The App Router's ownfocus()call targets the first element of the changed segment (the<header>), which is not focusable, so it is a no-op. This is the mechanismadmin/sync/page.tsx:86-92argues for in writing, applied to the case where the inserted element is the whole page.Two things measured rather than assumed
Both recorded in comments so they are not re-litigated:
metadataexport applies; the segment-scoped one's does not.page.tsx's own metadata survives itsnotFound(), so that tab still reads "Payout operation". Correcting it needsgenerateMetadata+ a second lookup on a working page — left as follow-up, and cosmetic next to the announcement, which focus carries either way.h1, not the stale title, because React is mid-swap on the hoisted<title>when its effect fires. A race that lands well, not a contract — so the suite does not pin it.Stale claims from the sweep, corrected
SiteHeader'smeasureprop (which both reports' suggested fixes pass) no longer exists — removed by fix(ui): stop the header sliding sideways between routes #75.notFound()is atpayouts/[id]/page.tsx:91, not:58.Verification
npm run typechecknpm run lintno-img-elementwarnings on login/account)npm run format:checkAll matched files use Prettier code style!npm testnpm run test:e2enpm run build/_not-foundprerenders as○ (Static), confirming the no-session constraint holdsThe new spec covers the soft-nav arrival end to end (list open → row deleted behind it → click → boundary + focus + working exit), the 404 status surviving a custom component, ground/typeface not being the injected ones, the pasted truncated uuid landing on the operation 404 rather than the error boundary, and the payouts boundary staying behind the payouts guard.
Where to look
src/app/_components/focus-heading.tsx— whether always-focusing is right. On a hard navigation the document load already announces, and this moves focus past the skip link a beat later. The tradeoff is argued in the file comment; the alternative is leaving focus on<body>after a soft nav, which has no recovery.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests