Skip to content

fix(web): identify cleanup results by machine and folder - #20211

Merged
juliusmarminge merged 5 commits into
pingdotgg:mainfrom
razzeee:worktree/clarify-cleanup-failures
Oct 20, 2026
Merged

juliusmarminge merged 5 commits into
pingdotgg:mainfrom
razzeee:worktree/clarify-cleanup-failures

Conversation

@razzeee

@razzeee razzeee commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Cleanup results hid the full worktree folder and omitted machine identity with one connected environment. A failed result could leave users unable to locate the checkout to investigate it.

Results now show the machine, project checkout, full folder, and associated thread separately from the failure reason. Paths wrap and can be selected. The server supplies project context, and reports from older servers decode a missing project path as null. Cleanup rules, deletion behavior, and permissions are unchanged.

Rebuilt from @razzeee’s proof of concept in #20211, preserving contributor commits and merging main. The focused contract/server mapping was retained. The UI removes optional machine-label branches and keeps the Thread field even when its title matches the folder name. Maintainer approval remains pending; this takeover does not merge the PR.

Verification on headless Linux:

  • Focused suites: 142 → 141 passing (server 138 → 137, browser 4 → 4). Removed a duplicate Git index-lock test and consolidated its assertions into the existing real-Git case. Strengthened the visible failed-result check for one connected machine. Reverting to main fails project-context, legacy-decoding, and visible-machine regressions.
  • Final head: 137 cleanup tests, 4 Chromium report tests, and 6 existing environment-label tests pass. The earlier invalid-gitfile failure did not reproduce on Git 2.43 in either the baseline or final suite.
  • Scoped lint, formatting, contracts/web/server typechecks, and whitespace checks pass.
  • Integrated Browser panel: isolated worktree state and temporary real Git repository/worktree, with an index lock causing a real cleanup failure. Both the automatic sweep and Settings → Storage → Delete now returned that failure. Last run shows the sole machine fs-23, the actual fixture’s project and full folder paths, the thread, and a distinct reason. Long paths wrap and are selectable.
  • Project/thread projection rows were ephemeral visual fixtures written while the server was stopped; no coherent event-history claim is made. The cleanup report itself came from the running service, without report injection. Server tests use real Git with existing mocked SQL/projection dependencies.

One fresh parallel whole-diff review round by Claude Opus 5.5 and Codex GPT-6 Sol found no confirmed bugs or blockers. Both checked old-server/new-client and new-server/old-client compatibility, live/deleted project mapping, per-environment identity, markup, and bounded rendering. No extra tests or implementation changes were requested. The prior bot approval covers the original revision; CI and review must cover this pushed head.

Web and desktop share this screen. Mobile currently exposes no cleanup report. Native macOS, Windows, packaged Electron, iOS, Android, narrow mobile web widths, and remote/relay/tunnel connections were not exercised; the existing environment association and disambiguation helper are preserved.

Before: the exact pinned-main StorageSettings component rendered against the same current service and fixture report. This compares the base and rebuilt interfaces, rather than claiming a full base-server run.

Before: cleanup failure with short name and no machine or full folder context

After: the reviewed head, with machine and full location details.

After: cleanup failure showing machine, project, folder, and thread

Implemented with Codex gpt-6.1-sol; focused tests by Codex gpt-6-astra; independent reviews by Claude claude-opus-5-5 and Codex gpt-6-sol, through T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 16, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4042c03

Macroscope's review found this PR approvable — This is a small, targeted cleanup-report fix that enriches existing results with machine, project, folder, and thread context without changing cleanup or deletion behavior. The contract remains backward-compatible and the new report and UI cases are covered by tests.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ee75712d-4ab3-4f61-a840-bdd84fd2d93f
📥 Commits

Reviewing files that changed from the base of the PR and between 4042c03 and 4d2bb54.

📒 Files selected for processing: 5
  • apps/server/src/storageCleanup.test.ts
  • apps/server/src/storageCleanup.ts
  • apps/web/src/components/settings/StorageSettings.browser.test.tsx
  • apps/web/src/components/settings/StorageSettings.tsx
  • packages/contracts/src/settings.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fe88d90f-6a8d-4c61-8512-f6fa693d0e79
📥 Commits

Reviewing files that changed from the base of the PR and between e3b35d1 and 4042c03.

📒 Files selected for processing: 5
  • apps/server/src/storageCleanup.test.ts
  • apps/server/src/storageCleanup.ts
  • apps/web/src/components/settings/StorageSettings.test.tsx
  • apps/web/src/components/settings/StorageSettings.tsx
  • packages/contracts/src/settings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Cleanup report entries now include a nullable project path. The settings view displays available machine, project, folder, and thread details separately from the cleanup reason.

Changes: Storage cleanup reports

Layer / File(s) Summary
Add project paths to cleanup reports
packages/contracts/src/settings.ts, apps/server/src/storageCleanup.ts, apps/server/src/storageCleanup.test.ts
The report contract adds a nullable projectPath. Cleanup results populate the field, and tests cover report values and legacy decoding when the field is absent.
Render structured cleanup details
apps/web/src/components/settings/StorageSettings.tsx, apps/web/src/components/settings/StorageSettings.test.tsx
The settings view separates the cleanup reason from available machine, project, folder, and thread details. It passes the environment label for single- and multi-environment views.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: maria-rcks

Merge Risk: ⚪ Minimal · up to 4042c

Cleanup reports now show available project and machine context separately from cleanup reasons, while remaining compatible with older reports. No material current-head merge risk remains.

Security architecture risk: ⚪ Minimal · up to 4042c

The added project location uses the existing authenticated report channel. Those readers already have access to project workspace locations. The change preserves cleanup permissions and deletion behavior, displays location values as text, and supports reports from older servers.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected disclosure surface is the existing environment-wide cleanup report, not a new tenant or service boundary. Project workspace roots are already included in project-shell data available under the same orchestration-read scope; the PR adds another presentation of that location context.

Security Findings and Attack Paths

  • observed — Potentially influenced path, reason, and thread strings are rendered as JSX text children. The changed report renderer does not turn them into HTML, navigation targets, commands, or filesystem operations.

Trust Boundaries and Controls

  • observed — WebSocket authentication establishes the session before RPC authorization is installed. Cleanup-report subscriptions retain orchestration:read enforcement, and manual cleanup retains settings-write authorization. Neither authorization mapping nor handler routing changes in this PR.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the main change: cleanup results now include machine and folder context.
Description check Passed The description clearly explains the problem, implementation, compatibility behavior, verification results, screenshots, limitations, and approval status. It is detailed and aligned with the requested…
Full details: Description check

Explanation

The description clearly explains the problem, implementation, compatibility behavior, verification results, screenshots, limitations, and approval status. It is detailed and aligned with the requested change.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one as maintainer for batch server-persistence-01. I will preserve contributor history, resolve main conflicts, and rebuild the focused cleanup-result context with minimal behavior tests and independent reviews. I will refresh browser proof where available and leave the PR unmerged.

@juliusmarminge

Copy link
Copy Markdown
Member

Rebuilt from @razzeee’s cleanup-context fix, preserving contributor commits and merging current main. The contract/server metadata mapping remains focused; the UI now always shows machine identity and keeps thread context even when its title matches the folder. Duplicate index-lock coverage was consolidated: focused tests went from 142 to 141 passing, with another 6 existing environment-label tests passing. Scoped lint/formatting and contracts/web/server typechecks pass.

The integrated Browser panel used isolated state and a real temporary Git worktree with a locked index. Both automatic cleanup and Delete now reported the real failure, and Last run displayed machine, full project/folder paths, thread, and a separate reason. The PR body includes freshly uploaded base-UI/head screenshots and states exactly how the visual fixture was seeded and what platforms remain unverified. Fresh independent Opus 5.5 and GPT-6 Sol whole-diff reviews found no code blockers.

Pushed without rewriting contributor history; left unmerged for current-head CI and maintainer approval.

Implemented with Codex gpt-6.1-sol through T3 Code.

@juliusmarminge
juliusmarminge merged commit 5803c8b into pingdotgg:main Oct 20, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants