fix(desktop): keep sidebar surfaces inset beside relay rail - #6850
fix(desktop): keep sidebar surfaces inset beside relay rail#6850matt2e wants to merge 3 commits into
Conversation
Apply the multi-community overlap only to the pinned header so footer action cards and draggable channel rows retain their normal sidebar inset. Add a smoke regression for the update card, channel row, and drag overlay. Signed-off-by: Codex <noreply@openai.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 52621c09bea503f4d5860030dfabfaf9ade71bfa..2e1eae7319fc480af93e43cf51e8e6529cdfc4a5 (exact live head)
Risk: medium — user-visible shared sidebar geometry across multi-community, footer cards, rows, and drag overlays.
No blocking defect found. The change removes the -11px/+11px expansion from the full sidebar scroll anchor and confines overlap to the pinned header via a local md:-ml-2 class (desktop/src/features/sidebar/ui/AppSidebar.tsx:457-464, AppSidebarPinnedHeader.tsx:54-73). That restores the ordinary sidebar coordinate box for channel rows, unread surfaces, update/connection cards, profile controls, and DnD sources without changing shared tokens, semantics, focus order, keyboard handling, or macOS top-chrome ownership. The multi-community predicate matches the rail boundary, and the md: guard preserves narrow/off-canvas behavior.
Regression quality: the new smoke test directly measures the update card (+8px), channel row (+11px), and pointer drag overlay (>=10px) against the sidebar. A causal mutation restoring the old ancestor expansion failed the test with the expected 11px delta (update inset -3 instead of 8), then passed again after restoring the exact head.
Validation at clean exact head:
git diff --check 52621c09...HEAD— pass.pnpm -C desktop build:e2eplus focused regression--repeat-each=3— 3/3 pass.just desktop-check(existing warnings only),just desktop-typecheck,just desktop-test(5,508/5,508), andjust desktop-build— pass.- GitHub exact-head: all four Desktop Smoke E2E shards, macOS build, both integration shards/aggregate, relay E2E, DCO, and release-candidate checks pass. Desktop Core remains in progress; CI owns that required gate.
Author action: none.
Verification owner: CI for Desktop Core; reviewer/tooling for native-visible macOS/AX evidence.
Residual risk: no native Tauri screenshot/MP4/AX run, and manual-update, relay-connection, theme/zoom, and narrow matrices were not independently instantiated. These are low-risk confidence gaps rather than code defects: the variants share the restored unshifted container, while the patch changes no theme/semantic values and retains the existing responsive gate.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Approved at exact head 2e1eae7319fc480af93e43cf51e8e6529cdfc4a5 (base 52621c09bea503f4d5860030dfabfaf9ade71bfa). Both assigned lanes found no author-actionable code, product, accessibility, or integration defect.
The geometry fix is narrowly owned: the full sidebar scroll anchor no longer carries the multi-community negative margin/extra width, while only the pinned header receives the responsive conditional overlap. Channel rows, footer update/connection surfaces, and drag source/overlay geometry therefore remain inside the sidebar boundary. Single-community, narrow/off-canvas, macOS traffic-light, focus/keyboard, theme-token, and stacking contracts are unchanged.
Exact-head evidence:
git diff --checkpassed.- Desktop check, typecheck, build, and unit suite passed (5,508/5,508).
- New focused rail-inset regression passed 3/3, observing update-card, channel-row, and drag-overlay inset geometry.
- Causal mutation restoring the base whole-anchor shift failed with the expected 11px delta; restoring the candidate passed cleanly.
- GitHub macOS build, all four Desktop smoke shards, integration lanes, release candidate, relay, and DCO are green/skipped as expected. Desktop Core remains in progress at submission and remains a required CI-owned merge gate.
Confidence gaps: native Tauri screenshot/MP4/accessibility observation and independent manual-update/relay-connection card variants were not completed. Those are reviewer/tooling evidence gaps, not demonstrated defects; the affected surfaces share the corrected unshifted container boundary.
Author action: none. Verification owner: Desktop Core CI for the remaining exact-head gate; reviewer/native tooling for optional visible/AT evidence. This approval does not bypass branch protection. Any new head invalidates it.
Signed-off-by: Codex <noreply@openai.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head f5506a093c257a064c63f3594d0f4bd70d216222 over base 52621c09bea503f4d5860030dfabfaf9ade71bfa.
No unresolved author-actionable defect found.
Integrated review
The changed-head delta from the expired approval (2e1eae7..f5506a0) is test-only: it strengthens desktop/tests/e2e/sidebar-community-rail-insets.spec.ts by fixing initialization order, seeding a real custom group, locating its droppable body, dragging #agents over it, and asserting the live ring-2 target state. Production behavior is unchanged from the previously reviewed head.
Geometry ownership remains appropriately narrow. AppSidebar.tsx:457-484 restores the shared scroll/footer owner to normal width and applies md:-ml-2 only to AppSidebarPinnedHeader; rows, footer cards, custom-section droppables, and the DndContext remain unshifted (AppSidebar.tsx:481-817). The portal DragOverlay continues to track pointer coordinates independently (SidebarDnd.tsx:260-280). No shared primitive, global CSS, semantic, focus-order, keyboard, theme, or platform code changed. The existing md: and communities.length > 1 guards preserve narrow and single-relay behavior.
Exact-head evidence
git diff --check 52621c09b...f5506a093: pass.- New focused regression with
--repeat-each=3: 3/3 pass in both independent lanes. - Causal mutation checks: restoring the old whole-container translation made the update-card inset assertion fail (
received -3px, expected8px); an isolated-11pxcustom-section mutation made the drop-target inset assertion fail (received -8px, expected3px). Both worktrees were restored clean and reran green. just desktop-check,desktop-typecheck,desktop-test, anddesktop-build: pass; 5,508/5,508 unit tests passed.- Five PR screenshots support corrected multi-relay card/drop-target insets and unchanged single-relay layout.
- Exact-head CI: macOS Desktop Build, both Desktop E2E Integration shards, Desktop E2E Relay, Web, DCO, and release-candidate checks passed at review time.
CI / residual confidence
Desktop Smoke E2E (2) failed on mentions.spec.ts:1002 after three attempts, while 337 tests passed and two unrelated tests were classified flaky. The PR changes only sidebar geometry and its dedicated regression; it does not touch mention composition. This is therefore an unrelated required-gate failure, not an author-actionable PR defect. Author action: none. Verification owner: CI/maintainers for rerun or triage. Other smoke shards and Desktop Core were still running at final freshness.
No native Tauri/AX observation or explicit zoom/theme/narrow/focus matrix was performed. Those are reviewer/tooling confidence gaps, not author defects; structural scope and focused browser evidence bound the risk.
…pick
Desktop Smoke E2E (2) failed on mentions.spec.ts ("typing a mention
before existing text does not interleave spaces"): picking a suggestion
mid-draft and typing immediately could swallow the draft's space
("hello @bob abcworld"). Chromium sometimes rewrites the whole
whitespace run around the caret as replace(" " -> "\u00A0a"), and the
settling-time text-input redirect only recognized a caret or a selected
trailing space, so the rewrite fell through to the destructive default
replacement.
Teach the redirect to recognize a space-run rewrite (space or NBSP) and
insert only the typed remainder after the mention's trailing space. The
race pre-dates this branch (reproduces on main at ~5-25% per local run);
300+ consecutive passes after the fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES at exact head 05302ca879197cbe7ab1abcdffc51772f7071fe8 over base 52621c09bea503f4d5860030dfabfaf9ade71bfa.
[P2] Split or fix the unrelated mention-composer change; its claimed race still reproduces
The changed-head delta from the last reviewed head (f5506a093..05302ca87) modifies only desktop/src/features/messages/lib/mentionHighlightExtension.ts and its helper tests. It does not change the sidebar geometry or sidebar regression. This introduces independent user-visible ProseMirror/Chromium text-input behavior into a PR whose title, screenshots, risk analysis, and rollback contract concern relay-rail sidebar geometry. Shipping the two together means rolling back the sidebar fix also removes a composer fix that needs a different review boundary.
More importantly, the new production behavior does not reliably eliminate its named failure. At exact head, the existing browser journey in desktop/tests/e2e/mentions.spec.ts:1002-1027 was run with --repeat-each=10; one valid execution failed with:
- expected:
hello @bob abc world - received:
hello @bob abcworld
Two separate repetitions failed because the preview server refused connections and are excluded from this product finding; seven passed. A sequential --repeat-each=20 --workers=1 run passed 20/20, establishing that the symptom is timing-sensitive rather than a deterministic setup error—which is the boundary this commit claims to repair. The helper suite passes and its causal mutation is caught, but a production mutation rebuilt into the browser app still passed the focused E2E 20/20 in another cohort. The helper test models the desired payload; it does not prove Chromium reliably reaches the new branch.
Impact: immediately typing after selecting a mention in the middle of an existing draft can still swallow the separator and concatenate text with the following word, silently changing an unsent message.
Author action: remove 05302ca87 from this sidebar PR and land the composer fix separately, or explicitly expand the PR scope and make the browser race robust under CI-shaped concurrency with mutation-sensitive browser evidence. The smallest and safest action is the scope split.
Verified unaffected geometry
The sidebar production code and its E2E are byte-identical to f5506a093. At this exact head, the geometry regression passed 3/3, full Desktop unit tests passed 5,509/5,509, typecheck and E2E build passed, and the final worktrees were clean. The geometry correction itself remains sound; approval cannot carry forward over unrelated, failing production behavior.
Verification owner: author for split/fix; CI for remaining exact-head gates; reviewer/tooling for optional native Tauri/AX and theme/zoom/narrow/focus geometry confidence.
|
Test is flakey, fixing here instead: #6875 |
3 participants
Summary
Why drag-and-drop was affected
The multi-relay offset was applied to the shared scroll container, so it shifted draggable channel rows, the custom section
DroppableSectionBody, and the liveDragOverlay11px left along with footer notification cards. That put the section'sring-2hover highlight underneath the relay rail while dragging a channel into a group. Moving the offset toAppSidebarPinnedHeaderkeeps the header alignment without shifting any drop surface.Related issue
None found in open issues or pull requests. Reported in Buzz.
Testing
./bin/just ci./bin/pnpm -C desktop build:e2eUI evidence
Update card with multiple relays
The before image is a deterministic E2E reproduction of the pre-fix CSS. After the fix, the relay rail remains visible while the card returns to its normal inset.
Channel-to-group hover edge with multiple relays
These equal close crops show
#agentsactively held over theProjectsgroup, with the relay-rail boundary fixed at the same screen coordinate. They were captured from separate builds of the untouched parent commit and the PR head:52621c09b): the drop target begins at x=48 while the rail ends at x=56, placing 8px of the target and 10px including its outer ring behind the rail.f5506a093): the target begins at x=59 and its ring begins at x=57, so the complete rounded edge is visible beside the rail.Update card with one relay
The single-relay layout has no relay rail and retains its existing card inset.