Skip to content

feat(desktop): page zoom for embedded browser (#1333) - #1509

Merged
Astro-Han merged 3 commits into
devfrom
feat/1333-browser-page-zoom
Jul 10, 2026
Merged

feat(desktop): page zoom for embedded browser (#1333)#1509
Astro-Han merged 3 commits into
devfrom
feat/1333-browser-page-zoom

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

First shippable slice of #1333: conversation-scoped page zoom for the embedded browser (WebContentsView), independent of app shell zoom.

  • Overflow menu: Codex-style row Zoom + / percent / + / circular reset
  • Keyboard: Cmd/Ctrl +/-/0 while the embedded page is focused
  • Wheel / pinch: zoom-changed on the page WebContents (sync + step fallback)
  • Zoom lives on the per-conversation controller; navigation does not reset it; sessions do not share zoom
  • Bounds still use window zoom only — panel geometry is unchanged

Why

Users cannot enlarge page content without resizing the whole app or changing OS display scale. Existing Cmd/Ctrl +/-/0 only zooms the app shell, not the embedded page. This is the P1 first slice of the embedded-browser completion umbrella.

Related Issue

Closes part of #1333 (page zoom only). Settings/full CDP, safe preview, and annotations remain follow-up PRs.

Human Review Status

Pending

Review Focus

  1. Page zoom vs shell zoom isolation (setZoomFactor on the view’s WebContents only; computeViewBounds still uses window zoom).
  2. Menu row UX: keeps menu open while stepping; reset is the circular control (percent is display-only).
  3. Input routing: before-input-event on the page when focused; shell shortcuts unchanged when the page is not focused.
  4. zoom-changed re-entrancy guard (applyingPageZoom) so programmatic setZoomFactor does not double-step.

Risk Notes

  • Wheel/pinch behavior depends on Electron/Chromium zoom-changed semantics; both “factor already applied” and “direction request” paths are handled. macOS desktop walkthrough confirmed by maintainer.
  • Visible UI change in the browser overflow menu; no new dependencies, permissions, or data migration.
  • Conditional checklist: no release notes / credentials / deletion behavior changes.

How To Verify

packages/desktop-electron: bun test src/main/browser/logic.test.ts
  → 26 pass (page zoom clamp/step/reset + existing browser logic)

packages/desktop-electron: bun run typecheck
  → ok

packages/app: bun run typecheck
  → ok

packages/app: bun test src/i18n/parity.test.ts src/pages/session/browser
  → 25 pass

bun run dev:desktop (macOS)
  → maintainer confirmed: overflow Zoom row and page zoom look correct;
     shell chrome does not resize with page zoom

Screenshots or Recordings

Maintainer verified live in bun run dev:desktop on the PR branch (embedded browser overflow Zoom row + page zoom behavior).

image

Checklist

  • Type labelenhancement
  • Routing labelsapp, ui, platform, desktop
  • Priority labelP1
  • Human Review Status above is set to Pending, Approved by @<reviewer>, or Not required: <reason> (default is Pending; "not required" is restricted to bot-authored low-risk PRs).
  • I linked the related issue, or stated in Summary why there is no issue.
  • I described the review focus and any meaningful risks.
  • I replaced the example block in How To Verify with the real verification steps and the key result for each.
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope.
  • (conditional) I manually checked visible UI or copy changes when needed, with screenshots or recordings. Leave unticked only if no visible UI or copy changed.
  • (conditional) I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes. Leave unticked only if no platform/packaging surface was touched.
  • (conditional) I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant. Leave unticked only if none of those surfaces was touched.
  • I reviewed the final diff for unrelated changes and suspicious dependency changes.
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English.

Summary by CodeRabbit

  • New Features

    • Added embedded page zoom controls with zoom in, zoom out, reset, and current zoom percentage display.
    • Added keyboard shortcuts for adjusting page zoom.
    • Zoom level now persists while navigating within a browser session.
    • Added English and Chinese translations for zoom controls.
  • Bug Fixes

    • Prevented page zoom from exceeding supported limits or causing unintended menu behavior.
    • Removed conflicting browser zoom items from the desktop View menu.

Add conversation-scoped page zoom for the embedded WebContentsView,
independent of app shell zoom. Overflow menu gets a Codex-style
Zoom row (− / percent / + / reset); keyboard and wheel/pinch when the
page is focused use the same main-process path.
@Astro-Han Astro-Han added enhancement New feature or request P1 High priority app Application behavior and product flows ui Design system and user interface platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions desktop labels Jul 10, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes user-path files (packages/app/src/context/platform.tsx, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/session/browser/browser-panel.tsx, packages/desktop-electron/src/main/browser/controller.ts, packages/desktop-electron/src/main/browser/logic.test.ts, packages/desktop-electron/src/main/browser/logic.ts, packages/desktop-electron/src/main/ipc/browser.ts, packages/desktop-electron/src/preload/index.ts)).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review 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: Pro Plus

Run ID: 422eb20e-1ad4-49f6-8969-86f923fa968c

📥 Commits

Reviewing files that changed from the base of the PR and between 7e889a9 and 9cecb75.

📒 Files selected for processing (6)
  • packages/desktop-electron/src/main/browser/controller.ts
  • packages/desktop-electron/src/main/browser/logic.test.ts
  • packages/desktop-electron/src/main/browser/logic.ts
  • packages/desktop-electron/src/main/menu-labels.ts
  • packages/desktop-electron/src/main/menu-template.ts
  • packages/desktop-electron/src/main/menu.test.ts
💤 Files with no reviewable changes (3)
  • packages/desktop-electron/src/main/browser/logic.ts
  • packages/desktop-electron/src/main/menu-labels.ts
  • packages/desktop-electron/src/main/menu.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/desktop-electron/src/main/browser/controller.ts

📝 Walkthrough

Walkthrough

Adds embedded browser page zoom with clamped zoom math, per-conversation state, keyboard and menu controls, renderer-to-main IPC wiring, localized labels, and synchronization with Chromium WebContents.

Changes

Browser page zoom

Layer / File(s) Summary
Zoom state and math
packages/desktop-electron/src/main/browser/logic.ts, packages/desktop-electron/src/main/browser/logic.test.ts, packages/app/src/context/platform.tsx
Defines zoomFactor, zoom bounds, step/reset resolution, derived-state clamping, and corresponding tests.
Controller and IPC flow
packages/desktop-electron/src/main/browser/controller.ts, packages/desktop-electron/src/main/ipc/browser.ts, packages/desktop-electron/src/preload/index.ts, packages/app/src/context/platform.tsx
Applies zoom to embedded WebContents, handles keyboard and Chromium zoom events, preserves state across navigation, and exposes target-based zoom actions through IPC.
Browser zoom controls
packages/app/src/pages/session/browser/browser-panel.tsx, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts
Adds localized controls showing the current percentage with zoom-in, zoom-out, and reset actions.
Desktop menu cleanup
packages/desktop-electron/src/main/menu-labels.ts, packages/desktop-electron/src/main/menu-template.ts, packages/desktop-electron/src/main/menu.test.ts
Removes legacy page-zoom menu roles and updates the View-menu localization expectations.

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

Possibly related issues

Possibly related PRs

  • Astro-Han/pawwork#184 — Modifies the same desktop menu role-label plumbing used by the removed zoom roles.
  • Astro-Han/pawwork#1221 — Modifies the same BrowserViewController area where page-zoom state and teardown updates are added.

Sequence Diagram(s)

sequenceDiagram
  participant BrowserPanel
  participant PreloadAPI
  participant BrowserIPC
  participant BrowserViewController
  participant WebContents
  BrowserPanel->>PreloadAPI: Request zoom action
  PreloadAPI->>BrowserIPC: Invoke browser:zoom
  BrowserIPC->>BrowserViewController: Forward target and action
  BrowserViewController->>WebContents: Set zoom factor
  WebContents-->>BrowserViewController: Emit zoom-changed
  BrowserViewController-->>BrowserPanel: Publish zoomFactor state
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: embedded browser page zoom support.
Description check ✅ Passed The description matches the template and includes all required sections, verification, screenshots, and a completed checklist.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/1333-browser-page-zoom

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Keep conversation pageZoom as the only desired factor: step wheel/keyboard
from that value, reassert on navigate and display claim/attach so origin-
scoped Chromium zoom cannot desync the menu, drop dead format helper, and
allow Shift+= for zoom-in.
@Astro-Han

Copy link
Copy Markdown
Owner Author

Follow-up (Occam pass)

Pushed 5c37cf67f7 after Codex/Pi first-principles review:

  • Single truth: pageZoom only; zoom-changed only steps in/out from that value (no dual-source reconciliation with getZoomFactor).
  • Reassert: setZoomFactor(pageZoom) on did-navigate and on display attach/claim so origin-scoped Chromium zoom cannot leave the page at 100% while the menu still shows the old percent. Geometry RAF ticks do not reassert every frame.
  • Deleted: unused formatPageZoomPercent.
  • Keyboard: allow Shift so Shift+= → zoom in on US layouts.
  • Pinch: still not enabled. Electron visual pinch is a second scale (off by default); turning it on without owning it would lie in the menu percent. Wheel + menu + shortcuts remain the solid paths; true pinch is a follow-up if we want a honest owned path.

Contract residual: same-origin zoom is partition-scoped in Chromium. Reassert on show/navigate is the smallest fix without splitting the shared login partition.

Shell zoom is pinned to 1, so zoomIn/Out/resetZoom only stole
Cmd/Ctrl+/-/0 from the embedded browser before-input path. Page zoom
stays in the browser overflow menu.
@Astro-Han

Copy link
Copy Markdown
Owner Author

Contract + menu cleanup

Pushed after first-principles re-review:

  • Removed View menu zoomIn / zoomOut / resetZoom (macOS + Windows). Shell zoom is already pinned to 1; those roles only stole keyboard accelerators from the embedded page before-input-event path.
  • Page zoom surface remains the browser overflow menu ( / percent / + / reset) + focused-page shortcuts + wheel.
  • Deferred / residual (documented, not coded):
    • Trackpad pinch: Electron visual zoom is a second scale; left off so menu % stays honest.
    • Same-origin multi-session simultaneous zoom: Chromium HostZoomMap in the shared login partition; reassert on navigate/display covers sequential use. No partition split.

Please re-check on macOS: page focused → ⌘= / ⌘- / ⌘0 all step page zoom only.

@Astro-Han
Astro-Han merged commit e8299d6 into dev Jul 10, 2026
45 checks passed
@Astro-Han
Astro-Han deleted the feat/1333-browser-page-zoom branch July 10, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app Application behavior and product flows desktop enhancement New feature or request P1 High priority platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions ui Design system and user interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant