MC Modrinth: Implement mod/plugin management with update and uninstall capabilities#96
MC Modrinth: Implement mod/plugin management with update and uninstall capabilities#96H1ghSyst3m wants to merge 17 commits intopelican-dev:mainfrom
Conversation
Introduces update and uninstall actions for Minecraft mods/plugins, with UI enhancements for status tracking and confirmation dialogs. Adds persistent metadata management via a .modrinth-metadata.json file to track installed mods, versions, and installation dates. Updates translations, documentation, and bumps plugin version to 1.1.0.
Deleted 'download' related keys from both German and English language files as they are no longer needed.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds install/update/uninstall mod management with persistent Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Page as MinecraftModrinthProjectPage
participant Service as MinecraftModrinthService
participant API as Modrinth API
participant FileRepo as DaemonFileRepository
participant Server as Server
Note over User,Server: Install Mod Flow
User->>Page: Click Install
Page->>API: Request version & file list
API-->>Page: Return versions & files
Page->>Page: Select primary file & validate filename
Page->>API: Download file content
API-->>Page: File content
Page->>FileRepo: Save file to server path
FileRepo->>Server: Persist file
Page->>Service: Save metadata (.modrinth-metadata.json)
Service->>FileRepo: Write metadata
Page->>Page: Clear caches & emit notification
Page-->>User: Install success
Note over User,Server: Update Mod Flow
User->>Page: Click Update
Page->>Service: Load installed metadata
Page->>API: Fetch available versions
API-->>Page: Versions list
Page->>Page: Detect newer version
User-->>Page: Confirm update
Page->>API: Download new file
API-->>Page: New file
Page->>FileRepo: Replace file (save new, delete old)
Page->>Service: Update metadata
Page-->>User: Update success
Note over User,Server: Uninstall Mod Flow
User->>Page: Click Uninstall
Page->>User: Confirm uninstall
User-->>Page: Approve
Page->>Service: Remove metadata entry
Service->>FileRepo: Update metadata file
Page->>FileRepo: Delete mod file from server
Page->>Page: Clear caches & emit notification
Page-->>User: Uninstall success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
![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 (1)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (1)
18-20:⚠️ Potential issue | 🟠 MajorRemove unused
Sectionimport to fix Pint failure.The lint failure (
no_unused_imports) points to this unused import.🧹 Suggested fix
-use Filament\Schemas\Components\Section;
🤖 Fix all issues with AI agents
In
`@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php`:
- Around line 213-232: The primaryFile['filename'] from Modrinth must be
validated/sanitized before using it in fileRepository->pull and
MinecraftModrinth::saveModMetadata; call the existing validateFilename() (or
equivalent sanitizer) on $primaryFile['filename'] inside the block that gets
$primaryFile (around getPrimaryFile(...) and before
$fileRepository->setServer(...)->pull(...)) and use the sanitized filename for
both the pull destination and the saved metadata (replace uses of
$primaryFile['filename'] with the sanitized variable); apply the same change to
the similar logic in the other occurrence referenced (lines ~319-351) so no
unvalidated filename is persisted or used for file paths.
In `@minecraft-modrinth/src/Services/MinecraftModrinthService.php`:
- Around line 188-196: The code checks the result of
$fileRepository->setServer($server)->putContent(...) against false, but
DaemonFileRepository::putContent() returns an Illuminate\Http\Client\Response,
so replace the boolean check with the response failure check: call ->failed() on
the $result (i.e. if ($result->failed()) return false;). Update both places
where metadata is written (the blocks that call getMetadataFilePath($server) and
then putContent via $fileRepository->setServer(...)->putContent) to use
$result->failed() instead of === false.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
minecraft-modrinth/README.mdminecraft-modrinth/lang/de/strings.phpminecraft-modrinth/lang/en/strings.phpminecraft-modrinth/plugin.jsonminecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.phpminecraft-modrinth/src/Services/MinecraftModrinthService.php
🧰 Additional context used
🧬 Code graph analysis (1)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (2)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (2)
fromServer(29-45)getFolder(21-27)minecraft-modrinth/src/Enums/MinecraftLoader.php (1)
fromServer(23-30)
🪛 GitHub Actions: Lint
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
[error] 1-1: Pint: 'no_unused_imports' issue detected. Remove unused imports in MinecraftModrinthProjectPage.php.
🪛 GitHub Check: PHPStan (8.2)
minecraft-modrinth/src/Services/MinecraftModrinthService.php
[failure] 224-224:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
[failure] 194-194:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
🪛 GitHub Check: PHPStan (8.3)
minecraft-modrinth/src/Services/MinecraftModrinthService.php
[failure] 224-224:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
[failure] 194-194:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
🪛 GitHub Check: PHPStan (8.4)
minecraft-modrinth/src/Services/MinecraftModrinthService.php
[failure] 224-224:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
[failure] 194-194:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
🪛 GitHub Check: PHPStan (8.5)
minecraft-modrinth/src/Services/MinecraftModrinthService.php
[failure] 224-224:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
[failure] 194-194:
Strict comparison using === between Illuminate\Http\Client\Response and false will always evaluate to false.
🔇 Additional comments (7)
minecraft-modrinth/plugin.json (1)
5-6: Version/description update matches the new feature set.minecraft-modrinth/lang/en/strings.php (1)
38-63: Strings align cleanly with the new install/update/uninstall flow.minecraft-modrinth/lang/de/strings.php (1)
38-63: German translations are consistent with the new action set.minecraft-modrinth/src/Services/MinecraftModrinthService.php (2)
105-154: Metadata file path + validation flow looks solid.
236-277: Helper accessors + update check are clear and well-scoped.minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (1)
36-148: Helper methods for metadata caching and filename validation look good.minecraft-modrinth/README.md (1)
3-41: Docs clearly reflect the new install/update/uninstall lifecycle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
|
I also thought about adding a filter to only show installed mods/plugins, but I'm not sure whether I should just add the “Installed only” filter or also add “Updates available.” |
Added filename validation to prevent path traversal when handling files from the Modrinth API. Updated file save logic to check for failed responses instead of boolean false, improving reliability and security.
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 (1)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (1)
20-20:⚠️ Potential issue | 🟡 MinorRemove unused import
Section.The pipeline failure indicates an unused import. The
Filament\Schemas\Components\Sectionimport is not used in this file.🧹 Proposed fix
use Filament\Schemas\Components\Grid; -use Filament\Schemas\Components\Section; use Filament\Schemas\Schema;
🤖 Fix all issues with AI agents
In `@minecraft-modrinth/src/Services/MinecraftModrinthService.php`:
- Around line 105-110: getMetadataFilePath currently calls
ModrinthProjectType::fromServer($server)->getFolder() without checking for null,
which can cause a fatal error when fromServer returns null; update
getMetadataFilePath in MinecraftModrinthService.php to handle the null case by
first assigning $type = ModrinthProjectType::fromServer($server), then if $type
is null either throw a clear exception (e.g., InvalidArgumentException with a
message referencing the Server) or return a sensible default path, otherwise
call $type->getFolder() to build and return the metadata path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.phpminecraft-modrinth/src/Services/MinecraftModrinthService.php
🧰 Additional context used
🧬 Code graph analysis (2)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (2)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (2)
fromServer(29-45)getFolder(21-27)minecraft-modrinth/src/Enums/MinecraftLoader.php (1)
fromServer(23-30)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (2)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (5)
getInstalledModsMetadata(113-154)getInstalledMod(237-248)getModrinthVersions(74-103)saveModMetadata(156-204)removeModMetadata(206-234)minecraft-modrinth/src/Enums/ModrinthProjectType.php (2)
fromServer(29-45)getFolder(21-27)
🪛 GitHub Actions: Lint
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
[error] 1-1: Pint test failed due to no_unused_imports issue. Unused imports detected in MinecraftModrinthProjectPage.php. Step: vendor/bin/pint --test. Process exited with code 1.
🔇 Additional comments (9)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (4)
112-154: Well-structured metadata validation.The validation approach using
array_flipandarray_diff_keyis efficient. Good defensive coding with structure validation and safe fallback on exceptions.
156-204: Metadata save logic is correct.The approach of removing existing entries before adding the new one ensures clean updates. The
$response->failed()check correctly handles the Response object now.
250-263: LGTM!Version comparison by
version_idis reliable since Modrinth returns versions newest-first.
265-278: LGTM!Clean backward-compatible helper that extracts and normalizes filenames.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (5)
79-147: Well-designed caching and validation helpers.The request-scoped caching via properties avoids redundant API/file reads. The
validateFilenamemethod properly guards against path traversal by checking for separators and usingbasename().
185-263: Install action is well-implemented.Good defensive coding: validates version structure, sanitizes filename before use, and invalidates cache on both success and failure paths. The download-before-metadata-save order is correct.
264-384: Update action handles file replacement safely.The download-first approach at line 334 prevents leaving the mod in a broken state if the download fails. Both old and new filenames are validated, and the old file is only deleted when filenames differ (line 338).
385-406: LGTM!The installed badge correctly shows for up-to-date mods and gracefully handles the case when version info is unavailable.
407-469: Uninstall action follows a safe ordering.The comment on lines 431-433 correctly explains why metadata is removed first: an orphaned file is safer than metadata pointing to a non-existent file. Filename validation prevents path traversal on the delete operation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Added validation to ensure the server supports Modrinth mods or plugins before performing operations. Exceptions are now thrown when the project type is not available, improving error handling and preventing undefined behavior.
Deleted the import of Filament\Schemas\Components\Section from MinecraftModrinthProjectPage.php as it was not used in the file.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (1)
18-21:⚠️ Potential issue | 🟡 MinorRemove unused
Sectionimport to satisfy lint.
The lint job reportsno_unused_imports, andFilament\Schemas\Components\Sectionisn’t referenced here.🧹 Remove unused import
-use Filament\Schemas\Components\Section;
🤖 Fix all issues with AI agents
In
`@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php`:
- Around line 187-270: The install action can leave a downloaded JAR on disk if
MinecraftModrinth::saveModMetadata(...) returns false; after calling
$fileRepository->setServer($server)->pull(...), ensure you delete the downloaded
file on metadata-save failure (use the same $fileRepository with $server and the
target folder from $type->getFolder() and the validated $safeFilename) before
throwing/returning an error so the filesystem and UI stay consistent; update the
closure that calls DaemonFileRepository->pull and
MinecraftModrinth::saveModMetadata to perform this rollback cleanup when save
fails and then surface the failure to trigger the existing catch/notification
path.
- Around line 271-395: The current update action deletes the old file after
downloading the new file but before saving metadata, which can leave metadata
pointing to a deleted file if MinecraftModrinth::saveModMetadata fails; change
the flow in the Action::make('update')->action(...) closure to: 1) download the
new file with $fileRepository->pull as now, 2) call
MinecraftModrinth::saveModMetadata(...) immediately to update metadata to the
new filename/version, 3) only after a successful save delete the old file via
Http::daemon(...)->post(...), and 4) if save fails, rollback by deleting the
newly downloaded file (use the same $folder and $safeNewFilename validated via
validateFilename) and rethrow/handle the exception; also ensure
installedModsMetadata is invalidated consistently after successful update or on
rollback failure.
In `@minecraft-modrinth/src/Services/MinecraftModrinthService.php`:
- Around line 257-270: The isUpdateAvailable method assumes
$availableVersions[0] is newest; instead sort $availableVersions by the version
published timestamp (e.g. the version's "published" or "date_published" field)
in descending order before picking the latest, then compare
$installedMod['version_id'] to the sorted first element's 'id'; implement the
sort inside isUpdateAvailable (e.g. with usort) so the latest version is
deterministically selected.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.phpminecraft-modrinth/src/Services/MinecraftModrinthService.php
🧰 Additional context used
🧬 Code graph analysis (1)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (2)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (2)
fromServer(29-45)getFolder(21-27)minecraft-modrinth/src/Enums/MinecraftLoader.php (1)
fromServer(23-30)
🪛 GitHub Actions: Lint
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
[error] 1-1: vendor/bin/pint --test: no_unused_imports violation in MinecraftModrinthProjectPage.php.
🔇 Additional comments (17)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (6)
105-116: Good guard for unsupported server types.
Clear exception prevents null dereferences and gives a helpful error.
119-160: Robust metadata validation.
Required-key filtering plus safe fallback keeps invalid metadata out of the UI.
163-210: Metadata write flow is clean.
Filtering existing entries before appending makes updates deterministic.
213-240: Clean removal path.
243-255: Straightforward lookup.
272-285: Back-compat filename list looks good.
Lowercasing helps keep comparisons stable.minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (11)
36-40: Caching fields look good.
56-64: Graceful fallback label.
81-94: Lazy-load metadata cache is solid.
96-108: Simple lookup is fine.
110-119: Per-project version cache is fine.
122-135: Primary-file selection is clear.
137-149: Good filename sanitization guard.
397-418: Installed-state action is clear.
419-485: Uninstall flow is consistent.
490-507: Header action guard is solid.
509-549: Installed-count display update looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php`:
- Around line 247-248: When clearing the installed mods metadata, also clear the
versions cache to avoid stale state: wherever you set
$this->installedModsMetadata = null (e.g. the occurrences at the shown
locations), also set $this->versionsCache = null in the same block; ensure every
success/failure cache reset that touches $this->installedModsMetadata includes
$this->versionsCache = null so both caches are reset together.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
🧰 Additional context used
🧬 Code graph analysis (1)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (4)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (3)
fromServer(29-45)getLabel(13-19)getFolder(21-27)minecraft-modrinth/src/Enums/MinecraftLoader.php (2)
fromServer(23-30)getLabel(18-21)minecraft-modrinth/src/Services/MinecraftModrinthService.php (4)
getInstalledModsMetadata(120-161)getInstalledMod(244-255)saveModMetadata(163-211)removeModMetadata(213-241)minecraft-modrinth/src/Facades/MinecraftModrinth.php (1)
MinecraftModrinth(19-25)
🔇 Additional comments (3)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (3)
28-148: Nice helper/cache layer with filename sanitization.
Clear separation of metadata lookup, version caching, and filename validation keeps the action handlers much cleaner and safer.
396-417: Installed badge logic looks solid.
The visibility rules map cleanly to “installed + latest” vs “installed but outdated.”
494-547: Server-type guard additions are a good safety net.
Returning empty header actions and “unknown” states when unsupported avoids null issues and confusing UI.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
Adds rollback logic to delete downloaded files if saving mod metadata fails during install or update, ensuring consistency. Moves old file deletion after successful metadata save during updates. Ensures UI cache is invalidated after install, update, or uninstall actions. Adds defensive sorting of Modrinth versions by publish date to guarantee latest version is first.
Introduces a new 'Installed' view for Modrinth projects, allowing users to filter and display only installed mods. Adds logic to fetch installed mods' data in bulk from the Modrinth API, gracefully handles unavailable mods by displaying a message, and updates language files for new UI labels. Also ensures author metadata is stored and displayed where available.
Updated type annotations and logic in both MinecraftModrinthProjectPage and MinecraftModrinthService to support an optional 'author' field in installed mod metadata arrays. This allows mod author information to be stored and retrieved when available.
| $folder = $type->getFolder(); | ||
|
|
||
| return [ | ||
| Action::make('view_all') |
There was a problem hiding this comment.
"All" and "Installed" should be tabs.
There was a problem hiding this comment.
Hey! I switched the actions to tabs like you suggested, but since built-in tabs only work on ListRecords pages, I had to build a custom TabsComponent instead. Does this approach work for you? See c713f81
Also here a Screenshot:
There was a problem hiding this comment.
I'm pretty sure filament has a HasTabs trait that you can add. Then you can use the normal getTabs function etc.
Switch German localization strings from formal "Sie" to informal "du" for uninstall and notification messages (uninstall_description, install_failed_body, update_failed_body, uninstall_failed_body) to adopt a friendlier tone. Also revert plugin.json version from 1.1.0 back to 1.0.0.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@minecraft-modrinth/lang/de/strings.php`:
- Around line 56-66: Missing translation key 'uninstall_success_body' causes
inconsistent behavior: add the key 'uninstall_success_body' to the German and
English strings arrays (mirroring the pattern of 'install_success_body' and
'update_success_body', e.g. a message using :name and :version placeholders) and
update the notification/rendering logic that currently falls back to
$record['title'] (where 'uninstall_success' is handled) so it reads the new
'uninstall_success_body' translation like it does for 'install_success_body' and
'update_success_body' (ensure you reference the same translation lookup
function/array used for 'install_success_body' and 'update_success_body').
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
minecraft-modrinth/lang/de/strings.phpminecraft-modrinth/plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- minecraft-modrinth/plugin.json
🔇 Additional comments (2)
minecraft-modrinth/lang/de/strings.php (2)
19-21: LGTM!New page-level keys are clear and consistent.
42-53: LGTM!Actions and modal strings use correct informal "du" form consistently, and placeholders (
:old_version,:new_version,:name) are properly included.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add 'uninstall_success_body' translation to English and German language files (minecraft-modrinth/lang/*). The new messages include a :name placeholder and provide a user-facing confirmation after a successful uninstall, keeping localization consistent with other status messages.
Add a Filament tabs UI and view loading provider, and refactor the Modrinth project page to use it. New files: components/tabs.blade.php, modrinth-project-page.blade.php, TabsComponent, and MinecraftModrinthPluginProvider to load plugin views. Refactor MinecraftModrinthProjectPage: replace activeView with a URL-bound activeTab, add normalizeTab/mount/setActiveTab/updatedActiveTab for validation and syncing, update logic to check activeTab, remove action buttons used for switching views, and inject TabsComponent into the page header so tabs drive the table state and URL.
Localize the uninstall success notification: replace the direct use of $record['title'] with a trans() call (minecraft-modrinth::strings.notifications.uninstall_success_body) passing the project title as the 'name' parameter. This enables proper localization and consistent formatting of the notification message.
minecraft-modrinth/src/Providers/MinecraftModrinthPluginProvider.php
Outdated
Show resolved
Hide resolved
| $folder = $type->getFolder(); | ||
|
|
||
| return [ | ||
| Action::make('view_all') |
There was a problem hiding this comment.
I'm pretty sure filament has a HasTabs trait that you can add. Then you can use the normal getTabs function etc.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
Migrate from a custom tabs implementation to Filament's built-in HasTabs. Removed custom Blade views (components/tabs.blade.php, modrinth-project-page.blade.php), TabsComponent class, and the MinecraftModrinthPluginProvider that loaded views. Updated MinecraftModrinthProjectPage to use HasTabs and Filament Tabs\Tab via getTabs(), removed the manual activeTab Url attribute/property and normalization logic, and replaced TabsComponent::make() with the framework's tabs content component. Also inlined the refresh logic after uninstall actions instead of using a separate helper. Simplifies tab handling and reduces custom view/provider surface.
Add a new "versions" record action that opens a version-selection modal using Filament Sections, listing per-version details (type, downloads, published date, changelog) and install/header actions. Improve install/uninstall flows with safer file deletion and rollback logic: wrap daemon delete calls in try/catch, attempt rollback deletes/restores and report failures, and refresh cached metadata on success/failure. Import Section component and add translation strings for "versions" in English and German. Also remove an obsolete comment in the MinecraftModrinthService.
Add a PHPDoc docblock to MinecraftModrinthProjectPage::getTabs() specifying the return type as array<string, Tab>. This improves static analysis and IDE autocompletion for the tabs structure.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (2)
524-524: Unused$recordparameter inmodalHeadingclosure.The
fn (array $record) =>closure captures$recordbut never uses it. Simplify to a plain string.♻️ Proposed simplification
- ->modalHeading(fn (array $record) => trans('minecraft-modrinth::strings.modals.update_heading')) + ->modalHeading(trans('minecraft-modrinth::strings.modals.update_heading'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php` at line 524, The modalHeading closure currently declares an unused parameter (fn (array $record) => trans('minecraft-modrinth::strings.modals.update_heading')); update the call to pass the heading string directly (e.g., modalHeading(trans('minecraft-modrinth::strings.modals.update_heading'))) by removing the unused $record parameter and the arrow function so the modalHeading receives the static string instead of an unused closure.
176-187: Search is silently ignored on the "Installed" tab.When
activeTab === 'installed', the$searchparameter is available but unused. Users who type into the search bar on the Installed tab will see no effect, with no indication that filtering is disabled. Consider either wiring$searchthrough togetInstalledModsFromModrinth(client-side filter on title/slug) or disabling/hiding the search input when the installed tab is active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php` around lines 176 - 187, The installed-tab branch ignores the $search input so typed queries have no effect; update the logic in the activeTab === 'installed' branch to apply filtering: obtain $installedMods via getInstalledModsMetadata(), then either pass $search into getInstalledModsFromModrinth (update that method signature to accept a search string and filter by title/slug) or filter $installedMods locally by matching $search against mod title/slug before calling getInstalledModsFromModrinth; ensure $projects and $totalCount reflect the filtered results so the LengthAwarePaginator receives the correct items and total_hits.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
minecraft-modrinth/lang/de/strings.phpminecraft-modrinth/lang/en/strings.phpminecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.phpminecraft-modrinth/src/Services/MinecraftModrinthService.php
🧰 Additional context used
🧬 Code graph analysis (2)
minecraft-modrinth/src/Services/MinecraftModrinthService.php (3)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (2)
fromServer(29-45)getFolder(21-27)minecraft-modrinth/src/Enums/MinecraftLoader.php (1)
fromServer(23-30)minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (3)
getInstalledModsMetadata(99-111)content(783-828)getInstalledMod(114-125)
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (3)
minecraft-modrinth/src/Enums/ModrinthProjectType.php (3)
fromServer(29-45)getLabel(13-19)getFolder(21-27)minecraft-modrinth/src/Services/MinecraftModrinthService.php (7)
getInstalledModsMetadata(218-257)getInstalledMod(345-356)getModrinthVersions(164-201)getInstalledModsFromModrinth(77-161)getModrinthProjects(26-71)saveModMetadata(259-312)removeModMetadata(314-342)minecraft-modrinth/src/Facades/MinecraftModrinth.php (1)
MinecraftModrinth(19-25)
🔇 Additional comments (7)
minecraft-modrinth/lang/de/strings.php (1)
19-68: LGTM — all new keys are consistent and complete.Placeholder variables (
:old_version,:new_version,:name,:version) match the PHP usage inMinecraftModrinthProjectPage.php. The previously flaggeduninstall_success_bodykey has been correctly added.minecraft-modrinth/lang/en/strings.php (1)
19-68: LGTM — keys, placeholders, and structure are consistent with the German locale.minecraft-modrinth/src/Services/MinecraftModrinthService.php (1)
259-342: LGTM —saveModMetadataandremoveModMetadataare well-structured.Both correctly use
$response->failed()(previously flagged issue ✅), handle exceptions with rollback-friendly returns, and maintain atomic read-modify-write on the metadata. TheJSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHESflags produce readable, clean metadata files.minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php (4)
176-187: LGTM — tab branching logic is correct.The
activeTabcheck correctly dispatches to the installed-mods paginator (usinggetInstalledModsFromModrinthfor Modrinth enrichment) vs. the standard search paginator.$totalCount = count($installedMods)accurately reflects the full installed set for correct pagination.
410-502: LGTM — install action with rollback is correct.The install flow correctly: validates filename, downloads, saves metadata, and on metadata failure attempts to roll back the downloaded file before throwing. Cache invalidation (
$installedModsMetadata = null,$versionsCache = []) is applied on both success and failure paths.
503-663: LGTM — update action rollback sequence is correct.The previously flagged metadata-before-delete ordering has been resolved: the new file is downloaded, metadata is saved to the new version, and only then is the old file deleted. Rollback logic (delete new file + restore old metadata) is applied if old-file deletion fails. Cache invalidation is symmetric across success and failure paths.
18-36:Filament\Resources\Concerns\HasTabsis not designed for standalonePageclasses and this setup carries runtime risks.The
HasTabstrait is built for resource record-listing pages (e.g.,ListRecords,ManageRecords). When applied to a standaloneFilament\Pages\Page, the trait's lifecycle methods (updatedActiveTab()callsresetPage()for pagination, and query modification viamodifyQueryWithActiveTab()) expect resource-specific patterns. This code uses HasTabs but bypasses these mechanisms: it does not callmodifyQueryWithActiveTab(), returns plainTabobjects instead of resource Tabs withmodifyQuery(), and relies on manualif ($this->activeTab === 'installed')branching.Risks:
- If
resetPage()is not available on thePagebase class or trait, a tab switch will error.- If the trait fails to initialize
$this->activeTabon standalone pages, it remainsnull, causing the condition at line 176 to always evaluate false—the "installed" tab will never actually filter the table, despite appearing selected.- The code pattern (manual pagination + trait-based tab initialization) is unconventional and may break silently across Filament versions.
Either refactor to use a dedicated tabs UI component (e.g., form tabs) without
HasTabs, or explicitly verify that your Filament version safely handles this non-standard usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php`:
- Around line 157-164: validateFilename currently only rejects slashes but still
allows ".." and NUL bytes which enable path traversal; update
validateFilename($filename) to reject any filename that equals '.' or '..',
contains the substring ".." as a path segment (e.g. via strpos($filename, '..')
!== false or a stricter regex detecting ".." between separators), and contains a
NUL byte ("\0"), throwing the same Exception for invalid input; continue
returning basename($filename) for valid names.
- Around line 696-758: Currently removeModMetadata(...) is called before
attempting the remote file deletion, which can lead to orphaned files if
ModrinthProjectType::fromServer(...) or Http::daemon(...)->post(...)->throw()
fails; change the sequence in the action closure so you first determine the
ModrinthProjectType (ModrinthProjectType::fromServer), validate the filename
(validateFilename/getInstalledMod), then call the daemon delete
(Http::daemon(...)->post(...)->throw()) and only after a successful delete call
MinecraftModrinth::removeModMetadata(...); keep the same error handling
(catch/report) and still clear $this->installedModsMetadata and
$this->versionsCache and refresh $this->js('$wire.$refresh()') as currently done
on both success and failure so the UI remains consistent and retryable.
In `@minecraft-modrinth/src/Services/MinecraftModrinthService.php`:
- Around line 96-118: The code assumes $modrinthProjects is iterable but
Http::get(...)->json() can return null; add a null-guard right after the
cache()->remember(...) call to ensure $modrinthProjects is an array before the
foreach (e.g., if (!is_array($modrinthProjects)) $modrinthProjects = [];), or
cast it to an array, so the foreach over $modrinthProjects and the creation of
$modrinthMap (used in the foreach loop) never throws a TypeError.
---
Nitpick comments:
In
`@minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php`:
- Line 524: The modalHeading closure currently declares an unused parameter (fn
(array $record) => trans('minecraft-modrinth::strings.modals.update_heading'));
update the call to pass the heading string directly (e.g.,
modalHeading(trans('minecraft-modrinth::strings.modals.update_heading'))) by
removing the unused $record parameter and the arrow function so the modalHeading
receives the static string instead of an unused closure.
- Around line 176-187: The installed-tab branch ignores the $search input so
typed queries have no effect; update the logic in the activeTab === 'installed'
branch to apply filtering: obtain $installedMods via getInstalledModsMetadata(),
then either pass $search into getInstalledModsFromModrinth (update that method
signature to accept a search string and filter by title/slug) or filter
$installedMods locally by matching $search against mod title/slug before calling
getInstalledModsFromModrinth; ensure $projects and $totalCount reflect the
filtered results so the LengthAwarePaginator receives the correct items and
total_hits.
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
minecraft-modrinth/src/Filament/Server/Pages/MinecraftModrinthProjectPage.php
Show resolved
Hide resolved
Strengthen filename validation to block '.' , '..' and null bytes to prevent path traversal. Add case-insensitive search filtering for the installed mods tab (matches project_title or project_slug). Simplify modal heading to use the translation string directly. Reorder metadata removal to occur after the file download request and add a failure check. Harden MinecraftModrinthService by defaulting non-array API responses to an empty array to avoid errors.
|
I'm slowly starting to like coderabbitai. 😄 |
2 participants
Overview
This PR adds comprehensive mod lifecycle management to the Minecraft Modrinth plugin. Currently, users can only install mods but have no way to update or remove them through the interface. This creates a frustrating user experience where they need to manually manage files outside the plugin.
Changes
Core Functionality
I've implemented update and uninstall capabilities for both mods and plugins. When users view their installed items, they'll now see action buttons that allow them to update to the latest version or remove items entirely. Both operations include confirmation modals to prevent accidental actions.
New Installed View
A new Installed tab has been added to the header, allowing users to switch between browsing available mods and viewing their currently installed items directly within the interface.
Metadata File
To make updates and uninstalls possible, I introduced a metadata tracking system using
.modrinth-metadata.json. This file stores essential information about each installed mod (version, filename, installation date), which lets us determine when updates are available and ensures clean uninstallation.Localization & Documentation
I've added the necessary translation strings for English and German to support the new UI elements, and updated the README to document the new features.
Screenshots
Implementation Notes
The metadata file approach was chosen over database storage to keep the plugin lightweight and make the data easily portable with the mod directory. The file is updated on each install/uninstall operation and read when checking for available updates.
Tested
Version
Bumped to
1.1.0to reflect the new feature set.Fixes Issues
Closes #62
Summary by CodeRabbit
New Features
Documentation
Localization