Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Tab close gesture and close flow crates/ui-egui/src/chrome.rs |
doc_tabs routes Shift-clicks to unsaved::close_all with "closeAll" and displays returned errors in the status bar. Plain clicks retain the individual-tab close path. A test checks that Shift-clicking closes three open documents. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature · Severity of issue fixed: Low
Sequence Diagram(s)
sequenceDiagram
participant CloseButton
participant doc_tabs
participant close_all as unsaved::close_all
participant status as app.status
CloseButton->>doc_tabs: Shift-click close button
doc_tabs->>close_all: close_all(app, "closeAll")
close_all-->>doc_tabs: Return result
doc_tabs->>status: Display returned error
Possibly related PRs
- storytold/vectorcraft#3844: Adds document-tab context-menu close actions that use the same close flow.
Suggested reviewers: bflatastic
Merge Risk: 
Minimal · up to 31f78
-
The tab gesture appears ready to merge after normal checks; no actionable closing-flow risk remains identified.
Architecture risk: 
Medium · up to 31f78
-
The change affects 1 system.
Changed systems:
cratesArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — crates (service) was modified; 1 changed file maps to changed impact.
Before / after behavior
- observed — Modified behavior in crates/ui-egui/src/chrome.rs:
doc_tabsadds aclose_allflag to track whether the close-button click requests closing every document. - observed — Modified behavior in crates/ui-egui/src/chrome.rs: A close-button click with Shift sets the close-all flag; without Shift, it records the clicked tab for individual closure.
- observed — Modified behavior in crates/ui-egui/src/chrome.rs:
doc_tabscallsunsaved::close_allwith"closeAll"when the flag is set and reports errors throughapp.status; otherwise, the existing individual-tab close path runs. - observed — Modified behavior in crates/ui-egui/src/chrome.rs: Adds a test that opens three documents, Shift-clicks the first tab’s close button, and asserts that all documents are closed.
Reliability and maintainability
- inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1
-
Pre-merge checks: 
5
-
Full details: Title check
-
Explanation
The title "UI: Shift-click document tab close box closes all" matches the change in crates/ui-egui/src/chrome.rs, where Shift-clicking a document tab's close button runs the closeAll flow. A plain click still closes only that tab. The title is specific and describes the primary change. It uses a "UI:" prefix, which is acceptable.
Full details: Linked Issues check
-
Explanation
Issue [
#3900] requires Shift+click on any tab close box to close all documents, while a plain click closes one document. The diff adds Shift detection for each tab close response and routes the gesture tocrate::unsaved::close_all(app, "closeAll"); the plain path still callscrate::unsaved::close(app, i). The existingclose_allandconfirmflow preserves modified-document prompts, stops on cancellation, and usesfile.closeAll. The added test covers three documents, and existingclose_all_and_quit_ask_once_per_modified_documentcovers the shared prompt flow.
Full details: Out of Scope Changes check
-
Explanation
The changes are limited to document-tab close handling and a focused UI test in
crates/ui-egui/src/chrome.rs. The test supports issue [#3900]. No unrelated product behavior or new command is introduced.
Full details: Description check
-
Explanation
The description states the purpose (Shift-click on a document tab's close box runs the Close All path, plain clicks close only that tab, and unsaved documents keep the Save / Don't Save / Cancel flow) and lists three test commands. It references the issue (
#3900). It does not follow the template's 'What and why' and 'How it was tested' headings, and it has no explicit 'Why' rationale beyond the linked issue. The checklist is filled in and mirrors the template. Its 'How it was tested' content is limited to test command names without describing manual checks (CLI, MCP, screenshots) or results, and the checklist items are ticked without evidence. Overall the description is mostly complete, so it passes.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
FredApps
left a comment
There was a problem hiding this comment.
Read the diff. A plain click on a tab close box still closes that tab; with Shift held it runs Close All, and a failure goes to the status line as before. The new test sends a real shift-click on the first tab close box and checks every document closed, and the gate passes at this head.
|
Thanks @MiQ71! The code is fine and goes through the existing close-all flow, but I couldn't find any public documentation that Shift-clicking a tab's close box closes all documents in the reference app (the tip I found is for Photoshop). I'm holding it on parity; if you can link a source for Illustrator, I'll take another look. |
|
Thanks for catching that. I went back and checked the Illustrator references more carefully, and I can't substantiate the Shift-click behavior there either. The Illustrator material I found documents Close All via the tab context menu / Ctrl+Alt+W, while Shift-clicking the close box appears to be Photoshop behavior. So the parity claim in #3900 seems to be incorrect. No objection to holding this PR on that basis. |
Thanks for digging into the references again — and thanks for the PR itself, it's appreciated. One clarification on the parity claim, because I think we're measuring different things. The Shift+click isn't something I took from Adobe's documentation: it's an undocumented gesture in Illustrator. It isn't in the help pages, the shortcut list, or (as far as I could find) any community post — which is exactly why the references don't substantiate it. So "the docs don't describe it" and "the app doesn't do it" are two different statements here, and my original write-up didn't make that distinction clearly enough. I've since updated the issue to say the behaviour is observed, not documented. What I can offer instead of a citation is a reproduction on a stock Illustrator install:
Whether Photoshop shares the gesture I honestly don't know; both use the same tabbed document shell, so it wouldn't surprise me. But "also in Photoshop" wouldn't make it Photoshop-only — the point is that Illustrator does it as well, just quietly. Since there's no documentation to cite, the practical check is the app itself: anyone with Adobe Illustrator installed can repeat steps 1–3 above in under a minute and see the result directly. |
Messhias
left a comment
There was a problem hiding this comment.
cargo xtask ci is incomplete or unchecked on this PR. Run it end to end (fmt, clippy, tests, assets, brands, layers, wasm), tick the checklist when green, and reply here with the outcome. That is required before any approve from here.
|
Ran |
@Messhias |
Fixes #3900.
Shift-clicking the close box on any document tab now uses the existing Close All path. Plain clicks still close only that tab, and unsaved documents keep the existing Save / Don't Save / Cancel flow.
Tests:
cargo test -p vectorcraft-ui-egui shift_clicking_a_tabs_close_box_closes_all_documents --libcargo test -p vectorcraft-ui-egui a_tabs_close_box_sits_where_the_platform_puts_it --libcargo test -p vectorcraft-ui-egui close_all_and_quit_ask_once_per_modified_document --libChecklist
cargo xtask cipasses (fmt, clippy, tests, assets, brands, layers, wasm).unwrap(),expect(),panic!,unreachable!,todo!orunimplemented!outside tests, and no unchecked indexing on data from files, parameters or messages. Errors are returned asResultand shown to the user.ASSETS.md(no new assets in this PR).Summary by CodeRabbit