feat(web-shell): hide sources panel add-source button behind URL flag - #11639
Conversation
The "+" button in the sources panel header serves no practical purpose in the current product flow, so it is now hidden by default. The code path is kept intact and can be re-enabled by appending ?addSource=1 to the Web Shell URL, following the existing ?composer= escape-hatch pattern. Existing EnvironmentPanel tests opt into the flag in a beforeEach; new collocated tests cover both the hidden default and the enabled state.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
What this PR does / PR 主旨
EN: Hides the "+" (add-source) button in the Web Shell sources-panel header by default, gating it behind a ?addSource=1 URL parameter while keeping the whole add-source dialog code path intact. The gate is a single extra render condition state?.supported && isAddSourceEnabled(); isAddSourceEnabled() reads window.location.search with an SSR guard. Existing panel tests opt into the flag via a shared beforeEach, and new collocated tests assert hidden-by-default and shown-with-flag.
中文：默认隐藏 Web Shell 来源面板头部的 "+"（添加来源）按钮，改为通过 URL 参数 ?addSource=1 开启，同时完整保留添加来源对话框的代码路径。开关是一处额外的渲染条件 state?.supported && isAddSourceEnabled()；isAddSourceEnabled() 带 SSR 守卫地读取 window.location.search。现有面板测试通过共享的 beforeEach 开启该开关，新增随源测试断言"默认隐藏"和"带开关显示"两种状态。
Verification / 核对（static only, head 23780bd6）
- The gate is sound and no orphan dialog path remains.
setAdding(true)is reachable only from the button'sonClick(SourcesSection.tsx:107-115); with the button hidden,addingstaysfalse, so theDialogShelland theonDialogOpenChange?.(adding && state?.supported === true)effect can never fire from a hidden UI. Grepped the package: the only non-test references tosources.add/ the button arei18n.tsx(key definition) andSourcesSection.tsxitself — theactions.tshit is an unrelated"Add source timed out"string. No e2e/.spec.tsexercises the sources panel, so nothing regresses there. - Test assertion matches the rendered label. The button's
aria-labelist('sources.add'), which resolves to"Add source"in EN (i18n.tsx:2892), sobutton[aria-label="Add source"]in the new test is correct; the ZH value (添加来源, i18n.tsx:6385) is present too. - The shared
beforeEachis necessary and does not weaken existing assertions. The three dialog tests (EnvironmentPanel.test.tsx:820/853/891) query the button and.click()it, so they genuinely need?addSource=1now that the flag is ANDed into the gate. The twotoBeNull()assertions (646, 935) live in render paths wherestate?.supportedis already false, so turning the flag on cannot resurrect the button there — they stayed valid before and after.afterEachresets the URL to/, keeping per-test isolation clean. - Convention consistency.
new URLSearchParams(window.location.search).get('…')matches the established escape-hatch pattern in App.tsx (2247/2256), main.tsx (94/136/148/153), and config/daemon.ts (5/73). Reading it non-reactively at render is consistent with those call sites and appropriate for a load-time switch. - Cross-PR check. The author's other open PR touching this area (PR 11641, "move source download to the header") modifies
ArtifactPanel.tsx/SourcePreview.test.tsx, notSourcesSection.tsx— no file-level conflict or shared-literal collision between the two.
No Critical and no Important issues found.
Non-blocking note / 非阻塞小点（Nit）
- The
"is hidden by default"test relies on the ambient URL being clean rather than setting it explicitly at the start of the test (it's first in the file andafterEachresets to/, so it's correct today). For order-independence you could add an explicitwindow.history.replaceState({}, '', '/')at the top of that case — purely defensive, no current failure.
CI status at review time / 审查时的 CI 状态
Honest note: at review time on head 23780bd6, Lint & Static, Integration Tests (no-AK) and both Desktop Shell lanes were green, but Test (ubuntu-latest, Node 22.x) (which runs the new SourcesSection.test.tsx) and Capture web-shell visuals were still pending. This approval rests on static verification of the head tree; recommend confirming the Test lane lands green before merge.
如实说明：审查时 head 23780bd6 上 Lint & Static、Integration Tests (no-AK) 与两个 Desktop Shell lane 已通过，但跑新增 SourcesSection.test.tsx 的 Test (ubuntu-latest, Node 22.x) 与 Capture web-shell visuals 仍在 pending。此批准基于对 head tree 的静态核验；建议合并前确认 Test lane 变绿。
Verdict / 结论: ✅ Approve — a minimal, well-scoped, convention-consistent UI gate with correct test coverage; no blocking issues. Merge should wait for the pending Test lane to confirm green.
|
Thanks for the PR — and for redoing this against Template looks good ✓ — every required section is filled in, and the Chinese translation tracks the English paragraph for paragraph. Problem: this isn't a bug fix, so there's nothing to reproduce — it's a product judgment, and the description is honest about that. One thing worth putting on the record for the maintainer: the add-source button shipped in v0.23.3 (2026-09-10) via #11262, so this gates a user-facing entry point roughly a day after it landed. There's no linked issue, no design doc, and no user feedback cited. You authored #11262, so you know this surface better than anyone reviewing it — but "serves no practical purpose in the current product flow" is an assertion a maintainer will want to confirm rather than take on faith. Direction: plausible and in scope. Web Shell UI decluttering touches no auth, sandbox, model-selection, telemetry, release, or public-contract surface, and the reference CHANGELOG has no direct signal either way. My reservation isn't about whether to declutter — it's about how. See Approach. Size: core paths are not touched (everything sits under Approach: the implementation is clean, but I'd like a maintainer to settle flag-vs-delete, because that's where I think this PR is genuinely arguable rather than merely stylistic. AGENTS.md puts Simplicity First at the top of its working principles — "nothing speculative", "no flexibility or configurability that wasn't requested" — and the stated rationale for keeping the code is speculative future use ("may still be wanted for debugging or future iterations"). Concretely, hiding the button leaves the ~110-line To be clear, that's a question, not a defect. The Risk: no elevated risk signals. None of the changed files match the revert-correlated paths from the high-risk screen, the blast radius is one render condition, and the change is trivially reversible. Flagging the flag-vs-delete question for a maintainer before this goes further. Code review and CI evidence follow below. 🔍 中文说明感谢贡献！也感谢你在 #11632 撞上已下线的 desktop 目录之后，重新对 模板完整 ✓ —— 所有必填章节都写了，中文说明与英文逐段对应。 问题： 这不是 bug fix，所以没有可复现的东西——它是一个产品判断，PR 描述对此也很坦诚。有一点需要记录给 maintainer：这个"添加来源"按钮是刚刚通过 #11262 随 v0.23.3（2026-09-10） 发布的，也就是说本 PR 在功能上线约一天后就把这个面向用户的入口关掉了。没有关联 issue、没有设计文档、也没有引用用户反馈。#11262 是你写的，你比任何评审者都更了解这块——但"在当前产品流程中没有实际意义"这个判断，maintainer 大概会想自己确认一下，而不是直接采信。 方向： 合理且在范围内。Web Shell 的界面精简不涉及 auth、sandbox、模型选择、telemetry、发布或公开契约，参考仓库的 CHANGELOG 也没有直接信号。我的保留意见不在于"要不要精简"，而在于"怎么精简"，见下面的"方案"。 规模： 未触及核心路径（改动都在 方案： 实现本身很干净，但"加开关还是直接删"这一点我希望由 maintainer 来定，因为这里我认为是有实质争议的，而不只是风格问题。AGENTS.md 把 Simplicity First 放在工作原则的第一位——"不做投机性设计"、"不引入未被要求的可配置性"——而保留这段代码的理由恰恰是投机性的未来用途（"将来调试或迭代时可能仍需要"）。具体来说，隐藏按钮之后，约 110 行的 需要说明的是，这是一个疑问，不是缺陷。 风险： 无升级风险信号。改动文件均未命中高风险路径筛查中与 revert 相关的模式，影响面只是一个渲染条件，且极易回滚。 在继续推进之前，先把"加开关还是直接删"这个问题提给 maintainer。代码审查与 CI 证据见下方。🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo correctness bugs, no security concerns, and no regressions that I can find. The production change is one extra term in a render condition, and it follows the existing
Non-blocking observations, none of which I'd hold the PR for:
Test evidenceThis is an unattended CI run, so nothing here was built or executed locally — the evidence below is the PR's own CI, read from the checks API at the reviewed commit. At the time of writing the suite had not finished: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Outside the table: Two caveats on reading this as evidence. Sandboxed verification would settle what static review can't: 中文说明代码审查： 没有发现正确性问题、安全隐患或回归。生产代码改动只是在一个渲染条件上多加了一项，而且与既有的 新测试不会"空过"。
"面板其他部分不受影响"是准确的。头部按钮是唯一的 没有 e2e 或视觉回归。"现有 e2e 没有用例涉及来源面板"这一说法核实无误：唯一的引用是
约定方面没有问题。 非阻塞的观察（都不足以卡住这个 PR）：参数在每次渲染时都会重新解析，而 测试证据： 本次为无人值守 CI 运行，未在本地构建或执行任何 PR 代码——以上证据来自被审查提交上 PR 自身的 CI，通过 checks API 读取。撰写时测试套件尚未跑完： 表格之外： 关于证据有两点需要说明： 沙箱验证可以补上静态审查补不了的部分： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — clean implementation with no defects found at any stage; the single question I couldn't settle from the diff has been settled by a maintainer on this exact commit, and what remains is non-blocking. This comment reverses an earlier verdict, so the reasoning is worth stating plainly. My first pass through this PR landed at 3/5 and deferred, for one reason: the code was clean, but choosing between a permanent URL flag and outright deletion is a product call, and with no issue, no design doc, and no user feedback behind it I didn't think it was mine to make. While that pass was running, @ytahdn — who has write access — reviewed and approved commit On the merits, my read of the code didn't change and still holds up. The gate reuses a real in-repo precedent instead of inventing a mechanism, the tests are paired so neither direction can pass vacuously, and the things that usually break in this shape of change — a param silently stripped by URL rewriting, a selector matching nothing, a second entry point left dangling, an e2e spec clicking the now-hidden button — I checked each one and none of them bite. Nine production lines, well tested, trivially reversible. Named non-blocking notes, recorded once and not relitigated:
CI at the time of this update: ⏳ Not approving in this run. The unit suite is still in flight on the reviewed commit, and approving now would attest to a result that doesn't exist yet. Approval is deferred until CI lands green on 中文说明信心度：4/5 —— 实现干净，各阶段都未发现缺陷；我唯一无法从 diff 判断的问题，已由 maintainer 在同一个提交上给出结论，剩下的都是非阻塞项。 本评论推翻了先前的结论，所以有必要把理由讲清楚。 我第一轮审查给的是 3/5 并选择转交，原因只有一个：代码本身没问题，但"用永久 URL 开关还是直接删除"属于产品决定，而这个 PR 背后没有 issue、没有设计文档、也没有用户反馈，我不认为这个决定该由我来做。就在这一轮运行期间，@ytahdn（具备 write 权限）审查并批准了提交 就代码本身而言，我的判断没有变化，而且依然站得住。这个开关复用了仓库里真实存在的先例，而不是自造机制；测试成对出现，因此任一方向都不会"空过"；这类改动通常会出问题的几个点——参数被 URL 改写悄悄清掉、选择器什么都匹配不到、留下第二个悬空入口、某个 e2e 用例去点已被隐藏的按钮——我逐一查过，一个都没踩中。9 行生产代码，测试充分，且极易回滚。 以下非阻塞意见记录一次，不再反复：
截至本次更新的 CI 状态： ⏳ 本次运行不批准。 被审查提交上的单元测试仍在进行，此刻批准等于为一个尚不存在的结果背书。批准推迟到 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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 — CI landed green after the review. ✅
3 participants
What this PR does
Hides the "+" button in the header of the Web Shell sources panel by default. The button's code path — including the add-source dialog it opens — is kept intact: appending the URL parameter
?addSource=1brings it back, following the same escape-hatch pattern the Web Shell already uses for switches like?composer=. Everything else in the panel (source list, attachments, empty state, view-all toggle) is unchanged. Existing panel tests opt into the flag in a shared setup hook, and new collocated tests cover both the hidden default and the enabled state.Why it's needed
The plus button in the sources panel header serves no practical purpose in the current product flow, but deleting the code outright would throw away a working add-source interaction that may still be wanted for debugging or future iterations. Hiding it behind a URL flag declutters the default UI while keeping the behavior one parameter away.
Reviewer Test Plan
How to verify
?addSource=1appended to the URL: the "+" button reappears and opens the add-source dialog as before.cd packages/web-shell && npx vitest run client/components/panels/SourcesSection.test.tsx client/components/panels/EnvironmentPanel.test.tsx— all 38 tests pass.Evidence (Before & After)
New collocated tests render the panel in jsdom and assert the button is absent by default and present with the flag; the full existing environment-panel suite (36 tests) passes unchanged against the enabled state.
tsc --noEmitfor the package is clean, and eslint passes on the touched files.Tested on
Environment (optional)
jsdom component tests via the package vitest config; Web Shell is browser- and OS-independent for this change.
Risk & Scope
?addSource=1to restore it.Linked Issues
N/A — supersedes the closed #11632, which targeted the retired Electron desktop package.
中文说明
本 PR 做了什么
默认隐藏 Web Shell 来源面板头部的 "+" 按钮。按钮的代码路径——包括它打开的添加来源对话框——完整保留：在 Web Shell URL 后追加参数
?addSource=1即可恢复显示，沿用了 Web Shell 已有的?composer=等逃生开关模式。面板的其他部分（来源列表、附件、空状态、查看全部开关）均不受影响。现有面板测试在共享的 setup 钩子中开启该开关，新增的随源测试同时覆盖默认隐藏和开启两种状态。为什么需要
来源面板头部的加号按钮在当前产品流程中没有实际意义，但直接删除代码会丢掉一个可用的添加来源交互，将来调试或迭代时可能仍需要。用 URL 开关把它隐藏起来，既让默认界面更干净，又让该行为只需一个参数即可找回。
评审者验证计划
如何验证
?addSource=1刷新："+" 按钮重新出现，且照常打开添加来源对话框。cd packages/web-shell && npx vitest run client/components/panels/SourcesSection.test.tsx client/components/panels/EnvironmentPanel.test.tsx——38 个测试全部通过。证据（前后对比）
新增的随源测试在 jsdom 中渲染面板，断言按钮默认不存在、带开关时存在；现有环境面板测试套件（36 个）在开启开关的状态下全部原样通过。该包的
tsc --noEmit无错误，改动文件通过 eslint。已测试平台
macOS ✅ 已测试；Windows⚠️ 未测试；Linux ⚠️ 未测试（本改动为浏览器端条件渲染，与操作系统无关）。
环境（可选）
通过包的 vitest 配置运行 jsdom 组件测试。
风险与范围
?addSource=1即可恢复。关联 Issue
无——替代已关闭的 #11632（该 PR 误投向已移除的 Electron 桌面包）。