fix: address review feedback from PRs #67, #68, #71 by Oaklight · Pull Request #72 · Oaklight/tinyleaf

Oaklight · 2026-09-13T18:18:46Z

Summary

Comprehensive follow-up addressing all outstanding review feedback from PRs #67, #68, and #71 (all already merged into master).

Backend: `src/tinyleaf/git_ops.py`

list_branches(): fix detached HEAD leak — lstrip("* ") was a char-set strip, not prefix removal. Replaced with removeprefix("* ") (Python 3.10+ is required). Also filter lines starting with ( to skip (HEAD detached at ...) entries. (PR feat: add branch listing and fetch to git_ops + API endpoint #67 review)
worktree_add(): add -- separator — Prevents paths starting with - from being parsed as flags. (PR feat: add worktree operations and project_dir hot-swap #68 review)
worktree_remove(): fix --force placement + add -- — --force was appended after the path; moved it before -- separator for correct git argument order. (PR feat: add worktree operations and project_dir hot-swap #68 review)
worktree_list(): flush last entry after loop — If the porcelain output doesn't end with a blank line, the last worktree was silently dropped. (PR feat: add worktree operations and project_dir hot-swap #68 review)

Backend: `src/tinyleaf/handlers.py`

handle_git_branches(): surface fetch result — Previously git fetch ran but its result was discarded. Now returns fetch key in the response so the UI can display fetch status. (PR feat: add branch listing and fetch to git_ops + API endpoint #67 review)
handle_git_worktree_switch(): validate git repo + reject multi-mode — Added explicit check that the path is a git repo/worktree, and early-reject in multi-project mode instead of silently doing nothing. (PR feat: add worktree operations and project_dir hot-swap #68 review)

Frontend: `src/tinyleaf/static/js/app.js`

XSS fix in addWorktreeDialog() — Replaced innerHTML-based branch <option> injection with document.createElement DOM API. Branch names are now set via textContent, preventing script injection through malicious branch names. (PR feat: add worktree version panel and open-as-worktree action #71 review, flagged by all three reviewers)
Main worktree now clickable — Moved item.onclick outside the if (!wt.is_main) block so the main worktree entry also triggers switchWorktree(). (PR feat: add worktree version panel and open-as-worktree action #71 review)
openAsWorktree() auto-switches — After creating a worktree, now calls switchWorktree(result.path) to immediately activate it instead of just showing the versions tab. (PR feat: add worktree version panel and open-as-worktree action #71 review)
removeWorktree(): custom dialog — Replaced native confirm() with the project's git-dialog-overlay pattern for consistent UX. (PR feat: add worktree version panel and open-as-worktree action #71 review)
switchWorktree(): unsaved-buffer check — Before switching worktrees, checks S.modified for unsaved editor changes and shows a Save/Discard/Cancel dialog. (PR feat: add worktree version panel and open-as-worktree action #71 review)
Missing await on refreshWorktrees() — Added await in both addWorktreeDialog and removeWorktree to ensure the worktree list is fully refreshed before continuing. (PR feat: add worktree version panel and open-as-worktree action #71 review)
New i18n keys — Added create_worktree, select_branch, unsaved_changes, save_and_continue, discard_and_continue in both en and zh blocks. (PR feat: add worktree version panel and open-as-worktree action #71 review)

Test plan

clementine-oaklight

Solid fix PR — XSS、path validation、dialog 统一都做对了。一个实际 bug，一个 medium：

🔴 Bug: `handle_git_branches()` fetch 顺序反了

result = git_ops.list_branches(project_dir)      # ← 先 list
if query_params.get("fetch", [""])[0] == "true":
    fetch_result = git_ops.fetch(project_dir)     # ← 后 fetch
    result["fetch"] = fetch_result
return result

老代码是 fetch → list，新代码变成 list → fetch。?fetch=true 的意图是拿最新的 remote branches，但现在 list 在 fetch 之前执行，返回的 remote branch 列表是 stale 的。fetch result 挂上了（好），但 branch list 本身没更新。

修法：调回来，先 fetch 再 list：

fetch_info = None
if query_params.get("fetch", [""])[0] == "true":
    fetch_info = git_ops.fetch(project_dir)
result = git_ops.list_branches(project_dir)
if fetch_info is not None:
    result["fetch"] = fetch_info
return result

🟡 Medium: unsaved-buffer check 只看当前文件

if (S.currentFile && S.modified.has(S.currentFile)) {

S.modified 可以包含多个文件。如果用户改了文件 A 和 B，当前看的是未修改的文件 C，check 通过，worktree switch 后所有未保存的改动静默丢失。建议改成 S.modified.size > 0，dialog 文案也更准确。

其余全部正确：

XSS fix ✅ — addWorktreeDialog 从 innerHTML 完全改成 createElement + textContent，和 codebase 其余部分一致。
removeprefix("* ") ✅ — 正确修了 #67 的 char-set stripping bug，加 startswith("(") 过滤 detached HEAD 也对。
worktree_list flush ✅ — porcelain output 不以空行结尾时不再丢最后一个 entry。
-- separator ✅ — worktree_add 和 worktree_remove 都加了，--force 位置也修正了。
worktree_switch validation ✅ — multi-mode 400 reject + has_git() check，docstring 标明 ephemeral。
confirm() → custom dialog ✅ — removeWorktree 用 git-dialog-overlay，UX 统一。
Main worktree clickable ✅ — onclick 移到 if (!wt.is_main) 外面。
openAsWorktree auto-switch ✅ — 创建后调 switchWorktree(result.path)。
await refreshWorktrees() ✅ — 两处都加了 await。
i18n keys ✅ — en/zh 双语都补了，命名合理。

fetch 顺序修一下就能合。

elena-oaklight

Review: fix: address review feedback from PRs #67, #68, #71

Thorough fix-up PR — most of the changes are solid and well-implemented. Two issues need addressing before merge.

Issue Checklist

#	Issue	Status	Notes
1	🔴 XSS in `addWorktreeDialog`	✅ Fixed	innerHTML → DOM API with `textContent`. Clean, correct.
2	🔴 Silent fetch failure hiding stale data	⚠️ Regressed	Fetch result is now surfaced, but *branch list is generated before* the fetch** (see below).
3	🟡 Wrong i18n keys ("Create Branch" → "Create Worktree")	✅ Fixed	New `create_worktree` / `select_branch` keys in both `en` and `zh`.
4	🟡 `worktree_switch` doesn't validate git repo	✅ Fixed	`has_git()` check added.
5	🟡 `worktree_switch` silently no-ops in multi-project mode	✅ Fixed	Early `abort(400)` in non-single mode.
6	🟡 Missing `--` separator in git commands	✅ Fixed	Added in both `worktree_add` and `worktree_remove`.
7	🟡 Path traversal in `worktree_add`	❌ Not fixed	See below.
8	🟡 Detached HEAD leaking into branch list	✅ Fixed	Lines starting with `(` now skipped.
9	🟡 `lstrip("* ")` stripping by character set	✅ Fixed	`removeprefix("* ")` — correct.
10	🟡 `worktree_list` dropping last entry	✅ Fixed	Flush after loop.

Additional fixes not in the original list — all good:

Main worktree now clickable ✅
openAsWorktree auto-switches ✅
confirm() → custom dialog ✅
Unsaved-buffer check before switch ✅
Missing await on refreshWorktrees() ✅

🔴 1. Fetch ordering regression (`handlers.py`)

The refactored handle_git_branches now calls list_branches() before fetch():

result = git_ops.list_branches(project_dir)       # ← branches listed with stale data
if query_params.get("fetch", [""])[0] == "true":
    fetch_result = git_ops.fetch(project_dir)       # ← fetch happens after
    result["fetch"] = fetch_result
return result

The original code had the correct order (fetch first, then list), but discarded the fetch result. This refactoring inverts the order — so when fetch=true, the response always contains stale branch data. The fetch result field is a good addition, but the ordering needs to be restored:

fetch_result = None
if query_params.get("fetch", [""])[0] == "true":
    fetch_result = git_ops.fetch(project_dir)

result = git_ops.list_branches(project_dir)
if fetch_result is not None:
    result["fetch"] = fetch_result
return result

🔴 2. Path traversal in `worktree_add` still unfixed (`handlers.py`)

handle_git_worktree_add uses the name body parameter directly in path construction:

wt_name = body.get("name", "").strip() or branch.replace("/", "-")
parent_dir = os.path.dirname(os.path.abspath(project_dir))
wt_path = os.path.join(parent_dir, f"{project_basename}--{wt_name}")

A request with {"branch": "main", "name": "../../etc/evil"} would create a worktree at <grandparent>/project--../../etc/evil, escaping the expected sibling directory. Suggested fix — reject path separators and traversal patterns:

wt_name = body.get("name", "").strip() or branch.replace("/", "-")
if "/" in wt_name or "\\" in wt_name or wt_name in (".", "..") or ".." in wt_name:
    abort(400, "Invalid worktree name")

Or, more robustly, validate that the resolved path stays under parent_dir:

resolved = os.path.realpath(wt_path)
if not resolved.startswith(os.path.realpath(parent_dir) + os.sep):
    abort(400, "Worktree path escapes project directory")

Minor note (not blocking)

The slashed branch name issue with the DELETE /api/projects/<name>/git/branches/<branch_name> route (from PR #69 review) is not in scope for this PR but should be tracked separately — <branch_name> in the Bottle route won't match names like feature/x.

Summary

Most fixes are clean and correct — the XSS fix, -- separators, removeprefix, detached HEAD filtering, worktree_switch validation, custom dialogs, and unsaved-buffer check are all well done. Requesting changes for the fetch ordering regression and the missing path traversal sanitization.

milo-oaklight

Review: PR #72 — fix: address review feedback from PRs #67, #68, #71

CI: ✅ All checks pass (lint, GitGuardian)

Nice comprehensive follow-up. Most of the original findings are resolved. A few items are still open, and the fix for one introduced a regression. Going through each systematically:

✅ Resolved — PR #71 Findings

1. XSS in addWorktreeDialog — FIXED

The entire dialog is now built via DOM API (createElement + .textContent). No innerHTML with user-controlled data remains. This is the correct fix. 👍

2. openAsWorktree auto-switch — FIXED

openAsWorktree() now calls await switchWorktree(result.path) after creation. One cosmetic note: switchGitSubTab("versions") is called after switchWorktree(), which internally calls refreshFiles() + refreshGit(). If those reload the git panel, the sub-tab switch might have no visible effect or reference stale DOM. Not a bug, but worth testing — if it works, fine.

3. Wrong button label — FIXED

t("create_branch") → t("create_worktree") with proper i18n keys in both en and zh. ✅

4. confirm() → custom dialog — FIXED

removeWorktree() now uses the git-dialog-overlay pattern consistently with the rest of the UI.

5. Unsaved-buffer check — FIXED

switchWorktree() checks S.modified before switching and shows a save/discard/cancel dialog. Clean implementation using the same dialog pattern.

Minor concern: if the user picks "Save & Continue" but saveCurrentFile() fails (network error, etc.), the switch proceeds anyway. Consider checking the save result before continuing:

if (action === "save") {
  try { await saveCurrentFile(); }
  catch { setStatus("Save failed — switch cancelled", "error"); return; }
}

6. Missing await on refreshWorktrees() — FIXED

Both addWorktreeDialog and removeWorktree now await refreshWorktrees().

7. New i18n keys — FIXED

create_worktree, select_branch, unsaved_changes, save_and_continue, discard_and_continue added in both language blocks. ✅

✅ Resolved — PR #68 Findings

8. -- separator in worktree_add / worktree_remove — FIXED

Both now use -- to separate options from path arguments, preventing flag injection from paths starting with -. worktree_remove also correctly places --force before --.

9. worktree_switch validation — FIXED

Now rejects multi-project mode (abort(400)) and validates has_git(path). This is a clear improvement.

10. worktree_list last-entry flush — FIXED

The parser now flushes current after the loop exits, handling porcelain output that doesn't end with a blank line. ✅

✅ Resolved — PR #67 Findings

11. Detached HEAD leak in list_branches() — FIXED

Two-part fix: lstrip("* ") → removeprefix("* ") (fixes the char-set strip bug — lstrip would have mangled branch names starting with * or space), and if line.startswith("("): continue skips detached HEAD entries. Correct. ✅

⚠️ Still Open / New Issues

12. 🔴 Fetch-then-list ordering regression (handlers.py:1125-1129)

This is a new bug introduced by this PR. The old code:

if query_params.get("fetch", ...):
    git_ops.fetch(project_dir)
return git_ops.list_branches(project_dir)

The new code:

result = git_ops.list_branches(project_dir)   # ← branches listed FIRST
if query_params.get("fetch", ...):
    fetch_result = git_ops.fetch(project_dir)  # ← fetch happens AFTER
    result["fetch"] = fetch_result
return result

When fetch=true, the branch list is now populated before the fetch runs, so newly fetched remote branches won't appear in the response. The fetch result is surfaced (good!), but the ordering needs to be:

fetch_result = None
if query_params.get("fetch", [""])[0] == "true":
    fetch_result = git_ops.fetch(project_dir)
result = git_ops.list_branches(project_dir)
if fetch_result is not None:
    result["fetch"] = fetch_result
return result

13. 🟡 .current still marks main, not active worktree (app.js:3975)

Still the same issue flagged in PR #71. The code is:

item.className = "git-worktree-item" + (wt.is_main ? " current" : "");

The accent border always highlights the main worktree, not the one currently active via switchWorktree(). After switching to a non-main worktree, the visual indicator is misleading. The API would need to return which worktree is actually active (matching config["project_path"]), and the frontend should key on that.

14. 🟡 Path traversal in handle_git_worktree_add still possible (handlers.py:1249-1252)

The -- separator prevents flag injection but not path traversal. The name parameter flows into os.path.join(parent_dir, f"{project_basename}--{wt_name}") unsanitized. A request with name=../../etc/foo would create a worktree outside the expected parent directory. Consider:

wt_name = body.get("name", "").strip() or branch.replace("/", "-")
if os.sep in wt_name or wt_name.startswith("."):
    abort(400, "Invalid worktree name")

Or validate that the resolved wt_path stays within the expected parent directory.

15. 🟡 worktree_switch doesn't verify same repo (handlers.py:1278)

has_git(path) confirms the path is a git repo, but not necessarily a worktree of the same repository. You could switch to a completely unrelated repo. Consider cross-checking the common git directory:

def is_same_repo(project_dir, path):
    """Check if two paths belong to the same git repository."""
    def common_dir(d):
        rc, out, _ = _run_git(d, "rev-parse", "--git-common-dir")
        return os.path.realpath(out.strip()) if rc == 0 else None
    return common_dir(project_dir) == common_dir(path)

Summary

Finding	Status
XSS in `addWorktreeDialog`	✅ Fixed
`openAsWorktree` auto-switch	✅ Fixed
Wrong button label (`create_branch`)	✅ Fixed
`confirm()` → custom dialog	✅ Fixed
Unsaved-buffer check	✅ Fixed (minor edge case)
Missing `await` on refresh	✅ Fixed
i18n keys	✅ Fixed
`--` separator (flag injection)	✅ Fixed
`worktree_switch` basic validation	✅ Fixed
`worktree_list` last-entry flush	✅ Fixed
Detached HEAD in branch list	✅ Fixed
Fetch result surfaced	✅ Fixed (but ordering regression ↓)
Fetch-before-list ordering	🔴 Regression
`.current` marks main, not active	🟡 Still open
Path traversal in `worktree_add`	🟡 Still open
`worktree_switch` same-repo check	🟡 Still open

The big one is #12 — the fetch ordering regression is a functional bug that should be fixed before merge. The rest are pre-existing issues from the previous review that weren't addressed in this round.

Solid work overall. The XSS fix is clean, the dialog migrations are consistent, and the backend validations are meaningful improvements.

— Milo 👻

Oaklight · 2026-09-13T21:08:26Z

Addressing remaining feedback

Thanks all three reviewers. Fixing the outstanding items in a follow-up:

🔴 Fetch ordering regression: Will restore fetch-before-list order while keeping fetch result in response ✅
🟡 Path traversal in worktree_add: Will validate worktree name and reject path separators/traversal ✅
🟡 Unsaved-buffer check too narrow: Will check S.modified.size > 0 instead of just current file ✅

fix: address review feedback from PRs #67, #68, #71

d101417

This was referenced Sep 13, 2026

feat: add branch switch/create/delete and stash operations #69

Merged

feat: add branch selector dropdown with dirty-state guard #70

Merged

Oaklight merged commit 3aadf3f into master Sep 13, 2026
2 checks passed

Oaklight deleted the fix/review-feedback-roundup branch September 13, 2026 18:20

clementine-oaklight Bot reviewed Sep 13, 2026

View reviewed changes

elena-oaklight Bot requested changes Sep 13, 2026

View reviewed changes

milo-oaklight Bot reviewed Sep 13, 2026

View reviewed changes

clementine-oaklight Bot mentioned this pull request Sep 13, 2026

fix: address review feedback from PRs #69 and #70 #73

Merged

6 tasks

Oaklight mentioned this pull request Sep 13, 2026

fix: fetch ordering, path traversal guard, unsaved-buffer check, CSS variable #74

Merged

6 tasks

Navigation Menu

fix: address review feedback from PRs #67, #68, #71 - #72

fix: address review feedback from PRs #67, #68, #71#72
Oaklight merged 1 commit into
masterfrom
fix/review-feedback-roundup

Oaklight commented Sep 13, 2026

Uh oh!

Uh oh!

clementine-oaklight Bot left a comment

Uh oh!

elena-oaklight Bot left a comment

Uh oh!

milo-oaklight Bot left a comment

Uh oh!

Oaklight commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Oaklight commented Sep 13, 2026

Summary

Backend: src/tinyleaf/git_ops.py

Backend: src/tinyleaf/handlers.py

Frontend: src/tinyleaf/static/js/app.js

Test plan

Uh oh!

Uh oh!

clementine-oaklight Bot left a comment

Choose a reason for hiding this comment

🔴 Bug: handle_git_branches() fetch 顺序反了

🟡 Medium: unsaved-buffer check 只看当前文件

Uh oh!

elena-oaklight Bot left a comment

Choose a reason for hiding this comment

Review: fix: address review feedback from PRs #67, #68, #71

Issue Checklist

🔴 1. Fetch ordering regression (handlers.py)

🔴 2. Path traversal in worktree_add still unfixed (handlers.py)

Minor note (not blocking)

Summary

Uh oh!

milo-oaklight Bot left a comment

Choose a reason for hiding this comment

Review: PR #72 — fix: address review feedback from PRs #67, #68, #71

✅ Resolved — PR #71 Findings

✅ Resolved — PR #68 Findings

✅ Resolved — PR #67 Findings

⚠️ Still Open / New Issues

Summary

Uh oh!

Oaklight commented Sep 13, 2026

Addressing remaining feedback

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Backend: `src/tinyleaf/git_ops.py`

Backend: `src/tinyleaf/handlers.py`

Frontend: `src/tinyleaf/static/js/app.js`

🔴 Bug: `handle_git_branches()` fetch 顺序反了

🔴 1. Fetch ordering regression (`handlers.py`)

🔴 2. Path traversal in `worktree_add` still unfixed (`handlers.py`)