Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(ui): separate project row actions from navigation by MoonOld · Pull Request #2951 · apache/maka · GitHub
Skip to content

fix(ui): separate project row actions from navigation - #2951

Merged
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360
Aug 13, 2026
Merged

fix(ui): separate project row actions from navigation#2951
Astro-Han merged 2 commits into
apache:mainfrom
MoonOld:fix/sidebar-project-row-actions-2360

Conversation

@MoonOld

Copy link
Copy Markdown
Contributor

Summary

  • move the project MoreMenu out of SideNavItem.endContent, so project navigation and row actions render as sibling buttons instead of nested interactive controls
  • keep unavailable state and session-count metadata inside the navigation button, with a reserved trailing slot that preserves the existing layout for collapsible and empty projects
  • add a focused project-row DOM regression test covering sibling controls, collapsible ARIA wiring, and the absence of nested buttons

This completes the remaining project-row half of the issue; the session-row half was fixed in #2566. It intentionally keeps the change local to Maka rather than expanding the Astryx SideNavItem API in this PR.

Fixes#2360

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/ui run test — 137 passed
  • npm --workspace @maka/desktop run typecheck
  • npx biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx apps/desktop/src/renderer/styles/sidebar.css
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 126 stories passed
  • Storybook Native Conversation, grouped by project: 11 project rows, 0 nested buttons, no console errors; collapse, unavailable/relink, archived/restore, and rename focus-return paths verified

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@MoonOld

Copy link
Copy Markdown
ContributorAuthor

Hi @Astro-Han, could you review this when you have a chance? This completes the project-row half of #2360 using the same sibling-control pattern as #2566. The branch is rebased onto current main, and all CI checks are green. Thanks!

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right, and EndContentHitTarget is genuinely gone — no remaining callers anywhere in the repo.

One P1 to fix before this merges: for an expanded project the row's ... menu now lands after the entire session subtree in Tab order. Details inline on the render site.

Root cause, and why it shows up here and not in #2566

SideNavItem keeps the row's interaction on the row element and renders both endContent and the collapsible subtree inside its own root (SideNavItem.js: <div root>{itemElement}{childrenGroup}</div>). Any trailing control therefore has to be laid over the row from outside it, and every non-cosmetic finding below is the price of that overlay: the broken tab order, the hardcoded Astryx geometry, the reserved placeholder slot, the lost hover, the leftover stopPropagation. #2566 paid that price once on session rows, where the row has no subtree and the tab-order cost was zero. Applying the same template to a row that does have a subtree is exactly where it breaks.

The class of bug is closed today, and the summary's claim is accurate

Every remaining SideNavItem endContent in the repo is non-interactive: session-sidebar-nav.tsx:58 a <kbd>, settings-surface.tsx:308 a Badge, and this file's now-pure meta. SideNavSection's endContent is safe because its header is a <div>.

But nothing enforces it. endContent?: ReactNode carries no constraint, and the only automated guard is two SSR unit tests. The Storybook smoke cannot help, because no story renders project grouping at all — app-shell.stories.tsx:301 defaults to 'conversation' and nothing in the repo passes 'project'. The smoke run fails on console.error, which is precisely how this bug announced itself, so a two-line story export with initialViewMode: 'project' turns it into a real regression gate. (#2582 removed the assertion layer that used to cover this.)

Consider unifying the two action components rather than shrinking this diff

ProjectItemActions and SessionItemActions are now ~55 lines of line-for-line duplication: trailingRef, menuOpen, pendingAction + pendingActionRef, mountedRef, the cleanup effect, runXAction, the wrapper span, and the onOpenChange intent flush. Only label, items and the position differ. A shared RowActionsMenu({ label, position, buildItems }) collapses them, absorbs the write-only ProjectRowActionId / SessionRowActionId pair along the way, and means a third caller inherits the fix instead of repeating the template a third time.

Looking at the project row alone this diff is already close to minimal — the 40 reindented lines in menuItems are forced churn from lifting the project && actions guard to the call site, not avoidable rewrite. The only way to make it meaningfully smaller is to merge it with the session row.

Staying local is the right call, and worth recording upstream anyway

Astryx is a pinned external dependency (0.3.0 in three places) and the local patch file already runs 779 lines across 8 components — changing SideNavItem's DOM structure is the riskiest kind of entry to add there. Astryx also treats the sibling shape as its own sanctioned answer: the split-action path in SideNavItem.js is commented "This avoids nesting interactive elements", it just only triggers when a primary action and collapsible are both set. So this is not working around the design system.

The end state is still the upstream one. useClickableContainer / interactiveRef on SideNavItem would delete both overlays, both CSS blocks, the placeholder slot and every stopPropagation, and make focus order correct by construction. That is a cross-repo component refactor against three lines here, so not now — but the price of staying local is real and worth naming: the before-disclosure offset mirrors three Astryx internals (nav item padding-inline, gap, chevron box) that live behind compiled StyleX classes and are not public contract. The arithmetic is right today — 8 + 24 + 8 lands exactly on the placeholder — but an Astryx minor that changes the gap or the chevron, or anyone passing size="sm" to this SideNavItem, drifts the menu off its slot with nothing to catch it. Please open a separate upstream-tracking issue rather than letting the case die when Fixes #2360 closes.

Tests have teeth, and are heavier than they need to be

Reverting the fix does fail them — verified by compiling the mutant. But four of the nine new assertions are insensitive to that mutation, and one of the three that do kill it relies on the SSR parser preserving invalid nesting, which a real browser parser flattens. Trimming is noted inline.

The larger gap is that of the issue's three stated reasons, only "no longer nested" is covered: real tab order, two independently focusable controls, and whether stopPropagation still swallows list-level keys are all untested — as are the empty-project (position: 'trailing', no disclosure) and archived (restore-only menu) branches this PR newly introduces. packages/ui/stories/session-list-panel.tsx's ProjectGroups story already has an unavailable project and an empty one, so no new fixture is needed. A single Playwright spec — Tab reaches the row, Tab reaches the menu, Enter opens it, Escape returns focus, Enter on the row only toggles — is the honest home for the interaction half; apps/desktop/e2e/ has no sidebar project-row coverage today.

中文

方向是对的,EndContentHitTarget 也确实删干净了——全仓无残留调用点。

有一条 P1 建议在合并前修:展开状态下,项目行的 ... 菜单在 Tab 顺序里落到了整个会话子树之后。细节见渲染处的行内评论。

根因,以及它为什么在这里出现而 #2566 没有

SideNavItem 把行的交互留在行元素上,同时endContent 和可折叠子树都渲染在自己的 root 内(SideNavItem.js<div root>{itemElement}{childrenGroup}</div>)。因此任何尾部控件都只能从行外面上去,而下面每一条非装饰性的 finding 都是这个覆盖层的价钱:断裂的 Tab 顺序、硬编码的 Astryx 几何、预留占位槽、丢失的 hover、残留的 stopPropagation#2566 在会话行上付过一次这个价钱,那里的行没有子树,Tab 顺序的代价为零。把同一模板套用到确实有子树的行上,正是它失效的地方。

这类 bug 目前是关闭的,summary 的说法准确

仓库里剩余的每一处 SideNavItem endContent 都是非交互的:session-sidebar-nav.tsx:58<kbd>settings-surface.tsx:308Badge,本文件的 meta 现在也是纯展示。SideNavSectionendContent 安全,因为它的 header 是 <div>

但没有任何机制约束这一点。endContent?: ReactNode 不带任何约束,唯一的自动化守卫是两个 SSR 单测。Storybook smoke 帮不上忙,因为没有任何 story 渲染项目分组——app-shell.stories.tsx:301 默认 'conversation',全仓没有一处传 'project'。smoke 以 console.error 判失败,而这个 bug 正是这样暴露出来的,所以一个两行的 initialViewMode: 'project' story export 就能把它变成真正的回归关卡。(#2582 移除了原先承担这件事的断言层。)

建议合并两个 action 组件,而不是缩小本 diff

ProjectItemActionsSessionItemActions 现在有约 55 行逐行重复:trailingRefmenuOpenpendingAction + pendingActionRefmountedRef、清理 effect、runXAction、wrapper span,以及 onOpenChange 的 intent flush。差异只有 labelitems 和位置。一个共享的 RowActionsMenu({ label, position, buildItems }) 可以把它们合并,顺带消化掉只写不读的 ProjectRowActionId / SessionRowActionId,并让第三个调用点继承修复,而不是第三次复述模板。

单看项目行,这个 diff 已接近最小——menuItems 那 40 行重新缩进是把 project && actions 守卫上提到调用方后的强制 churn,不是可以省掉的重写。要显著更小只能与会话行合并。

保持本地修法是对的,但仍值得记录到上游

Astryx 是 pin 死的外部依赖(三处 0.3.0),本地补丁文件已达 779 行、覆盖 8 个组件——改 SideNavItem 的 DOM 结构是其中风险最高的一类条目。而且 Astryx 自己就把兄弟节点当作官方解法:SideNavItem.js 里 split-action 路径的注释原文是 "This avoids nesting interactive elements",只是它仅在同时设置 primary action 与 collapsible 时才触发。所以这不是绕过设计系统。

终局仍然在上游。SideNavItem 接入 useClickableContainer / interactiveRef 会删掉两处覆盖层、两块 CSS、占位槽和全部 stopPropagation,并让焦点顺序天然正确。那是一次跨仓库的组件重构,对比这里的三行改动,现在不值得——但保持本地的代价是真实的,值得点名:before-disclosure 的偏移复刻了三个 Astryx 内部量(nav item 的 padding-inlinegap、chevron 尺寸),它们藏在编译后的 StyleX class 里,不是公开契约。今天的算术是对的——8 + 24 + 8 精确落在占位槽上——但一次改动 gap 或 chevron 的 Astryx minor 升级,或有人给这个 SideNavItemsize="sm",都会让菜单静默漂出槽位而无人拦截。请另开一个上游跟踪 issue,别让 Fixes #2360 关闭时把这个案子一并埋掉。

测试有牙齿,但比需要的重

回退修复确实会让它变红——已编译变异体验证。但新增的九条断言里有四条对该变异完全无感,而能杀死变异的三条里还有一条依赖 SSR 解析器保留非法嵌套,真实浏览器解析器会把它拍平。裁剪建议见行内。

更大的缺口是:issue 列出的三条理由里只有「不再嵌套」被覆盖,真实 Tab 顺序、两个可独立聚焦的控件、以及 stopPropagation 是否仍在吞掉列表级按键,全部没测;本 PR 新引入的空项目分支(position: 'trailing',无 disclosure)与 archived 分支(只有 restore 的菜单)同样没测。packages/ui/stories/session-list-panel.tsxProjectGroups story 已经带了 unavailable 项目和空项目,不需要新造 fixture。交互那一半的正确归属是一条 Playwright spec——Tab 到行、Tab 到菜单、Enter 打开、Escape 回焦、行上的 Enter 只切换展开;apps/desktop/e2e/ 目前没有任何侧边栏项目行覆盖。

Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/session-history-list.tsx Outdated
Comment threadapps/desktop/src/renderer/styles/sidebar.css Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
Comment threadpackages/ui/src/__tests__/session-history-row-actions.test.tsx Outdated
@MoonOld

Copy link
Copy Markdown
ContributorAuthor

@Astro-Han, I addressed the P1 and all inline P3 findings in 28aa7cf. Could you take another look when convenient?

What changed:

  • rendered the project menu before the keyed SideNav root, giving menu → project navigation → sessions in DOM/Tab order;
  • added the direct-action hover/pressed bridge without leaking hover from nested session rows;
  • removed the dead project-row keydown propagation guard;
  • reused the existing session trailing/action CSS primitives;
  • trimmed the brittle SSR assertions and added the document-order assertion;
  • switched the unit selector away from the menu-state-dependent aria-expanded="true".

For permanent interaction coverage, I added an Electron Playwright journey that uses a real seeded project and verifies Tab order, Enter collapse/expand, aria-hidden/inert, menu isolation, Escape focus restoration, and rename-dialog focus restoration.

I reused the existing ProjectGroups story in packages/ui/stories/session-list-panel.stories.tsx (it already sets viewMode: 'project' and is included as product-sidebar-session-list--project-groups). Storybook smoke owns render/console health; Electron E2E owns the keyboard interaction contract.

I kept the project/session action-component merge out of this review fix because it would expand into the existing session-row behavior without being required to close #2360. I did record the missing upstream component seam as requested: facebook/astryx#4987.

Validation completed locally:

  • npm --workspace @maka/ui run test: 137 passed
  • Desktop typecheck: passed
  • full Desktop Electron E2E: 22 passed, 1 existing conditional skip
  • Storybook build: passed
  • Storybook render smoke: 126 stories passed
  • real-window programmatic smoke: passed
  • real-browser ProjectGroups verification: Tab/menu/disclosure/hover behavior passed; no console errors
  • Biome + git diff --check: passed

I also ran the root npm test flow. The changed UI/Desktop suites passed; the command's overall exit remained non-zero in two untouched areas: the local system Python is 3.9 and cannot parse eval's Python 3.10 Path | None annotation, and the current runtime-host authenticated-WebSocket test reproduces Session model is not enabled even when run alone. This branch has no eval/runtime-host changes; the PR CI run is the authoritative clean-environment result.

@Astro-Han

Copy link
Copy Markdown
Contributor

All six findings are addressed, and the E2E spec goes further than what I asked for — Tab adjacency, disclosure toggling with aria-hidden/inert, Escape returning focus, and the rename dialog's focus return are all covered. Moving the actions to the first child also made the ordering assertion fall out of the unit test naturally.

Two non-blocking follow-ups: SessionItemActions and ProjectItemActions are still ~55 lines of the same component, worth its own issue; and the upstream ask for a real SideNavItem action slot is still worth opening at facebook/astryx — with it, both row types could drop the absolute positioning and the mirrored geometry.

Squash-merging. Thanks for the quick turnaround.

Review assistance: Claude Code (Opus) ran parallel correctness, simplification, test-quality and architecture passes over this PR; I verified the focus-order finding and the Astryx DOM structure by hand before filing it.

中文

六条 findings 全部处理,E2E 还超出了我要求的范围——Tab 相邻性、开合 disclosure 时的 aria-hidden/inert、Escape 回焦、重命名对话框关闭后的回焦都覆盖了。把 actions 移到第一个子节点之后,单测里的顺序断言也自然成立了。

两条不阻塞的后续:SessionItemActionsProjectItemActions 仍是同一个组件的约 55 行重复,值得单开一个 issue;SideNavItem 真正的 action 槽位这条上游诉求仍值得去 facebook/astryx 提——有了它,两种行都能去掉绝对定位和复刻的几何。

Squash 合并。感谢这么快的响应。

评审协助说明:Claude Code (Opus) 对本 PR 并行跑了正确性、简化、测试质量与架构四轮审查;焦点顺序那条结论与 Astryx 的 DOM 结构,我在提出前手工核实过。

@Astro-Han
Astro-Han merged commit e74a9c0 into apache:mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar rows nest a <button> inside a <button>

2 participants

@MoonOld@Astro-Han