Conversation
Pages can declare sidebar destinations with navigation: [{ title, icon,
params?, requiresCommunity? }]. Channels contributes Inbox and Bestie,
Agents contributes Agents, and external plugins can add their own. The
host lists bundled pages before external ones. The sidebar no longer
hard-codes any destinations.
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
Reviewed head 3a21b543de7f0e153a29c103fe6b486271881276 against base/merge-base ac002d36aafbaae2a88fae34972f2a438bf1dcd6 (13 changed files; immutable GitHub blobs, no working-tree inputs).
Four actionable findings: three inline product findings (one P1, two P2), plus the public-material finding below. This is a non-blocking COMMENT review, not an approval or merge authorization.
[P2] Remove deployment-scoped identity from public commit metadata
This repository is public. The sole PR commit, 3a21b543de7f, exposes an internal relay deployment hostname and its agent public-key identifier in the author email, committer email, and Signed-off-by trailer. That unnecessarily publishes internal environment/account attribution; it is not a private-key leak. Use an appropriate public-safe attribution address in all three locations, preserving actual authorship and valid DCO certification, and coordinate any history rewrite with the branch owner. Editing only the PR description would leave these values public in the commit.
Scope and limitations
Read the affected page-registration, contribution lifecycle, shell/sidebar, navigation parser/controller, rendering/recovery paths, and changed tests; checked applicable contribution/plugin/channel/navigation docs and Buzz vision guidance. Inspected the PR description and both attached screenshots: the screenshots display fixture content and I found no additional actionable disclosure in them. Checked changed source/docs/tests and the commit metadata; no configuration, generated-artifact, or browser-test files changed.
Source analysis only: no PR code, tests, builds, app launches, or live workflows executed; CI not checked. Error/retry and focus paths were traced separately from successful navigation; browser/native layout and focus behavior remain unverified. The inline examples are source-derived, not runtime reproductions.
| } | ||
| // Image URLs are masks so plugin icons follow the row's text color. | ||
| function PageIcon({ icon: Icon }: { icon: string | ComponentType }) { | ||
| if (typeof Icon === "function") return <Icon />; |
There was a problem hiding this comment.
[P1] Isolate contributed icon render failures from the shell
icon is now arbitrary plugin React code, but it renders directly inside the persistent sidebar. If an otherwise valid contributed icon throws (for example because its optional data is unavailable), SidebarBoundary initially catches it, then renders its fallback containing the same SidebarNavigation and the same failing icon (lines 86–98 and 225–234). An error in that fallback escapes the boundary; neither AppShell nor the startup tree supplies an outer render boundary, so an icon failure can unmount the app instead of leaving Settings or Retry available. This is distinct from the existing PageBoundary, which never encloses these icons.
Wrap contributed icons in a small error boundary with a host-owned, non-plugin fallback, and reset it on contribution replacement. Add coverage with a throwing icon proving the other destinations, page, and recovery controls remain usable.
| pageId, | ||
| scope: community, | ||
| ...(entry.params !== undefined && { | ||
| route: { version: 1, params: entry.params }, |
There was a problem hiding this comment.
[P2] Open contributed params with the owning page’s route version
This hard-codes the page route version to 1, although PagesService.register accepts any positive safe-integer page.route.version and validates these params against that contract. A page registered with route.version: 2 and a valid navigation entry therefore renders an enabled destination, but clicking it is rejected by useAppNavigation (src/app/navigation.ts:73–80) because the target version differs. Retry reuses the same invalid version, so the destination cannot open. The outer OpenTarget.version: 1 is a separate protocol version and can remain fixed.
Carry the registered page’s route version through navigationDestinations/SidebarDestination and use it here when params exist. Cover a non-v1 page through the real navigation admission path, not only a mocked navigator.open.
| (entry.params === undefined || | ||
| JSON.stringify(target.route?.params) === JSON.stringify(entry.params)), |
There was a problem hiding this comment.
[P2] Compare destination params using navigation’s canonical semantics
parseOpenTarget recursively sorts object keys (src/features/navigation/targets.ts:83–114), while registration uses structuredClone and preserves declaration order. For a valid entry with params: { view: "active", filter: "unread" }, clicking it stores { filter: "unread", view: "active" } in the target. These stringifications differ, so the opened destination never receives its selected styling or aria-current. The tests use string params and a mocked navigator, which bypass this normalization.
The entry.params === undefined branch also treats a default-view entry as a wildcard: a page declaring both a default destination and a parameterized view marks both current when the latter opens. That contradicts the new page-and-params selection contract. Compare canonical params (including an explicit absent/default case rather than a wildcard), and add cases for nonalphabetical/nested object keys and default-plus-parameterized entries.
wpfleger96
left a comment
There was a problem hiding this comment.
I reviewed 3a21b543de7f0e153a29c103fe6b486271881276, including registration, contribution removal/replacement, navigation admission and sidebar rendering. The existing contribution registry and deterministic ordering are a good fit. I read the earlier review and am only adding two distinct registration-contract findings, not repeating the route-version, selected-state or render-boundary comments.
Both additional findings are IMPORTANT correctness/contract issues I think should be fixed before merging. This is a comment review, not an approval or a merge authorization.
Hosted CI passed for this head; Windows validation was skipped. I also ran a small source-level probe using this commit's actual validEntry and parseOpenTarget: a 257-item array and Infinity pass the new registration validator but fail navigation parsing, while an ordinary object route passes both. That probe did not exercise Cordis registration or React. I did not rerun CI suites or launch the app; icon behavior is source-traced, not a live reproduction.
| (typeof entry.icon === "string" && !!entry.icon.trim())) && | ||
| (entry.requiresCommunity === undefined || | ||
| typeof entry.requiresCommunity === "boolean") && | ||
| (entry.params === undefined || page.route?.validate(entry.params) === true) |
There was a problem hiding this comment.
[P2] Validate navigation params against the host contract too
I think registration needs to apply the host's route-data validation before accepting an entry, not just the plugin's route.validate. For example, a page accepting a list of IDs can register params: Array(257).fill("thread-id"): it passes this check and structuredClone, but parseOpenTarget rejects arrays over 256 elements. The same mismatch exists for non-finite numbers, excessive nesting and byte limits. Clicking that enabled row returns failed / invalid-target before the controller changes history or publishes a failure state (controller.ts:194–205), and the sidebar discards the result, so the button silently does nothing.
Please reuse the host's bounded JSON/target validation when preparing navigation entries, then run the page validator on the retained canonical params. Reject unusable entries during registration and cover a host-invalid but page-validator-approved value through the real admission path. This is separate from the earlier selected-state comparison finding: these destinations never open at all, even with route version 1.
| (typeof entry.icon === "function" || | ||
| (typeof entry.icon === "string" && !!entry.icon.trim())) && |
There was a problem hiding this comment.
[P2] Accept wrapped React components in the icon contract
The new type/documentation accepts a React component, but this check rejects React.memo(...) and React.forwardRef(...) components because their runtime value is an object, not a function. This affects the app's own icon exports too: defineIcon in src/shared/design-system/icons/createDecorativeIcon.tsx returns forwardRef(...), so a natural icon: RobotIcon declaration fails registration. Unless the plugin catches that exception, activation fails and all of its contributions are removed. The bundled entries avoid the problem only by wrapping the icons in fresh functions.
Please make validation accept the supported React component types and have PageIcon distinguish image URLs with typeof Icon === "string", rendering accepted non-string types as components. Updating validation alone leaves those types on the CSS-mask branch. Add registration/rendering coverage for a memoized or forwarded-ref icon alongside the function and URL cases.
|
Closing: #401 (now on main) replaced the sidebar design this PR changed, so these changes are no longer needed. The review findings were addressed in an unpublished follow-up, and that follow-up is moot now. |
🤖
Summary
Details
navigationlist when it is registered. Each entry has:title;icon: either an image URL, drawn in the current text color like panel launcher icons, or a React component (the bundled plugins use Phosphor icons);params(optional): selects a view inside the page. The value must pass the page's own route validation. An entry withoutparamsopens the page's default view;requiresCommunity(optional): keeps the entry disabled until a community is selected, as Inbox and Bestie behave today.paramsare copied, so a plugin cannot change them later.paramsboth match the current location. This is how Inbox and Bestie stay separately selected on one page.NavigationEntryis exported from the plugin author types.docs/plugin-architecture.mdnow describes this contract. It used to say that plugins have no sidebar contract.src/features/pages/service.ts,src/plugins/author.tsandsrc/app/App.tsx.Screenshots
The sidebar on this branch with only the bundled plugins. It is the same as before.
An example plugin ("Active threads") that adds one entry. The browser test harness loads test plugins as bundled plugins, so here the entry sorts among the bundled pages. An installed plugin's entry comes after Agents.