arma-reforger-workshop - Plugin for managing Arma Reforger mods. #20
Conversation
|
Warning Rate limit exceeded@szpolny has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Arma Reforger Workshop Filament plugin: manifest, README, English translations, a Facade, a service to interact with Bohemia Workshop and edit server config.json, and two Filament server pages for browsing and managing workshop mods. Changes
Sequence DiagramssequenceDiagram
participant Admin as Admin User
participant Page as ArmaReforgerWorkshopPage
participant Service as ArmaReforgerWorkshopService
participant FileRepo as Daemon File Repository
participant Workshop as Bohemia Workshop
rect rgb(240,248,255)
Note over Admin,Workshop: Add Mod Flow
Admin->>Page: Submit Add Mod (modId, name, version)
Page->>Service: addMod(server, FileRepo, modId, name, version)
Service->>FileRepo: read config.json
FileRepo-->>Service: return config
Service->>Service: merge/add mod entry (idempotent)
Service->>FileRepo: write updated config.json
FileRepo-->>Service: write result
Service-->>Page: success / failure
Page->>Admin: show notification
end
sequenceDiagram
participant Admin as Admin User
participant Browse as BrowseWorkshopPage
participant Service as ArmaReforgerWorkshopService
participant Workshop as Bohemia Workshop
participant FileRepo as Daemon File Repository
rect rgb(250,240,255)
Note over Admin,Workshop: Browse & Install Flow
Admin->>Browse: Search or paginate
Browse->>Service: browseWorkshop(search, page)
Service->>Workshop: fetch search page (cached ~15m)
Workshop-->>Service: HTML with embedded JSON
Service->>Service: parse & build paginator
Service-->>Browse: paginator (mods)
Browse->>Admin: display mods
Admin->>Browse: Confirm Install
Browse->>Service: addMod(server, FileRepo, modId, name, version)
Service->>FileRepo: read/update/write config.json
FileRepo-->>Service: result
Service-->>Browse: success / failure
Browse->>Admin: show notification / update row
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
🧹 Nitpick comments (6)
arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (2)
169-177: Consider using a generic error message instead of exposing exception details.Displaying
$exception->getMessage()directly to users could expose internal implementation details or sensitive information. Consider using a generic error message and logging the full exception for debugging.🔎 Proposed fix
} catch (Exception $exception) { report($exception); Notification::make() ->title('Failed to add mod') - ->body($exception->getMessage()) + ->body('Could not update the server configuration. Please try again.') ->danger() ->send(); }
156-168: Use localization strings instead of hardcoded text.The localization file defines notification strings (
notifications.mod_added,notifications.mod_added_body, etc.) that are not being used here. This duplicates text and makes translations harder to maintain.🔎 Proposed fix
if ($success) { Notification::make() - ->title('Mod added') - ->body("'{$record['name']}' has been added to your server configuration.") + ->title(__('arma-reforger-workshop::arma-reforger-workshop.notifications.mod_added')) + ->body(__('arma-reforger-workshop::arma-reforger-workshop.notifications.mod_added_body', ['name' => $record['name']])) ->success() ->send(); } else { Notification::make() - ->title('Failed to add mod') - ->body('Could not update the server configuration.') + ->title(__('arma-reforger-workshop::arma-reforger-workshop.notifications.failed_to_add')) + ->body(__('arma-reforger-workshop::arma-reforger-workshop.notifications.config_update_failed')) ->danger() ->send(); }arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
76-90: Unused$searchparameter.The
$searchparameter is declared but never used. Consider either implementing client-side filtering on the installed mods or removing the parameter if Filament allows.Additionally,
getModDetails()is called for each installed mod. While results are cached for 6 hours, the initial load with many mods could be slow due to sequential HTTP requests.🔎 Proposed fix to use the search parameter
$mods = ArmaReforgerWorkshop::getInstalledMods($server, $fileRepository); +// Filter by search if provided +if (!empty($search)) { + $mods = collect($mods)->filter(function ($mod) use ($search) { + return str_contains(strtolower($mod['name']), strtolower($search)) + || str_contains(strtolower($mod['modId']), strtolower($search)); + })->values()->toArray(); +} + // Enrich with workshop details $enrichedMods = collect($mods)->map(function ($mod) {arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (3)
92-97: Case-sensitive comparison inconsistency.The duplicate check here uses case-sensitive comparison (
=== $modId), butisModInstalled()(line 318) uses case-insensitive comparison viastrtoupper(). While the UI normalizes to uppercase before callingaddMod, this inconsistency could cause issues if the service is called directly with mixed-case IDs.Consider normalizing the modId to uppercase within this method for consistency:
🔎 Proposed fix
+ $modId = strtoupper($modId); + // Check if mod already exists foreach ($config['game']['mods'] as $existingMod) { - if (($existingMod['modId'] ?? '') === $modId) { + if (strtoupper($existingMod['modId'] ?? '') === $modId) { return true; // Already installed } }
138-141: Same case-sensitivity inconsistency in removeMod.The filter uses case-sensitive comparison. For consistency with
isModInstalled(), consider using case-insensitive comparison:🔎 Proposed fix
+ $modId = strtoupper($modId); + $config['game']['mods'] = collect($config['game']['mods']) - ->filter(fn ($mod) => ($mod['modId'] ?? '') !== $modId) + ->filter(fn ($mod) => strtoupper($mod['modId'] ?? '') !== $modId) ->values() ->toArray();
170-214: Error results may be cached for 6 hours.If an exception occurs or the HTTP request fails inside the
cache()->remember()callback, the minimal fallback result['modId' => $modId]will be cached for 6 hours, preventing retry until the cache expires.Consider caching only successful results:
🔎 Proposed fix using flexible() or manual caching
public function getModDetails(string $modId): array { - return cache()->remember("arma_reforger_mod:$modId", now()->addHours(6), function () use ($modId) { + $cacheKey = "arma_reforger_mod:$modId"; + $cached = cache()->get($cacheKey); + if ($cached !== null) { + return $cached; + } + + $result = (function () use ($modId) { try { $response = Http::timeout(10) ->connectTimeout(5) ->get(self::WORKSHOP_URL . '/' . $modId); if (!$response->successful()) { return ['modId' => $modId]; } // ... rest of parsing logic ... return array_filter($details, fn ($v) => $v !== null); } catch (Exception $exception) { report($exception); - return ['modId' => $modId]; + return null; // Don't cache errors } - }); + })(); + + if ($result !== null && count($result) > 1) { + cache()->put($cacheKey, $result, now()->addHours(6)); + return $result; + } + + return ['modId' => $modId]; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
arma-reforger-workshop/README.md(1 hunks)arma-reforger-workshop/lang/en/arma-reforger-workshop.php(1 hunks)arma-reforger-workshop/plugin.json(1 hunks)arma-reforger-workshop/src/ArmaReforgerWorkshopPlugin.php(1 hunks)arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php(1 hunks)arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php(1 hunks)arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php(1 hunks)arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (3)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (9)
canAccess(38-48)getNavigationLabel(50-53)getModelLabel(55-58)getPluralModelLabel(60-63)getTitle(65-68)table(73-173)getHeaderActions(175-253)ArmaReforgerWorkshopPage(27-285)content(255-284)arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (5)
isArmaReforgerServer(14-22)browseWorkshop(222-285)getModWorkshopUrl(158-161)isModInstalled(313-324)addMod(73-122)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (8)
ArmaReforgerWorkshopService(10-325)isArmaReforgerServer(14-22)getInstalledMods(29-55)getModDetails(168-215)getModWorkshopUrl(158-161)removeMod(127-153)addMod(73-122)getConfigPath(60-68)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (1)
ArmaReforgerWorkshopService(10-325)
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (3)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
content(255-284)arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (1)
content(209-219)
🪛 markdownlint-cli2 (0.18.1)
arma-reforger-workshop/README.md
61-61: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
76-76: Avoid unused parameters such as '$search'. (undefined)
(UnusedFormalParameter)
arma-reforger-workshop/src/ArmaReforgerWorkshopPlugin.php
22-22: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (19)
arma-reforger-workshop/plugin.json (1)
1-17: LGTM!The plugin manifest is well-structured with all required metadata fields properly defined.
arma-reforger-workshop/src/ArmaReforgerWorkshopPlugin.php (1)
15-22: LGTM!The plugin registration correctly uses Filament's page discovery mechanism. The empty
boot()method with unused$panelparameter is required by thePlugininterface contract, so the static analysis warning is a false positive.arma-reforger-workshop/README.md (1)
1-61: LGTM!The README provides clear and comprehensive documentation covering features, requirements, configuration, and caching behavior. The bold styling for the author section is a minor style choice flagged by markdownlint, but it's acceptable for this context.
arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (3)
35-45: LGTM!The access control correctly validates both tenant existence and server type before granting access.
70-83: LGTM!The table configuration correctly integrates with the workshop service, using deferred loading and matching pagination to the service's per-page count.
195-219: LGTM!Header actions and content structure are properly implemented with clear navigation between pages and an accessible link to the external workshop.
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
23-29: Facade implementation is correct with comprehensive PHPDoc annotations.The @method annotations (lines 11-19) accurately document all service methods with proper type hints and return types. The getFacadeAccessor returns the service class name, which Laravel will auto-resolve from the container.
Verify that the
ArmaReforgerWorkshopServicebinding is registered in the application's service provider (typically in a service provider's boot method) to ensure proper container resolution at runtime.arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (5)
38-48: LGTM!The access control logic properly validates the tenant exists before checking if it's an Arma Reforger server, and combines with parent access checks.
103-128: LGTM!Good column configuration with appropriate placeholders, icons, and toggleable visibility.
130-172: LGTM!The remove action has proper confirmation, error handling, and user feedback via notifications.
175-253: LGTM!Good validation for mod ID (16-character hex), proper error handling, and consistent uppercase normalization before storing.
255-284: LGTM!Good defensive error handling in the content display, gracefully falling back to 'Unknown' on failure.
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (7)
14-22: LGTM!Clean implementation with proper null coalescing for missing features/tags.
29-55: LGTM!Robust implementation with proper error handling and filtering of invalid entries.
60-68: LGTM!Clean implementation with sensible default fallback.
158-161: LGTM!Simple and correct URL construction.
222-285: LGTM!Good implementation with appropriate timeouts and 15-minute cache TTL. The same caching-of-failures concern applies here but is less impactful due to the shorter cache duration.
290-308: LGTM!Good defensive implementation with proper fallbacks for missing thumbnail data.
313-324: LGTM!Correct case-insensitive comparison for mod ID matching.
arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php
Outdated
Show resolved
Hide resolved
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
Please fix pint and what rabbit mentioned.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (2)
76-76: Remove unused parameter.The
$searchparameter is declared but never used in the records closure. Consider removing it or implementing search functionality.
271-276: Duplicate call togetInstalledMods().The
getInstalledMods()method is called both in thetable()method (line 83) and here incontent(). On a single page load, this reads and parses the same config file twice.Consider caching the result in a property or using Livewire computed properties to avoid redundant file operations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arma-reforger-workshop/lang/en/arma-reforger-workshop.phparma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.phparma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.phparma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.phparma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php
🚧 Files skipped from review as they are similar to previous changes (3)
- arma-reforger-workshop/lang/en/arma-reforger-workshop.php
- arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php
- arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php
🧰 Additional context used
🧬 Code graph analysis (2)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (3)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (8)
ArmaReforgerWorkshopService(10-330)isArmaReforgerServer(14-22)getInstalledMods(29-56)getModDetails(171-219)getModWorkshopUrl(161-164)removeMod(129-156)addMod(74-124)getConfigPath(61-69)arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (5)
canAccess(37-47)table(90-199)getHeaderActions(201-213)BrowseWorkshopPage(24-226)content(215-225)
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (3)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
content(256-287)arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php (1)
content(215-225)
🪛 PHPMD (2.15.0)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
76-76: Avoid unused parameters such as '$search'. (undefined)
(UnusedFormalParameter)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
Outdated
Show resolved
Hide resolved
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
362-362: Dirname edge case still present.As noted in previous reviews, for a config path like
config.jsonat the root,dirname()returns'.'(current directory), not an empty string. The?: '/'fallback won't trigger as expected.🔎 Suggested fix
-->url(fn () => ListFiles::getUrl(['path' => dirname(ArmaReforgerWorkshop::getConfigPath($server)) ?: '/']), true), +->url(fn () => ListFiles::getUrl(['path' => ($dir = dirname(ArmaReforgerWorkshop::getConfigPath($server))) === '.' ? '/' : $dir]), true),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.phparma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.phparma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- arma-reforger-workshop/src/Filament/Server/Pages/BrowseWorkshopPage.php
🧰 Additional context used
🧬 Code graph analysis (1)
arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (2)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
content(366-397)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
224-231: Reconsider caching strategy for failed responses.Failed responses (which return only
['modId' => $modId]) are still cached with the same 6-hour TTL as successful responses. Transient failures like temporary workshop downtime will remain cached, preventing retries and leaving users with incomplete mod information.Consider implementing one of these approaches:
- Skip caching entirely for minimal/failed responses
- Use a significantly shorter TTL (e.g., 2-5 minutes) for failed responses
- Add a failure marker to distinguish cached failures from cached successes
🔎 Example approach: differentiate TTL based on response richness
+ // Determine TTL based on whether we got rich details or just modId + $isMinimalResponse = count($details) === 1 && isset($details['modId']); + $ttl = $isMinimalResponse ? now()->addMinutes(5) : $cacheTtl; + foreach ($uncachedMods as $index => $mod) { $response = $responses[(string) $index] ?? null; $details = $this->parseModDetailsFromResponse($response, $mod['modId']); // Cache the result - cache()->put($cachePrefix . $mod['modId'], $details, $cacheTtl); + $responseTtl = (count($details) === 1 && isset($details['modId'])) + ? now()->addMinutes(5) + : $cacheTtl; + cache()->put($cachePrefix . $mod['modId'], $details, $responseTtl); $cachedDetails[$index] = $details; }
🧹 Nitpick comments (1)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (1)
190-191: Consider extracting configuration values to class constants.The cache TTL (6 hours), HTTP timeouts (10s/5s), and cache prefix could be extracted to class constants for easier configuration and maintenance. This is a minor improvement that would enhance code clarity.
🔎 Example refactoring
+ protected const CACHE_PREFIX = 'arma_reforger_mod:'; + protected const CACHE_TTL_HOURS = 6; + protected const HTTP_TIMEOUT_SECONDS = 10; + protected const HTTP_CONNECT_TIMEOUT_SECONDS = 5; + protected function enrichModsWithDetails(array $mods): array { - $cachePrefix = 'arma_reforger_mod:'; - $cacheTtl = now()->addHours(6); + $cachePrefix = self::CACHE_PREFIX; + $cacheTtl = now()->addHours(self::CACHE_TTL_HOURS); // ... $responses = Http::pool(function ($pool) use ($uncachedMods, $workshopUrl) { $requests = []; foreach ($uncachedMods as $index => $mod) { $requests[$index] = $pool->as((string) $index) - ->timeout(10) - ->connectTimeout(5) + ->timeout(self::HTTP_TIMEOUT_SECONDS) + ->connectTimeout(self::HTTP_CONNECT_TIMEOUT_SECONDS) ->get($workshopUrl . '/' . $mod['modId']); }Also applies to: 216-217
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php
🧰 Additional context used
🧬 Code graph analysis (1)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (2)
arma-reforger-workshop/src/Facades/ArmaReforgerWorkshop.php (1)
ArmaReforgerWorkshop(23-29)arma-reforger-workshop/src/Services/ArmaReforgerWorkshopService.php (7)
ArmaReforgerWorkshopService(10-336)isArmaReforgerServer(14-22)getInstalledMods(29-56)getModWorkshopUrl(167-170)removeMod(132-162)addMod(74-127)getConfigPath(61-69)
🔇 Additional comments (3)
arma-reforger-workshop/src/Filament/Server/Pages/ArmaReforgerWorkshopPage.php (3)
74-180: LGTM!The table configuration is well-structured with proper pagination, search filtering, and enriched mod details. The concurrent fetching pattern in
enrichModsWithDetails()effectively addresses the N+1 HTTP call concern from previous reviews. The remove action includes appropriate confirmation and error handling.
362-362: LGTM - dirname edge case properly handled.The use of
rtrim(dirname(...), '.') ?: '/'correctly addresses the edge case wheredirname()returns'.'for root-level config files. This fix ensures the path resolves to'/'as expected.
292-350: Well-implemented add mod action.The form validation is comprehensive with appropriate constraints for the mod ID (16-character hex validation), and error handling provides clear user feedback. The modId normalization to uppercase is consistent with the service layer expectations.
2 participants
Hi, i created plugin for managing mods for Arma Reforger. The plugin is mainly vibe-coded (because of my poor php knowledge), so free to suggest any improvements.
Summary by CodeRabbit
New Features
Documentation
Localization
✏️ Tip: You can customize this high-level summary in your review settings.