Repository navigation
Fix Cmd+click file path punctuation trimming - #2831
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded trailing punctuation trimming logic for cmd+click file path resolution. The implementation introduces sanitization helpers that use delimiter-balance heuristics to strip sentence-terminating punctuation while preserving legitimate path characters, along with comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 SummaryThis PR fixes Cmd+click file-path resolution by stripping trailing sentence punctuation ( Confidence Score: 5/5Safe to merge — the trimming logic is correct and the only finding is a missing test for a behaviour the implementation already handles properly. All findings are P2. The core implementation (balance-tracking loop, candidate ordering, empty-string guard) is correct. The single concern is that testPreservesBalancedParensInMiddleOfPath exercises a trivial non-paren-ending path rather than the important 'balanced closer at path end is preserved' case — a missing regression test, not a bug. cmuxTests/TerminalAndGhosttyTests.swift — consider adding the missing 'balanced closer at path end' test case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rawText from Cmd+click] --> B[cmuxQuicklookPathCandidates]
B --> C[append: trimmed literal]
C --> D{cmuxTrimTerminalPathTrailingPunctuation}
D --> E{punctuationTrimmed != trimmed?}
E -- yes --> F[append: punctuation-trimmed fallback]
E -- no --> G[skip fallback]
B --> H[append: unescaped variants]
B --> I[append: unquoted variants]
subgraph cmuxTrimTerminalPathTrailingPunctuation
T1[scan from end] --> T2{trailing char type?}
T2 -- sentence punct / quote --> T3[strip and continue]
T3 --> T1
T2 -- closing pair --> T4{cmuxHasUnmatchedOpeningPathDelimiter?}
T4 -- no unmatched opener --> T3
T4 -- has unmatched opener --> T5[stop - preserve balanced closer]
T2 -- other char --> T5
end
F --> R[cmuxResolveQuicklookPath: iterate candidates]
G --> R
H --> R
I --> R
R --> S{fileExists?}
S -- first match --> Z[return path]
S -- no match --> R
Reviews (1): Last reviewed commit: "Trim trailing punctuation from cmd-click..." | Re-trigger Greptile |
| func testPreservesBalancedParensInMiddleOfPath() { | ||
| XCTAssertEqual( | ||
| cmuxTrimTerminalPathTrailingPunctuationForTesting( | ||
| "/tmp/fixtures/report (draft)/notes.txt" | ||
| ), | ||
| "/tmp/fixtures/report (draft)/notes.txt" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Test doesn't cover the key "balanced closer at path end" scenario
The input /tmp/fixtures/report (draft)/notes.txt ends in .txt, not ), so the loop breaks immediately on the first character without exercising the closing-pair preservation logic at all. The test verifies "don't corrupt a path that has parens inside it" — which is fine — but the advertised behaviour ("preserves balanced parens") is the case where the path itself ends with a ) that IS balanced, e.g. /tmp/fixtures/report (draft).
For that path: the balance check on characters[..<(len-1)] (/tmp/fixtures/report (draft) finds one unmatched (, so cmuxHasUnmatchedOpeningPathDelimiter returns true and the closing paren is preserved. This is the correct and important case — but it is currently untested.
Consider adding:
func testPreservesTrailingCloseParenWhenBalancedWithOpener() {
XCTAssertEqual(
cmuxTrimTerminalPathTrailingPunctuationForTesting(
"/tmp/fixtures/report (draft)"
),
"/tmp/fixtures/report (draft)"
)
}…ick-trailing-punctuation Fix Cmd+click file path punctuation trimming
Summary
Testing
Closes #2807
Note
Low Risk
Low risk: changes are limited to Cmd+click file path candidate parsing and add regression tests; main risk is minor behavior changes in which path variant gets selected when filenames legitimately end with punctuation.
Overview
Improves Cmd+click Quick Look path resolution by adding a “smart” fallback candidate that trims trailing sentence punctuation/quotes and certain unmatched closing delimiters (e.g.,
),]) so paths followed by punctuation in terminal output still resolve.Candidate ordering is updated to try the literal token first and only then try the punctuation-trimmed variant, reducing false negatives while preserving legitimate filenames that end with punctuation. Adds focused XCTest coverage for the trimming rules and resolution preference behavior.
Reviewed by Cursor Bugbot for commit c77d2bd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Smart-trims trailing punctuation on Cmd+click file paths so sentence-ending characters don’t break resolution. Keeps legitimate trailing punctuation by trying the literal path first, then a trimmed fallback.
Written for commit c77d2bd. Summary will update on new commits.
Summary by CodeRabbit