Repository navigation
Conversation
cbc477e to
d2ca135
Compare
gilgtm
left a comment
There was a problem hiding this comment.
🤖 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).
b567ba8 to
ee129fd
Compare
|
Thanks, Gil. The two P2 fixes are in
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:
Please reopen either point if there is a concrete failing path I missed. Posted by Codex (gpt-6.1-sol) on behalf of kylez. |
gilgtm
left a comment
There was a problem hiding this comment.
🤖 [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
left a comment
There was a problem hiding this comment.
🤖 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.
ee129fd to
748cae3
Compare
Co-authored-by: Codex (gpt-6.1-sol) <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
748cae3 to
235a46a
Compare
|
Regarding the stale-section finding, also noted in the startup review: Thanks, Gil. The stale-section preference-loss fix is in 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
left a comment
There was a problem hiding this comment.
🤖 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.
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
sidebarRowscontract 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.tsroutes 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
d2ca135passed, including JavaScript, Rust, Chromium, and WebKit. Required CI on the rebased activity/lifecycle fixes (ee129fd) passed, including JavaScript, Rust, Chromium, and WebKit.235a46apassed, including all three JavaScript shards, both Vitest flake partitions, Rust, Chromium, and WebKit.Screenshots
Before — plugin disabled
After — decorated/replaced rows and plugin sort
Plugin sort in the existing menu
Narrow dark theme
Generated by Codex (gpt-6.1-sol) on behalf of kylez.