Skip to content

fix(desktop): match expanded thread margins to docked sidebar - #7169

Closed
loganj wants to merge 2 commits into
mainfrom
fix/thread-overlay-margins
Closed

loganj wants to merge 2 commits into
mainfrom
fix/thread-overlay-margins

Conversation

@loganj

@loganj loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

🤖

Summary

Expanding a Thread panel currently leaves large empty margins around messages and the reply box. This change lets the content fill the expanded panel using the same edge spacing as the docked sidebar, without changing the panel’s outer size or position.

The loading placeholders use the same spacing too. The docked layout, resize behavior, and controls are unchanged. In code, the expanded overlay is focus and the docked sidebar is split.

Related issue

Related existing draft: #4007 — remove the reading-width cap in expanded threads, which addresses the same visible problem. This patch also removes the unused width-constraint plumbing.

Original request and before/after discussion

Testing

Desktop unit tests, checks/typechecking, and seven targeted browser checks passed against this production patch. Temporary geometry probes reproduced the original bug and verified matching margins at 1280, 1720, and 2560px, including repeated runs; these probes are retained as external evidence, not added to the test suite. Existing tests are unchanged. Resizing and switching back preserve the typed draft and saved sidebar width. Desktop and web builds, native-backend checks/tests, and all 2,019 mobile tests also passed.

Two full local CI attempts stopped in existing agent-process timing tests (keepalive_resets_idle_past_deadline and idle_resets_on_stdout_activity); both passed three isolated reruns each against unchanged Rust sources. Neither full local CI attempt was green. Remote CI and all reported checks passed for head 28a5f5ea1a5f778dd5b3102542a9752866c102f7.

To check visually: open a thread, click Expand thread, and compare the message and reply-box margins with Show thread beside channel.

Before and after

Matched captures at 1720×1000, with the same messages, light theme, and 800px docked width. These show production React components in Chromium with a mock desktop bridge—not a native-app or live-conversation capture.

Expanded — before: the message rows and reply box have 281.5px and 289.5px of space per side.

Expanded thread before: large message and reply-box margins

Expanded — after: those insets are 12px and 20px, matching the docked sidebar. The expanded panel’s bounds are unchanged.

Expanded thread after: content fills the panel with docked margins

Docked — before and after: the captures are pixel-identical; this single image represents both.

Docked thread: unchanged before and after

Signed-off-by: Logan Johnson <loganj@squareup.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 114dbf745f1fe0bdbecb70e07a07f03c23687bdc...28a5f5ea1a5f778dd5b3102542a9752866c102f7.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 28a5f5ea1a5f778dd5b3102542a9752866c102f7 to authorize a new review.
Any previous review applies only to its recorded range.

@loganj

loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

@buzz-security-review e3baba7

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026
Signed-off-by: Logan Johnson <loganj@squareup.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026
@loganj

loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

@buzz-security-review 28a5f5e

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026
@loganj
loganj marked this pull request as ready for review September 1, 2026 15:46
@loganj
loganj requested a review from a team as a code owner September 1, 2026 15:46
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026

@jedwards27 jedwards27 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.

Reviewed exact head 28a5f5ea1a5f778dd5b3102542a9752866c102f7 over base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be.

The runtime layout change is appropriately narrow: it removes only the focus-only centered/max-width wrappers from the live panel and loading skeleton while preserving the shared message/composer gutters. I found no defect in panel bounds, focus/keyboard behavior, scroll state, resize behavior, identity/authorization, persistence, relay/Tauri contracts, or release wiring. Exact-head Desktop, smoke E2E, relay/integration, Windows/macOS build, Semgrep, zizmor, and DCO checks are green; local exact-head validation also passed just desktop-check, just desktop-typecheck, the full just desktop-test (5,843/5,843), just desktop-build, and the full thread-focus-mode.spec.ts smoke file (2/2).

One blocking regression-coverage defect remains:

  • 28a5f5ea1 deletes desktop/tests/e2e/thread-focus-margins.spec.ts and its Playwright registration after e3baba744 added them. That test exercised the production MessageThreadPanel at 1280/1720/2560px and asserted the contract this bug fix changes: focus message/composer insets match split mode, no horizontal overflow, and unchanged drawer geometry. Searches of the remaining focus-mode, ultrawide, and desktop tests found interaction coverage but no remaining gutter/column-width assertion. Restoring the old 880px cap would therefore pass the shipped suite. This violates the repository's falsifiable production-seam regression standard in TESTING.md:25-31.

Please retain a deterministic production-panel regression at normal and wide viewports (the deleted E2E is the right seam), and mutation-prove that restoring the old constrained-column wrapper/cap makes it fail for the intended inset mismatch. Screenshots and temporary external probes are useful evidence, but they cannot prevent this exact regression from returning.

Native Tauri rendering and OS accessibility-tree observation were not independently run; that is a reviewer/release confidence gap, not additional author rework.

@jedwards27 jedwards27 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.

Review verdict: request changes

Reviewed base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be through exact live head 28a5f5ea1a5f778dd5b3102542a9752866c102f7, integrating both assigned lanes. Runtime/source behavior looks correct, but this bug fix removes its only biting production-seam regression.

The shipped tree cannot detect restoration of the focus-column cap

An earlier commit in this PR added desktop/tests/e2e/thread-focus-margins.spec.ts, exercising the production MessageThreadPanel across normal and ultrawide viewports, comparing split/focus message and composer insets, overflow, and drawer geometry. The exact head deletes that test and its Playwright registration. Scoped review of the remaining focus-mode, ultrawide, and component tests found interaction coverage but no assertion over the changed gutter/column-width contract. Reintroducing the old 880px cap would therefore leave the checked-in suite green.

That violates the repository's falsifiable production-seam regression requirement for bug fixes (TESTING.md:25-31). External/manual geometry probes are useful evidence, but they are not durable protection.

Author action: retain or replace the deleted deterministic production-panel regression, covering focus message and composer gutters against split mode without horizontal overflow at normal and wide viewports. Include mutation evidence that restoring the old constrained wrapper/cap fails the test.

Verification

The product/UI lane found the actual five-file presentation delta correct and passed the full focus-mode E2E 2/2. The systems lane passed check, typecheck, build, and the full Desktop suite 5,843/5,843. Native visual observation remains a confidence gap, not additional author work.

— :bot: Jude’s code review agent

@loganj

loganj commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Closing per discussion - wrong solution to the problem, and this UI is overloaded.

@loganj loganj closed this Sep 2, 2026
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.

2 participants