Use absolute paths in clipboard text instead of @.rfa prefix#86
Merged
Conversation
The @-mention only resolved relative to the agent's current working directory, which silently broke when the user pasted from a different shell. Drop the @ and emit the full path so the file resolves no matter where it's pasted. https://claude.ai/code/session_01QsSDogBG9br2K8wUnsHjZN
📝 WalkthroughWalkthroughThe export clipboard text now references the full file path instead of a basename-only reference. ChangesExport Path Clarity
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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/CommentExporter.php (1)
24-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbsolute-path contract is not enforced at the exporter boundary.
$pathis only as absolute as incoming$repoPath. If a caller passes a relative repo path, clipboard text becomes relative again, which conflicts with this PR’s stated behavior.Proposed fix
public function export(string $repoPath, array $comments, string $globalComment = '', array $diffContext = [], CommentExportKind $kind = CommentExportKind::Review): array { + if (! str_starts_with($repoPath, '/')) { + throw new RuntimeException("Repository path must be absolute: {$repoPath}"); + } + $basename = date('Ymd_His').'_comments_'.Str::random(8); $rfaDir = $repoPath.'/.rfa'; $path = "{$rfaDir}/{$basename}.md";Also applies to: 38-46
🤖 Prompt for 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. In `@app/Services/CommentExporter.php` around lines 24 - 29, The export method currently builds $rfaDir and $path directly from the incoming $repoPath, so a relative $repoPath yields relative clipboard paths; fix by normalizing/enforcing an absolute repo path at the start of export: resolve $repoPath with realpath($repoPath) and if realpath returns false, prefix $repoPath with getcwd() (or otherwise convert to an absolute path) before computing $rfaDir and $path; apply the same normalization wherever $repoPath is used later in this file (lines around the other $rfaDir/$path uses referenced in the comment). Ensure you update the export method and any other functions in this file that construct $rfaDir/$path so $path is always absolute.
🧹 Nitpick comments (1)
tests/Unit/CommentExporterTest.php (1)
53-59: ⚡ Quick winAdd a regression test for relative
repoPathinput.
These assertions validate the happy path, but not the contract edge case: passing a relative repository path should be rejected (or normalized), so clipboard output never regresses to relative paths.As per coding guidelines, "tests/**/*.php: Every code change must be programmatically tested."
🤖 Prompt for 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. In `@tests/Unit/CommentExporterTest.php` around lines 53 - 59, Add a regression test in CommentExporterTest that verifies relative repoPath inputs are rejected: create a new test (e.g., test('rejects relative repoPath', function () { ... })) that calls $this->exporter->export('relative/path', [], 'test') and uses $this->expectException(InvalidArgumentException::class) (or the project's canonical validation exception) before invoking export; this ensures the export(string $repoPath, ...) contract on the export method enforces absolute/normalized paths and prevents clipboard output from containing relative paths.
🤖 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.
Outside diff comments:
In `@app/Services/CommentExporter.php`:
- Around line 24-29: The export method currently builds $rfaDir and $path
directly from the incoming $repoPath, so a relative $repoPath yields relative
clipboard paths; fix by normalizing/enforcing an absolute repo path at the start
of export: resolve $repoPath with realpath($repoPath) and if realpath returns
false, prefix $repoPath with getcwd() (or otherwise convert to an absolute path)
before computing $rfaDir and $path; apply the same normalization wherever
$repoPath is used later in this file (lines around the other $rfaDir/$path uses
referenced in the comment). Ensure you update the export method and any other
functions in this file that construct $rfaDir/$path so $path is always absolute.
---
Nitpick comments:
In `@tests/Unit/CommentExporterTest.php`:
- Around line 53-59: Add a regression test in CommentExporterTest that verifies
relative repoPath inputs are rejected: create a new test (e.g., test('rejects
relative repoPath', function () { ... })) that calls
$this->exporter->export('relative/path', [], 'test') and uses
$this->expectException(InvalidArgumentException::class) (or the project's
canonical validation exception) before invoking export; this ensures the
export(string $repoPath, ...) contract on the export method enforces
absolute/normalized paths and prevents clipboard output from containing relative
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77c75cc7-a6e3-457d-afb0-abc1071c5ff6
📒 Files selected for processing (3)
README.mdapp/Services/CommentExporter.phptests/Unit/CommentExporterTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changed the clipboard text generation to use absolute file paths instead of the
@.rfa/prefix notation. This makes the exported paths more explicit and easier to work with in various contexts.Key Changes
clipboardText()method to accept the full file path instead of just the basename@.rfa/{basename}Implementation Details
export()method now passes the full$pathtoclipboardText()instead of$basenamehttps://claude.ai/code/session_01QsSDogBG9br2K8wUnsHjZN
Summary by CodeRabbit
Bug Fixes
Documentation