GitDesktop / Tickets / #96 fix(actions,fsops,mcp,tests,ui): preserve IDs and report accurate counts

Anonymous - 2026-07-20

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 5f8f5c7

Status: ✅ Deploy successful!

Preview URL: https://3a0803eb.gitdesktop.pages.dev

Branch Preview URL: https://fix-temp-test-cleanup-ci-ids.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 Context for reviewers — deliberate calls in this batch (numbered for reference; each names its evidence):

String-safe CI ids stop at the lookup path by design. Only forge_ci_run_view / forge_ci_run_failed_logs / forge_ci_job_logs, their TS wrappers/hooks, ChecksRollup, and the MCP arg structs went string-safe. The serde output structs (WorkflowRun/RunJob/RunDetail/Workflow) deliberately keep u64 → JS number, so RunDetailView, ui.ts's openRun, and notifications.ts still hold numeric ids — migrating output ids is a separate, much larger cross-surface change (recorded on the backlog). Real forge ids (~1e10) sit far below the 2^53 cliff; the string path exists where string ids already enter the app (PrCheckOut.runId/jobId, parsed from details URLs).

forge_ci_run_rerun / forge_ci_run_cancel and their TS wrappers stay numeric — same boundary. Their MCP arg surface did gain the dual-accept id (they share RunIdArg). forgeGlCiPlayJob unchanged.

CiId accepts a JSON number OR a numeric string — existing MCP clients that send numbers keep working; the schema advertises oneOf integer/string; unit tests cover 123, "123", and rejection of "abc" / -1 / 1.5. The == 119 tool-count test is type-blind and correctly untouched.

The temp-dir sweep is test-only. Every hunk sits in #[cfg(test)] except Cargo.toml/Cargo.lock and one cosmetic strip of a stray leading UTF-8 BOM on git/ops.rs line 1 (reviewed, disclosed). Deliberate keepers that still call env::temp_dir — not missed conversions: git/compare.rs:865 (the function under test hardcodes the real OS temp dir), oplog.rs:106/:537/:560/:565 (production fixed path + its literal assertions), agent_sandbox.rs ×4, git/branches.rs:731, git/conflict.rs:200, git/compare.rs:329/:353/:415 (production lifecycles with their own cleanup).

merge_local_pr_clean_…'s second root is deliberately NOT pre-created (root_holder.path().join("root"), no create_dir_all) — production merge_local_pr is expected to create it, and a TempDir::new root would silently weaken that precondition. The conflict variant keeps its explicit create_dir_all.

tempfile under [dev-dependencies] is newly added on master (the section didn't exist). The in-flight LAN epic branch adds the identical tempfile = "3" independently — an expected, trivially-resolvable merge at epic close, not a duplicated mistake.

git_untrack and the untrack toasts are deliberately unchanged — they count untracked files, which was already accurate; only the four ignore/AI-exclude toasts reword off the new actually-appended count. RepositoryFilesDialog untouched for the same reason.

README and the marketing site keep the literal "right-click" — static documents with no runtime resolver. The platform-aware surfaces are the in-app guide ({{secondaryclick}} token) and the BranchSwitcher tooltips (derived secondaryClickLabel). In-guide noun usages ("its right-click menu") became "context menu"; verb usages use the token — the capitalized {{Secondaryclick}} spelling is intentional (sentence-initial case; the resolver handles both spellings, all 8 instances verified against the regex two independent ways).

src/lib/git/api.ts and ChangesPanel.tsx received a content-neutral CRLF→LF normalization (numstat identical before/after) per the repo's autocrlf remediation — formatting-looking noise there is EOL-only.

Verification on the merged tree: pnpm build green · cargo test 793 passed · clippy -D warnings clean · biome check green on all touched files · whole-batch adversarial spec review: approve ×4 (sole nit = the BOM strip in item 4).

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No genuine security issues in these changes — the diff is test-fixture refactoring (RAII tempfile::TempDir), CI id string-threading that parses to u64 before any sink (strictly safer than the prior direct u64), append-count return values, and platform-derived UI copy; none introduce an exploitable path.

(Copilot's write_forge.rs as_u64() notes are a JSON number-precision/correctness concern, not a security vulnerability — not a finding.)

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

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus

This change threads CI run/job ids as strings across the Rust↔JS boundary (parsing to u64 at the provider edge), makes the two ignore-append commands return the count actually written and surfaces that in toasts, adds a platform-derived secondary-click label, and migrates Rust test fixtures to RAII TempDir. It's sound and I found nothing blocking — the id plumbing, count plumbing, and copy sweep are all internally consistent, and it's safe to merge.

What I verified:

Id threading is consistent end-to-end. forgeCiRunView/forgeCiRunFailedLogs/forgeCiJobLogs stringify (String(runId)) before invoke, the Rust commands parse::<u64>() with a clean InvalidArgument on failure, and MCP read_forge passes args.run_id.as_string(). useRunDetail/useRunFailedLogs normalize the query key via String(runId ?? 0), and no other call site constructs ["repo", repo, "actions", "run", …] with a numeric id — so cache sharing/invalidation stays intact. jobForCheck correctly compares String(j.id) === check.jobId (RunJob.id numeric, check.jobId string).

The toast count flows correctly. useRepoMutation<TArgs, TData> returns Promise<number> for both append commands, so the per-call onSuccess(added) in ChangesPanel receives the real count; append_to_gitignore/append_repo_ai_ignore return 0 on empty/all-duplicate batches, and ignoreToast's branches (already in / X of Y / X entries) are grammatically correct and can't hit total === 0 (guarded by selectionCount === 0 early returns). Tests cover full/partial/zero.

The secondary-click copy sweep is complete. Every remaining right-click in src/ is a code comment, not user-facing copy; the guide/tooltip strings all route through secondaryClickLabel (capitalization is idempotent for the already-capitalized macOS "Control-click"). A changelog fragment is present.

On the prior review comments and Copilot: the two write_forge.rs findings (rerun/cancel echoing run_id as a JSON number) are already resolved in this diff — both now echo args.run_id.as_string(), so I'm not re-raising them. The rerun/cancel commands staying numeric on the frontend (forgeCiRunCancel(runId: number)) while their MCP arg surface gained CiId is the author's explicitly recorded deliberate scope decision (context note [#2]), not an oversight.

Nits

nit — src-tauri/src/mcp_server/mod.rs, impl Display for CiId: the Display impl is dead — every stringification site uses as_string() and no code path formats a CiId directly. Harmless (trait impls don't trip dead-code lints), but you could drop it or route as_string through it to avoid the third redundant stringify path (Display, as_string, as_u64().to_string()).

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

Related

Tickets: #2
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

Re: the general review's nit (dead Display on CiId) — accepted, fix applied in the working tree, lands in the next push. Chose deletion over routing as_string() through it: of the two alternatives the review offered, removing the impl leaves exactly two accessors (as_string() for the string-taking dispatchers, as_u64() for the numeric forge fns) and no redundant third path; Display can trivially return if a formatting call site ever appears. Verified after the removal: mcp_server suite 60/60, clippy -D warnings clean.

All review items are now closed: Copilot's two threads (fixed in b30df00, resolved), the audit (clean), and this nit (fix pending push). Thanks for the end-to-end traces — the query-key and toast-branch verifications matched the pre-merge review exactly.

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

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Verified the two concerns I was unsure about: the stringified useRunDetail/useRunFailedLogs query keys are only ever invalidated by the prefix ["repo", repo, "actions"] (actions.ts:304), so no numeric/string key mismatch exists — that plumbing is sound. The other check turned up a docs-sync gap.

Summary

Threads CI run/job ids as strings across the Rust↔JS boundary (parsing to u64 at the provider edge), returns the real appended-count from the two ignore commands and surfaces it in toasts, derives a platform-appropriate secondary-click label, and migrates Rust test fixtures to RAII TempDir. The code is sound and nothing is blocking. The id/count/label plumbing is internally consistent, and the TempDir sweep correctly uses named _guard bindings everywhere (no bare _ = that would drop the dir before the test runs). One non-blocking docs-sync gap.

Documentation sync

should-fix — README.md:561 and site/src/pages/index.astro/ai.astro:279 (the Exclude files from AI bullet), plus README.md:371 and :447. This PR changed the in-app guide's "right-click a changed file → Exclude from AI" to the platform-adaptive {{secondaryclick}} (content.ts:1493) and swapped several "right-click menu" phrasings to the neutral "context menu". But the identical user-facing claims still hardcode "Right-click"/"right-click menu" in the README and marketing site — e.g. README.md:561 "Right-click a changed file" and ai.astro:279 "Right-click a changed file to keep it out of AI context". The repo's docs-sync rule explicitly warns that the same claim hides across README/site/help, and the changelog fragment itself asserts "References to 'its right-click menu' are now the platform-neutral 'context menu'" — which isn't true of the README (:371, :447). These surfaces can't derive the platform at render time, so the fix is to neutralize them to "context menu" (as the fragment already endorses) rather than adapt them. (CHANGELOG.md matches are historical release notes — leave those alone per the fragments convention.)

Resolved since last review

Dead Display on CiId (mcp_server/mod.rs) — removed in the current diff; as_string()/as_u64() are the only accessors and both are used.

rerun_workflow_run / cancel_workflow_run echoing run_id as a JSON number (write_forge.rs:958/:975, also flagged by Copilot) — both now echo args.run_id.as_string(), so responses stay precision-safe. Confirmed in the current diff; not re-raising.

Everything else I re-checked holds: CiId accepts number-or-numeric-string and rejects "abc"/-1/1.5 with tests covering all three; the oneOf schema advertises both wire forms; jobForCheck compares String(j.id) === check.jobId; and ignoreToast's branches are grammatical and unreachable at total === 0 (guarded by the selectionCount === 0 early returns).

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

Re: round 2's docs-sync should-fix — accepted, and it fairly overturns my context note [#8]. The recorded call ("static docs keep the conventional word") conflated two things the review correctly separated: static surfaces can't adapt to platform (true, and the fix respects it — no token machinery added), but the noun-phrase neutralization this PR itself made in-guide works identically in static text, and the changelog fragment's claim wasn't true until the static copies matched. Overturn acknowledged; the never-defend-the-precedent rule applies.

Applied in the working tree (lands in the next push):

README.md:371 — "the branch switcher's right-click menu" → "the branch switcher's context menu"

README.md:447 — "right-click a local PR in the list" → "open a local PR's context menu in the list"

README.md:561 — "Right-click a changed file → Exclude from AI … creates" → "A changed file's context menu → Exclude from AI … creates"

site/src/pages/ai.astro:279 — "Right-click a changed file to keep it out of AI context" → "Open a changed file's context menu to keep it out"

The changelog fragment now scopes its claim accurately: "…the platform-neutral 'context menu' — across the in-app guide, README, and marketing site."

Verified: grep -rin "right.click" README.md site/src/ (changelog excluded) → 0 hits; site && pnpm build → 7 pages, clean. One precision note for the record: site/src/pages/index.astro named in the finding had no occurrences (grep-confirmed before and after) — the four real spots were README ×3 + ai.astro ×1, all now fixed. CHANGELOG.md historical entries left alone per the finding's own guidance.

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

Related

Tickets: #8
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Verified against the filesystem: grep for right.?click returns zero matches in README.md, all of site/src/, and content.ts — so the neutralization is complete (including site/src/pages/index.astro, which this PR didn't touch but has no lingering reference). And useJobLogs keys on String(job?.id ?? 0) (actions.ts:284), so number- and string-callers share one cache entry.

Summary

Threads CI run/job ids as strings across the Rust↔JS boundary (parsing to u64 at the provider edge and dual-accepting number-or-numeric-string in the MCP arg structs), returns the real appended-count from the two ignore commands and surfaces it in toasts, derives a platform-appropriate secondary-click label, and migrates the Rust test fixtures to RAII TempDir. The change is sound and nothing is blocking — the id/count/label plumbing is internally consistent and the TempDir sweep uses named guard bindings throughout (no early-drop).

Resolved since last review

Docs-sync gap (README.md:371/447/561, site/src/pages/ai.astro:279) — the current diff neutralizes all of these to "context menu"/"open … context menu", and I confirmed via grep that no right-click remains anywhere in README.md, site/src/, or content.ts. The changelog fragment's broadened claim ("across the in-app guide, README, and marketing site") is now accurate. site/src/pages/index.astro — named in the earlier finding — has no occurrence, so nothing was left behind there.

rerun_workflow_run / cancel_workflow_run echoing run_id as a JSON number (write_forge.rs:957/:974, also flagged by Copilot) — both now echo args.run_id.as_string(); forge calls still use .as_u64() against the unchanged u64-taking forge_ci_run_rerun/forge_ci_run_cancel. Precision-safe and internally consistent. Not re-raising.

Notes on the rest

Everything else I re-checked holds:

CiId accepts a JSON number or numeric string and rejects "abc"/-1/1.5 (visit_i64 gates negatives via u64::try_from, floats fall through to the default error); the oneOf schema advertises both wire forms; unit tests cover all cases.

forge_ci_run_view/_failed_logs/_job_logs parse String → u64 once at the edge and return InvalidArgument on garbage; the TS wrappers String(...) their ids, and forgeCiRunCancel/forgeCiRunRerun correctly stay numeric to match their unchanged u64 backends.

jobForCheck compares String(j.id) === check.jobId, and useRunDetail/useRunFailedLogs/useJobLogs normalize the id into the query key with String(...), so no number/string cache split.

append_to_gitignore/append_repo_ai_ignore return the true appended count (0 on empty-batch and all-duplicates paths), api.ts types both as invoke<number>, and ignoreToast plus the single-pattern branches produce grammatical copy that's unreachable at total === 0 (guarded by the selectionCount === 0 early returns).

secondaryClickLabel's capitalize-first-letter is idempotent on the macOS "Control-click" value; BranchSwitcher now shares one publishHint string across aria-label and title.

The TempDir migration binds every guard by name (_tmp/_base/_dir/root_holder) so the directory outlives the test body; the git/ops.rs line-1 change is just a stray UTF-8 BOM strip.

No blockers, should-fixes, or nits worth raising.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

fix(actions,fsops,mcp,tests,ui): preserve IDs and report accurate counts

Milestone

Searches

Help

#96 fix(actions,fsops,mcp,tests,ui): preserve IDs and report accurate counts

CI and MCP identifiers

Ignore counts and user feedback

Platform-specific UI copy

Test fixture cleanup

Changelog

Discussion

Deploying gitdesktop with Cloudflare Pages

Nits

Related

Summary

Documentation sync

Resolved since last review

Related

Summary

Resolved since last review

Notes on the rest

Latest commit:	`5f8f5c7`
Status:	✅ Deploy successful!
Preview URL:	https://3a0803eb.gitdesktop.pages.dev
Branch Preview URL:	https://fix-temp-test-cleanup-ci-ids.gitdesktop.pages.dev