Repository navigation
feat(onboarding): animated splash into first-run login (#519) - #524
waterbro-8 wants to merge 2 commits into
Conversation
Add the first-run gate: the brand-mark splash plays for 900ms, then a 400ms shared-element transition lands on the login/register card (~1.3s cold), or a 300ms one reaches the workspace when a session is saved (~1.2s warm). An expired session gets an inline notice, never a modal, and the dead record is cleared. prefers-reduced-motion switches to 240ms of fades; the window is revealed on ready-to-show so no empty frame paints before the splash. Authentication sits behind a narrow adapter interface with an explicitly stubbed default that persists no password and compares nothing, so #395 can land independently. The login card is native form elements plus CSS: no new runtime dependency, and ADR-0002's provider chain stays inside App. The #127 full-app layout smoke renders the real app and has no session on a fresh CI machine, so it now declares itself in the loaded URL and the renderer skips onboarding only for that exact declaration; the trusted renderer URL is built with the same query so window IPC trust still matches. Refs #519
|
CI note on the two red What fails — one spec, one assertion, both platforms, identical signature: Why it is not this PR. The two files involved carry the same blob hashes on
This PR's 19 changed files include neither. The only input that changed between the last green run and the first red run is the calendar:
Mechanism. The schedule window starts at the Monday of the current week ( Plan. #526 pins the clock inside that one spec (the pattern already used in Everything else on this PR is green: layout parity, both unpacked staging smoke legs, both unsigned-installer legs, CodeQL, OpenSSF Scorecard and dependency review. |
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Requesting changes on the updated head (merge of main). The splash-into-first-run-login direction is right, but the branch fails deterministically on both platforms after the update-branch re-verification, and the failures point at the feature, not at infra:
Node 24 / ubuntu-latest— layout parity step fails withmac.json: layout is null — the two-column org module did not renderandlayout measurement never settled within its budget. The parity smoke launches the real app and waits for the workspace layout; a first-run splash/login surface that mounts before (or instead of) the workspace leaves nothing to measure.Node 24 / macos-14—Test timed out in 5000msin the renderer suite as well.
Suggested next steps:
- Give the smoke/parity path a deterministic bypass: e.g. an env/flag (
OWB_SMOKE=1or a persisted "onboarded" marker the harness can seed) that skips the splash animation and lands directly in the logged-in workspace, so the measured surface is the one the parity contract actually asserts. - Or mount the two-column workspace behind the splash so layout settles while the splash is up, and keep the animation purely presentational.
- The 5s timeouts on macOS should also be checked against the same mechanism — if the splash never dismisses under test,
findBy*waits will burn their full budget.
Not product acceptance. Will re-review once both required jobs are green.
sun-970
left a comment
sun-970
left a comment
There was a problem hiding this comment.
Review — feat(onboarding): animated splash into first-run login (#519)
Verdict: REQUEST_CHANGES. The craft here is high and the description is a model of honest reporting — but the one interaction the description flags as its biggest unknown is failing in CI right now, and the node job is red because of it.
The #127 layout smoke is broken by this change — the exemption does not work
The description says, correctly:
A fresh CI machine has no saved session, so a gate in front of the app would park that harness on the login card and the columns would never mount — a correct build judged red. Rather than weaken the gate, the harness now declares itself in the loaded URL (
?orgWorkbenchLayoutSmoke=1...) and the renderer skips onboarding only for that exact declaration.
That is exactly what should happen. It is not what happens. From Node 24 / ubuntu-latest on head d0f5ece:
layout parity FAILED:
- macos: column bottom delta 40px exceeds 2px
layout parity FAILED:
- left column width differs: mac 560px vs win 500px (max 4px)
Error: /tmp/owb-parity-tXIVeJ/mac.json: layout is null — the two-column workspace did not render
Error: /tmp/owb-parity-or4UdH/mac.json: smoke report has no layout measurement
Error: /tmp/owb-parity-5yUE87/win.json: layout measurement never settled within its budget
##[error]Process completed with exit code 1.
The failure signature is precisely the predicted one: layout is null and never settled within its budget mean the two-column workspace never mounted. So the exemption is not reaching the renderer in the packaged smoke path. Note also that the standalone Layout parity / macOS arm64 vs Windows x64 job passes — which is what makes this confusing to read from the checks page: the dedicated parity job is green while the same parity assertion inside the node job is red.
That is the headline. The good news, and it should be said plainly: your own renderer tests all pass on CI — Test Files 74 passed (74), and the focused 3 passed (3). So the gate, the reducer, the login card and the motion guards are all green. The only red is this integration.
Two things follow:
- The exemption needs to be diagnosed, not assumed. Either
trustedRendererUrldoes not match the frame that the layout harness actually loads, or theorgWorkbenchLayoutSmokeflag is being read after the gate has already decided. V12 only proves the exemption's absence fails; it does not prove its presence succeeds in the packaged harness — which is exactly the gap you flagged as "nothing local can prove the packaged run itself". CI has now proven it does not succeed. Please treat that as the finding of this review round. - V13 should be revised. V13 compares
App.test.tsxA/B on a loaded Windows box and concludes the 48 failures are "run-to-run spread, not a regression". CI says74 passed (74)for the suite — but CI runs the renderer tests, and V13's failures are in the renderer suite too. If CI is fully green onApp.test.tsxwhile your local box fails 43/94, then V13 is measuring your machine, not the change — which is what you suspected. Say that more strongly: V13 is now superseded by CI, and the local numbers should not be presented alongside CI as though they were comparable evidence.
On the stub adapter: I agree with shipping it, with one guard
Point 2 of your reviewer notes asks whether the stub should ship as the default while #395 is open. I think yes — that is the point of landing #519 ahead of #395, and auth-adapter.ts documents its own non-authority clearly ("buys usability, not safety"). The property that makes it acceptable is that the stub gates nothing on the server side: the control plane's boot token and IPC trust checks are untouched, so a permissive client-side stub cannot authorize a request. You state that; I verified it is consistent with the diff (no server-side change in the 19 files).
The one guard I would add in this PR rather than deferring: make the "stub is not a security boundary" claim machine-checked, not just documented. The pattern already exists in this repo — onboarding-motion.test.cjs scans for bare imports. A sibling assertion that the session record key (owb.onboarding.session.v1) is never read by anything under apps/server/ would turn "the stub authorizes nothing" from a comment into a guard. Right now it is the load-bearing security claim of the PR and it rests on prose.
Strong points that deserve to be recorded
- The gate sits above
<App />and the reasoning is correct. KeepingApp = workspace shelland proving routing through an exported pure reducer (reduceOnboarding) rather than through fake timers is the right call, and it is why the 36 tests in the three onboarding files are cheap and deterministic. REDUCED_MOTION_ONBOARDING_TIMINGS+ a real@media (prefers-reduced-motion: reduce)block, with both the timing swap and the animation→fade swap asserted, is more thorough than most motion work in this repo.- Four negative controls (V9–V12) each ending in a byte-identical restore. That is genuinely good validation hygiene — it proves the guards can fail, which is the part most PRs skip.
auth-adapter.tsvalidating what it reads back from localStorage as untrusted input, treating malformed as "never signed in" rather than crashing, is exactly right.- The size disclosure. 2246 lines is over the preference and you say so, justify it, and offer a concrete split. I do not think the split is necessary — the gate and its landing surface are genuinely one unit — so my answer to reviewer note 4 is: keep it as one PR.
Required before re-review
- Fix or explain the layout-smoke exemption. This is the blocker. Green node CI, or a documented reason the harness must change instead.
- Rebase —
mergeable_stateisbehind, and the parity work is exactly the kind of thing that drifts. - Update the ledger with the CI result (
74 passed (74)renderer) so the description's evidence matches the run, and downgrade V13 to "local only, superseded by CI". - Add the "stub authorizes nothing" guard described above, or state explicitly why a prose claim is sufficient here.
- Leave the author-checklist first item unchecked. You are right not to claim a maintainer readiness decision that was never recorded, and I would rather see that honesty than a checked box.
This branch has not been deployed
3 participants
Tracking record
Closes #519
Summary
Adds the desktop first-run experience the issue specifies: an animated brand-mark
splash that transitions into a login/register card, or straight into the
workspace when a saved session exists.
What changed
apps/desktop/renderer/src/onboarding/(new):OnboardingGate(routing statemachine),
SplashScreen,LoginScreen,BrandMark,auth-adapter, andonboarding.css.apps/desktop/renderer/src/main.tsx: the renderer entry is wrapped byOnboardingGate; the onboarding stylesheet is imported with the rest.apps/desktop/renderer/src/WindowControls.tsx(new, moved out ofApp.tsx):shared, because the first-run surface runs in the same frameless window as the
workspace but
<App />is not mounted there.apps/desktop/src/main.js: the window now waits forready-to-show.apps/desktop/src/packaged-smoke.cjs: one new declared channel so the fix(ui): align position-record and turn panels + Chinese IME + Windows parity evidence #127full-app layout smoke can reach the workspace (see the interaction note below).
packages/ui/src/locales/{zh,en}.ts: the first-run copy, both catalogs in step.Why the gate is above
<App />rather than inside it<App />is the workspace shell, andApp.test.tsxrenders it directly in 30+cases that assert the workspace is immediately usable. Putting the login flow
inside
Appwould force those cases to sign in first — a real behaviour changetraded for a test adaptation. With the gate wrapping the renderer entry,
"App = workspace shell" keeps its meaning and both halves stay independently
testable. The state machine is an exported pure reducer (
reduceOnboarding), sothe routing rules are proven directly rather than inferred through fake timers.
Deliberate choices worth reviewing
ConfigProvider/DSProviderlive insideApp; dragging that provider chain in front of thesigned-out state would make the lowest-fidelity screen carry the full token
setup. Native form elements +
onboarding.cssinstead. This also makes the"no new runtime dependency" criterion structural rather than a promise.
keeps a synthetic session in localStorage, never persists a password, and
compares nothing. It buys usability, not safety, and no deployment may read
"the stub let me through" as an authorization result. feat(auth): per-deployment login for the client — sessions scoped to each deployment, git-credentials style #395 swaps it behind the
same interface.
auth-adapter.tsvalidates everything it reads back from storage, becausethat record is untrusted input (written by a previous run, hand-editable). A
malformed record is treated as "never signed in", not as a crash.
WindowControlsmoved to its ownmodule so the splash and login card render the same three buttons App does. A
copy would have kept passing the feat(desktop): reachable dark-theme entry point + Agent Host select that fits its labels #94 / feat(theme): consume shared theme profiles #248 bundle guards while drifting. The
first-run chrome is suppressed as soon as the app mounts, so there is never a
second title bar in the DOM.
Interaction the issue could not have anticipated (please review)
The #127 full-app layout smoke deliberately renders the real renderer and
polls for the two org columns for 5s. A fresh CI machine has no saved session, so
a gate in front of the app would park that harness on the login card and the
columns would never mount — a correct build judged red. Rather than weaken the
gate, the harness now declares itself in the loaded URL
(
?orgWorkbenchLayoutSmoke=1, built exactly like the existing static-smokeoption, including the matching
trustedRendererUrl), and the renderer skipsonboarding only for that exact declaration. The packaged behavior smoke needs
no change: it drives the bridge directly and only requires
#rootto havechildren, which the splash satisfies.
Size, and why this is one pull request
2413 additions and 46 deletions across 19 files (GitHub's count): 12 new files, of
which 7 are production (1390 lines, incl. 440 of CSS) and 5 are tests (861 lines),
plus 7 existing files edited. That is well over the 400-line
preference, and I want to be explicit rather than let it slide. The breakdown is:
the issue's own file plan (5 new renderer modules + 2 test files) does not
decompose into smaller independently-mergeable PRs, because a gate without a
landing surface fails its own first acceptance criterion, and the guards for the
packaging interaction have to ship with the change that creates the interaction.
Roughly a third of the added lines are comments recording the reasoning and
test assertions. If the maintainers prefer, the natural split is (a) splash +
gate + adapter + WindowControls extraction, then (b) login/register card +
validation, with (a) landing behind the same gate and a temporary pass-through
surface — I am happy to re-cut it that way.
Intentionally out of scope (per the issue): the real auth protocol,
credential storage format and per-deployment session scoping (#395);
account/profile management; mobile shells.
Acceptance criteria
DEFAULT_ONBOARDING_TIMINGS= 900 + 400ms; asserted inonboarding-gate.test.tsx("inside the 1.3s acceptance budget") and observed in the timered component test.inside the 1.5s acceptance budget) plus the warm-launch component test, which also proves the login card never appears.SessionRestore'sexpiredstate;restoreFromStoragedrops the dead record. Test asserts the notice text and that norole="dialog"exists.role="tablist"with two tabs (+ arrow keys);validateCredentialsis a pure, separately tested function; component test asserts errors render and that invalid input sends no request.prefers-reduced-motionrespected (fades only, shorter splash)REDUCED_MOTION_ONBOARDING_TIMINGS(240ms splash ≤ 500ms) + a@media (prefers-reduced-motion: reduce)block; component test runs the sequence under a matched media query, andonboarding-motion.test.cjsasserts the reduced block swaps the mark animation for a fade.--ui-*tokens; the overlay paints the brand ground on its first frame andmain.jsnow shows the window onready-to-showwhile keepingbackgroundColor.onboarding-motion.test.cjsscans the module set and fails on any bare import outsidereact/@roleweave/ui/@roleweave/shared; the card avoids antd entirely.npm run checkpassesonboarding-gate.test.tsx(machine + wiring),onboarding-login.test.tsx(card),onboarding-entry.test.tsx(mount point). Local suite results in the ledger; CI runs the fullnpm run check.auth-adapterinterface with a stub default — functional without #395createStubAuthAdapteris the default ofresolveDefaultAuthAdapter();auth-adapter.tsstores no password and compares nothing, and says so.Validation ledger
node node_modules/typescript/bin/tsc --noEmit -p packages/uinode node_modules/typescript/bin/tsc --noEmit -p apps/desktopnpm run test:renderer(full suite)App.test.tsx(44),project-board,budget-dashboard,service-connections,reports-center(1 each). See V13.npm run build(tsc -b)npm run build:renderer(vite build)apps/desktop/dist/renderer/assets/*node --test apps/desktop/test/*.test.cjsonboarding-gate/onboarding-login/onboarding-entryonboarding-motion/onboarding-brand-markBrandMark.tsxvs the branding SVGdiffclean)onboarding.cssvs the SVG hextransformforwidthonboarding-entry3/4 → fails;main.tsxrestored byte-identicallyApp.test.tsxA/B against an unchanged checkoutApp.tsx+main.tsxrestored in the same tree 39/94 failed. Both are 5s timeouts on a loaded Windows box (the file takes >10 min here), so the delta is run-to-run spread, not a regression. CI (Node 24 / ubuntu-latestandmacos-14) is green on the base commit.npm run security:check(npm audit --audit-level=high)npm run check. No dependency was added or changed.smoke:package:*andlayout-paritystaging-macos-arm64,staging-windows-x64andlayout-parityjobs. The one packaging interaction (the layout harness) is pinned by V12.Tests and coverage
renderer/test/onboarding-gate.test.tsx(reducer: routing rules,order independence, timing budgets, reduced-motion read),
renderer/test/onboarding-login.test.tsx(validation, focus timing, inline errors, tab switching incl. keyboard, failure
mapping, busy state, English catalog),
renderer/test/onboarding-entry.test.tsx(mount point + both harness exemptions),
apps/desktop/test/onboarding-motion.test.cjs(keyframes are compositor-only, reduced-motion lands, one duration source of
truth, no new runtime dependency,
ready-to-showscoping, layout-harnesschannel),
apps/desktop/test/onboarding-brand-mark.test.cjs(icon geometry andpalette pinned to the shipped asset).
coverage gate in
npm run check.feat(auth): per-deployment login for the client — sessions scoped to each deployment, git-credentials style #395), and the native packaging behaviour, which only a packaged run can
exercise.
Change classification
Risk and rollback
packaging harness. No server, API, or data-format change.
user lands on a login card that the stub accepts with any plausible email and
an 8-character password — that is the point of landing feat(onboarding): animated app-icon splash that transitions into a login/register screen on first launch #519 without feat(auth): per-deployment login for the client — sessions scoped to each deployment, git-credentials style #395, and
it is the main thing to confirm before this ships to real users.
transform/opacity/filteronly (pinnedby
onboarding-motion.test.cjs); no network request is added before the firstpaint; the workspace bundle is not paid for twice (no new dependency).
owb.onboarding.session.v1localStorage key, which is harmless when the gateis gone.
Breaking or security notes
No breaking change.
Security: the stub is not a security boundary and does not pretend to be. It
stores a synthetic session record (no password, no token) under
owb.onboarding.session.v1, reads it back defensively, and accepts anythingplausible. Until #395 provides per-deployment authentication, the login card
gates nothing; nothing in this PR authorizes a request, and the control plane's
existing boot-token and IPC trust checks are untouched.
Author checklist
GOVERNANCE.md.Author checklist note: the first item is deliberately left unchecked. #519 is
status: reportedwithenhancementand no recorded maintainer confirmation, soI am not claiming a readiness decision that was never made. The requirement
record, if the maintainers want one, is the issue body itself.
Reviewer notes
Four places deserve extra scrutiny:
layoutSmokeLoadOptions+isLayoutSmokeEntry). This is the only place the gate is bypassed by a URL,and it exists because the fix(ui): align position-record and turn panels + Chinese IME + Windows parity evidence #127 full-app layout smoke measures a real
workspace that a fresh CI machine cannot reach past a login card. Please
confirm you are comfortable with the mechanism, and that the trusted renderer
URL still matches the loaded frame — V12 fails if the exemption disappears,
but nothing local can prove the packaged run itself (V15).
auth-adapter.ts). It is deliberately permissive andsays so. The question for review is whether shipping it as the default is
acceptable while feat(auth): per-deployment login for the client — sessions scoped to each deployment, git-credentials style #395 is open, or whether it should be behind a flag.
<App />. If you would rather have it insideApp, the cost is adapting
App.test.tsx's 30+ direct renders; I chose not tospend a behaviour change on that, but it is a reversible decision.
Summary — say the word and I will re-cut it.
Reproducing the numbers: V1–V8 are repository commands listed in the ledger; V9–V12 are
the four negative controls, each ending with a byte-identical restore. V13 is the A/B I
would most like reproduced on a Linux box, where
App.test.tsxis expected to pass infull — if it does not, the gate's cost is real and I want to know.