feat(web-shell): modernize multi-workspace sidebar - #6804
Conversation
|
Re-run — updated after author pushed fix commits ( Template looks good ✓ Problem: UX improvement — the previous sidebar used a flat layout that made workspace/session relationships hard to scan, and session creation/restoration flows assumed the primary workspace. No reproduction needed for a feature PR. Direction: Aligned. Web Shell is a first-class package, multi-workspace is a real daemon capability. Making the sidebar workspace-aware and carrying workspace identity through creation, restoration, URLs, and embedding callbacks is a natural extension. Size: 32 files changed, ~3141 additions / ~5170 deletions. Production logic ~5143 lines (excluding tests). Past the 1000-line advisory, but the scope is coherent — all sidebar/workspace related. No core paths touched. Approach: The workspace-grouped sections, workspace-level actions, search, pin/archive, and shadcn-based dialogs feel right. Two fix commits since the initial review addressed IME Escape handling, page sizes for secondary workspace requests, grouped session rendering, The 3300-line 中文说明重新运行 — 作者推送了修复提交（ 模板完整 ✓ 问题： UX 改进 — 旧 Sidebar 使用扁平布局，工作区/会话关系难以扫描，会话创建和恢复流程默认使用主工作区。功能 PR 无需复现。 方向： 对齐。Web Shell 是一等包，多工作区是 daemon 的真实能力。让 Sidebar 感知工作区并在创建、恢复、URL 和嵌入回调中保持工作区身份是自然的扩展。 规模： 32 个文件变更，约 3141 行新增 / 5170 行删除。生产逻辑约 5143 行（排除测试）。超过 1000 行建议阈值，但范围一致 — 均围绕 Sidebar/工作区。未触及核心路径。 方案： 工作区分区、工作区级操作、搜索、置顶/归档、基于 shadcn 的对话框设计合理。两次修复提交解决了 IME Escape 处理、非主工作区请求分页大小、分组会话渲染、 3300 行的 — Qwen Code · qwen3.7-max |
|
Re-run — updated after fix commits ( Code ReviewThe two fix commits address all concerns raised in the previous review:
No new correctness bugs or security issues found in the fix commits. Test ResultsWeb Shell: 93/94 files passed, 1471/1478 tests passed. The 7 failures are all in Previously failing tests now pass: WebUI daemon session actions: 20/20 passed ✅ tmux testing: Not applicable — Web Shell is a browser-based UI requiring a daemon connection. Unit tests are the appropriate verification. 中文说明重新运行 — 修复提交后更新（ 代码审查两次修复提交解决了上一轮审查中提出的所有问题：
修复提交中未发现新的正确性 bug 或安全问题。 测试结果Web Shell： 93/94 文件通过，1471/1478 测试通过。 7 个失败全部在 WebUI daemon 会话操作： 20/20 通过 ✅ tmux 测试： 不适用 — Web Shell 是基于浏览器的 UI，需要 daemon 连接。单元测试是适当的验证方式。 — Qwen Code · qwen3.7-max |
|
Re-run — the author addressed all feedback from the previous review in two fix commits. Looking at the full picture again: this PR delivers a genuine, well-motivated feature. The workspace-aware sidebar solves a real usability problem — sessions were hard to find across workspaces, and creation/restoration flows defaulted to the primary workspace. The implementation is correct and the fix commits are thorough. The two fix commits addressed every concern raised previously: IME Escape handling (clever approach — masking the key for Radix without preventing the native event), explicit page sizes for secondary workspaces, grouped session rendering in both sidebar paths, My earlier reservations about the 3300-line Overall: the feature ships what it promises, the code is clean, the fix commits show responsiveness to review feedback, and the test coverage for the logic layer is solid. 中文说明重新运行 — 作者在两次修复提交中解决了上一轮审查的所有反馈。 再次审视全局：此 PR 交付了一个真实的、动机明确的功能。工作区感知的 Sidebar 解决了实际的可用性问题 — 跨工作区难以找到会话，创建和恢复流程默认使用主工作区。实现正确，修复提交很全面。 两次修复提交解决了之前提出的所有问题：IME Escape 处理（巧妙的方案 — 为 Radix 屏蔽按键但不阻止原生事件）、非主工作区的显式分页大小、两条 Sidebar 路径中的分组会话渲染、 之前关于 3300 行 整体：功能兑现了承诺，代码干净，修复提交展示了对审查反馈的积极响应，逻辑层的测试覆盖扎实。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — solid feature, well-implemented. Two minor test assertion updates needed (GitBranchIndicator title→tooltip, SessionOverviewPanel case match) but the code itself is correct. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review summary
Thanks for this — the workspace-aware session flow (create → restore → URL → embedding callback) is coherent and well thought through, and the daemon-side routing change is small, well-scoped, and tested. I traced the end-to-end multi-workspace path and it holds up. No Criticals. I've left two inline comments (a Medium on DialogShell stacked-focus restoration, and a dead-state cleanup in App.tsx); the rest is below.
🟡 Suggestion — the test deletion is broader than the stated "DOM-coupled" rationale
Removing brittle DOM-structural sidebar tests during rapid visual iteration is reasonable, but the deletion also drops:
DialogShellbehavioral contracts — Escape ordering across stacked shells, IME-composition guard, focus trap/restore.DialogShellis rewritten in this same PR, so this removes the safety net exactly where the implementation changed (see the inline Medium).- Non-DOM sidebar logic — group color normalization (hex/preset toggling), and the pin/archive concurrency guards (
does not drop organization actions … while one is busy,does not report organization failure when post-mutation reload fails). Pure helpers likenormalizeGroupColorInput/getGroupColorClass/isPrimaryWorkspaceCwdare unit-testable without any DOM.
Suggest porting the DialogShell behavioral tests and the sidebar's pure-logic tests before merge, and restoring DOM-level coverage later as the description promises.
🟢 Nit — breaking change not listed
WebShellApi.openSessionDrawer is removed. The description enumerates the onSessionIdChange signature change but not this one — please add it to the breaking-changes notes for embedders.
Verified during review (no action needed)
- Deleting
client/index.tsis safe: the real entry isclient/index.tsx(bothvite.lib.config.tsandtsconfig.build.jsonpoint there) and nothing imports the barrel. Its still-relevant exports were folded intoindex.tsx, which actually widens the exported surface. DialogPortaltargetsuseWebShellPortalRoot(), so embedded/shadow-DOM styling is preserved after droppingcreatePortal(document.body).onEscapeKeyDownpreserves the important semantics:defaultPrevented, IME guard (isComposing/keyCode 229), and topmost-stack gating.- Workspace routing is correct end-to-end:
createDetachedSession(workspaceCwd)honors the explicit cwd;restoreWorkspaceCwdpriority and the effect dep are correct; the standalone URL only carries?workspace=for secondary workspaces, matching theonSessionIdChangecontract. - The i18n keys used by the new components exist in both locales (note: keys are typed as
string, so locale gaps aren't caught bytypecheck).
Reviewed at dfb2615. I did not re-run build/typecheck/tests locally; relied on the reported green run plus static analysis.
dfb2615 to
6ab6e25
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] CSS classes .groupedSessionRow and .runningSession are referenced in WebShellSidebar.tsx:1959-1960 but were deleted from WebShellSidebar.module.css. CSS modules resolve missing class names to undefined; cx() silently drops undefined. Grouped sessions lose their indentation, muted color, and distinct hover styling. Running sessions lose visual differentiation. Fix: restore the CSS rules or remove the styles.* references.
[Critical] .headerActive in WorkspaceSection.module.css had background: var(--sidebar-accent); font-weight: 600; — both properties were removed, leaving an empty rule. The active workspace row now has no visual distinction from inactive rows. Fix: restore at least the background property.
[Suggestion] The deleted index.ts re-exported WebShellSidebarBranding, WebShellSidebarFooterItem, and WebShellSidebarFooterOptions. These three types are NOT re-exported from the new index.tsx, even though WebShellSidebarOptions (still exported) references them in its public API fields. External consumers importing these types by name get TS2305. This is an undocumented breaking change — add them back to the export type block.
[Suggestion] 2,877 lines of test coverage deleted (DialogShell.test.tsx at 482 lines + WebShellSidebar.test.tsx at 2,395 lines) with only 2 replacement tests added. The PR notes sidebar UI tests are deferred during visual iteration, but the deleted tests cover state-management behaviors (stacked-dialog Escape ordering via shellStack/isTopDialogShellId, archive/pin/export concurrency guards, workspace persistence negotiation, reload-token deduplication) that are independent of DOM structure and unlikely to break from visual changes.
— qwen3.7-max via Qwen Code /review
|
Addressed the latest review in commit ea9b8c9.\n\nBlocking fixes:\n- Preserved IME Escape through Radix without preventing the native event.\n- Added the workspace-specific load-failure title.\n- Applied explicit page sizes to secondary workspace session requests.\n- Restored grouped/running session styling and grouped rendering in both sidebar paths.\n- Restored the active workspace header state.\n- Re-exported the public sidebar branding/footer types.\n\nAlso applied the valid suggestions for translator/footer memoization, duplicate autofocus removal, guarded backdrop dismissal, resumeSession workspace forwarding, and README callback documentation.\n\nVerification:\n- Web Shell: 94 files, 1,478 tests passed.\n- DialogShell focused coverage: 4/4 passed.\n- WebUI session actions: 20/20 passed.\n- Full build, bundle, and typecheck passed.\n- Pre-commit Prettier and ESLint passed.\n\nThe large DOM-coupled Sidebar UI suite remains intentionally removed during rapid visual iteration, as documented in the PR. Focused behavioral tests are retained for stable dialog and session-routing contracts; comprehensive Sidebar UI coverage will be restored once the interaction/layout contract stabilizes. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| @@ -2287,36 +1961,31 @@ export function WebShellSidebar({ | |||
| session.isPinned && styles.pinnedSession, | |||
There was a problem hiding this comment.
[Suggestion] styles.pinnedSession is referenced here but the .pinnedSession class was deleted from WebShellSidebar.module.css. CSS modules resolve missing classes to undefined; cx() silently drops it. The pinned-session visual distinction (previously a subtle color tint) is lost without error. Either restore the CSS rule or remove this dead reference.
— qwen3.7-max via Qwen Code /review
|
|
||
| export interface SessionGroupSectionProps { | ||
| id: string; | ||
| label: string; |
There was a problem hiding this comment.
[Suggestion] id: string is declared in SessionGroupSectionProps and passed by callers (e.g., id={group:${group.id}}) but is never destructured or used inside the component. Either remove it from the interface (and update callers) or apply it to the <section> element for scroll-to/anchor targeting.
— qwen3.7-max via Qwen Code /review
| workspaceCwd: entry.cwd, | ||
| })); | ||
| }), | ||
| ).then((results) => { |
There was a problem hiding this comment.
[Suggestion] Rejected results from Promise.allSettled for secondary pinned sessions are silently filtered to empty arrays with no error state or logging. Compare with the archived sessions handler which tracks failures via setSecondaryArchivedError and logs with console.warn. Consider adding a similar error indicator so users know when pinned sessions from a secondary workspace failed to load.
— qwen3.7-max via Qwen Code /review
| mockComponent('./components/dialogs/ApprovalModeDialog', 'ApprovalModeDialog'); | ||
| mockComponent('./components/dialogs/ResumeDialog', 'ResumeDialog'); | ||
| mockComponent('./components/dialogs/ToolsDialog', 'ToolsDialog'); | ||
| mockComponent('./components/tools/ToolsManagerPage', 'ToolsManagerPage'); |
There was a problem hiding this comment.
[Suggestion] These mocks for ./components/tools/ToolsManagerPage and ./components/skills/SkillsManagerPage reference components that don't exist anywhere in the repo. No imports reference them. These are dead mocks — either remove them or add the corresponding components.
— qwen3.7-max via Qwen Code /review
| return { kind: 'list', path: dirPath, entries: [], truncated: false }; | ||
| } | ||
| const result = (await client.dirList(dirPath)) as { | ||
| kind: 'list'; |
There was a problem hiding this comment.
[Suggestion] The dirList() response is cast with as { kind: 'list'; ... } without any runtime validation, unlike the glob handler which defensively filters matches to strings. If the daemon returns a malformed response, downstream code could see unexpected undefined fields. Consider adding a runtime guard on entries similar to the glob pattern.
— qwen3.7-max via Qwen Code /review
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #6804 — feat(web-shell): modernize multi-workspace sidebar
Type: New Feature
Change size: +3141/-5170 across 32 files
HEAD: ea9b8c9d
Findings Summary
- Critical/Major: 0
- Minor: 1
- Nit: 0
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
Critical: CSS classes groupedSessionRow/runningSession deleted |
wenshao, qwen-code-ci-bot | Partially fixed — groupedSessionRow restored, but runningSession still has no standalone CSS definition (see below) |
| Critical: Stacked-dialog focus restoration broken | wenshao, doudouOUC | Fixed — scope nodes used directly, IME composition guard before preventDefault |
Critical: pageSize missing in secondary workspace archived call |
qwen-code-ci-bot | Fixed — all session list calls include SESSION_LIST_PAGE_SIZE |
Critical: Wrong i18n key daemon.loadFailed for workspace errors |
qwen-code-ci-bot | Fixed — uses workspace-specific workspace.loadFailed |
Critical: Grouped sessions not rendered with { grouped: true } |
qwen-code-ci-bot | Fixed — renderSessionRow(session, { grouped: true }) |
Critical: resumeSession doesn't forward workspaceCwd |
qwen-code-ci-bot | Fixed — accepts and forwards through startSessionSwitch |
Critical: IME preventDefault() before composition guard |
qwen-code-ci-bot | Fixed — composition checked first |
Critical: SessionOverviewPanel test case mismatch |
wenshao | Fixed |
Critical: GitBranchIndicator test assertion mismatch |
wenshao | Fixed |
Suggestion: forceMobileDrawer dead state |
doudouOUC | Fixed — removed |
Suggestion: footerItems not memoized |
qwen-code-ci-bot | Fixed |
| Suggestion: Autofocus duplicated | qwen-code-ci-bot | Fixed |
| Suggestion: Radix backdrop dismissal | qwen-code-ci-bot | Fixed |
| Suggestion: Workspace translator not memoized | qwen-code-ci-bot | Fixed |
Suggestion: README missing workspaceId param |
qwen-code-ci-bot | Fixed |
Remaining Finding
Minor: runningSession CSS class has no standalone definition (WebShellSidebar.module.css)
styles.runningSession is referenced at line 1962 of WebShellSidebar.tsx:
session.hasActivePrompt && styles.runningSession,But the CSS file only uses .runningSession in negation selectors (:not(.runningSession)) at lines 843-865 — there is no .runningSession { ... } definition. CSS modules resolve undefined classes to undefined; cx() silently drops it.
Impact: Running sessions get no distinct CSS treatment from this class. Additionally, the :not(.runningSession) selectors become :not() (matching everything), so running sessions incorrectly receive the hover/focus styles intended only for non-running sessions. Minor visual bug, not functional.
Fix: Add a .runningSession definition in the CSS module, or remove the reference from the TSX if the running-state styling was intentionally consolidated elsewhere.
Additional Audit Coverage
- Workspace-aware session flow: End-to-end path traced from sidebar workspace selection → composer workspace picker →
createAndAttachSessionForPromptwithworkspaceCwd→onSessionIdChange(sessionId, workspaceId)callback → URL persistence. The flow is coherent. onSessionIdChangebreaking change: SecondworkspaceId?parameter is additive (optional). Existing hosts that only destructure the first argument are unaffected. README updated.openSessionDrawerremoval: Breaking change for hosts usingWebShellApi.openSessionDrawer(). Noted in the PR description. Hosts should migrate to the new sidebar flow.- Test removal: Sidebar UI tests explicitly removed and deferred. Logic-level tests remain. Acceptable tradeoff during rapid visual iteration.
- Workspace git branch display: New
selectedWorkspaceGitBranchstate fetched viaworkspace.client.workspaceByCwd().workspaceGit(). Correctly scoped to pre-session state (cleared when session is active, uses connection's branch instead).
Final Verdict
COMMENT. 15 of 16 prior review findings verified as fixed at HEAD. One Minor remaining (runningSession CSS class). The workspace-aware session flow is well-designed and the multi-workspace plumbing is coherent through creation, restoration, URL, and embedding callbacks.
This review was generated by QoderWork AI
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed at ea9b8c9d (fix commits 6ab6e25c + ea9b8c9d). Thanks for the thorough follow-up — everything from my earlier review is addressed, with regression tests. 👍
Resolved (matching my earlier threads):
- ✅ DialogShell stacked-dialog focus restoration — now targets the scope node directly (
topPanel = scopes[scopes.length - 1]), with a regression test (restores focus to the remaining top shell when a lower shell unmounts). - ✅
forceMobileDrawerdead state — removed entirely (state, thecloseMobileDrawerreset, themobileDrawerForcedclass, and the toggle reset). - ✅ DialogShell behavioral coverage restored — IME Escape left unhandled, backdrop single-click close, drag-safe close, plus the stacked-focus test above.
- ✅
WebShellSidebarBranding/WebShellSidebarFooterItem/WebShellSidebarFooterOptionsare re-exported fromindex.tsx.
Two small, non-blocking items remain:
-
resumeSession(sessionId, { workspaceCwd })is currently a dead switch. The new option is forwarded correctly and covered by a unit test, but grepping call sites shows no production caller passes it (web-shell has noresumeSessioncall at all) — only the test sets it. It's harmless (correct fallback to the connection workspace) and gives API symmetry withloadSession, but if there's no near-term cross-workspace resume path, consider either wiring the caller or noting it's intentionally reserved. (loadSession'sworkspaceCwd, by contrast, is genuinely wired through the sidebar.) -
Sidebar non-DOM logic is still untested.
DialogShellcoverage is back, butWebShellSidebar.test.tsxremains fully removed, and this round reworked sidebar behavior again (grouped-row styling, running-session hover/focus selectors, secondarypageSize). Pure helpers likenormalizeGroupColorInput/getGroupColorClass/isPrimaryWorkspaceCwd, and the pin/archive concurrency guards, are unit-testable without any DOM — worth a small logic-only suite even while the visual layout keeps changing.
Nit (optional): the openSessionDrawer removal from WebShellApi still isn't listed in the breaking-changes notes.
Not blocking — LGTM once you're comfortable with the test-coverage tradeoff on the sidebar.
|
@chiga0 Thanks for the review and the cross-validation. I checked the remaining CSS Modules treats local classes referenced anywhere in a selector as module classes, including inside The class intentionally has no standalone declarations. Its purpose is to exclude active sessions from the normal hover/focus rules that hide the timestamp and reveal session actions. The running-state visual itself is provided by the Based on this, I believe the remaining Minor is a false positive and no code change is needed. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Approving. ✅ Re-verified at ea9b8c9d against green CI (ubuntu Test, web-shell E2E Smoke, review-pr all pass). Every substantive finding across the review rounds is resolved with regression tests, and no Criticals remain.
Resolved
- DialogShell stacked-dialog focus restoration — fix + regression test.
forceMobileDrawerdead state — removed.- DialogShell behavioral coverage — IME Escape, backdrop single-click, drag-safe close, stacked focus.
resumeSessionworkspace threading (+ test), secondary sessionpageSize, footer memoization, IME-Escape-across-Radix, backdrop pointer-down suppression, WorkspaceSessionProvider workspace-specific load-failure title, sidebar type exports re-added, READMEonSessionIdChange/onSessionCreated.
Open threads — all verified non-blocking Suggestions (fine to defer or fold into a follow-up):
SessionGroupSectionidprop is declared and passed by callers but never used (dead prop).styles.pinnedSessionis referenced (WebShellSidebar.tsx) but.pinnedSessionwas removed from the CSS module — resolves toundefined, so the per-row pinned tint is silently lost (.pinnedSessionListstill exists; purely cosmetic).- Secondary pinned-session load rejections are silently dropped (no error state/log), unlike the archived handler.
App.test.tsxmocks./components/tools/ToolsManagerPageand./components/skills/SkillsManagerPage, which don't exist in the repo (dead mocks).useComposerCorecasts thedirList()response without the runtime guard the siblingglobhandler uses.
Plus my two standing Lows: resumeSession's workspaceCwd has no production caller yet (test-only dead switch), and the sidebar's non-DOM logic (color normalization, pin/archive concurrency guards) remains untested.
None of these block merge. Nice work on the multi-workspace flow and the thorough follow-up.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-Review — Approving
Author's explanation of the runningSession CSS class is correct. CSS Modules exports classes referenced anywhere in a selector (including :not()) — styles.runningSession resolves to a valid hashed name, not undefined. It's an intentional marker class for negation selectors, with the running-state visual provided by .sessionLoading. My previous Minor finding was a false positive.
All other findings from prior reviews verified as fixed. LGTM.
Integrate main's sidebar restructure (QwenLM#6804) with the workspace runtime removal feature. The workspace removal UI is adapted to main's new headerActions callback pattern: the removal trigger is now a Trash2Icon button in the workspace header actions alongside group-create and new-task, rather than the PR's inline dropdown menu. The removal confirmation dialog and all removal state/callbacks are preserved in WebShellSidebar. WorkspaceSection and its CSS follow main's grouping and controlled-expansion structure unchanged. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
| ): void { | ||
| const url = new URL(window.location.href); | ||
| url.pathname = sessionId ? `/session/${encodeURIComponent(sessionId)}` : '/'; | ||
| if (sessionId && workspaceId) { |
There was a problem hiding this comment.
[Critical] A blank task in a secondary workspace emits an undefined session ID plus the workspace ID, but this URL update preserves workspace only when sessionId is truthy. The resulting navigation rewrites /?workspace=<secondary> to /, so refresh cannot restore the selected workspace. Preserve the workspace parameter independently of the session parameter.
— Codex $qreview via Qwen Code /review
| } | ||
| }, [connection.workspaceCwd, currentSessionId, displayedWorkspaces]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[Critical] Choosing New Task for the primary workspace represents primary as undefined, but clearSession() retains the previous secondary workspaceCwd. The no-session synchronization effect then selects that secondary workspace again, so the explicit primary action does not stick. Clear or explicitly replace the retained workspace when this action targets primary.
— Codex $qreview via Qwen Code /review
| : selectedWorkspaceCwd | ||
| } | ||
| workspaceSelectionDisabled={Boolean( | ||
| connection.sessionId, |
There was a problem hiding this comment.
[Critical] The workspace picker remains enabled while isPreparingPrompt is true and no session ID exists. The pending prompt has already captured its target workspace, so changing the picker during preparation makes the UI claim a different destination from the session that will be created. Disable workspace changes for the whole preparation interval.
— Codex $qreview via Qwen Code /review
| )} | ||
| <div className={styles.toolbarLeft}> | ||
| {workspaces && workspaces.length > 1 && onSelectWorkspace && ( | ||
| <Select |
There was a problem hiding this comment.
[Critical] Trust is not enforced consistently at the new workspace entry points: the composer selector offers untrusted workspaces, organized untrusted rows bypass the read-only renderer, and restored/embedded workspace IDs are mounted after checking existence only. Those paths expose actions that the daemon rejects for untrusted runtimes. Filter mutation targets by trusted and route untrusted targets through the read-only or recovery UI.
— Codex $qreview via Qwen Code /review
| `groupColor${color[0]!.toUpperCase()}${color.slice(1)}` as keyof typeof styles | ||
| ] | ||
| : styles.sessionGroupDotMuted; | ||
| const dotStyle: CSSProperties | undefined = color?.startsWith('#') |
There was a problem hiding this comment.
[Critical] Custom group colors are stored in --session-group-custom-color, but no stylesheet rule consumes that variable. Persisted custom colors therefore render with the neutral fallback. Apply the variable to the group marker or other intended visual element.
— Codex $qreview via Qwen Code /review
| session.isPinned && | ||
| styles.activeSessionActionButton, | ||
| )} | ||
| type="button" |
There was a problem hiding this comment.
[Suggestion] Session details moved into More Actions, but that menu is omitted for read-only rows. Untrusted/read-only sessions can no longer expose their full metadata. Keep a non-mutating details affordance available in read-only mode.
— Codex $qreview via Qwen Code /review
| > | ||
| <PlusIcon size={16} strokeWidth={1.2} /> | ||
| </button> | ||
| <button |
There was a problem hiding this comment.
[Suggestion] Secondary workspace New Task buttons remain enabled while creation is already in progress; the handler silently discards the click via creatingSessionRef. Mirror the primary button's disabled state so the UI communicates that the action is unavailable.
— Codex $qreview via Qwen Code /review
| string | undefined | ||
| >(undefined); | ||
| useEffect(() => { | ||
| if (connection.sessionId) { |
There was a problem hiding this comment.
[Critical] The selected-workspace branch is fetched only when the CWD changes. Later git_branch_changed events update connection.gitBranch, but the blank composer continues rendering the stale selectedWorkspaceGitBranch. Refresh or reconcile the selected branch when Git events arrive.
— Codex $qreview via Qwen Code /review
| <button | ||
| className={styles.projectsHeaderToggle} | ||
| type="button" | ||
| aria-expanded={projectsExpanded} |
There was a problem hiding this comment.
[Critical] Collapsing Projects hides every workspace/result but leaves the search input visible and editable. Users can enter a query and see no results or indication that results are hidden. Hide/disable search with the section or automatically expand Projects when searching.
— Codex $qreview via Qwen Code /review
| const client = workspace.client.workspaceByCwd(atWorkspaceCwd); | ||
| workspaceActionsRef.current = { | ||
| ...workspace.actions, | ||
| async globWorkspace(pattern, options) { |
There was a problem hiding this comment.
[Suggestion] The qualified glob adapter drops the caller's maxResults. The daemon may scan and transfer up to its 5,000-result default before the client slices to 50, making large secondary workspaces unnecessarily expensive. Forward the requested limit.
— Codex $qreview via Qwen Code /review
5 participants
What this PR does
Why it's needed
The previous sidebar made the relationship between projects, workspaces, and sessions difficult to scan, while several session flows still assumed the primary workspace. That could make creating or reopening a task in a secondary workspace confusing and could route requests through the wrong workspace runtime.
This change gives the sidebar a consistent interaction model and carries workspace identity through creation, restoration, URLs, and embedding callbacks. It also removes bespoke menu and dialog behavior where the existing shared component foundation already provides consistent focus, portal, keyboard, and theme handling.
Reviewer Test Plan
How to verify
onSessionIdChange(undefined, undefined).Automated verification completed locally:
npm run build && npm run bundle && npm run typecheckEvidence (Before & After)
Before: the sidebar used a flatter bespoke layout and did not provide a complete workspace-aware creation and restoration flow.
After: the sidebar groups sessions by workspace, exposes workspace-scoped actions and state, and keeps the selected workspace consistent from the first prompt through URL restoration. Local visual captures were produced for the multi-workspace sidebar, dark theme, session menu, and composer workspace selector; they are not committed to the source tree.
Tested on
Environment (optional)
Local Web Shell connected to a daemon with primary and secondary workspaces; automated builds and tests ran with the repository's Node.js toolchain.
Risk & Scope
WebShellApi.openSessionDrawer()is removed with the forced compact-drawer behavior. Embedded multi-workspace consumers can passworkspaceIdand should persist both values emitted byonSessionIdChange(sessionId, workspaceId). Hosts do not need additional shadcn or Tailwind configuration.Linked Issues
N/A
Pictures
中文说明
本 PR 做了什么
为什么需要
旧 Sidebar 中项目、工作区和会话之间的关系不够直观，同时部分会话流程仍默认使用主工作区。这会让用户难以判断任务应创建或恢复到哪个工作区，也可能让非主工作区请求被错误路由。
本次改动统一了 Sidebar 的交互模型，并让工作区身份贯穿创建、恢复、URL 和嵌入回调。对于菜单和对话框，也复用现有共享组件基础，以统一焦点、Portal、键盘和主题行为。
Reviewer 测试计划
如何验证
onSessionIdChange(undefined, undefined)。本地已完成以下自动化验证：
npm run build && npm run bundle && npm run typecheck证据（Before & After）
Before：旧 Sidebar 使用更扁平的定制布局，缺少完整的工作区感知创建与恢复流程。
After：新 Sidebar 按工作区组织会话，提供工作区级操作和状态，并让所选工作区从首次输入一直保持到 URL 恢复。本地已保存多工作区 Sidebar、深色主题、会话菜单和输入框工作区选择器截图；截图未提交到源码仓库。
测试平台
环境（可选）
本地 Web Shell 连接包含主工作区和非主工作区的 daemon；自动化构建和测试使用仓库指定的 Node.js 工具链。
风险与范围
WebShellApi.openSessionDrawer()及其强制紧凑抽屉行为已移除。嵌入式多工作区接入方可以传入workspaceId，并应同时持久化onSessionIdChange(sessionId, workspaceId)返回的两个值。宿主无需额外配置 shadcn 或 Tailwind。关联 Issue
N/A