Skip to content

Let plugins decorate host sidebar rows and add sort modes - #1853

Open
kazemek21 wants to merge 1 commit into
mainfrom
kylez_NEXUS-3997_sidebar_row_contract
Open

kazemek21 wants to merge 1 commit into
mainfrom
kylez_NEXUS-3997_sidebar_row_contract

Conversation

@kazemek21

@kazemek21 kazemek21 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR

Let plugins decorate, replace, or hide existing channel and direct-message sidebar rows without copying the host renderer. Plugins can also add named sort modes to the existing Sort menu, with the selected mode saved in sidebar preferences.

The optional, versioned sidebarRows contract passes each plugin the row data and live host element. Contributions compose in a stable order and follow plugin enable/disable lifetime. A callback, invalid return, or child-render failure restores only that row's host element; a failed comparator preserves host order. Unknown plugin sort IDs fall back to the built-in A–Z sort while retaining the saved choice. Plugin choices live in a separate extension of the existing preference record so older clients still decode built-in sort fields.

This PR remains limited to Team/DM rows; Solo row support is deferred to a separate follow-up. This version does not add rows inside host sections or reorder them without a menu choice; plugins can continue adding their own sections through sidebarSections.

The FOUNDATION change in src/features/relay/session.ts routes selected plugin sorts through the existing activity loader, deduplication, and cancellation path. The startup reveal gate shares that activity-demand decision and retains its existing 1.5-second budget. This narrow demand-wiring change introduces no new data kinds, writes, or identities and needs stricter review.

Saved choices for sections absent from a caller’s snapshot are preserved; only superseded sort bindings are reclaimed. This avoids losing another device’s choice, but absent-section choices can continue to count toward the existing 104-entry limit. Unavailable plugin modes also retain activity demand until the user chooses built-in A–Z; disabling a plugin alone does not stop those reads.

Validation

  • An earlier revision passed 143 focused tests across nine files and five native sidebar-validator tests. It also passed five browser-fixture repetitions in each of Chromium and WebKit. Those runs predate the final rebase and review fixes. Required CI on d2ca135 passed, including JavaScript, Rust, Chromium, and WebKit. Required CI on the rebased activity/lifecycle fixes (ee129fd) passed, including JavaScript, Rust, Chromium, and WebKit.
  • Regression coverage includes decoration, replacement/hiding, per-row failure and recovery, composition, sorting persistence, disable/re-enable, and unchanged host output. Added review-fix cases cover stale sort bindings, independent source limits, focused-row identity, and native confirmation through encrypted readback. Local regression runs were refused by the host-load guard, so those additions have no executed fail-before unit record. A separate pre-fix native transport reproduction did show the false confirmation conflict. The latest regressions also cover activity loading/cancellation for plugin sorts and neighbor selection after deleting a custom-sorted row. Local execution was refused by the load guard and cloud verification did not start, so those cases also have no executed fail-before record; current-head CI supplies the pass-after evidence.
  • Latest review fixes preserve another device’s plugin choice during unrelated writes and align startup reveal with activity demand. The encrypted browser-writer regression failed on the old code. The new startup case had no executed fail-before run because the load guard refused it. The first CI run exposed an inventory fixture that omitted activity readiness; that fixture now reports unavailable activity for its no-transport session. Required CI on 235a46a passed, including all three JavaScript shards, both Vitest flake partitions, Rust, Chromium, and WebKit.
  • The new browser journey exercises the actual plugin toggle and Sort hit targets, reload persistence, keyboard selection, and disable restoration. No browser cases were removed. Menu hit testing and encrypted preference readback after reload need the browser fixture; component tests own the render/failure matrix.
  • Existing shared row, context-menu, drag, and section-menu owners remain in use. Captures cover light/dark themes, narrow/wide widths, and enlarged text. Descendant render failures can remount their row; callback and invalid-return fallbacks preserve its identity.
  • The native app was not exercised. Browser-fixture screenshots are agent checks, not human testing.

Screenshots

Before — plugin disabled

sidebar-rows-before

After — decorated/replaced rows and plugin sort

sidebar-rows-after-1440-light

Plugin sort in the existing menu

sidebar-rows-sort-menu

Narrow dark theme

sidebar-rows-after-390-dark

Generated by Codex (gpt-6.1-sol) on behalf of kylez.

@kazemek21
kazemek21 force-pushed the kylez_NEXUS-3997_sidebar_row_contract branch 3 times, most recently from cbc477e to d2ca135 Compare October 28, 2026 23:15
@kazemek21
kazemek21 marked this pull request as ready for review October 28, 2026 23:35
@kazemek21
kazemek21 requested a review from a team as a code owner October 28, 2026 23:35

@gilgtm gilgtm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 duel-review deep/dual — initial review feedback

[P2] Custom activity sort modes do not load activity history

The “Oldest activity first” example compares channel.lastActivityAt, but the session only loads channel activity when a saved sort is built-in recent or a filter is active. Selecting this plugin mode alone leaves most timestamps unknown, so the comparator falls back to its ID tie-break instead of activity order. Let activity-dependent plugin modes request the existing activity loader/readiness gate, or otherwise ensure the comparator receives the required data. Add coverage through the session loader. Evidence: session.ts.

[P2] Lifecycle completion chooses neighbors from the unsorted rows

The custom comparator changes the rendered sortedSections, but lifecycle neighbor capture and fallback use sections.flatMap(...). When a user leaves or deletes the selected row in a custom-sorted section, focus/selection can move to a different channel from the row adjacent on screen. Use the displayed ordering for both neighbor capture and fallback. Evidence: ChannelInventory.tsx.

[P3] Test the native signer’s 104/105 plugin-sort boundary

The native signer rejects pluginSort.g above 104 entries, but the new signer tests cover valid/invalid mode and register shapes without checking exactly 104 accepted versus 105 rejected entries. Add both cases at the native signer boundary. Evidence: identity.rs and identity/tests.rs.

[P3] Cover stale sort-binding cleanup through both host writers

The helper prunes bindings for removed sections, and both host sort writers pass the current section IDs, but the browser and native writer tests do not exercise that cleanup through the actual sort-write paths. Add one stale plugin-sort binding, perform a write with the section removed, and verify it is reclaimed through each adapter. Otherwise a regression can retain deleted entries until the 104-entry cap prevents later writes. Evidence: sidebar-sort.mjs.

All findings are from the completed deep-dual source review; the comparison remains head d2ca135a978ce18dfb211cca00620a8953b5f4af against base main (PR base SHA 6dc6e1db06b5e85dd31697bb28f64dbc00859ca9, current branch tip 91e25346668d7ce6f5e38ea53d75b447a75d576f, merge base 911526a8799f42265b655e72aa088a968573a4dd).

@kazemek21
kazemek21 force-pushed the kylez_NEXUS-3997_sidebar_row_contract branch 2 times, most recently from b567ba8 to ee129fd Compare October 29, 2026 03:40
@kazemek21

Copy link
Copy Markdown
Contributor Author

Thanks, Gil. The two P2 fixes are in ee129fd8471c791c258b495542da155964f95390:

  • Activity sorts: selecting a plugin sort now shares Recent’s existing activity read, deduplication, and cancellation path. The session-loader tests cover first demand, shared demand, live updates, cancellation, and re-entry for both built-in and plugin sorts. The plugin docs now explain when activity becomes available.
  • Lifecycle neighbors: neighbor capture and completion fallback now use the custom-sorted sections. A mounted-inventory regression deletes the selected middle row, removes it from the roster before completion, and checks that navigation opens its displayed neighbor.

Required CI on this head passed, including JavaScript, Rust, Chromium, and WebKit. The targeted local test run was refused by the host-load guard, so there is no executed fail-before record for these regressions. The native app was not exercised.

I’m declining the two P3 test additions as optional coverage, rather than fixing demonstrated defects:

  • Native 104/105 boundary: the native signer directly enforces pluginSort.g.len() <= 104, and its tests cover mode and register validation. The shared tests exercise the cap, but are not native signer-boundary proof; the extra native boundary matrix remains a coverage gap, not a known accepted-input failure.
  • Adapter cleanup: both writers pass current section IDs to the same editSidebarSort implementation. Its full-cap regression verifies reclamation of deleted/superseded bindings while preserving live unavailable choices. Separate cleanup cases in each adapter would add wiring coverage; there is no demonstrated adapter mismatch to repair.

Please reopen either point if there is a concrete failing path I missed.

Posted by Codex (gpt-6.1-sol) on behalf of kylez.

@kazemek21
kazemek21 requested a review from gilgtm October 29, 2026 03:56

@gilgtm gilgtm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P3] A stale section list can erase a valid plugin sort

Each sort write passes the caller’s preferences.data.sections as sectionIds (ChannelInventory.tsx). editSidebarSortRecord then uses that client list to prune choices from the freshly read shared sort record (sidebar-registers.ts). If device A creates a section and chooses a plugin mode, device B that has not received the section can change the sort on another group; B’s write deletes A’s still-valid saved mode, and that section falls back to A–Z. Prune only against an authoritative current section set, or preserve choices absent from a potentially stale list during unrelated sort writes.

Posted by duel-review skill [driver: codex(max), mode: deep-dual, issue surfaced by claude(high)]

[P3] Unavailable saved plugin sorts keep activity reads alive

Unavailable plugin sort choices are deliberately retained, while ChannelInventory falls back to A–Z when no registered mode matches (ChannelInventory.tsx; plugin contract). But refreshChannelActivity treats every saved plugin: value as demand and refreshes activity for the whole roster (session.ts). Disabling or uninstalling a plugin therefore keeps triggering reads on session starts and roster changes even though no comparator is visible. Count only currently registered plugin modes as activity demand, or have the inventory report active demand while keeping the saved choice.

Posted by duel-review skill [driver: codex(max), mode: deep-dual, issue surfaced by claude(high) and codex(max)]

@kalvinnchau kalvinnchau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 One additional startup-ordering finding below. The stale-section pruning defect already reported in the existing review also remains actionable: an unrelated sort write can silently erase another device’s saved plugin choice. We assess that as P2 preference loss; it is not repeated inline.

Comment thread src/features/relay/session.ts Outdated
@kazemek21
kazemek21 force-pushed the kylez_NEXUS-3997_sidebar_row_contract branch from ee129fd to 748cae3 Compare October 29, 2026 05:03
Co-authored-by: Codex (gpt-6.1-sol) <noreply@openai.com>
Signed-off-by: Kyle Zemek <kylez@squareup.com>
@kazemek21
kazemek21 force-pushed the kylez_NEXUS-3997_sidebar_row_contract branch from 748cae3 to 235a46a Compare October 29, 2026 05:13
@kazemek21

Copy link
Copy Markdown
Contributor Author

Regarding the stale-section finding, also noted in the startup review:

Thanks, Gil. The stale-section preference-loss fix is in 235a46a5378cda28f2638073cd6e46e0d66259c1. Sort writes no longer treat a caller’s section snapshot as proof of deletion. They preserve other groups’ plugin choices and reclaim only bindings superseded in the sort record itself. An encrypted browser-writer regression reproduced the loss before the fix. Choices for absent sections can therefore remain within the existing 104-entry limit; a full record rejects a new choice rather than discarding another device’s preference.

Required CI on this head passed, including JavaScript, Rust, Chromium, WebKit, and both Vitest flake partitions. The targeted local pass-after run was refused by the host-load guard; there is no executed startup fail-before run. The native app was not exercised.

On unavailable modes retaining activity reads, I confirmed the behavior and am leaving that optimization out of this change. The saved choice remains so re-enabling the plugin restores it, while row order falls back to A–Z. Activity uses the existing roster deduplication, 128-channel batches, cancellation, and 10-second per-read timeout, with no new polling. Disabling a plugin does not itself stop those reads; cold startup can also use the existing 1.5-second reveal budget. Selecting built-in A–Z removes that sort’s demand. Avoiding this extra work would require feeding plugin availability into the session’s activity-demand owner; there is no demonstrated incorrect row order or material performance regression to justify that additional coupling here.

Please reopen either point if the fix misses a failing path or the retained reads cause a concrete performance problem.

Posted by Codex (gpt-6.1-sol) on behalf of kylez.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Star Lord’s automated source review via Wes’s account: no actionable findings. Rocket, Mantis and Groot completed independent lifecycle, persistence/security and product/accessibility lanes; I integrated them and checked the public description and four screenshots.

Reviewed head 235a46a5378cda28f2638073cd6e46e0d66259c1, base e210c8a472cc49ee7c0b49b62bfad10fd5ef4c8e. Source-only: no tests or app execution; one hosted snapshot showed CI required successful and Windows native skipped, while native behavior, performance and human acceptance remain unverified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants