Restore file preview review CI coverage

[lawrencecchen]

Summary

Restores cmuxTests/FilePreviewReviewFeedbackTests to the required macOS unit-test job by removing the broad class-level skip from .github/workflows/ci.yml.

This continues the quarantine reduction tracked by #4524.

Verification

• git diff --check
• ruby -e 'require "yaml"; YAML.load_file(".github/workflows/ci.yml"); puts "ci.yml parsed"'\n- Hosted PR checks will exercise the restored class.\n

---

^{Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.}

---

Note

Low Risk
CI skip removal and XCTest cleanup only; no app or auth/data-path changes.

Overview
Re-enables FilePreviewReviewFeedbackTests on the macOS unit-test job by dropping the class-level -skip-testing entry from .github/workflows/ci.yml, continuing quarantine rollback for file-preview review coverage.

Tests in that class now tear down UI state so CI is less flaky: defer { panel.close() } on FilePreviewPanel fixtures, window cleanup in the focus-coordinator test, and workspace.teardownAllPanels() in the file-open pane test.

^{Reviewed by Cursor Bugbot for commit 2cfc97f. Bugbot is set up for automated code reviews on this repo. Configure here.}

---

Summary by cubic

Restores CI for cmuxTests/FilePreviewReviewFeedbackTests by removing its skip in .github/workflows/ci.yml, and hardens tests with defer { panel.close() }, window cleanup, and workspace.teardownAllPanels() to reduce flakiness. Continues the quarantine rollback in #4524.

^{Written for commit 2cfc97f. Summary will update on new commits. Review in cubic}

Summary by CodeRabbit

• Chores
  • Updated CI test configuration to change which unit tests are executed in the pipeline, altering the set of tests excluded from the main run.
• Tests
  • Improved unit tests to ensure UI and workspace resources are reliably cleaned up after each run, reducing flakiness and preventing lingering state between tests.

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
cmux	Ready	Preview, Comment	May 27, 2026 3:38pm
cmux-staging	Building	Preview, Comment	May 27, 2026 3:38pm

[chatgpt-codex-connector]

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3030c496-764d-4d7f-8fb2-1e0de2fc3f6f

📥 Commits

Reviewing files that changed from the base of the PR and between d03a22d and 2cfc97f.

📒 Files selected for processing (2)

• .github/workflows/ci.yml
• cmuxTests/FilePreviewReviewFeedbackTests.swift

💤 Files with no reviewable changes (1)

• .github/workflows/ci.yml

---

📝 Walkthrough

Walkthrough

CI xcodebuild -skip-testing list is changed (FilePreviewReviewFeedbackTests now runs; three other tests are skipped). Tests in cmuxTests/FilePreviewReviewFeedbackTests.swift add defer cleanup to close Quick Look panels, detach/close windows, and teardown workspace panels after tests.

Changes

Unit Test Quarantine Updates

Layer / File(s)	Summary
xcodebuild test skip list .github/workflows/ci.yml	The xcodebuild -skip-testing argument list in the Run unit tests step is modified: cmuxTests/FilePreviewReviewFeedbackTests removed from the skip list and three specific test methods/classes added to the skip list.
FilePreviewReviewFeedbackTests cleanup cmuxTests/FilePreviewReviewFeedbackTests.swift	Adds defer cleanup in Quick Look tests to call panel.close(), detaches and closes NSWindow in the focus coordinator test, and calls workspace.teardownAllPanels() via defer in the file-open destination test.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• Replace broad macOS app-host CI quarantines with gated coverage #4524: Related edits to the CI -skip-testing list and test quarantine adjustments for FilePreviewReviewFeedbackTests.

Possibly related PRs

• manaflow-ai/cmux#4882: Also modifies the .github/workflows/ci.yml -skip-testing list for macOS unit tests.
• manaflow-ai/cmux#4878: Also updates the macOS tests job xcodebuild -skip-testing list, affecting which tests are excluded.

Poem

🐇 I close the panel, then I hop away,
Defer keeps things tidy at the end of the day,
CI skips shuffled in a gentle spin,
Tests run cleaner — a rabbit’s grin,
Hooray for tidy teardown, hip-hop hooray!

🚥 Pre-merge checks | ✅ 17 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (17 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Restore file preview review CI coverage' directly matches the main objective of restoring the FilePreviewReviewFeedbackTests to CI by removing its skip entry from the workflow.
Description check	✅ Passed	The PR description includes a Summary section explaining what changed and why, and Verification steps demonstrating how the change was tested, covering most required template sections.
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	PR only modifies test files (cmuxTests/FilePreviewReviewFeedbackTests.swift) and CI configuration; test files are explicitly exempted from actor isolation checks per the rule guidelines.
Cmux Swift Blocking Runtime	✅ Passed	PR only modifies CI config and test files with deterministic resource cleanup defer blocks; no production Swift code changes introduce blocking/timing synchronization.
Cmux No Hacky Sleeps	✅ Passed	All changes are test-only in cmuxTests/; waitForPanelSave uses Task.yield() polling which is deterministic test scaffolding allowed by the rule.
Cmux Algorithmic Complexity	✅ Passed	PR contains only test code and CI workflow config changes; no production code modifications. Per algorithmic-complexity.md, test-only code is exempt from this rule.
Cmux Swift Concurrency	✅ Passed	PR adds only XCTest code with modern async/await and Task.yield patterns; no legacy Dispatch queues, Combine, completion handlers, or fire-and-forget Tasks introduced.
Cmux Swift @Concurrent	✅ Passed	FilePreviewReviewFeedbackTests.swift has proper @MainActor isolation; all async functions intentionally inherit MainActor context for test coordination, complying with swift-concurrent-annotation.md.
Cmux Swift File And Package Boundaries	✅ Passed	PR only modifies test code (cmuxTests/) with 7 lines of resource cleanup and CI config changes; test fixtures are explicitly allowed under the rule.
Cmux Swift Logging	✅ Passed	PR modifies only test code and CI config with no logging statements, only defer blocks for cleanup.
Cmux User-Facing Error Privacy	✅ Passed	PR contains only test and CI workflow changes. Per the user-facing error rule, tests are explicitly allowed to pass, and no production code with user-facing errors was added.
Cmux Full Internationalization	✅ Passed	PR contains only test-file and CI-workflow changes; no user-facing strings, localization files, or production code modifications requiring i18n compliance.
Cmux Swiftui State Layout	✅ Passed	PR contains only CI/test infrastructure and AppKit-based test code. No SwiftUI changes, state management, @Published/@observable patterns, or layout violations detected.
Cmux Architecture Rethink	✅ Passed	Test-only cleanup changes (defer blocks for resource teardown) do not violate Swift architectural rethink rules; they fall under the allowed case of test-only synchronization with clear invariants.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PR only adds cleanup defer blocks to test-only window fixtures in cmuxTests/FilePreviewReviewFeedbackTests.swift; no production window code is added or materially changed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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-filepreview-review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

Restores CI coverage for cmuxTests/FilePreviewReviewFeedbackTests by removing the class-level -skip-testing entry from the macOS unit-test job, and adds explicit AppKit/workspace teardown to the affected tests to prevent resource leaks on CI runners.

• CI: Removes the FilePreviewReviewFeedbackTests skip line from .github/workflows/ci.yml; all other skips are left in place.
• Test teardown: Adds defer { panel.close() } to three Quick Look panel tests, defer { window.contentView = nil; window.close() } to the focus-coordinator test, and defer { workspace.teardownAllPanels() } to the file-open routing test.

Confidence Score: 5/5

This PR is safe to merge — it touches only CI config and test scaffolding with no production code changes.

The change is confined to a CI skip-list removal and test-only teardown additions. Defer ordering in all modified tests is correct (panels and windows are released before their backing files are deleted), and no production Swift, state, or runtime code is touched.

No files require special attention.

Important Files Changed

Filename	Overview
.github/workflows/ci.yml	Removes one -skip-testing entry to re-enable FilePreviewReviewFeedbackTests in the macOS unit-test job; surrounding quarantine list is unchanged.
cmuxTests/FilePreviewReviewFeedbackTests.swift	Adds defer-based teardown (panel.close, window cleanup, workspace.teardownAllPanels) to five tests; no production code is touched and defer ordering is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[macOS Unit Test Job] --> B{FilePreviewReviewFeedbackTests}
    B -->|Previously skipped| C[Tests not run]
    B -->|After this PR| D[Tests run]
    D --> E[testSavingTextViewUsesChordedSaveShortcut]
    D --> F[testQuickLookSessionCloseDoesNotDeactivateMountedRepresentableView]
    D --> G[testQuickLookSessionDismantlingRetiredViewDoesNotResetActivePreviewItem]
    D --> H[testFocusCoordinatorKeepsPendingFocusUntilEndpointHasWindow]
    D --> I[testFileOpenHonorsExplicitPaneDestinationInsteadOfReusingExistingPreview]

Loading

_{Reviews (3): Last reviewed commit: "Restore file preview review CI coverage" | Re-trigger Greptile}