Improve join_paths helper method
#1668
Conversation
📝 WalkthroughWalkthroughMove actions now use the raw destination location and compute destinations with resolve_path(join_paths(...)) (no leading "./"); per-file and bulk moves emit detailed Activity events and Notifications with old/new paths; join_paths was refactored to trim slashes from base and each segment and drop empty parts, changing how base '' or '/' are handled. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Filament UI
participant L as ListFiles action
participant H as join_paths()
participant RP as resolve_path()
participant R as Repository (renameFiles)
participant A as Activity / Notifications
U->>F: Initiate move (location, files)
F->>L: Submit payload
note right of L #f8f9fa: Use location as-provided (no rtrim)
L->>H: join_paths(this->path, location, filename/paths)
note right of H #f1f4f8: Trim slashes per segment, drop empties, join with '/'
H-->>L: normalized path(s)
L->>RP: resolve_path(normalized path)
RP-->>L: resolved destination(s)
L->>R: renameFiles(renamePaths)
R-->>L: Result
L->>A: Activity::event('server:file.rename') + Notification(s)
A-->>F: Trigger UI notifications
F-->>U: Show outcome
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (2)
359-371: Same double-slash and relativity issue in bulk move.Mirrors the per-file action; normalize $location before composing targets to avoid "/dir//name" and unintended absolutes.
Apply:
- $location = $data['location']; - - $files = $files->map(fn ($file) => ['to' => join_paths($location, $file['name']), 'from' => $file['name']])->toArray(); + $location = $data['location']; + $files = $files + ->map(fn ($file) => [ + 'to' => resolve_path(join_paths('', $location, $file['name'])), + 'from' => $file['name'], + ]) + ->toArray();If helpful, I can add a small unit test matrix for join_paths covering root/non-root bases, trailing slashes, empty segments, and “/” as a segment.
199-206: Normalize and sanitize the “to” path before calling renameFiles
renameFiles sends your$this->pathasrootand expects eachtoentry to be relative—passing the raw$locationcan introduce leading slashes or “//” and allow absolute or traversal paths. Wrap it inresolve_path(join_paths('', $location, $file->name)):- $location = $data['location']; - $files = [['to' => join_paths($location, $file->name), 'from' => $file->name]]; + $location = $data['location']; + $to = resolve_path(join_paths('', $location, $file->name)); + $files = [['to' => $to, 'from' => $file->name]];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php(2 hunks)app/helpers.php(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (3)
438-440: Fix duplicated filename in Activity “file” property after new file creation.$path already includes the filename; joining again appends it twice.
- Activity::event('server:file.write') - ->property('file', join_paths($path, $data['name'])) + Activity::event('server:file.write') + ->property('file', $path) ->log();
190-206: Canonicalize user input before rename; align preview default
Normalize$data['locationwithresolve_path($data['location'] ?? '')before building the'to'entries, and change the placeholder’s default from'/'to''(i.e.->content(fn(Get $get…) => resolve_path(join_paths($this->path, $get('location') ?? '', $file->name)))) so the UI preview and daemon payload remain consistent.
356-373: Normalize bulk move location
In the bulk move action callback (app/Filament/Server/Resources/FileResource/Pages/ListFiles.php around line 359), replace$location = $data['location'];with
$location = resolve_path($data['location'] ?? '');to ensure the target directory is normalized before use.
♻️ Duplicate comments (1)
app/helpers.php (1)
55-65: Confirm intent: join_paths now drops leading “/” from $base (absolute → relative).This changes join_paths('/dir', 'a') from '/dir/a' to 'dir/a'. Verify all call sites (URLs vs daemon repo) expect this. If you meant to only trim trailing slashes while preserving absolute bases, apply the rtrim + root-case approach (previously suggested) and reindex $paths.
- $base = trim($base, '/'); + // Preserve absolute bases ("/foo") but normalize trailing "/". + $isRoot = ($base === '/'); + $base = $isRoot ? '' : rtrim($base, '/'); - $paths = array_map(fn (string $path) => trim($path, '/'), $paths); - $paths = array_filter($paths, fn (string $path) => strlen($path) > 0); + $paths = array_map(fn (string $path) => trim($path, '/'), $paths); + $paths = array_values(array_filter($paths, fn (string $path) => $path !== '')); - if (empty($base)) { + if ($isRoot || $base === '') { return implode('/', $paths); } return $base . '/' . implode('/', $paths);To verify impact:
#!/bin/bash # Find join_paths($this->path, ...) usages and show contexts. rg -nP -C2 'join_paths\(\s*\$this->path\s*,' # Inspect DaemonFileRepository expectations for absolute vs relative paths. rg -nP -C3 'class\s+DaemonFileRepository|function\s+(copyFile|putContent|renameFiles)\b'
🧹 Nitpick comments (2)
app/helpers.php (2)
57-58: Minor: reindex after filtering.array_filter preserves keys; implode works, but reindexing helps consistency and avoids surprises if array operations are added later.
- $paths = array_filter($paths, fn (string $path) => strlen($path) > 0); + $paths = array_values(array_filter($paths, fn (string $path) => $path !== ''));
53-54: Add a short docblock clarifying absolute/relative semantics.Explicitly document that base '' or '/' yields a relative result (no leading slash), and that segments are trimmed and empty parts removed.
- function join_paths(string $base, string ...$paths): string + /** + * Joins path segments with single slashes. + * - base '' or '/' => returns a relative path (no leading slash). + * - Each segment is trim($seg, '/'); empty parts are dropped. + */ + function join_paths(string $base, string ...$paths): string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php(2 hunks)app/helpers.php(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (3)
196-196: Normalize preview input (handle backslashes/whitespace).Users often paste Windows-style paths. Normalize before joining so the preview is accurate.
- ->content(fn (Get $get, File $file) => resolve_path(join_paths($this->path, $get('location') ?? '/', $file->name))), + ->content(fn (Get $get, File $file) => resolve_path( + join_paths( + $this->path, + str_replace('\\', '/', trim((string) ($get('location') ?? '/'))), + $file->name + ) + )),
356-356: Unify bulk preview behavior with per-file and normalize input.Use the same default “/” and handle backslashes/whitespace so root previews don’t render empty at “/”.
- ->content(fn (Get $get) => resolve_path(join_paths($this->path, $get('location') ?? ''))), + ->content(fn (Get $get) => resolve_path( + join_paths( + $this->path, + str_replace('\\', '/', trim((string) ($get('location') ?? '/'))) + ) + )),
359-371: Sanitize bulk $location and optionally pre-normalize each “to”.This avoids odd paths if users type “.\” or include spaces and keeps Activity/Notification aligned with actual ops.
- $location = $data['location']; + $location = str_replace('\\', '/', trim((string) ($data['location'] ?? ''))); @@ - $files = $files->map(fn ($file) => ['to' => join_paths($location, $file['name']), 'from' => $file['name']])->toArray(); + $files = $files->map( + fn ($file) => ['to' => resolve_path(join_paths($location, $file['name'])), 'from' => $file['name']] + )->toArray();If the daemon requires raw relative paths (and does its own normalization), skip resolving “to” above and keep only the $location sanitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (4)
app/helpers.php (2)
resolve_path(69-87)join_paths(53-65)app/Filament/Server/Resources/FileResource/Pages/EditFiles.php (1)
getDaemonFileRepository(232-239)app/Repositories/Daemon/DaemonFileRepository.php (1)
putContent(52-64)app/Services/Activity/ActivityLogService.php (1)
property(108-116)
🔇 Additional comments (2)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (2)
441-441: LGTM: log the exact file path used.Using the computed $path in the activity record is clearer and avoids drift.
199-206: Sanitize and normalize$locationbefore useTrim whitespace, convert backslashes to forward slashes, and normalize dot-segments via
resolve_path()so both the client logs and daemon API receive a consistent, safe path.@@ app/Filament/Server/Resources/FileResource/Pages/ListFiles.php Lines 199-206 - $location = $data['location']; - $files = [['to' => join_paths($location, $file->name), 'from' => $file->name]]; + $locationRaw = (string) ($data['location'] ?? ''); + $location = str_replace('\\', '/', trim($locationRaw)); + $to = resolve_path(join_paths($location, $file->name)); + $files = [['to' => $to, 'from' => $file->name]]; @@ - $newLocation = resolve_path(join_paths($this->path, $location, $file->name)); + $newLocation = resolve_path(join_paths($this->path, $to));Confirm the daemon’s
/files/renameendpoint does its own path normalization and prevents directory-traversal before merging.
Better handling for trailing
/or empty paths.