Skip to content

Restore file explorer remote root CI coverage - #4891

Closed
lawrencecchen wants to merge 2 commits into
mainfrom
issue-4524-file-explorer-store
Closed

lawrencecchen wants to merge 2 commits into
mainfrom
issue-4524-file-explorer-store

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 27, 2026 •

Copy link
Copy Markdown
Contributor

Restores CI coverage for FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath by removing its targeted -skip-testing quarantine.

This is part of #4524.

Testing:

  • git diff --check
  • parsed .github/workflows/ci.yml with Ruby YAML loader
  • hosted CI will run the restored test

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Note

Low Risk
Small behavioral guard plus CI re-enablement; main risk is renewed CI flakiness if the underlying app-host instability was not fully fixed.

Overview
Re-enables FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath in the macOS unit-test job by dropping its -skip-testing quarantine and the related CI comments.

Adds an early Task.isCancelled check at the start of FileExplorerStore.loadChildren so cancelled directory loads do not touch loading state or trigger UI updates—supporting stable remote SSH home resolution when loads are superseded.

Reviewed by Cursor Bugbot for commit 8a41296. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Chores
    • Updated CI test configuration to change which tests are skipped during automated runs and tightened test-run behavior.
  • Bug Fixes
    • File/folder loading now aborts immediately when a load task is cancelled, preventing extra work and avoiding incorrect UI state.

Review Change Stack


Summary by cubic

Re-enabled CI for FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath by removing its skip in cmux-unit. Added an early cancellation check in FileExplorerStore.loadChildren to prevent stale updates and keep SSH home resolution stable.

  • Bug Fixes
    • Return early when Task.isCancelled in FileExplorerStore.loadChildren.
    • Remove the test’s -skip-testing entry in .github/workflows/ci.yml.

Written for commit 8a41296. Summary will update on new commits.

Review in cubic

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@vercel

vercel Bot commented May 27, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 28, 2026 12:55am
cmux-staging Building Building Preview, Comment May 28, 2026 12:55am

@greptile-apps

greptile-apps Bot commented May 27, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR restores CI coverage for FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath by removing its -skip-testing quarantine, and stabilises the test by adding a cooperative cancellation guard at the entry of FileExplorerStore.loadChildren.

  • FileExplorerStore.swift: Adds guard !Task.isCancelled else { return } as the first statement in loadChildren. This closes the narrow window where a task cancelled before the first await would still dirty loadingPaths / isRootLoading state, complementing the existing try Task.checkCancellation() after the await and the cancellation-aware catch block.
  • .github/workflows/ci.yml: Removes the lone -skip-testing flag and the comment block that accompanied it; with no quarantined tests remaining, neither entry is needed.

Confidence Score: 5/5

Safe to merge — a single-line cooperative cancellation guard and a CI skip-list removal, both with clear and correct intent.

The Swift change is a standard, idiomatic cancellation check at a function entry point. It does not introduce new state, alter any data flow, or touch anything outside the already-established cancellation pattern in loadChildren. The CI change is a straightforward removal of a test quarantine that was in place precisely because this race existed.

No files require special attention.

Important Files Changed

Filename Overview
Sources/FileExplorerStore.swift Adds a cooperative cancellation guard at the entry of loadChildren — idiomatic and correct. Pre-existing try Task.checkCancellation() after the await and the cancellation-aware catch block already handled mid-flight cancellation; the new guard closes the remaining window where state could be dirtied for tasks cancelled before the first await.
.github/workflows/ci.yml Removes the -skip-testing quarantine for testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath and the comment block that explained it. The comment was tied solely to the now-restored test, so its removal is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["loadChildren called"] --> B{"Task.isCancelled?\n(NEW guard)"}
    B -- "yes" --> Z["return immediately\n(no state mutation)"]
    B -- "no" --> C{"provider available?"}
    C -- "no" --> Z
    C -- "yes" --> D["Set loadingPaths, isRootLoading\nobjectWillChange.send()"]
    D --> E["await provider.listDirectory()"]
    E --> F{"try Task.checkCancellation()\n(pre-existing)"}
    F -- "throws CancellationError" --> G{"catch: Task.isCancelled?\n(pre-existing)"}
    G -- "yes" --> Z2["return\n(skip UI cleanup — cancelAllLoads handles it)"]
    G -- "no" --> H["Update error state\nclean loadingPaths/loadTasks\nobjectWillChange.send()"]
    F -- "ok" --> I["Update children/rootNodes\nclean loadingPaths/loadTasks\nobjectWillChange.send()"]
    I --> J["Spawn child Tasks for\npreviously-expanded dirs"]
Loading

Reviews (3): Last reviewed commit: "Fix stale file explorer load cancellatio..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented May 27, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The CI workflow .github/workflows/ci.yml updates xcodebuild's -skip-testing list, removing a skip for FileExplorerStoreTests and adding skips for AppDelegateShortcutRoutingTests and two CLINotifyProcessIntegrationRegressionTests cases. Sources/FileExplorerStore.swift adds an early Task.isCancelled guard to loadChildren(for:at:silent:).

Changes

CI Test Quarantine

Layer / File(s) Summary
Skip test selection update
.github/workflows/ci.yml
The xcodebuild -skip-testing list in the Run unit tests step removes FileExplorerStoreTests from the skip list and instead skips AppDelegateShortcutRoutingTests and two CLINotifyProcessIntegrationRegressionTests tests; an explanatory inline comment was removed.

FileExplorerStore runtime change

Layer / File(s) Summary
Early cancellation guard in loadChildren
Sources/FileExplorerStore.swift
FileExplorerStore.loadChildren(for:at:silent:) now returns immediately when Task.isCancelled is true, preventing further provider access, loading-state updates, or directory listing after cancellation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • manaflow-ai/cmux#4882: Also edits .github/workflows/ci.yml to change which XCTest cases are quarantined via -skip-testing.
  • manaflow-ai/cmux#4885: Modifies the same CI -skip-testing list and references overlapping CLINotifyProcessIntegrationRegressionTests / FileExplorerStoreTests entries.
  • manaflow-ai/cmux#4913: Adjusts the CI xcodebuild invocation and -skip-testing selection in the tests job.

Poem

🐰
I hopped through builds at break of day,
Skipped a test that ran astray,
When Tasks are cancelled I bound away,
Small fixes, soft paws, now code can play.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Cmux Full Internationalization ❌ Error FileExplorerStore.swift uses String(localized: "fileExplorer.error.sshFailed", ...) but this key is missing from Resources/Localizable.xcstrings, violating the full-internationalization rule. Add the missing "fileExplorer.error.sshFailed" key to Resources/Localizable.xcstrings with translations for all supported locales.
✅ Passed checks (17 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
Cmux Swift Actor Isolation ✅ Passed FileExplorerStore.swift adds early Task.isCancelled check to @MainActor loadChildren method; valid defensive improvement that does not introduce or worsen Swift 6 actor isolation issues.
Cmux Swift Blocking Runtime ✅ Passed The Swift production change adds only guard !Task.isCancelled else { return }, a non-blocking cooperative cancellation check. No blocking primitives, locks, sleeps, or delays were introduced.
Cmux No Hacky Sleeps ✅ Passed CI YAML workflow changes are explicitly out of scope per rule; FileExplorerStore.swift is Swift code covered by swift-blocking-runtime.md rule. No new sleeps/timers added to covered languages.
Cmux Algorithmic Complexity ✅ Passed PR adds early Task.isCancelled guard in FileExplorerStore.loadChildren, reducing work on cancelled tasks without introducing nested scans or unbenchmarked algorithms.
Cmux Swift Concurrency ✅ Passed The PR adds a proper Swift concurrency pattern (Task.isCancelled guard) and removes test quarantine. No legacy async patterns are introduced or materially expanded.
Cmux Swift @Concurrent ✅ Passed Swift change adds early cancellation check guard to existing @MainActor async method, preserving proper actor isolation without modifying annotations, call sites, or execution patterns.
Cmux Swift File And Package Boundaries ✅ Passed PR adds only +1 line to existing oversized FileExplorerStore.swift; matches allowed case for focused bug fixes without introducing mixed responsibilities.
Cmux Swift Logging ✅ Passed All NSLog statements in FileExplorerStore.swift are protected by #if DEBUG guards. No violations of print, file logging, or secrets exposure found.
Cmux User-Facing Error Privacy ✅ Passed PR changes are internal: FileExplorerStore adds cancellation logic; CI modifies test selection. No user-facing error messages added or modified.
Cmux Swiftui State Layout ✅ Passed PR touches existing legacy ObservableObject state incidentally. Only adds guard !Task.isCancelled to loadChildren method. No new SwiftUI state patterns introduced.
Cmux Architecture Rethink ✅ Passed Early task cancellation check in loadChildren prevents stale UI updates. Uses standard Swift concurrency pattern with clear ownership, no problematic sleep/delay/polling/locks.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds Task.isCancelled check to FileExplorerStore and updates CI test skips. No window-related code changes detected.
Title check ✅ Passed The title 'Restore file explorer remote root CI coverage' clearly summarizes the main change: re-enabling CI coverage for a previously skipped test related to file explorer remote root functionality.
Description check ✅ Passed The PR description is clear and detailed, covering the main changes, testing approach, and related issue reference.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-4524-file-explorer-store

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@lawrencecchen
lawrencecchen force-pushed the issue-4524-file-explorer-store branch from 40e534d to 8a41296 Compare May 28, 2026 00:35
@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 8a412968 Deployed May 28, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants