fix(desktop): remove duplicated task menu rename - #4363
Conversation
7aa8202 to
1127b42
Compare
f7390cc to
f8b4174
Compare
|
@Astro-Han, when you have a moment, could you please take a look at this PR? The change is intentionally narrow: task rename currently has two entry points, and this removes only the redundant action-menu entry while keeping double-click rename. The resulting diff is small and focused, so it should be quick to review. Thanks! |
Astro-Han
left a comment
There was a problem hiding this comment.
The removal itself is clean and is a net deletion, but it takes the last keyboard-reachable rename with it, and that is worth settling before merge.
What holds up. The menu item is gone along with everything that only existed to serve it: trailingRef, pendingMenuIntentRef, and the requestAnimationFrame handover in MoreMenu's onOpenChange, which is now just setMenuOpen. SessionRowActionId narrows, copy.rename leaves the session catalog while projectRename stays, and nothing is left dangling: SessionNavRow still uses props.onStartRename for the double-click at session-history-list.tsx:777, SessionRenameTarget's session kind is still constructed, and Pencil is still the project rename icon. The tests do fail on the old behavior through the real components rather than a fixture: the story play at session-list-panel.stories.tsx:313 now asserts the item is absent, and the two menuLabels() assertions drop Rename. The new e2e covers both the project group and the ungrouped one, which is what #4362 asked for, and it checks that closing the dialog returns focus to the row.
P1-adjacent, filed as P2: after this change, renaming a task from the rail is a pointer-only gesture. session-history-list.tsx:776 gates rename on event.detail > 1, so a keyboard activation (detail === 0) never reaches it. handleListKeyDown at :289 handles only Escape. And handleListContextMenu at :274 opens the row's own menu rather than a second one, which the comment above it presents as the guarantee that "the menu and right-click agree" is a fact rather than a promise, so right-click loses rename at the same moment the menu does. The menu item was the keyboard equivalent of the double-click, and there is no affordance for the gesture that replaces it.
I am calling it P2 rather than P1 only because a detour exists: a keyboard user can open the task and rename it from the titlebar, which is a real button (titlebar-session-identity.tsx:219 inside app-shell.tsx:2756). But that only renames the task you are willing to open, and it moves the main pane to do it. The smallest fix is a branch in handleListKeyDown that answers F2 on a focused button.astryx-side-nav-item with the same startRename the double-click calls; it needs none of the focus handover the deleted menu item did, because focus is already on the row. If you would rather not, please say so in the description as a deliberate trade and take it back to #4362, since the issue did not weigh this.
P3: the Verification section is still the unfilled template. The template asks for the checks you actually ran and their results; the body has a screenshot and nothing else, while the checklist claims lint, format, typecheck and the affected suites passed locally.
Verification I ran: static read of f8b41743 against main, grepped the renderer for every other rename entry point, and checked the e2e fixture is test-scoped (fixtures.ts:692) so the grouping switch in the new test cannot leak into its neighbours. The hosted test check is green at this head. No build, no test run, no Desktop, so the keyboard claim is derived from the event condition and the two handlers rather than observed in the app.
AI-assisted review: drafted with Maka. I traced the double-click condition, the context-menu handler and the titlebar rename path by hand.
f8b4174 to
cae95fa
Compare
|
Addressed the latest review in cae95fa: focused task rows now support F2 rename, the E2E restores Enter-key coverage for the remaining action menu and covers F2 rename, and the PR Verification section now lists the commands and results actually run. The targeted Desktop E2E was attempted locally but its projectSidebarWindow fixture timed out before assertions, so the hosted run will validate that scenario. |
|
@Astro-Han, I retested the shortcut in the actual Desktop on macOS. Once the task row has keyboard focus, Fn+F2 opens the task rename dialog directly; plain F2 remains the system brightness key. This confirms that the current F2 handler is usable on my Mac through the function-key modifier. I do not currently have Windows or Linux environments, so I have not independently verified the physical F2 behavior on those platforms. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for going back to a real Desktop for that — and for the honest scope note about Windows/Linux. No need to chase those; the hosted E2E covers the interaction, and the platform difference here is the OS's, not the code's.
F2 is the right call given the constraint. Enter is already the action-menu trigger on the row, so it isn't available for rename, and F2 is the conventional fallback where Enter is taken. Needing Fn on a Mac is a real cost, but rename from the rail stays a secondary path — double-click is still the primary gesture — so I'm not going to trade the primary interaction to avoid a modifier on the secondary one.
The handler itself is clean: it early-returns unless the row actually has actions, calls preventDefault(), and hands event.currentTarget back as the focus target so the dialog returns focus to the row it came from. The E2E asserts both paths and the focus return on each.
That closes the P2 I raised — rename is keyboard-reachable again, and the menu duplication is gone with everything that only existed to serve it. Approving.
Task rows already support rename on double-click. Removing the duplicate menu action keeps one sidebar interaction while preserving project rename. CLOSES apache#4362 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com> Generated-by: Codex
cae95fa to
58f0e3f
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 58f0e3f7597515d49b5d3de5c69e3452f92a74ca (OPEN, MERGEABLE/BLOCKED awaiting human review). One P2 below. The direction stands — removing the duplicated entry is right — but see the P2. (The PR body's menu screenshot is illustrative only; this conclusion rests entirely on code.)
P2 — after removing the menu item, rename is effectively unreachable for keyboard users
The replacement entry points exist nowhere users can find them across three spots, and on one platform the key cannot even be pressed (fn-dependent). This is graded P2 not P3 because it is "a feature disappearing for a class of users", not "slightly worse experience". Minimum fix: register F2 in the keyboard-help.tsx list; more robustly, also hang an entry in the command palette, or use an fn-independent chord on macOS (e.g. Enter or ⌘Enter on the selected row).
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移。当前 head 是 58f0e3f,未关闭。方向成立,一条 P2:删掉菜单项后键盘用户实际上够不到重命名。修好再合。
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up to my review above, correcting an omission: the earlier comment stated the P2 without the evidence behind it. Same head 58f0e3f7597515d49b5d3de5c69e3452f92a74ca (OPEN, MERGEABLE/BLOCKED awaiting human review), +31/-46 across 5 files. Grade unchanged: one P2, plus one P3 that the earlier comment dropped entirely. At freeze time the only hosted check was test:in_progress — no terminal result — so nothing below is machine-verified.
The author's premise checks out
The Summary says task rows already support rename on double-click. That is true: packages/ui/src/session-history-list.tsx:774-793, where the row's onClick branches on if (event.detail > 1 && props.actions) straight into rename. So this PR is not removing the only entry point, and the direction — dropping a duplicated menu action — is right.
The PR also adds a keyboard entry point that the Summary does not mention (session-history-list.tsx:797-808, an onKeyDown handler firing on F2). That addition is what the whole question below turns on.
P2 — after the menu item is removed, rename is effectively unreachable for keyboard users
With copy.rename and the menu item gone, task rename has two paths left: double-click (mouse only) and F2. Three specific problems with the F2 path:
apps/desktop/src/renderer/keyboard-help.tsx:22-24— the file's own header comment says it "Lists every shortcut the renderer reacts to so users don't need to scrape the README." Searching that file forF2,rename, or重命名returns nothing. This PR adds a shortcut the renderer reacts to without registering it in the panel that claims to list all of them, so the change breaks an invariant that file states about itself.apps/desktop/src/renderer/command-palette.tsx— searching forrename/Renamereturns nothing, so the command palette is not an alternative route either.- On macOS the function row is hardware keys by default (F2 is brightness up); unless the user has enabled "Use F1, F2, etc. keys as standard function keys," the app never receives a plain F2 — it requires fn+F2.
Together: on a default-configured macOS, a keyboard-only user has no discoverable, working way to rename a task. The removed menu item was duplicated, but it was the only visible one.
Minimum fix: register F2 in the keyboard-help.tsx list. More robust: also add a command-palette entry, or use an fn-independent chord (e.g. Enter or ⌘Enter on the selected row).
P3 — F2 does nothing while the row's ⋯ button holds focus
session-history-list.tsx:1274 keeps onKeyDown={(event) => event.stopPropagation()} on the actions span, while the F2 listener sits on the row. Any focus inside the row-actions control swallows the event. The PR's own story shows the friction: packages/ui/stories/session-list-panel.stories.tsx now presses {Escape}, asserts focus returned to the actions button, calls taskControl.focus(), and only then sends {F2} — three steps to get focus back onto the row.
Verified sound
Pencildoes not become an unused import:session-history-list.tsx:1178still uses it for the project rename item.copy.renameis removed cleanly — no remaining reference in the file, and the key is dropped from all three locale blocks (zh-CN, zh-TW, en) rather than one.- No dead backend path:
apps/desktop/src/renderer/features/session-navigation/controller/session-row-actions.ts:167renameSessionis still reached through the rename dialog. - Multi-select is unaffected:
bulkCount > 1takes the selection-commands branch, which never had rename. - The two unit assertions moving from
['Pin', 'Rename', 'Archive']to['Pin', 'Archive']match the change.
What I could not judge
The application was not run, Storybook was not run, and the E2E was not run. The macOS F2 point is derived from platform default behavior, not observed on a macOS machine — if this app handles the function row specially somewhere I did not find, that point falls, and I would welcome a correction. The author reports the local projectSidebarWindow fixture timed out and the targeted E2E did not complete, leaving hosted CI as the evidence; at freeze time hosted test was still in_progress, so there is currently no machine-verified evidence for anything here, including the new F2 path.
The menu screenshot in the PR body is illustrative; this conclusion rests entirely on the code.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移。当前 head 是 58f0e3f,未关闭。这条是补上一条评论删掉的证据,等级不变。
作者的前提成立:双击重命名本来就存在(session-history-list.tsx:774-793),所以删的不是唯一入口,方向是对的。P2 是删掉菜单项后键盘用户实际够不到重命名——新增的 F2 监听(:797-808)没有登记进 keyboard-help.tsx,而那个文件头注释自称列出渲染层响应的全部快捷键;命令面板里也搜不到 rename;macOS 默认 F 区是硬件键,不改系统设置收不到 F2。最小修法是把 F2 登记进那份清单。P3 是焦点在行操作按钮上时 F2 被 :1274 的 stopPropagation 吞掉,PR 自己的 story 用三步绕开。
需要说明的是,macOS 那条来自平台默认行为,没有在 macOS 上实测;冻结时 hosted test 还是 in_progress,因此包括新增 F2 路径在内,目前没有任何机器验证过的证据。
Superseded by my comment below: withdrawing the approval, this is a design change that needs deciding first.
Astro-Han
left a comment
There was a problem hiding this comment.
I am withdrawing my earlier approval on this one, and I want to be straightforward about why, because it is my reversal, not a problem with your code.
I went back to the question this PR should have started from: is removing the menu entry a fix, or a design change? It is a design change, and #4362 records it as a preference ("these duplicate interactions are unnecessary") rather than a problem someone hit. The two entry points are not equivalent. The ⋯ menu item is the one a user can find: it is visible, it is what right-click opens, and it is the only one that says "Rename" anywhere. Double-click is a convention nothing on the row announces, and F2 needs fn on a Mac and is not listed in the keyboard help. So the net effect of this change for someone who has not read this thread is that rename disappears from where they would look for it. That is a real regression of a working feature, and the PR gives no concrete reason for it beyond the duplication itself. Two entry points for one action is not a cost when one is discoverable and the other is fast; that is the normal shape for rename in list UIs.
I should have asked this before the first review instead of reviewing the implementation, and the F2 round I sent you on was work you did on my account. Sorry for that.
Where this leaves it: I am not going to merge a removal of the visible rename entry. If you think the sidebar should have a single rename gesture, the place to make that case is a Discussion or on #4362 with the reasoning spelled out, so it can be decided before code is written; if that lands, I will review the PR that follows. I would rather close this one than keep it open against a decision that has not been made, but I will leave that to you.
简体中文
我撤回之前的 approve,原因在我不在代码。这是一个设计变更而不是修复:#4362 记录的是偏好("重复的交互没必要"),不是有人遇到的问题。两个入口并不等价:⋯ 菜单项是用户找得到的那个(可见、右键也是它、唯一写着"重命名"的地方),双击没有任何提示,F2 在 Mac 上要按 fn 且不在快捷键帮助里。所以对没读过这个串的用户来说,重命名从他会去找的地方消失了。这是真实的功能退化,PR 除了"重复"本身没有给出具体理由。第一轮我就该先问这个问题而不是评实现,让你补 F2 那一轮是我的责任,抱歉。我不会合并删掉可见重命名入口的改动;如果你认为侧栏只该保留一种重命名手势,请先在 Discussion 或 #4362 里把理由讲清楚,定了再写代码。
|
Thanks @Astro-Han for explaining the discoverability concern. I opened #4964 to discuss the interaction before taking this PR further. I am not committed to removing the Rename entry or making double-click the only path; one option is to keep Rename in the context menu and offer Pin/Archive as direct row actions, while discussing how the full menu stays discoverable and keyboard-accessible. I am happy to close this PR if the community prefers the current behavior or a different design. |
Summary
Task rows already support rename on double-click. Removing the duplicate menu action keeps one sidebar interaction while preserving project rename.
Fixes #4362
Verification
npm run format:checknpm run lintnpm run typecheck --workspace packages/uinpm --workspace @maka/ui run buildnode --test packages/ui/dist/__tests__/session-history-multi-select.test.js(22 tests passed)
The targeted Desktop E2E was attempted locally, but its
projectSidebarWindowfixture timed out before reaching the assertions.The hosted CI run covers the updated keyboard scenarios.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex GPT 5.6 sol
Checklist
Does this PR entail a change in behavior?