Skip to content

fix(browser): handle absolute local file paths - #4251

Closed
gazzua wants to merge 2 commits into
manaflow-ai:mainfrom
gazzua:fix-browser-absolute-file-path
Closed

gazzua wants to merge 2 commits into
manaflow-ai:mainfrom
gazzua:fix-browser-absolute-file-path

Conversation

@gazzua

@gazzua gazzua commented May 16, 2026 •

Copy link
Copy Markdown

Summary

  • Resolve absolute POSIX paths entered in the browser omnibar as local file:// URLs.
  • Preserve existing handling for explicit file://, HTTP(S), localhost, and search input.
  • Add regression coverage for absolute local paths, including paths with spaces.

Closes #4250

Testing

  • xcrun swift resolver harness confirms /Users/msk/Desktop/disk-cleanup-report.html resolves to file:///Users/msk/Desktop/disk-cleanup-report.html.
  • Not run locally: ./scripts/reload.sh --tag fix-local-html --launch failed because zig is not installed in this environment.
  • Not run locally: tagged xcodebuild failed because only CommandLineTools are selected and full Xcode is not installed.

Demo Video

  • Not included: this environment cannot build or launch the tagged app because required prerequisites are missing (zig, full Xcode).

Review Trigger

Bot reviews are already running on the latest commit. Manual trigger equivalent:

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

Checklist

  • I tested the change locally
  • I added or updated tests for behavior changes
  • I updated docs/changelog if needed
  • I requested bot reviews after my latest commit (copy/paste block above or equivalent)
  • All code review bot comments are resolved
  • All human review comments are resolved

@vercel

vercel Bot commented May 16, 2026

Copy link
Copy Markdown

@gazzua is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented May 16, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Recognizes trimmed inputs starting with "/" as absolute POSIX filesystem paths and converts them directly to file:// URLs via URL(fileURLWithPath:), with tests ensuring paths (including those with spaces) are preserved and returned as file URLs.

Browser Omnibar File Path Resolution

Layer / File(s) Summary
Absolute file path to file URL conversion
Sources/Panels/BrowserPanel.swift, cmuxTests/GhosttyConfigTests.swift
resolveBrowserNavigableURL(_:) now treats trimmed inputs beginning with / as absolute filesystem paths and immediately returns URL(fileURLWithPath:). Tests verify standard absolute paths and paths with spaces.

🎯 3 (Moderate) | ⏱️ ~20 minutes

I’m a rabbit in the omnibar light,
I turn bare slashes into file:// delight.
Paths with spaces, I carry through,
Local pages open, neat and true. 🐇📁

🚥 Pre-merge checks | ✅ 15 | ❌ 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 (15 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: handling absolute local file paths in the browser, matching the core objective from issue #4250.
Linked Issues check ✅ Passed The PR successfully implements the objective from #4250: it treats bare absolute POSIX paths (e.g., /Users/msk/Desktop/file.html) as local file paths, normalizing them to file:// URLs while preserving existing behavior for explicit file://, HTTP(S), localhost, and search input, with test coverage added.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the stated objective: the resolver function was modified to handle absolute paths, tests were added for this functionality, and no unrelated changes are present.
Cmux Swift Actor Isolation ✅ Passed Code addition uses only value types with pure operations. No new actor isolation issues introduced. Existing code patterns preserved.
Cmux Swift Blocking Runtime ✅ Passed No blocking/timing synchronization introduced. The resolveBrowserNavigableURL addition is a simple path check with no semaphores, sleeps, delays, main-queue syncs, or locks. Tests are deterministic.
Cmux No Hacky Sleeps ✅ Passed PR only modifies Swift files. The "cmux no hacky sleeps" rule covers TypeScript, JavaScript, shell, or build scripts. Swift is explicitly out of scope.
Cmux Swift Concurrency ✅ Passed No Swift concurrency violations found. Code is pure synchronous URL logic and XCTest with no prohibited patterns (Dispatch queues, Combine, completion handlers, fire-and-forget Tasks).
Cmux Swift @Concurrent ✅ Passed No @concurrent annotation violations. Modified synchronous function resolveBrowserNavigableURL adds simple string prefix check. No async work, no actor-isolation, no heavy operations.
Cmux Swift File And Package Boundaries ✅ Passed Focused bug fix: adds only 5 lines to existing utility function. The function is independent, testable domain logic for URL parsing used across multiple files, with clear extraction path.
Cmux Swift Logging ✅ Passed No Swift logging violations found. Production code changes add pure logic without print, debugPrint, dump, or NSLog. Test additions only use XCTAssert calls.
Cmux User-Facing Error Privacy ✅ Passed PR adds internal URL normalization logic and unit tests with no user-facing errors, alerts, or messages. Tests are explicitly allowed per privacy rules.
Cmux Swiftui State Layout ✅ Passed PR changes URL resolver logic and adds tests. No SwiftUI state changes, ObservableObject classes, or state mutations are introduced.
Cmux Architecture Rethink ✅ Passed Straightforward fix: adds absolute path check to URL resolver. No timing delays, locks, observers, or state duplication introduced. Clear invariant with good tests.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR modifies URL resolution logic and adds unit tests. No NSWindow, NSPanel, NSWindowController, SwiftUI Window/WindowGroup code added. Test class is a test-only fixture, which is allowed.
Description check ✅ Passed The PR description comprehensively covers all required template sections with clear explanations of changes, testing approach, and checklist completion.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

@greptile-apps

greptile-apps Bot commented May 16, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes omnibar handling for absolute POSIX paths by inserting an early-return in resolveBrowserNavigableURL that converts any /-prefixed input to a file:// URL, intentionally bypassing the space guard so paths with spaces work. Two regression tests cover the new path and the spaces case.

  • Production change: A 5-line guard in resolveBrowserNavigableURL (BrowserPanel.swift) exits early for strings beginning with /, calling URL(fileURLWithPath:) which always succeeds and percent-encodes spaces internally.
  • Tests: BrowserNavigableURLResolverTests in GhosttyConfigTests.swift verifies both a plain path and a path-with-spaces round-trip correctly through isFileURL and .path.

Confidence Score: 5/5

Safe to merge — the change is a small, self-contained early-return that delegates to a well-understood Foundation API, with no shared mutable state, no actor-isolation concerns, and no logging.

The new branch in resolveBrowserNavigableURL is minimal: it checks a single prefix condition, calls URL(fileURLWithPath:) (which always succeeds and handles percent-encoding internally), and returns. Placement before the space guard is intentional and correct — paths containing spaces must bypass that guard. The two regression tests cover both the normal case and the spaces case. No actor isolation, blocking primitives, legacy concurrency, or logging concerns are introduced.

No files require special attention.

Important Files Changed

Filename Overview
Sources/Panels/BrowserPanel.swift Adds an early-return branch in resolveBrowserNavigableURL that recognises any trimmed input starting with / as an absolute POSIX path and returns the corresponding file:// URL; intentionally placed before the space-guard so paths with spaces are accepted.
cmuxTests/GhosttyConfigTests.swift Adds BrowserNavigableURLResolverTests with two cases: a plain absolute path and a path containing spaces; both assert isFileURL and round-trip the unencoded .path correctly.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([input string]) --> B[trim whitespace/newlines]
    B --> C{empty?}
    C -- yes --> NILX([nil])
    C -- no --> D{starts with '/'?}
    D -- yes --> E[URL fileURLWithPath]
    E --> FILERET([file:// URL ✓])
    D -- no --> F{contains space?}
    F -- yes --> NIL2([nil])
    F -- no --> G{localhost / loopback?}
    G -- yes --> H[URL string http://…]
    H --> LOCRET([http:// URL ✓])
    G -- no --> I{URL string with http/https/file scheme?}
    I -- yes --> J([scheme URL ✓])
    I -- no --> K{contains ':' or '/'?}
    K -- yes --> L([https:// URL ✓])
    K -- no --> M{contains '.'?}
    M -- yes --> N([https:// URL ✓])
    M -- no --> NIL3([nil])
Loading

Reviews (4): Last reviewed commit: "refactor(browser): simplify absolute pat..." | Re-trigger Greptile

Comment thread Sources/Panels/BrowserPanel.swift Outdated
@gazzua

gazzua commented May 16, 2026

Copy link
Copy Markdown
Author

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

@cubic-dev-ai

cubic-dev-ai Bot commented May 16, 2026

Copy link
Copy Markdown

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

@gazzua I have started the AI code review. It will take a few minutes to complete.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the changed files
echo "=== BrowserPanel.swift ==="
cat Sources/Panels/BrowserPanel.swift

echo ""
echo "=== GhosttyConfigTests.swift (relevant test file) ==="
cat cmuxTests/GhosttyConfigTests.swift

echo ""
echo "=== List all test files to find the right one ==="
fd -e swift . cmuxTests/

Length of output: 262203


🏁 Script executed:

#!/bin/bash
echo "=== Test file ==="
cat cmuxTests/GhosttyConfigTests.swift

echo ""
echo "=== All swift test files ==="
fd -e swift . cmuxTests/

echo ""
echo "=== Find resolveBrowserNavigableURL tests ==="
rg -n "resolveBrowserNavigableURL\|BrowserNavigableURL" cmuxTests/ -A3 -B3

Length of output: 177586


@gazzua Thanks for the ping — here’s a focused review.

What looks good

  • The early absolute-path check in resolveBrowserNavigableURL(_:) is simple and correct. It returns a proper file:/// URL via URL(fileURLWithPath:), so spaces and other characters are percent-encoded as expected.
  • The new tests exercise both a basic absolute path and a path with spaces. They pass against the new behavior and protect against future regressions.

Behavioral gap and a likely regression to consider

  • Inputs that begin with “//” (network-path references) are typically interpreted by browsers as “https://…”. With the new early return on any leading “/”, entries like “//example.com” or “//localhost:3000” will now resolve to file:////example.com, which is surprising and deviates from mainstream omnibar heuristics.

Low-risk refinement to avoid that:

  • Treat “//” specifically as a network-path reference before the absolute-path branch, then keep the new absolute-path handling for single “/…”.

Illustrative patch (Swift)

 func resolveBrowserNavigableURL(_ input: String) -> URL? {
     let trimmed = input.trimmingCharacters(in: .whitespacesAndNewlines)
     guard !trimmed.isEmpty else { return nil }
 
+    // Treat network-path references (e.g., //example.com) as HTTPS.
+    // This must run before the absolute-path branch below.
+    if trimmed.hasPrefix("//") && !trimmed.hasPrefix("///") {
+        return URL(string: "https:\(trimmed)")
+    }
+
     if trimmed.hasPrefix("/") {
         return URL(fileURLWithPath: trimmed)
     }
 
     guard !trimmed.contains(" ") else { return nil }
     …
 }

Optional enhancements (nice-to-haves)

  • Tilde expansion: Support “~/Desktop/file.html” via NSString(expandingTildeInPath:), before the space guard.
    • Guard with FileManager.default.fileExists(atPath:) if you want to keep heuristics conservative.
  • Encoded local paths: If a user pastes a percent-encoded path (e.g., “/Users/me/disk%20report.html”), consider trying trimmed.removingPercentEncoding before fileURLWithPath so both encoded and unencoded forms work.

Suggested tests to add

  • Network-path reference resolution:
    • “//example.com/docs” → https://example.com/docs
    • “//localhost:3000” → http://localhost:3000 (your code prefers https; if you’d like localhost to stay http, you can special-case it, but https is generally fine).
  • Tilde (if you add it):
    • “~/Desktop/report.html” resolves to a file:// URL with the expanded absolute path.

Verdict

  • The core fix is solid and solves #4250. I recommend the small “//” pre-check to avoid surprising users who type scheme-less network URLs. Happy to draft a follow-up patch or open a tracking issue if you prefer to land this PR as-is first.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@gazzua gazzua closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Browser omnibar treats absolute local file paths as web URLs

1 participant