Add a shared full-page surface - #29
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head efec988ab28a238e344cea93623638422e1b443c against base e70ac4565cb560e05e54948c8badcd33f4c94950.
Changes requested: two P2 regressions in the Agents migration. The optional FullPageSurface composition and page/session ownership remain sound. Restore agent-specific disclosure names and actual deferred avatar fetching; neither fix requires changing library grouping, panel lifecycle, or page composition.
Validation: source/caller review plus focused Chromium and WebKit probes using the actual production components in a separate exact-head worktree with locked, isolated dependencies. Both engines reproduced indistinguishable identity button names and an offscreen DS avatar request before scrolling, while the legacy Avatar deferred its request. No-referrer behavior held. These are browser/fixture results, not attended screen-reader or native-app acceptance. Broad CI-equivalent suites were not rerun.
Separate CI note: run 34649306172 is red because WebKit shard 1 failed appearance.spec.mjs:161 at the fixture console-error gate, reporting relay stream/query access-control errors. This journey does not open Agents/Projects; I have not established a causal link to this diff or called it a harmless flake. Other CI lanes passed. This is separate from the two code findings below.
|
🤖 Both requested changes are addressed at |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 loading-state regression in the shared Avatar. Reviewed head 7cdf9ad7fd7d3ef7e1f7371a23b8a4b09a77d2b5 against base e70ac4565cb560e05e54948c8badcd33f4c94950.
The earlier agent-specific disclosure-name and eager-offscreen-fetch findings are addressed. FullPageSurface remains an optional Panel composition, with padding/scrolling owned by Agents and Projects; session identity, read/retry, and host appearance ownership are preserved. This review does not request a panel-lifecycle or fullscreen redesign.
Blocking: the new delayed fallback and pending image occupy separate implicit grid rows rather than the same avatar slot. See the inline finding. Keep the lazy/async/no-referrer correction and fix the loading-state placement, with a held-image regression case.
Non-blocking: Projects retains legacy heading typography inside the new shared boundary (src/bundled/projects/index.tsx:19); use the intended named type role. Also restore the valid default PNG in tests/fixtures/agents.tsx:48 and its successful-image assertion: the data payload shrank from 68 to 50 bytes and no longer contains a valid IDAT chunk, while the old naturalWidth check was removed. These are not additional production blockers.
Validation and limits: pinned-object source review and independent layout/accessibility review on the designated Mac Studio; no PR code was checked out or executed. Traced shared Avatar consumers (Agents, ProfilePanel, and the design-system specimens), missing/pending/success/error/source-replacement states, registry/specimen wiring, and page sizing/scroll ownership. Native/attended accessibility acceptance was not performed.
Existing CI run 34654689011 tested GitHub merge cb629ae126aa4239cb713472bd3cd05af7066e70 (parents 54383673556b05de89b3efb9ac32fc6ae197a956, 7cdf9ad7fd7d3ef7e1f7371a23b8a4b09a77d2b5). Its Agents, avatar-loading, Profiles and Projects WebKit journeys passed, but WebKit shard 1 failed the existing panel-resize journey at timeline.mjs:113: upward movement yielded 344px against >400px. This PR changes neither that case/helper nor the Messages runtime paths used by it. I have not established that failure’s root cause or called CI green; it is separate from the loading-state finding.
|
🤖 Updated in bac639c: avatar pictures are now proven decoded and visible, pending initials retain their position, failed images disappear cleanly, and changing from a loaded URL to a held/failed URL and back recovers. Restored the damaged inline test image and its successful-display assertion. Chromium/WebKit complete Agents and avatar-loading files pass; design-system tests (39) and build pass. Please re-review this latest head. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
REVIEW CLEAR: the prior production blockers are resolved
Reviewed head bac639cf76a8b5039bc09d216972a81cb44d674e against base e70ac4565cb560e05e54948c8badcd33f4c94950. This COMMENTED review is not approval or authorization to merge; required CI remains a separate failing gate.
- P2 loading-state placement resolved.
components.css:194–227positions the pending image absolutely within the relative avatar root, removing it from grid flow. The fallback is now the only in-flow grid item. The image stays transparent untilAvatar.tsx:17–29marks a successful load; success removes the fallback, error removes the image, and the source-keyed child resets state on replacement. Lazy loading, async decoding, no-referrer requests and the named outer image remain intact. The earlier agent-specific disclosure-name correction is unchanged. - Integration retained. Compared exact objects with previously reviewed
7cdf9ad7: Agents, Projects, ProfilePanel, FullPageSurface, registry and specimens are unchanged. Rechecked the shared Avatar consumers and missing/pending/success/error/source-replacement/unmount states. The optional full-page surface still composes Panel; pages retain content padding, scrolling, spatial composition and session ownership. No panel-lifecycle redesign is requested. - Non-blocking follow-ups. The held-image test at
avatar-loading.spec.mjs:78–80checks fallback text, image opacity and the fixed outer box, but not fallback centering or image bounds. Add those inner-placement assertions so reverting absolute positioning cannot silently reintroduce the grid defect. Princess Donut independently identified this coverage limit; it does not negate the inspected production fix. The inline fixture now has decodable IDAT data and restorednaturalWidth > 0/visible-image checks, but its IDAT CRC is still mismatched (efa3c35bstored versusefa2a75bcomputed); use a conformant fixture rather than relying on browser tolerance. Projects' previously noted legacy heading typography remains non-blocking.
Validation and CI boundary
Hosted run 34656275027 passed JavaScript, Rust/tool integration, browser measurements, both Chromium shards and WebKit shard 2. WebKit shard 1 explicitly passed Agents, avatar-loading and Profiles. Its sole failure was the existing panel-resize journey's wheel setup, timeline.mjs:113: 325px movement against >400px. This PR does not change that helper, the failing case or its Messages runtime paths; its cause is not established here and CI is not green.
The job checked merge 5783c9a57f43433774192f02cd6711231b2b9e68, whose parents are 54383673556b05de89b3efb9ac32fc6ae197a956 and this PR head. All five incremental files are object-identical between that merge and the reviewed head; the merge also includes newer main changes, so these are merge-run results, not an isolated-head test claim.
All repository reads, diffs and artifact processing ran on strict-host-verified BLKD2G9MWNRWY.local using pinned objects. Head/base git diff --check passed; the unrelated checkout stayed clean at dbad6526346e7350c528280ccc5cf6c68e740607. No checkout, PR-code execution, dependency installation, build, test or browser/native run was performed by this review. Hosted results are reused evidence. Attended accessibility/native acceptance was not performed.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
REVIEW CLEAR: no new blockers in the merge update
Reviewed head 04e30f0b0fe05a06a3e0041eaadd0d5ed8ab32a9 against base 17f90c18fff6b86bc029e710401fb2b60bc385ea. This COMMENTED review is not approval or authorization to merge.
- Prior fixes preserved. The shared Avatar, its CSS, Agents, Projects, FullPageSurface, registry/specimens, and avatar/Agents regressions are object-identical to the previous clear head,
bac639cf76a8b5039bc09d216972a81cb44d674e. Rechecked missing/pending/success/error/source-replacement/unmount states: the pending image remains out of grid flow; delayed fallback, load/error transitions, source-keyed reset, lazy loading, async decoding, and no-referrer requests remain intact. Agents’ identity disclosures remain agent-specific. - Integration retained. FullPageSurface still composes Panel without taking ownership of page layout, content padding, scrolling, session lifetime, or host appearance. Rechecked Agents, Projects, ProfilePanel and design-system consumers. The merge retains full-workspace bounds assertions and internal scrolling coverage; the independent test/integration lane found no new blockers. Unrelated Markdown and terminal features already on main were not re-audited as new PR work.
- Existing non-blocking follow-ups remain non-blocking. The prior review’s inner-avatar placement assertions, conformant PNG fixture, and Projects typography follow-ups are unchanged by this merge. They are not new production defects or reopened blockers.
Validation: strict-host-verified Studio source/metadata review of pinned objects, ownership/caller comparisons, and git diff --check against the stated base (passed). No checkout, PR-code execution/imports, dependencies, tests, builds, browser/native probes, or CI polling. Current-head CI status was not assessed; historical checks and PR-body validation claims are not presented as verification of this head. No native or attended accessibility acceptance is claimed.
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
04e30f0 to
01c5f9e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
REVIEW CLEAR: no new blockers after the mainline refresh
Reviewed head 01c5f9e1a3a1a99c920d35172d7d07f4f99b4561 against base b5d3f0c7fd49c16e81ee084012f2d0ab5fc92494. This COMMENTED review is not approval or authorization to merge.
- Prior fixes preserved. FullPageSurface, Panel, Agents, Projects, shared Avatar/CSS, registry/specimens, and the Agents/avatar regressions are object-identical to the previously cleared head,
04e30f0b0fe05a06a3e0041eaadd0d5ed8ab32a9. Rechecked the missing/pending/loaded/failed/source-replacement/unmount paths: lazy loading, async decoding, no-referrer requests, source-keyed reset, and the out-of-flow pending image remain intact. Identity disclosures retain their agent-specific accessible names and exact keys. - Current integration holds in source. FullPageSurface still delegates appearance to Panel; pages retain padding, scrolling and composition, while the host retains appearance and page/panel lifetime. Checked the current shell, Agents session/refresh boundary, Projects sizing assertions, and ProfilePanel’s inherited View activity addition. That addition does not alter its avatar inputs or session/identity keying. Independent avatar/identity review is integrated. The two feature-adjacent test files changed since the prior review only through mainline activity/Settings coverage; the surface assertions remain intact. Unrelated mainline membership, telemetry, and timeline behavior were not re-audited as new PR work.
- Previous non-blocking follow-ups stay non-blocking. Stronger inner-avatar geometry assertions, a conformant PNG fixture, and Projects heading typography remain follow-ups, not reopened production blockers.
Validation: strict-host-verified Studio source review of pinned Git objects and git diff --check against the stated base (passed). The existing checkout remained clean and was not used as the reviewed revision. No checkout, PR-code execution/imports, dependencies, tests, builds, browser/native probes, or CI polling. Current CI was not independently assessed; PR-body validation claims and historical runs are not verification of this head. No native or attended accessibility acceptance is claimed.
* origin/main: fix(messages): prevent native WebKit prepend blanking (#49) feat: add shared browser and standard desktop notifications (#38) Prevent whole-window scroll bounce (#41) test: replace timing races with explicit lifecycle barriers (#45) test(browser): isolate fixture optimizer caches (#46) test(browser): settle startup catch-up before request baselines (#43) feat(messages): show message-carried blurhash before decoded originals (#40) Add a shared full-page surface (#29) fix(messages): prevent image-driven timeline scroll snap-back (#31) Restore grouped channel membership activity (#30) feat: add broker-backed Agent Activity panel (#36) Signed-off-by: Clay Delk <clay.delk@gmail.com> # Conflicts: # src/bundled/channels/ChannelsPage.tsx # src/features/messages/ChannelTimeline.tsx # src/features/messages/MessageRow.tsx # src/features/relay/contracts.ts # src/features/relay/fold.ts
What this does
Adds an optional full-page surface for pages that use one rounded workspace-filling region, and adopts it in Projects and Agents. Agents also moves its visible controls, typography, avatars, and identity disclosure onto the shared design system.
This does not make panels fullscreen or change panel lifecycle. Pages continue to own spatial composition, content padding, scrolling, and whether they use zero, one, or multiple panels.
Why it matters
Pages no longer need to rebuild the same full-workspace surface recipe. The component name keeps that single-surface choice distinct from ordinary panels used inside multi-panel layouts.
Agents and Projects now share the same responsive light/dark surface while retaining page-specific content behavior.
New rule:
FullPageSurfaceis optional; it fills the available workspace, while its page owns everything arranged inside it.How it works
The shared surface composes the design system's existing Panel appearance rather than duplicating its visual rules. Projects centers its placeholder within the surface. Agents owns its internal scrolling and readable content width, and uses shared controls and type roles throughout the migrated boundary.
Shared avatars preserve lazy loading, asynchronous decoding, and no-referrer requests. Browser coverage verifies workspace sizing, internal scrolling, authored appearance in both modes, and narrow, intermediate, and wide layouts.
Verification
bin/just scan