fix(ENG-914): frame the first-run screen for new users#448
fix(ENG-914): frame the first-run screen for new users#448hazemahmedx0 wants to merge 2 commits into
Conversation
…G-914) - Retitle the first-run auth screen from "Sign in" to "Get started" and make "Create a free account" the primary button — most people seeing this screen have never used the app - "Already have an account? Sign in" sits one click below, so returning users lose nothing - The create-account button now runs the same browser sign-in flow but opens on Keycloak's registration form, so a new user comes back to the app already signed in — previously the "Create one for free" link sent them to the console website and left the app waiting on the sign-in screen - The register/login choice travels as an optional flag on the existing mindshub:login IPC call; older main processes simply ignore it and fall back to the login form Refs ENG-914
hazemahmedx0
left a comment
There was a problem hiding this comment.
Code review — ENG-914: frame the first-run screen for new users
Reviewed commit: `466e99eb0e0dc161baaf94f08ced281bc4334c50`
Clean, well-scoped change. I traced the register flag through the whole IPC chain and verified backwards compatibility, correctness, and the copy. Verdict below.
What I checked
Register-flag threading (correct across all 5 layers)
OnboardingScreen.handleMindsSSO(register) → host.mindshubLogin({ register }) → preload.mindshubLogin(opts) → IPC.MINDSHUB_LOGIN handler (_evt, opts?: { register?: boolean }) → opts?.register ? KEYCLOAK_REGISTER_URL : KEYCLOAK_AUTH_URL. Signatures line up at every hop, and global.d.ts matches the runtime bridge.
Event-leak guard (nice) — every handleMindsSSO call site is wrapped in an arrow (() => handleMindsSSO(true) / () => handleMindsSSO()), so the click Event never leaks in as a truthy register argument. Easy mistake to make here; it's done right.
Backwards compatibility — both directions hold
- Older main + newer renderer: the extra IPC arg is ignored by the old arity-0 handler → opens the login form (which still offers registration). No break.
- Newer main + older renderer:
mindshubLogin()with no args →opts?.registerisundefined→ falls through toKEYCLOAK_AUTH_URL. No break. - No IPC channel added or renamed — reuses
MINDSHUB_LOGIN, so the snapshot-locked channel map is untouched. Confirmed the diff carries no snapshot changes.
KEYCLOAK_REGISTER_URL — built from the same env-aware base + realm as KEYCLOAK_AUTH_URL, only the endpoint differs (/auth → /registrations). The new unit test asserts exactly that invariant, which is the right thing to lock (a new user must not land on a different Keycloak env than sign-in uses).
Tests & types — ran locally: minds-auth.env.test.ts passes 8/8, and full npm run typecheck (main + renderer + test) is clean.
Copy — title/subtitle, primary button, secondary link, the "back to account options" link, and the cancelled-error message are all consistent with the new framing. Removing the stale "Press Sign in with MindsHub…" reference in the cancelled message is a good catch.
Notes (non-blocking)
- Primary action now routes to registration. The most prominent button (
Create a free account) starts the register flow, so a returning user acting on muscle memory could land on the Keycloak sign-up page. That's the intended ENG-914 framing, the Keycloak registration page links back to sign-in, and the explicitSign in →secondary is right there — so this is fine as-is. Flagging only so it's a conscious call. - No test on the URL-selection branch in the IPC handler (register vs auth). It's a one-line ternary in orchestration code, so I wouldn't add a test just for it — the URL-shape test plus manual verification cover the risk.
Verdict
APPROVED — no blocking issues.
- Per the ticket mock: both actions are full-width buttons, not a button-plus-link — "Create a free account" filled/primary on top, "Sign in" outlined/subtle directly underneath - New arc-btn-ghost-stacked modifier keeps the ghost button's quiet look but matches the primary button's corner radius and height, so the stacked pair reads as one group (the pill radius looked off here) Refs ENG-914
hazemahmedx0
left a comment
There was a problem hiding this comment.
Re-reviewed the delta (466e99e..3581d60) — the text link → full-width secondary button swap.
Looks good, approving. Verified:
- Corner radius tracks the primary in every preset: default and gameboy fall through to the base 3px (there's no gameboy
.arc-btnradius override), midnight/daylight get 10px matching their primary. The11px 16pxpadding is an exact match to the midnight/daylight primary too. - Specificity is right — the two-class
.arc-btn-ghost.arc-btn-ghost-stackedcleanly overrides the base pillborder-radius: 999px, and the preset-scoped rules win over it. - Both controls are real
<button type="button">, full-width, keyboard-focusable with visible focus outlines. The standalone "Sign in" label is clear on its own.
nit (non-blocking, pre-existing): .arc-btn-ghost has no :disabled styling and its :hover isn't guarded with :not(:disabled), so during the brief validating phase the Sign in button still hovers/reads as active. The old arc-link had the same gap, so it's not a regression — worth a one-liner someday if you want the pair to dim together.
Summary
mindshub:logincall. An older main process paired with a newer UI simply ignores the flag and opens the login form (which still offers registration), so nothing breaks across staggered updates.Testing
~/.antonso the app starts fresh, then launch it.npm run typecheckandnpm testpass (349 tests, incl. a new one covering the registration URL).Verified by booting the built app with a clean profile — light and dark presets both render correctly.
Fixes ENG-914
Glossary