Repository navigation
Fix local HTML file routing in open wrapper - #684
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (4)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds local file and file URL support across the open wrapper and embedded browser: new URL/path helper functions, path-to-file-URL conversion (with Python fallback and manual encoding), revised routing to classify cmux targets vs passthrough args, and tests plus browser changes to allow file:// navigations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant Wrapper as Open Wrapper
participant Router as Routing Logic
participant Cmux as cmux Browser
participant SysOpen as system_open
User->>Wrapper: invoke with arguments
Wrapper->>Router: parse args, normalize hosts, convert local paths to file://
Router->>Router: classify into cmux_targets, passthrough_args, failed_urls
alt cmux_targets present AND passthrough_args empty AND failed_urls empty
Router->>Cmux: open cmux_targets
Cmux->>Cmux: embed/open targets (http(s) or file://)
else
Router->>SysOpen: fallback with passthrough_args + failed_urls + remaining args
SysOpen->>SysOpen: system open handling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7059c45dd
ℹ️ 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".
| elif is_html_extension "$arg"; then | ||
| if local_file_url="$(path_to_file_url "$arg")"; then | ||
| cmux_targets+=("$local_file_url") |
There was a problem hiding this comment.
The new is_html_extension branch converts any argument ending in .html/.htm into a file:// target, even when the input is not a local path (for example example.com/report.html). That means these no-scheme web links are now routed to cmux browser open file:///... instead of being passed through to /usr/bin/open as before, so users/scripts that call open with host/path-style URLs will open a non-existent local file. Please gate this conversion on local-path conditions (e.g., existing path or explicit path prefix) before rewriting.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR enhances the Major changes:
Implementation highlights:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: d7059c4 |
![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
🧹 Nitpick comments (2)
tests/test_open_wrapper.py (1)
322-332: Add a no-Pythonfile://regression to cover the fallback parser path.Current no-Python coverage validates local path conversion, but not
file://...HTML detection without Python. Adding that case would lock behavior for the shell fallback path.Also applies to: 334-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_open_wrapper.py` around lines 322 - 332, Add a regression test that exercises the shell fallback parser path for file:// HTML detection by expanding or duplicating the existing test_file_url_html_routes_to_cmux to run with the "no-Python" scenario; specifically, invoke run_wrapper with a file://... .html URL, the same intercept_setting and whitelist args used here, and assert that exit code is 0, system open (open_log) is empty, and cmux_log contains the expected "browser open {url}" entry, so the fallback parser path (used when Python is unavailable) is validated; locate and modify the test function test_file_url_html_routes_to_cmux and/or the adjacent tests that call run_wrapper to add this case.Resources/bin/open (1)
213-236: Consider falling back to shell conversion if Python conversion fails.At Line 232, any Python conversion failure exits without trying
path_to_file_url_without_python, even though a fallback exists. Using fallback on Python failure would make conversion more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/bin/open` around lines 213 - 236, The Python conversion in path_to_file_url currently returns immediately after invoking the here-doc, preventing fallback; change the control flow so that after running the "$PYTHON3_BIN" block you check its exit status and if it failed (non-zero) call path_to_file_url_without_python "$raw" instead of returning; ensure you still propagate success (return 0) when Python succeeds and propagate the non-zero exit code from the shell fallback when Python fails, referencing the path_to_file_url and path_to_file_url_without_python functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/bin/open`:
- Around line 155-157: The non-Python branch currently calls is_html_extension
on the full URL (variable without_query), which misclassifies file:// URLs that
have a host but no path (e.g., file://example.html). Update the logic so that
when the URL has the file:// scheme you strip the scheme and any authority/host
portion before calling is_html_extension: remove the leading "file://" from
without_query, then if the remainder starts with "//" drop the authority up to
the next "/" (or treat as empty path if no "/" exists), and finally call
is_html_extension on that resulting path string instead of the original
without_query.
---
Nitpick comments:
In `@Resources/bin/open`:
- Around line 213-236: The Python conversion in path_to_file_url currently
returns immediately after invoking the here-doc, preventing fallback; change the
control flow so that after running the "$PYTHON3_BIN" block you check its exit
status and if it failed (non-zero) call path_to_file_url_without_python "$raw"
instead of returning; ensure you still propagate success (return 0) when Python
succeeds and propagate the non-zero exit code from the shell fallback when
Python fails, referencing the path_to_file_url and
path_to_file_url_without_python functions.
In `@tests/test_open_wrapper.py`:
- Around line 322-332: Add a regression test that exercises the shell fallback
parser path for file:// HTML detection by expanding or duplicating the existing
test_file_url_html_routes_to_cmux to run with the "no-Python" scenario;
specifically, invoke run_wrapper with a file://... .html URL, the same
intercept_setting and whitelist args used here, and assert that exit code is 0,
system open (open_log) is empty, and cmux_log contains the expected "browser
open {url}" entry, so the fallback parser path (used when Python is unavailable)
is validated; locate and modify the test function
test_file_url_html_routes_to_cmux and/or the adjacent tests that call
run_wrapper to add this case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e77dfc80e
ℹ️ 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".
| elif is_html_extension "$arg"; then | ||
| if local_file_url="$(path_to_file_url "$arg")"; then | ||
| cmux_targets+=("$local_file_url") |
There was a problem hiding this comment.
The .html fallback currently rewrites any non-schemed token ending in .html to a file:// URL, so host/path-style inputs like example.com/report.html are treated as local files and sent to cmux browser open file:///... instead of being passed through to system open. This is a behavior regression for scripts/users that rely on no-scheme web targets, and I verified the current wrapper produces file:///tmp/.../example.com/report.html for that input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 406-408: The current fallback that returns URL(fileURLWithPath:
"/") when parent.path.isEmpty broadens file read scope; update the validation
around allowingReadAccessTo so you first confirm the URL is a file URL and has
an absolute path (use url.isFileURL && url.path.hasPrefix("/")), and if
parent.path.isEmpty do NOT return root—return nil or propagate an error/invalid
result to the caller so access is denied; apply the same fix where parent/path
fallback is used (including the code near allowingReadAccessTo and the block
around parent.path.isEmpty).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swifttests_v2/test_browser_file_url_load.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8284925687
ℹ️ 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".
| parts = urlsplit(value) | ||
| path = unquote(parts.path or "") | ||
| lower = path.lower() | ||
| if lower.endswith(".html") or lower.endswith(".htm"): |
There was a problem hiding this comment.
file_url_points_to_html only checks the parsed path suffix and never validates the file URL host, so inputs like file://example.com/report.html are now classified as local HTML and routed to cmux browser open instead of /usr/bin/open. In this commit, resolveBrowserNavigableURL also accepts file URLs based on path alone, so hosted file URLs can be misinterpreted as local paths (for example /report.html), breaking UNC/hosted file links that previously opened through the system handler. Please require an empty/localhost file host before treating a file:// target as a local HTML file.
Useful? React with 👍 / 👎.
* Route local HTML open targets to cmux browser * Keep file:// omnibar navigation inside cmux browser * Load local file URLs via WKWebView file API * Add browser regression test for local file URL loads * Address PR feedback on local HTML and file URL handling
This branch was successfully deployed
1 participant
Summary
.html/.htmtargets from the terminalopenwrapper tocmux browser open(plain paths andfile://URLs)ftp:andmailto:)python3URL encoding fallbackTesting
bash -n Resources/bin/openpython3 tests/test_open_wrapper.py(pass)./scripts/reload.sh --tag html-files-browser(build + launch succeeded)Issues
Summary by CodeRabbit
New Features
Tests