Menu ▾ ▴

#130 fix(github,jira,repository,git): edit commit comments on the right repo

closed
nobody
bug (36)
2026-07-28
2026-07-28
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

Commit comments live in a single repository's namespace, but the GitHub edit and delete paths were origin-pinned while list and create resolved through the active remote lens — so editing or deleting a comment while viewing a fork through its parent (upstream) lens hit the wrong repo and failed. This threads the lens through the whole commit-comment cluster so all four operations agree, and picks up three unrelated fixes found alongside it (a sticky Jira estimate warning, a relocate merge that let duplicates through, and mojibake in a rewrite error).

Commit-comment lens (GitHub)

  • Adds a lens: Option<&str> parameter to commit_comment_edit and commit_comment_delete in src-tauri/src/github/pr.rs, swapping gh_origin_slug for gh_lens_slug so the comment is edited/deleted on the repo it was created on.
  • Plumbs the same parameter through src-tauri/src/forge/github.rs and the neutral forge_commit_comment_edit / forge_commit_comment_delete commands in src-tauri/src/forge/mod.rs, where only the GitHub arm consumes it (lens is a fork-network concept; the GitLab and Bitbucket arms are untouched).
  • Updates the commit_diff doc comment in src-tauri/src/github/pr.rs, which previously documented the cluster as deliberately non-uniform, to record that the comment ops now uniformly resolve through the lens while commit_diff stays origin-pinned.
  • Frontend: forgeCommitCommentEdit and forgeCommitCommentDelete in src/lib/git/api.ts take a RemoteLens and pass it in the invoke payload; useEditCommitComment and useDeleteCommitComment in src/lib/git/queries.ts forward the lens they already receive.

Jira estimate input

  • JiraEstimateInput in src/features/issues/JiraIssueView.tsx now resets its local invalid flag during render when currentDisplay changes (the adjust-state-on-prop-change pattern), since the key-based reset remounts the Input but not the component — so the "Enter a Jira duration" warning no longer persists after the server value refreshes. The component's doc comment is updated to describe the added reset.

Repository relocate merge

  • mergeIds in src/lib/repo-data-migration.ts now adds each accepted old record's id to the seen set as it iterates, so duplicates within old are dropped too — previously only collisions against keep were filtered, letting repeated ids (and multiple idless records) through. The doc comment is rewritten to state the first-occurrence-wins rule.

Encoding and comment cleanup

  • Repairs mojibake em-dashes across doc comments in src-tauri/src/git/ops.rs, including the user-visible InvalidArgument message in rewrite_commits ("the working tree has uncommitted changes — commit or stash them first").
  • Corrects the pathspec-exclude comment in src-tauri/src/git/diff.rs: git pathspec globs are only close to gitignore semantics — * also matches / (wildmatch without WM_PATHNAME), so a pattern like src/*.rs also hides nested files.

Changelog

  • Adds four fragments under changelog.d/: fixed-commit-comment-fork-lens.md, fixed-jira-estimate-stale-warning.md, fixed-relocate-duplicate-records.md, and fixed-rewrite-error-garbled-dash.md.

Discussion

  • Anonymous

    Anonymous - 2026-07-28
     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    Context for reviewers — deliberate calls and disclosures, one claim per item. (Round-zero gate ran over the full diff before open: cargo test 872/872, clippy -D warnings clean, pnpm build green, biome check-only clean, plus an independent adversarial spec-review of both halves.)

    What this PR is. Five small bug fixes surfaced by the [#129] comment-trim audit. The headline: GitHub commit-comment edit/delete were origin-pinned while list/create resolved through the active fork lens, so a comment created under the upstream lens landed on the parent repo and then edit/delete 404'd against the fork. The lens now threads through the whole cluster. Riders: a sticky Jira estimate warning, a relocate merge that let within-array duplicates through, mojibake em-dashes in ops.rs (one in a user-facing error), and a false pathspec doc claim in diff.rs.

    1. commit_diff stays origin-pinned — deliberately. It is not a missed sibling of this fix: GitHub's fork-network storage serves ANY network SHA via the fork's own commits endpoint, so the pin cannot 404 there, and its doc comment records exactly that. A comment, by contrast, lives in one repo's namespace — which is why the comment ops must agree on the lens while commit_diff need not.
    2. The other origin-pins in github/pr.rs are also deliberate. We censused all gh_origin_slug/gh_lens_slug sites: star status/set, branch protections, gh_pr_poll, and gh_repo_url each carry a documented rationale. No other member of a lens-resolved cluster is origin-pinned.
    3. lens is Option<String> on the Rust commands but required (RemoteLens) in TS. This mirrors forge_commit_comment_create exactly; the TS layer always sends it, and Option keeps any external invoker of the command compatible.
    4. The unused sha on the GitHub edit/delete arms is pre-existing, kept for the neutral shape — GitLab and Bitbucket both need it. Not this PR's to change.
    5. JiraEstimateInput resets invalid during render, not in an effect. This is React's documented adjust-state-on-prop-change pattern ("You Might Not Need an Effect"); the key-based remount only resets the inner Input, not the component's state, so the render-time reset is the missing half of the existing idiom. The component has no early returns, so hook order is unconditionally stable.
    6. mergeIds behavior change is deliberate: uniform first-occurrence-wins, including id === undefined. That matches the function's documented contract (previously the dedup only checked against keep, so duplicates within one legacy array all survived). Duplicates already inside keep still pass through — at every call site keep is either [] or an accumulator already deduped by this same function (verified through the fold in migrateValue).
    7. No README/site/help edits — deliberate. These are behavior fixes with no capability change. The doc sweep found nothing falsified; notably, the help's existing claim that the upstream lens makes comments read and write the parent was subtly false before this fix and is true after it. Changelog presence is the four fragments.
    8. Disclosure — not live-fired. The fork-lens fix is verified by code-reading (gh_origin_slug is literally gh_lens_slug(repo_path, None), so the old path always addressed origin) and by the green suites, not against a real fork/upstream pair; that live arm is on our post-merge watch list.
    9. Disclosure — diff.rs rider is doc-only. The pathspec comment now matches compare.rs's measured wording (git pathspec * crosses / — wildmatch without WM_PATHNAME, measured on git 2.51.1). No behavior change in that file.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

    Related

    Tickets: #129

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    The PR threads the fork/upstream lens through GitHub commit-comment edit/delete (previously origin-pinned while list/create were lens-resolved), plus four small riders: a Jira estimate warning reset, a within-array dedup in the relocate merge, mojibake em-dashes, and a pathspec doc correction. Sound and not blocking — I traced the lens end-to-end (CommitComments.tsx:238-241 → queries.ts:1215-1248 → api.ts:1402-1425 → forge/mod.rs:1094-1132 → github/pr.rs:2924-2974 → gh_lens_slug) and the list/create/edit/delete cluster now agrees on one slug for every code path; lens_remote already accepts Some("origin"), so the default path is byte-identical to the old gh_origin_slug. Only nits below.

    Consistency

    • nit — src/lib/git/repo-identity.ts:39-46, mergeById: the sibling of the helper this PR just hardened has the identical non-growing seen set (const seen = new Set(base.map(...)); return [...base, ...extra.filter(...)]), so duplicate ids within extra all survive when folding a legacy path-keyed list (local-prs.json, local-issues.json, pr-reviews.json). If the within-array case is worth fixing in mergeIds, apply the same for-loop + seen.add(x.id) shape here and extend its doc comment ("keep's items come first and win on a shared id") to say first-occurrence-wins inside extra too — or state why it's deliberately out of scope.

    Readability & doc accuracy

    • nit — src/lib/repo-data-migration.ts:112-116: the new contract sentence "idless objects all collide and only the first one survives" isn't true of keep — idless (or duplicate-id) entries already inside keep are spread through untouched. The recorded note says every call site passes keep as [] or a mergeIds-deduped accumulator, but combine's id-merge branch passes the raw new-key value (const keep = Array.isArray(newVal) ? newVal : [] at line 163, used at line 169). Reword the last clause to e.g. "…and undefined is an id like any other, so only the first idless old record survives. keep passes through verbatim — duplicates already inside it are preserved."
    • nit — src-tauri/src/git/conflict.rs:69-72 still describes the same :(exclude) machinery as "git's own gitignore-style pathspec matching (the same engine the staged diff uses)", the wording class this PR corrected in diff.rs to match compare.rs. Add the same caveat there (* also matches / — wildmatch without WM_PATHNAME) or trim the phrase to "git's own pathspec matching", so the third copy doesn't re-assert exact-gitignore semantics.

    Documentation

    Acknowledged as a recorded decision (note 7): no README / site / help edits. I checked the one claim at risk — src/features/help/content.ts:638 ("…comments, reviews… read and write the parent repository instead of your fork") — and it is true only after this change, so nothing there is falsified. The four changelog.d/fixed-*.md fragments match the changelog.d/README.md format and cover the four user-facing fixes; the diff.rs rider is comment-only and correctly carries none. Also noted: commit_diff staying origin-pinned (note 1) and the not-live-fired disclosure (note 8) remain recorded decisions.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    No security issues found in these changes. (The new lens parameter reaching gh_lens_slug is allowlist-validated to origin/upstream in lens_remote before any spawn, the comment id is parsed to u64 before interpolation, the edit body travels as JSON on stdin via run_gh_input, and mergeIds only reads id into a Set — no key assignment.)

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    Round-1 dispositions — all four findings verified against the files and accepted; every fix lands in one push (gates re-run on the batch: pnpm build green, clippy -D warnings clean, cargo 872/872, biome clean).

    1. mergeById sibling (consistency nit) — fixed. Same loop shape as mergeIds (seen.add per admitted record) so first occurrence wins within extra too; doc comment extended with both the within-extra clause and the keep-passes-through-verbatim clause so the twin docstrings agree. Caller sweep re-checked: all seven call sites keep their contract (order preserved; undefined keep still yields the same results).
    2. mergeIds doc overclaim (accuracy nit) — fixed, and your refutation of context item 6 is correct. The comment now reads: an idless record suppresses every later idless old one; "keep itself passes through verbatim (never deduped)". On item 6: combine's id-merge branch does pass the raw new-key value as keep on the final merge — only the loop accumulator is pre-deduped — so "at every call site keep is already deduped by this same function" was wrong as I recorded it. Corrected here for the record; the practical exposure is unchanged (destination values are app-written, UUID-keyed), but the claim was the claim.
    3. conflict.rs third "gitignore-style" copy (accuracy nit) — accepted, with an upgrade. Rather than copying the *-crosses-/ caveat into a third site, we live-probed the semantics and found the existing enumeration was incomplete in both twins: a bare name like notes.md is root-anchored as a pathspec (:(exclude)notes.md leaves docs/notes.md listed) where gitignore matches at any depth — measured on git 2.51.1, alongside the known *-crosses-/ over-match. So: conflict.rs now says "close to but not identical to gitignore semantics" (non-exhaustive by construction), and the canonical twins in diff.rs/compare.rs both carry the measured two-sided divergence, kept byte-identical. Format-descriptor mentions of "gitignore-style" (README, site, instructions header, etc.) are deliberately unchanged — they describe how patterns are written; whether user-facing aiignore docs should spell out the matching semantics is a pre-existing backlog item and stays homed there.
    4. Copilot's wording nit ("undedupable") — fixed in the same mergeIds comment rewrite ("they can't be deduplicated").

    Security audit: acknowledged — no findings, and its trace of the lens_remote allowlist, the u64 id parse, and the stdin JSON body path matches ours.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    The lens threading for GitHub commit-comment edit/delete, and the three doc/dedup fixes pushed since the last round, all check out end to end — I re-traced CommitComments.tsx:238-241 → queries.ts:1215-1248 (both call sites now pass lens, and commitCommentsKey at queries.ts:1076 already keys the optimistic cache by lens) → api.ts:1402-1427 → forge/mod.rs:1094-1132 → github/pr.rs:2924-2974 → gh_lens_slug/lens_remote (validated allowlist, None/Some("origin") byte-identical to the old gh_origin_slug), and confirmed via a repo-wide grep that commit_comment_edit/_delete have no other callers (no MCP or LAN caller to update). Nothing blocking.

    Resolved since last review

    • src/lib/git/repo-identity.ts:40-53 — mergeById now grows seen per admitted record, so first occurrence wins inside extra too; the doc comment carries both the within-extra clause and the keep-passes-through-verbatim clause, matching its mergeIds twin. The seven callers are unaffected (order preserved, undefined keep still yields base = []).
    • src/lib/repo-data-migration.ts:112-129 — the doc no longer overclaims about keep; "keep itself passes through verbatim (never deduped)" is true of the code as written (combine's id-merge branch at line 163-170 passes the raw new-key value as keep, which is exactly what the reworded sentence now permits). Copilot's "undedupable" wording nit is fixed in the same rewrite.
    • src-tauri/src/git/conflict.rs:69-73 — the third "gitignore-style" copy is gone; and the two canonical twins (diff.rs:406-411, compare.rs:112-117) are now byte-identical and correctly describe the :(exclude)<pattern> form actually built at diff.rs:418 / conflict.rs:88 (no glob magic, so wildmatch-without-WM_PATHNAME and root-anchoring are both the right caveats).

    Documentation

    • nit — changelog.d/fixed-relocate-duplicate-records.md: the fragment is scoped to "Relocating a repository", but the fix pushed this round also changed mergeById in src/lib/git/repo-identity.ts, which runs from identityKeyFor on the legacy path→identity fold (first read/write of a per-repo store after upgrade), not on relocate. Either broaden the bullet — e.g. "Duplicate legacy records — including records without ids — are no longer carried through when merging a repository's old app data, on relocate and when folding pre-worktree-aware records onto the repository's identity key." (drop nothing else; the "including records without ids" clause still belongs to the mergeIds half, since mergeById's T requires id: string) — or record that the mergeById half is deliberately covered by the same bullet.

    Nothing further to raise.

    Verdict: no blocking issues — remaining items are non-blocking; merge when ready

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    Round-2 disposition — the one nit is verified correct and fixed, held as the closing push.

    The fragment's "Relocating a repository" subject did under-cover the batch: mergeById runs from identityKeyFor on the legacy path→identity fold, not on relocate. The bullet is now:

    Merging a repository's old app data — on relocate, or when older path-keyed records are folded onto its stable identity — no longer lets duplicate legacy records (including records without ids) through.

    Both scope claims re-verified against the code (relocate arm = mergeIds via the raw-path store migration; identity-fold arm = mergeById via identityKeyFor). Per your note, the "records without ids" clause belongs to the mergeIds half — mergeById's T requires id: string — and the combined phrasing follows the shape your suggestion already acknowledged for that split.

    pnpm changelog:preview assembles clean. This one-line fragment edit is the closing batch; nothing else is outstanding on our side.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    Resolved since last review

    • changelog.d/fixed-relocate-duplicate-records.md — the bullet no longer scopes the fix to relocate alone; it now names both arms ("on relocate, or when older path-keyed records are folded onto its stable identity"). I re-verified both scope claims: the relocate arm is mergeIds in src/lib/repo-data-migration.ts:117-127, and the identity-fold arm is mergeById (repo-identity.ts:40-53) passed as merge into identityKeyFor, which folds the raw-path value onto the git-common-dir key at repo-identity.ts:87-93. The "(including records without ids)" clause still reads as belonging to the mergeIds half, which is correct — mergeById's T extends { id: string } can't carry idless records.

    The reworded hunk introduces no collateral (single fragment file, no code touched, filename still matches the changelog.d/<added|changed|fixed>-<slug>.md convention). Nothing further to raise.

    Verdict: no blocking issues — remaining items are non-blocking; merge when ready

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-28

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB