feat(web-shell): named session groups and color tags in the sidebar#6350
Conversation
Extend web-shell session organization with named groups (create / rename / delete, assign a session to a group) alongside quick color tags, and surface pin / archive state. The grouping data is plumbed end-to-end through the daemon. - core: session-organization-service carries group id / name / color and pin / archive metadata on organized-list entries - sdk / acp-bridge: session-list entries gain groupId / groupName / groupColor / archivedAt; add SessionGroupColor and list-session-groups result types - cli/serve: dispatch + session routes expose listing and assigning groups - web-shell: sidebar group management UI (create / rename / delete groups, color picker, pin, archive) and reuse the shared "Group" label for the group action, dropping the redundant "Move to group" string
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR!
The PR body doesn't follow the pull request template. The template requires these sections:
- What this PR does / Why it's needed — the PR uses a single
## Summaryheading instead - Reviewer Test Plan — with
How to verify,Evidence (Before & After), and theTested ontable. This is the section maintainers rely on to verify the change actually works - Risk & Scope — main risk, what's out of scope, breaking changes
- Linked Issues —
Closes #NorRelated Issues
Could you update the PR body to match the template? It doesn't need to be long — just the right headings so reviewers can find what they need.
中文说明
PR 正文没有按照 PR 模板 填写。模板要求以下几个部分:
- What this PR does / Why it's needed — 目前只有一个
## Summary,需要拆开 - Reviewer Test Plan — 包括
How to verify、Evidence (Before & After)和Tested on表格。这是 reviewer 确认改动是否生效的关键依据 - Risk & Scope — 主要风险、不在范围内的内容、破坏性变更
- Linked Issues —
Closes #N或相关 issue
请按模板更新 PR 正文,不需要写很长,只要把对应的 heading 补齐即可。
— Qwen Code · qwen3.7-max
|
Updated the PR body to follow the current template — What this PR does, Why it's needed, Reviewer Test Plan (How to verify / Evidence / Tested on), Risk & Scope, and Linked Issues. My local checkout's template was a few commits behind 中文已按当前模板更新 PR 正文——What this PR does、Why it's needed、Reviewer Test Plan(How to verify / Evidence / Tested on)、Risk & Scope、Linked Issues。之前本地检出的模板落后 |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present: What this PR does, Why it's needed, Reviewer Test Plan (How to verify / Evidence / Tested on), Risk & Scope, Linked Issues. Problem: This is a genuine UX gap — users with many sessions in the web-shell sidebar have only named groups for organization, but no lightweight per-session color tagging. The PR adds a first-class Direction: Aligned. Session management is on the roadmap ( Approach: The scope feels right — adding color as a third organization dimension (alongside group and pin) without restructuring the existing system. Each layer does the minimum: core adds One note: Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必需章节齐全。 问题: 这是一个真实的 UX 缺口——web-shell 侧栏有大量会话的用户只能通过命名分组来组织,缺少轻量级的单会话颜色标签。PR 在会话组织模型中加入一等公民的 方向: 对齐。会话管理在路线图上( 方案: 范围合理——将 color 作为第三个组织维度加入,不重构现有系统。每层只做最少的事。UI 中颜色与命名分组互斥是正确的——保持侧栏分类简洁。核心服务保持独立,互斥由客户端执行。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
wenshao
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/services/session-organization-service.ts:477 |
Unknown session colors silently normalized to null in readStore() with no warnOnce — unlike the group color path at line 586 which does log. A corrupted or forward-version store file loses session color data with no diagnostic trail. |
Add a warnOnce call when an unsupported session color is encountered, mirroring the group color path: this.warnOnce(unknown-session-color:${sessionId}\0${color}, Session ${sessionId} uses unsupported color "${color}"; normalizing to null)
|
— qwen3.7-max via Qwen Code /review
Color / named group / recent are mutually exclusive buckets in the web-shell sidebar — a color-tagged session shows in its color section, not "recent". But the organized session-list `group=ungrouped` filter only checked `groupId == null`, so a color-tagged session with no named group leaked into ungrouped results for REST/ACP consumers, disagreeing with the UI taxonomy. Align the server filter: ungrouped means no named group and no color tag. Adds an ACP session/list test asserting a color-tagged session is excluded from group=ungrouped (fails on the old filter, passes on the new one).
saveGroupEditor's create-with-target path assigned the new group but left any existing color tag in place, unlike the sibling assignSessionGroup / assignSessionColor paths that keep color and named group mutually exclusive. Because color takes precedence in the sidebar's section bucketing, the session stayed in its color section and the group assignment had no visible effect. Send `color: null` alongside `groupId` on that path, and extend the create-group dialog test to assert the assignment clears the color.
|
@qwen-code /triage |
Code reviewIndependent proposal before reading the diff: add a The PR's approach matches this almost exactly. A few observations:
TestsReal-scenario testingThis is a web-shell browser UI feature (sidebar session groups/colors). It cannot be meaningfully tested via tmux 中文说明代码审查独立方案(在读 diff 之前):在会话组织中加入 PR 的做法与此基本一致。
测试所有 917 个测试通过。构建 ✓ 类型检查 ✓ Lint ✓ 真实场景测试浏览器 UI 功能,无法通过 tmux 测试。单元测试端到端覆盖。 — Qwen Code · qwen3.7-max |
|
This is a well-scoped feature PR that does what it says: adds lightweight per-session color tagging alongside the existing named-group system, plumbs it through the daemon, and integrates it into the web-shell sidebar. The mutual-exclusivity design (color XOR named group in the UI, both allowed in storage) is the right call — it keeps the core service generic while enforcing a clean taxonomy at the presentation layer. Every layer does the minimum it needs to. The core change is ~40 lines of source code. The API boundary validates with existing Build, typecheck, lint, and all test suites pass cleanly (917 tests total across core, CLI, and web-shell). The only testing gap is that this is a browser UI feature — I can't meaningfully capture the sidebar rendering via tmux. The unit tests cover the data flow and component behavior, but a visual check in a real browser would be the final confirmation. That's a standard limitation for web-shell PRs and CI will catch cross-browser issues. One minor suggestion (non-blocking): the No concerns. Ships the feature cleanly, the code is straightforward, the tests are thorough. 中文说明这是一个范围合理的 feature PR,做了它说要做的事:在现有命名分组系统旁加入轻量级每会话颜色标签,通过 daemon 打通,并集成到 web-shell 侧栏。互斥设计(UI 中颜色或命名分组二选一,存储层都允许)是正确的——保持核心服务通用,在展示层执行干净的分类。 每一层只做最少需要做的事。核心改动约 40 行源码。API 边界用现有的 构建、类型检查、lint 和所有测试套件均通过(共 917 个测试)。 一个小建议(非阻塞): 没有顾虑。功能干净交付,代码直接,测试充分。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR. The inline comment still marks the blocking issue.
Follow-up to the ungrouped filter fix: the per-group filter (group=<id>) also ignored color precedence. Core and the REST/ACP update paths can persist both groupId and color, and the sidebar renders such a session in its color bucket, so group=<id> API consumers saw a session the web-shell shows elsewhere. Require `color == null` there too, matching the sidebar taxonomy (color > group > recent). Adds an ACP session/list test for a session with both groupId and color set.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-structured feature addition. Color tagging is plumbed end-to-end (Core → ACP → REST → SDK → UI) with consistent mutual-exclusivity semantics and solid test coverage across all layers.
Highlights
- Three-layer input validation chain (ACP → REST → Core) prevents invalid colors from reaching storage
viewOrganizationnormalizes unknown stored colors tonull— good forward-compatibility hygiene- Color-first bucketing in
session-list.tsfiltering matches the sidebar's visual priority, keeping REST/ACP consumers aligned with the UI - Tests cover: assignment, clearing, invalid input rejection, ungrouped/group filter exclusion, and sidebar rendering
Optional Suggestions (non-blocking)
-
Duplicated color list —
SESSION_GROUP_COLORSin the sidebar mirrorsGROUP_COLOR_OPTIONSfrom core. The isolation comment is fair, but a future palette expansion requires updating both. Consider exporting from the SDK if this becomes a maintenance concern. -
DaemonSessionOrganizationResult.coloris optional (?:) whileSessionOrganizationView.coloris required. Current sidebar code handles this gracefully viasession.color && SESSION_GROUP_COLORS.includes(...), but future code paths should be aware of the distinction. -
Menu height estimation —
34 * (groups.length + SESSION_GROUP_COLORS.length + 2) + 25packs several magic numbers; extracting named constants would improve readability (very minor).
LGTM — approve.
What this PR does
Adds named session groups to the web-shell session sidebar — create, rename, and delete a group, and assign a session to one — alongside the existing quick color tags, and surfaces pin / archive state. The grouping data (group id / name / color, pinned / archived state) is plumbed end-to-end through the daemon so every web-shell client sees the same organization. It also unifies the group action label: the trigger button and the popover it opens now share a single
Groupstring (sidebar.sessionGroup), dropping the redundantMove to groupwording.Why it's needed
Color tags alone are ad-hoc; users juggling many sessions want durable, named buckets they can rename and reuse. Separately,
Move to groupwas both inaccurate and inconsistent — the popover it opens is already titledGroup, and its actions are broader than "move" (assign a color tag, ungroup, or create a group), so a singleGrouplabel is clearer and matches the panel it opens.Reviewer Test Plan
How to verify
The backend and daemon plumbing are covered by unit tests (see Evidence). For the UI: run
qwen serve --web, open the session sidebar, and use the group action (the group icon on a session row, or the "…" overflow menu). Confirm that (1) the action readsGroupand matches the title of the popover it opens; (2) the popover can create / rename / delete a named group, assign a color tag, ungroup, pin, and archive a session; (3) assigning a session to a group persists across a sidebar reload.Evidence (Before & After)
Label — Before: the group button/menu read
Move to groupwhile the popover it opened readGroup(two strings for one action). After: both readGroup, asserted by the updatedWebShellSidebar.test.tsx(selector switched to[aria-label="Group"]). The broader grouping UI and daemon plumbing are verified by the unit suites below rather than screenshots.Unit suites run locally, all green:
packages/core—session-organization-service.test.ts(23)packages/cli—serve/server.test.ts(35),serve/acp-http/transport.test.ts(14),serve/acp-http/dispatch.test.ts(14),serve/routes/session.test.ts(23),serve/server/session-list.test.ts(5)packages/web-shell—WebShellSidebar.test.tsx(31)Tested on
Environment (optional)
Local unit tests via
vitest(no dev server or sandbox). Ran on macOS (Darwin); Windows and Linux are covered by CI.Risk & Scope
Grouparia-label; screen readers still disambiguate them by ARIA role (button vs. menu), so the redundancy is cosmetic. The rest of the change is additive.groupId/groupName/groupColor/archivedAt) are optional, and the label change is internal to web-shell i18n.Linked Issues
None.
中文说明
What this PR does
在 web-shell 会话侧栏加入命名分组——新建、重命名、删除分组,并把会话归入某个分组——与已有的快捷颜色标签并存,同时展示置顶 / 归档状态。分组数据(分组 id / 名称 / 颜色、置顶 / 归档状态)经 daemon 端到端打通,任何 web-shell 客户端看到的组织方式都一致。此外统一了分组按钮文案:触发按钮与它弹出的面板现在共用同一个
Group(sidebar.sessionGroup),去掉多余的Move to group。Why it's needed
只有颜色标签是临时性的;管理大量会话的用户想要可重命名、可复用的持久命名分组。另外
Move to group既不准确也不一致——它弹出的面板本就叫Group,而且能做的不止"移动"(选颜色标签、移出分组、新建分组),因此统一为Group更清晰、也与它弹出的面板一致。Reviewer Test Plan
How to verify
后端与 daemon plumbing 由单元测试覆盖(见 Evidence)。UI 部分:运行
qwen serve --web,打开会话侧栏,使用分组操作(会话行上的分组图标,或 "…" 溢出菜单)。确认:(1) 操作显示为Group,与它弹出面板的标题一致;(2) 面板可新建 / 重命名 / 删除命名分组、选颜色标签、移出分组、置顶、归档;(3) 把会话归入分组后,刷新侧栏仍保留。Evidence (Before & After)
文案——Before:分组按钮 / 菜单显示
Move to group,而它弹出的面板显示Group(一个操作两套词)。After:两者都显示Group,由更新后的WebShellSidebar.test.tsx断言(选择器改为[aria-label="Group"])。更大范围的分组 UI 与 daemon plumbing 由下方单元套件验证,而非截图。本地运行的单元套件,全部通过:
packages/core—session-organization-service.test.ts(23)packages/cli—serve/server.test.ts(35)、serve/acp-http/transport.test.ts(14)、serve/acp-http/dispatch.test.ts(14)、serve/routes/session.test.ts(23)、serve/server/session-list.test.ts(5)packages/web-shell—WebShellSidebar.test.tsx(31)Tested on
见上方表格(macOS ✅ 本地单元测试;Windows / Linux⚠️ 由 CI 覆盖)。
Environment (optional)
本地用
vitest跑单元测试(无 dev server 或 sandbox)。在 macOS(Darwin)上运行;Windows 与 Linux 由 CI 覆盖。Risk & Scope
Group这个 aria-label;屏幕阅读器仍能靠 ARIA role(button vs. menu)区分,冗余仅是外观层面。其余为纯增量改动。groupId/groupName/groupColor/archivedAt)均为可选,文案改动仅限 web-shell i18n 内部。Linked Issues
None.