Add page-owned sidebar contributions - #844

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b075645432

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Request changes: the plugin direction is sound, but two P2 host-integration defects need repair. Reviewed 2f0ec73058db22dccae9e4cd9e7166794b73d7e4 against 82d165d5b6fd7dc1f3e9784675d27c04aacdbeae.

Page.sidebar is an appropriate small extension: it follows the page’s existing registration/disposal, removes the Me-specific branch, and retains host-owned navigation, disclosure and error isolation. No second router, registry or sandbox claim is needed. Keep that design; address the admission and recovery-scroll findings inline.

One non-blocking product choice deserves an explicit contract: a sidebar can omit the supplied Pages children. Me already does, and Search remains available, so this is not a new lockout. Decide whether consistent primary navigation is mandatory before widening adoption. External width/resize helpers and cross-version SDK guarantees remain separate work, not prerequisites for this patch.

Validation: exact-head CI passed. Additional mounted-App probes reproduced rejected-target delivery and a pre-admission scope mismatch; the complete modified App profile test file passed 9/9, including three defect-confirmation probes. Independent real-App Chromium/WebKit diagnostics reproduced clipped recovery navigation and exercised healthy disclosure/focus behavior. Temporary fixture/test edits only; no production changes. Prior reported retry/doc fixes are present. Native/assistive-technology and human acceptance remain unverified.

Merge criteria: both inline fixes plus regressions, while retaining healthy same-page sidebar state and shell recovery. This is not an approval.

No changes requested: the two findings in the prior review are addressed in source, with regression coverage for rejected/scoped targets and recovery scrolling while retaining healthy sidebar state.

Star Lord’s automated source review via Wes’s account, at head c0282939c3f6475478735fcb0051d506be756c0f against base 37dcd836fbcf1613ddd27cece5a99e04d601bd64.

Source-only; no tests or app execution performed, so native/human acceptance remains unverified. One hosted CI snapshot showed CI required passing; Windows native validation was skipped.

🤖 Star Lord’s automated source review via Wes’s account.

8/10: one P2 lifecycle finding inline. Replacing the persistent conversation sidebar can silently lose a rejected mute/unmute and its Retry action. The new sidebar admission, independent error boundary, Settings retention and scrollable fallback otherwise remain consistent with the intended contract.

Reviewed head fb942ca919ed5f6f9f7ca1fcf775f6796629c819 against base/merge-base 986b4ca6299e655d9acbc7899c8fa5ca4a2d91d1, including the integration changes since previously reviewed c0282939c3f6475478735fcb0051d506be756c0f. Independent lanes: Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility/source-visible flows), integrated by Star Lord. Public description and feature commit inspected; no attached images or disclosure finding.

Separate validation gap: the new sidebar lifecycle test at src/app/App.profile.test.tsx:171 timed out at the default 5 s in four of five repetitions of the advisory Vitest job; the fifth passed. The log establishes the timeout, not its cause or a product failure. Investigate the slow phase; splitting independent lifecycle scenarios while preserving the transitions under test may help. Do not mask it by relaxing timeouts or removing coverage. The single CI snapshot still had required jobs running and Windows native validation skipped.

Source-only: no PR code, tests, builds or apps executed. Live rejected-write recovery, native behavior, rendered themes/layout, keyboard/assistive-technology behavior and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

8/10: one P2 introduced by the mute-recovery repair, detailed inline. The prior lost-notice defect is addressed: pending writes and failed-write notices now survive sidebar replacement. The recovery callback registration still needs to survive cached-to-live session replacement without an inventory remount.

Reviewed head e93b283cb338d9dc1c011504c9259a7f4faf2c27, base/merge-base 986b4ca6299e655d9acbc7899c8fa5ca4a2d91d1, focusing on the seven-file follow-up since fb942ca919ed5f6f9f7ca1fcf775f6796629c819. Rocket checked systems/performance/lifecycle; Mantis checked correctness/security/concurrency; Groot checked product/accessibility/source-visible flows; Star Lord independently reconciled the findings. The split tests retain the previous lifecycle transitions, and new gated cases cover failure before departure and while away, then recovery after returning. Public description and feature commit inspected; no attached images or disclosure finding.

Optional: add coverage for activating Retry/Dismiss while the contributed sidebar is still open. The new tests return to Messages before using these actions. With no inventory mounted, muteRecovery.current is absent; consider an appropriate surviving focus target for that path, using the existing shell/focus owners.

Validation limits: source-only; no PR code, tests, builds or apps executed. One hosted-check snapshot showed 28 successes, three WebKit jobs still running, one cancelled JavaScript job and Windows native validation skipped; CI completion and the author's timing/stability claims were not independently established. Live cached-to-ready recovery, keyboard/assistive-technology behavior, rendered themes/layout, packaged/native behavior and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

9/10 — no actionable findings in this follow-up. The previous P2 is addressed: removing the provider’s render-time reset leaves mute-recovery callback registration and identity-checked cleanup with the mounted ChannelInventory. Retry/Dismiss can retain their row-focus and menu-close callbacks through a cached-to-live handoff without an inventory remount. The two new gated regressions verify that the row survives, rejected writes expose recovery, Dismiss does not rewrite, and Retry closes the menu and saves the mute.

Reviewed head 6c81d6efb610834d17c1996451f60688c6fa7d2c against base/merge-base 986b4ca6299e655d9acbc7899c8fa5ca4a2d91d1, focusing on the two-file delta since reviewed e93b283cb338d9dc1c011504c9259a7f4faf2c27. Independent lanes completed: Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), integrated by Star Lord. Public description and follow-up commit inspected; no attached images or disclosure findings.

The previously optional coverage for activating Retry/Dismiss while the inventory is absent remains unadded; existing app tests return to Messages first. This repair adds no new browser cases or changes to the prior feature patches.

Validation limits: source-only; no PR code, tests, builds or apps executed. One hosted-check snapshot showed Chromium shard 6/9 failing, three WebKit shards still running, and Windows native validation skipped. The available failure annotation reports only exit code 1; cause and relation to this change remain unresolved. Both Vitest flake-check lanes passed in that snapshot, but the author's timing and fail-before/pass-after claims were not independently reproduced. Live handoff/focus, rendered themes/layout, assistive technology, packaged/native behavior and human acceptance remain unverified. This COMMENT is not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

9/10 — no actionable findings in this test-only follow-up. Compared with reviewed 6c81d6efb610834d17c1996451f60688c6fa7d2c, only src/app/App.profile.test.tsx changes; production code is identical. The expanded matrix preserves the prior Messages recovery cases and adds Retry/Dismiss while the contributed page remains open, checking the saved preference after Retry and notice cleanup after returning to Messages.

Reviewed head 0a344944dcc199892b55c84c5a4648e25a2c472d, base 986b4ca6299e655d9acbc7899c8fa5ca4a2d91d1. Used the small unchanged-feature follow-up exception to independent team review; the preceding feature review had all three independent lanes.

Validation: source and endpoint-diff review only; no tests, builds, or app execution. Current-head CI was still running in the single snapshot; Windows native validation was skipped. The new jsdom cases explicitly establish a focus target before failure and do not prove browser-native focus fallback, keyboard/assistive-technology behavior, rendered layouts, or packaged-app behavior. Public PR description inspected; no attached images. This COMMENT is not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

No actionable findings in this follow-up; source assessment: 9/10. Compared with reviewed revision 0a344944dcc199892b55c84c5a4648e25a2c472d, only src/app/App.profile.test.tsx changes (41 insertions, 12 deletions); production source is identical. The account-menu journey remains covered, and the separate same-session cache test gates refresh, checks the captured session and cached field, and releases pending work in finally. Existing cleanup restores spies. This uses the small unchanged-feature follow-up exception; no new independent team lanes were run.

Reviewed head: 3ba159a0b372213b52f53554a4b9fc84df061d5d. Base: ba0357f4ba2cfea8250496dbf43b927cce3a60ac. Public PR description and commit material inspected; no attached images or actionable disclosure finding.

Source-only: no tests, builds, app launches, or PR code execution. CI snapshot: Rust and tool integration failed; JavaScript, browser, and flake-check work was still running, and Windows validation was skipped. The failure’s cause and test-cost impact are unverified. This does not establish resolution of historical timeouts, rendered focus/accessibility, native behavior, or human acceptance. This is a non-blocking COMMENT review, not approval.

🤖 Star Lord’s automated source review via Wes’s account.

No new actionable findings in this rebase follow-up; source assessment: 9/10. The feature patch is identical to reviewed revision 3ba159a under git range-diff. The new base preserves sidebar admission, independent error recovery, Settings retention, session-owned mute notices and cached-to-live recovery registration. Nearby drag-click cleanup, session emoji-upload and shared-control changes show no source-demonstrated integration regression.

Reviewed head: 6d19147. Selected base: f63082f. Merge base/head parent: abdc0e4; the selected base adds a separate Windows test-path fix. Independent lanes: Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), integrated by Star Lord. Public description and feature commit inspected; no attached images or actionable disclosure findings.

Source-only: no PR code, tests, builds or apps executed. One hosted-check snapshot showed Chromium shard 1/9 and WebKit shard 2/12 failing, other browser/Rust work still running, and Windows native validation skipped. Failure causes, historical timeout resolution, rendered focus/accessibility and themes, native behavior, performance and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

No new actionable findings in this rebase follow-up; source assessment: 9/10. The feature’s production hunks are unchanged from reviewed revision 6d19147. Range-diff shows only profile-test context differences. The new base’s interrupted-agent notice and workspace-focus shortcuts show no source-demonstrated conflict with sidebar admission, Settings retention, independent sidebar error recovery, or session-owned mute Retry/Dismiss. Existing cached-to-live recovery registration and stale-write fencing remain intact.

Reviewed head: fe7ad48. Base/merge base: 1311492. Independent lanes completed: Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), integrated by Star Lord. This follow-up checks the rebase and prior fixes; unrelated base changes were not exhaustively re-reviewed. Public PR description and feature commit inspected; no attached images or actionable disclosure finding.

Source-only: no PR code, tests, builds or apps executed. One current-head hosted-check snapshot showed CI required and all other reported checks successful, except Windows native validation skipped. Historical timing claims, live failure/retry focus, rendered themes/layout, assistive technology, packaged/native behavior, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

No new actionable findings in this rebase follow-up; source assessment: 9/10. Compared with reviewed revision fe7ad48, the feature patch changes only to preserve the new base’s chatIcons prop and fixture context. The default PrimarySidebar keeps that wiring, and the new avatar provider and channel-creation changes remain intact. Prior sidebar admission/error isolation, Settings retention, session-owned mute Retry/Dismiss, and cached-to-live recovery registration remain intact in source.

Reviewed head: ead53c2. Base/merge base: fdc9fa3. Independent lanes: Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), integrated by Star Lord. This follow-up covers rebase integration and prior repairs; unrelated base changes were not exhaustively re-reviewed. Public description and feature commit metadata inspected; no attached images or actionable disclosure finding.

Source-only: no PR code, tests, builds or apps executed. A single current-head CI snapshot showed browser measurements, macOS WebKit recording, DCO and security checks successful; broad browser/JavaScript/Rust and flake-check lanes were still running, and Windows native validation was skipped. Current-head runtime correctness, live failure/retry focus, rendered themes/layout, assistive technology, native behavior, performance and human acceptance remain unverified. Historical validation in the description is not current-head proof. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Star Lord’s automated source review via Wes’s account.

No new actionable findings in this rebase follow-up; source assessment: 9/10. Compared with reviewed revision ead53c2, the feature patch adapts the shared ContributionBoundary to retain the base’s message-renderer failure callbacks and preserves the decisions export in the author-package test. Current callers use sidebar reset identity separately from message-renderer failure reporting. Sidebar admission, healthy-state retention through navigation/Settings, and session-owned mute recovery remain intact in source.

Reviewed head: ab7a0c0. Target base: 708294b. Diff merge base: df6843c. Independent lanes: Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), integrated by Star Lord. Scope is rebase integration and prior repairs, not an exhaustive re-review of unrelated upstream changes. Public description and feature commit metadata inspected; no attached images or actionable disclosure finding.

Source-only: no PR code, tests, builds or apps executed; CI was not assessed in this cycle. Native behavior, performance, rendered themes/layout, assistive technology and human acceptance remain unverified. Live Retry/Dismiss focus while away from Messages, including multiple concurrent notices, remains a validation gap; the shared Sonner focus restoration prevents treating the absent inventory callback alone as proof of focus loss. Historical validation in the description is not current-head runtime proof. This is a non-blocking COMMENT review, not approval or merge authorization.

🤖 Larry's independent review (agent, via Logan's account).

Verdict: approve. No blocking findings at ab7a0c0c9a693154e224207395f5b97660358cb6 (merge base df6843cbf).

I reviewed the diff and the surrounding owners first, before I read any earlier comments. I checked: page admission (startup, route params, scope) before the plugin sidebar gets a target; the boundary reset on a new PageNavigation request; retained-request identity through Settings (it does not reset a failed sidebar, and it does not remount a healthy one); the waiting/failure paths; the narrow-drawer toggle in AppShell; and the move of mute intent and Retry/Dismiss into ChannelNavigationProvider. There is one ChannelInventory instance, so the single muteRecovery slot is enough.

The earlier threads already covered admission, the scrollable fallback, reset on target change, mute recovery across replacement and the cached-to-live handoff. Those are fixed in source and I do not repeat them. The two inline notes are new and non-blocking. Both are small and you can do them here or in a follow-up.

Evidence: source review only. I did not run the tests locally. Hosted CI on this head: 37 pass, Windows native validation skipped.