feat: harvest the v1 login UI and rewire it to the facade - #911
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThe web app adds Core Kit and SIWE authentication, authentication state orchestration, provider wiring, login and logout controls, environment validation, wallet configuration, terminal-themed styling, and related tests. ChangesWeb authentication and shell
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Brings back the login page, its three methods, and the terminal styling from v1, driving them through the P2 login handoff instead of the v1 vault bootstrap: Core Kit authenticates on the UI thread, the exported secret is transferred once to the engine, and every derivation happens in Rust. Wallet login collects the SIWE signature with wagmi and forwards it to facade.siweLogin; logout runs facade.logout plus the Core Kit teardown. Closes #804
Security and crypto review: wagmi no longer persists the wallet address, the SIWE nonce fetch is bounded and timed out, the wallet signature pins the account the message names, the Core Kit storage comment now states what is actually persisted, and a handoff the engine refuses ends the Core Kit session instead of leaving a live credential behind a signed-out UI. Altitude: the once-per-tab login guards moved to module scope so a second useAuth consumer cannot drive a second cold start, and EngineProvider rebuilds its client after logout rather than a leaf button reloading the document. Simplify and reuse: one terminal-button rule replaces three near-identical copies, LoginError and errorMessage are shared, MatrixBackground drops its unused props and redundant per-column canvas state writes, and dead CSS and design tokens are gone.
829ab79 to
af74145
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 55 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
apps/web/src/test/authFakes.tsx (1)
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate the
QueryClientonce per wrapper.Line 107 constructs a new
QueryClienton every render ofWrapper. Wagmi hooks then lose their cache on each re-render, which makes wallet-flow tests order dependent. Build the client in the enclosing closure.♻️ Proposed refactor
export function pageWrapper(client: EngineClient, session: CoreKitSession) { const Auth = authWrapper(client, session); + const queries = new QueryClient(); return function Wrapper({ children }: { children: ReactNode }) { return ( <WagmiProvider config={wagmiConfig} reconnectOnMount={false}> - <QueryClientProvider client={new QueryClient()}> + <QueryClientProvider client={queries}> <Auth>{children}</Auth> </QueryClientProvider> </WagmiProvider> ); }; }🤖 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 `@apps/web/src/test/authFakes.tsx` around lines 102 - 113, The pageWrapper function currently creates a new QueryClient on every Wrapper render; instantiate one QueryClient in the enclosing pageWrapper closure and reuse it in QueryClientProvider, preserving the existing provider hierarchy and behavior.apps/web/src/auth/useAuth.test.tsx (1)
20-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the serialized-transition guard.
useAuthkeepsinFlightat module scope so a second transition rejects rather than starting a second login. No test covers that rejection. Add a case that starts a login which does not settle, then callsloginWithGoogleagain and asserts the rejection messageanother sign-in is already in progress.🤖 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 `@apps/web/src/auth/useAuth.test.tsx` around lines 20 - 38, Add a test in the useAuth suite that starts a login with an unresolved transition, invokes loginWithGoogle again before it settles, and asserts the second call rejects with “another sign-in is already in progress.” Use the existing mount and fake engine/Core Kit helpers, and ensure the pending transition is cleaned up so it does not affect other tests.apps/web/src/components/auth/WalletLoginButton.tsx (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
0xstrip intofromHex.
fromHexonly accepts bare hex, sostrip0xis needed here, but decoding wallet signatures inapps/webviolates the UTF-8 codec boundary. Put the0xhandling in@cipherbox/client/src/seams/bytes.ts, or usehexToBytesfromvieminstead of adding codec state to the UI layer.🤖 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 `@apps/web/src/components/auth/WalletLoginButton.tsx` at line 67, Update the WalletLoginButton login flow to stop decoding the signature with fromHex and strip0x in the UI layer; use hexToBytes from viem or move 0x-prefix handling into the shared bytes seam, then pass the resulting bytes to onLogin.Source: Coding guidelines
apps/web/src/index.css (2)
73-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the keyword-case lint error for
text-rendering.Stylelint flags
optimizeLegibilityas violatingvalue-keyword-case. Use the lowercase keyword to satisfy the project's lint configuration.🎨 Proposed fix for the keyword case
- text-rendering: optimizeLegibility; + text-rendering: optimizelegibility;🤖 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 `@apps/web/src/index.css` around lines 73 - 74, Update the text-rendering declaration near font-synthesis to use the lowercase optimizelegibility keyword required by the value-keyword-case lint rule.Source: Linters/SAST tools
123-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the deprecated
clipproperty.Stylelint flags
clipas deprecated (property-no-deprecated). Useclip-path: inset(50%)for the visually-hidden pattern instead.🎨 Proposed fix to replace the deprecated property
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 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 `@apps/web/src/index.css` around lines 123 - 133, Update the .sr-only visually hidden style to replace the deprecated clip property with clip-path: inset(50%), while preserving the existing accessibility-focused hiding behavior and other declarations.Source: Linters/SAST tools
🤖 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 `@apps/web/src/auth/coreKit.ts`:
- Around line 49-64: Update login in the CoreKit wrapper to invoke the available
Core Kit cleanup/logout flow when this.coreKit.status is not
COREKIT_STATUS.LOGGED_IN, before throwing the approval or recovery error. Ensure
partial OAuth/tKey state is cleared from the instance and local storage, while
preserving the existing commitChanges path for successful logins.
In `@apps/web/src/auth/useAuth.ts`:
- Around line 31-37: Move the login busy state from per-instance useAuth state
into shared auth state, such as authStore or an external store consumed by
useSyncExternalStore. Update the useAuth login flow and all isBusy reads so
every useAuth consumer observes the same in-flight transition, including
disabling LogoutButton while LoginPage is authenticating; preserve the existing
inFlight guard and error behavior.
In `@apps/web/src/components/auth/LogoutButton.tsx`:
- Around line 9-22: Update the LogoutButton signOut invocation to catch and
swallow rejected logout promises, matching LoginPage’s established
login.catch(() => undefined) handling while preserving the existing
navigate('/') finally behavior.
In `@apps/web/src/components/auth/WalletLoginButton.tsx`:
- Around line 83-95: Update the WalletLoginButton component’s picking flow to
move focus to the first interactive wallet-picker element after the picker
renders, and restore focus to the [WALLET] trigger when cancellation closes it.
Use refs and an effect or equivalent lifecycle handling tied to picking,
preserve existing busy/no-provider behavior, and verify the keyboard focus path
in the browser.
- Around line 49-53: In the WalletLoginButton connection flow, validate that
accounts contains a usable first account immediately after connectAsync returns
and before setPhase('signing') or createSiweMessage. Handle the empty-account
case through the existing user-facing error path, and only build the SIWE
message when accounts[0] is defined.
In `@apps/web/src/components/StagingBanner.tsx`:
- Line 8: Update the root banner element in StagingBanner to use role="alert"
instead of role="banner", and remove its aria-live="polite" attribute while
preserving the existing test id and className.
In `@apps/web/src/engine/config.ts`:
- Around line 10-12: Update apiBaseUrl to treat an empty VITE_API_URL as absent,
matching the existing environment helper, so it returns DEFAULT_API_URL instead
of propagating an empty string to EngineHostConfig and URL construction.
In `@apps/web/src/main.tsx`:
- Line 33: Update the createCoreKitSession flow used by CoreKitProvider so
Web3AuthMPCCoreKit does not receive window.localStorage for device-factor or
login-state persistence; configure session restoration to require a new factor
or use the project’s supported non-Web-Storage/ephemeral storage option, while
preserving normal session creation.
---
Nitpick comments:
In `@apps/web/src/auth/useAuth.test.tsx`:
- Around line 20-38: Add a test in the useAuth suite that starts a login with an
unresolved transition, invokes loginWithGoogle again before it settles, and
asserts the second call rejects with “another sign-in is already in progress.”
Use the existing mount and fake engine/Core Kit helpers, and ensure the pending
transition is cleaned up so it does not affect other tests.
In `@apps/web/src/components/auth/WalletLoginButton.tsx`:
- Line 67: Update the WalletLoginButton login flow to stop decoding the
signature with fromHex and strip0x in the UI layer; use hexToBytes from viem or
move 0x-prefix handling into the shared bytes seam, then pass the resulting
bytes to onLogin.
In `@apps/web/src/index.css`:
- Around line 73-74: Update the text-rendering declaration near font-synthesis
to use the lowercase optimizelegibility keyword required by the
value-keyword-case lint rule.
- Around line 123-133: Update the .sr-only visually hidden style to replace the
deprecated clip property with clip-path: inset(50%), while preserving the
existing accessibility-focused hiding behavior and other declarations.
In `@apps/web/src/test/authFakes.tsx`:
- Around line 102-113: The pageWrapper function currently creates a new
QueryClient on every Wrapper render; instantiate one QueryClient in the
enclosing pageWrapper closure and reuse it in QueryClientProvider, preserving
the existing provider hierarchy and behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 319897e4-5674-4cda-85f3-da71e889f924
📒 Files selected for processing (26)
apps/web/src/App.test.tsxapps/web/src/auth/CoreKitProvider.tsxapps/web/src/auth/coreKit.tsapps/web/src/auth/siweNonce.test.tsapps/web/src/auth/siweNonce.tsapps/web/src/auth/useAuth.test.tsxapps/web/src/auth/useAuth.tsapps/web/src/components/MatrixBackground.tsxapps/web/src/components/StagingBanner.tsxapps/web/src/components/auth/EmailLoginForm.tsxapps/web/src/components/auth/GoogleLoginButton.tsxapps/web/src/components/auth/LoginError.tsxapps/web/src/components/auth/LogoutButton.tsxapps/web/src/components/auth/WalletLoginButton.tsxapps/web/src/engine/config.test.tsapps/web/src/engine/config.tsapps/web/src/index.cssapps/web/src/lib/errorMessage.tsapps/web/src/lib/wagmi.tsapps/web/src/main.tsxapps/web/src/providers/EngineProvider.tsxapps/web/src/routes/FilesPage.tsxapps/web/src/routes/LoginPage.tsxapps/web/src/styles/login.cssapps/web/src/test/authFakes.tsxapps/web/src/vite-env.d.ts
Drop the partial Core Kit session when a login lands short of LOGGED_IN, swallow the rejected logout promise the button discarded, guard an empty wallet account tuple, move keyboard focus with the wallet picker, and read a blank VITE_API_URL as unconfigured rather than as a base URL.
Nitpick verdictsThe five nitpicks live in the review body rather than in threads, so the verdicts are here. Fixes are in 2137b07.
|
1 participant
Closes #804
Brings the v1 login page back and rewires it onto the v2 facade. Everything in this diff lives in
apps/web— nopackages/clientfiles were touched, so it does not collide with #907.What the login flow now does
loginWithOAuth: Google, or Web3Auth's email-passwordless flow seeded with the address typed into the form).useAuthhands the exported login secret to the engine exactly once through the web: land the useSyncExternalStore snapshot adapter and Core Kit login handoff #803 handoff —handOffLoginSecret→facade.start(secret)— and registers the session with theLoginSecretSourceso a leadership failover can re-export it.facade.startruns the engine's own challenge-signature login.useAuthforwards it tofacade.siweLogin(message, signature).facade.logout()plus the Core Kit teardown, disarms the secret source, clears the UI auth store, and asksEngineProviderfor a fresh client —facade.logoutcloses this tab's client permanently, so without that the tab could never log in again.Harvested from
v1vs written freshroutes/Login.tsxstructure, copy, footerhooks/useAuth.ts— the v1 hook drove the vault bootstrap, IPNS derivation, and token lifecycle in TS; all of that is engine-side nowcomponents/MatrixBackground.tsx,components/StagingBanner.tsxGoogleLoginButton— v1 loaded Google Identity Services and posted the idToken to a CipherBox identity endpoint; Core Kit owns the OAuth round-trip now, so the button is a buttonindex.cssdesign tokens, the login half ofApp.cssEmailLoginForm— the v2 API has no OTP endpoint; the code is entered in Web3Auth's own window, so the two-step form collapses to onelib/wagmi/*,WalletLoginButtonconnector-picker UX and SIWE message shapeWalletLoginButtontransport — v1 calledidentityWalletNonce/identityWalletVerify; this posts the signature tofacade.siweLoginLogoutButtonlib/web3auth/*→auth/coreKit.ts— a narrowCoreKitSessionseam souseAuthnever sees a Web3Auth parameter shape and a test can substitute a plain objectDeliberately not harvested: the MFA /
REQUIRED_SHAREscreens (DeviceWaitingScreen,RecoveryInput) — #809 defers the mfa specs; a device without its factor gets a plain error. Also dropped: the health-check gating on the login buttons, the GIS One Tap fallback, and the placeholder help/privacy/terms footer links.The one architectural debt: the SIWE nonce
facade.siweLoginexists but nothing exposes the challenge an EIP-4361 message has to embed — the engine'sApiClient::siwe_challengeis unreachable from the facade. Wiring one through would have meant editingprotocol.ts,transport.ts, both transports,serve.ts,facade.ts,crates/wasm, and the engine facade — precisely #907's blast radius.So
apps/web/src/auth/siweNonce.tsfetches the public, single-use nonce straight fromPOST /auth/siwe/challenge, bounded and under a timeout. It is the only direct API call inapps/weband it contradicts the blueprint's "no seams inapps/web". Filed as #910 with a dependency edge back here.Review gates
/simplify,/security-review, and/crypto-privacy-reviewall ran againstgit diff main...HEAD. Folded back in:createConfigdefaultsstoragetolocalStorage, and itspartializewritesconnections[].accounts— the address — towagmi.store.disconnect()normally clears it seconds later, but a tab closed while the signing prompt is open leaves it indefinitely. Nowstorage: null, verified in the browser: the key no longer appears.session.method()/email()callgetUserInfo(), which throws when Core Kit is not logged in, and they sat outside the failure envelope — so a throw after a successfulstartleft the secret source armed with the UI rendering signed out. They are read before arming now, and a handoff the engine refuses ends the Core Kit session rather than leaving a 24-hour bearer credential the user has no button to clear.sessionIdthat reconstitutes a logged-in instance. The comment now says what is actually stored, and web: decide the Core Kit storage scope and gate its key set #913 makes the storage scope an explicit decision with a web-e2e assertion over the key set.CoreKitProviderStrictMode claim was false — constructing in an effect is what causes a double construction. The session and its restore promise are latched in refs now, so a remount reuses the instance instead of racing a secondinit()against the same store.LogoutButtonalso callsuseAuth, so on a direct load of/fileswith a surviving session it was the logout button drivingfacade.start. The guards are module-scoped now, keyed on the session object, and a collision rejects instead of resolving silently.window.location.assign('/')as teardown moved up a layer:EngineProviderrebuilds its client, so any logout path works rather than only the one that remembered to reload.AbortSignal.timeout, an upper length bound, andnew URLresolution. Signing pinsaccount: accounts[0].0xstripping is conditional.VITE_ENVIRONMENTrejects an unrecognized value rather than defaulting it — a typo would silently pick the wrong Web3Auth network, deriving a different identity over an empty vault..terminal-btnrule replaces three near-identical button rule sets,LoginErroranderrorMessageare shared,MatrixBackgrounddrops unused props and redundant per-column canvas state writes, and dead CSS and unused design tokens are gone.Two findings were too deep to fix here and are filed instead: #914 (auth state is a tab-local optimistic store, not derived from the engine — log out in one tab and another keeps rendering
/files; the event stream carries nothing auth-shaped yet) and the SIWE encoding bugs added to #910 (the facade sends unprefixed hex where the API DTO requires0x, and the API does not validateuri).Verified vs blocked
Verified:
apps/webunit suite — 65 tests green. TheuseAuthsuite asserts each method dispatches the handoff, that SIWE routes tofacade.siweLoginwhile exporting no secret at all, that logout tears both sides down and yields a fresh client, that a Core Kit session surviving a reload re-hands the secret, and that a refusedstartleaves the tab signed out with the buffer scrubbed, the re-export capability disarmed, and the Core Kit session ended.localStorageholds only Web3Auth loglevel keys — nothing key-shaped, no wallet address.tsc -b,eslint,prettier, and the productionvite buildare clean.Not verifiable yet:
VITE_WEB3AUTH_CLIENT_ID/VITE_WEB3AUTH_VERIFIER, and the rest of the v2 web runtime is still stubbed. The flow dispatches correctly; whether Web3Auth's popup returns a usable TSS key against a v2 verifier is a human check.facade.siweLoginis aCommand, so the engine refuses it withNotStartedbeforestart, and the facade's signature encoding does not match the API DTO. Both are tracked on web: move the SIWE challenge below the facade #910; the path is fail-closed today, so nothing lands the tab in an authenticated state it has not earned.useAuth.test.tsxwas renamed to what it actually proves — a fake session has no SDK behind it, so only web-e2e can assert the real key set (web: decide the Core Kit storage scope and gate its key set #913).New build-time environment keys
VITE_ENVIRONMENT,VITE_WEB3AUTH_CLIENT_ID,VITE_WEB3AUTH_VERIFIER— all declared invite-env.d.ts; the app fails loudly at Core Kit construction if the last two are absent.Summary by CodeRabbit