fix(desktop): order unnamed roster members by full canonical npub - #7503
Conversation
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: a1ffa774b291557c57860baecc05f2220b6dc8ae..07bcf2e4051335b4d119d76a29d2c1b6fec71193 (exact head 07bcf2e4051335b4d119d76a29d2c1b6fec71193)
Risk: medium — shared Desktop roster comparators change visible ordering across the Members sidebar and channel-management member lists, but do not alter identity storage, authorization, relay contracts, row identity, or interaction semantics.
Behavior/contracts traced: canonicalNpub normalization and invalid-key fallback; authored-name, unnamed-member, duplicate-name, current-user, and role precedence; all production consumers of compareMembersByRole, compareMembersForModal, compareMemberNames, and formatMemberName; virtualized row identity; the focused visible-order regression seam. The change matches the PR’s deterministic-ordering intent and the repository identity/display direction without expanding into relay, schema, persistence, or release scope.
Findings: no blocking or non-blocking code defect found. desktop/src/features/channels/lib/memberUtils.ts:28-79 keeps compact npubs presentation-only, orders unnamed members by full canonical npub, and deterministically breaks collation-equal surfaces by the full identity key. Existing current-user and role precedence remain ahead of that stage at memberUtils.ts:81-96; the Members sidebar preserves its coarser owner/admin/other ranking at desktop/src/features/channels/ui/MembersSidebar.tsx:108-127. Full pubkeys remain the stable rendered-row identity, so reordering does not retarget actions. The unit cases at memberUtils.test.mjs:37-121 cover opposite input orders, duplicate names, all role tiers, current-user precedence, and invalid keys; the visible test at desktop/tests/e2e/channels.spec.ts:4266-4315 binds the actual sidebar comparator and fails under the old compact-label ordering.
Author action: none.
Verification owner: reviewer/tooling for any optional native installed-app spot-check; CI/release gate for normal merge policy.
Validation at matching clean HEAD:
just desktop-ci— PASS at07bcf2e4051335b4d119d76a29d2c1b6fec71193; clean before and after.- Independent full Desktop unit package — PASS, 6,469/6,469.
- Independent focused Playwright visible-order journey — PASS, 1/1.
- Independent mutation proof replacing the sidebar delegation with the old compact-label comparator — expected FAIL (
v24Position=4,v5Position=3); restoration returned the tree clean. - Independent comparator mutation reverting
compareMembersByRoleto compact-label ordering — expected unit failures; restoration returned the tree clean. git diff a1ffa774b291557c57860baecc05f2220b6dc8ae..HEAD --check— PASS.- Live required checks inspected at the exact head: Desktop core, all four smoke shards, integration shards, macOS/Windows builds, DCO, Semgrep, and zizmor green; no pending or failed check run.
Manual/native evidence: focused browser-rendered Members sidebar workflow exercised at the exact head. Native Tauri/VoiceOver observation was not run; no semantics, layout, or input code changed.
Residual risk: a real installed-app/native assistive-tech pass could still expose an environment-specific presentation issue, but this is a non-blocking reviewer-verification gap, not author rework. The changed comparator behavior is covered at both pure and rendered production seams with mutation evidence.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: a1ffa774b291557c57860baecc05f2220b6dc8ae..07bcf2e4051335b4d119d76a29d2c1b6fec71193 (exact live head 07bcf2e4051335b4d119d76a29d2c1b6fec71193)
Risk: medium — user-visible ordering in shared roster comparators and a virtualized member list; no relay, persistence, IPC, identity-storage, or release contract changes.
Behavior/contracts traced: authored-name ordering, full canonical-npub fallback and tie-breaking, malformed-key fallback, current-user/role precedence, classified sidebar/archived lists, member management, stable row identity/actions, and the visible virtualized roster seam. Compact npub labels remain presentation-only. Canonically equivalent valid encodings compare equal, as they represent the same identity.
Findings: no blocking or non-blocking code defects. The implementation centralizes the name/key stage in desktop/src/features/channels/lib/memberUtils.ts:28-79; both production ordering paths reach it through compareMembersByRole or compareMemberNames (useClassifiedMembers.ts:52-82, MembersSidebar.tsx:108-127,188-194). Full pubkeys remain React/virtual-list keys (MembersSidebar.tsx:687-689,815-823), so reordering does not retarget member actions. Unit coverage exercises opposite input orders, compact/full disagreement, duplicate names, role tiers, current-user precedence, and invalid keys (memberUtils.test.mjs:36-121). The rendered E2E inserts the collision pair opposite the expected order and asserts their relative row order (channels.spec.ts:4266-4315).
Author action: none.
Verification owner: CI/release owns normal merge and artifact gates. An installed-app/native accessibility spot-check is optional and non-blocking because no semantics, layout, or input code changed.
Validation at matching exact head:
- Two independent clean-checkout runs of the full Desktop unit package:
pnpm test/pnpm --dir desktop test— 6,469 passed, 0 failed. pnpm typecheck && pnpm check— pass.- Focused built smoke E2E for full-npub roster ordering — 1/1 pass.
- Mutation proof at the production modal seam: restoring the old compact-label comparison made the focused E2E fail on all three attempts (
v24Position=4,v5Position=3); restoring exact head passed. - Comparator mutation: restoring the old
compareMembersByRolebehavior failed the new full-npub/invalid-key unit assertions; restoring exact head passed. git diff --check— pass; exact-head worktree clean.- Fresh CI inspection: Desktop core, four smoke shards, both integration shards, Windows/macOS builds, DCO, Semgrep, and zizmor are successful; PR reports
CLEAN.
Manual/native evidence: no real Tauri/VoiceOver observation. Browser-rendered row order is directly proven by the focused E2E; no accessibility semantics changed.
Residual risk: native observation was not run. Given the bounded comparator-only production change, direct browser seam coverage, mutation evidence, full package tests, and platform builds, this does not imply author rework or block approval.
— :bot: Jude’s code review agent
8cce3c0 to
30cac57
Compare
07bcf2e to
4d2aff9
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 30cac57f71a374d1176667c5c7efcf4c76070e05..4d2aff9af4b17df989be9cf1b5c4e473adaf8ee5 (exact head 4d2aff9af4b17df989be9cf1b5c4e473adaf8ee5)
Risk: medium — deterministic roster ordering across shared member-list consumers and a visible sidebar seam.
Behavior/contracts traced: current-user/role precedence; authored-name versus full canonical-npub ordering; full-identity tie-breaks; classified member lists, channel management, and sidebar grouping; virtualized row identity/action targeting; relay/IPC/persistence boundary impact.
Findings: no unresolved author-actionable defect. Both assigned lanes are clear. The changed comparator preserves current-user and role precedence, sorts unnamed members by full canonical npub rather than ambiguous compact labels, and retains full-pubkey row keys so reordering cannot retarget member actions. The changed-head E2E now binds the actual rendered sidebar seam and reverses under mutation of production delegation.
Author action: none.
Verification owner: CI/release gate for the remaining nonterminal exact-head Desktop Core and Smoke shard 4 checks; reviewer/tooling for optional native observation. A later PR-caused gate failure or new head requires fresh review.
Validation: clean detached exact-head lanes passed git diff --check, full Desktop unit suite (6,470/6,470), just desktop-ci, and a focused built Playwright journey. Independent mutation of the production comparator delegation failed the intended rendered-order assertion and restoration returned a clean tree. At final refresh, Windows/macOS builds, smoke shards 1–3, relay-backed integration shards, DCO, Semgrep, and zizmor were green; Desktop Core and Smoke shard 4 were still running without failure.
Manual/native evidence: no fresh native Tauri/VoiceOver artifact; sorting changes no semantics, focus, input, labels, or layout.
Residual risk: exact-head CI completion remains externally owned. One product-lane local Playwright command selected zero tests due to a wrong testDir-relative path, but the systems lane’s focused built journey passed and the checked-in CI smoke journey is running; this is reviewer-command debt, not a defect.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 30cac57f71a374d1176667c5c7efcf4c76070e05..4d2aff9af4b17df989be9cf1b5c4e473adaf8ee5 (exact head 4d2aff9af4b17df989be9cf1b5c4e473adaf8ee5)
Risk: medium — user-visible ordering in shared Desktop roster comparators and a virtualized member list; no relay, IPC, persistence, authorization, identity-storage, lifecycle, schema, or release contract changes.
Behavior/contracts traced: authored-name/full-canonical-npub ordering and deterministic identity tie-break; malformed-key fallback; current-user and role precedence; classified people/bot/archive lists; Channel Management; the sidebar's coarser owner/admin/other grouping; stable virtualized-row identity/actions; and the rendered sidebar regression seam.
Findings: no blocking or non-blocking code defect found. desktop/src/features/channels/lib/memberUtils.ts:28-79 owns the name/full-canonical-npub stage and deterministic identity tie-break while current-user and role precedence remain ahead of it at memberUtils.ts:81-96. Production paths delegate through useClassifiedMembers.ts:52-82, ChannelManagementSheet.tsx:135-141, and MembersSidebar.tsx:108-127,188-194. Full pubkeys remain rendered/virtual-list keys at MembersSidebar.tsx:682-689,815-823, so reordering does not retarget actions. The changed-head E2E inserts the collision pair opposite expected order and asserts the actual visible sidebar order (desktop/tests/e2e/channels.spec.ts:4266-4315).
Author action: none.
Verification owner: CI owns the still-running exact-head Desktop Core gate. This is an external merge gate, not author rework. Reviewer/tooling owns any optional installed-native accessibility observation.
Validation at matching exact head:
- Independent clean detached checkout: merge-base matched the reviewed base;
git diff --checkpassed; tree remained clean. - Full Desktop unit package
just desktop-test— 6,470/6,470 passed. - Independent
just desktop-ci— passed, including Desktop checks, typecheck/build/unit, and Tauri test coverage. - Focused built Playwright sidebar journey — 1/1 passed.
- Mutation proof at the production
compareMembersForModaldelegation — all three attempts failed on the intended ordering assertion (v24Position=4,v5Position=3); restoring exact head returned the tree clean. - Live exact-head CI: all four smoke shards, both integration shards, macOS/Windows builds, DCO, Semgrep, and zizmor passed; Desktop Core remains in progress with no failed check.
Manual/native evidence: no installed Tauri/VoiceOver observation. No semantics, labels, focus, input, or layout code changed; browser-rendered row order is directly covered.
Residual risk: exact-head Desktop Core has not reached a terminal state. Normal branch protection must keep owning that gate. Native assistive-tech observation was not run, but the bounded comparator-only change, production-seam E2E, mutation evidence, full package validation, and platform builds make this non-blocking.
— :bot: Jude’s code review agent
30cac57 to
bd05f63
Compare
4d2aff9 to
42a2c32
Compare
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: bd05f631fc1ecd10307ee85234db56a474f7206f..42a2c32345da260490dc432d3544e147934c9f57 (exact head 42a2c32345da260490dc432d3544e147934c9f57)
Risk: medium — user-visible ordering in shared Desktop roster comparators and a virtualized member list; no relay, protocol, IPC, persistence, authorization, identity-storage, lifecycle, community-cache, schema, or release contract changes.
Behavior/contracts traced: authored-name/full-canonical-npub ordering and deterministic identity tie-break; invalid-key normalization; current-user and role precedence; classified people/bot/archive lists; Channel Management; the sidebar's coarser owner/admin/other grouping; stable virtualized-row identity and actions; keyboard/focus/AX order; and the rendered sidebar regression seam.
Findings: no blocking or non-blocking code defect found. Compact npub remains presentation-only in desktop/src/features/channels/lib/memberUtils.ts:17-26; compareMemberNames owns the full-canonical-npub ordering and identity tie-break at memberUtils.ts:28-79, while current-user and role precedence stay ahead of that stage at memberUtils.ts:81-97 and desktop/src/features/channels/ui/MembersSidebar.tsx:108-127. Production consumers in the searched desktop/src and desktop/tests scopes are useClassifiedMembers.ts:74, ChannelManagementSheet.tsx:139, and MembersSidebar.tsx:190-192. Full pubkeys remain React/virtualizer keys at MembersSidebar.tsx:682-689,815-823, so reordering cannot retarget member actions. DOM/virtualized reading order follows the sorted array, with existing search focus unchanged (MembersSidebar.tsx:695-703,758-823). The E2E inserts V5 before V24 but requires full-npub order V24 before V5 after both rows render (desktop/tests/e2e/channels.spec.ts:4277-4314).
Author action: none.
Verification owner: CI/branch protection owns the still-running exact-head Desktop Domain / Desktop Core aggregate. This is an external merge gate, not author rework. Optional installed-native/VoiceOver observation remains reviewer/release-owned.
Validation at matching clean exact head:
- Dedicated detached checkouts matched the remote head and reviewed base;
git diff --checkpassed and trees remained clean. - Full Desktop package:
just desktop-check,just desktop-typecheck,just desktop-test(6,476/6,476), andjust desktop-build— PASS. - Full repository
just ci— PASS, rc 0. - Full Desktop smoke project — 90/90 PASS, including the visible-order regression.
- Independent focused built Playwright journey — 1/1 PASS.
- Two independent production-seam mutations restoring compact-label ordering each made the focused regression fail on all three attempts at the intended assertion (
v24Position=4,v5Position=3); exact head was restored clean. - Live exact-head CI: all four smoke shards, both integration shards, macOS/Windows builds, DCO, Semgrep, and zizmor passed; Desktop Core remains in progress with no failed check.
Manual/native evidence: no installed Tauri/VoiceOver observation. No keyboard handler, focus behavior, role, label, semantics, layout, or styling changed; browser-rendered order is directly exercised.
Residual risk: exact-head Desktop Core has not reached a terminal state, so normal branch protection must keep owning that merge gate. Native assistive-tech observation was not run, but the bounded comparator-only change, full repository/package validation, full and focused browser journeys, causal mutation evidence, and platform builds make this non-blocking.
— :bot: Jude’s code review agent
42a2c32 to
22e52fc
Compare
The roster ordered members by the compact `npub1abcd…wxyz` display label — a recognition aid that hides almost the entire key — so two distinct identities sharing an npub head and checksum tail collapsed into one roster position. Sort unnamed members by their full canonical npub instead, and break every collation-equal name surface — duplicate authored names, matching labels, invalid keys — by the full identity key, so incoming membership-event order is never the tie policy. The compact label stays display-only and unchanged. - memberUtils: new shared `compareMemberNames` owns the name stage — surface (authored name, else full canonical npub), then full identity key for ties. `compareMembersByRole` delegates to it with its current-user and role precedence unchanged. - MembersSidebar: the add-member modal comparator keeps its intentionally coarser owner/admin rank and current-user-first priority and delegates the same name stage. - e2e: the virtualization spec's ordering comment follows the new rule (comment-only; the spec already resolves members from the rendered window). - unit tests: five cases bind the production comparator — full-npub vs compact-label disagreement, duplicate-name identity-key tie-break, role precedence, current-user precedence, invalid-key determinism. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
The full-npub ordering change bound its five unit cases to compareMembersByRole only. The visible members-sidebar roster sorts through compareMembersForModal, whose delegation to the shared name stage had no regression: reverting just that delegation back to formatMemberName(...).localeCompare kept every existing test green. Add one focused e2e case on the existing small-sidebar workflow seams: the two unnamed fixtures from the memberUtils unit pair join the three-member "random" roster — both visible in the initial virtual window, no 500-member roster needed — arriving in the opposite of the expected order, and the rendered roster must place V24 before V5. The full canonical npubs disagree with the compact labels for exactly this pair, so a display-label order fails the assertion. Falsified against ec7a9cf by reverting only the modal delegation (byte-identical to parent a1ffa77): the new case failed at the order assertion while both existing members-sidebar workflows stayed green; restored production passes all three. Production files are untouched and byte-identical to ec7a9cf. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
22e52fc to
f062679
Compare
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
No blocking findings at exact head f06267940a90fc5eee0e06c1bdeebc3494d3d654 against base 82656ffea080cc28cec9163ebbf7d1c6be327e43.
The changed head preserves the reviewed implementation and adds coverage at the production sidebar seam. Full canonical npub determines unnamed-member ordering while compact npub remains display-only; current-user and role precedence are unchanged. The sidebar continues to key rendered rows and actions by full pubkey (desktop/src/features/channels/lib/memberUtils.ts:25-96, desktop/src/features/channels/ui/MembersSidebar.tsx:108-127,815-823, desktop/src/features/channels/lib/useClassifiedMembers.ts:65-83).
Validation on clean matching HEAD:
just desktop-test: 6,488/6,488 passedjust desktop-check: passed (2,662 files; existing warnings only)just desktop-typecheck: passedjust desktop-build: passed- focused rendered Playwright regression: 1/1 passed
- full
channels.spec.ts: command passed; 89 direct passes and one unrelated retry-pass - causal mutation restoring compact-label ordering failed the new assertion on every attempt, then passed after restoration
git diff --check/ clean-tree checks: passed
CI is successful/skipped except Desktop Domain / Desktop Core, which remains in progress with no observed failure. Branch protection owns its terminal result.
Author action: none.
Residual risk: installed-native assistive-technology observation was not run. No semantics, focus, input, label, or layout code changed, so this remains a reviewer/release confidence gap rather than author rework. Any new head invalidates this approval.
Desktop Core job 102588378475 (run 34387730254, head f062679) hit its explicit 45m job ceiling while the compiled-flag verification step's third full-suite run was still printing passing tests — after five clean recompiles and two complete green suites (3173 passed, 0 failed each), with no compiler, test, or network errors in the step. desktop-tauri-test-compiled-flags intentionally recompiles the workspace for each BUZZ_BUILD_* state (build.rs rerun-if-env-changed) and runs the full test suite under all three compile states; that complete recipe needs ~16m on top of the ~30m build/lint/test prefix, so 45m is structurally at the limit for an all-green run. Raise only the desktop-core job timeout 45m -> 60m — the smallest change that fits the demonstrated complete workload with headroom. Every command, config, test, and assertion, and every other job timeout is unchanged; no retries added, no flag coverage removed. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
|
Desktop Core budget repair (785725f) Previous head
That was a budget kill, not a test failure: the step log shows five clean
This change raises only the |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed exact head 785725faac2ed99c310afb45896a761d0fd186d3 against base 82656ffea080cc28cec9163ebbf7d1c6be327e43.
No author-actionable defects found.
Ordering behavior
- The sidebar now compares unnamed members by the full canonical npub while retaining compact npubs only for display (
desktop/src/features/channels/ui/MembersSidebar.tsx:113-126,desktop/src/features/channels/lib/memberUtils.ts:51-79). - Full Desktop unit, typecheck, check, and build commands passed on the exact head.
- The rendered Playwright regression at
desktop/tests/e2e/channels.spec.ts:4266-4314passed and exercises the production sidebar with two npubs whose compact labels sort differently from their full canonical values. - A causal mutation restoring the old compact-label comparator made that regression fail on all three attempts (
Expected < 3; Received 4), then restoring the candidate made it pass. This binds the test to the production ordering seam rather than only to a helper.
CI timeout delta
The only change since previously reviewed head f06267940a90fc5eee0e06c1bdeebc3494d3d654 is .github/workflows/_ci-desktop.yml:27-30: the Desktop Core ceiling rises from 45 to 60 minutes with rationale. Commands, assertions, retries, coverage, and all other timeouts remain unchanged.
That increase is supported by observed runtime rather than guesswork:
- Prior job
102588378475was canceled specifically for exceeding45m0s; preceding steps passed, and compiled-flag verification had completed two3173 passed; 0 failedsuites while its third full-suite state was still producing passing tests. - Exact-head job
102619692584completed successfully in 36m10s, including the 15m10s compiled-flag step and its clean/capability/demo-boundary assertions. actionlint .github/workflows/_ci-desktop.ymlandgit diff --checkpassed on the pinned tree.
Gates and residual confidence
All exact-head CI checks are terminal success/skipped/neutral, including all four Desktop Smoke E2E shards, Desktop Core, macOS build, Windows build, Rust, integration, security, and DCO gates. The authenticated reviewer is jedwards27; PR author is loganj, so this is not self-review.
A local attempt to run the entire unsharded smoke suite exceeded the reviewer tool's 20-minute ceiling after 688 consecutive passes, and no native Tauri capture was run. Those are confidence gaps, not defects: the changed rendered journey passed both targeted causal validation and exact-head sharded CI, and this patch changes deterministic row order without changing native IPC, controls, accessibility semantics, or lifecycle behavior.
Author action: none.
Verification owner: none; exact-head required gates are green.
Residual risk: a genuinely wedged Desktop Core job may now consume 15 additional runner-minutes before cancellation. The cost is bounded and does not weaken test coverage.
Any new head invalidates this approval pending delta review.
…stody * origin/main: fix(desktop): order unnamed roster members by full canonical npub (block#7503) fix(mobile): standardize public-key identity display on npub (block#7493) fix(desktop): npub identity controls across profile, agents, and workflows (block#7489) fix(desktop): npub identity displays for mention, member, and workflow surfaces (block#7495) fix(desktop): shared npub identity foundation (canonicalNpub, PubKey gate, strict parser) (block#7488) fix(mobile): render push notification sender identity as npub (block#7494) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
🤖
Summary
Related issue
Based on #7495 (introduced the abbreviated npub labels this follows up on). The original five presentation PRs remain independently reviewable. No closer duplicate found.
Testing
Buzz provenance: channel 1f0e4a3d-7e01-4efe-bb16-843b357f85c9 / task 340c3de9b27dbedb8453c0c7652220f9080d30fcc70a7c4f6e27fdd4fa378056