Repository navigation
Conversation
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 Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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: b075645432
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
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.
Co-authored-by: Codex (gpt-6.1-sol) <noreply@openai.com> Co-authored-by: Codex (gpt-6-luna) <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
2f0ec73 to
c028293
Compare
|
Addressed both required fixes from this review in For the optional Pages-children question, this patch deliberately keeps them optional. The shell contract states that Me uses its own navigation and Search still lists every active page. This does not introduce a requirement that every sidebar render primary navigation; width/resize helpers and SDK version guarantees remain outside this patch. The PR remains ready for review. Human/native acceptance is still pending; resolving the repaired threads is not an approval. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
2 participants
TL;DR
Let a plugin page provide its own sidebar instead of Channels. The shell keeps control of collapse and narrow-screen navigation. Me now uses the same page contribution, without changing its sidebar UI.
Page.sidebarreceives the currentOpenTargetonly after route validation and scope selection are ready. Supplied Pages navigation is optional; Me uses its own navigation, and Search lists every active page. Pages without a sidebar retain Channels. Sidebar failures keep the page and shell usable. A new navigation request retries a failed sidebar without remounting healthy state.#762 deliberately left a plugin sidebar API out of scope. This adds that small contract through the existing page registration lifetime, rather than a new routing or navigation system. The author types and plugin architecture docs describe the host/page boundary.
FOUNDATION files:
src/features/pages/service.ts,src/app/App.tsx, and the matching author-facing export insrc/plugins/author.ts. No other FOUNDATION files are changed.Validation
Regression coverage exercises registration/disposal, published author types, target updates, sidebar failure, same-page recovery, shell disclosure, and Me's registered sidebar. Mounted-App cases cover rejected parameters, denied scopes, and delivery after personal-scope selection. Browser journeys cover actual slot geometry, collapse/drawer behavior, Escape/focus, both themes, enlarged text, and scrolling to a failed sidebar's retry destination. Two browser cases are added; none are removed.
The admission cases and recovery-scroll case failed before their repairs. The target-change recovery test also failed before its fix; a temporary remount mutation failed the healthy-state assertion. At
c0282939, focused Vitest (6 files, 51 tests), TypeScript, scoped Biome, design checks and the author build passed. The complete page-placement spec passed in Chromium and WebKit (4 cases per engine, 8 executions).Human acceptance is pending; the review-completed marker is intentionally absent. No packaged/native-app result is claimed.
Test plan
source bin/activate-hermit, thenbin/pnpm test:browser tests/browser/page-placement.spec.mjs --project chromium --no-deps --headed -g 'a page sidebar uses the shell slot|recovery navigation scrolls'.— Posted by Codex (gpt-6.1-sol) on behalf of kylez