Repository navigation
Open markdown files with shared viewer path - #4285
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR migrates AppDelegate, ContentView, RightSidebarToolPanel, and Workspace drop handlers to use ChangesFile-open routing migration to openFileSurfaces
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryRoutes all file-opening entry points (external opens via
Confidence Score: 5/5Safe to merge; the change is a clean unification of three independent file-open entry points through a well-tested shared helper with no new state or timing dependencies. All three call-sites (external open, file explorer, sidebar) are updated consistently. Drop insertion preserves always-create semantics by omitting No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "harden markdown reuse test" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertEqual(markdownPanels.first?.filePath, fileURL.path) | ||
| XCTAssertEqual(markdownPanels.first?.displayMode, .preview) | ||
| XCTAssertTrue(workspace.panels.values.compactMap { $0 as? FilePreviewPanel }.isEmpty) | ||
|
|
There was a problem hiding this comment.
XCTFail without early return in #else branch
XCTFail records a test failure but does not stop execution. In a non-DEBUG build the test continues past the #else block and calls openFilePreviewInPreferredMainWindow on an AppDelegate whose window context was never registered, which could produce a misleading second failure or a crash rather than a single clear signal. Adding return after XCTFail keeps the failure atomic and avoids confusing downstream assertions.
There was a problem hiding this comment.
Fixed by returning after the non-DEBUG XCTFail so that branch reports a single atomic failure.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/MarkdownPanelTests.swift`:
- Around line 104-120: The test creates a TabManager but never sets it as the
active manager, so call TerminalController.shared.setActiveTabManager(manager)
right after creating manager (same spot as in
testFileOpenRoutesMarkdownFilesToPreviewMarkdownPanel) and add cleanup in the
defer to reset the active manager to nil
(TerminalController.shared.setActiveTabManager(nil)) to restore test isolation;
references: TabManager, TerminalController.shared.setActiveTabManager(...), and
AppDelegate.openFilePreviewInPreferredMainWindow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17dbc686-0a40-4b8b-906a-a2b12e59ebd8
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/RightSidebarToolPanel.swiftSources/Workspace.swiftcmuxTests/MarkdownPanelTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/MarkdownPanelTests.swift`:
- Around line 132-145: Capture the initial MarkdownPanel instance (e.g., let
original = workspace.panels.values.compactMap { $0 as? MarkdownPanel }.first)
and store its identity (use the instance itself or a unique property like
objectIdentifier(original) or original.id if available), then call
appDelegate.openFilePreviewInPreferredMainWindow(...) and assert that the same
MarkdownPanel instance still exists in workspace.panels (compare identities, not
just count) to ensure reuse rather than replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5d2f8b7-a9e7-47aa-9489-0791d7fff041
📒 Files selected for processing (1)
cmuxTests/MarkdownPanelTests.swift
manaflow-ai#4285 routes all .md file opens through Workspace.openFileSurfaces and sends them directly to MarkdownPanel (the rendered viewer), so a .md file no longer lands in FilePreviewPanel from any current entry point. The in-header markdown preview button is therefore unreachable in normal flows and would only fire on legacy session restores. Remove the button, isMarkdownFile, and owningWorkspace() helper. Keep the .mkd/.mkdn/.mdwn/.mdown additions in textExtensions inline, since upstream MarkdownPanelFileLinkResolver.isMarkdownPathLike only covers md/markdown/mkd/mdx — the long-tail aliases still benefit text-mode detection if they ever reach FilePreviewPanel. PR scope is now syntax highlighting only.
This branch was successfully deployed
1 participant
Summary:
Tests:
Note
Medium Risk
Changes core file-opening paths (external open, sidebar open, drag-and-drop) to a unified helper that can reuse existing tabs and route
.mdfiles toMarkdownPanel, so regressions could affect tab creation/focus and drop behavior.Overview
Unifies file opening behavior across the app. External file opens (
AppDelegate), sidebar opens (ContentView/RightSidebarToolPanel), and drag-and-drop inserts (Workspace) now route throughWorkspace.openFileSurfaces(..., reuseExisting: true)so markdown paths open as previewMarkdownPanels and reopening focuses the existing surface instead of creating duplicates.Simplifies drop handling. Removes the bespoke
openDroppedFileSurfaceshelper and renamessplitPaneWithDroppedFiletosplitPaneWithFileSurface, relying on the shared open/split logic and returning success based on whether any panels were opened.Adds coverage for the new external-open path. Introduces
MarkdownPanelTests.testExternalFileOpenRoutesMarkdownFilesToPreviewMarkdownPanelto verify.mdexternal opens create a preview markdown surface and repeated opens reuse the same panel.Reviewed by Cursor Bugbot for commit 2607bd1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Route all markdown file opens through the shared
openFileSurfacespath so they always open as a previewMarkdownPaneland reuse existing tabs. This unifies external, file explorer, and drag-and-drop behavior and removes duplicate code.Bug Fixes
MarkdownPaneland reuse an existing tab.Refactors
openFileSurfaces; removedopenDroppedFileSurfaces; renamedsplitPaneWithDroppedFiletosplitPaneWithFileSurface.AppDelegate,ContentView, andRightSidebarToolPanelto callopenFileSurfaces(focus: true, reuseExisting: true).MarkdownPanelinstance, and noFilePreviewPanel.Written for commit 2607bd1. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Refactor
Tests