Refresh page after file updates - #1759
Conversation
📝 WalkthroughWalkthroughMoves Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Files UI (ListFiles / EditFiles)
participant Backend as Server/Filesystem API
rect rgb(250,250,255)
note over UI: User triggers a file action (create/rename/move/delete/upload/edit/permissions/archive...)
User->>UI: Trigger action
UI->>Backend: Perform action
Backend-->>UI: Success / Error
end
alt Success
note over UI #E6F7EA: Centralized refresh / redirect
UI->>UI: refreshPage(oneBack?) / redirectToList()
UI->>Backend: Requery via File::get(server, path)
Backend-->>UI: Files list (ordered by is_directory)
UI-->>User: Render updated list
else Error
note over UI #FFF4E6: Error handling (no refresh)
UI-->>User: Show error message
end
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Actionable comments posted: 1
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/Files/Pages/ListFiles.php (2)
281-294: Permissions action doesn’t refresh; table stays stale.After chmod, the mode column won’t update until a manual reload. Call refreshPage() here too.
Notification::make() ->title(trans('server/file.actions.permissions.notification', ['mode' => $mode])) ->success() ->send(); + + $this->refreshPage();
504-513: Fix file upload: UploadedFile::getContent() is not a method.Use UploadedFile::get() (or file_get_contents($file->getRealPath())). Current code will error on upload.
- $this->getDaemonFileRepository()->putContent(join_paths($this->path, $file->getClientOriginalName()), $file->getContent()); + $this->getDaemonFileRepository()->putContent( + join_paths($this->path, $file->getClientOriginalName()), + $file->get() + );Reference for UploadedFile::get(). (laravel.com)
Also applies to: 523-524
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (2)
app/Models/File.php (1)
File(29-215)app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
getUrl(285-288)
🔇 Additional comments (4)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (4)
169-169: Great: unified post‑action refresh calls.Once refreshPage() targets the current path, these hooks will resolve the “ghost file” issue reported in #1694.
Also applies to: 188-188, 231-231, 320-320, 340-340, 358-358, 390-390, 417-417, 435-435, 461-461, 491-491, 523-523
39-39: Import looks right.FilamentView is the correct facade for SPA navigate checks.
62-65: Title method relocation is fine.No behavior change; improves readability.
98-99: Query tweak LGTM.Ordering directories first via orderByDesc('is_directory') with default name sort is sensible.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
98-98: Dirs-first query is good; consider pushing name sort into the query.Current behavior yields folders first via orderByDesc('is_directory') with defaultSort('name'). You could express both in SQL for clarity:
- Keep folders-first, then name, and drop defaultSort.
- ->query(fn () => File::get($server, $this->path)->orderByDesc('is_directory')) - ->defaultSort('name') + ->query(fn () => File::get($server, $this->path) + ->orderByDesc('is_directory') + ->orderBy('name'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (3)
app/Models/File.php (1)
File(29-215)app/helpers.php (1)
join_paths(53-65)app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
getUrl(285-288)
🔇 Additional comments (4)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (4)
62-65: Title method placement LGTM; ensure translation exists.Moving getTitle() up is fine; verify the key server/file.title exists across locales.
169-169: Good: consistent refresh after file mutations.Calling $this->refreshPage() after rename/copy/move/archive/unarchive/delete/bulk/new/upload meets the issue’s goal (ListFiles reflects changes).
Please sanity-check each action in UI: rename, copy, move, archive/unarchive, delete (single/bulk), create file/folder, upload (files and URL).
Also applies to: 188-188, 231-231, 320-320, 340-340, 358-358, 390-390, 417-417, 435-435, 454-454, 485-485, 528-528
579-583: Refactor refreshPage signature; avoid dirname-based branching.Boolean $oneBack with dirname($this->path) is brittle and caused the above misnavigation. Prefer an optional path param and default to the current path.
- private function refreshPage(bool $oneBack = false): void + private function refreshPage(?string $path = null): void { - $url = self::getUrl(['path' => $oneBack ? dirname($this->path) : $this->path]); + $path ??= $this->path; + $url = self::getUrl(['path' => $path]); $this->redirect($url, FilamentView::hasSpaMode($url)); }Optionally, if your routes expect encoded paths, wrap $path with encode_path(...) here for consistency with recordUrl/create-directory links.
462-462: Don’t navigate to parent on errors; refresh current path instead.On FileExistsException you call refreshPage(true), which goes one directory up. This contradicts the objective to refresh the current listing and matches prior feedback. Use the current path.
- $this->refreshPage(true); + $this->refreshPage();Also applies to: 495-495
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
202-206: Nice SPA‑aware redirect helper; consider reuse/DRY with ListFiles.This mirrors ListFiles::refreshPage(). If more pages need this, consider extracting a tiny trait/utility (e.g., SpaRedirects::redirectTo(url)) to avoid duplication and keep behavior uniform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Files/Pages/EditFiles.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
ListFiles(50-621)
🔇 Additional comments (2)
app/Filament/Server/Resources/Files/Pages/EditFiles.php (2)
100-101: Centralizing “save and close” redirect is good.Routing back via a single helper improves consistency and respects SPA mode. LGTM.
170-196: Redirect from within CodeEditor default: please verify no edge-case regressions.Calling redirectToList() during the field’s default() evaluation is pragmatic, but Livewire/Filament can be sensitive to redirects during form schema evaluation. Please sanity‑check:
- SPA enabled: no double navigation/flicker, and banners persist/appear on ListFiles.
- SPA disabled: no “redirect during render” warnings and no blank frame flashes.
- ConnectionException: ListFiles shows the intended banner and there’s no redirect loop if the daemon remains down.
If any issues surface, consider deferring the redirect (e.g., emit a browser event to navigate after first render) or short‑circuiting earlier in mount(). For now, a quick manual verification across both modes should suffice.
2 participants
Closes #1694