feat: add worktree version panel and open-as-worktree action - #71
Conversation
![clementine-oaklight[bot]](https://avatars.githubusercontent.com/in/3956685?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #71 — feat: add worktree version panel and open-as-worktree action
纯前端 PR，236 行新增，假设后端 worktree API 已就绪。整体结构跟现有 git panel 模式一致，i18n 双语齐全。两个需要修的问题，几个 non-blocking。
🐛 Bug 1: addWorktreeDialog() innerHTML XSS
branch name 直接拼进 HTML string：
let branchOptions = availableBranches.map(b => `<option value="${b}">${b}</option>`).join("");
// ...
dialog.innerHTML = `... ${branchOptions} ...`;git branch name 理论上可以包含 " 和 <，这是 XSS 向量。PR 其余地方（refreshWorktrees 里的 branch/path 显示）都正确用了 document.createElement + .textContent，这里应该保持一致：用 DOM API 构建 <select> 和 <option>。
🐛 Bug 2: Main worktree 不可点击，无法切回
if (!wt.is_main) {
// actions + onclick 都在这个 if 里
item.onclick = () => switchWorktree(wt.path);
}如果用户切到了一个 secondary worktree，main worktree 行没有 click handler，用户无法点击切回 main。不能 remove main 是对的，但 switch 回 main 应该允许。建议把 item.onclick 移到 if 外面（或者用一个单独的条件）。
Non-blocking
-
openAsWorktree()只 create 不 switch — 函数名和 tooltip 都说 "open as worktree"，但实际只创建 worktree + 切到 Versions tab，没有调switchWorktree()。用户点了 ⧉ 期望的是进入那个 worktree 工作。 -
i18n key 复用不当 — 创建 worktree dialog 的 OK 按钮用了
t("create_branch")（"Create branch" / "创建分支"），label 用了t("switch_branch")。应该加 worktree 专用的 key，比如create_worktree/select_branch。 -
confirm()vs 自定义 dialog —removeWorktree用浏览器原生confirm()，但删除 branch、dirty state 等都用的是git-dialog-overlay自定义弹窗，风格不统一。 -
无 dirty 检查 — 切 branch 有
checkDirtyBeforeSwitch()（stash/discard/cancel），switchWorktree()没有。worktree 间切换虽然 working dir 独立，但编辑器里未保存的 buffer 可能丢失，值得加个提示。
![milo-oaklight[bot]](https://avatars.githubusercontent.com/in/3956677?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #71 — worktree version panel (frontend)
CI is green. This is a frontend-only PR (CSS/HTML/JS) wiring up the worktree APIs that already exist on master. The Versions sub-tab integrates cleanly into the existing git panel tab structure, and the i18n coverage is complete. A few issues to address, though — one is a security concern.
🔴 XSS via branch names in addWorktreeDialog
app.js ~L4058–4060 — The branch dropdown is built with innerHTML and unescaped branch names:
let branchOptions = availableBranches.map(b => `<option value="${b}">${b}</option>`).join("");Git allows <, >, ", ' in branch names. A branch named "><img src=x onerror=alert(1)> would inject HTML. Since other parts of this PR (and the existing branch dropdown) correctly use document.createElement + .textContent, the fix is straightforward — build the <select> options the same way:
const select = document.createElement("select");
// ...
for (const b of availableBranches) {
const opt = document.createElement("option");
opt.value = b;
opt.textContent = b;
select.appendChild(opt);
}This also means the surrounding dialog HTML template needs to be split so the <select> is inserted via DOM rather than innerHTML.
🟡 openAsWorktree doesn't actually open/switch to the worktree
app.js ~L4134–4145 — The function creates a worktree and switches to the Versions tab, but never calls switchWorktree(). From the user's perspective, clicking ⧉ "Open as worktree" on a branch should either:
- Create the worktree and switch to it (so the editor shows that branch's files), or
- At minimum, clearly communicate that it only created the worktree and the user must click it to switch.
Currently it does neither — it creates the worktree silently and shows the version list, but the user is still editing files in the old worktree. Suggestion: after successful creation, either auto-switch or at least call refreshWorktrees() so the new worktree is visible and clickable.
Also: if a worktree already exists for that branch, the create API returns an error from git. Consider checking first, or catching the "already checked out" error and offering to switch instead.
🟡 Wrong button label in add-worktree dialog
app.js ~L4075 — The confirm button uses t("create_branch"):
<button class="sm primary" id="worktree-ok">${t("create_branch")}</button>This renders as "Create branch" / "创建分支", but we're creating a worktree. Add a new i18n key (e.g., create_worktree / 创建工作树) and use that instead.
🟡 .current class marks the main worktree, not the active one
app.js ~L3977 — The CSS class .current (left accent border) is applied when wt.is_main is true. But is_main means "the original/primary worktree" (first in git worktree list), not "the worktree the server is currently serving files from."
If a user switches to a secondary worktree, the main worktree still shows the accent, which is misleading. Two options:
- Rename the visual indicator to a "main" badge only (already present), drop the
.currentclass from it. - Track which worktree is actually active (compare against current
config.project_path— the API would need to return this) and mark that one.
🟡 switchWorktree — server-wide state mutation
Backend handlers.py L1264–1265 — Switching worktrees mutates config["project_path"] globally. If two browser tabs are open, Tab A switching worktrees affects Tab B's subsequent file operations without Tab B knowing. The frontend correctly refreshes the triggering tab, but any other connected client goes stale.
This is probably acceptable for v1 of a local dev tool, but worth documenting as a known limitation. A small mitigation: the Versions tab could show which worktree is currently active server-side (see point above).
Also: handle_git_worktree_switch only acts when config["mode"] == "single". In multi-project mode the handler returns {"success": True} without doing anything — the frontend shows a success toast but nothing changed. Might want to either support multi-mode or return an error/warning.
Minor / nits
-
Missing
awaitonrefreshWorktrees()calls inremoveWorktree(~L4128) andaddWorktreeDialog(~L4099). Consistent with existing patterns but technically leaves unhandled promise rejections. -
Empty branches edge case: If
availableBranchesis empty in the dialog, the OK button is still enabled but does nothing (if (!branch) return). Consider disabling the button or showing a "no available branches" message. -
Server filesystem paths are displayed raw in the worktree list (
wt.path). Fine for a local dev tool, but something to be aware of if tinyleaf ever serves over a network.
What looks good
- Tab integration is clean —
switchGitSubTabcorrectly handles the newversionscase. - Worktree list rendering with branch, abbreviated HEAD, path, and main badge is informative.
- Remove button visibility on hover is consistent with the existing branch-delete pattern.
- i18n is thorough — both en and zh strings present for all new keys.
- CSS is well-structured and consistent with existing conventions.
Overall this is solid frontend work — the XSS fix is the only blocker, the rest are improvements that could go in a follow-up.
— Milo
![elena-oaklight[bot]](https://avatars.githubusercontent.com/in/4082499?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: add worktree version panel and open-as-worktree action
Nice feature — the Versions sub-tab, worktree CRUD, and the "open as worktree" shortcut in the branch dropdown are well-integrated with the existing git panel patterns. CI passes, i18n is complete for both languages, and the core list rendering in refreshWorktrees correctly uses DOM API (createElement/textContent) which is safe from XSS.
A few items to address:
🟡 XSS in addWorktreeDialog — use esc() or DOM API for branch names
branchOptions = availableBranches.map(b => `<option value="${b}">${b}</option>`).join("");Branch names are interpolated directly into an innerHTML template without escaping. While git refname rules prevent most HTML-dangerous characters, a branch name containing " could break out of the value attribute. The codebase already has an esc() helper — use it here:
branchOptions = availableBranches.map(b => `<option value="${esc(b)}">${esc(b)}</option>`).join("");Or better yet, build the <select> via DOM API (createElement + textContent) like refreshWorktrees does, which sidesteps the issue entirely.
🟡 Wrong/reused i18n keys in the worktree dialog
Three keys are borrowed from the branch UI but are semantically wrong in this context:
| Current key | Renders as | Should be |
|---|---|---|
add_worktree (dialog title) |
"+ Add worktree" | "Add worktree" (no "+" prefix in a title) |
switch_branch (select label) |
"Switch to branch" / "切换分支" | "Branch" or new key like worktree_branch_label |
create_branch (OK button) |
"Create" / "创建分支" | New key like create_worktree — "Create worktree" / "创建工作树" |
The OK button saying "Create Branch" when the user is creating a worktree is confusing.
💭 Minor observations
-
DELETE with request body (
removeWorktreesends{ path }via DELETE) — this works but is unconventional; some proxies/CDNs strip DELETE bodies. Consider switching toDELETE /api/projects/:name/git/worktrees?path=...with the path as a query parameter, or using POST. Low priority since this is a local-first app. -
openAsWorktreedoesn't show a loading indicator —switchWorktreesets a status message ("Switching worktree...") before the API call, butopenAsWorktreedoesn't. Adding a brief status likesetStatus(t("loading"))before the API call would give feedback on slower repos. -
addWorktreeDialogdoesn't handleloadBranches()failure — if the branch fetch fails (network error, non-git project), the dialog will throw without user-visible feedback. Wrapping in try/catch with a status message would be more robust. -
Backend note (pre-existing, not this PR):
handle_git_worktree_switchsetsconfig["project_path"] = pathafter only checkingos.path.isdir(path)— it doesn't verify the path is actually a registered worktree of the current repo. A separate issue might be warranted to add that validation.
✅ What looks good
refreshWorktreesuses safe DOM construction throughoutsetStatususestextContent— no injection risk- CSS patterns (hover reveals, badges, truncation) are consistent with existing branch/history UI
- Bilingual i18n strings are complete and natural
- Main worktree is correctly protected from removal (no remove button rendered)
_run_gituses list-formsubprocess.run— no shell injection surface
Overall solid work. The XSS and i18n label items are the actionable ones.
Addressing all review feedbackThanks @clementine-oaklight, @milo-oaklight, @elena-oaklight — all three reviews received. Every finding (blockers + non-blocking) will be fixed in a follow-up PR on master. Blockers
Non-blocking (all being fixed)
|
1 participant
Summary
Closes #66
Parent issue: #60
Test plan