Add onboarding design system showcase - #7511
Conversation
**Category:** fix **User Impact:** Link previews can keep loading while a message is being composed, while sending still has a finite escape hatch and stalled network transports cannot occupy preview slots forever. **Problem:** Native metadata and image deadlines could collapse slow previews into fallback cards while the user was still composing, and a shared image-host cooldown made pasted batches fail inconsistently after one rate limit. **Solution:** Keep preview resolution user-paced with no aggregate request deadline, bound transport inactivity (15s DNS/connect, 30s idle read), serialize image requests by host, and allow at most one server-directed cooldown wait of up to 30s across an image fetch and its redirects. The existing bounded post-Send preparation and immediate Skip paths remain unchanged. <details> <summary>File changes</summary> **desktop/src-tauri/src/commands/link_preview.rs** Removes aggregate native deadlines so composer metadata work can complete at the user's pace, while retaining DNS/connect/idle-read liveness bounds. Adds bounded host-paced image request coordination that releases its gate during cooldown, waits inline at most once for at most 30 seconds, and cannot renew that wait through redirects or the outer transient retry. Same-host image and favicon requests remain deliberately serialized to align with host rate limits. **desktop/src-tauri/src/commands/link_preview_rate_limit.rs** Adds a fixed-size striped host gate so concurrent image requests are serialized without retaining an unbounded attacker-controlled hostname map. **desktop/src-tauri/src/commands/link_preview_tests.rs** Moves native link-preview tests into a dedicated module and covers the user-paced metadata contract, bounded one-shot cooldown behavior, and gate release while a rate-limited request sleeps—including a different host sharing the same bounded gate stripe. **desktop/src-tauri/src/commands/link_preview_youtube.rs** Removes the thumbnail fetch deadline so YouTube previews follow the same composer lifecycle contract while using the shared bounded transport. **desktop/src/shared/lib/useResolvedLinkPreviews.ts** Adds development-only metadata outcome diagnostics with elapsed time and image/fallback state, without logging encoded image payloads. </details> ### Reproduction steps 1. Open the desktop composer and paste several GitHub pull request links whose OpenGraph images share a host. 2. Observe that image requests are paced by host instead of racing, and slow-but-progressing preview work remains pending rather than immediately becoming a completed favicon fallback. 3. Send while preview work is still pending and confirm **Preparing link preview** remains bounded by the existing post-Send budget. 4. Use **Skip** during preparation and confirm the message proceeds immediately. 5. In a development build, inspect the console for `[link-preview] metadata fetch completed` diagnostics containing elapsed time and image state without base64 payloads. ### Related issue N/A — scoped from the linked Buzz implementation room. ### Testing At current head `dfb394aafbee537e9ffb04ad3732d08f65f30b8e`: - Production-bound paused-time metadata regression passed through `fetch_link_preview_metadata`; restoring the former 10-second aggregate wrapper makes it fail at the pending assertion. - Native link-preview module: 19/19 passed. - `cargo check --manifest-path desktop/src-tauri/Cargo.toml` passed. - Rust formatting and `git diff --check` passed. - Pre-push `push-head-scope`, org safety, differential file-size, branch-skew, and `desktop-tauri-checks` hooks passed. At prior head `59e2dcf167b15c7a3e637ad2608008b7f9cef5f3`: - Full Tauri Rust suite: 3,056 passed, 19 ignored; integration crates 7 + 3 passed. - Focused native link-preview suite: 26/26 passed. - The pasted multi-preview workflow was exercised in the desktop app and confirmed improved before draft publication. --------- Signed-off-by: Taylor Ho <taylorkmho@gmail.com> Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz> Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@users.noreply.github.com>
🤖 ## Summary When a Buzz agent falls behind on incoming messages, its connection can make the backlog worse while trying to recover. The connection buffers messages from the relay server until the agent is ready to process them; if that buffer overflows, recovery previously requested history for **every subscribed channel** and paused socket reads while sending those requests. That adds traffic to an already overloaded connection. This change requests history only for affected subscriptions, once the code consuming those messages has room, with at least five seconds between attempts. The recovery path now: - Combines repeated losses into one pending recovery per affected subscription, keeping the oldest dropped timestamp so replay starts early enough. - Waits until at least half the consumer queue is free and the relay's existing rate-limit delay has expired. The queue wakes recovery when space becomes available; recovery does not periodically sample capacity or hold queue space away from live messages. - Attempts one subscription at a time, choosing the least recently attempted so a busy channel cannot crowd out other channels or membership notifications. The five-second delay starts when an attempt finishes, including a failed write; failed writes leave recovery pending. Recovery is paced by available capacity, not by how often messages are lost. This is not a larger buffer or a cutoff that abandons recovery. Subscription identifiers, message filters, replay timestamp overlap and duplicate filtering are unchanged; no downstream agent changes are required. This targets a reproducible overload **amplifier**, not every cause of overload or every catch-up limitation. The initial live overload's cause has not been established. Recovery remains best effort: a successful request write is not proof of delivery, and existing history/retention limits, bounded duplicate tracking and replay limitations still apply. There is no exactly-once or complete catch-up guarantee. A stalled write can still pause socket reads for the existing ten-second timeout; the pacing bound does not cover initial subscriptions, reconnects or other retry paths. ### Related issue Closest related: #5014 (channel re-subscription); also #6661 (membership reconciliation) and #6090 (relay backpressure gap signaling). This addresses local overflow recovery scheduling, not those separate mechanisms. ### Testing Recorded offline comparisons against the previous behavior, with the final implementation at `8000636f3073167c5a5107bb179c7d91160f1729`: | Same fixture: 18 subscriptions, three overload rounds | Before | After | | --- | --- | --- | | Recovery history requests | 108 | 3 | | Ping-response delay | About 4.6 seconds | Below the measurement's 1 ms resolution | A separate bounded-history fixture delivered all 320 events plus subsequent live traffic in **both** versions. Regression coverage exercises the real socket-handling task, including intermittent consumer capacity, fairness, failed writes and cancellation of capacity waits before live delivery. These are synthetic results, not production throughput measurements or evidence of a deployed cure. The full local `RUST_TEST_THREADS=4 just ci` run passed on September 4, 2026. Earlier unsuccessful local runs remain part of the validation history. The [recorded validation evidence and separate desktop follow-up](#7325 (comment)) preserve the original desktop mock-history scroll failure, its passing rerun and the remaining investigation. That desktop path does not run the agent connection code; neither this repair nor the passing rerun fixes the observed scroll problem. --------- Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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. |
Signed-off-by: kenny lopez <klopez4212@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02a0ea8fd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const TRANSPORT_CONNECT_TIMEOUT: Duration = Duration::from_secs(15); | ||
| const TRANSPORT_IDLE_TIMEOUT: Duration = Duration::from_secs(30); | ||
| const DNS_RESOLUTION_TIMEOUT: Duration = Duration::from_secs(15); | ||
| const MAX_INLINE_IMAGE_COOLDOWN: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
When an HTTPS host continuously trickles a small chunk just before each 30-second idle timeout, that per-read timeout resets while read_bytes_prefix/read_limited_bytes continues toward its 256 KiB/2 MiB cap. A single fetch can therefore occupy native work and one of the renderer's two metadata scheduler slots for months; two such URLs block all subsequent previews while they remain visible. Retain the new cancellation path, but add a practical total-duration or minimum-throughput bound around the complete fetch.
AGENTS.md reference: AGENTS.md:L194-L202
Useful? React with 👍 / 👎.
| signal: AbortSignal, | ||
| ): Promise<LinkPreviewMetadata | null> { | ||
| return invokeTauri<LinkPreviewMetadata | null>( | ||
| const requestId = crypto.randomUUID(); |
There was a problem hiding this comment.
On the macOS 10.15 release target declared in desktop/scripts/build-release-config.mjs:43, the system WKWebView predates crypto.randomUUID(). Every external preview fetch therefore throws synchronously here before invoking Tauri; after two attempts, the scheduler's active count is also left at its concurrency limit because task() throws before its .finally() is installed, so later previews remain queued. Use a request-ID implementation available on the supported WebView baseline or provide a fallback.
Useful? React with 👍 / 👎.
| .onboarding-profile-choices { | ||
| display: flex; | ||
| gap: var(--space-2); |
There was a problem hiding this comment.
On a narrow viewport such as 380px, this non-wrapping row needs at least five fixed 5rem buttons plus four gaps, while the card's responsive padding leaves substantially less horizontal space. Because .onboarding-scene also hides overflow, the final profile choices spill outside the panel and are clipped rather than remaining usable. Add a narrow-layout wrap, resize, or intentional horizontal-scrolling treatment.
Useful? React with 👍 / 👎.
| aria-label={choice.label} | ||
| onClick={() => setSelected(choice.label)} | ||
| data-selected={selected === choice.label || undefined} |
There was a problem hiding this comment.
When a screen-reader user selects a profile, only the private data-selected attribute changes, so every choice continues to be announced as an unrelated, unpressed button and the current mutually exclusive selection cannot be determined. Model these controls as a named radio group or expose an equivalent checked/pressed state for each button.
AGENTS.md reference: AGENTS.md:L218-L226
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dcc595f80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if *waited_for_cooldown { | ||
| return None; | ||
| } | ||
| set_image_host_cooldown(url, retry_after); |
There was a problem hiding this comment.
When a host returns another 429/5xx with Retry-After after this request has already waited once, waited_for_cooldown is true and this returns before updating the shared host cooldown. Queued previews therefore pass the gate at the old expiry and immediately retry the host despite its renewed backoff request. Record the new cooldown before declining a second inline sleep.
AGENTS.md reference: AGENTS.md:L194-L202
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b4d184242
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <div className="code-input-group"> | ||
| {digits.map((digit, index) => ( | ||
| <div className="code-input-slot" key={CODE_INPUT_DIGIT_KEYS[index]}> | ||
| <input |
There was a problem hiding this comment.
This new shared interactive component renders native <input> elements and recreates their focus, change, paste, and keyboard handling even though the client already uses Base UI's corresponding Input primitive. That bypasses the centralized accessibility and behavior layer, so fixes or conventions applied to shared inputs will not reach verification-code fields; wrap the slots with Base UI Input while retaining the segmented behavior. The scoped desktop-new/AGENTS.md explicitly permits native elements only when Base UI has no corresponding primitive or the component is semantically static.
AGENTS.md reference: desktop-new/AGENTS.md:L112-L116
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 3ef9466878ab45dbecfb1886cf1c52f6988f9b2b..0b4d18424268b24ddb86e7b5a1717ab52c3700e1 (exact head 0b4d18424268b24ddb86e7b5a1717ab52c3700e1)
Risk: medium — new user-visible onboarding compositions and shared input behavior, with accessibility and responsive-layout contracts.
Behavior/contracts traced: intended 12-file desktop-new delta; Base UI TextField composition; CodeInput filtering, paste, focus/backspace, callback, disabled/autofill, and animation lifecycle; profile selection semantics; narrow geometry; light/dark error and focus states. GitHub's ACP/link-preview entries are merged-main history, not authored onboarding scope. No intended persistence, network, secrets, native lifecycle, community-isolation, or release behavior was added.
Blocking findings
-
Profile selection is styling-only —
desktop-new/src/features/onboarding/ui/OnboardingShowcase.tsx:228-250. The five mutually exclusive choices remain ordinary buttons whose selected state exists only indata-selected. Browser/AX validation found no radio/checked or pressed state, and ArrowRight neither moved nor changed selection. Keyboard and screen-reader users cannot perceive or operate the single-select contract.
Author action: use a radio group or equivalent mutually-exclusive semantics and keyboard behavior; add a focused browser regression for role, checked state, focus, and arrow selection.
Verification owner: author test; reviewer AX/keyboard rerun. -
Verification-code and profile controls are clipped at narrow widths — fixed-width children in
desktop-new/src/shared/styles/components.css:667-677anddesktop-new/src/shared/styles/product.css:192-211sit inside the hidden-overflow scene atproduct.css:28-35;product.css:242-255does not adapt either row. At 320/375/390px, scene client widths were288/343/358, while CodeInput/profile scroll widths remained378/482. Later controls are visibly inaccessible.
Author action: make both groups responsive within the card via flexible sizing, wrapping, or intentional accessible scrolling; add 320/375/390px containment and visibility regressions for every digit/profile choice and Continue.
Verification owner: author tests; reviewer geometry/screenshots. -
TextField error text uses a known failing color while the guard falsely passes —
desktop-new/src/shared/styles/components.css:635-637uses--red-11.desktop-new/DESIGN.md:210-218,289-292records red-11 at APCA Lc 59.7 on dark panel against the Lc 60 target and requires red-12.desktop-new/scripts/check-contrast.mjs:62-83excludes red-11 because it claims there are no consumers, so the green contrast gate misses this real reader.
Author action: use red-12 and bind the actual rendered error consumer to contrast coverage; mutating it back to red-11 must fail.
Verification owner: author guard/test; reviewer rerunspnpm check. -
The custom CodeInput interaction machine has no behavior tests —
desktop-new/src/shared/ui/CodeInput.tsx:19-192owns filtering, paste distribution, replacement, focus movement, backspace, callback emission, duplicate render state, animation-end cleanup, disabled behavior, and OTP autofill. Searches acrossdesktop-new/src/**/*test*anddesktop-new/tests/**/*spec*found no CodeInput behavior assertion; the package still reports 44 tests. This violates the production-seam/falsifiability requirements indesktop-new/AGENTS.md:286-294andTESTING.md:19-32, particularly because state cleanup depends on an asynchronous DOM event.
Author action: add focused production-component tests covering sequential entry, mixed paste/filtering, replacement, backspace/focus, callback values, disabled behavior, and animation-end cleanup/re-entry; mutation-prove the handlers.
Verification owner: author evidence; reviewer full-package rerun and assertion review.
Validation
At clean exact head 0b4d18424268b24ddb86e7b5a1717ab52c3700e1:
pnpm check— PASS, while finding 3 demonstrates the contrast reader blind spot.- Full
pnpm test— PASS, 8 files / 44 tests. - Generated-route Vite build, standalone typecheck, and production build — PASS; existing >500 kB chunk warning only.
- Focused Playwright interaction probe — CodeInput basic digit entry, focus progression, direct deletion, empty-slot Backspace, Left arrow, six AX labels, and reduced-motion duration passed; chooser defect reproduced.
- Focused Playwright narrow probe — FAIL at 320/375/390px with the overflow measurements above.
git diff --check, public-export documentation, and added productionunwrap/expectaudit — PASS.- Live head remained exact and the PR was mergeable immediately before submission. Required CI still had Rust/unit/Desktop jobs in progress; this is a CI-owned confidence gap, not additional author action.
Manual/native evidence: browser validation covered the visible contracts above. Native Desktop/OS OTP autofill was not run; desktop-new has no Tauri shell yet.
Residual risk / confidence gaps: native OTP autofill, native assistive technology, disabled interaction, and rapid animation interleavings remain unproven. Soft TextField's intentional unconditional pointer focus and literal 0.1s ease remain a design-owner policy question, not an additional blocker. These gaps do not replace the four concrete author actions above.
wesbillman
left a comment
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 0b4d18424268b24ddb86e7b5a1717ab52c3700e1 against base 3ef9466878ab45dbecfb1886cf1c52f6988f9b2b. The onboarding contract is a design showcase, not live signup; missing backend actions are not findings.
Two additional P2 defects are annotated below: the six-slot CodeInput cannot fit the narrow onboarding panel, and the soft TextField replaces keyboard focus with a sub-3:1 indicator.
Existing findings independently confirmed at this head
These remain unresolved; linking their existing threads instead of creating duplicate inline comments:
| Priority | Finding and current-head evidence |
|---|---|
| P1 | Preview admission starvation: connect/read-idle limits do not bound total duration. Two mounted previews receiving a trickle inside each 30s idle window can retain both renderer metadata slots for operationally excessive periods. Keep cancellation, but restore a practical aggregate duration or throughput bound. |
| P1 | UUID compatibility and leaked slots: the non-async metadata task calls crypto.randomUUID() before returning a promise. On a WebView lacking this API, task() throws before the scheduler installs its finally; two failures exhaust admission. The release config still targets macOS 10.15. Use baseline-compatible request IDs and ensure synchronous task failures release the slot. |
| P2 | Renewed host cooldown is not persisted: after one inline wait, retryable_image_cooldown returns before updating shared state. A second 429 with a longer Retry-After lets queued same-host requests pass at the old expiry. Record every renewed cooldown independently of whether this caller may sleep again. |
| P2 | Narrow profile choices clip: five fixed 5rem buttons plus four gaps still do not wrap inside the narrow panel. |
| P2 | Profile selection lacks assistive state: selecting a choice changes only data-selected, not checked/pressed state. |
Exit criteria: resolve these five existing defects and the two new annotations, with regressions for admission after synchronous failure, trickling responses, renewed host backoff, narrow layouts, and keyboard/assistive states. No new signup behavior is requested. The separate Base UI/registry alignment concerns are architectural follow-up, not additional P1 runtime defects established by this review.
Coverage and limits: source-only review on the designated remote workstation, using exact-commit snapshots and verified matching blobs for reused evidence. Traced showcase registration/consumers, TextField/CodeInput state and input paths, legacy renderer/native admission, cancellation and cooldown ownership, and ACP overflow-recovery pacing/capacity/reconnect/unsubscribe paths. No new ACP blocker found; successful-REQ cursor clearing is not proof of end-to-end delivery. No checkout, build, tests, browser execution, or native runtime validation was performed. Layout and contrast findings are source-derived, not browser reproductions.
| .code-input-slot { | ||
| position: relative; | ||
| width: 3rem; | ||
| height: 3rem; | ||
| flex: 0 0 auto; |
There was a problem hiding this comment.
[P2] Make all six code slots fit the narrow showcase panel
These fixed, non-shrinking slots require 6 × 3rem + 5 × 0.5rem = 328px. At a 375px viewport (default 16px rem), /design/components/onboarding has 343px after the design-content padding, a 295px card, and about 245px of panel content after mobile padding/borders (product.css:64–79,242–254). The sixth slot starts 280px into that content, beyond the panel's clipping edge; .panel has overflow: hidden (components.css:212–218). All six controls therefore cannot remain visible together, and the last is initially unavailable to pointer interaction.
Source-derived reproduction: open that route at 375px and inspect the “Check your email” specimen, including entering/pasting all six digits rather than just its default 28. Make slot width/gaps responsive or provide sufficient non-clipped space, and cover a narrow-width specimen/regression. This is separate from the existing profile-choice clipping thread. No browser execution was performed.
| .text-field[data-variant="soft"] .text-field-control:focus { | ||
| outline: none; | ||
| box-shadow: 0 0 0 2px var(--neutral-7); |
There was a problem hiding this comment.
[P2] Give the soft field a sufficiently contrasting keyboard indicator
This more-specific :focus rule also wins for keyboard focus, clearing the shared outline and leaving only the neutral-7 shadow. From the exact-head tokens, that shadow is about 1.44:1 against light neutral-2 (#cecece / #f5f5f6) and 1.80:1 in dark (#424242 / #161616); against the panel it is about 1.57:1 / 1.70:1. Those are below the 3:1 control-state requirement documented in DESIGN.md:344–349, making keyboard position difficult to distinguish.
Source-derived reproduction: Tab to a soft field on /design/components/text-field; its outline resolves to none and only this neutral ring remains. Keep the proposed soft appearance if desired, but provide a measured >=3:1 focus indicator in both modes. Merely restoring purple-8 unchanged is not a verified fix for light mode. Token contrast was calculated independently; no browser execution was performed.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Review of 0b4d18424268b24ddb86e7b5a1717ab52c3700e1 against base 3ef9466878ab45dbecfb1886cf1c52f6988f9b2b.
Required changes
-
[P2] Expose the profile chooser as a real single-select control. In
desktop-new/src/features/onboarding/ui/OnboardingShowcase.tsx:228-250, the five choices are ordinary buttons and selection is conveyed only bydata-selected. Runtime validation confirmed that selecting Sun leaves the accessibility tree with five undifferentiated buttons and ArrowRight neither moves nor changes selection. A keyboard or screen-reader user cannot determine or operate the selected value. Use native/Base UI radio-group semantics (and separate the custom-image action if it launches a picker), then add a browser regression covering role, checked state, focus, and arrow selection. -
[P2] Keep both onboarding control rows inside narrow cards. Fixed-width children in
desktop-new/src/shared/styles/components.css:667-677anddesktop-new/src/shared/styles/product.css:192-211sit inside the hidden-overflow scene atproduct.css:28-35; the narrow rule atproduct.css:242-255adapts neither row. At 320/375/390px, scene client widths were 288/343/358px while the verification-code and profile rows retained scroll widths of 378/482px, visibly clipping later controls and the Continue action. Make both groups responsive (flexible grid/slots, wrapping, or another accessible containment strategy) and add 320, 375, and 390px assertions that every digit/profile choice and Continue remain visible and operable. -
[P2] Use the documented error-text color and bind the contrast guard to the real consumer.
.text-field-erroruses--red-11atdesktop-new/src/shared/styles/components.css:635-637, althoughdesktop-new/DESIGN.md:210-218,289-292records that step at APCA Lc 59.7 on a dark panel against the Lc 60 body target and requires red-12.desktop-new/scripts/check-contrast.mjs:62-83still says red-11 has no consumers, sopnpm check:contrastpasses while missing the new reader—the exact false-green failure local policy warns against. Switch the error text to red-12 and add consumer-bound guard coverage whose mutation back to red-11 fails. -
Add falsifiable behavior coverage for the custom
CodeInputinteraction machine.desktop-new/src/shared/ui/CodeInput.tsx:19-192owns filtering, paste distribution, replacement, focus movement, Backspace, callback emission, disabled behavior, OTP autofill, duplicate render state, and animation-end cleanup, but no tracked test/spec exercises it. Basic keyboard paths passed our browser probe, but that does not bind the unexercised handlers or asynchronous cleanup. This conflicts withdesktop-new/AGENTS.md:286-294andTESTING.md:19-32. Add production-seam tests for sequential entry, mixed paste/filtering and replacement, Backspace/focus, emitted values, disabled behavior, and animation-end cleanup/re-entry; demonstrate the assertions fail when their production handlers/cleanup are removed.
Evidence and scope
- Intended diff: 12
desktop-newfiles, +1140/-1. The ACP/link-preview files shown in GitHub are merged-main history rather than authored onboarding scope. TextField's Base UI label/description/error/disabled/name wiring is structurally sound. No persistence, network, secret, native lifecycle, tenancy, or release behavior is introduced by the intended delta.- Exact-head local gates passed:
pnpm check, all 44 Vitest tests, generated-route Vite build, standalone typecheck, and production build. The existing >500 kB chunk warning remains.git diff --checkpassed; added exports have doc comments; no productionunwrap/expectwas added. - Focused browser checks passed basic CodeInput digit entry, focus progression, deletion/Backspace, Left Arrow, labels, and reduced-motion duration. The narrow and chooser checks failed as quantified above.
Confidence gaps (not additional blockers)
- CI was still running at submission; CI owns completion. No current required check failure is attributed to the author.
- Native OTP autofill and real OS accessibility were not run.
desktop-newhas no Tauri shell yet. - CodeInput's historical focus/motion concern is resolved by tokenized transitions, keyboard-navigation gating, and reduced-motion handling. Soft
TextFieldstill intentionally uses literal0.1s easeand unconditional:focus; because the source marks this as a proposed comparison treatment, the desktop-new design owner should adjudicate it rather than treating it as an additional blocker.
Verdict: REQUEST CHANGES. The first three findings are reproduced user-facing defects; the fourth is a repository-required test seam for a substantial custom interaction contract.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 004f841cb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * A segmented numeric code field with one-time-code autofill, paste | ||
| * distribution, and keyboard movement between digits. | ||
| */ | ||
| export function CodeInput({ |
There was a problem hiding this comment.
This new shared component has no COMPONENTS entry, and Onboarding's composes list also omits it, so the /design inventory never exposes CodeInput or audits its Base UI backing; the current one-way registry tests still pass despite the gap. Add a proposed, owned registry entry and include it in Onboarding's composition list.
AGENTS.md reference: desktop-new/AGENTS.md:L127-L131
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| .text-field-error { | ||
| color: var(--red-11); |
There was a problem hiding this comment.
When a validation error is rendered on the dark panel used by these forms, --red-11 reaches only about APCA Lc 59.7, below the repository's Lc 60 target for text users must read. Because check-contrast.mjs audits --red-12 but not this newly consumed step, pnpm check also passes without detecting the regression; use the established --red-12 error step.
AGENTS.md reference: desktop-new/AGENTS.md:L50-L54
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 3ef9466878ab45dbecfb1886cf1c52f6988f9b2b..004f841cb01a386b3edba0c16ed2120a234bd2e8 (exact head 004f841cb01a386b3edba0c16ed2120a234bd2e8)
Risk: medium — new user-visible onboarding compositions and shared input behavior, with accessibility, responsive-layout, design-token, and test-policy contracts.
Behavior/contracts traced: intended 12-file desktop-new delta; Base UI and component-registry boundaries; CodeInput filtering, paste, focus, keyboard, disabled/autofill, callback, and animation lifecycle; profile selection semantics; narrow geometry; light/dark field and error styling. GitHub's ACP/link-preview entries are merged-main history rather than authored onboarding scope. No intended persistence, relay, tenancy, secrets, native lifecycle, or release behavior was added.
Blocking findings
-
Profile selection is not exposed as a single-select control —
desktop-new/src/features/onboarding/ui/OnboardingShowcase.tsx:235-257. The five ordinary buttons store selection only indata-selected. Exact-head Chromium exposed five unselected buttons/all tab stops; clicking Green changed only that private attribute, and ArrowRight did not move focus or selection.
Author action: use the appropriate Base UI/native radio-group semantics, expose checked state, preserve roving focus/arrow operation, and add a browser regression for role, checked state, Tab/arrow/Space behavior.
Verification owner: author test; reviewer AX/keyboard rerun. -
Code and profile controls are clipped and unreachable at narrow widths — fixed slot/profile widths in
desktop-new/src/shared/styles/components.css:667-677anddesktop-new/src/shared/styles/product.css:200-219remain inside hidden-overflow scenes atproduct.css:28-35;product.css:250-263does not adapt them. Exact-head measurements at 320/375/390px found scene widths288/343/358, code/profile scroll widths378/482, and clipped later controls/Continue.
Author action: add a shrinking/wrapping/reflow layout that preserves operability, and assert every digit, profile choice, and Continue remains contained at 320/375/390px.
Verification owner: author E2E; reviewer geometry/screenshots and mutation rerun. -
TextField and CodeInput use an undeclared surface token —
desktop-new/src/shared/styles/components.css:585,686references--bg-inset, but a tracked search ofdesktop-new/src/shared/stylesfound only those two consumers and no declaration. Exact-head Chromium computed the CodeInput background as transparent in both themes, making the promised inset surface depend on its container.
Author action: use the intended declared surface token or define/register/audit--bg-inset, with computed-style coverage in light and dark supported placements.
Verification owner: author test; reviewer theme rerun. -
The contrast guard is false-green for a known failing error consumer —
.text-field-erroruses--red-11atdesktop-new/src/shared/styles/components.css:635-637, whiledesktop-new/DESIGN.md:210-218,289-291documents that step below the Lc 60 body target on dark panel/composer and requires red-12.desktop-new/scripts/check-contrast.mjs:62-83excludes red-11 while claiming there are no consumers, sopnpm checkcertifies the opposite of rendered reality.
Author action: use the documented passing step and bind the rendered consumer to contrast coverage; mutation back to red-11 must fail.
Verification owner: author guard/test; reviewer mutation andpnpm checkrerun. -
Shared CodeInput bypasses required component boundaries and has no behavior regression seam —
desktop-new/src/shared/ui/CodeInput.tsx:23-203implements six native inputs plus filtering, paste, replacement, focus/Backspace/arrows, callbacks, disabled/autofill, autofocus, dual render state, and animation cleanup. This conflicts with the Base UI wrapping policy indesktop-new/AGENTS.md:112-116anddesktop-new/DESIGN.md:386-390; CodeInput is also absent fromregistry.ts:216-238,308-325, soregistry.test.ts:105-113only sees Input inherited through unrelated TextField. Searches across trackeddesktop-new/src/**/*test*anddesktop-new/tests/**/*spec*found no CodeInput behavior assertion; the suite remains 8 files/44 tests.
Author action: build slots on the applicable Base UI Input boundary, register CodeInput and its Onboarding composition honestly, bind the registry audit to CodeInput's own import, and add production-component tests for initial/partial autofocus, sequential entry/emissions, mixed paste/filtering/replacement, Backspace/arrows/focus, disabled state, and animation-end deletion/re-entry; mutation-prove material handlers.
Verification owner: author implementation/tests; reviewer policy, mutation, and full-package rerun.
Validation
At clean exact head 004f841cb01a386b3edba0c16ed2120a234bd2e8:
pnpm --dir desktop-new check— PASS (131 files), with finding 4 demonstrating its contrast blind spot.- Full
pnpm --dir desktop-new test— PASS, 8 files / 44 tests. - Generated-route Vite build, standalone typecheck, and production build — PASS; existing >500 kB chunk warning only.
- Focused Chromium probe — happy-path CodeInput entry/paste/replacement/Backspace/autofocus passed; chooser semantics and 320/375/390 containment failed as above; both themes computed the undeclared inset surface transparent.
git diff --check, DCO trailers, public-export docs, and scoped added productionunwrap/expectaudit — PASS.- Live head remained exact and mergeable immediately before submission. Required CI still had Rust unit/Windows, Desktop core/smoke, and relay-artifact jobs in progress; those are CI-owned confidence gaps, not additional author actions.
Manual/native evidence: Chromium covered visible layout, computed styles, and browser AX/keyboard contracts. Native VoiceOver/NVDA and OS OTP autofill were not run; desktop-new is currently web-only. These are reviewer/tooling confidence gaps and do not replace the concrete defects above.
Residual risk: native speech phrasing, OS autofill, and rapid animation interleavings remain independently unverified.
5 participants
Summary
insetandsoftvisual variantsValidation
0 0 0 2pxfocus and error ringsReview note
The branch includes a merge of current
main, required by the repository pre-push branch-skew guard. The onboarding foundation is commit7cd451337; the soft text-field comparison is commitec0fe9ba1.