spec: per-window zoom level (#10115)#10498
spec: per-window zoom level (#10115)#10498lonexreb wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec defines a per-window zoom model, keeps the persisted setting as the default for new windows, and covers the main shortcuts, settings behavior, acceptance criteria, implementation pointers, and tests. No design-level security findings were identified for this local UI-state change.
Concerns
- Pinch/trackpad zoom is internally inconsistent: it is excluded as a non-goal but later left open to include in V1 if it is not already focused-window-scoped.
- Two coverage clarifications would make the spec easier to implement and verify: whether the "Apply to all open windows" control is required, and how tab moves between windows are tested.
Verdict
Found: 0 critical, 1 important, 2 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - Per-monitor adaptive zoom that auto-scales when a window moves between displays. | ||
| - Persisting per-window zoom across app restart (V1.5 candidate — see Open Questions). | ||
| - Changing the underlying zoom mechanism, range, or step size. | ||
| - Touch / trackpad pinch-zoom semantics (already focused-window-scoped where supported). |
There was a problem hiding this comment.
| - New windows open at the user's global zoom default (the value in Settings → Appearance → Zoom). | ||
| - All tabs and splits inside a single window share that window's zoom level. Switching tab or focused split inside the same window does not change the zoom. | ||
| - The Settings → Appearance → Zoom UI label is updated to disambiguate it as the default for new windows, not a global override. | ||
| - Optional one-shot "Apply to all open windows" button gives users a fast escape hatch when they want to re-sync everything. |
There was a problem hiding this comment.
💡 [SUGGESTION] "Optional" conflicts with B6/A7 treating the button as required; either remove "Optional" or explicitly mark the button as a non-blocking enhancement.
|
|
||
| ### B3. Tabs and splits share window zoom | ||
|
|
||
| All tabs and splits inside a window share that window's zoom-level value. Creating a new tab or split, switching the active tab, or moving focus between splits does not change the rendered zoom. Moving a tab to a different window adopts the destination window's zoom. |
There was a problem hiding this comment.
💡 [SUGGESTION] B3 defines moved tabs adopting the destination window's zoom, but the acceptance criteria/tests never cover tab moves; add an A/T item so implementers verify this edge case.
|
Round-1 fixes for oz-for-oss review: (1) resolved pinch/trackpad zoom — IS in V1, follows same per-window scoping as keyboard shortcuts; non-goal wording removed; A_pinch_per_window_scope and T_pinch_zoom_per_window added; (2) 'Apply to all open windows' button is REQUIRED in V1 — enabled only when ≥1 open window differs from global default; greyed out with tooltip otherwise; A_apply_to_all_button_resets_all_open added; (3) tab-move tests added — moved tab adopts destination window zoom (T_tab_move_adopts_dest_zoom); drag-out to new window uses global default, not source-window zoom (T_tab_move_to_new_window_uses_global_default). |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines in-memory per-window zoom with focused-window-scoped actions, default inheritance for new windows, settings label/UI changes, acceptance criteria, and tests. The supplemental security pass did not identify a new trust boundary or sensitive data handling concern.
Concerns
- The Settings/API surface says
appearance.zoom_levelsemantics are unchanged, but the behavior contract changes it from a live global zoom value to a default for new windows. Resolve this contradiction before implementation.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ## Settings / API surface | ||
|
|
||
| - `appearance.zoom_level` (existing): unchanged in schema, semantics, and storage. Now documented as "default for new windows". User-facing label updated to match. |
There was a problem hiding this comment.
| - `appearance.zoom_level` (existing): unchanged in schema, semantics, and storage. Now documented as "default for new windows". User-facing label updated to match. | |
| - `appearance.zoom_level` (existing): schema and storage unchanged. Runtime semantics change from the live global zoom value to the default used when creating new windows. User-facing label updated to match. |
…reb/warp into spec/10115-per-window-zoom # Conflicts: # specs/GH10115/SPEC.md
|
Addressed the 1 important concern from the latest oz-for-oss review:
|
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines per-window zoom state for keyboard, menu/palette, and pinch inputs while preserving appearance.zoom_level as the default for new windows. It covers settings semantics, tab/split behavior, tab moves between windows, acceptance criteria, tests, open questions, and telemetry.
Concerns
- No blocking concerns found. The spec is clear about in-memory V1 behavior, global-default semantics, focused-window scoping, and the required resync control.
- Security review found no material security concerns for this local UI/window-state feature.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Thanks a lot for putting this spec together and iterating on the review feedback. I should have started this contribution with a spec first, and I really appreciate you helping fill that gap here. This feature is still a very strong need for my workflow: having zoom scoped per window would make Warp much easier to use across multiple windows / displays. I also noticed that I hope this can land soon once the remaining spec review is resolved. For any future contributions from my side, I’ll make sure to start with a spec before implementation. |
2 participants
Spec-only PR for #10115 — independent zoom level per window.
Summary
Today
Cmd +/Cmd -/Cmd 0(and Command Palette zoom actions) mutate a single global zoom value, so every open Warp window re-renders simultaneously. This spec scopes those actions to the focused window only. Tabs and splits inside a window continue to share that window's zoom — the scope is per-window, not per-pane.Scope (V1)
Non-Goals
Spec document
specs/GH10115/SPEC.md— full Behavior Contract (B1–B7), Acceptance Criteria (A1–A8), Implementation Pointers verified against the codebase (crates/warpui_core/src/zoom.rs,app/src/appearance.rs,app/src/settings_view/appearance_page.rs), Tests (T1–T8), Open Questions, and Telemetry notes.Labels
ready-to-spec