Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@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 sidebar row actions from navigation buttons by jackeyfaker77 · Pull Request #2944 · apache/maka · GitHub
Skip to content

fix(ui): separate sidebar row actions from navigation buttons - #2944

Closed
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling
Closed

fix(ui): separate sidebar row actions from navigation buttons#2944
jackeyfaker77 wants to merge 1 commit into
apache:mainfrom
jackeyfaker77:fix/project-row-actions-sibling

Conversation

@jackeyfaker77

@jackeyfaker77jackeyfaker77 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Render session and project overflow menus as sibling controls instead of nesting buttons inside SideNavItem.
  • Add an Astryx siblingAction slot for project rows so keyboard order stays project, project actions, then nested sessions without moving the control visually.
  • Cover both row types and project focus order with server-rendered regression tests.

Verification

  • npm --workspace @maka/ui test — 131 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Storybook product-sidebar-session-list--project-groups: confirmed zero nested buttons, project → project actions → session focus order, and unchanged action placement

Before review

  • Human contributor reviews the diff and confirms ownership of the submission

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

Generated-by: Codex

@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review August 13, 2026 07:48
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis here is the sharper of the two PRs. siblingAction is the right shape, and your ordering assertion is the only test in either PR that catches the real defect: #2951 renders the menu after </SideNavItem>, which puts it after the entire session subtree in focus order.

I'm still leaning toward #2951 for this fix, for one reason. patches/README.md asks for a patch only when a bug blocks shipping and cannot be worked around at the call site, and this one can be — rendering the actions as the first child of .maka-project-row gives menu → row → sessions with no change to Astryx. Two smaller points pointing the same way: the slot lands as a block child of the flex-column root, so it buys DOM order but not layout — this PR still carries the same calc(var(--space-2) + var(--space-6) + var(--space-2)) geometry as #2951, plus a > div > selector into Astryx's internals — and the patch file is currently in conflict with #2923 and churned about a third at the last Astryx bump.

Two things from here are worth keeping. The ordering assertion, ported to compareDocumentPosition rather than string indexOf. And siblingAction itself, as an upstream proposal to facebook/astryx — if the slot goes inside the split-action row <div> instead of the column root, it removes the geometry hardcoding for both row types, which is the version actually worth sending. I'd rather carry that as a real API than as a ninth local hunk. Would you be up for opening it upstream?

One unrelated note: .appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } can never match — session-list-panel.tsx:154 unmounts the list entirely when the sidebar collapses.

中文

感谢这个 PR——两个方案里它的诊断更准。siblingAction 是对的形状,而你的顺序断言是两个 PR 中唯一能抓住真实缺陷的测试:#2951 把菜单渲染在 </SideNavItem> 之后,焦点顺序上它落在整个会话子树后面。

这次我仍倾向 #2951,理由只有一条。patches/README.md 要求只为阻塞发布、且无法在调用点绕开的 bug 打补丁,而这个可以绕开——把 actions 渲染成 .maka-project-row第一个子节点,就能得到 菜单 → 项目行 → 会话,完全不动 Astryx。另有两点指向同一方向:槽位插在 flex column root 的直接子节点上,只买到 DOM 顺序、没买到布局,所以本 PR 仍然带着与 #2951 相同的 calc(var(--space-2) + var(--space-6) + var(--space-2)) 几何硬编码,还多了一条指向 Astryx 内部结构的 > div > 选择器;而且这个 patch 文件当前与 #2923 冲突,上一次 Astryx bump 时被重写了约三分之一。

有两样东西值得保留。一是顺序断言,改用 compareDocumentPosition 而非字符串 indexOf。二是 siblingAction 本身,作为提给 facebook/astryx 的上游提案——如果槽位放进 split-action 那条路径的行 <div> 而不是 column root,它能同时消掉两种行的几何硬编码,那才是值得发上去的版本。比起第九个本地 hunk,我更愿意把它作为真正的 API 来用。你愿意去开这个上游 PR 吗?

一条无关的提醒:.appFrame[data-sidebar-state="collapsed"] .maka-project-row-action { display: none } 永远匹配不到——侧边栏折叠时 session-list-panel.tsx:154 会把整个列表卸载。

@jackeyfaker77

Copy link
Copy Markdown
ContributorAuthor

Thanks — this makes sense. I agree that #2951 is the better landing path for the immediate fix if the ordering can be corrected at the call site without adding another local patch.

I'd be happy to open an upstream Astryx PR. I'll move siblingAction into the split-action row so the API owns both DOM order and layout, and I'll use a DOM-based compareDocumentPosition assertion rather than string indexOf.

Good catch on the unreachable collapsed-sidebar selector as well. I'll leave this PR as-is for now unless you'd prefer me to close it after #2951 lands.

中文

感谢说明,这个取舍很合理。如果能够在调用点修正顺序、避免新增本地 patch,我同意 #2951 更适合作为当前问题的落地方案。

我愿意给 Astryx 开上游 PR。我会把 siblingAction 放进 split-action 的行容器中,让这个 API 同时负责 DOM 顺序和布局,并把测试改成基于 DOM 的 compareDocumentPosition,而不是字符串 indexOf

也感谢指出折叠侧边栏选择器永远匹配不到。我暂时保留这个 PR;如果你希望在 #2951 合并后关闭它,我可以再处理。

Generated-by: Codex

@Astro-Han

Copy link
Copy Markdown
Contributor

#2951 has landed, so I'm taking you up on the offer and closing this. Thank you for the work here, and for how gracefully you handled ending up as the duplicate.

I did check whether to rebase and keep parts of it first — there's nothing left to salvage. The remaining delta over main is just the Astryx patch; everything else would revert what landed. #2951 went in with the call-site ordering fix plus an E2E that tabs through the row, toggles the disclosure and checks focus return.

Your upstream plan is exactly right, and the split-action row is the part that matters — that's what makes the API own layout as well as DOM order, which is what lets both row types drop the absolute positioning and the mirrored chevron geometry. Link the Astryx PR here or on #2360 when it's up; I'll track it and delete our workaround the day it ships.

Review assistance: Claude Code (Opus); I verified the focus-order behaviour and the Astryx DOM structure by hand.

中文

#2951 已经合并,那就按你说的关掉这个。感谢你在这里的工作,也感谢你面对「成了重复的那一个」时的坦然。

我先确认过要不要 rebase 再挑一部分进去——没有可挑的了。相对 main 的残余只剩那条 Astryx patch,其余部分都会把已合并的东西改回去。#2951 最终带着调用点的顺序修复合入,另有一条 E2E:按 Tab 走过整行、开合 disclosure、检查回焦。

你的上游计划完全正确,而 split-action 行容器正是关键的那一步——它让这个 API 同时负责布局和 DOM 顺序,也正因如此,两种行才能去掉绝对定位和复刻的 chevron 几何。Astryx PR 开好后在这里或 #2360 贴个链接,我会跟进,上游一落地就删掉我们的绕行方案。

评审协助说明:Claude Code (Opus);焦点顺序的行为与 Astryx 的 DOM 结构由我手工核实。

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.

2 participants

@jackeyfaker77@Astro-Han