feat(web): customize session list toolbar - #1309
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Desktop machine-filter customization is ignored. The configured toolbar slot renders a mobile-only control, while the desktop chip bar remains unconditional. Evidence:
web/src/components/SessionList.tsx:1412,web/src/components/SessionList.tsx:1523,web/src/components/MachineFilterBar.tsx:262.
Suggested fix:const machineFilterVisible = showMachineFilterBar && !toolbarLayout.hidden.includes('machineFilter') // Render both responsive variants from the configured slot; remove the unconditional bar.
- [Minor] The Codex-import toolbar shortcut becomes inert after its dialog is closed once on the still-mounted New Session route. Evidence:
web/src/components/NewSession/index.tsx:705,web/src/components/NewSession/index.tsx:712.
Suggested fix:initialCodexImportHandledRef.current = true setIsCodexImportDialogOpen(true) props.onInitialCodexImportHandled?.() // replace the URL without codexImport
Summary
- Review mode: initial. Two issues found: one desktop customization regression and one repeat-action regression.
Testing
- Not run (automation). Static review only; executing PR code is prohibited by the review prompt. Added tests do not cover desktop machine-filter layout behavior or reopening the Codex shortcut from the same route.
HAPI Bot
| align={group} | ||
| /> | ||
| ) | ||
| case 'machineFilter': |
There was a problem hiding this comment.
[MAJOR] The machine-filter layout is ignored on desktop
This configured slot only renders MachineFilterMenu, whose root is md:hidden (web/src/components/MachineFilterBar.tsx:262). At md and above, the separate MachineFilterBar is still rendered unconditionally at web/src/components/SessionList.tsx:1523. Therefore moving or hiding machineFilter has no desktop effect, even though this PR exposes it as a configurable toolbar item.
Suggested fix:
const machineFilterVisible =
showMachineFilterBar && !toolbarLayout.hidden.includes('machineFilter')
// Render the desktop/mobile machine control from this configured slot,
// and remove the unconditional MachineFilterBar below the toolbar.
if (!machineFilterVisible) return null
return <ResponsiveMachineFilter {...machineFilterProps} />| } | ||
| if (!machineId) return | ||
|
|
||
| initialCodexImportHandledRef.current = true |
There was a problem hiding this comment.
[MINOR] Clear the route trigger after consuming it
This ref suppresses the effect for the component's entire lifetime, but ?codexImport=true remains in the URL. Because /sessions/new is a child of SessionsPage, the desktop sidebar remains available: after closing the dialog, clicking its Codex-import toolbar action again navigates to the same URL, this guard returns, and the shortcut no longer opens. The in-form import button is only a workaround.
Suggested fix:
initialCodexImportHandledRef.current = true
setIsCodexImportDialogOpen(true)
props.onInitialCodexImportHandled?.()
void loadCodexImportSessions()Have NewSessionPage implement the callback with a replace-navigation that preserves the other search fields but removes codexImport; the next toolbar click will then produce a fresh trigger.
|
Thanks for the thorough work here — the implementation quality is clearly high (normalization, a11y fallbacks, tests). But I'd like to push back on the scope before this lands. The problem is real; the framework is disproportionate. The genuine UX issue is narrow: the date filter works independently of a text query but is only reachable after expanding search. That deserves a fix — but it's a one-line layout decision (render the date filter next to compact search), not a customization system. What this PR adds beyond that is a full toolbar-layout engine — preference schema, normalized persistence, drag-and-drop settings editor, i18n, tests (~950 LOC) — to manage a toolbar with ~5 actions. Customization frameworks pay off when the action set is large or grows uncontrollably; here the configuration surface is larger than the problem it configures. It also partially reverts two intentional simplifications:
This PR reintroduces both as optional toolbar actions. If we believe those removals were right, adding a framework to optionally undo them is a step sideways. If we believe they were wrong, we should discuss reverting them directly rather than shipping per-user escape hatches. Suggestion — split the PR:
One smaller note: if any discovery concern remains after the date-filter fix, the existing FUE primitive ( |
中文感谢您详细的反馈，也感谢您对实现质量的肯定。我理解您对范围和长期维护成本的顾虑。 我最初采用通用配置方案，是因为我观察到这部分工具栏操作在多个版本中经历过多次调整，例如刷新按钮被下拉刷新取代、Codex 导入被合并进新建会话流程等。不同布局和操作方式可能适合不同用户，因此我原本希望通过可配置的排序和隐藏能力兼顾这些偏好，避免以后继续反复调整默认布局。 另一个考虑是，项目中已经有会话输入框工具栏的自定义实现、交互设计和相关经验，所以我想沿用类似模式，一次性建立完整的配置能力。不过我认同您的判断：会话列表工具栏的操作数量更少，完整的布局引擎带来的复杂度和持续维护成本可能确实超过了当前需求，尤其是在尚无足够用户需求证据的情况下。 对我来说，目前真正希望保留的需求其实只有两点：
我可以移除：
也就是说，调整后的范围将不再是通用的工具栏布局系统，而只是日期筛选可访问性修复，加上一个针对搜索显示方式的单一偏好设置。 您是否愿意接受这个缩小后的范围？如果您仍然认为搜索显示模式不适合放在本次 PR 中，我也可以按照您的建议，将本 PR 进一步缩减为仅保留日期筛选常驻的修复，并把搜索布局选项和通用工具栏自定义暂时保留在单独分支中，等待更多用户需求证据。 在范围确认之前，我先不继续调整实现，避免产生更多无效改动。 EnglishThank you for the detailed feedback, and also for recognizing the implementation quality. I understand the concern about scope and long-term maintenance cost. My original motivation for building a generic configuration system was that the actions in this area have changed several times across releases—for example, Refresh was replaced by pull-to-refresh, and Codex import was consolidated into the New Session flow. Since different layouts and workflows may suit different users, I initially thought configurable ordering and visibility could accommodate those preferences without repeatedly changing the default toolbar. I was also taking cues from the existing session composer toolbar customization work, where the project already has related implementation experience and interaction patterns. That made a more complete solution seem like a reasonable way to address the problem in one step. That said, I agree with your assessment that the session-list toolbar has far fewer actions, and that a generic layout engine introduces disproportionate complexity and ongoing maintenance cost without enough demonstrated demand. The requirements I actually want to preserve are much narrower:
I can remove:
The revised scope would therefore no longer be a general-purpose toolbar layout system. It would be the date-filter accessibility fix plus a single preference for how search is presented. Would you be open to approving that narrower scope? If the search presentation preference is still too much for this PR, I’m also happy to reduce it further to the date-filter-only fix you suggested, and keep both the search layout option and the generic customization work on a separate branch until there is stronger evidence of user demand. I’ll hold off on reworking the implementation until the intended scope is confirmed, to avoid creating more unnecessary churn. |
|
Following the review feedback, I extracted the date-filter accessibility fix into #1367. I’m converting this PR to draft and parking the broader toolbar customization work until there is stronger evidence of user demand. No further review is requested here for now; I’ll keep it open as a reference for the existing implementation. |
2 participants
Summary
Context
The session date filter works independently of a text query, but it was only reachable after expanding search. The session-list header also had several actions whose availability and placement changed over time, without a way for users to choose the tools that fit their workflow.
This follows the composer-toolbar customization pattern from #1298. The optional Codex action deliberately reuses the consolidated New Session import flow from #1240 instead of restoring a second import implementation.
Changes
UX details
Test plan
bun typecheckbun run test:web(206 files, 1775 tests)bun run test:e2e -- terminal-wrap-fidelity.spec.ts(2 tests, against a manually started Vite fixture server)bun run test:shared(158 tests)bun run build:webbun run --cwd hub generate:embedded-web-assetsbun run build:hubgit diff --check origin/main...HEADNotes