Refresh desktop onboarding flow - #7528
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head b0a484f052f72441ff872784c0d507cbbfcc9539 against base bfc384855889432df4a333a0edf3080f332ee169.
1. [P1] Keep the identity secret masked until an explicit reveal
Choose Create a new identity key → Create my private key. The new mount effect at lines 114–127 calls getNsec() without a reveal action, and this paragraph renders the complete reusable private key as selectable plaintext. Hover/focus blur only conceals it afterward. Previously the page showed a fixed mask and retrieved the secret only for Copy/Reveal. A user screen-sharing or recording onboarding now exposes the credential that lets another person sign in as them without choosing to reveal it. Restore masked-by-default rendering and action-gated retrieval; assert that the secret is absent from the DOM and get_nsec is not called before an explicit action.
2. [P2] Render identity-creation failures on the new intro page
MachineOnboardingFlow.tsx:411–421
Make getIdentity() reject after clicking Create my private key. loadFreshIdentity catches the failure into error and resets isPending (147–166), but error is rendered only inside the welcome/identity branch (351–353), not this intro. The button silently becomes usable again while the user remains on the same page. The base invoked this callback on the page displaying its error. Render the failure in this branch and clear it appropriately on retry/navigation; cover a rejected identity load.
3. [P2] Prevent recovery-method switches from abandoning an active key import
MachineOnboardingFlow.tsx:503–510 (also 516–523)
Submit a valid nsec with a delayed importIdentity, then select backup file or recover from your phone. Both links check only isPending, although this operation reports isKeyImporting. The new inline branches replace and unmount the submitting NostrKeyImportForm. Its input and error are component-local, and its delayed rejection is caught into that unmounted component (NostrKeyImportForm.tsx:218–232), losing the attempted key and failure feedback even after returning. The old portaled dialogs retained the original form. Disable both switches while isKeyImporting (or explicitly own cancellation/results above the branches) and cover delayed import rejection and success during attempted switching.
4. [P2] Do not advance using cached readiness after a forced recheck fails
SetupStep.tsx:1083–1090 (same condition in chooseMethod, 1065–1072)
Warm the catalog with a ready harness, revisit setup, and let its forced discovery reject. The forced-query hook intentionally retains the last good catalog alongside the new error. Clicking that harness now calls actions.next based solely on the cached entry, despite runtimeProviders.errorMessage; selecting API can likewise bypass the failed check using cached Buzz readiness. The base Next button explicitly blocked on this error. This also does not get repaired by the defaults page: it consumes the separate cheap query, whose warm cache can remain successful after the forced-key failure, and trusts the supplied ready IDs. Keep warm rows visible, but gate readiness-based advancement after a failed recheck and provide a working retry; cover the actual selection callback, not only status text. Apply the same failure guard to the detail auto-advance effect. The explicit Set up later escape can remain available.
Scope and verification
Source/diff-only review on Wes’s Mac Studio; no PR checkout, build, tests, browser rendering, or runtime execution. Traced machine onboarding, identity/backup/recovery, provider configuration and native discovery, shared layout consumers, and relevant changed tests. Runtime layout/focus and native workflow behavior remain unverified. The suspected avatar clipping was withdrawn; the larger minimum window size was treated as a product tradeoff, not a defect. Successful-empty API validation was not promoted to a finding because an affected production producer was not established.
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: request changes
Reviewed exact head b0a484f052f72441ff872784c0d507cbbfcc9539 against base bfc384855889432df4a333a0edf3080f332ee169. The live head still matched at submission.
Blocking: relay-recovery controls are exposed but pointer-inoperable
desktop/src/features/onboarding/ui/OnboardingCard.tsx:54-68 puts the refreshed onboarding content inside an overflow-hidden card / clipped smooth-corner containing block. desktop/src/features/onboarding/ui/ProfileStep.tsx:147-159 renders the relay-recovery alert as position: fixed, but it remains a descendant of that clipped card and transition rather than a shell-level overlay.
Both the Integration CI run and an isolated same-head Playwright reproduction resolve the recovery controls in the accessibility tree, then time out on hover/click because onboarding-gate intercepts pointer events; failure screenshots show only a clipped sliver. The reconnect and dismiss journeys fail deterministically across retries. This leaves pointer users without the presented recovery path while assistive semantics misleadingly advertise working controls.
Required fix: render the relay-recovery alert outside the clipped card/transition (for example, via a shell-level portal), or make it an in-card non-fixed alert. Add a regression that actually clicks reconnect and dismiss at the supported minimum window size; visibility or AX presence alone will not catch this failure.
Required merge-gate reconciliation
The same run has deterministic PR-caused failures in Desktop Smoke E2E shard 2 and Desktop E2E Integration shard 1. Besides the actionable defect above, several executable contracts still describe the previous journey/surface (More harnesses, identity help styling, key-import styling, backup-dialog locator, and geometry/avatar expectations). Some may be stale tests rather than product defects, but the refreshed flow cannot merge while its acceptance contracts contradict it.
Required fix: confirm the supported refreshed journey, then update either implementation or tests accordingly and rerun every affected Desktop Smoke and Integration shard green. The mentions-ordering retry-pass appears unrelated/flaky and is not part of this block.
Compatibility decision required
desktop/src-tauri/tauri.conf.json:20-21,33-34 raises the entire application's default/minimum window from the prior 800×600 / 800×500 to 900×650, while OnboardingCard.tsx:54 introduces a fixed 38rem+2px minimum card width. This narrows compatibility for every app surface to accommodate onboarding.
Required fix: either preserve the prior minimum with a responsive card and validate onboarding at 800×500 (including zoom), or link the approved product/platform decision for the application-wide minimum increase and provide regression evidence for the newly supported boundary.
Evidence and residual verification
At the pinned clean SHA, reviewers passed just desktop-check, just desktop-typecheck, full just desktop-test (6,465 passed, 0 failed), and git diff --check. Focused Playwright Integration reproduction failed on the pointer-actionability paths above. Identity-scoped completion/migration, ready-state navigation, restart/concurrency, and renderer↔Tauri tracing found no additional concrete defect.
Native recording, exhaustive keyboard focus/Escape, theme/zoom, and repaired minimum-size evidence remain for follow-up verification after the blocking changes land.
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head d19c1f1b0dfe392e579930fba9182c7014c15cea against base 82656ffea080cc28cec9163ebbf7d1c6be327e43.
[P2] Stabilize the alternate-harness Back callback before publishing it to parent state
desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx:625-633 creates a fresh onInitialListBack function whenever isChoosingDifferentHarness is true. SetupStep.tsx:1043-1065 depends on that function to create navigateBack, then runs an effect that publishes navigateBack through onBackActionChange. The parent callback at MachineOnboardingFlow.tsx:142-145 stores that new function in React state, causing another parent render and another fresh onInitialListBack. There is no identity-stable callback or equality fence to terminate the cycle.
Reproduction to validate: choose API key, reach Connect with an API key, then click Use a different harness. While the harness chooser stays mounted, it repeatedly triggers parent/child renders even without further input, causing continuous render work and React maximum-update-depth warnings in development. The existing E2Es do visit this path, but checking the visible heading/back navigation does not detect continuous effect churn. Memoize the parent’s return-to-config handler (or keep the changing action behind a stable ref) before passing it to SetupStep, and add a regression that detects repeated updates on the mounted chooser.
This loop is introduced by the overall PR relative to base; it was already present in the earlier reviewed PR head and was missed in that review. It is not caused by the current readiness repair.
[P2] Keep discovery updates from replacing the selected-harness handoff
In desktop/src/features/onboarding/ui/SetupStep.tsx:1067-1073, the detail-ready layout effect advances with only [selectedRuntime.id]. But the same successful-discovery render also schedules the passive effect at 1016-1032, which publishes all ready runtime IDs. Both callbacks write MachineOnboardingFlow’s readyRuntimeIds (136-139 and 597-609). React flushes remaining passive effects after a layout-effect state update, so the all-ready publication can replace the selection as the configuration step opens.
Reproduction to validate: on a fresh configuration with a cached catalog containing Buzz plus another ready harness, start a forced recheck and choose Use an API key before it completes. The new guard correctly waits in detail. When discovery succeeds, the selected Buzz handoff is overwritten by the multi-runtime list. With no saved preferred runtime, DefaultConfigStep.tsx:197-205 auto-selects only when there is exactly one runtime, while 247-272 hides the harness picker in API mode. The config then has no selected runtime, and Next remains disabled (215-222): the user must back out/reselect or skip rather than configure the chosen harness. This is a successful-readiness handoff defect, not the previously fixed failed-probe bypass.
Keep catalog readiness and the user’s chosen runtime separate, or suppress the background all-ready publication once the detail step hands off. Add a parent/child regression with multiple ready runtimes, selection during a pending forced probe, and no saved default. The current isolated SetupStep tests collect the callbacks separately and do not exercise their shared parent state.
[P2] Reconcile E2E assertions with the refreshed onboarding
desktop/tests/e2e/onboarding-docked-cta-screenshots.spec.ts:18-20,37 fixes the viewport at 1280×800 and defaults the shared card-width expectation to 610px. All six calls (206, 274, 396, 627, 687, 696) omit an override. The production card now sets min(100vw - 2rem, 50rem) in OnboardingCard.tsx:54, which is 800px at that viewport and default text scale; the shared Card adds no maximum-width constraint. Consequently those geometry assertions reject the intended layout before their later screenshot/interaction checks can run. Update the assertion to the intended 800px contract (or explicit responsive bounds). This is a source-proven test/implementation mismatch, not a claim that this review executed the spec.
The modified desktop/tests/e2e/harness-management.spec.ts:683-692 also selects subscription and then clicks onboarding-setup-more-harnesses, but the refreshed SetupStep renders no such action. Its retained navigateToAgentSettings callback is not invoked by either current footer. Reconcile that E2E with the intended new navigation (or restore the shortcut only if it remains a product requirement); it currently waits for an unavailable control. This is another source-verified test contract mismatch, not a claim that this review ran or independently verified CI.
Earlier findings and verification scope
The previous raw-key exposure, invisible identity-creation errors, active-import method switching, and cached-readiness advancement findings are repaired in this head. The relay-recovery card also now portals outside the clipped onboarding card.
Source/metadata-only review on Wes’s Mac Studio. No checkout, build, tests, browser, or PR-code execution. Native keychain/dialog/pairing behavior, actual providers, and runtime layout/performance remain unverified. Product intent remains the refreshed onboarding journey with preserved identity, drafts, recovery, and readiness boundaries.
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: request changes
Reviewed exact head d19c1f1b0dfe392e579930fba9182c7014c15cea against base 82656ffea080cc28cec9163ebbf7d1c6be327e43. The live head matched at submission.
Blocking: required onboarding smoke contracts contradict the submitted flow
Two required exact-head CI shards are red with deterministic, PR-caused onboarding failures:
- Desktop Smoke E2E (2): 3 onboarding tests fail through both retries.
desktop/tests/e2e/harness-management.spec.ts:634-692was updated for the refreshed subscription journey but still clicksonboarding-setup-more-harnesses; the refreshedSetupStep.tsxno longer renders that action, leaving the productionnavigateToAgentSettingsbinding inMachineOnboardingFlow.tsx:611-620unreachable from setup.identity-key-help.spec.ts:35still requires the old dialogshadow-noneclass although the refreshed inline help rendersw-full, andkey-import-reveal.spec.ts:31still requires the old olive ink although the refreshed card input uses neutral foreground. A fresh isolated local E2E build reproduced these same results: 15 passed, 3 failed. - Desktop Smoke E2E (3): 23 deterministic failures, including 22 onboarding acceptance failures. Nineteen
onboarding-agent-defaults.spec.tscases never reachonboarding-page-config. All three docked-card suites also enforce an internally inconsistent new geometry contract:onboarding-docked-cta-screenshots.spec.ts:20-37defaults to 610px, whileOnboardingCard.tsx:50caps the card at50rem(800px at the test viewport); CI receives 800px on every retry.
Required fix: reconcile the refreshed product journey with its acceptance contracts. Either restore a reachable Settings → Agents affordance or remove the dead navigation contract and test the intended replacement; update superseded dialog/color assertions without discarding masking/reveal and accessibility coverage; settle the shared-card width; and repair the defaults journey so required smoke shards 2 and 3 pass at a new immutable head.
Cleared areas
The previous clipped relay-recovery and application-minimum findings are cleared: Tauri remains at an 800×500 minimum, the onboarding card is viewport-bounded and scrollable, and the relay-recovery card is portaled outside the clipped card. Focused relay recovery passed 3/3, including reconnect and dismiss at minimum size. Review of ACP forced-refresh ordering, setup/default readiness propagation, identity renderer↔Tauri contracts, persistence/recovery, and relay reconnect concurrency found no additional concrete defect.
Exact-head local evidence also passed pnpm check, pnpm typecheck, full desktop pnpm test (6,483 passed), just file-size-check, and scoped Biome checks. macOS/Windows builds, integration, relay, Semgrep, zizmor, and release-candidate checks passed. The unrelated file-attachment smoke failure passed on retry and is not part of this block.
Packaged-native keyboard/VoiceOver/theme lifecycle evidence remains a reviewer/native-tooling confidence gap, not requested author rework. Re-review should use the new exact head and corrected required smoke gates.
— :bot: Jude’s code review agent
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 82656ffea080cc28cec9163ebbf7d1c6be327e43..1ca35e8425a8ae6fddae2a058d9e71396d94fd25 (exact head 1ca35e8425a8ae6fddae2a058d9e71396d94fd25)
Risk: high — this refresh changes a user-visible onboarding state machine, ACP runtime discovery/readiness, persistence/retry journeys, and shared responsive UI contracts.
Behavior/contracts traced: renderer onboarding state and navigation; ACP forced-probe launch/pending ownership and cache handoff; selected-runtime fencing across advance/back; identity-scoped completion/recovery; defaults persistence, retry, masking/reveal, relay recovery, shared-card geometry, and the required Desktop gates.
Findings: no unresolved blocking code, product, or test defect at this head. The prior blockers are resolved:
- The superseded More-harnesses path is removed end to end, including its dead navigation state and obsolete acceptance contract.
- Forced ACP probes now expose owner-controlled pending state, while the existing dedup/cancellation/final-cache ordering remains intact.
- Catalog updates cannot overwrite the selected runtime after advancing; Back explicitly reopens selection. Unit and E2E regressions cover the cached multi-ready/pending-force seam.
- Identity-help and key-input assertions now describe the refreshed surfaces; defaults/retry journeys reach configuration; the shared-card test now matches the production
50rem/ 800px cap. - The previously cleared relay-recovery pointer behavior and
800×500minimum-window contract remain intact.
Author action: none.
Verification owner: CI/release gate for the still-running exact-head Desktop Core job; release/native QA for an optional packaged VoiceOver, keyboard, and theme spot-check.
Validation at clean exact head 1ca35e8425a8ae6fddae2a058d9e71396d94fd25:
- Full Desktop unit suite: 6,479 passed, 0 failed.
- Desktop check, typecheck, file-size check, E2E build, and
git diff --check: passed. - Focused corrected contracts: 5/5 passed.
- Full defaults + docked-card suites: 29/29 passed.
- Impacted onboarding acceptance set (
harness-management, identity help, key import/reveal, defaults, docked-card): 46/46 passed. - GitHub Desktop Smoke E2E shards 1–4: SUCCESS at submission time. Desktop Core remained IN_PROGRESS; that is a named external gate, not evidence of an author-actionable defect.
Manual/native evidence: generated browser screenshots for subscription selection and revealed/copyable backup-key states were inspected and were readable and contained. No fresh packaged-native VoiceOver/keyboard/theme recording was obtained.
Residual risk: packaged-native assistive-technology and theme behavior was not independently witnessed at this head. Automated semantics, focused journeys, responsive checks, and screenshots found no concrete failure; ownership remains with native/release verification rather than the PR author.
— :bot: Jude’s code review agent
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear
Reviewed head 1ca35e8425a8ae6fddae2a058d9e71396d94fd25 against base 82656ffea080cc28cec9163ebbf7d1c6be327e43, with the follow-up diff from d19c1f1b0dfe392e579930fba9182c7014c15cea.
No remaining actionable code/product blocker found in the reviewed scope. The earlier callback-feedback loop, selected-runtime overwrite, and stale E2E contracts are repaired. The stable Back callback breaks the render cycle; the synchronous parent handoff fence preserves explicit selection; forced-query launch/pending ownership covers completion through shared-cache publication. Normal Back releases the selection fence, while alternate-picker Back preserves the configuration draft. The obsolete More-harnesses navigation and its tests are removed together.
Non-blocking coverage improvement: desktop/tests/e2e/onboarding-agent-defaults.spec.ts:267-288 primes the catalog by first entering configuration, which can also preserve a Buzz draft. On the second trip, DefaultConfigStep.tsx:71-88,144-147 can select Buzz from that draft even if a catalog broadcast replaces the selected-ID list. Thus this E2E does not isolate the repaired parent handoff fence. Prefer a cached multi-ready first handoff with no prior draft, or a mounted parent test that sends the selection then the catalog broadcast. The new isolated SetupStep test verifies the selected callback payload, not the parent’s shared state. This is a coverage limitation, not evidence that the fixed product path still fails; no mutation test was executed.
Source/metadata-only review on Wes’s Mac Studio. No checkout, build, tests, browser, imports, or PR-code execution. Earlier unchanged identity/recovery and community-flow evidence was retained; actual native keychain/dialog/pairing, provider execution, rendered layout/accessibility, and runtime performance remain outside this verification. Product intent remains refreshed onboarding with preserved identity, drafts, recovery, and readiness boundaries.
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: approve
Reviewed exact head 1ca35e8425a8ae6fddae2a058d9e71396d94fd25 against base 82656ffea080cc28cec9163ebbf7d1c6be327e43. The live head matched at submission.
The prior required-gate blockers at d19c1f1b0dfe392e579930fba9182c7014c15cea are resolved:
- The superseded More-harnesses route is removed consistently from
App.tsx,MachineOnboardingFlow.tsx, the setup action type, and its obsolete unit/E2E contracts; no stale symbol or test ID remains indesktop/srcordesktop/tests. - Fast forced ACP discovery now exposes owner-controlled started/pending state (
acpRuntimesQuery.ts:275-304), so a batched fast response cannot strand readiness. Forced-key dedup and the awaited cheap-query cancellation/final forced-cache write remain intact. - Setup-to-config selection is fenced during handoff and reopened on Back (
MachineOnboardingFlow.tsx:122-140,271-279,593-614), preventing a later catalog update from replacing the chosen runtime. The corrected unit and production-seam E2E regressions cover cached multi-runtime readiness during a pending forced probe. - Identity-help and key-input assertions now describe the refreshed inline/neutral surfaces; defaults journeys reach configuration; and the shared-card expectation now matches the production
50rem/800px geometry.
Verification
At the pinned head, reviewers passed:
- Full Desktop unit suite: 6,479 passed, 0 failed.
pnpm check,pnpm typecheck, file-size checks, and E2E build.- Focused corrected contracts: 5/5 passed.
- Full defaults plus docked-card suites: 29/29 passed.
- Fresh impacted acceptance set across harness management, identity help, key reveal, defaults, and docked-card coverage: 46/46 passed.
- Focused relay-recovery coverage remained 3/3 passed, including reconnect and dismiss at the 800×500 minimum.
Exact-head CI completed Desktop Core successfully, with all four Desktop Smoke E2E shards, macOS/Windows builds, integration, relay, security, and release-candidate checks successful. Renderer↔Tauri identity contracts, identity/tenant persistence and recovery, ACP refresh concurrency, defaults persistence/retry, responsive containment, and the prior relay-recovery/minimum-window concerns revealed no unresolved concrete defect.
Fresh packaged-native VoiceOver, full keyboard traversal, and OS-theme lifecycle observation remain a release/native-QA confidence gap, not author rework; current automated semantics and focused journeys establish no failure.
— :bot: Jude’s code review agent
Signed-off-by: kenny lopez <klopez4212@gmail.com>
…-card-refresh Signed-off-by: kenny lopez <klopez4212@gmail.com>
…sh' into kennylopez-onboarding-card-refresh Signed-off-by: kenny lopez <klopez4212@gmail.com> # Conflicts: # desktop/src/features/onboarding/ui/OnboardingCard.tsx
Signed-off-by: kenny lopez <klopez4212@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Review clear
No blocking code, product, or security defects found at head 426bf88daf63312a94b7bda346c76b6c4a46b975, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa.
This is a source-only review, not a GitHub approval or a claim that CI is green.
Scope and evidence
- Retained the previous clear review: range-diff confirms its four patches are unchanged. Reviewed the added avatar refinement
222b136c90cb895dc7c16bfd52813385113fa67dand the merged Codex readiness contract, rather than reopening resolved findings. - Traced image/URL upload, emoji and custom-color editing, camera selection/live/review/apply, pending-save navigation, disabled/focus behavior, and shared profile/community consumers. The new inline presentation is opt-in; existing modal/settings consumers retain their presentation. Checked updated E2E selectors, geometry assertions, and production callback wiring. Independent avatar, test-contract, and runtime-integration lanes are complete.
- Preserved the existing identity/recovery and defaults-persistence contracts. Mobile/relay behavior beyond the desktop call boundary and unrelated changes already on main were not re-reviewed.
Non-blocking note
For adapter_outdated, the setup detail says Codex is not detected, although it was detected but needs updating. State-specific copy would be clearer. I am not requesting restoration of one-click installation: discovery supplies the adapter-specific guide and Check again forces rediscovery; no broken recovery path was established from source.
Validation limits
All repository inspection ran on Wes's Mac Studio using pinned Git objects. No checkout, build, test, browser session, or PR-code execution was performed. Current GitHub checks are not all green: Desktop E2E Integration (1/2) failed, and other desktop checks were pending at the review snapshot. CI and native/runtime validation remain external gates; historical test passes apply only to their recorded heads.
Carl, an automated reviewer, commenting via Wes's GitHub account.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..0171ec233d3802025b75a5fc9927226cc356d7fd (exact head 0171ec233d3802025b75a5fc9927226cc356d7fd)
Risk: high — the PR changes onboarding, avatar capture/editing, identity-backed profile persistence, ACP readiness, and shared responsive behavior.
Blocking — keep the existing profile editor height stable when camera startup completes
desktop/src/features/profile/ui/AnimatedAvatarCameraControls.tsx:46-49,90-125 now creates cameraAction only when retry/live becomes true. In the default non-stacked profile editor, that changes the grid from picker-only to picker + the h-14 action row + parent gap-4: a deterministic 72 px growth when the camera becomes live. The profile dialog visibly jumps during ordinary camera initialization.
This violates the existing stability contract at desktop/tests/e2e/animated-avatar.spec.ts:106-113. At this exact clean head:
pnpm exec playwright test --project=smoke \
tests/e2e/onboarding-avatar-skip.spec.ts \
tests/e2e/animated-avatar.spec.ts
produced 9 passed, 1 failed after all 3 attempts; each failure was Expected <= 1; Received 72 at line 113. The failure screenshot confirms the vertical dialog growth.
Author action: preserve the action-row slot in the default non-stacked profile editor during idle/starting states, while keeping the new collapsing/animated behavior scoped to the stackCameraOptions onboarding presentation. The existing ≤1 px stability assertion must pass without weakening it; rerun both focused specs on the replacement exact head.
Integrated review evidence
The follow-up 426bf88d..0171ec23 changes only four renderer/test files (+28/-4), does not touch the defective camera-control component, and introduces no Tauri, persistence, schema, migration, lockfile, or dependency change. The onboarding-specific avatar acceptance otherwise passed: Skip, required-avatar Next, identity preservation, Back behavior, compact navigation, and reviewed image/animated/custom-color/emoji states. Systems tracing found no additional author-actionable defect: avatar changes still flow through the onboarding profile draft and identity-scoped update_profile; upload remains blocked in flight and non-image MIME fails closed; profile-query cancellation/cache ownership remains relay URL + current pubkey; no renderer↔Tauri payload, schema, migration, or lockfile contract changed.
Exact-head checks passed:
pnpm install --frozen-lockfilejust desktop-checkjust desktop-typecheckjust desktop-test— 6,484 passed, 0 failedjust file-size-checkCI=1 pnpm build:e2e- focused onboarding avatar smoke — 7 passed; combined avatar/profile set — 9 passed, 1 deterministic failure
git diff --check 051c3a270be9c73da9ab06700bcab7d5552fceaa...HEAD
Verification owner: reviewer for the focused profile/onboarding rerun after the author fix; CI for the independent relay-startup/A3 failures.
Residual risk: no packaged-native macOS evidence was obtained for real camera selection, permissions, Continuity Camera, or device lifecycle. The current integration failures inspected did not reach Playwright; relay startup timed out in an object-store conformance probe, and this PR range has no relay/object-store change. Those are separate confidence/gate issues, not the basis for this request.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..0171ec233d3802025b75a5fc9927226cc356d7fd (exact head 0171ec233d3802025b75a5fc9927226cc356d7fd)
Risk: high — this changes the onboarding state machine and shared avatar editor, including camera lifecycle, responsive layout, profile persistence, and assistive interaction surfaces.
Blocking — preserve the Profile avatar editor height when the camera becomes live
desktop/src/features/profile/ui/AnimatedAvatarCameraControls.tsx:46-49,90-125 now mounts the h-14 pt-2 camera action only when retry/live state exists. In the default, non-stacked Profile presentation, the parent also contributes gap-4, so camera startup completion inserts exactly 72 px instead of preserving the editor's reserved action slot.
The existing production-seam acceptance contract at desktop/tests/e2e/animated-avatar.spec.ts:60-113 fails deterministically: idle → starting stays stable, but idle → live reports Expected <= 1; Received 72. Independent clean-head executions reproduced that exact result on every attempt. Users see the Profile avatar editor jump vertically as camera initialization settles.
Author action: keep the new collapsing/animated action row scoped to the stacked onboarding presentation, while reserving the action-row height in the default/non-stacked Profile editor even when idle. Make the existing ≤1 px stability assertion pass; do not weaken that contract.
Other reviewed contracts: The bounded 426bf88d..0171ec23 compact-card change introduces no Tauri/native, persistence, migration, schema, lockfile, or dependency contract. Picker → onboarding draft → profile mutation ownership remains intact, uploads remain blocked while pending, and stale profile reads remain cancelled around mutation. All seven focused onboarding-avatar rows pass, including compact 610 px non-avatar cards, wider stable avatar cards, equal Skip/Next radius, mode switching, Skip, avatar-required Next, identity preservation, and Back.
Validation at clean exact head:
pnpm check— PASS (repository-wide pre-existing advisory diagnostics only)pnpm typecheck— PASSpnpm test— 6,484 passed, 0 failedjust file-size-check— PASS (10/10 policy tests plus all surface checks)pnpm build:e2e— PASSpnpm exec playwright test --project=smoke tests/e2e/onboarding-avatar-skip.spec.ts tests/e2e/animated-avatar.spec.ts— 9 passed, 1 failed; the sole failure is the 72 px live-camera regression above- two additional isolated reruns of that live-camera case at the prior unchanged implementation reproduced the same 72 px failure
git diff --check— PASS
All cited local commands ended at the stated clean HEAD. The new-head GitHub Desktop matrix was still running at submission; an earlier integration-shard failure stopped during relay A3 startup before Playwright and is an external CI issue, not the author-actionable renderer defect above.
Verification owner: author fixes the shared default-profile action-slot regression; reviewer reruns the focused animated-avatar plus onboarding-avatar set at the replacement exact head. CI owns its required matrix. Release/native QA owns packaged Continuity Camera, permissions, VoiceOver, keyboard, and theme observation.
Residual risk: mocked Playwright evidence does not establish packaged Tauri camera/device-permission behavior. That confidence gap is separate from the deterministic renderer failure.
— :bot: Jude’s code review agent
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Review clear
No blocking code, product, or security defects found at head 998e714c31d383a43d2da51d3c83bd879356c728, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa.
- Retained the previous clear review. Range-diff confirms its five patches are unchanged; this review covers the two added commits and their shared consumers. The contract remains simpler onboarding without changing identity, recovery, or defaults persistence.
- Checked compact sizing across machine, relay, and community onboarding, the explicit wide-avatar exception, footer/navigation reachability, skip-button behavior, and default/community/inline camera-control transitions. The camera action spacer is restored for non-stacked consumers without changing record/retry callbacks. Updated geometry and copy tests bind the production components.
- The prior non-blocking outdated-adapter copy note is fixed in runtimeUnavailableDescription. The guide-first setup and forced rediscovery flow remain intact; this is not a request to restore one-click installation.
Validation: source/metadata-only review on Wes's Mac Studio using pinned Git objects; git diff --check passed for the exact base/head pair. No checkout, build, tests, browser session, or PR-code execution. This is not GitHub approval or evidence of green CI or native/runtime behavior. Unchanged earlier contracts retain their recorded review evidence; mobile/relay internals and unrelated main changes were not re-reviewed.
Carl, an automated reviewer, commenting via Wes's GitHub account.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..998e714c31d383a43d2da51d3c83bd879356c728.
The changed head resolves the prior Profile avatar-editor stability defect without disturbing the onboarding-specific compact presentation.
- Default/non-stacked Profile measured 212 px idle → 212 px starting → 212 px live: 0 px idle-to-live growth, satisfying the existing ≤1 px contract.
- Stacked onboarding keeps its intentional internal collapse while the outer shell remains stable.
- A causal mutation restoring the prior conditional slot failed the production-seam assertion 3/3 with the original 72 px growth; restoring this head returned the suite green.
Author action: none.
Verification owner: CI owns any still-running/failing required shard; release/native QA owns packaged macOS camera permissions, physical/Continuity Camera, keyboard, VoiceOver, and theme observation. These are confidence gaps, not evidence of an author-actionable defect in this head.
Validation at clean exact head: focused avatar/onboarding Playwright 10/10 passed; focused geometry rerun 2/2 passed; Desktop checks/typecheck passed; full Desktop unit package 6,484/6,484 passed; file-size and git diff --check passed. The bounded five-file Desktop delta adds no IPC/Tauri command, persistence/schema, dependency, or identity/community contract change.
Residual risk: browser evidence used the mock Tauri camera bridge; packaged native camera/permission behavior remains unobserved.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..998e714c31d383a43d2da51d3c83bd879356c728 (exact head 998e714c31d383a43d2da51d3c83bd879356c728)
Risk: high — the PR changes onboarding state/navigation, camera-based avatar editing, identity-backed profile persistence, ACP readiness, and shared responsive UI.
Findings: no unresolved author-actionable defect. The prior Profile editor stability blocker is fixed in AnimatedAvatarCameraControls.tsx:46-50,121-124: default/non-stacked mode always reserves the fixed h-14 camera-action slot, while stacked onboarding retains its intentional 0rem ↔ 3.5rem internal row transition.
Independent exact-head measurement found the normal Profile editor remained 212 px idle → 212 px starting → 212 px live (0 px delta, contract ≤1 px). The onboarding shell remained 420 px while its internal option/action region compacted from 204 px to 168 px, preserving the intended stacked presentation and bottom alignment.
Regression falsifiability: replacing the fix with the prior implementation caused animated-avatar.spec.ts:113 to fail on the initial attempt and both retries with the original 72 px growth. Restoring exact-head code returned the same production-seam test green. This proves the regression test detects the defect rather than merely decorating the dungeon wall.
Behavior/contracts traced: camera idle/starting/live/retry rendering; normal Profile versus stacked onboarding ownership; stream acquisition/release and recording transitions; avatar upload/apply and host-profile persistence; responsive card and navigation behavior. The bounded five-file repair adds no Tauri command or payload, durable write, migration/schema, dependency, or identity/community boundary.
Author action: none.
Verification owner: CI owns the unrelated exact-head Smoke E2E shard 3 failure; native/release QA owns a physical-camera/macOS-permission and Continuity Camera spot-check.
Validation at clean exact head:
just desktop-check— PASSjust desktop-typecheck— PASSjust desktop-test— 6,484/6,484 PASSjust file-size-check— PASS- E2E build — PASS
animated-avatar.spec.ts+onboarding-avatar-skip.spec.ts— 10/10 PASS in one run; focused measured rerun 2/2 PASSgit diff --check— PASS- macOS and Windows builds plus relay-backed integration — SUCCESS in exact-head CI
CI classification: Smoke shard 3 currently fails in unchanged, out-of-range surfaces: profile-custom-emoji-status.spec.ts failed 3/3 waiting for status-expiry validation, while persistent-agent-audience and navigation/composer cases also failed or retried. The bounded repair changes only onboarding setup/harness tests and avatar-camera controls; it does not touch custom-status, audience, navigation, or composer code. This is a real external gate for CI ownership, but present evidence does not make it author rework for this PR.
Manual/native evidence: browser/mock geometry and interaction were exercised and mutation-proven. No packaged native physical-camera/permission/Continuity Camera run was obtained.
Residual risk: native OS permission/device-routing behavior was not independently witnessed. The repair is renderer-only and leaves acquisition/release and persistence paths unchanged; this remains release/native verification rather than an unresolved code defect.
— :bot: Jude’s code review agent
Signed-off-by: Clay Delk <clay.delk@gmail.com>
|
🤖 Clay and I reviewed the refreshed onboarding flow and pushed a follow-up content and layout pass in AI connection
Backup creation and verification
Key recovery
Layout fixes
We intentionally left Create a new identity key unchanged for now so the team can align on that wording separately. |
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..819694cd970b93026ffcba7df56bbb69f427026a (exact live head 819694cd970b93026ffcba7df56bbb69f427026a)
Risk: high — onboarding crosses identity backup/recovery, native persistence, navigation, and first-run configuration; this head itself is primarily copy/layout refinement, but it changes a required integration assertion without updating all consumers.
Concrete defect
desktop/tests/e2e/onboarding.spec.ts:1894-1898still waits for the old heading, “Configure your default model settings,” while production intentionally changed that heading to “Choose your model settings” atdesktop/src/features/onboarding/ui/DefaultConfigStep.tsx:396-400. Exact-head CI job Desktop E2E Integration (1/2) fails this assertion on the initial attempt and both retries; an independent exact-head local focused run reproduced it 1/1. This is a PR-caused required-gate failure, not a navigation defect: the artifact reaches the config route with the new heading and expected Claude Code default.- Author action: update the regression to assert the intentional new heading while retaining its route, harness, and enabled-Next checks. If the old copy is contractual, restore that copy instead.
- Verification owner: author + CI; reviewer re-checks the new immutable head and affected gate.
Behavior/contracts traced
- Backup state remains truthful: completion follows successful native save; cancellation/save failure rolls back; wrong-file/wrong-password paths remain retryable; skip/back do not claim verification.
- The passphrase crosses the native encryption boundary without passing raw nsec into
EncryptedBackupCreator; async completion remains request-correlated. - Imported/recovered identity completion clears the continuation ref before completion, avoiding pinned onboarding state.
- Community → config back navigation is intact; setup/config navigation remains owned by the onboarding flow.
Validation at clean exact head
just file-size-check— PASSgit diff --check— PASSjust desktop-check— PASS (reported warnings/info are in unchanged out-of-range files)just desktop-typecheck— PASS- full Desktop package — 6,484/6,484 PASS
- E2E-mode build — PASS
onboarding-backup.spec.ts— 11/11 PASS independently in both review lanes- focused failing integration row — 1/1 FAIL, reproducing the stale heading assertion
- exact-head CI: Windows and macOS Desktop builds PASS; integration shard reports 1 failed, 1 flaky, 1 skipped, 135 passed
- product probes: success
Continuerendered; controls remained reachable at 400×500 and 640×360 without document horizontal overflow
Non-blocking confidence gaps / residual risk
name-only community profile save preserves an existing avatarfailed once and passed retry. Final behavior preserved the avatar, while payload timing differed. Author action: none for this PR. Verification owner: Desktop test owner/CI to harden or clarify the payload contract.- Packaged native save-dialog behavior, VoiceOver announcement/order, and OS keyboard behavior were not independently observed; browser journey evidence uses the Tauri mock bridge. Author action: none. Verification owner: native/release QA.
- Reviewer-only narrow/zoom and success-control probes add no durable regression row. Given the bounded prose/alignment change and passing production journey, this is not required author work.
The product path works. The required gate is stepping on copy the PR deliberately removed—an unusually polite rake, but still a rake.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..819694cd970b93026ffcba7df56bbb69f427026a (exact head 819694cd970b93026ffcba7df56bbb69f427026a)
Risk: high — the PR changes onboarding navigation, identity backup/recovery, native-backed key persistence, provider setup, and shared responsive UI. This round reviewed the one-commit delta from prior approved head 998e714c31d383a43d2da51d3c83bd879356c728.
Blocking finding — intentional copy change leaves a required integration test stale
desktop/src/features/onboarding/ui/DefaultConfigStep.tsx:396-406 intentionally changes the normal config heading to “Choose your model settings.” However, desktop/tests/e2e/onboarding.spec.ts:1892-1902 still navigates back to that production screen and requires “Configure your default model settings.” The exact-head relay-backed integration shard failed that assertion on the initial attempt and both retries (job 102933956463); a clean local reproduction of the named test also failed 1/1. The artifact confirms navigation itself is sound—the expected config route, new heading, Claude Code default, and enabled completion control are present—so this is a stale regression assertion, not a product-navigation defect.
Author action: update desktop/tests/e2e/onboarding.spec.ts:1894-1898 to assert the intentional new heading while retaining the route, harness, and enabled-completion checks. If the old text is contractual, restore that production copy instead. Rerun the affected integration gate on the new immutable head.
Verification owner: author + CI for the corrected required gate; reviewer for the new-head delta and failure classification.
Integrated findings and validation
No second product, accessibility, persistence, or lifecycle defect was established. Source tracing and browser evidence show:
- Backup remains create → successful native save → optional file/password verification → continue; cancel/save failure does not claim creation, wrong-file/password errors remain retryable, and skip/back remain available (
EncryptedBackupCreator.tsx:516-560,BackupTestFlow.tsx:285-344,DownloadKeyStep.tsx:152-177). - Imported/recovered identity completion clears the continuation fence before completion; the community → config back handoff reaches the intended screen.
- At 400×500 and 640×360 stress sizes, the changed backup controls remained reachable without document horizontal overflow.
Validation at clean exact head: just desktop-check PASS (warnings/info only in unchanged files); just desktop-typecheck PASS; full just desktop-test 6,484/6,484 PASS; just file-size-check PASS; E2E-mode build PASS; complete onboarding-backup.spec.ts 11/11 PASS; git diff --check PASS. Windows and macOS Desktop builds passed in exact-head CI. Authenticated reviewer jedwards27 differs from PR author klopez4212.
Confidence gaps / residual risk: packaged native save-dialog behavior, VoiceOver order, and OS keyboard behavior were not independently observed; native/release QA owns those checks. A separate name-only profile-save test failed once then passed on retry while preserving the avatar payload, so CI/test ownership should track that flake. Neither confidence gap is the basis for this verdict.
A new head invalidates this verdict until its delta and required gate are reviewed.
wesbillman
left a comment
There was a problem hiding this comment.
Changes requested
One P2 test-code regression at head 819694cd970b93026ffcba7df56bbb69f427026a, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa.
P2: Update all registered onboarding assertions for the revised copy
The new connection-method cards intentionally remove both descriptions, the default-config heading is now “Choose your model settings,” and the phone-recovery instructions have changed. Four registered E2E specs still assert the previous copy contract:
-
onboarding-agent-defaults.spec.ts:117-126 waits for the two deleted descriptions before it can test subscription/API routing.
-
onboarding-docked-cta-screenshots.spec.ts:390-395 requires the same deleted descriptions inside the cards before its later geometry/navigation assertions.
-
onboarding.spec.ts:1892-1898 returns from first-community setup and waits for “Configure your default model settings,” which DefaultConfigStep:396-406 no longer renders.
-
identity-lost.spec.ts:115-122 opens lost-identity phone recovery, requires the new “Scan this code with a device…” sentence to have count zero, and then requires the removed Settings instruction to be visible. MachineOnboardingFlow:461-468 now renders that new sentence in the loading/QR state.
The method-card and lost-identity specs run in smoke; the first-community spec runs in integration (desktop/playwright.config.ts:28,148,155,189). Following the existing named test paths reaches assertions that cannot match the rendered UI and will time out, preventing the remaining behavior checks. This is a source-established regression from the previous head, not an observed test-run result.
Smallest fix: keep the intended new UI copy; replace the deleted-description assertions with the surviving method labels, update the old config-heading assertion, and make the recovery assertion require the new sentence instead of rejecting it. Retain routing, geometry, back-navigation and QR/recovery coverage. No product-copy rollback or new machinery is needed.
Scope and limits
Retained the previous review: range-diff confirms all seven prior patches are unchanged and adds one eight-file commit. Reviewed the new backup labels/button layout, optional verification/recovery navigation, shared card minimum-height override, connection-method presentation, default-config copy and corresponding test consumers. The product contract remains simpler onboarding without changing key security, recovery, defaults persistence, or guide-first runtime setup. Unchanged mobile/relay internals and prior resolved contracts were not reopened.
Source/metadata-only review on Wes's Mac Studio with pinned Git objects. Exact base/head git diff --check passed. No checkout, build, test, browser session, or PR-code execution; no current-CI or runtime-success claim.
Carl, an automated reviewer, commenting via Wes's GitHub account.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..6c4a56d9165aa86140e492e9603b5193c6cc4d63 (exact head 6c4a56d9165aa86140e492e9603b5193c6cc4d63)
Risk: critical — onboarding handles the reusable private identity credential, backup/recovery, native persistence, and first-run agent configuration. The replacement head introduces a direct secret-exposure regression and leaves required acceptance gates stale.
Blocking findings
-
The reusable private identity key is exposed by default.
desktop/src/features/onboarding/ui/BackupStep.tsx:114-127now callsgetNsec()on mount, and:374-382renders the returned nsec as ordinary selectable text. Concealment occurs only while the key well is hovered or focused (:377-388), so the resting screen, screenshots/screen shares, DOM, and assistive output receive the credential. The changed test atdesktop/tests/e2e/onboarding-backup.spec.ts:129-139now explicitly requires that default exposure, reversing the prior explicit Reveal/Hide boundary. Exact-head screenshot evidence also visibly contains the mock nsec.- Author action: restore concealed-by-default behavior and require deliberate Reveal/Hide (or an equivalent explicit activation that does not fetch/render the key on mount); retain explicit Copy and retry behavior. Assert that initial DOM/AX output excludes the nsec, Reveal exposes it, and Hide removes/conceals it.
- Verification owner: author + Desktop smoke/AX/native QA.
-
The previous stale-heading blocker remains, with additional stale acceptance assertions. Production renders “Choose your model settings” at
desktop/src/features/onboarding/ui/DefaultConfigStep.tsx:396-400, butdesktop/tests/e2e/onboarding.spec.ts:1973-1983still requires “Configure your default model settings.” Focused exact-head smoke additionally failed stale assertions inonboarding-agent-defaults.spec.ts:117-126,409-410,580-582andidentity-lost.spec.ts:115-122. Result: 53 passed / 4 failed, with every failure repeated through both retries. Exact-head CI independently reports required Desktop E2E Integration failures (jobs102965879736,102969172237, aggregate102969111685).- Author action: update all affected consumers to the intended surviving copy while preserving route, harness selection, enabled-completion, QR, and recovery assertions; rerun the required smoke and integration gates.
- Verification owner: author + Desktop smoke/integration CI.
-
“I’ve saved my key” records an unearned security action.
desktop/src/features/onboarding/ui/BackupStep.tsx:442-451enables that declarative success claim after only the cosmetic creation hold;backupNextDisabled()is unconditionally false at:51-54. Displaying a key is not evidence that the user saved it, and locked backup remains optional.- Author action: use truthful non-assertive copy such as Continue / Skip for now, or gate the claim on an observed copy/download/backup success. Cover both the unsaved path and the chosen completion event.
- Verification owner: author + product/E2E.
Validation
At clean exact head 6c4a56d9165aa86140e492e9603b5193c6cc4d63:
git diff --check 051c3a270be9c73da9ab06700bcab7d5552fceaa...HEAD— PASS- isolated
CI=1 pnpm --dir desktop test:e2e:smoke -- onboarding-backup.spec.ts onboarding-agent-defaults.spec.ts identity-lost.spec.ts onboarding-docked-cta-screenshots.spec.ts— 53 passed / 4 failed; each failure survived both retries - generated docked-backup screenshot inspected — default mock nsec visibly rendered
- Windows Desktop build, DCO, Rust lint, Semgrep, and integration shard 2/2 — PASS at review time; integration shard 1/2 and aggregate — FAIL
- authenticated reviewer
jedwards27; live PR authorklopez4212
Systems tracing found the prior heading assertion unchanged and independently confirmed the on-mount get_nsec → renderer DOM exposure. The final systems-lane gate summary did not arrive after its process ended and two explicit callbacks; this is a reporting/tooling confidence gap, not the basis for the defects above. The product/UI lane completed on a clean tree and supplied the focused counts and screenshot proof.
Confidence gaps / residual risk: packaged native save-dialog behavior, VoiceOver ordering, OS keyboard traversal, dark theme, and additional zoom matrices were not independently completed. Author action: none beyond the concrete fixes above. Verification owner: native/release QA and reviewer/tooling.
A new head invalidates this verdict until its delta and affected gates are reviewed.
wesbillman
left a comment
There was a problem hiding this comment.
Changes requested
Three blockers at head 6c4a56d9165aa86140e492e9603b5193c6cc4d63, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa: two new production regressions and the partially fixed prior test-copy regression.
P1: Keep the private key concealed until an explicit Reveal action
BackupStep.tsx:114-127 now calls getNsec() on mount; lines 374-388 put the complete reusable secret into an ordinary text node. The new CSS blurs it only while hovering or focusing the key well. The resting state is readable, and CSS blur does not remove the secret from the accessibility tree. The prior explicit Reveal/Hide control was deleted.
Reproduction/impact: create a new identity and leave the pointer outside the key well. After the creation animation, the full private key appears without choosing Reveal or Copy. A screen share, recording, screenshot, or nearby observer can capture the credential and impersonate this identity. Returning from the optional encrypted-backup view exposes it again. The native get_nsec command returns the actual signing secret, not a masked display token (commands/identity.rs:218-223).
Smallest fix: restore concealed-by-default rendering and deliberate Reveal/Hide, keeping Copy separate. Do not fetch the raw key merely to display the resting placeholder; concealment must remove it from DOM/accessibility text rather than only blur it. Restore the regression assertion that initial render and Hide contain no secret. The updated test currently asserts default exposure instead (onboarding-backup.spec.ts:129-139).
P2: Do not let the first profile save satisfy the pre-existing-profile gate
App.tsx:323-333 now mounts an actionable OnboardingFlow while useFirstRunOnboardingGate is still blocking. That gate has not yet decided whether a profile existed before onboarding.
Reproduction/impact: on the fresh-key continuation path with a configured community, delay the initial profile read, then enter a name and submit before it settles. useUpdateProfileMutation:536-577 cancels the in-flight read and writes the newly created profile into the same query cache. A successful kind:0-backed response has hasProfileEvent: true. The still-unsettled gate:371-415 interprets that as an existing profile, persists onboarding completion, and moves to ready. AppReady then removes the flow, bypassing the intended avatar step and completeAndShowWelcome path. Background starter setup uses focus: false, unlike explicit completion (hooks.ts:574-585,621-635).
Smallest fix: keep the visual continuation, but prevent submission until the initial identity/profile check settles, or explicitly latch the fresh-user gate open before accepting profile writes. Add a delayed-initial-read + immediate-submit regression that reaches avatar and focuses Welcome only through final completion. The new test does not exercise this: its profileHasEvent: false override returns before profileReadDelayMs is applied (e2eBridge.ts:6746-6760), and it never submits a name.
P2: Finish updating the registered test consumers of the revised copy
The prior test-copy finding is only partly fixed. onboarding-docked-cta-screenshots.spec.ts now uses the surviving method labels, but three registered spec files still contradict production:
- onboarding-agent-defaults.spec.ts:117-126 waits for the deleted connection-method descriptions. Its 409-410 and 580-582 assertions also require “Choose your model settings” to be absent after reaching subscription configuration, although
DefaultConfigStep.tsx:396-400now renders that heading there. - onboarding.spec.ts:1973-1983 returns from first-community setup and waits for the removed “Configure your default model settings” heading.
- identity-lost.spec.ts:115-122 rejects the new recovery sentence and requires the old Settings instruction, whereas
MachineOnboardingFlow.tsx:468-471renders the new sentence in the loading/QR state.
Following these existing test paths reaches assertions that cannot match the current UI, preventing the later routing/recovery checks. The first and third specs are registered in smoke; onboarding.spec.ts is registered in integration (playwright.config.ts:148,155,189). Keep the intended production copy and update these assertions without deleting behavioral coverage. These are source-established contradictions, not a claim that I ran the suites.
Scope and limits
Retained the prior review and checked the one-commit, 15-file delta from 819694cd970b93026ffcba7df56bbb69f427026a: machine-to-community continuation, community/identity gates, key presentation, encrypted save/cancel/retry and optional verification controls, card/footer/profile layout, native window sizing, and affected test consumers. Product direction remains simpler card-based onboarding while preserving key safety, recovery, defaults persistence, and guide-first runtime setup. Optional backup verification and user attestation are not new blockers; no evidence justifies adding mandatory backup machinery.
Source/metadata-only review on Wes's Mac Studio with immutable Git objects. Exact base/head git diff --check passed. No checkout, build, test, browser/native app session, or PR-code execution. Native layout, screen-reader behavior, zoom, real relay timing, and current CI success were not runtime-verified. Unchanged mobile/relay internals and previously resolved contracts were not reopened.
Carl, an automated reviewer, commenting via Wes's GitHub account.
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
|
@wesbillman @jedwards27 Addressed the current-head findings in
Author verification on the exact committed tree: — Princess Donut |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..29c84fae48dc325b274fd51fbd1db065069eb788 (exact head 29c84fae48dc325b274fd51fbd1db065069eb788)
Risk: critical — onboarding handles the reusable private identity credential, backup/recovery, native persistence, and first-run agent configuration. The replacement head was reviewed against every prior blocker and the complete changed boundary.
Findings: no unresolved author-actionable defect.
- Credential boundary repaired:
desktop/src/features/onboarding/ui/BackupStep.tsx:89-169,389-441no longer fetchesget_nsecon mount. Initial DOM/AX content is a fixed bullet mask; only explicit Reveal or Copy retrieves the key. Hide removes the nsec from rendered text; Copy remains explicit and does not reveal it. Unmount fencing prevents late state writes. The acceptance path asserts no initial nsec orget_nsec, Reveal/Hide, masked Copy, retrieval failure, retry, and post-copy concealment (desktop/tests/e2e/onboarding-backup.spec.ts:129-157,484-535). - Truthful state repaired: the ungated key step now says Continue, not “I’ve saved my key” (
BackupStep.tsx:476-485), so it no longer records an unearned backup claim. - Acceptance consumers repaired without weakening behavior: connection-method, model-heading, first-community return-route, and phone-recovery assertions now match production while retaining harness selection, no-intermediate-page, selected-default, enabled-completion, QR, and recovery checks (
onboarding-agent-defaults.spec.ts:114-140,390-412,561-582;onboarding.spec.ts:1973-1983;identity-lost.spec.ts:112-130). - Systems boundary remains sound: the changed head does not alter the native key-retrieval contract. Identity import still persists before active-key swap; recovery flags clear after durable commit; stale-backup cleanup remains last/best-effort; encrypted-backup progression follows successful save; machine-flow navigation retains one owner.
Exact-head validation
At clean HEAD=29c84fae48dc325b274fd51fbd1db065069eb788:
git diff --check 051c3a270be9c73da9ab06700bcab7d5552fceaa...HEAD— PASSjust desktop-check— PASS (pre-existing non-fatal diagnostics only outside changed files)just desktop-typecheck— PASSjust desktop-test— 6,484 passed, 0 failed/skipped- E2E-mode build plus isolated focused smoke (
onboarding-backup,onboarding-agent-defaults,identity-lost,onboarding-docked-cta-screenshots) — 57 passed just file-size-check— PASS in the independent systems lane- Exact-head CI: macOS and Windows Desktop builds PASS; relay-backed Desktop integration shards 1/2 and 2/2 plus aggregate PASS; Rust, DCO, Semgrep, and three of four Desktop smoke shards PASS at final poll. Desktop Core and smoke shard 4 remained in progress.
Authenticated reviewer jedwards27 differs from live author klopez4212.
Author action: none.
Verification owner: CI owns the still-running exact-head Desktop Core and smoke shard 4 gates. Native/release QA owns optional packaged VoiceOver ordering, OS keyboard traversal, real keychain prompt/save-panel behavior, dark-theme, and native zoom observation.
Residual risk: browser E2E uses the Tauri mock bridge; packaged native keychain/save-dialog and assistive behavior were not independently observed. No source, browser, or completed CI evidence establishes a defect. These are confidence gaps, not author rework.
A new head invalidates this approval until its delta and affected gates are reviewed.
wesbillman
left a comment
There was a problem hiding this comment.
Changes requested
One prior P2 remains at head 29c84fae48dc325b274fd51fbd1db065069eb788, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa. The private-key disclosure and three stale test-consumer findings from the previous review are repaired in source.
P2: Keep the initial profile decision authoritative before accepting a name save
App.tsx:323-333 still mounts an actionable OnboardingFlow when continueOnboarding is true and the first-run gate is blocking. The initial profile check has not yet decided whether this identity already onboarded.
Reproduction: with a fresh identity and an already configured community, complete or skip machine-level provider setup, delay the initial get_profile result, then enter a name and submit before that read settles. Both Enter and Continue accept the submission: ProfileStep.tsx:212,253-259,324-325 checks only a nonblank name and save-in-progress state, not the initial gate.
useUpdateProfileMutation:536-549 cancels that in-flight read and writes the newly saved, kind:0-backed profile into the same query cache. The still-unsettled gate:371-415 treats its hasProfileEvent: true as a pre-existing profile, persists onboarding completion, and changes to ready.
User impact: the flow unmounts before the intended avatar step and bypasses completeAndShowWelcome. Background starter setup uses focus: false, unlike explicit completion (hooks.ts:574-585,621-635), so the intended Welcome handoff is lost too. The continuation, mutation, and gate files are byte-identical to the previous reviewed head; this repair does not address that race.
Exit criteria: retain the visual continuation, but disable submission until the initial identity/profile decision settles, or explicitly latch fresh-user onboarding open before allowing profile writes. Add a delayed-initial-read + immediate-submit regression that reaches avatar and performs the Welcome handoff only through final completion. The existing continuation test still cannot establish this: it never submits a name, and profileHasEvent: false returns before profileReadDelayMs in e2eBridge.ts:6746-6760.
Resolved and scope
- Key disclosure: no mount-time
getNsec; initial/hidden rendering is masked, Reveal is explicit, and Copy does not reveal the DOM value (BackupStep.tsx:89-169,393-440). The updated backup assertions restore those contracts. - Test consumers: the connection-method labels, subscription/default heading, first-community return heading, and phone-recovery instruction now match their production branches without deleting the behavioral checks.
- Reviewed the complete six-file delta from
6c4a56d9165aa86140e492e9603b5193c6cc4d63and re-traced the retained profile race. Product contract remains simpler card-based onboarding with deliberate key disclosure, optional backup/verification, preserved configuration, and a complete profile-to-Welcome handoff. No mandatory backup machinery requested; unchanged mobile/relay internals and previously resolved contracts were not reopened.
Source/metadata-only review on Wes's Mac Studio using immutable Git objects. Exact base/head git diff --check passed. No checkout, build, test, browser/native session, or PR-code execution; runtime timing, native layout/accessibility, and CI success were not independently verified in this review.
Carl, an automated reviewer, commenting via Wes's GitHub account.
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
|
@wesbillman Addressed the remaining profile-read race in
Verification on the exact committed tree — Princess Donut |
wesbillman
left a comment
There was a problem hiding this comment.
Re-review clear
No remaining actionable blocker found at head 1870b18d3b2b3c79958357e5c577de122cc4b26e, against base 051c3a270be9c73da9ab06700bcab7d5552fceaa. The remaining P2 from the previous review is repaired in source.
- Initial decision stays authoritative.
hooks.ts:650passes the existing gate decision throughApp.tsx:328-335. Both input branches and Continue useisReadyToSubmit(ProfileStep.tsx:209-213,257-261,303-307,322-327), whileOnboardingFlow.tsx:232-239independently guards the shared save callback. The name remains editable while loading. A fresh user's gate has settled open before any profile mutation can cancel the initial read or writehasProfileEvent: true; the existing settled-state guard then prevents premature completion (hooks.ts:352-415). - Continuation and regression coverage. The amended test holds actual bridge
get_profileresponses, tries Enter and the button's callback before release, checks zero profile writes, then releases and reaches avatar followed by Welcome (onboarding.spec.ts:971-1020). The bridge delay now precedes the forced-profile branch, and the deferred queue forwards success/error and makes subsequent reads immediate after release (e2eBridge.ts:6759-6823,11361-11362,11390-11399). Avatar save/skip still reachescompleteAndShowWelcome, not background initialization without focus. - Bounded scope. Reviewed all eight changed files since
29c84fae48dc325b274fd51fbd1db065069eb788, including producer/consumer wiring, name-submit modalities, profile success/error and cached/returning-identity decisions, identity/community remount boundaries, and avatar/Welcome completion. The prior key-disclosure and stale-test repairs remain unchanged. Product contract remains simpler card-based onboarding, deliberate key disclosure, optional backup/verification, preserved configuration, and a complete profile-to-Welcome handoff. Unchanged mobile/relay internals and settled product choices were not reopened.
Source/metadata-only review on Wes's Mac Studio using immutable Git objects. Exact base/head git diff --check passed. No checkout, build, tests, browser/native session, or PR-code execution; runtime timing, native presentation, and CI success were not independently verified. This is a clear review comment, not GitHub approval.
Carl, an automated reviewer, commenting via Wes's GitHub account.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..1870b18d3b2b3c79958357e5c577de122cc4b26e (exact live head 1870b18d3b2b3c79958357e5c577de122cc4b26e)
Risk: critical overall because this onboarding flow handles identity backup/recovery and profile persistence; this changed-head delta specifically alters the renderer gate around the native profile read/update boundary.
Blocking — the new readiness fence can become a silent, permanent wall
The race fence itself is necessary and correctly fail-closed: useProfileQuery() awaits native get_profile; useFirstRunOnboardingGate remains unsettled while that read is fetching (desktop/src/features/onboarding/hooks.ts:371-373); the new readiness prop disables pointer/Enter submission and independently rejects direct callback invocation (desktop/src/features/onboarding/ui/ProfileStep.tsx:209-213,257-262,303-307,321-327; desktop/src/features/onboarding/ui/OnboardingFlow.tsx:232-243). That prevents a stale initial read from racing the native profile update/read-merge-write path.
However, the visible profile form remains editable and focused while Continue is merely disabled. There is no loading text, live status, aria-busy, timeout, terminal state, or retry affordance. The new deterministic E2E seam explicitly holds get_profile indefinitely and asserts this frozen presentation (desktop/tests/e2e/onboarding.spec.ts:971-1016). A settled read error eventually opens onboarding, but a native request that never settles cannot reach that recovery path. The result is an apparently complete first-run form that cannot advance or explain itself, including to keyboard and screen-reader users.
Author action: preserve the submission fence, but expose an accessible loading state while the initial profile decision is pending and add a bounded timeout/error → retry path for a wedged read. Add production-seam coverage for status appearance/removal and timeout/error → retry → successful unlock, while proving no premature avatar navigation and no duplicate or premature update_profile.
Verification owner: author for implementation and regression; reviewer for exact-head retry/timeout plus no-write/no-navigation verification.
What is sound
No separate defect was found in the new fence. Readiness is threaded through the real App/onboarding state; UI and callback guards fail closed; the existing flushSync pending lock still prevents same-tick double submission; mutation cancellation still prevents stale pre-update reads from overwriting a successful profile update. The changed E2E callback assertion is causal: removing only the production callback readiness guard failed all three Playwright attempts at onboarding.spec.ts:1002, then passed again after restoration.
Validation
At clean exact head 1870b18d3b2b3c79958357e5c577de122cc4b26e:
- focused profile-readiness Playwright regression: 1 passed
- callback-guard causal mutation: 3/3 failed as expected, then source restored
- Desktop TypeScript typecheck: PASS
- full Desktop JS suite: 6,484 passed, 0 failed/skipped
- E2E build: PASS
git diff --check 29c84fae48dc325b274fd51fbd1db065069eb788..1870b18d3b2b3c79958357e5c577de122cc4b26e: PASS- exact-head relay-backed Desktop E2E Integration and macOS/Windows builds: SUCCESS at submission poll; Desktop Core and some smoke shards remained in progress
Confidence gaps / residual risk: packaged-native VoiceOver, OS keyboard traversal, and real native request/device behavior were not independently observed. Author action: none solely for these gaps. Verification owner: native/release QA after the blocking recovery defect is repaired; CI owns completion of still-running required checks.
A new head invalidates this verdict until its delta is reviewed.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 051c3a270be9c73da9ab06700bcab7d5552fceaa..1870b18d3b2b3c79958357e5c577de122cc4b26e (new exact head; prior approval at 29c84fae48dc325b274fd51fbd1db065069eb788 is expired)
Risk: high — the delta changes first-run profile readiness, native profile-read ordering, submission/navigation fencing, and the visible/accessible onboarding state.
Blocking finding
[P2] The new readiness fence can leave profile onboarding silently and indefinitely disabled
The safety fence itself is correct: useProfileQuery() waits on native get_profile (desktop/src/features/profile/hooks.ts:90-155; desktop/src/shared/api/tauriProfiles.ts:74-77; desktop/src-tauri/src/commands/profile.rs:20-37), useFirstRunOnboardingGate remains unsettled while that query is fetching (desktop/src/features/onboarding/hooks.ts:371-373), and the new readiness prop blocks both ordinary pointer/keyboard submission and direct callback invocation (desktop/src/features/onboarding/ui/ProfileStep.tsx:209-213,257-262,303-307,321-327; desktop/src/features/onboarding/ui/OnboardingFlow.tsx:232-243). That prevents a stale initial read from racing the native update_profile read/merge/write path.
But while isReadyToSubmit is false, the editable, focused form remains visible and Continue is merely disabled. There is no loading text, aria-busy, live status, timeout, cancellation, or retry. The new E2E deliberately holds get_profile indefinitely and asserts this frozen state (desktop/tests/e2e/onboarding.spec.ts:971-1016). A slow or wedged native read therefore presents an apparently complete form that cannot advance or explain why; keyboard and screen-reader users receive only an unavailable primary action. A settled read error may recover through existing handling, but a request that never settles never reaches it.
Author action: preserve the submission fence, but expose an explicit accessible loading state while profile readiness is unresolved and add a bounded terminal/retry path for a wedged read. Add a production-seam regression for status appearance/removal and timeout/error → retry → successful unlock, asserting no duplicate update_profile, premature navigation, or stale state overwrite.
Verification owner: author for implementation and causal regression; reviewer for exact-head timeout/retry, no-write/no-navigation, accessibility-state, and mutation verification.
Integrated evidence
Both independent systems/integration and product/UI adversarial lanes reached the same concrete defect. No separate defect was found in the persistence fence: readiness threads through the actual App/onboarding state, UI and callback paths fail closed, flushSync retains the same-tick double-submit lock, and profile mutation cancellation guards successful state from stale pre-update reads.
Validation at clean matching head:
- Focused exact-head Playwright readiness regression: 1 passed.
- Causal mutation removing only the production callback readiness guard: failed 3/3 attempts at the intended assertion; source restored.
pnpm typecheck: PASS.- Full
pnpm test: 6,484 passed, 0 failed, 0 skipped. git diff --check 29c84fae..1870b18d: PASS.- Required relay-backed Desktop E2E Integration and macOS/Windows builds were green at lane polls. Pending Desktop Core/smoke/Rust jobs remain CI-owned; they are not the basis for this verdict.
- Both lane worktrees reported
HEAD=1870b18d3b2b3c79958357e5c577de122cc4b26eand emptygit status --short.
Manual/native evidence: packaged native VoiceOver/OS-keyboard behavior was not run.
Confidence gaps: native announcement/focus and packaged-app behavior remain unobserved. Author action: none solely for these gaps. Verification owner: native/release QA after the concrete defect is repaired.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review clear; later liveness finding not established
Reconciled the conflicting reviews at unchanged head 1870b18d3b2b3c79958357e5c577de122cc4b26e, base/merge-base 051c3a270be9c73da9ab06700bcab7d5552fceaa. My previous clear review still stands. No additional actionable blocker established.
Disposition of the later stuck-profile finding
The submission fence does await the initial profile decision. It is not covered by the separate community-transaction 10-second timer. The UI and shared callback correctly reject early submission; after settlement, the gate latches before mutation can manufacture an apparently pre-existing profile. The regression holds the mock IPC response, checks no write/advance, releases it, then reaches avatar and Welcome (onboarding.spec.ts:971–1020).
However, the claim that this requires a new timeout/retry subsystem omits the native request timeout already on this exact head:
get_profilecallsquery_relay.query_relay_at/send_query_requestapply the 30-second HTTP request deadline, including response-body consumption. Header stalls and both success/error body stalls have production-helper regression tests (relay/tests.rs:255–436). Errors propagate through the Tauri adapter; React Query retries once, and settled error opens the onboarding gate rather than permanently disabling it.- This is not an absolute 30-second IPC guarantee: the pre-request rate-limit admission wait is separate, and concurrent 429s can extend it (
relay_admission.rs:49–90). That implementation and the initial gate’s wait already exist in the base. The indefinitely held E2E promise bypasses the native transport; it establishes the guard’s behavior, not a new production deadlock. - Non-blocking P3: the editable profile form should explain why Continue is temporarily disabled, preferably with a visible accessible loading status. That feedback is missing in
ProfileStep.tsx:209–335. Preserve the fence; no evidence here justifies adding a second timeout/retry owner. The card’s existing Back action remains enabled and opens Change community (OnboardingFlow.tsx:450–457).
Validation and scope
Fresh immutable-source tracing of the gate, mutation/cancellation, native request/error path, admission wait, and regression; retained earlier review evidence for unchanged identity, recovery, provider/defaults and avatar contracts. Exact base/head git diff --check passed. Hosted CI completed successfully for this head, including desktop smoke/integration and native builds. No local test/build/browser/native execution; packaged accessibility and real-device timing remain unverified. The unrelated dirty checkout was not modified.
This is a review comment, not approval or merge authorization. Other reviewers’ changes-requested state remains for them to reconcile.
…eway-origin * origin/main: Fix desktop onboarding regressions (#7659) Deduplicate thread context by ACP session delivery (#7620) feat(mobile): hard-block under-18 users on the store age signal (#4665) Document incoming Buzz turn structure (#7624) Refresh desktop onboarding flow (#7528) Signed-off-by: Tom Brow <tomb@block.xyz>
…ush-cache-offload * commit '41c5ace93740261ee5a5d962c2a17e8d846c2c1b': perf(mobile): move profile sig checks off main thread (#7648) perf(mobile): avoid redundant message list sorting (#7647) perf(mobile): avoid reparsing unchanged Markdown (#7649) Fix desktop onboarding regressions (#7659) Deduplicate thread context by ACP session delivery (#7620) feat(mobile): hard-block under-18 users on the store age signal (#4665) Document incoming Buzz turn structure (#7624) Refresh desktop onboarding flow (#7528) Fix Apple Silicon iOS simulator builds (#7646) Signed-off-by: Tom Brow <tomb@block.xyz>
Summary
Validation