Repository navigation
feat(canvas): add Auto-sort to the empty-canvas Tools submenu - #136
BIackFIame wants to merge 3 commits into
Conversation
|
@codex review Please review this PR for correctness, regressions and security issues. Review only; post findings on GitHub without changing code. This is part 6/7 of a cumulative series. Focus on changes introduced after predecessor |
There was a problem hiding this comment.
@BIackFIame, the maintainer has reviewed the current Workspace Tools UI in the combined Linux DEV build. Automatic sorting is a useful idea that we want to retain, but this PR needs a complete redesign of that interface and a much narrower product scope before it can be accepted. We are keeping this PR open for rework.
The current panel combines card arrangement, broadcast input, workspace snapshots and presets into one administration window. These are separate workflows with different consequences. Grouping them under a generic tools button makes a simple operation harder to discover and forces users to understand additional terminology and controls. Automatic sorting should be one clear canvas tool.
There are also concrete design-contract violations in the implementation. The .workspace-tools rules in src/renderer/src/styles/app.css hard-code white text, a dark RGBA background, border colors and pixel-based dimensions and spacing. These do not consistently follow the active system palette and root --ui-scale. WorkspaceBacklogTools.tsx introduces a separate composition of fieldsets, legends, custom button/input styles and a literal close character. Please rebuild the interface using the existing CanvasTTY menu components, theme tokens, typography, spacing, focus states and official vendored Lucide icons. Changing a few colors in the current window is not sufficient.
The requested direction is:
- Use the existing empty-canvas right-click context menu as the entry point. Add a small Tools menu with a single Auto-sort action initially, localized consistently with the selected application language. Reuse the current menu interaction and appearance instead of adding another standalone workspace dashboard or toolbar entry.
- Keep the sorting criteria together within that one tool. If task, status, project or grid choices are retained, present them as a compact choice using the existing menu pattern. Do not expose a separate “layouts” management surface alongside sorting. The tool should make its affected cards clear and retain a straightforward way to undo the position change.
- Remove snapshot import/export, preset management and broadcast/common input from this proposed core interface and narrow the PR accordingly. They are not part of the accepted sorting direction. Do not move the same bundle into another settings page or hide it behind a new advanced panel.
- Keep the Tools menu as a basic entry point for actual tools. Future independent tools can be proposed separately and added there when accepted. There is no need for a new tool framework, registry or plugin API in this change just to support one action.
- Follow the existing UI contract. The replacement must respect palette changes, UI scale, localization, keyboard access and the existing ownership of terminal/text-input events. Please verify the new interface on Linux as well.
This feedback accepts the sorting idea, not the rest of the current feature bundle. Project context, memory, output search and the Electron update still need independent review; please split unrelated work so it can be assessed separately.
The series is cumulative. Please provide the revised change on the accepted base without carrying the rejected #132, #134 or #137 functionality back into the project through this PR. The maintainer has separately accepted the #133 functionality after local inspection; that does not accept its rejected ancestors or establish compatibility for every combined runtime path.
Please rework the UI and scope along these lines. This PR remains open, with changes requested; it is not ready to merge.
This integrates the six contributions accepted during the maintainer's DEV review, together with the maintainer's local UI and minimap fixes. Rejected orchestration features, including #133, are excluded entirely. ## Included contributions and credit - #125 by @tab11pm: read-only file cards, directory browsing, and richer Markdown/code previews. - #130 by @SaneSanders: image/material remarks and handoff to an existing agent session. - #131 by @BIackFIame: macOS clipboard/edit context menus for text fields. - #138 by @pepechka-kun: usage history, measured quota changes, local-log evidence, and explicit attribution uncertainty. - #139 by @CLOSETTTY: interactive mascots and the Codex mascot creation workflow. - #140 by @teo-nex: the macOS node-pty file-descriptor leak fix, patch delivery, and resource diagnostics. Thank you to all six contributors. Each original PR head and its author commits are retained in Git history through separate merge commits. This integration must use a merge commit rather than squash or rebase. ## Maintainer patches - Fit the minimap overview to the camera viewport and all canvas objects together, with padding; retain stable drag coordinates during a gesture. - Keep the material handoff icon fixed in the header, remove the count beneath it, and place equally sized Cancel/Send actions at the bottom right of the handoff dialog. - Split usage history into five left-side sections: conditional estimate, quotas, agents, local-log tokens, and source state. Use pages within sections to fit the window without a long scrolling report. - Resolve shared App/IPC/preload integrations while retaining the file, handoff, and usage-history flows. - Retain the approved DEV dependency set and Electron 43.7.7; reuse the matching lockfile with patch-package 8.0.1. ## Scope #132, #133, #134, #135, and #137 are not included. #136 is also excluded pending the previously requested UI/UX redesign. No commits from that cumulative orchestration series are ancestors of this branch. Material handoff in #130 does not automatically apply a saved JSON agent report to the material state; this remains the reviewed behavior. ## Validation - `VITE_BUILD_CHANNEL=DEV npm run build` passed, including the script's TypeScript checks and native-helper build. - `git diff --check` passed. - Verified that all six accepted PR heads are ancestors of the integration and all six excluded PR heads are not. - Verified that the three approved local UI/minimap implementations match the selected DEV copy. - Added minimap geometry coverage for distant camera positions and adapted usage-history fixtures for section navigation. - The maintainer manually reviewed the accepted features in the earlier combined DEV build. The final integration without #133 has not received another manual UI pass. Automated tests and macOS/Windows runtime checks were not run locally for this integration.
…e startup bundle The canvas command palette and the canvas context menu are rendered only while they are open, but both were static imports of the canvas and so part of the startup bundle. They are now separate chunks, which frees about 18 KB of the startup bundle, and still open at once: the canvas preloads both chunks once it is idle after its first paint, and renders the loaded component directly. There is no lazy() and no Suspense boundary on this path (React holds a suspended boundary's reveal for up to 300 ms, which would delay the first right-click and the first Cmd/Ctrl+K). An open before the preload has finished loads the chunk at once and shows the menu as soon as it arrives. renderer-bundle-splitting checks that each is its own chunk, reachable only through a dynamic import(); canvas-preloaded-surfaces renders both with the real React through the real imports and checks that a preloaded menu and palette are in the very first render of an open.
Right-clicking empty canvas now shows a Tools row with one Auto-sort action. The criteria (by task, by status, by project, grid) are radio rows in that submenu, built with the existing canvas menu primitives. There is no separate panel, toolbar, presets or layouts list. - Scope: the current multi-selection when it holds two windows, otherwise terminal, plugin and Browser windows that are not completely inside a region. Companion mascots move only when selected. Sticky notes, materials and Files cards never move. - "By task" uses the orchestrator-to-subagent relation that already exists (parentSessionId): each subagent goes to the right of its parent. - The layout is deterministic: ties follow the current reading order, and sorting twice changes nothing. The block keeps the windows' top-left corner and keeps clear of HOME, regions, notes, materials, Files cards and windows it does not move (it shifts right or down, whichever is shorter, and never lands inside a region). With snapping on, every window lands on the 10px grid with the 20px snapping gap. - Moved windows get a presentation-only outline that clears on the next press or after a few seconds (they are not made the marquee selection), and a short glide that is skipped when reduced motion is requested. The Browser card does not glide, because its native view is placed from the measured card. - "Undo auto-sort" stays in the submenu while a sorted window is still where the sort put it. It restores only those windows. There is no global undo shortcut, so terminals and text inputs keep their keys. - The last criterion is kept in renderer memory for the running session, only after a sort happened. Nothing is persisted. A failed load or sort shows a localized toast. - The group-drag commit is extracted into commitLayerBounds and shared with auto-sort. - The Tools submenu body, scope and layout are not in the startup bundle: they load while the empty-canvas menu is open (or when Tools is pressed first), so the submenu still opens at once. Adds the official Lucide wrench and undo-2 icons, ru/en strings, and the UI contract text in en/ru/zh-CN. Tests drive the menu and the submenu.
…ed out - A context menu opened near a corner panel (minimap, shortcut hints, Home/zoom controls) moves up until it clears it; it was drawn over the bottom of the Home/zoom bar. When moving up cannot clear every panel (a tall menu near the right edge at UI scale 1.25, between the minimap and the shortcut hints), it moves sideways beside the panel it hits, at its own height; with no room on either side it keeps its place. Menus stay above the corner panels. - A submenu (Tools, Launch agent) opens where it covers no corner panel either: when its usual side and direction would run over the shortcut hints, minimap or Home/zoom controls (in whichever corners Settings put them), it opens in the other direction or on the other side; at UI scale 1.25 near the right edge, Tools opened over the hints panel and left its clipped text showing beside it. - The chevrons of Launch agent and Tools point to the side their submenu opens (the vendored chevron, rotated), not down. - "Needs at least 2 windows" is a plain hint, not the uppercase group label. It lines up with the radio labels and is as muted as the disabled rows it explains, so it does not read like an active item. - Zoomed out (summary), the outline of the windows a sort moved grows with the summary scale, so it stays visible at overview zoom for its 4 s. The placement code (and the existing chooseCanvasSubmenuSide) moves from canvasContextRouting to canvasMenuPlacement, which only the lazily loaded menu imports, so it stays out of the startup bundle.
3b151b2 to
9bd9079
Compare
BIackFIame
changed the title
|
I reduced this PR to auto-sort only, as you asked, and force-pushed the branch. What changed:
The PR is now three commits on |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd907968b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const value = surface.loaded(); | ||
| useEffect(() => { | ||
| if (!wanted || surface.loaded() !== null) return; |
There was a problem hiding this comment.
When the idle preload is already in flight and completes after this render reads surface.loaded() but before the passive effect runs, this fresh check sees a loaded value and returns without calling setLoaded. No listener from preloadWhenIdle triggers a render, so the first context-menu or command-palette open can remain blank until some unrelated state change or another open attempt; notify the hook when the render-time value was null even if the surface has become loaded by effect time.
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| if (kind !== "empty" || loadedToolsMenu) return; | ||
| let live = true; | ||
| loadToolsMenu().then((loaded) => { if (live) setToolsMenu(() => loaded); }, () => undefined); |
There was a problem hiding this comment.
If an empty-canvas menu is closed while the Tools import is pending and reopened just before it resolves, the new instance initializes ToolsMenu to null, then this guard can observe loadedToolsMenu becoming non-null between render and effect and return without updating local state. toggleTools also skips its loader when the global value is non-null, so that menu instance expands the Tools row but can never render the submenu; copy the cached component into state when this branch observes it.
Useful? React with 👍 / 👎.
|
Reworked as #163, because this branch was still based on 1.8.1. The new PR is rebased on current |
Branch
pr/canvas-auto-sort(head9bd90796, 3 commits), based onmainba427487. It is pushed to #136's branchbacklog/5-workspace.Why
Your review of #136 kept only the auto-sort idea and listed what it should be:
Everything else from #136 is gone: snapshots, presets, broadcast input, the Tools panel, output search, the drop preview and the earlier layers.
What changes for users
lazy()/Suspense, whose reveal throttle would hold the first open about 300 ms). The first right-click and the first Cmd/Ctrl+K open as fast as onmain. That frees about 16 KB of startup bundle, far more than auto-sort adds, and leaves room under the 1,700,000 budget for other PRs (macOS: editing keys for Claude Code cards (newline, select-all, copy/cut/paste) and UTF-8 locale #145 among them). The menu's placement code lives in its own module that only the menu chunk imports; the Tools submenu body loads while the empty-canvas menu is open.Commits
877c1f99perf(canvas): keep the command palette and the context menu out of the startup bundleb2fd3e4cfeat(canvas): add Auto-sort to the empty-canvas Tools submenu9bd90796fix(canvas): keep the Tools menu readable near corner panels and zoomed outCommit 1 is independent and can be taken alone. Commit 3 also changes the placement of the existing Launch agent submenu.
Verification
macOS: this exact tree, with a fake HOME. Results: helpers, catalog (no diff), typecheck, full suite 2187 / 2183 / 0 / 4, build and
audit:secretspass; startup bundle 12,956 bytes smaller thanmain(1,676,167). Each commit also type-checks and passes the tests it touches on its own. New tests:canvas-preloaded-surfaces(real React and react-dom/server through the real dynamic imports: nothing loaded at startup, a preloaded menu and palette are in the very first render of an open, idle preload with a timeout, a failed load is retried) and, incanvas-auto-sort, the Tools body is ready by the first render after Tools;renderer-bundle-splittingstill checks that both are their own chunks reachable only throughimport().Linux (Ubuntu 24.04 VM, xvfb): screenshots of every criterion, the outline, undo, the disabled hint and submenu placement, in ru/en, sage/night, at UI scale 0.85, 1.0 and 1.25.
First open on Linux (VM, xvfb, CDP input, integration build, 3 launches × 2 combos): first right-click 9.3–13.1 ms (2nd/3rd 4–7 ms;
main5.1–8.8 ms); first Ctrl+K synchronous (in the DOM before the key event's dispatch ends, as onmain); Tools submenu 1.8–3.4 ms.Not verified: Windows; the lilac palette. The screenshots predate the chunk split of the context menu (commit 1, which changes no markup or styles); the menu behaviour tests pass on this head.
Risks
Depends on
Nothing. It conflicts only in text with #133 (
docs/UI_CONTRACT.ru.mdand.zh-CN.md: both add a line to the same list; keep both) and with #149 (docs/UI_CONTRACT.md: keep this PR's bullets and add #149's clause about the card menu and hints panel).