Repository navigation
Conversation
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
|
…rney Apply focus scroll padding to ordinary dialogs after their popup became the scroll owner. Keep the strict narrow/enlarged WebKit viewport assertion and update the agent creation journey for its current fields and avatar controls. Co-authored-by: bmo <a7b504d9a2bfe905589e9ac237d7d8d023b8d10547060ec9618e6487b55d756c@buzz.block.builderlab.xyz> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 1a0b408 against target base 7535101. The feature diff starts at merge-base d92bfb2; unrelated base-only native-MCP changes are not PR removals.
Three actionable findings are inline: empty-name focus can remain under sticky chrome, stable-height modal footers no longer retain their reserved placement, and the existing creation browser journey still targets the replaced UI. Source assessment: 8/10 until those are addressed. Avatar creation/upload continues to use the existing media owner, pending exclusion and abort path. Clearing instructions on harness/model changes matches the stated product behavior and is not reported as an accidental defect.
Optional follow-ups: give the color swatches friendly names alongside their hex values and consider a concise selection announcement when dock artwork swaps. The selected artwork already has image alt text/video aria-label. Also prune detached children from createDockMagnifier’s values map on mode changes: React reuses the fieldset while replacing its keyed children, so pointer-only switches retain old slots until keyboard reset or unmount. Heap impact was not measured.
Validation: immutable source, callers, relevant product/design guidance, and whitespace checks. No tests, builds, installs, app launches or PR code execution. Browser cases added/removed: 0/0; the new component cases were inspected, and the author’s fail-before/pass-after result was not reproduced. The 11:34 UTC hosted-check snapshot has CI required failed: the agent-library journey fails in Chromium and WebKit on the removed name placeholder; a separate WebKit plugin-import enlarged-label check reports viewport ratio 0.99238 instead of full visibility (job 120127772231). That second failure’s cause remains unverified; do not relax its assertion without investigating. Windows native validation was skipped.
The public PR description and commit metadata were inspected with no evident sensitive disclosure. No screenshots are attached; the description’s local captures were unavailable for inspection. Native upload/create acceptance, rendered sticky geometry, error/retry focus, both themes, forced colors, enlarged text, screen-reader behavior, measured performance and final human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
| contain: paint; | ||
|
|
||
| > .buzz-dialog-body { | ||
| overflow: visible; |
There was a problem hiding this comment.
🤖 [P2] Account for sticky chrome when revealing the invalid Name field
Moving the scrollport to the popup makes the sticky header overlap the body, but useNamePrompt.ts:12–21 still regards the entire nearest scrolling ancestor rectangle as visible. If an empty Name is scrolled into that header-covered band and Create is pressed, prompt() focuses it with preventScroll, inView() returns true, and the reveal scroll is skipped. The user is left editing an obscured invalid field. Previously the body scrollport began below the header. Preserve the design guide’s modal focus/recovery contract by checking the unobscured region between the sticky bars (or keeping a separate body viewport), and add a focused regression for submitting while Name lies beneath the header. This is source-derived; rendered geometry was not executed.
| [data-placement="right"], | ||
| [data-size="expanded"] | ||
| ) { | ||
| display: block; |
There was a problem hiding this comment.
🤖 [P3] Preserve the expanding body in stable-height dialogs
This selector also matches height="stable"/"stable-tall" consumers such as ProviderDialog.tsx:78 and SoloShare.tsx:822. Their fixed outer height remains, but display:block removes the body’s flex:1 expansion. For short content, the footer now follows the content with unused space below; sticky bottom only constrains an overflowing footer and does not push it down to fill that space. Loading/result/error changes consequently move the actions inside a supposedly stable modal. Keep stable-height consumers on the existing layout, or retain an expanding body while adding the sticky treatment. Validate a current stable-height caller across short and long content; the content-height creation fixture cannot establish that contract.
| <div ref={nameControl}> | ||
| <Input | ||
| ref={nameField} | ||
| placeholder="Name" |
There was a problem hiding this comment.
🤖 [P2] Update the existing browser journey for the new creation controls
The shipped test tests/browser/agent-library.spec.mjs:26 still locates "Name your agent", so both Chromium and WebKit fail before exercising this form (hosted jobs 120127772342 and 120127772243). Changing only that locator will not finish the migration: the same case assumes the old transparent field styling, [data-avatar-view="selected"], Choose avatar button, and Gloopies button/catalog with a pressed selected tile; this PR replaces those with outlined fields and a swapping dock/menu. Update the existing journey to target the current semantic controls and retain its focus, narrow/enlarged-text, theme and selected-avatar assertions using the new UI contract. Run the affected complete browser test file in both engines rather than deleting or skipping the coverage.
Agent creation now opens with a randomized avatar and a centered picker, with a compact scrolling dock for bundled avatars or emoji background colors. The customization menu offers Gloopies, Emoji, and Upload image; emoji selection uses a popover and the tuned spring transition. Name and instructions remain directly editable, with harness/model settings below and creation in the modal footer.
Validation: independent review completed; avatar-catalog regression demonstrated failing before its fix and passing afterward; 21 creation tests pass. Pre-commit checks and all required pre-push lanes pass on 1a0b408 (types/related unit tests, design-system, applicable Clippy gate, and organization/history checks). Browser fixture inspected at 320×480, 375×667, and 667×320 for scrolling, model menu, keyboard access, and emoji search; desktop-sized fixture captures show the final UI. Arjun approved the responsive layout iteration.
Draft: final native upload/create acceptance, dark theme, enlarged text, full shared-modal/browser coverage, and hosted CI remain pending. No new browser test journeys were added or removed. Screenshots use real components with fixture data, not a live community.
Screenshot attachments pending: the available browser is signed out of GitHub. Reviewed captures are saved outside the repository.
CI repair (
e113cf066): reproduced both stale agent-library failures and the WebKit plugin-import clipping failure before fixing them. The ordinary modal popup now owns the existing focus scroll padding; the unchanged strict full-viewport assertion passes at 320px and 200% size. Updated the creation journey for the current accessible Name field and avatar menu/swatch selection, retaining focus, both themes, enlarged text, responsive layouts, mode restoration, and visible selection on hover. No browser journeys added or removed and no retries, delays, or assertion tolerances changed.Repair validation: complete agent-library file passes in Chromium/WebKit (4 tests); complete plugin-import, dialog-actions, and channel-modal-dismissal files pass in both engines (24 tests). Independent static review found no blockers. Pre-commit and every required pre-push lane pass at
e113cf066; design typecheck, guards, and build pass. Optional broad local runs were interrupted by host sleep/timeouts; hosted broad validation is running at https://github.com/block/buzz-app/actions/runs/39961122851. Native acceptance and other previously deferred checks remain unclaimed.