Repository navigation
feat: navigation, loading/error states, and generation UX (D2) - #52
Conversation
The composer and the dashboard each carried a byte formatter, and the composer's stopped at KB. One implementation in src/lib/format, promoting on the rounded figure so 1 048 575 bytes reads "1.0 MB", not "1024.0 KB". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- aria-disabled / readOnly instead of disabled while streaming, so focus is not dropped to <body> when Generate is pressed - focus the result panel (now a labelled region) when generation finishes - Cmd/Ctrl+Enter submits from the prompt (IME-safe), with a visible hint - beforeunload guard while a stream is running - polite live announcement per completed file - memo(FilePanel) and rAF-batched, per-file-coalesced chunk dispatch - stick to the bottom only while the reader is already near it - shared formatBytes; next/link for the wordmark and Open artifact Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The auth screens post plain HTML forms to route handlers, so a double click sent two requests (a second rate-limit slot, a second reset email). useFormStatus cannot see URL-action forms, so SubmitButton latches on the form's submit event (reopened on bfcache restore), shows a pending label, and uses aria-disabled so focus stays put. Footer links use next/link; the OIDC start link stays a plain <a> because it targets a route handler. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- next/link for internal links on the dashboard and trash pages - loading.tsx skeletons for /dashboard and /trash (opacity shimmer) - root error.tsx with retry(), digest reference, and focus on the heading - not-found.tsx moves to a CSS module and links home - trash: "Restoring" busy label; after restore, a status line linking to the restored artifact that survives the router.refresh() - dashboard sign-out uses SubmitButton Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
loading.tsx wraps the page in Suspense, so the page's redirect('/signin')
was streamed after the skeleton and performed client-side after load. A
signed-out visitor saw a skeleton then a second navigation, and the
design-system e2e spec's page.evaluate died with "Execution context was
destroyed". Dashboard and trash now get a layout.tsx that checks the session
outside the boundary (a real HTTP redirect); the lookup is memoised per
request with React cache() so layout + page cost one query.
The spec signed in through the standalone `request` fixture, whose cookies
are not shared with `page`, so every signed-in surface silently measured
/signin. It now signs in through page.request (minimal change; the file is
owned by PR #55, which renames it).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
datj9
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code)
Verdict: fix the HIGH before merging. The rest of the PR is careful work. The batcher ordering, the aria-disabled guards, the layout-level session gate and the error boundary all hold up when I read them.
Findings: 0 CRITICAL, 1 HIGH, 1 MEDIUM, 3 LOW
Checks run locally on 031aa27: pnpm typecheck passed, pnpm lint passed, pnpm vitest run --project unit passed (76 files, 1205 tests). I did not run e2e or a browser.
Summary
- HIGH
app/dashboard/artifact-list.tsx:102(andapp/new/prompt-composer.tsx:88, trash "Open it"). Soft navigation to/a/{id}puts a single-use, 30 s handoff token into the client router cache. Browser Back/Forward replays that cached token, so the artifact iframe fails to load. Details are inline. - MEDIUM
app/trash/trash-list.tsx:44.busyIdis cleared beforerouter.refresh()lands. A second Restore click on the same row during that window wipes the success line and shows a false "could not be restored". - LOW
app/_components/submit-button.tsx:48. If the user stops the navigation (Esc or the Stop button), the lock never reopens. The button stays on "Signing in" and refuses every later submit until a reload. - LOW
app/error.tsx:53. The root error boundary also covers anonymous routes (/s/[token],/a/[id]for share visitors), so "Back to artifacts" sends them to/dashboard, which then redirects to/signin.not-found.tsxalready links to/for exactly this reason. - LOW Test gaps. No test covers the new UI states: the trash restored-status line and its link, focus moving to the result panel, the SubmitButton lock (only the pure latch is tested), and the layout-level redirect. The PR says e2e wasn't run, so none of the keyboard, focus or live-region behaviour has been exercised yet. One e2e assertion each for
trash-restored-linkand thegeneration-resultfocus would cover the new contract.
| }} | ||
| > | ||
| <a className={styles.link} href={`/a/${item.id}`}> | ||
| <Link className={styles.link} href={`/a/${item.id}`}> |
There was a problem hiding this comment.
HIGH: this breaks the artifact view on browser Back/Forward.
/a/[id]/page.tsx mints a handoff token (signHandoffToken: HANDOFF_TTL_SECONDS = 30, single-use via consumedTokenIds) and bakes it into the iframe src (__enter?t=...). With next/link, the RSC payload for /a/{id} goes into the client router cache. App Router back/forward navigation restores from that cache without refetching, and staleTimes don't apply to back/forward.
How it fails:
- Dashboard, then click a row (soft nav). The token is consumed by the iframe load.
- Press browser Back (dashboard), then Forward.
/a/{id}is restored from cache and the iframe remounts with the consumed or expired token. The handoff rejects it, so the artifact doesn't load.
Before this PR the row was a plain <a>, so this flow was a full document load (or bfcache with the iframe still alive). The same applies to "Open artifact" in app/new/prompt-composer.tsx:88 and the trash "Open it" link. The PR's risk note ("renders the same either way") misses the back/forward case.
Fix options: keep plain <a> for links into /a/{id} (the same reasoning as the OIDC link), or mint the token client-side or on mount (for example a route handler the frame calls) so it is never part of a cacheable RSC payload.
| setBusyId(artifactId) | ||
| setBusyId(item.id) | ||
| setErrorMessage(null) | ||
| setRestored(null) |
There was a problem hiding this comment.
MEDIUM: router.refresh() isn't awaited (it is a transition), so finally (line 57) clears busyId while the restored row is still on screen. During that window (a force-dynamic page plus a DB read, so a few hundred ms) the row shows "Restore" and is enabled again.
A second click on the same row calls setRestored(null) here, and the POST fails because the artifact is already restored. The user then sees "Its restore window may have run out" for an artifact that was just restored, and the new "Restored ... Open it" line is gone.
Fix: keep the row busy until the refresh lands, for example startTransition(() => router.refresh()) combined with useTransition's isPending, or hide or lock rows whose id matches restored.id.
| event.preventDefault() | ||
| return | ||
| } | ||
| setIsSubmitting(true) |
There was a problem hiding this comment.
LOW: the lock only reopens on pageshow with persisted. If the user cancels the in-flight POST (Esc, or the browser Stop button during a slow sign-in), no navigation happens and no pageshow fires. The button stays "Signing in", aria-disabled, and every later submit is preventDefaulted until a manual reload.
A cheap way out: reopen after a timeout (for example 10 to 15 s), or on the next input or change event in the form, so a stalled submit can be retried.
| <button className="button-primary" type="button" onClick={() => retry()}> | ||
| Try again | ||
| </button> | ||
| <Link className="button-secondary" href="/dashboard"> |
There was a problem hiding this comment.
LOW: this root boundary also catches errors on anonymous surfaces (/s/[token], /a/[id] for share-link and org viewers). For those visitors, "Back to artifacts" goes to /dashboard, and the layout gate redirects them to /signin. not-found.tsx links to / for exactly this reason, so consider the same target here, or choosing the target based on whether a session cookie exists.
- Dashboard rows, "Open artifact" and the trash restore link go back to plain <a>. The viewer
page embeds a single-use handoff token in the iframe URL, and the App Router replays the
cached render on Back/Forward, so a soft-navigated /a/{id} could re-present a burnt token
and show the re-entry page.
- design-system spec: the combined desktop/typography loops measured /signin and
/forgot-password while signed in, which redirects to /dashboard. Public pages are now
measured after clearing cookies.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review summary (D2) Fixed in 76b10a6
Checked, no defect found
Left (minor, not fixed)
Gates: typecheck, lint, unit (1205), and build all pass. e2e not run (no Docker). |
Implements plan section D2
feat/navigation-and-generation-ux. Touches only D2-owned paths plus new files in them (app/_components/{submit-button,submit-gate,list-skeleton}*,app/status-page.module.css,app/new/stream-{batch,view}.ts,src/lib/format/bytes.ts) and new unit tests.Changes per item
1.
next/linkfor internal links/newwordmark and "Open artifact"; auth footers (Forgot password?, Back to sign in); error/not-found pages./api/auth/oidc/start) stays a plain<a>on purpose. It points at a route handler that redirects to the IdP, so it needs a full document navigation. A comment explains why.2. Loading / error / not-found
app/dashboard/loading.tsxandapp/trash/loading.tsxrender a sharedListSkeleton: the same header bar and a column of rows, with an opacity-only linear shimmer. It's functional motion, so per docs/motion.md it keeps running under reduced motion. It has no links or headings, so tests and screen readers can't pick up duplicates, plus an sr-only status line andaria-busyon<main>.app/error.tsxuses Next 16.3's stableretry(), which re-fetches the segment (reset()would just show the same server error again). It showserror.digestas a reference, focuses the heading, and offers "Try again" and "Back to artifacts".not-found.tsx: the inline styles move to a CSS module shared with error.tsx. It links to/, not /dashboard, because anonymous share-link visitors land here too.3. Generation flow (
app/new/**)readOnlyand the Generate, starter and Retry buttons arearia-disabled, notdisabled, so focus doesn't drop to<body>. Handlers checkisStreaming.role="region"labelled by a new "Artifact ready"<h2>, so the next Tab reaches "Open artifact".form.requestSubmit(), so it passes the same guard as a click. It's ignored during IME composition. There's a visible hint (aria-describedby) andaria-keyshortcuts.beforeunloadguard is active only while streaming.file_endchanges the text, so each file is announced once.memo(FilePanel): the reducer keeps the identity of files it didn't touch, so only the file being written re-renders.createActionBatcher: chunks are queued and flushed once per animation frame, and consecutive chunks for the same path are merged when they're queued. Every non-chunk action flushes the queue first and is dispatched right away, so order is kept and ✓ / result / failure never wait for a frame. Every exit path (end of stream, error, abort) flushes before its own dispatch.4. Shared
formatBytes(src/lib/format/bytes.ts)5. Trash
role="status"line above the list says "Restored <title>. Open it" and links to/a/{id}. It lives in local state outside the list, so it survivesrouter.refresh()removing the row, including when the list becomes empty.6. Auth POST forms:
SubmitButtonuseFormStatusonly reports pending for forms whoseactionis a function. These forms post to URLs and navigate natively, so it would never fire on its own.SubmitButtontherefore attaches asubmitlistener to its form. The first submit goes through, later ones arepreventDefaulted, and the lock reopens on a bfcachepageshow.useFormStatusis still combined in for any future server-action form.submitonly fires after constraint validation, so an invalid form never locks itself. Before hydration the form works normally but has no guard.aria-disabled.AuthScreen, so setup, signin, signup, forgot-password and reset-password all get it, plus the dashboard's sign-out form.Accessibility notes
disabledmid-action anywhere in these flows. Every aria-disabled button keeps focus, and its handler checks the state.:focus:not(:focus-visible).Testing
pnpm typecheck✅,pnpm lint✅,pnpm vitest run --project unit✅ (76 files / 1205 tests), andpnpm build✅ with a dummy env.format-bytes.test.tsgeneration-stream-batch.test.ts: frame batching, per-path merge, ordering, flush/dispose, one frame per batchgeneration-stream-view.test.ts: near-bottom threshold, announcement text, ⌘/Ctrl+Enter and IMEsubmit-gate.test.tsE2E specs touched
zz-design-system.spec.tsonly (see the follow-up section). Otherwise none: I checked the specs that use these pages:start-a-generation: link names are unchanged.trash-delete-restore: it filters bytrash-row, and the new status line sits outside those rows.signin-password-reset:getByRole('status')on forgot-password still matches only the success message. SubmitButton adds no status role, and thehref="/forgot-password"attribute is unchanged undernext/link.setup-and-signin,upload-and-list,zz-design-system: button names are only matched before clicking.toBeDisabledon the composer.Follow-up: signed-out redirect and design-system spec (commit 031aa27)
loading.tsxwraps the page in Suspense, so the page'sredirect('/signin')was streamed after the skeleton and carried out client-side afterload. Signed-out visitors saw a skeleton and then a second navigation.app/dashboard/layout.tsxandapp/trash/layout.tsxnow check the session outside that boundary throughapp/_components/session-gate.ts(requireSessionUser), so the redirect is a real HTTP one. The lookup goes through Reactcache(), so the layout and page share one query. The pages keep their own check. Only dashboard and trash have loading states.tests/e2e/zz-design-system.spec.ts, renamed todesign-system.spec.tsby PR chore(ci): harden CI, coverage and e2e ordering #55):signInused the standalonerequestfixture, which doesn't share cookies withpage. So every signed-in surface was actually measuring/signinafter the server redirect, a bug that predates this PR. It now signs in throughpage.request. That's the minimal change: 4 call sites plus a doc comment. Expect a trivial conflict with chore(ci): harden CI, coverage and e2e ordering #55's rename./signinand/forgot-passwordinPUBLIC_PAGESredirect to/dashboardin the two combined loops, so those iterations measure the dashboard. The standalone public-page tests still measure the real pages.Risks / notes
/a/{id}now goes throughnext/link(dashboard rows, "Open artifact", trash restore link). The artifact page renders the same way either way, but any spec that clicks a row and relied on a full document load would be affected. I found none./newmid-stream throughnext/link(the wordmark) doesn't firebeforeunload. The fetch isn't aborted on unmount either, so the generation still finishes server-side and shows up on the dashboard. A full unload does abort it (the server stops onrequest.signal), and that's what the guard covers.app/_components/marketing/*: nav, CTA, colophon →/signin) are still plain<a>. They live outside my owned paths (onlyapp/(marketing)/**is mine, and it has no internal links).prettier --checkstill flags two pre-existing files inapp/_components/marketing/that I didn't touch.🤖 Generated with Claude Code