fix(desktop): show archived actions inline - #4375
Conversation
6189e6b to
5b773db
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The part that answers #4370 is clean; the icon redraw riding along with it is a separate decision and I would rather it travelled separately.
What holds up. Two IconButtons replace the one-item MoreMenu, the accessible names still carry the task name (unarchiveTask / deleteTask) while the tooltips carry the short verb, which is tidier than the old aria-label patch on a shared label. The copy catalog drops moreActions / restore / restoreTask in both locales rather than keeping them around, the surface inventory is regenerated, and the confirmation dialog and cleanup flow are untouched. The e2e genuinely fails on the old behavior: it clicks a button by a name that does not exist before this change, then asserts one aria-hidden svg per button and zero 更多操作 buttons.
P2 on the icons, details inline. Short version: icons.tsx describes itself as the seam that decides which library backs these names, and this change makes it a drawing surface as well, shadowing lucide's own Archive while removing ArchiveRestore. The motivation is fair, lucide has no archive-down counterpart, but the blast radius reaches the rail's row menus and none of that is in the before/after images.
P3: the irreversible action is now one click away in the same neutral ghost styling as the safe action beside it. The confirmation dialog still stands, so this is recoverable and minor, but a Trash2 and an Unarchive rendered identically and adjacent at 32px invites the wrong one. A destructive-toned icon, or separation from the unarchive button, would carry the difference the old menu used to.
P3: icons.test.tsx is mostly a change detector. assert.deepEqual(archive.slice(0, 2), unarchive.slice(0, 2)) states a real rule, that the two share a tray. The two assertions after it transcribe the arrow path data, so any nudge to the glyph turns them red without anything being broken. Either drop them or express the intent, that the stems run opposite ways and the heads point opposite ways.
P3: the rail now says 取消归档 for sessions while copy.projectRestore still says 恢复 for projects, and this change gives both the same Unarchive glyph. One mark, two words. If Unarchive is the more precise verb, the project entry should follow.
P3: feabaffa and 5b773db8 carry no Generated-by trailer, only the head commit does. CONTRIBUTING.md asks for it on each affected commit and through the squash. Also, the checklist leaves "Lint, format, typecheck and the affected suites pass locally" unticked while the Verification section lists exactly those as passing.
Mergeability: this head is CONFLICTING. git merge-tree against current main shows the only conflict is docs/astryx-surface-file-inventory.md, which is generator output, so a rebase plus a rerun of generate-astryx-surface-inventory clears it. session-history-list.tsx auto-merges.
Verification I ran: static read of d3a039a6 against main; I read lucide 1.33.0's Archive and ArchiveRestore node data out of node_modules to check the pairing claim, and derived the conflict scope from git merge-tree. The hosted test check is green at this head. No build, no test run, no Desktop, and I did not render any of the icons, so the glyph judgements come from path data rather than from looking at them.
AI-assisted review: drafted with Maka. I checked the icon call sites, the lucide node data and the merge conflict myself.
10d24bf to
7afa1a1
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
A note on current state, not a technical verdict: this PR is currently CONFLICTING with main. Line-level review now would not survive the rebase.
Could the author please rebase onto current main and let CI run? Happy to review once it is rebased and green.
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.
7afa1a1 to
62bf194
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks. At 62bf194c3, the newly added story play fails before reaching its action assertions because it selects a revision that the production projection deliberately hides. I checked the exact-head CI failure against the fixture and revision-selection source. This is separate from the existing icon/style discussion; please repair the test selection without changing production revision folding. Local Desktop interaction/visual acceptance was not rerun.
中文
新story选择了生产折叠逻辑隐藏的旧revision,导致动作断言尚未执行就失败。已对照当前head CI日志、fixture和投影源码,独立于已有图标/样式讨论。应修测试选择器,不修改生产revision折叠;本轮未重新做本机Desktop视觉验收。
AI-assisted review using OpenAI Codex/Astra; coordinating-agent checks and probe limits are stated above. This is not an independent human review.
Archived task rows mixed a text action with a one-item overflow menu. Direct icon actions make both choices visible without redundant chrome. CLOSES apache#4370 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
The deletion flow still opened the overflow menu removed by the UI change. Clicking the direct action keeps the E2E on the user path. Refs apache#4370 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com> Generated-by: Codex
62bf194 to
d79de1b
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
The change does what it claims — archived rows now expose Unarchive and Delete inline instead of one button plus a single-item overflow — and I found no correctness, data-loss or state-machine regression. The archived action set is {Unarchive, Delete} while the active row keeps {Pin, Rename, Archive}, no action is newly possible or lost, and deletion is still gated by a confirm dialog. Two small items.
P3 (Nice-to-have) — this PR edits a branch that is unreachable in production, so part of the change is a no-op and the branch can be deleted.
// packages/ui/src/session-history-list.tsx:1356-1364
props.session.isArchived ? copy.unarchive : copy.archive, icon Unarchive, actions.onUnarchiveThe rail's session list is filtered before it reaches the row renderer — the projection applied to SessionRailData.sessions drops archived sessions (session-nav-filter.ts, applied where the navigation reads are built), and no test sets isArchived: true on the rail. So the isArchived === true arm cannot be taken, and the icon swap in it has no effect. Since the repository's review method asks for removable code: collapse the row to the active case and drop SessionRowActions.onUnarchive plus its provider wiring; the unarchive copy in the rail's catalog is then unused too.
P3 (Nice-to-have) — the story pins presence, not behaviour. The new play assertion checks both buttons exist with the right accessible names and that the old overflow control is gone, but never clicks. Swapping the two handlers, or dropping the click handlers entirely, would still pass. One click on Unarchive asserting the row leaves the list would cover the "act on an archived row" path this PR is about.
Review-relevant risks. Presentation and wiring stay at the existing source of truth, and @maka/ui is private with no in-repo consumer of the removed icon export, so there is no public-contract effect. No security, licensing, release or governance effect identified.
Required conclusion.
- Optimal for the actual problem? Substantially yes for the linked issue. The smallest coherent change would arguably skip the new archive glyph (the icon library already ships
Archive; onlyUnarchiveis missing) and drop the dead branch, but neither is a defect. - Production code that can be deleted? The unreachable archived arm in the rail row plus
SessionRowActions.onUnarchiveand its provider wiring. - Low-quality tests to delete or replace?
none identified; the icon test's frozen path strings are over-specified but do catch the one real bug they target — tighten rather than delete. - Deeper refactor required? No.
- Ready to merge? Yes in my assessment; both items above are cleanup.
- Residual risks / verification gaps: the archived-tasks page has no end-to-end coverage, and the story only runs under the Storybook smoke job. I did not run the suite; the unreachable-branch conclusion is from reading the filter and the row renderer.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
Revision folding hides the older root title, so the story must select the visible representative to exercise its action assertions. Refs apache#4370 Generated-by: OpenAI Codex Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
d79de1b to
a26d71e
Compare
Summary
Archived task rows exposed Restore as a text button while hiding their only
other action, Delete, behind an overflow menu. Show both actions directly as
compact icon buttons, rename Restore to the more precise Unarchive, and use a
matched Archive/Unarchive icon pair so the inverse relationship is visible.
The existing permanent-delete confirmation and task cleanup flow are unchanged.
Fixes #4370
Verification
npm run format:check -- apps/desktop/e2e/parent-session-deletion.spec.ts apps/desktop/src/renderer/locales/settings-tasks-copy.ts apps/desktop/src/renderer/settings/tasks-settings-page.tsx packages/ui/src/__tests__/icons.test.tsx packages/ui/src/icons.tsx— passednpm --workspace @maka/ui run typecheck— passednpm --workspace @maka/desktop run typecheck— passednpm --workspace @maka/ui run build— passednode --test packages/ui/dist/__tests__/icons.test.js— 1 test passedapps/desktop/e2e/parent-session-deletion.spec.ts; CI is pendingBefore Change
After Change
AI use
Select exactly one:
Tool(s) and scope: Codex — implementation, tests, and PR preparation.
Checklist
Does this PR entail a change in behavior?