Skip to content

fix: resolve an untracked selection into Unstaged once the lists are merged - #32

Merged
ashproto merged 2 commits into
nextfrom
fix/untracked-presence-under-unify
Aug 10, 2026
Merged

fix: resolve an untracked selection into Unstaged once the lists are merged#32
ashproto merged 2 commits into
nextfrom
fix/untracked-presence-under-unify

Conversation

@ashproto

Copy link
Copy Markdown
Owner

Reported by Codex review on the v0.4.0 promotion (#30). next is protected by a ruleset requiring a PR, so the fix lands here and #30 picks it up.

The bug

SectionPresence already documents the rule — presence describes lists on screen, not file states, and explicitly says the untracked case must report "unstaged" while merging. The component derived the three fields inline, and they disagreed about what "rendered" meant:

unstaged:  unstagedDisplay.some(...)   // merged display list  ✅
untracked: untrackedFiles.some(...)    // raw file state       ❌

So: select an untracked file with the sections separate, then enable Merge Untracked into Unstaged. Presence reports the path in both, resolveSection keeps the recorded "untracked", and the pane resolves to a section that renders no rows in that mode — nothing highlights, and the Unstaged header offers "Stage all" instead of "Stage".

The fix

Two parts, and the second is the one that makes the first safe.

1. The rule moves out of the component into renderedPresence in workingSection.ts — beside the type it builds and the resolver that consumes it. This rule has now been violated once per field (the unstaged half was the same bug, caught in #26 review), which is the argument for it being one tested function rather than three inline expressions that can drift.

2. untracked stops answering two questions through one field. It now answers only "is the Untracked section showing this row", which is false while merging. Whether the file is untracked — which selects git diff --no-index and disables hunk staging — now reads from the file list directly and is unaffected by the mode, which is what that call always needed. Had I only changed presence, selectedIsUntracked would have flipped to false for merged untracked files and fetched the wrong diff.

The Untracked section header also consults it, but that section renders only when NOT merging, so the two readings coincide there.

Tests

Five new cases on renderedPresence, including the merged one and an end-to-end pairing with resolveSection:

renderedPresence("n.txt", files, false)  -> { untracked: true }
renderedPresence("n.txt", files, true)   -> { unstaged: true }
resolveSection("untracked", before)      -> "untracked"
resolveSection("untracked", after)       -> "unstaged"

Watched failing first — the type gate reported has no exported member 'renderedPresence' before the function existed.

npm run check 530 files / 0 errors · npm test 393 passed (up from 388) · cargo test 232 + 7.

🤖 Generated with Claude Code

…merged

Section presence describes lists ON SCREEN, not file states — the rule the
SectionPresence doc already spells out. The component derived the three fields
inline and they disagreed about it: `unstaged` came from the merged display
list, `untracked` came from the raw file state.

So selecting an untracked file and then turning on "Merge Untracked into
Unstaged" reported presence in BOTH. resolveSection kept the recorded
"untracked", the pane resolved to a section with no rows in that mode, nothing
highlighted, and the Unstaged header offered "Stage all" instead of "Stage".

The rule has now been violated once per field, so it moves out of the component
into `renderedPresence` in workingSection.ts, beside the type it builds and the
resolver that consumes it, with the merged case under test.

`untracked` was also answering two different questions through one field. It
now answers only "is the Untracked SECTION showing this row", which is false
while merging. Whether the FILE is untracked — which selects `git diff
--no-index` and disables hunk staging — reads from the file list directly and
is unaffected by the mode, which is what it always needed to be. The Untracked
header that also consults it renders only when NOT merging, so the two readings
coincide there.

Reported by Codex review on the v0.4.0 promotion (#30).

npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

await gitActions.deleteBranch(

P1 Badge Revalidate the repository after branch-delete confirmation

If the user leaves this confirmation open and switches repositories through the newly added native Open Repository… menu, refActionsBlocked() has already run and the dialog remains pending. Confirming afterward passes the old branch name and options to gitActions.deleteBranch, which operates on the now-current repository; when both repositories contain that branch name, this can delete the wrong branch (including force-deleting it). Capture the repository before opening the dialog and abort or retarget if it changes before this call.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

`refActionsBlocked()` covers the instant of the click. Every ref action that
asks first then waits on a prompt with no timeout, and `gitActions` targets
`appState.repo` — not whatever was on screen when the dialog opened. Switch
repositories mid-prompt and confirming runs the OLD repo's branch or tag name
against the NEW repository; where both hold that name, "Delete" force-deletes
the wrong one. The same time-of-check/time-of-use shape as the stale-diff guard
on discard, which is the precedent for fixing it the same way.

Codex flagged branch delete. Six more sites have the identical gap, three of
them destructive, so all seven are guarded rather than the one reported:

  confirmBranchDelete -> deleteBranch          (reported)
  prompt              -> renameBranch
  prompt              -> createTag
  prompt              -> createBranch (from ref)
  confirm             -> deleteRemoteBranch
  confirm             -> deleteTag
  prompt              -> createBranch (detached HEAD)

`sameRepoAfterPrompt()` captures the repo before the await and returns a
predicate checked after it, so each call site keeps its own control flow
instead of being wrapped in a callback. Refusing states plainly that nothing
was done, rather than failing silently or acting on a guess.

Reported by Codex review on #32.

npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ashproto

Copy link
Copy Markdown
Owner Author

Confirmed and fixed in 3d8f409. This is the same time-of-check/time-of-use shape as the stale-diff guard on discard, and I should have recognised it when I wrote refActionsBlocked() — that guard only covers the instant of the click, and every one of these then waits on a prompt with no timeout.

Extended past the reported site. Six other call sites in Sidebar.svelte have the identical gap, three of them destructive. Fixing only branch-delete would have left the same P1 in six places:

awaits then calls
confirmBranchDelete deleteBranch reported
prompt renameBranch
prompt createTag
prompt createBranch (from ref)
confirm deleteRemoteBranch destructive
confirm deleteTag destructive
prompt createBranch (detached HEAD)

sameRepoAfterPrompt() captures appState.repo before the await and returns a predicate checked after it — a predicate rather than a callback wrapper so each call site keeps its own control flow readable. On mismatch it refuses and says so plainly ("Repository changed while that dialog was open — nothing was done") rather than failing silently or acting on a guess.

I audited every await dialogs. in the file afterwards to confirm none was missed — all 7 now capture before the await and check after it.

Your "capture the repository before opening the dialog" is what I implemented; I did not take the "or retarget" alternative. Retargeting would silently apply an action the user authorised against a different repository's state — the branch they confirmed deleting is not the branch they would be deleting.

npm run check 530 files / 0 errors · npm test 393 · cargo test 232 + 7.

@ashproto ashproto added the skip-build Infra/bootstrap PR: skip release build; may target main directly label Aug 10, 2026
@ashproto
ashproto merged commit 554c1c9 into next Aug 10, 2026
2 checks passed
@ashproto
ashproto deleted the fix/untracked-presence-under-unify branch August 10, 2026 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-build Infra/bootstrap PR: skip release build; may target main directly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant