Skip to content

fix(settings): make popup components respect UI zoom scaling#450

Open
a981008 wants to merge 2 commits into
NanmiCoder:mainfrom
a981008:main
Open

fix(settings): make popup components respect UI zoom scaling#450
a981008 wants to merge 2 commits into
NanmiCoder:mainfrom
a981008:main

Conversation

@a981008
Copy link
Copy Markdown
Contributor

@a981008 a981008 commented May 14, 2026

Modal, Toast, and dropdown menus were rendered to document.body via createPortal, bypassing the CSS zoom applied to .app-shell. Now all overlay components portal to #app-overlay-root inside AppShell so they inherit the zoom transform.

Summary

Feature Quality Contract

  • Changed surface:
  • Tests added or updated:
  • Coverage evidence:
  • E2E / live-model evidence:
  • Known risk / rollback:

Verification

  • I ran the relevant local checks, or explained why they do not apply.
  • I added or updated same-area tests for every production behavior change.
  • I ran bun run verify for code changes, including the coverage gate.
  • New or changed executable production lines meet the changed-line coverage threshold, or the blocker/maintainer override is documented.
  • I attached or summarized the quality report path, JUnit/log artifact path, and pass/fail/skip counts.
  • I ran E2E/live smoke for cross-boundary, provider/runtime, desktop chat, agent-loop, native, or release changes, or documented the blocker.

Risk

  • This PR does not touch CLI core paths, or it has maintainer approval for allow-cli-core-change.
  • Production code changes include matching tests, or have maintainer approval for allow-missing-tests.
  • Coverage baseline/threshold changes have maintainer approval for allow-coverage-baseline-change.
  • Quarantined tests still have owners, exit criteria, and unexpired review windows.
  • Provider/runtime changes were covered by mock contract tests, and live smoke was run or explicitly deferred.

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

Modal, Toast, and dropdown menus were rendered to document.body via
createPortal, bypassing the CSS zoom applied to .app-shell. Now all
overlay components portal to #app-overlay-root inside AppShell so they
inherit the zoom transform.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 14, 2026
@github-actions
Copy link
Copy Markdown

PR quality triage

Changed areas: area:desktop

CLI core policy: No CLI-core policy block detected.

Missing-test policy: Blocked by policy until a maintainer applies allow-missing-tests or matching tests are added.

Coverage baseline policy: No coverage-baseline policy block detected.

CLI core files:

  • none

Coverage policy files:

  • none

Expected checks:

  • change-policy
  • desktop-checks
  • desktop-native-checks
  • coverage-checks

Test coverage signals:

  • BLOCKING unless allow-missing-tests is applied: Desktop product files changed without a desktop test file in the PR.

Risk notes:

  • No special risk notes from changed paths.

Hard merge gates still come from GitHub Actions, not AI review.

Dosu handoff: Dosu can be used as the AI reviewer for risk explanation, missing-test prompts, and maintainer Q&A. If it does not comment automatically from the PR template, ask:

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

@a981008 a981008 closed this May 14, 2026
@dosubot
Copy link
Copy Markdown

dosubot Bot commented May 14, 2026

Here's my review of PR #450 across the requested dimensions:

Changed-Area Risk: LOW

The change is well-scoped — it introduces a centralized #app-overlay-root container in AppShell and updates 5 components (Modal, Toast, DirectoryPicker, PermissionModeSelector, RepositoryLaunchControls) to portal there instead of document.body. The fallback to document.body in getOverlayRoot() is a nice safety net. No complex logic, no state changes, no async operations.

Missing Tests: ⚠️ Concern

No tests were added or updated. Specific gaps:

  • No unit tests for the new overlayRoot.tsx utility (getOverlayRoot(), createOverlayPortal())
  • Existing component tests for Modal, DirectoryPicker, PermissionModeSelector, and RepositoryLaunchControls were not updated to verify the new portal target
  • Toast has no test file at all

Recommendation: At minimum, add tests for overlayRoot.tsx verifying the DOM query, caching behavior, and document.body fallback. Updating component tests to assert createPortal is called with the correct container would strengthen confidence.

Docs Impact: Minor

Architecture documentation and design system specs reference modals/overlays. A brief note in the component guidelines about using getOverlayRoot() (instead of document.body) for future portal components would help prevent regression of this pattern.

Desktop Startup Risk: Negligible

The only addition to the startup path is a static <div id="app-overlay-root" /> in AppShell's render tree — no async work, no additional queries, no effect on the bootstrap sequence.

CLI Core Impact: None

All changes are confined to desktop/src/. The desktop UI layer is architecturally separated from CLI core (src/cli/) via an HTTP API abstraction. No CLI paths are touched.


Summary: Low-risk, well-isolated UI fix. The main gap is missing test coverage for the new utility and updated portal targets. The Feature Quality Contract sections (tests added, coverage evidence, E2E evidence) appear unfilled, which aligns with this finding.

To reply, just mention @dosu.


Share context across your team and agents. Try Dosu.

Leave Feedback Learn about cc-haha with Dosu Add Dosu to your team

- Use mousedown with button check (e.button !== 0) to prevent menu closing on right-click
- Add onClick stopPropagation on all context menu divs to prevent event bubbling
- Apply uiZoom scaling correction to context menu positioning across Sidebar, TabBar, and WorkspacePanel

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@a981008 a981008 reopened this May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:desktop needs-maintainer-approval size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant