spec: open review panel maximised by default (#10036)#10223
spec: open review panel maximised by default (#10036)#10223lonexreb wants to merge 5 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 default-off setting to open the code-review panel maximised in one click while preserving the current default behavior. The behavior and acceptance criteria are mostly clear, and the security review found no material high-level security concerns for this UI preference.
Concerns
- The setting key is specified as
editor.review_panel_open_maximized, but the referenced code-review/editor settings in this repository use thecode.editor.*TOML namespace. The spec should name the final TOML path consistently before implementation. - The test plan omits coverage for the runtime-toggle behavior required by A3.
Verdict
Found: 0 critical, 1 important, 1 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
|
|
||
| ## Behavior contract | ||
|
|
||
| - B1. New setting `editor.review_panel_open_maximized: bool`, |
There was a problem hiding this comment.
code.editor.show_code_review_button, code.editor.show_code_review_diff_stats, code.editor.auto_open_code_review_pane_on_first_agent_change). Specify code.editor.review_panel_open_maximized consistently, or explicitly justify the new top-level editor.* namespace, so implementation and TOML round-trip behavior are unambiguous.
|
|
||
| ## Test plan | ||
|
|
||
| - T1. Setting round-trips through TOML. |
There was a problem hiding this comment.
💡 [SUGGESTION] Add a test for A3 so toggling the setting while a non-maximised panel is already open is verified not to mutate the current panel state.
|
Round-1 fixes for oz-for-oss review: (1) renamed setting key to |
|
/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 a default-off setting for opening the code review panel maximised and covers the core behavior, acceptance criteria, and runtime toggle expectations.
Concerns
- The public TOML setting key is inconsistent between the PR description and the spec/test plan, so implementation could ship an incompatible config key unless the spec chooses one canonical name.
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
|
|
||
| ## Goal | ||
|
|
||
| Add a boolean setting `code.editor.review_panel.open_maximized` |
There was a problem hiding this comment.
editor.review_panel_open_maximized, but the spec and tests require code.editor.review_panel.open_maximized; pick one canonical public TOML key here so implementation and compatibility tests do not diverge.
|
Round-2 fix: audited every mention of the setting key across Behavior, Settings, Acceptance, Tests, Implementation Pointers, and Open Questions; renamed any stragglers to the single canonical key |
|
/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 a default-off setting to open the code review panel maximized and covers the intended behavior, settings exposure, implementation sketch, and tests.
Concerns
- The settings UI path in the spec does not match the current Code settings layout, so implementation could put the toggle in the wrong place.
- The OFF/default acceptance criteria conflict with the existing maximize/restore state machine language; the spec should define behavior when the panel was previously maximized before being closed.
- The implementation sketch points to a non-existent
review_panelanchor inapp/src/settings/code.rs, while related code-review settings are currently split across other settings groups. - The PR description uses
editor.review_panel_open_maximized, which conflicts with the spec's canonicalcode.editor.review_panel.open_maximized; update the PR text to avoid propagating the wrong key.
Verdict
Found: 0 critical, 2 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
| affordance still works to shrink it. | ||
| - B3. When `false`, no behavior change — the existing two-click | ||
| flow is preserved exactly. | ||
| - B4. Setting is exposed in **Settings → Editor → Code review** |
There was a problem hiding this comment.
Code → Editor and Code Review); specify whether to use the existing Code subpage or intentionally create a new Editor page.
| - A1. With setting OFF (default): clicking code-review opens | ||
| side-panel; second click maximises. (Pixel-equivalent to today.) |
There was a problem hiding this comment.
|
|
||
| ## Implementation sketch | ||
|
|
||
| - Add the setting in `app/src/settings/code.rs` near the other |
There was a problem hiding this comment.
💡 [SUGGESTION] app/src/settings/code.rs has no review_panel settings today; update this sketch to point at the actual related settings anchors or explicitly require adding this one to CodeSettings despite the current split.
|
Addressed all 2 important + 2 suggestion concerns from latest oz-for-oss review: Settings UI path corrected (important #1) Implementation sketch grounded in real code (important #2) Maximize/restore state-machine conflict resolved (suggestion)
PR description (suggestion) — note for maintainer |
- Correct settings location: GeneralSettings in terminal/general_settings.rs (verified against codebase) - Reconcile B5 with maximize/restore state machine: setting governs fresh opens; snapshot wins on restore; close-then-reopen is fresh open - Codify OFF + previously-maximised-then-closed behavior in A1 - Mandate PR-description grep for non-canonical key spellings before merge
|
Addressed 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 a new default-off setting for opening the code review panel maximised and covers the user-facing setting, state-machine interaction, and tests. The overall scope is appropriate, and the security pass did not identify a material security surface.
Concerns
- The default-off behavior contract conflicts with the current right-panel state machine for close-then-reopen after a maximised panel.
- The implementation sketch omits the actual code-review button/toggle path and points implementers at entry points that do not own the shared open/maximise state transition.
- The rejected-key grep requirement is self-contradictory unless it excludes spec prose or scopes itself to implementation artifacts.
Verdict
Found: 1 critical, 2 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
| `workspace:toggle_maximize_code_review_panel` action) maximises | ||
| it. Pixel-equivalent to today. | ||
|
|
||
| **OFF + previously maximised then closed (no tab restore in |
There was a problem hiding this comment.
🚨 [CRITICAL] This conflicts with the default-off promise and the current state machine: closing the right panel does not clear is_right_panel_maximized, so a maximised panel can reopen maximised today. Either preserve that behavior when the setting is OFF or explicitly scope this as an intentional behavior change with tests.
| the `Category::new("Code Editor and Review", ...)` grouping (lines | ||
| 314, 438). Do NOT introduce a new category or settings page. | ||
|
|
||
| - The runtime entry points are `setup_code_review_panel` (line |
There was a problem hiding this comment.
ToggleCodeReviewPane flows through toggle_right_panel / update_right_panel_open_state, while setup_code_review_panel only populates panel content. Specify the shared state-update location so the primary click path actually seeds the maximised default.
| `app/src/settings_view/code_page.rs` (lines 297/301 and 421/425), | ||
| inside the `Category::new("Code Editor and Review", ...)` group | ||
| — not in any new category. | ||
| - T8. Grep test: no occurrence of `editor.review_panel_open_maximized`, |
There was a problem hiding this comment.
1 participant
Spec for #10036. New
code.editor.review_panel.open_maximizedboolean setting (default false). When enabled, clicking the code-review button opens the panel pre-maximised in one click. Default-off preserves today's behavior exactly.Closes (spec-only) #10036.