Skip to content

fix(desktop): keep window controls out of the compact sidebar - #11686

Closed
shivamhwp wants to merge 1 commit into
mainfrom
t3code/28792c27
Closed

fix(desktop): keep window controls out of the compact sidebar#11686
shivamhwp wants to merge 1 commit into
mainfrom
t3code/28792c27

Conversation

@shivamhwp

@shivamhwp shivamhwp commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

macOS window buttons spill past the compact sidebar when opening Settings. Compact mode now replaces them with the sidebar toggle inside the sidebar. Expanding restores the native buttons and their titlebar spacing, on both chat and Settings.

The desktop bridge method is optional for older shells. Browser, Windows/Linux desktop, and mobile layouts keep their existing controls. The toggle keeps its contrasting color over sidebar artwork.

Validation: 10 focused desktop IPC tests pass; web, desktop, and contracts typechecks pass; targeted lint passes with one existing set-state-in-effect warning. Tested the real client with an isolated database in Chromium using a simulated macOS bridge: compact/expanded Settings, chat/Settings navigation, keyboard toggling, fullscreen transitions, and a narrow viewport, with both sidebar implementations. Regular browser compact/expanded Settings also passed. Native macOS button rendering still needs a macOS check.

Before, reported on macOS After, Chromium with macOS bridge simulation
Before After

Model: GPT-6. Harness: Codex.

Summary by CodeRabbit

  • New Features

    • Added support for showing and hiding native macOS window buttons based on sidebar state.
    • Improved compact sidebar layout and spacing, including fullscreen behavior.
    • Added desktop bridge support for controlling native window button visibility.
  • Bug Fixes

    • Prevented window button updates from affecting unsupported platforms or unrelated windows.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 14, 2026
Comment thread apps/web/src/components/AppSidebarLayout.tsx
@macroscopeapp

macroscopeapp Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at f29c4c5

Macroscope's review found this PR approvable — This is a narrowly scoped macOS desktop UI fix with an additive, sender-scoped IPC bridge and tests for platform and window guards; the existing compact-sidebar setting remains default-off. An unresolved Medium finding still identifies incorrect control positioning after the sidebar expands.

Notes:

  • This verdict was updated automatically after the outstanding correctness findings were resolved. Macroscope did not re-review the code.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.6 KiB +12 B (+0.1%) 15.1 KiB
Codex Thread snapshot wire 7.0 KiB 7.1 KiB +7 B (+0.1%) 7.3 KiB
Codex Live turn WebSocket wire 6.5 KiB 6.5 KiB +5 B (+0.1%) 7.8 KiB
Codex Live turn WebSocket decoded 57.0 KiB 57.0 KiB 0 B (0.0%) 66.4 KiB
Codex Live turn messages 8 8 0 (0.0%) 21
Claude Total thread wire 13.6 KiB 13.6 KiB −29 B (−0.2%) 15.1 KiB
Claude Thread snapshot wire 7.1 KiB 7.1 KiB +1 B (+0.0%) 7.3 KiB
Claude Live turn WebSocket wire 6.5 KiB 6.5 KiB −30 B (−0.4%) 7.8 KiB
Claude Live turn WebSocket decoded 57.8 KiB 57.8 KiB −44 B (−0.1%) 66.4 KiB
Claude Live turn messages 9 8 −1 (−11.1%) 21

Baseline: 1bbca0e · PR result: f29c4c5 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c723fb7a-08a3-4fbe-875d-eba511c93cbd

📥 Commits

Reviewing files that changed from the base of the PR and between 1bbca0e and f29c4c5.

📒 Files selected for processing (7)
  • apps/desktop/src/ipc/DesktopIpcHandlers.ts
  • apps/desktop/src/ipc/channels.ts
  • apps/desktop/src/ipc/methods/window.test.ts
  • apps/desktop/src/ipc/methods/window.ts
  • apps/desktop/src/preload.ts
  • apps/web/src/components/AppSidebarLayout.tsx
  • packages/contracts/src/ipc.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a macOS-only IPC method for native window-button visibility. The sidebar uses this method during compact states and updates workspace-control layout classes. Tests cover supported and unsupported platforms, sender validation, and missing windows.

Changes

Native Window Button Visibility

Layer / File(s) Summary
IPC contract and platform behavior
apps/desktop/src/ipc/channels.ts, packages/contracts/src/ipc.ts, apps/desktop/src/ipc/methods/window.ts, apps/desktop/src/ipc/methods/window.test.ts
Adds the IPC channel and optional bridge method. The handler acts only on macOS, validates the sender, and updates the matching window. Tests cover hide, restore, unsupported platforms, mismatched senders, and missing windows.
Desktop IPC exposure
apps/desktop/src/ipc/DesktopIpcHandlers.ts, apps/desktop/src/preload.ts
Registers the handler and exposes setWindowButtonVisibility(visible) through desktopBridge.
Sidebar visibility and layout
apps/web/src/components/AppSidebarLayout.tsx
Detects the desktop capability, hides native buttons while the sidebar is compact unless fullscreen, restores them on cleanup, and updates sidebar control placement and workspace inset classes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SidebarControl
  participant desktopBridge
  participant setWindowButtonVisibility
  participant ElectronWindow
  SidebarControl->>desktopBridge: setWindowButtonVisibility(visible)
  desktopBridge->>setWindowButtonVisibility: invoke IPC channel
  setWindowButtonVisibility->>ElectronWindow: validate sender and platform
  setWindowButtonVisibility->>ElectronWindow: set native button visibility
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to f29c4

The desktop IPC and sidebar integration have no verified merge-blocking issue.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the desktop fix that prevents window controls from overlapping the compact sidebar.
Description check ✅ Passed The description explains the change, motivation, affected platforms, UI behavior, validation results, and remaining macOS verification. It does not reproduce the template headings or checklist, but it…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/28792c27

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #11685 (merged) — compact sidebar was reverted, so this change no longer applies.

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants