Add server-documentation plugin #83
Conversation
📝 WalkthroughWalkthroughAdds a new Server Documentation Pelican plugin: DB schema, Eloquent models/factories, Filament admin/server UI, services (document retrieval/versioning, Markdown conversion), policies/gates, migrations, translations, assets, tests, provider and plugin manifest/config. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ServerPage
participant DocumentService
participant Cache
participant Database
participant Policy
User->>ServerPage: open Documents page
ServerPage->>DocumentService: getDocumentsForServer(server, user)
alt cache hit
DocumentService->>Cache: fetch(key)
Cache-->>DocumentService: documents
else cache miss
DocumentService->>Database: query server-attached + global docs
Database-->>DocumentService: rows
DocumentService->>DocumentService: filter by getAllowedTypesForUser(user, server)
DocumentService->>Cache: store(key, documents)
end
DocumentService-->>ServerPage: documents
ServerPage->>User: render list
User->>ServerPage: select document
ServerPage->>Policy: authorize viewOnServer(user, document, server)
alt allowed
Policy-->>ServerPage: allowed
ServerPage->>User: show document content
else denied
Policy-->>ServerPage: denied
ServerPage->>User: deny / clear selection
end
sequenceDiagram
participant Admin
participant EditPage
participant DocumentModel
participant DocumentService
participant Database
participant Cache
participant AuditLog
Admin->>EditPage: submit edits
EditPage->>DocumentModel: save()
DocumentModel->>Database: persist changes
Database-->>DocumentModel: persisted
DocumentModel->>DocumentService: createVersionFromOriginal(...)
DocumentService->>Database: insert version record
Database-->>DocumentService: version saved
DocumentService->>Cache: clearDocumentCache(document)
DocumentService->>Cache: clearServerDocumentsCache(server)
DocumentService->>AuditLog: log version/change
DocumentService-->>EditPage: complete
EditPage->>Admin: show success notification
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)server-documentation/src/Filament/Admin/Resources/DocumentResource.php (4)
🪛 GitHub Actions: Lintserver-documentation/src/Filament/Admin/Resources/DocumentResource.php[error] 1-1: Pint style check failed: (unknown) DocumentResource.php 🪛 GitHub Check: PHPStanserver-documentation/src/Filament/Admin/Resources/DocumentResource.php[failure] 184-184: [failure] 149-149: 🪛 PHPMD (2.15.0)server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php21-21: Avoid unused parameters such as '$ownerRecord'. (undefined) (UnusedFormalParameter) 21-21: Avoid unused parameters such as '$pageClass'. (undefined) (UnusedFormalParameter) 🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php`:
- Around line 13-21: The foreign key for edited_by uses unsignedInteger which
may not match users.id type; replace the
unsignedInteger('edited_by')->nullable() plus separate
$table->foreign('edited_by')->references('id')->on('users')->nullOnDelete() with
a single
foreignId('edited_by')->nullable()->constrained('users')->nullOnDelete() (or
foreignId('edited_by')->nullable()->constrained()->nullOnDelete() if you rely on
the default users table) so the column type matches users.id and remove the
separate foreign(...) call.
In `@server-documentation/plugin.json`:
- Around line 10-13: Remove the top-level "meta" key from
server-documentation/plugin.json (which currently contains "status" and
"status_message"); delete the entire "meta" object so the file no longer
contains a "meta" property and ensure the resulting JSON remains valid (no
trailing commas or syntax errors) after removal.
In
`@server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php`:
- Around line 12-18: Remove the unused Filament imports causing the Pint lint
error: delete the imports for RichEditor, Select, Toggle, Section, and Schema
from the top of DocumentsRelationManager.php so only the actually used imports
(e.g., TextInput and RelationManager) remain; then save and re-run Pint to
confirm the no_unused_imports violation is resolved.
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php`:
- Around line 52-77: The exportAsMarkdown() method accesses $this->record which
PHPStan types as Model|int|string; cast it to the proper Document type to
silence errors by replacing unsafe uses with a typed retrieval (e.g. use
$document = $this->getRecord() with getRecord() declared/overridden to return
Document or add a local `@var` Document $document docblock above the assignment),
and/or add a class-level property to override the record type; update references
in exportAsMarkdown() (and any helpers like generateFilename, addFrontmatter) to
use the typed $document variable so property access (title, content, slug, type,
is_global, is_published, sort_order) is statically safe.
- Around line 36-37: The badge callback uses $this->record without a
PHPDoc/typed accessor which triggers PHPStan; add a typed accessor to the
EditDocument class (e.g. public function getRecord():
\Starter\ServerDocumentation\Models\Document) and update the badge callback to
call $this->getRecord() (and likewise any other places using $this->record in
this file), so DocumentResource::getUrl('versions', ['record' =>
$this->getRecord()]) and ->badge(fn () =>
$this->getRecord()->versions()->count() ?: null) use the strongly-typed method.
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php`:
- Around line 56-59: The table query uses $this->record->id without a static
type, so add a local type assertion for the record (e.g. /** `@var`
\App\Models\Document $this->record */) immediately before the return in the
table(Table $table): Table method so static analyzers know $this->record is a
Document; then keep the existing DocumentVersion::query()->where('document_id',
$this->record->id) call unchanged. This targets the table(...) method and the
$this->record->id access to resolve the typing warning.
- Around line 36-39: Add a typed accessor so PHPStan knows the record type: in
the ViewDocumentVersions class add a getRecord(): Document method (returning
$this->record) with a docblock of `@return`
\Starter\ServerDocumentation\Models\Document, and then replace direct
$this->record usages in getTitle() (and any other methods) with
$this->getRecord()->title to satisfy static analysis for the getTitle() method
and similar accessors.
In `@server-documentation/src/Models/Document.php`:
- Around line 86-93: The slug auto-generation in the Document::creating closure
uses Str::slug($document->title) which can violate the DB unique constraint for
duplicate titles; update the creating handler to generate a unique slug (e.g.,
attempt base = Str::slug($document->title) and, if it exists, append a numeric
suffix/increment until a unique slug is found) and set $document->slug to that
unique value, and also wrap the save path to handle potential duplicate key
exceptions gracefully (catch the DB unique constraint error and retry or return
a validation error) so Document::creating and $document->slug no longer cause
uniqueness violations.
In `@server-documentation/src/Services/DocumentService.php`:
- Around line 141-146: The isServerAdmin method currently uses global permission
checks which wrongly grant admin rights across all servers; update the
permission checks inside isServerAdmin (in DocumentService::isServerAdmin) to
use server-scoped checks by passing the $server as the second argument to
$user->can so the logic becomes owner OR $user->can('update server', $server) OR
$user->can('create server', $server).
In `@server-documentation/src/Services/MarkdownConverter.php`:
- Around line 84-94: The current entry in the $dangerous list in
MarkdownConverter.php uses a too-broad match for "data:" which can remove valid
text; narrow this rule so it only flags data: when used as a URL scheme (e.g.,
preceded by start-of-string, quotes, whitespace, or opening punctuation or a
tag/attribute context) or when followed by optional whitespace then a MIME/type
or base64 indicator, rather than any occurrence of the substring "data:"; update
the $dangerous array entry accordingly and add unit tests covering cases like
"metadata: value", inline text containing "data:" and legitimate data URLs so
only actual data: URLs are matched.
- Around line 345-359: The addFrontmatter method currently concatenates raw
values into YAML and must instead emit valid YAML: in addFrontmatter(string
$markdown, array $metadata) ensure scalar string values are YAML-escaped and
quoted (escape backslashes and double-quotes, replace literal newlines with "\n"
or switch to block style with "|" for multiline), boolean values remain
unquoted, and arrays are rendered as YAML sequences (each item on its own "-
item" line) rather than imploding with commas; implement escaping/quoting logic
for values and change the loop that builds $frontmatter so keys and their
properly formatted values produce valid YAML frontmatter.
🟡 Minor comments (10)
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php-1-42 (1)
1-42: Address Pint lint failures from pipeline.The pipeline reports Pint style violations:
class_definition,single_quote,braces_position. Run./vendor/bin/pintto auto-fix these formatting issues.server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php-19-22 (1)
19-22: PostgreSQL enum-to-varchar conversion requires explicit USING clause.When altering a PostgreSQL column from an enum type to VARCHAR, the explicit
USINGclause is mandatory—PostgreSQL does not provide an implicit cast between these types and the conversion will fail without it. Update to:-DB::statement("ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)"); +DB::statement("ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50) USING type::text");server-documentation/src/Models/Document.php-1-1 (1)
1-1: Address the lint failure:class_attributes_separationissue detected.The pipeline indicates a Pint lint failure for
class_attributes_separation. Run./vendor/bin/pintto auto-fix the class attribute spacing issues.server-documentation/src/Services/MarkdownConverter.php-1-1 (1)
1-1: Address the lint failure:phpdoc_alignissue detected.The pipeline indicates a Pint lint failure for
phpdoc_align. Run./vendor/bin/pintto auto-fix the PHPDoc alignment issues.server-documentation/tests/Unit/Policies/DocumentPolicyTest.php-35-42 (1)
35-42: Resetexplicit_permissionsconfig after the test.Mutating config without restoring it can leak into later tests in the suite.
✅ Suggested fix
- config(['server-documentation.explicit_permissions' => true]); + $original = config('server-documentation.explicit_permissions'); + config(['server-documentation.explicit_permissions' => true]); $user = User::factory()->create(); expect($user->can('viewList document'))->toBeFalse(); expect($user->can('create document'))->toBeFalse(); + + config(['server-documentation.explicit_permissions' => $original]);server-documentation/resources/views/filament/server/pages/documents.blade.php-48-55 (1)
48-55: Add Player color styling to keep icon cues consistent.The list icon colors cover Host/Server admin and mod, but not Player, so Player documents render without the success color cue.
🎨 Suggested tweak
<x-filament::icon :icon="$docType?->icon() ?? 'tabler-file-text'" `@class`([ 'h-4 w-4', 'text-danger-500' => $docType === DocumentType::HostAdmin, 'text-warning-500' => $docType === DocumentType::ServerAdmin, 'text-info-500' => $docType === DocumentType::ServerMod, + 'text-success-500' => $docType === DocumentType::Player, ]) />server-documentation/resources/views/filament/partials/permission-guide.blade.php-40-55 (1)
40-55: Localize the example copy for non‑English locales.The example section is hardcoded English while the rest of the view is translated. Move these strings into
server-documentation::stringsto keep i18n consistent.💡 Suggested update
- <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"> - <strong>{{ trans('server-documentation::strings.labels.all_servers') }} Toggle:</strong> - </p> + <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"> + <strong>{{ trans('server-documentation::strings.permission_guide.all_servers_toggle') }}</strong> + </p> <ul class="text-sm text-gray-600 dark:text-gray-300 space-y-1 list-disc list-inside"> - <li><strong>On</strong> → Document appears on every server</li> - <li><strong>Off</strong> → Must attach to specific servers</li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.toggle_on') }}</strong> → {{ trans('server-documentation::strings.permission_guide.toggle_on_desc') }}</li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.toggle_off') }}</strong> → {{ trans('server-documentation::strings.permission_guide.toggle_off_desc') }}</li> </ul> - <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"><strong>Examples:</strong></p> + <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"><strong>{{ trans('server-documentation::strings.permission_guide.examples_title') }}</strong></p> <ul class="text-sm text-gray-600 dark:text-gray-300 space-y-1 list-disc list-inside"> - <li><strong>Player + All Servers</strong> → Welcome guide everyone sees everywhere</li> - <li><strong>Player + Specific Server</strong> → Rules for one server only</li> - <li><strong>Server Admin + All Servers</strong> → Company-wide admin procedures</li> - <li><strong>Server Mod + Specific Server</strong> → Mod notes for one server</li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.example_player_global') }}</strong></li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.example_player_server') }}</strong></li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.example_admin_global') }}</strong></li> + <li><strong>{{ trans('server-documentation::strings.permission_guide.example_mod_server') }}</strong></li> </ul>server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php-86-86 (1)
86-86: Handle potentialfile_get_contentsfailure.
file_get_contents()can returnfalseon failure. While unlikely for a validated temporary upload, defensive handling prevents passingfalseto the converter.Proposed fix
- $content = file_get_contents($file->getRealPath()); + $content = file_get_contents($file->getRealPath()); + if ($content === false) { + Notification::make() + ->title(trans('server-documentation::strings.import.error')) + ->body(trans('server-documentation::strings.import.read_error')) + ->danger() + ->send(); + + return; + } $useFrontmatter = $data['use_frontmatter'] ?? true;server-documentation/src/Services/DocumentService.php-212-227 (1)
212-227: Rate-limited version updates may cause unexpected behavior.When a new edit occurs within 30 seconds of the last version, the existing version is updated rather than creating a new one. This means:
- The
originalTitle/originalContentbeing saved may overwrite content from the previous rapid edit- Users may be confused when their version history doesn't show intermediate changes
Consider documenting this behavior or updating with the latest pre-edit state rather than overwriting.
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php-7-14 (1)
7-14: Incorrect import namespaces causing lint failure.The imports for table actions use
Filament\Actions\*but should useFilament\Tables\Actions\*for table-context actions in relation managers.🔧 Proposed fix
-use Filament\Actions\AttachAction; -use Filament\Actions\DetachAction; -use Filament\Actions\DetachBulkAction; +use Filament\Tables\Actions\AttachAction; +use Filament\Tables\Actions\DetachAction; +use Filament\Tables\Actions\DetachBulkAction;
🧹 Nitpick comments (20)
server-documentation/composer.json (1)
2-2: Consider renaming the "starter" namespace for production use.The namespace
Starter\ServerDocumentationand package namestarter/server-documentationsuggest this is template/boilerplate code. For a production plugin, consider using a more distinctive vendor name (e.g.,pelican/server-documentationorgavin/server-documentation) to avoid potential conflicts with other plugins derived from the same starter template.Also applies to: 16-17
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
29-41: Down migration doesn't fully reverse PostgreSQL changes.The
down()method only restores the enum type for MySQL/MariaDB. For PostgreSQL, the column remains as VARCHAR. If full reversibility is important, consider adding PostgreSQL enum restoration or documenting this as intentional.server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (1)
9-14: Clarify the intent of the two-migration approach or consolidate with 000006.Migration 000005 adds a basic unique constraint on
slug, which migration 000006 immediately drops and replaces with driver-specific partial indexes (allowing slug reuse after soft delete). This is intentional incremental refinement—the first migration establishes basic uniqueness, the second refines it to handle soft deletes—but the design could be clearer with a comment explaining the progression or more maintainable if consolidated directly in 000006.server-documentation/src/Services/MarkdownConverter.php (1)
269-274: Pipe characters in table cells are not escaped.Cell content containing
|will break the Markdown table structure. Consider escaping pipes as\|.♻️ Suggested fix
foreach ($rows as $row) { // Pad row if needed while (count($row) < count($headers)) { $row[] = ''; } - $result .= '| ' . implode(' | ', $row) . " |\n"; + $escapedRow = array_map(fn ($cell) => str_replace('|', '\\|', $cell), $row); + $result .= '| ' . implode(' | ', $escapedRow) . " |\n"; }Apply similar escaping to the header row at line 266.
server-documentation/tests/Unit/Services/MarkdownConverterTest.php (1)
38-77: Consider adding test coverage for table and code block conversion.The
toMarkdownmethod supports tables and code blocks, but there are no tests covering these conversions. This would help catch regressions in the regex-based HTML-to-Markdown conversion.server-documentation/src/Models/Document.php (1)
95-131: Stored original values may persist if update fails.The
originalValuesForVersionarray is populated inupdatingbut only cleared inupdated. If the database update fails (e.g., validation, constraint violation), the values remain and could cause stale version data on the next successful update.Consider wrapping version creation in a try-catch or using a model event that only fires on successful commits, or clear the array in a
failedscenario.server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (1)
11-20: Consider adding an index onserver_idfor query performance.The composite unique constraint on
[document_id, server_id]will efficiently serve queries filtering bydocument_idfirst. However, queries that filter byserver_idalone (e.g., "find all documents for a server") won't benefit from this index due to column ordering.♻️ Optional: Add standalone index on server_id
$table->foreign('server_id')->references('id')->on('servers')->cascadeOnDelete(); $table->unique(['document_id', 'server_id']); +$table->index('server_id');server-documentation/README.md (2)
263-291: Add language specifier to fenced code block.Per static analysis (MD040), fenced code blocks should have a language specified for consistency and proper syntax highlighting. Since this is a directory tree, use
textorplaintext.♻️ Proposed fix
-``` +```text server-documentation/ ├── composer.json # PSR-4 autoloading (no external deps)
295-317: Add language specifier to fenced code block.Per static analysis (MD040), this database schema block should have a language specifier.
♻️ Proposed fix
-``` +```text documents ├── id, uuidserver-documentation/resources/views/filament/pages/document-versions.blade.php (1)
19-24: Consider using translation keys for fallback strings.The hardcoded fallback strings
'Never'and'Unknown'should use translation keys for i18n consistency with the rest of the template.♻️ Proposed fix
-<span class="ml-2">{{ $this->record->updated_at?->diffForHumans() ?? 'Never' }}</span> +<span class="ml-2">{{ $this->record->updated_at?->diffForHumans() ?? trans('server-documentation::strings.common.never') }}</span>-<span class="ml-2">{{ $this->record->lastEditor?->username ?? 'Unknown' }}</span> +<span class="ml-2">{{ $this->record->lastEditor?->username ?? trans('server-documentation::strings.common.unknown') }}</span>Ensure the translation keys are added to
lang/en/strings.php:'common' => [ 'never' => 'Never', 'unknown' => 'Unknown', ],server-documentation/tests/Unit/Services/DocumentServiceTest.php (1)
142-161: Clarify test setup to avoid confusion.The change summary
'Before change'is misleading since the version is created with the current document state ("New Title"), not before a change occurs. The test logic is correct, but the naming suggests the version captures a "before" state when it actually captures the current state at creation time.♻️ Suggested clarification
it('restores document to version state', function () { $document = Document::factory()->create([ - 'title' => 'New Title', - 'content' => '<p>New content</p>', + 'title' => 'Original Title', + 'content' => '<p>Original content</p>', ]); - $version = $this->service->createVersion($document, 'Before change'); + // Capture current state as version 1 + $version = $this->service->createVersion($document, 'Initial version'); + // Modify the document $document->update([ 'title' => 'Changed Title', 'content' => '<p>Changed content</p>', ]); + // Restore to version 1 $this->service->restoreVersion($document, $version); $document->refresh(); - expect($document->title)->toBe('New Title'); - expect($document->content)->toBe('<p>New content</p>'); + expect($document->title)->toBe('Original Title'); + expect($document->content)->toBe('<p>Original content</p>'); });server-documentation/tests/Unit/Models/DocumentTest.php (1)
188-195: Avoidsleep(1)in tests; use time travel instead.
sleepslows the suite and can still be flaky; Laravel’s Carbon time travel keeps tests deterministic while still exercising the debounce window.⏱️ Suggested refactor
- $document->update(['content' => '<p>v1</p>']); - sleep(1); // Ensure different timestamps for rate limiting - $document->update(['content' => '<p>v2</p>']); + $now = now(); + \Illuminate\Support\Carbon::setTestNow($now); + $document->update(['content' => '<p>v1</p>']); + \Illuminate\Support\Carbon::setTestNow($now->copy()->addSeconds(31)); + $document->update(['content' => '<p>v2</p>']); + \Illuminate\Support\Carbon::setTestNow();Add this import near the top of the file:
use Illuminate\Support\Carbon;server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php (1)
7-8: Remove unused import.
ActionGroupis imported but not used in this file.Proposed fix
use Filament\Actions\Action; -use Filament\Actions\ActionGroup; use Filament\Resources\Pages\CreateRecord;server-documentation/src/Policies/DocumentPolicy.php (1)
79-105: LGTM with optional refactor suggestion.The
viewOnServerlogic correctly implements the four-tier permission hierarchy. The server association check is duplicated on lines 84 and 97, but this is acceptable as it serves clarity for separate code paths (root admin vs regular user).If you want to reduce duplication, you could extract the check:
Optional refactor
+ private function isDocumentAccessibleOnServer(Document $document, Server $server): bool + { + return $document->is_global || $document->servers()->where('servers.id', $server->id)->exists(); + } + public function viewOnServer(User $user, Document $document, Server $server): bool { // Root admins can view all documents including drafts if ($user->isRootAdmin()) { - // Still check server association for non-global docs - if (!$document->is_global && !$document->servers()->where('servers.id', $server->id)->exists()) { + if (!$this->isDocumentAccessibleOnServer($document, $server)) { return false; } return true; } // Document must be published for non-root admins if (!$document->is_published) { return false; } - // Document must be linked to server or be global - if (!$document->is_global && !$document->servers()->where('servers.id', $server->id)->exists()) { + if (!$this->isDocumentAccessibleOnServer($document, $server)) { return false; }server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (1)
108-113: Consider adding a safety limit to slug uniqueness loop.The
whileloop could iterate many times if there are numerous documents with similar slugs. Adding a reasonable limit prevents edge-case performance issues.Proposed fix
// Ensure slug is unique $originalSlug = $slug; $counter = 1; - while (Document::where('slug', $slug)->exists()) { + $maxAttempts = 100; + while (Document::where('slug', $slug)->exists() && $counter <= $maxAttempts) { $slug = $originalSlug . '-' . $counter++; } + + if ($counter > $maxAttempts) { + Notification::make() + ->title(trans('server-documentation::strings.import.error')) + ->body('Could not generate unique slug') + ->danger() + ->send(); + + return; + }server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (1)
54-57: Missing label for sort_order input.The
sort_orderinput is missing a label, unlike the similar implementation inDocumentsRelationManagerwhich includes a label. This affects consistency and user experience.♻️ Proposed fix
TextInput::make('sort_order') + ->label(trans('server-documentation::strings.document.sort_order')) ->numeric() ->default(0) ->helperText(trans('server-documentation::strings.relation_managers.sort_order_helper')),server-documentation/src/Filament/Admin/Resources/DocumentResource.php (1)
70-89: Consider simplifying translation fallback pattern.The pattern of checking if
trans()returns the key itself before using it is verbose and repeated. Consider using Laravel'sLang::has()or the__()helper with a fallback parameter for cleaner code.♻️ Alternative pattern
public static function getModelLabel(): string { return trans()->has('server-documentation::strings.document.singular') ? trans('server-documentation::strings.document.singular') : 'Document'; }Or define fallbacks in the translation file itself and simply return
trans('server-documentation::strings.document.singular').server-documentation/src/Filament/Server/Pages/Documents.php (1)
47-55: Auto-selected document bypasses authorization check.In
mount(), the first document is auto-selected directly fromgetDocuments()without going through theselectDocument()method's authorization check. WhilegetDocuments()already filters by allowed types, explicit authorization viaselectDocument()would be more consistent.♻️ Proposed fix
public function mount(): void { $documents = $this->getDocuments(); // Auto-select first document if available if ($documents->isNotEmpty() && !$this->selectedDocument) { - $this->selectedDocument = $documents->first(); + $this->selectDocument($documents->first()->id); } }server-documentation/src/Services/DocumentService.php (2)
92-93: Use strict comparison inin_array.The
in_array($doc->id, $attachedIds)call should use strict mode to prevent type coercion issues.♻️ Proposed fix
- $globalDocs = $globalDocs->filter(fn ($doc) => !in_array($doc->id, $attachedIds)); + $globalDocs = $globalDocs->filter(fn ($doc) => !in_array($doc->id, $attachedIds, true));
343-348: Hardcoded cache driver list may be incomplete.The
cacheSupportsTaggingmethod hardcodes known tagging-capable drivers. Consider using a try-catch approach or checking for theTaggableStoreinterface for more robust detection.♻️ Alternative approach
protected function cacheSupportsTagging(): bool { try { Cache::tags(['test']); return true; } catch (\BadMethodCallException) { return false; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
server-documentation/docs/images/admin-create-document.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-documents-list.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-edit-document.pngis excluded by!**/*.pngserver-documentation/docs/images/player-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-admin-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-mod-view.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-preview.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restore.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restored.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history.pngis excluded by!**/*.png
📒 Files selected for processing (42)
server-documentation/LICENSEserver-documentation/README.mdserver-documentation/composer.jsonserver-documentation/config/server-documentation.phpserver-documentation/database/factories/DocumentFactory.phpserver-documentation/database/factories/DocumentVersionFactory.phpserver-documentation/database/migrations/2024_01_01_000001_create_documents_table.phpserver-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.phpserver-documentation/database/migrations/2024_01_01_000003_create_document_server_table.phpserver-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.phpserver-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.phpserver-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.phpserver-documentation/lang/en/strings.phpserver-documentation/plugin.jsonserver-documentation/resources/css/document-content.cssserver-documentation/resources/views/filament/pages/document-versions.blade.phpserver-documentation/resources/views/filament/pages/version-preview.blade.phpserver-documentation/resources/views/filament/partials/permission-guide.blade.phpserver-documentation/resources/views/filament/server/pages/documents.blade.phpserver-documentation/src/Enums/DocumentType.phpserver-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.phpserver-documentation/src/Filament/Concerns/HasDocumentTableColumns.phpserver-documentation/src/Filament/Server/Pages/Documents.phpserver-documentation/src/Models/Document.phpserver-documentation/src/Models/DocumentVersion.phpserver-documentation/src/Policies/DocumentPolicy.phpserver-documentation/src/Providers/ServerDocumentationServiceProvider.phpserver-documentation/src/ServerDocumentationPlugin.phpserver-documentation/src/Services/DocumentService.phpserver-documentation/src/Services/MarkdownConverter.phpserver-documentation/tests/Pest.phpserver-documentation/tests/Unit/Enums/DocumentTypeTest.phpserver-documentation/tests/Unit/Models/DocumentTest.phpserver-documentation/tests/Unit/Policies/DocumentPolicyTest.phpserver-documentation/tests/Unit/Services/DocumentServiceTest.phpserver-documentation/tests/Unit/Services/MarkdownConverterTest.php
🧰 Additional context used
🧬 Code graph analysis (14)
server-documentation/tests/Unit/Enums/DocumentTypeTest.php (1)
server-documentation/src/Enums/DocumentType.php (12)
hierarchyLevel(82-90)color(56-64)icon(69-77)tryFromLegacy(125-133)isValid(138-141)typesVisibleToLevel(105-120)isVisibleToLevel(95-98)options(148-156)simpleOptions(163-171)formatLabel(176-181)formatColor(186-191)formatIcon(196-201)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php (4)
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (1)
DocumentResource(41-251)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (3)
getHeaderActions(21-47)getFormActions(79-82)getRedirectUrl(84-87)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (1)
getHeaderActions(25-62)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
getHeaderActions(46-54)
server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (5)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (2)
up(9-29)down(31-34)server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (2)
up(9-24)down(26-29)server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (2)
up(9-21)down(23-26)server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (2)
up(9-27)down(29-41)server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (2)
up(10-47)down(49-81)
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (2)
server-documentation/src/Enums/DocumentType.php (1)
icon(69-77)server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php (6)
getDocumentTitleColumn(20-27)getDocumentTypeColumn(29-36)getDocumentGlobalColumn(38-45)getDocumentPublishedColumn(47-52)getDocumentUpdatedAtColumn(54-61)getDocumentTypeFilter(63-68)
server-documentation/src/Models/DocumentVersion.php (2)
server-documentation/database/factories/DocumentVersionFactory.php (1)
DocumentVersionFactory(14-47)server-documentation/src/Models/Document.php (3)
newFactory(27-30)casts(75-82)Document(21-312)
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (2)
up(9-29)down(31-34)
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (1)
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
table(35-83)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (5)
server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (2)
up(9-24)down(26-29)server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (2)
up(9-21)down(23-26)server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (2)
up(9-27)down(29-41)server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (2)
up(9-14)down(16-21)server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (2)
up(10-47)down(49-81)
server-documentation/tests/Unit/Services/MarkdownConverterTest.php (1)
server-documentation/src/Services/MarkdownConverter.php (7)
MarkdownConverter(12-396)toHtml(59-69)toMarkdown(40-52)sanitizeHtml(75-101)addFrontmatter(345-359)parseFrontmatter(367-395)generateFilename(320-325)
server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php (1)
server-documentation/src/Enums/DocumentType.php (6)
description(43-51)formatLabel(176-181)color(56-64)formatColor(186-191)options(148-156)simpleOptions(163-171)
server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (2)
server-documentation/src/Models/Document.php (2)
Document(21-312)servers(161-167)server-documentation/src/Policies/DocumentPolicy.php (1)
create(34-37)
server-documentation/tests/Unit/Services/DocumentServiceTest.php (5)
server-documentation/src/Models/Document.php (4)
Document(21-312)createVersion(177-180)restoreVersion(182-185)versions(169-173)server-documentation/src/Services/DocumentService.php (8)
DocumentService(17-432)getAllowedTypesForUser(103-112)getUserHierarchyLevel(117-136)isServerAdmin(141-146)createVersion(249-265)restoreVersion(270-299)getDocumentCount(366-375)pruneVersions(388-413)server-documentation/src/Policies/DocumentPolicy.php (2)
create(34-37)update(42-45)server-documentation/src/Enums/DocumentType.php (1)
hierarchyLevel(82-90)server-documentation/src/Models/DocumentVersion.php (1)
document(44-47)
server-documentation/tests/Unit/Models/DocumentTest.php (4)
server-documentation/src/Models/Document.php (10)
Document(21-312)author(151-154)versions(169-173)servers(161-167)lastEditor(156-159)getDocumentType(272-275)isHostAdminOnly(277-280)isPlayerVisible(292-295)getRequiredTier(300-303)isVisibleToTier(308-311)server-documentation/src/Models/DocumentVersion.php (2)
document(44-47)editor(49-52)server-documentation/src/Policies/DocumentPolicy.php (2)
create(34-37)update(42-45)server-documentation/database/factories/DocumentFactory.php (4)
hostAdmin(38-41)serverAdmin(46-49)serverMod(54-57)player(62-65)
server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (2)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (1)
up(9-29)server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (1)
up(9-24)
🪛 GitHub Actions: Lint
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php
[error] 1-1: Pint lint failed: no_unused_imports detected.
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php
[error] 1-1: Pint lint failed: class_definition, single_quote, braces_position style issues detected.
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
[error] 1-1: Pint lint failed: no_unused_imports detected.
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[error] 1-1: Pint lint failed: linting issue detected (unspecified rule).
server-documentation/src/Services/MarkdownConverter.php
[error] 1-1: Pint lint failed: phpdoc_align detected.
server-documentation/src/Models/Document.php
[error] 1-1: Pint lint failed: class_attributes_separation issue detected.
server-documentation/src/Services/DocumentService.php
[error] 1-1: Pint lint failed: phpdoc_separation detected.
🪛 GitHub Actions: Validate plugin.json files
server-documentation/plugin.json
[error] 1-1: Validation failed: 'server-documentation/plugin.json' contains a 'meta' key. Please remove it. (validatePluginJson.php exit code 1)
🪛 GitHub Check: PHPStan
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php
[failure] 70-70:
Cannot access property $slug on Illuminate\Database\Eloquent\Model|int|string.
[failure] 67-67:
Cannot access property $sort_order on Illuminate\Database\Eloquent\Model|int|string.
[failure] 66-66:
Cannot access property $is_published on Illuminate\Database\Eloquent\Model|int|string.
[failure] 65-65:
Cannot access property $is_global on Illuminate\Database\Eloquent\Model|int|string.
[failure] 64-64:
Cannot access property $type on Illuminate\Database\Eloquent\Model|int|string.
[failure] 63-63:
Cannot access property $slug on Illuminate\Database\Eloquent\Model|int|string.
[failure] 62-62:
Cannot access property $title on Illuminate\Database\Eloquent\Model|int|string.
[failure] 58-58:
Cannot access property $content on Illuminate\Database\Eloquent\Model|int|string.
[failure] 37-37:
Cannot call method versions() on Illuminate\Database\Eloquent\Model|int|string.
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[failure] 154-154:
Access to an undefined property Starter\ServerDocumentation\Models\Document::$is_global.
🪛 markdownlint-cli2 (0.18.1)
server-documentation/README.md
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
295-295: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
server-documentation/src/Providers/ServerDocumentationServiceProvider.php
33-33: Avoid unused parameters such as '$app'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
22-22: Avoid unused parameters such as '$ownerRecord'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$pageClass'. (undefined)
(UnusedFormalParameter)
server-documentation/src/ServerDocumentationPlugin.php
39-39: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Policies/DocumentPolicy.php
26-26: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
42-42: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
58-58: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (45)
server-documentation/LICENSE (1)
1-21: LGTM!Standard MIT License, appropriate for an open-source Pelican plugin.
server-documentation/resources/css/document-content.css (1)
1-150: LGTM!Well-structured CSS with consistent styling for document content. Good use of CSS variables for theming integration with Filament, clear typography hierarchy, and proper handling of code blocks and tables.
server-documentation/lang/en/strings.php (1)
1-134: LGTM!Well-organized translation file with logical groupings and comprehensive coverage of the document management UI. The placeholder syntax (
:version,:title,:time) is correctly used for dynamic values.server-documentation/database/factories/DocumentFactory.php (1)
1-98: LGTM!The factory is well-structured with clear state methods for each document type and visibility combination. The default definition covers all necessary attributes with reasonable randomization.
server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php (1)
1-81: LGTM!The trait provides well-organized, reusable table column and filter definitions. Good use of the
DocumentTypeenum methods for consistent badge styling and proper i18n support withtrans()calls.server-documentation/tests/Unit/Services/MarkdownConverterTest.php (2)
1-176: Overall test structure looks good.The tests are well-organized with clear describe/it blocks and cover the primary functionality of the MarkdownConverter service.
22-28: Test may be fragile depending on sanitization implementation.With the default
html_input: 'escape'configuration, CommonMark escapes HTML tags to entities (e.g.,<script>becomes<script>). The regex-based sanitization fallback (line 85) looks for literal<script>tags and won't match escaped entities, leaving the word "alert" as plain text in the output. The test assertion checkingnot->toContain('alert')could fail depending on whether Laravel'sStr::sanitizeHtml()is available. Consider testing the actual output directly rather than relying on assumptions about tag removal.server-documentation/src/Models/Document.php (1)
149-312: Relationships, scopes, and type helpers are well-implemented.The model provides a comprehensive API with proper query scopes for the permission system and clean delegation to
DocumentServicefor version management.server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (1)
1-35: LGTM!The migration is well-structured with appropriate column types, foreign key constraints with
nullOnDeletebehavior, and soft deletes support. The initial enum type fortypecolumn is correctly designed to be evolved by the subsequent migration (000004) that converts it to a varchar for the expanded 4-tier permission system.server-documentation/resources/views/filament/pages/version-preview.blade.php (1)
1-30: LGTM!The template correctly handles XSS prevention by using
sanitizeHtml()before rendering content with{!! !!}. Null-safe access for optional relationships and proper escaping for user-provided text (change_summary,title) are correctly implemented.server-documentation/tests/Unit/Services/DocumentServiceTest.php (4)
1-17: LGTM!Good test setup using Pest with
RefreshDatabaseand proper service resolution from the container.
18-61: LGTM!Comprehensive coverage of
getAllowedTypesForUsercovering all permission tiers: root admin, null user, server owner, and regular user.
63-107: LGTM!Good coverage of hierarchy level determination and server admin checks.
163-232: LGTM!The remaining tests for
restoreVersion,getDocumentCount, andpruneVersionsare well-structured with clear assertions and good edge case coverage (empty table, below limit scenarios).server-documentation/resources/views/filament/partials/permission-guide.blade.php (1)
5-38: Permission table rendering looks solid.Enum-driven labels/descriptions keep the guide aligned with role metadata.
server-documentation/resources/views/filament/server/pages/documents.blade.php (3)
7-11: Stylesheet injection is clean and scoped.Using
@once+@push('styles')keeps the asset load tidy.
13-26: Empty state UX looks good.Clear iconography and guidance for first-time users.
70-108: Content rendering is safely handled.Sanitized HTML output in the content pane is the right call for XSS defense.
server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (1)
26-29: Down migration is clean.Drop operation is straightforward and safe.
server-documentation/tests/Unit/Models/DocumentTest.php (4)
14-58: Creation coverage looks thorough.UUID, slug behavior, and author assignment cases are well covered.
60-109: Update/versioning tests are solid.Clear validation of version creation and editor attribution.
111-162: Scope coverage is comprehensive.Good breadth across type, published, global, and server-scoped filters.
201-240: Type helper assertions are clear.These checks closely mirror the enum contract.
server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (1)
45-115:viewOnServercoverage is strong.Role, scope, publish state, and association scenarios are well exercised.
server-documentation/tests/Unit/Enums/DocumentTypeTest.php (1)
7-106: Enum test suite is comprehensive.Covers hierarchy, legacy mapping, visibility, and formatting behavior thoroughly.
server-documentation/config/server-documentation.php (1)
13-71: Config defaults are clear and well documented.Looks solid as-is.
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
35-83: Relation manager wiring looks solid.Reordering, filters, and actions are consistent and well-scoped.
server-documentation/src/Providers/ServerDocumentationServiceProvider.php (1)
20-124: Service provider wiring is cohesive.Registration, bootstrapping, and gate setup are consistent.
server-documentation/src/Models/DocumentVersion.php (1)
13-57: DocumentVersion model looks good.Casts, relationships, and accessor are straightforward.
server-documentation/src/ServerDocumentationPlugin.php (1)
12-41: Plugin registration logic is clean and focused.Admin vs. server panel scoping is clear.
server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (1)
10-80: Indexing and slug-uniqueness handling look correct.Driver-specific paths are well separated.
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (1)
79-87: LGTM!The
getFormActions()andgetRedirectUrl()methods follow the established pattern seen in other page classes in this resource.server-documentation/tests/Pest.php (2)
45-58: LGTM with note on host application dependencies.The factory helpers are well-typed and follow Pest conventions. Note that
\App\Models\Userand\App\Models\Serverdepend on the host Pelican Panel application models, which is the expected integration pattern for this plugin.
30-32: LGTM!The custom expectation correctly validates against the four document type values defined in the
DocumentTypeenum.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php (1)
13-39: LGTM!The implementation follows the established pattern from
EditDocument.phpwith empty form actions and consistent redirect behavior. Disabling "Create Another" is a reasonable UX choice for document creation.server-documentation/src/Policies/DocumentPolicy.php (1)
26-69: LGTM - PHPMD warnings are false positives.The
$documentparameter warnings from PHPMD are false positives. Laravel policy methods require the model parameter in the signature even when not used, as the framework inspects the signature to resolve authorization.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (1)
144-178: LGTM!The
extractTitleFromMarkdownregex correctly matches H1 headings. ThenormalizeDocumentTypemethod properly validates types, handles legacy values, and logs warnings for invalid inputs with sensible fallback toplayertype.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
85-116: LGTM!The table actions are well-implemented. The restore action properly uses a
@varannotation for type safety, shows confirmation dialog, callsrestoreVersion(), and provides user feedback via notification.server-documentation/database/factories/DocumentVersionFactory.php (1)
1-47: LGTM!The factory is well-structured with proper type hints, fluent state helpers, and follows Laravel factory conventions. The relationship wiring via
Document::factory()and the state methods for customization are appropriately implemented.server-documentation/src/Enums/DocumentType.php (1)
1-202: LGTM!Well-designed enum with comprehensive helper methods for hierarchy-based permissions. The legacy compatibility handling via
LEGACY_ADMINconstant andtryFromLegacy()is thoughtfully implemented. The visibility methods correctly implement the permission hierarchy, and the formatters provide sensible defaults for unknown values.server-documentation/src/Filament/Admin/Resources/DocumentResource.php (2)
99-193: Form schema is well-structured.The form layout with collapsible sections, conditional visibility for server assignment, dynamic egg filtering, and the embedded permission guide provides a good UX. The slug auto-generation from title and unique validation are properly handled.
154-154: Theis_globalproperty is properly defined on the Document model.The property exists in the
$fillablearray (line 50), is cast tobooleanin the$castsarray (line 78), and has a corresponding database column defined in migrations. The PHPStan error is a false positive, likely due to PHPStan not recognizing Laravel's attribute casting mechanism. The code at line 154 is valid and requires no changes.Likely an incorrect or invalid review comment.
server-documentation/src/Services/DocumentService.php (2)
52-67: Service implementation is comprehensive and well-designed.The service properly handles:
- Cache management with tagging fallback
- Permission-based document filtering with hierarchy levels
- Transactional versioning with row locking to prevent race conditions
- Audit logging with configurable channel
- Version pruning with configurable retention
Good use of dependency injection via config and clean separation of concerns.
Also applies to: 74-96, 103-136, 196-243, 267-299, 366-375, 385-413
356-360: Thexxh3hash algorithm is available by default in PHP 8.2 as part of the bundled Hash extension (available since PHP 8.1). Since the project already requires PHP ^8.2, the algorithm is guaranteed to be available without additional dependencies or fallbacks.Likely an incorrect or invalid review comment.
server-documentation/src/Filament/Server/Pages/Documents.php (1)
65-69: Authorization check syntax is correct.The
cannot()call with[$document, $server]matches theDocumentPolicy::viewOnServer(User $user, Document $document, Server $server)signature correctly. In Laravel's authorization pattern, the user is implicit and the array elements map to the subsequent method parameters.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| $table->foreignId('document_id')->constrained()->cascadeOnDelete(); | ||
| $table->string('title'); | ||
| $table->longText('content'); | ||
| $table->unsignedInteger('version_number'); | ||
| $table->unsignedInteger('edited_by')->nullable(); | ||
| $table->string('change_summary')->nullable(); | ||
| $table->timestamps(); | ||
|
|
||
| $table->foreign('edited_by')->references('id')->on('users')->nullOnDelete(); |
There was a problem hiding this comment.
Fix potential FK type mismatch for edited_by.
Using unsignedInteger can conflict with the users.id type (commonly unsignedBigInteger), causing FK creation errors. Prefer foreignId() to match types.
🛠️ Proposed fix
- $table->unsignedInteger('edited_by')->nullable();
+ $table->foreignId('edited_by')->nullable()->constrained('users')->nullOnDelete();
$table->string('change_summary')->nullable();
$table->timestamps();
- $table->foreign('edited_by')->references('id')->on('users')->nullOnDelete();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $table->foreignId('document_id')->constrained()->cascadeOnDelete(); | |
| $table->string('title'); | |
| $table->longText('content'); | |
| $table->unsignedInteger('version_number'); | |
| $table->unsignedInteger('edited_by')->nullable(); | |
| $table->string('change_summary')->nullable(); | |
| $table->timestamps(); | |
| $table->foreign('edited_by')->references('id')->on('users')->nullOnDelete(); | |
| $table->foreignId('document_id')->constrained()->cascadeOnDelete(); | |
| $table->string('title'); | |
| $table->longText('content'); | |
| $table->unsignedInteger('version_number'); | |
| $table->foreignId('edited_by')->nullable()->constrained('users')->nullOnDelete(); | |
| $table->string('change_summary')->nullable(); | |
| $table->timestamps(); |
🤖 Prompt for AI Agents
In
`@server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php`
around lines 13 - 21, The foreign key for edited_by uses unsignedInteger which
may not match users.id type; replace the
unsignedInteger('edited_by')->nullable() plus separate
$table->foreign('edited_by')->references('id')->on('users')->nullOnDelete() with
a single
foreignId('edited_by')->nullable()->constrained('users')->nullOnDelete() (or
foreignId('edited_by')->nullable()->constrained()->nullOnDelete() if you rely on
the default users table) so the column type matches users.id and remove the
separate foreign(...) call.
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php
Outdated
Show resolved
Hide resolved
| ->url(fn () => DocumentResource::getUrl('versions', ['record' => $this->record])) | ||
| ->badge(fn () => $this->record->versions()->count() ?: null), |
There was a problem hiding this comment.
Type assertion needed for $this->record in badge callback.
The same PHPStan issue applies here. Consider extracting a typed accessor method for consistency.
Proposed fix
Action::make('versions')
->label(trans('server-documentation::strings.versions.title'))
->icon('tabler-history')
->iconButton()
->iconSize(IconSize::ExtraLarge)
->url(fn () => DocumentResource::getUrl('versions', ['record' => $this->record]))
- ->badge(fn () => $this->record->versions()->count() ?: null),
+ ->badge(fn () => $this->getRecord()->versions()->count() ?: null),Then add a typed method to the class:
/** `@return` \Starter\ServerDocumentation\Models\Document */
public function getRecord(): Document
{
return $this->record;
}🧰 Tools
🪛 GitHub Check: PHPStan
[failure] 37-37:
Cannot call method versions() on Illuminate\Database\Eloquent\Model|int|string.
🤖 Prompt for AI Agents
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php`
around lines 36 - 37, The badge callback uses $this->record without a
PHPDoc/typed accessor which triggers PHPStan; add a typed accessor to the
EditDocument class (e.g. public function getRecord():
\Starter\ServerDocumentation\Models\Document) and update the badge callback to
call $this->getRecord() (and likewise any other places using $this->record in
this file), so DocumentResource::getUrl('versions', ['record' =>
$this->getRecord()]) and ->badge(fn () =>
$this->getRecord()->versions()->count() ?: null) use the strongly-typed method.
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php
Show resolved
Hide resolved
| public function table(Table $table): Table | ||
| { | ||
| return $table | ||
| ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->record->id)) |
There was a problem hiding this comment.
Same type assertion needed for table query.
The $this->record->id access on line 59 has the same typing issue.
Proposed fix
public function table(Table $table): Table
{
return $table
- ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->record->id))
+ ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->getRecord()->id))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function table(Table $table): Table | |
| { | |
| return $table | |
| ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->record->id)) | |
| public function table(Table $table): Table | |
| { | |
| return $table | |
| ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->getRecord()->id)) |
🤖 Prompt for AI Agents
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php`
around lines 56 - 59, The table query uses $this->record->id without a static
type, so add a local type assertion for the record (e.g. /** `@var`
\App\Models\Document $this->record */) immediately before the return in the
table(Table $table): Table method so static analyzers know $this->record is a
Document; then keep the existing DocumentVersion::query()->where('document_id',
$this->record->id) call unchanged. This targets the table(...) method and the
$this->record->id access to resolve the typing warning.
| public function isServerAdmin(User $user, Server $server): bool | ||
| { | ||
| return $server->owner_id === $user->id || | ||
| $user->can('update server') || | ||
| $user->can('create server'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how server permissions are typically handled in the codebase
rg -n "can\('update server" --type=php -C2
rg -n "can\('create server" --type=php -C2Repository: pelican-dev/plugins
Length of output: 3607
isServerAdmin should use server-scoped permissions.
The method uses global can('update server') and can('create server') checks instead of server-scoped permissions. In other parts of the codebase (e.g., tickets/src/Filament/Admin/Resources/Tickets/TicketResource.php), permissions are checked with server context: can('update server', $server). Without the server parameter, any user with global permission to update or create servers will be considered admin on all servers.
Update to:
return $server->owner_id === $user->id ||
$user->can('update server', $server) ||
$user->can('create server', $server);🤖 Prompt for AI Agents
In `@server-documentation/src/Services/DocumentService.php` around lines 141 -
146, The isServerAdmin method currently uses global permission checks which
wrongly grant admin rights across all servers; update the permission checks
inside isServerAdmin (in DocumentService::isServerAdmin) to use server-scoped
checks by passing the $server as the second argument to $user->can so the logic
becomes owner OR $user->can('update server', $server) OR $user->can('create
server', $server).
| public function addFrontmatter(string $markdown, array $metadata): string | ||
| { | ||
| $frontmatter = "---\n"; | ||
| foreach ($metadata as $key => $value) { | ||
| if (is_bool($value)) { | ||
| $value = $value ? 'true' : 'false'; | ||
| } elseif (is_array($value)) { | ||
| $value = implode(', ', $value); | ||
| } | ||
| $frontmatter .= "{$key}: {$value}\n"; | ||
| } | ||
| $frontmatter .= "---\n\n"; | ||
|
|
||
| return $frontmatter . $markdown; | ||
| } |
There was a problem hiding this comment.
Frontmatter values are not escaped for YAML special characters.
Values containing colons, quotes, or newlines will produce invalid YAML frontmatter. For example, a title like My Title: Part 1 would break parsing.
🔧 Suggested fix
public function addFrontmatter(string $markdown, array $metadata): string
{
$frontmatter = "---\n";
foreach ($metadata as $key => $value) {
if (is_bool($value)) {
$value = $value ? 'true' : 'false';
} elseif (is_array($value)) {
$value = implode(', ', $value);
+ } elseif (is_string($value) && (str_contains($value, ':') || str_contains($value, '"') || str_contains($value, "\n"))) {
+ $value = '"' . str_replace(['"', "\n"], ['\\"', '\\n'], $value) . '"';
}
$frontmatter .= "{$key}: {$value}\n";
}
$frontmatter .= "---\n\n";
return $frontmatter . $markdown;
}🤖 Prompt for AI Agents
In `@server-documentation/src/Services/MarkdownConverter.php` around lines 345 -
359, The addFrontmatter method currently concatenates raw values into YAML and
must instead emit valid YAML: in addFrontmatter(string $markdown, array
$metadata) ensure scalar string values are YAML-escaped and quoted (escape
backslashes and double-quotes, replace literal newlines with "\n" or switch to
block style with "|" for multiline), boolean values remain unquoted, and arrays
are rendered as YAML sequences (each item on its own "- item" line) rather than
imploding with commas; implement escaping/quoting logic for values and change
the loop that builds $frontmatter so keys and their properly formatted values
produce valid YAML frontmatter.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
`@server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php`:
- Around line 7-40: Update the migration to satisfy Pint: use single quotes for
all string literals (e.g., change DB::statement("...") and where(..., "player")
to single quotes) and adjust brace placement to match project style (move
opening braces for the anonymous class and the up() and down() methods to the
next line as required); key locations to edit are the "return new class extends
Migration" declaration and the up() and down() methods, plus all DB::statement
and DB::table/... string literals (including 'player', 'admin', 'server_admin',
'host_admin', 'server_mod').
In
`@server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php`:
- Around line 36-41: The SQLite branch in the migration currently adds a plain
unique('slug') which prevents slug reuse after soft deletes; instead, in the
migration's up() replace that Schema::table(...) block with a raw DB::statement
that creates a partial unique index on documents(slug) WHERE deleted_at IS NULL
(e.g. CREATE UNIQUE INDEX documents_slug_unique_not_deleted ON documents (slug)
WHERE deleted_at IS NULL) so SQLite 3.9+ enforces uniqueness only for
non-deleted rows; also update down() to drop that index with a raw DB::statement
(e.g. DROP INDEX IF EXISTS documents_slug_unique_not_deleted) to fully reverse
the migration.
In `@server-documentation/README.md`:
- Around line 261-317: The fenced code blocks under the "File Structure" and
"Database Schema" sections in README.md are missing language identifiers and
trigger markdownlint MD040; update both opening fences to include a language tag
(e.g., change the three backticks before the directory tree and before the
schema to ```text) so the blocks become fenced code with a language and keep the
closing triple-backtick unchanged; locate the blocks by the surrounding headings
"## File Structure" and "## Database Schema" to modify the fences accordingly.
In
`@server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php`:
- Around line 80-82: The empty state texts in DocumentsRelationManager are using
server-centric translation keys; update the calls to emptyStateHeading,
emptyStateDescription (and any related empty state labels) in the
DocumentsRelationManager class to use document-specific translation keys (e.g.,
relation_managers.no_documents_linked and
relation_managers.attach_documents_description) or else swap to existing generic
keys that accurately describe "no documents linked"; then add those new keys
with appropriate translations to the relation_managers section of your language
files so the messages align with the documents relationship.
In `@server-documentation/src/Filament/Admin/Resources/DocumentResource.php`:
- Line 153: The PHPStan error is due to missing `@property` annotations on the
Document Eloquent model for attributes accessed as magic properties (e.g.,
$record->is_global); edit the Document model class (class Document extends
Model) and add a PHPDoc block above the class with `@property` annotations for the
attributes used (at minimum `@property` bool $is_global, `@property` bool
$is_published, `@property` int $sort_order, etc.) so PHPStan recognizes these
magic attributes referenced by code such as the visible callback in
DocumentResource.
- Around line 13-14: The imports for EditAction and DeleteBulkAction are
incorrect: replace the current Filament\Actions imports with the table-specific
namespace Filament\Tables\Actions so the table-level actions used in the
DocumentResource (see use of EditAction and DeleteBulkAction in the table
actions block around lines 220-224) are resolved; update the use statements to
import Filament\Tables\Actions\EditAction and
Filament\Tables\Actions\DeleteBulkAction and remove the now-unused
Filament\Actions imports.
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php`:
- Around line 85-88: The call to file_get_contents($file->getRealPath()) can
return false and must be handled to avoid downstream type errors; update the
code in the ListDocuments page where $content is assigned so you first check
that the path is readable and that file_get_contents did not return false (use
is_readable($file->getRealPath()) or check the boolean result), and on failure
either skip this file (continue) or set $content to an empty string and log or
collect an error, then proceed to set $markdownContent/$metadata only when
$content is a string; reference the $file variable, $content, $useFrontmatter,
$metadata and $markdownContent in your fix.
In
`@server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php`:
- Line 12: Remove the unused import statement "use Filament\Schemas\Schema;"
from the top of the ServersRelationManager class file; locate the import in the
file containing the class ServersRelationManager and delete that single unused
use line to avoid the unused import warning.
- Around line 7-14: Update the incorrect imports for table actions: replace
imports of AttachAction, DetachAction, and DetachBulkAction from
Filament\Actions with the equivalents in Filament\Tables\Actions so the actions
used in the table() method are imported from Filament\Tables\Actions (refer to
AttachAction, DetachAction, DetachBulkAction in ServersRelationManager and
ensure the rest of the file still uses TextInput, RelationManager, Schema,
TextColumn, Table as before).
In `@server-documentation/src/ServerDocumentationPlugin.php`:
- Around line 39-41: The empty body of the Plugin::boot implementation (public
function boot(Panel $panel): void) is causing a Pint formatting violation;
update the ServerDocumentationPlugin::boot method to use a single-line empty
body or otherwise satisfy Pint (e.g., change to "public function boot(Panel
$panel): void {}" or add a concise comment statement) while keeping the unused
Panel $panel parameter to satisfy the interface contract; ensure the method
signature remains unchanged.
♻️ Duplicate comments (8)
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
12-18: Unused imports causing Pint lint failure.The imports
RichEditor,Select,Toggle,Section, andSchemaare not used in this file and are causing the pipelineno_unused_importsviolation.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (1)
16-18: Add a typedgetRecord()accessor to resolve PHPStan errors.The base
EditRecordclass types$this->recordasModel|int|string, causing PHPStan failures when accessing model properties/methods (lines 37, 57-67). Add a class-level type override.Proposed fix
class EditDocument extends EditRecord { protected static string $resource = DocumentResource::class; + + /** `@return` \Starter\ServerDocumentation\Models\Document */ + public function getRecord(): Document + { + return $this->record; + }Then update usages throughout the file to call
$this->getRecord()instead of$this->record.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (2)
36-39: Add type assertion for$this->recordaccess ingetTitle().Accessing
$this->record->titlewithout type assertion triggers PHPStan errors. Add a typed accessor as suggested in previous reviews.Proposed fix
Add a typed accessor to the class:
/** `@return` \Starter\ServerDocumentation\Models\Document */ public function getRecord(): Document { return $this->record; }Then update line 38:
- return trans('server-documentation::strings.versions.title') . ': ' . $this->record->title; + return trans('server-documentation::strings.versions.title') . ': ' . $this->getRecord()->title;
56-59: Same type assertion needed for table query.The
$this->record->idaccess on line 59 requires type assertion.Proposed fix
- ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->record->id)) + ->query(fn (): Builder => DocumentVersion::query()->where('document_id', $this->getRecord()->id))server-documentation/src/Services/MarkdownConverter.php (2)
69-79: Overly broaddata:URL pattern may cause false positives.The pattern
/data\s*:/ion line 78 will match legitimate content like "metadata: value" or any text containing "data:". This could strip valid content during sanitization.
292-306: Frontmatter values are not escaped for YAML special characters.Values containing colons, quotes, or newlines will produce invalid YAML frontmatter. For example, a title like
My Title: Part 1would break parsing.server-documentation/src/Models/Document.php (1)
86-92: Slug generation may cause uniqueness constraint violations.When
slugis null, it's auto-generated fromtitle. If two documents have the same title, this will cause a database uniqueness constraint violation.server-documentation/src/Services/DocumentService.php (1)
134-139:isServerAdminshould use server-scoped permissions.The method uses global
can('update server')andcan('create server')checks instead of server-scoped permissions. Without the server parameter, any user with global permission to update or create servers will be considered admin on all servers.
🧹 Nitpick comments (5)
server-documentation/resources/views/filament/pages/version-preview.blade.php (1)
2-22: Consider using translation keys for labels.These labels are hard-coded in English; elsewhere in the plugin you use
trans()keys. Swapping to translation strings keeps i18n consistent.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (1)
100-105: Slug uniqueness loop is correct but consider a reasonable limit.The while loop ensures unique slugs. In practice this works, but a defensive limit prevents theoretical infinite loops with malformed data.
Optional enhancement
$slug = $metadata['slug'] ?? Str::slug($title); $originalSlug = $slug; $counter = 1; - while (Document::where('slug', $slug)->exists()) { + while (Document::where('slug', $slug)->exists() && $counter < 1000) { $slug = $originalSlug . '-' . $counter++; }server-documentation/src/Services/MarkdownConverter.php (1)
167-184: Nested lists are flattened during conversion.The
convertListmethod usesstrip_tagson list items (line 174), which flattens any nested<ul>or<ol>elements. Nested list structures will lose their hierarchy.This is acceptable for basic documentation but worth noting as a limitation.
server-documentation/src/Models/Document.php (1)
229-241: Consider database-specific full-text search for performance.The
scopeSearchmethod usesLIKE "%{$term}%"queries ontitle,slug, andcontentcolumns. For large documentation sets, this will not use indexes and may be slow.For production use with significant document volumes, consider:
- Adding full-text indexes (MySQL/PostgreSQL)
- Using a search service like Meilisearch/Algolia
This is acceptable for initial implementation.
server-documentation/src/Services/DocumentService.php (1)
292-301: Potential N+1 query in cache clearing.
clearDocumentCacheiterates over$document->servers(line 294), which triggers a database query if servers aren't already loaded. If called in a loop or batch operation, this could cause N+1 queries.Consider eager-loading servers before calling this method in batch contexts, or accepting an optional pre-loaded servers collection.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
server-documentation/docs/images/admin-create-document.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-documents-list.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-edit-document.pngis excluded by!**/*.pngserver-documentation/docs/images/player-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-admin-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-mod-view.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-preview.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restore.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restored.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history.pngis excluded by!**/*.png
📒 Files selected for processing (42)
server-documentation/LICENSEserver-documentation/README.mdserver-documentation/composer.jsonserver-documentation/config/server-documentation.phpserver-documentation/database/factories/DocumentFactory.phpserver-documentation/database/factories/DocumentVersionFactory.phpserver-documentation/database/migrations/2024_01_01_000001_create_documents_table.phpserver-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.phpserver-documentation/database/migrations/2024_01_01_000003_create_document_server_table.phpserver-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.phpserver-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.phpserver-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.phpserver-documentation/lang/en/strings.phpserver-documentation/plugin.jsonserver-documentation/resources/css/document-content.cssserver-documentation/resources/views/filament/pages/document-versions.blade.phpserver-documentation/resources/views/filament/pages/version-preview.blade.phpserver-documentation/resources/views/filament/partials/permission-guide.blade.phpserver-documentation/resources/views/filament/server/pages/documents.blade.phpserver-documentation/src/Enums/DocumentType.phpserver-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.phpserver-documentation/src/Filament/Concerns/HasDocumentTableColumns.phpserver-documentation/src/Filament/Server/Pages/Documents.phpserver-documentation/src/Models/Document.phpserver-documentation/src/Models/DocumentVersion.phpserver-documentation/src/Policies/DocumentPolicy.phpserver-documentation/src/Providers/ServerDocumentationServiceProvider.phpserver-documentation/src/ServerDocumentationPlugin.phpserver-documentation/src/Services/DocumentService.phpserver-documentation/src/Services/MarkdownConverter.phpserver-documentation/tests/Pest.phpserver-documentation/tests/Unit/Enums/DocumentTypeTest.phpserver-documentation/tests/Unit/Models/DocumentTest.phpserver-documentation/tests/Unit/Policies/DocumentPolicyTest.phpserver-documentation/tests/Unit/Services/DocumentServiceTest.phpserver-documentation/tests/Unit/Services/MarkdownConverterTest.php
✅ Files skipped from review due to trivial changes (2)
- server-documentation/plugin.json
- server-documentation/LICENSE
🚧 Files skipped from review as they are similar to previous changes (17)
- server-documentation/composer.json
- server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php
- server-documentation/tests/Unit/Models/DocumentTest.php
- server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php
- server-documentation/resources/views/filament/pages/document-versions.blade.php
- server-documentation/src/Models/DocumentVersion.php
- server-documentation/database/factories/DocumentFactory.php
- server-documentation/database/factories/DocumentVersionFactory.php
- server-documentation/src/Filament/Server/Pages/Documents.php
- server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php
- server-documentation/tests/Unit/Services/MarkdownConverterTest.php
- server-documentation/resources/views/filament/partials/permission-guide.blade.php
- server-documentation/resources/css/document-content.css
- server-documentation/src/Enums/DocumentType.php
- server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php
- server-documentation/tests/Pest.php
- server-documentation/config/server-documentation.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T11:13:45.197Z
Learnt from: HarlequinSin
Repo: pelican-dev/plugins PR: 67
File: subdomains/src/Models/Subdomain.php:49-65
Timestamp: 2026-01-04T11:13:45.197Z
Learning: In Subdomain model creating hook at subdomains/src/Models/Subdomain.php, the code uses setRelation('server', Filament::getTenant()) because the model hasn't been saved yet and load() won't work during creation. This is intentional for Filament-context creation.
Applied to files:
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
🧬 Code graph analysis (13)
server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (3)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (2)
up(9-29)down(31-34)server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (2)
up(9-24)down(26-29)server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (2)
up(9-14)down(16-21)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php (3)
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (1)
DocumentResource(41-250)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (1)
getHeaderActions(21-47)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
getHeaderActions(46-54)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (7)
server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (2)
up(9-24)down(26-29)server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (2)
up(9-21)down(23-26)server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (2)
up(9-27)down(29-41)server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (2)
up(9-14)down(16-21)server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (2)
up(10-47)down(49-81)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
table(56-116)server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (1)
table(27-69)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (3)
server-documentation/src/Models/Document.php (1)
Document(21-296)server-documentation/src/Services/MarkdownConverter.php (3)
MarkdownConverter(12-343)parseFrontmatter(314-342)toHtml(48-57)server-documentation/src/Enums/DocumentType.php (4)
icon(69-77)color(56-64)isValid(136-139)tryFromLegacy(124-131)
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (2)
server-documentation/src/Enums/DocumentType.php (1)
icon(69-77)server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php (5)
getDocumentTitleColumn(20-27)getDocumentTypeColumn(29-36)getDocumentGlobalColumn(38-45)getDocumentPublishedColumn(47-52)getDocumentUpdatedAtColumn(54-61)
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (1)
up(9-29)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (2)
server-documentation/src/Models/Document.php (2)
Document(21-296)restoreVersion(170-173)server-documentation/src/Services/DocumentService.php (1)
restoreVersion(259-287)
server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (3)
server-documentation/src/Models/Document.php (2)
Document(21-296)servers(151-157)server-documentation/src/Policies/DocumentPolicy.php (1)
create(34-37)server-documentation/database/factories/DocumentFactory.php (8)
unpublished(78-81)global(86-89)serverAdmin(46-49)published(70-73)notGlobal(94-97)player(62-65)hostAdmin(38-41)serverMod(54-57)
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (5)
server-documentation/src/Models/Document.php (1)
Document(21-296)server-documentation/src/Services/DocumentService.php (2)
DocumentService(17-413)getDocumentCount(347-356)server-documentation/src/Enums/DocumentType.php (2)
options(146-154)description(43-51)server-documentation/src/Policies/DocumentPolicy.php (1)
view(26-29)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (1)
ListDocuments(21-164)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (4)
server-documentation/src/Services/MarkdownConverter.php (4)
MarkdownConverter(12-343)toMarkdown(34-41)addFrontmatter(292-306)generateFilename(267-272)server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
getHeaderActions(46-54)server-documentation/src/Models/Document.php (1)
versions(159-163)server-documentation/src/Models/DocumentVersion.php (1)
document(44-47)
server-documentation/src/Policies/DocumentPolicy.php (3)
server-documentation/src/Models/Document.php (2)
Document(21-296)servers(151-157)server-documentation/src/Services/DocumentService.php (2)
DocumentService(17-413)getAllowedTypesForUser(100-109)server-documentation/src/Models/DocumentVersion.php (1)
document(44-47)
server-documentation/tests/Unit/Services/DocumentServiceTest.php (4)
server-documentation/src/Models/Document.php (4)
Document(21-296)createVersion(165-168)restoreVersion(170-173)versions(159-163)server-documentation/src/Services/DocumentService.php (8)
DocumentService(17-413)getAllowedTypesForUser(100-109)getUserHierarchyLevel(114-129)isServerAdmin(134-139)createVersion(239-254)restoreVersion(259-287)getDocumentCount(347-356)pruneVersions(369-394)server-documentation/src/Enums/DocumentType.php (1)
hierarchyLevel(82-90)server-documentation/src/Models/DocumentVersion.php (1)
document(44-47)
server-documentation/src/Providers/ServerDocumentationServiceProvider.php (5)
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
DocumentsRelationManager(25-84)server-documentation/src/Models/Document.php (1)
Document(21-296)server-documentation/src/Policies/DocumentPolicy.php (1)
DocumentPolicy(12-101)server-documentation/src/Services/DocumentService.php (1)
DocumentService(17-413)server-documentation/src/Services/MarkdownConverter.php (1)
MarkdownConverter(12-343)
🪛 GitHub Actions: Lint
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php
[error] 1-1: Pint: no_unused_imports violation.
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php
[error] 1-1: Pint: class_definition, single_quote, braces_position style violations.
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[error] 1-1: Pint: no_unused_imports violation.
server-documentation/src/Services/MarkdownConverter.php
[error] 1-1: Pint: phpdoc_align violation.
server-documentation/src/Models/Document.php
[error] 1-1: Pint: class_attributes_separation violation.
server-documentation/src/ServerDocumentationPlugin.php
[error] 1-1: Pint: single_line_empty_lines violation.
server-documentation/src/Services/DocumentService.php
[error] 1-1: Pint: phpdoc_separation violation.
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
[error] 1-1: Pint: no_unused_imports violation.
🪛 GitHub Check: PHPStan
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[failure] 153-153:
Access to an undefined property Starter\ServerDocumentation\Models\Document::$is_global.
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php
[failure] 67-67:
Cannot access property $slug on Illuminate\Database\Eloquent\Model|int|string.
[failure] 64-64:
Cannot access property $sort_order on Illuminate\Database\Eloquent\Model|int|string.
[failure] 63-63:
Cannot access property $is_published on Illuminate\Database\Eloquent\Model|int|string.
[failure] 62-62:
Cannot access property $is_global on Illuminate\Database\Eloquent\Model|int|string.
[failure] 61-61:
Cannot access property $type on Illuminate\Database\Eloquent\Model|int|string.
[failure] 60-60:
Cannot access property $slug on Illuminate\Database\Eloquent\Model|int|string.
[failure] 59-59:
Cannot access property $title on Illuminate\Database\Eloquent\Model|int|string.
[failure] 57-57:
Cannot access property $content on Illuminate\Database\Eloquent\Model|int|string.
[failure] 37-37:
Cannot call method versions() on Illuminate\Database\Eloquent\Model|int|string.
🪛 markdownlint-cli2 (0.18.1)
server-documentation/README.md
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
295-295: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
server-documentation/src/Policies/DocumentPolicy.php
26-26: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
42-42: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
58-58: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
server-documentation/src/ServerDocumentationPlugin.php
39-39: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
22-22: Avoid unused parameters such as '$ownerRecord'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$pageClass'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Providers/ServerDocumentationServiceProvider.php
31-31: Avoid unused parameters such as '$app'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (40)
server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (3)
12-17: LGTM!The performance indexes are well-designed for common query patterns. Explicitly naming indexes ensures predictable behavior across different database drivers during rollback.
43-46: LGTM!Adding the unique constraint on
(document_id, version_number)is the right approach to enforce data integrity and prevent duplicate version numbers caused by race conditions. This complements the existing non-unique index from migration 000002.
49-80: Rollback logic is correct.The
down()method properly reverses all changes with driver-specific handling. The order deviation (performance indexes removed last instead of first) is acceptable since these operations are independent.server-documentation/tests/Unit/Enums/DocumentTypeTest.php (1)
7-106: Solid enum coverage. The test suite exercises hierarchy, visibility, legacy mapping, and option helpers thoroughly.server-documentation/tests/Unit/Services/DocumentServiceTest.php (1)
18-231: Good coverage of service behavior. The tests hit permission tiers, versioning, restore flows, and pruning paths.server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
13-26:andserver-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (1)
20-27: UseforeignIdfor consistency and type safety with user references.This plugin references the
userstable withunsignedIntegercolumns, while other tables within the same migration (e.g.,document_idindocument_versions) already use the modernforeignId()syntax. Additionally, if the core Pelican Panel application definesusers.idasunsignedBigInteger(Laravel's default), this creates a type mismatch that will cause foreign key constraint failures in MySQL/PostgreSQL. UseforeignId()to automatically match the referenced column type and simplify the code.Suggested fix
- $table->unsignedInteger('author_id')->nullable(); - $table->unsignedInteger('last_edited_by')->nullable(); + $table->foreignId('author_id')->nullable()->constrained('users')->nullOnDelete(); + $table->foreignId('last_edited_by')->nullable()->constrained('users')->nullOnDelete(); $table->integer('sort_order')->default(0); $table->timestamps(); $table->softDeletes(); - - $table->foreign('author_id')->references('id')->on('users')->nullOnDelete(); - $table->foreign('last_edited_by')->references('id')->on('users')->nullOnDelete();Also apply this pattern to
edited_byin2024_01_01_000002_create_document_versions_table.phpfor consistency.server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (1)
12-115: Well-structured policy test suite with good coverage.The tests effectively cover the key permission scenarios for the DocumentPolicy:
- Root admin permissions
- Server admin default access via
update serverpermissionexplicit_permissionsconfig toggle behaviorviewOnServerscenarios including unpublished documents, global vs server-attached documents, and role-based visibilityThe use of Pest's
describeblocks and factory state methods (e.g.,unpublished(),global(),serverAdmin()) makes the tests readable and maintainable.server-documentation/lang/en/strings.php (1)
3-134: Comprehensive and well-organized translation file.The translation structure covers all plugin UI components with clear, descriptive keys. The use of Laravel's
:placeholdersyntax for dynamic values (:version,:title,:time) follows conventions correctly.server-documentation/resources/views/filament/server/pages/documents.blade.php (2)
106-108: Proper XSS prevention withsanitizeHtml().Using
str($selectedDocument->content)->sanitizeHtml()before rendering with{!! !!}is the correct approach for displaying user-generated HTML content safely. This aligns with the PR's stated XSS prevention via html_input escaping.
35-65: Clean document list implementation with type-based visual cues.The sidebar correctly maps document types to appropriate icons and colors, with Player type intentionally left unstyled as the default tier. The
tryFromLegacyhandling ensures backward compatibility with legacy type values.server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
35-83: Well-structured relation manager implementation.The table configuration effectively leverages the shared
HasDocumentTableColumnstrait for consistent column definitions. The attach/create actions with proper pivot handling and the redirect to edit page viaDocumentResource::getUrl()follow Filament best practices.server-documentation/src/ServerDocumentationPlugin.php (1)
24-37: Panel registration logic is correct.The conditional registration of
DocumentResourcefor admin panel andDocumentspage for server panel correctly separates concerns.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php (1)
13-39: Clean CreateDocument page implementation.The page follows the established pattern from
EditDocument.phpby moving the primary action to the header as an icon button. Disabling "create another" ($canCreateAnother = false) and redirecting to the index after creation provides a streamlined document creation workflow.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php (2)
69-73: LGTM on streaming implementation.The
streamDownloadpattern with appropriate content-type header is correct for Markdown file exports.
76-84: LGTM on form action and redirect overrides.Returning an empty array for form actions (moving save to header) and redirecting to index after save are sensible UX choices.
server-documentation/src/Policies/DocumentPolicy.php (3)
26-29: Unused$documentparameters are intentional for Laravel policy signatures.PHPMD flags these as unused, but Laravel's policy method contracts require the model parameter even when not used. This is a false positive.
Also applies to: 42-45, 50-53, 58-61, 66-69
79-100: Well-structured 4-tier permission check inviewOnServer.The logic correctly handles:
- Root admin access with server association validation
- Published status enforcement for non-admins
- Global vs server-specific document visibility
- Type-based filtering via
DocumentService::getAllowedTypesForUserOne minor note: the duplicated server association check (lines 82-84 and 93-95) could be extracted, but it's clear as-is.
18-21: LGTM on admin panel permission methods.The delegation to Gate permissions follows Pelican's space-separated permission pattern consistently across all CRUD operations.
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (2)
145-163: LGTM on type normalization with legacy support.Good defensive handling with
tryFromLegacyfor backward compatibility and logging on invalid types with a safe default.
50-59: LGTM on the permission guide modal.Clean implementation using a Blade partial for the modal content.
server-documentation/src/Providers/ServerDocumentationServiceProvider.php (4)
27-33: Unused$appparameter is standard Laravel singleton pattern.PHPMD flags this, but Laravel's container closure signature conventionally includes
$appfor consistency even when unused. This is a false positive.
64-71: LGTM on dynamic relation registration.Using
resolveRelationUsingis the correct pattern for plugins to add relations to core models without modifying them.
87-110: LGTM on gate registration with explicit permissions mode.The permission logic is clear:
- Root admins always have access
- When
explicit_permissionsis enabled, non-root users are denied- Otherwise, server management permissions (
update serverorcreate server) grant document accessThis provides a sensible default while allowing stricter control via configuration.
73-73:registerCustomRelationsis a valid Filament extension point and correctly used.
ServerResourceis imported from the main application (App\Filament\Admin\Resources\Servers\ServerResource). The method is consistently used across multiple plugins (ServerDocumentation, Subdomains, UserCreatableServers) with relation manager classes, indicating it's a well-established API for registering custom relations in Filament resources.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (2)
103-112: LGTM on restore action with inline type assertion.Good use of
/**@varDocument $document */for the inline type assertion. The restore flow with confirmation modal and success notification is well implemented.
86-94: LGTM on version preview modal.Clean implementation rendering the version preview via a Blade view inside a modal.
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (2)
22-25: Unused parameters are required byRelationManagerinterface.PHPMD flags
$ownerRecordand$pageClassas unused, but these are required by the parent class's method signature. This is a false positive.
27-69: LGTM on table configuration.The relation manager correctly implements:
- Drag-and-drop reordering via
pivot.sort_order- Searchable attach with UUID support
- Proper empty state messaging
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (2)
98-192: LGTM!The form structure is well-organized with logical sections (Details, Content, Server Assignment, Permission Guide). The slug auto-generation on blur, conditional visibility for server assignment based on
is_globaland operation context, and egg-based server filtering are all implemented correctly.
194-230: LGTM!The table configuration properly defines columns with appropriate toggleability, filters for type/global/published/trashed states, and standard edit/delete actions. The default sort by
sort_orderis appropriate for document ordering.server-documentation/src/Services/MarkdownConverter.php (2)
314-342: LGTM with noted limitations.The frontmatter parser correctly handles basic key-value pairs and boolean conversion. The use of
explode(':', $line, 2)properly handles values containing colons. The implementation is appropriately simple to match the output format ofaddFrontmatter.
16-28: LGTM!The CommonMark environment is properly configured with
html_inputcontrolled by config andallow_unsafe_linksdisabled for security. The GithubFlavoredMarkdownExtension provides good table and strikethrough support.server-documentation/src/Models/Document.php (3)
180-198: LGTM!The type-specific scopes are well-implemented and properly use the
DocumentTypeenum values. ThescopeServerAdmincorrectly includesLEGACY_ADMINfor backward compatibility.
256-295: LGTM!The type helper methods properly delegate to the
DocumentTypeenum for visibility and hierarchy logic. The use oftryFromLegacyingetDocumentType()handles legacy type values gracefully.
141-178: LGTM!The relationships are correctly defined with appropriate configurations. The
servers()relationship properly includes pivot data and ordering. Version-related methods appropriately delegate toDocumentServicefor centralized logic.server-documentation/src/Services/DocumentService.php (4)
188-233: Well-implemented version creation with rate limiting.The 30-second debounce mechanism that updates existing versions instead of creating new ones is a good approach to prevent version spam. The use of
DB::transactionwithlockForUpdate()properly handles concurrent writes and prevents race conditions on version numbers.
259-287: LGTM!The
restoreVersionmethod correctly usesupdateQuietlyto prevent recursive versioning from model events, then explicitly creates a version with an appropriate change summary. The audit logging before and after restoration provides good traceability.
369-394: LGTM!The
pruneVersionsimplementation correctly identifies versions to keep by ordering byversion_numberdescending and limiting to the configured count, then deletes the rest. The audit logging for pruning operations provides good visibility.
144-154: LGTM!The
isServerModmethod properly checks for the existence ofSubuserPermissionenum before using it, providing graceful degradation. The server-scoped permission checks with control-related permissions are appropriate for moderator detection.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| return new class extends Migration | ||
| { | ||
| public function up(): void | ||
| { | ||
| $driver = Schema::getConnection()->getDriverName(); | ||
|
|
||
| // Change enum to string for flexibility with new document types | ||
| if ($driver === 'sqlite') { | ||
| // SQLite doesn't support ALTER COLUMN, but it also doesn't enforce enum types | ||
| // The column will accept any value, so we just need to update the data | ||
| } elseif ($driver === 'mysql' || $driver === 'mariadb') { | ||
| DB::statement("ALTER TABLE documents MODIFY COLUMN type VARCHAR(50) NOT NULL DEFAULT 'player'"); | ||
| } elseif ($driver === 'pgsql') { | ||
| // PostgreSQL: drop the enum constraint and change to varchar | ||
| DB::statement("ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)"); | ||
| DB::statement("ALTER TABLE documents ALTER COLUMN type SET DEFAULT 'player'"); | ||
| } | ||
|
|
||
| // Migrate old 'admin' type to 'server_admin' for all drivers | ||
| DB::table('documents')->where('type', 'admin')->update(['type' => 'server_admin']); | ||
| } | ||
|
|
||
| public function down(): void | ||
| { | ||
| $driver = Schema::getConnection()->getDriverName(); | ||
|
|
||
| // Migrate back to old types | ||
| DB::table('documents')->where('type', 'server_admin')->update(['type' => 'admin']); | ||
| DB::table('documents')->whereIn('type', ['host_admin', 'server_mod'])->update(['type' => 'admin']); | ||
|
|
||
| // Change back to enum (MySQL only - other drivers will just have varchar) | ||
| if ($driver === 'mysql' || $driver === 'mariadb') { | ||
| DB::statement("ALTER TABLE documents MODIFY COLUMN type ENUM('admin', 'player') NOT NULL DEFAULT 'player'"); | ||
| } |
There was a problem hiding this comment.
Fix Pint style violations (single quotes + brace position).
CI reports Pint failures. Align this migration with the project’s style rules to unblock the lint job.
✅ Suggested fix
-return new class extends Migration
-{
+return new class extends Migration {
@@
- DB::statement("ALTER TABLE documents MODIFY COLUMN type VARCHAR(50) NOT NULL DEFAULT 'player'");
+ DB::statement('ALTER TABLE documents MODIFY COLUMN type VARCHAR(50) NOT NULL DEFAULT \'player\'');
@@
- DB::statement("ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)");
- DB::statement("ALTER TABLE documents ALTER COLUMN type SET DEFAULT 'player'");
+ DB::statement('ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)');
+ DB::statement('ALTER TABLE documents ALTER COLUMN type SET DEFAULT \'player\'');
@@
- DB::statement("ALTER TABLE documents MODIFY COLUMN type ENUM('admin', 'player') NOT NULL DEFAULT 'player'");
+ DB::statement('ALTER TABLE documents MODIFY COLUMN type ENUM(\'admin\', \'player\') NOT NULL DEFAULT \'player\'');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new class extends Migration | |
| { | |
| public function up(): void | |
| { | |
| $driver = Schema::getConnection()->getDriverName(); | |
| // Change enum to string for flexibility with new document types | |
| if ($driver === 'sqlite') { | |
| // SQLite doesn't support ALTER COLUMN, but it also doesn't enforce enum types | |
| // The column will accept any value, so we just need to update the data | |
| } elseif ($driver === 'mysql' || $driver === 'mariadb') { | |
| DB::statement("ALTER TABLE documents MODIFY COLUMN type VARCHAR(50) NOT NULL DEFAULT 'player'"); | |
| } elseif ($driver === 'pgsql') { | |
| // PostgreSQL: drop the enum constraint and change to varchar | |
| DB::statement("ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)"); | |
| DB::statement("ALTER TABLE documents ALTER COLUMN type SET DEFAULT 'player'"); | |
| } | |
| // Migrate old 'admin' type to 'server_admin' for all drivers | |
| DB::table('documents')->where('type', 'admin')->update(['type' => 'server_admin']); | |
| } | |
| public function down(): void | |
| { | |
| $driver = Schema::getConnection()->getDriverName(); | |
| // Migrate back to old types | |
| DB::table('documents')->where('type', 'server_admin')->update(['type' => 'admin']); | |
| DB::table('documents')->whereIn('type', ['host_admin', 'server_mod'])->update(['type' => 'admin']); | |
| // Change back to enum (MySQL only - other drivers will just have varchar) | |
| if ($driver === 'mysql' || $driver === 'mariadb') { | |
| DB::statement("ALTER TABLE documents MODIFY COLUMN type ENUM('admin', 'player') NOT NULL DEFAULT 'player'"); | |
| } | |
| return new class extends Migration { | |
| { | |
| public function up(): void | |
| { | |
| $driver = Schema::getConnection()->getDriverName(); | |
| // Change enum to string for flexibility with new document types | |
| if ($driver === 'sqlite') { | |
| // SQLite doesn't support ALTER COLUMN, but it also doesn't enforce enum types | |
| // The column will accept any value, so we just need to update the data | |
| } elseif ($driver === 'mysql' || $driver === 'mariadb') { | |
| DB::statement('ALTER TABLE documents MODIFY COLUMN type VARCHAR(50) NOT NULL DEFAULT \'player\''); | |
| } elseif ($driver === 'pgsql') { | |
| // PostgreSQL: drop the enum constraint and change to varchar | |
| DB::statement('ALTER TABLE documents ALTER COLUMN type TYPE VARCHAR(50)'); | |
| DB::statement('ALTER TABLE documents ALTER COLUMN type SET DEFAULT \'player\''); | |
| } | |
| // Migrate old 'admin' type to 'server_admin' for all drivers | |
| DB::table('documents')->where('type', 'admin')->update(['type' => 'server_admin']); | |
| } | |
| public function down(): void | |
| { | |
| $driver = Schema::getConnection()->getDriverName(); | |
| // Migrate back to old types | |
| DB::table('documents')->where('type', 'server_admin')->update(['type' => 'admin']); | |
| DB::table('documents')->whereIn('type', ['host_admin', 'server_mod'])->update(['type' => 'admin']); | |
| // Change back to enum (MySQL only - other drivers will just have varchar) | |
| if ($driver === 'mysql' || $driver === 'mariadb') { | |
| DB::statement('ALTER TABLE documents MODIFY COLUMN type ENUM(\'admin\', \'player\') NOT NULL DEFAULT \'player\''); | |
| } |
🤖 Prompt for AI Agents
In
`@server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php`
around lines 7 - 40, Update the migration to satisfy Pint: use single quotes for
all string literals (e.g., change DB::statement("...") and where(..., "player")
to single quotes) and adjust brace placement to match project style (move
opening braces for the anonymous class and the up() and down() methods to the
next line as required); key locations to edit are the "return new class extends
Migration" declaration and the up() and down() methods, plus all DB::statement
and DB::table/... string literals (including 'player', 'admin', 'server_admin',
'host_admin', 'server_mod').
| } else { | ||
| // SQLite and others: Just use regular unique (less ideal but functional) | ||
| Schema::table('documents', function (Blueprint $table) { | ||
| $table->unique('slug'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
SQLite fallback defeats the purpose of this migration.
For SQLite, restoring a regular unique('slug') constraint means slugs cannot be reused after soft delete—the very problem this migration is meant to solve. SQLite 3.9+ supports partial indexes via raw SQL.
🔧 Proposed fix using SQLite partial index
- } else {
- // SQLite and others: Just use regular unique (less ideal but functional)
- Schema::table('documents', function (Blueprint $table) {
- $table->unique('slug');
- });
+ } elseif ($driver === 'sqlite') {
+ // SQLite 3.9+: Use a partial unique index
+ DB::statement('CREATE UNIQUE INDEX idx_documents_slug_active ON documents(slug) WHERE deleted_at IS NULL');
+ } else {
+ // Fallback for unsupported drivers: regular unique (slug reuse after soft delete won't work)
+ Schema::table('documents', function (Blueprint $table) {
+ $table->unique('slug');
+ });
}Corresponding change in down():
- } else {
- Schema::table('documents', function (Blueprint $table) {
- $table->dropUnique(['slug']);
- });
+ } elseif ($driver === 'sqlite') {
+ DB::statement('DROP INDEX idx_documents_slug_active');
+ } else {
+ Schema::table('documents', function (Blueprint $table) {
+ $table->dropUnique(['slug']);
+ });
}🤖 Prompt for AI Agents
In
`@server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php`
around lines 36 - 41, The SQLite branch in the migration currently adds a plain
unique('slug') which prevents slug reuse after soft deletes; instead, in the
migration's up() replace that Schema::table(...) block with a raw DB::statement
that creates a partial unique index on documents(slug) WHERE deleted_at IS NULL
(e.g. CREATE UNIQUE INDEX documents_slug_unique_not_deleted ON documents (slug)
WHERE deleted_at IS NULL) so SQLite 3.9+ enforces uniqueness only for
non-deleted rows; also update down() to drop that index with a raw DB::statement
(e.g. DROP INDEX IF EXISTS documents_slug_unique_not_deleted) to fully reverse
the migration.
| ->emptyStateHeading(trans('server-documentation::strings.relation_managers.no_servers_linked')) | ||
| ->emptyStateDescription(trans('server-documentation::strings.relation_managers.attach_servers_description')) | ||
| ->emptyStateIcon('tabler-file-off'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for document-specific translation keys
rg -n "no_documents|attach_documents" server-documentation/lang/Repository: pelican-dev/plugins
Length of output: 384
🏁 Script executed:
# Check the full context of the relation manager file
cat -n server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php | head -100Repository: pelican-dev/plugins
Length of output: 3929
🏁 Script executed:
# Check what translation keys exist for servers and documents in relation managers
rg -n "relation_managers\.(no_servers|attach_servers|no_documents|attach_documents)" server-documentation/lang/Repository: pelican-dev/plugins
Length of output: 45
🏁 Script executed:
# Check if the used keys actually exist in translation files
rg -n "no_servers_linked|attach_servers_description" server-documentation/lang/Repository: pelican-dev/plugins
Length of output: 310
🏁 Script executed:
# Check the full structure of the relation_managers section in translations
rg -n "relation_managers" server-documentation/lang/ -A 5 -B 2Repository: pelican-dev/plugins
Length of output: 748
Translation key semantics are inverted for Documents relation manager.
The empty state uses relation_managers.no_servers_linked and relation_managers.attach_servers_description, which are semantically incorrect for a Documents relation manager. These keys exist and are defined for server-centric use, but when used in DocumentsRelationManager (which manages the documents relationship), they create confusing messaging. The empty state should convey that no documents are linked, not servers. Either define document-specific translation keys in the relation_managers section or reuse more appropriate generic keys that align with the relation manager's purpose.
🤖 Prompt for AI Agents
In
`@server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php`
around lines 80 - 82, The empty state texts in DocumentsRelationManager are
using server-centric translation keys; update the calls to emptyStateHeading,
emptyStateDescription (and any related empty state labels) in the
DocumentsRelationManager class to use document-specific translation keys (e.g.,
relation_managers.no_documents_linked and
relation_managers.attach_documents_description) or else swap to existing generic
keys that accurately describe "no documents linked"; then add those new keys
with appropriate translations to the relation_managers section of your language
files so the messages align with the documents relationship.
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
Outdated
Show resolved
Hide resolved
| Section::make(trans('server-documentation::strings.form.server_assignment')) | ||
| ->description(trans('server-documentation::strings.form.server_assignment_description')) | ||
| ->collapsed(fn (?Document $record) => $record !== null) | ||
| ->visible(fn (?Document $record, string $operation) => $operation === 'create' || ($record && !$record->is_global)) |
There was a problem hiding this comment.
PHPStan error: undefined property $is_global.
The static analysis tool reports that $is_global is not a declared property on the Document model. While Eloquent models access attributes via magic methods, PHPStan requires explicit @property annotations for type safety.
Add a PHPDoc block to the Document model class:
/**
* `@property` bool $is_global
* `@property` bool $is_published
* `@property` int $sort_order
* ...
*/
class Document extends Model🧰 Tools
🪛 GitHub Check: PHPStan
[failure] 153-153:
Access to an undefined property Starter\ServerDocumentation\Models\Document::$is_global.
🤖 Prompt for AI Agents
In `@server-documentation/src/Filament/Admin/Resources/DocumentResource.php` at
line 153, The PHPStan error is due to missing `@property` annotations on the
Document Eloquent model for attributes accessed as magic properties (e.g.,
$record->is_global); edit the Document model class (class Document extends
Model) and add a PHPDoc block above the class with `@property` annotations for the
attributes used (at minimum `@property` bool $is_global, `@property` bool
$is_published, `@property` int $sort_order, etc.) so PHPStan recognizes these
magic attributes referenced by code such as the visible callback in
DocumentResource.
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php
Show resolved
Hide resolved
...on/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
Show resolved
Hide resolved
...on/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
Outdated
Show resolved
Hide resolved
| public function boot(Panel $panel): void | ||
| { | ||
| } |
There was a problem hiding this comment.
Empty boot() method causing Pint formatting violation.
The empty method body may be triggering the single_line_empty_lines Pint rule. The unused $panel parameter is acceptable since it's required by the Plugin interface contract.
🧹 Proposed fix
public function boot(Panel $panel): void
{
+ //
}Or collapse to a single-line empty body if Pint allows:
- public function boot(Panel $panel): void
- {
- }
+ public function boot(Panel $panel): void {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function boot(Panel $panel): void | |
| { | |
| } | |
| public function boot(Panel $panel): void | |
| { | |
| // | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
39-39: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@server-documentation/src/ServerDocumentationPlugin.php` around lines 39 - 41,
The empty body of the Plugin::boot implementation (public function boot(Panel
$panel): void) is causing a Pint formatting violation; update the
ServerDocumentationPlugin::boot method to use a single-line empty body or
otherwise satisfy Pint (e.g., change to "public function boot(Panel $panel):
void {}" or add a concise comment statement) while keeping the unused Panel
$panel parameter to satisfy the interface contract; ensure the method signature
remains unchanged.
A documentation management plugin that allows administrators to create, organize, and distribute documentation to server users with granular permission-based visibility. Features: - Rich text editor with WYSIWYG editing - 4-tier permission system (Host Admin, Server Admin, Server Mod, Player) - Global and server-specific documentation - Server assignment during creation with egg-based filtering - Version history with preview and restore functionality - Markdown import/export with YAML frontmatter support - Drag-and-drop reordering in relation managers - Audit logging with configurable channel - Caching with tag support and fallback for non-tagging drivers Security: - HTML sanitization on markdown import - Authorization checks on all endpoints - Rate limiting on version creation - XSS prevention via html_input escaping Infrastructure: - Cross-database compatibility (MySQL, PostgreSQL, SQLite) - Comprehensive test suite with Pest PHP - Model factories for testing - Configurable via environment variables
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@server-documentation/resources/views/filament/partials/permission-guide.blade.php`:
- Around line 41-55: Replace the hardcoded English example text in the
permission-guide Blade view with translation keys to maintain i18n consistency:
replace the "On", "Off", "Examples:" label and each example sentence ("Welcome
guide everyone sees everywhere", "Rules for one server only", "Company-wide
admin procedures", "Mod notes for one server") with calls to the translation
helper (e.g., trans(...) or __(...)) using new or existing keys in your
server-documentation translation namespace so they match the surrounding
trans('server-documentation::strings...') usage; update the view strings in
permission-guide.blade.php (the list items and the <p> label) to use those
translation keys and add corresponding entries to the server-documentation
translation files.
In `@server-documentation/src/Models/Document.php`:
- Around line 84-94: The generateUniqueSlug() approach can still suffer a race:
update Document::booted() creating handler to retry slug creation on conflict by
wrapping slug generation and insert in a short loop/transaction and handling
unique-constraint exceptions from the DB; specifically, inside the
static::creating(...) closure call generateUniqueSlug($document->title) and
attempt to persist, catching the unique key exception (or using DB::transaction
with SERIALIZABLE/isolation and retry up to N times) and on retry call
Document::generateUniqueSlug again (enhance generateUniqueSlug to accept a retry
attempt and append a short random suffix when attempt>0); alternatively
implement the catch-and-retry logic in Document::save() to detect duplicate-slug
SQL errors, regenerate the slug, and retry the save a few times before bubbling
the exception.
In `@server-documentation/src/Services/MarkdownConverter.php`:
- Around line 43-47: The phpdoc block for the MarkdownConverter method (the
"Convert Markdown content to HTML." doc) is misaligned; run Pint or manually fix
phpdoc_align by aligning the asterisks and tag columns (e.g., `@param bool
$sanitize` and the description) to match project style; update the docblock in
the MarkdownConverter class/method so tags and descriptions are vertically
aligned and then run Pint to ensure no more phpdoc_align issues.
♻️ Duplicate comments (9)
server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (1)
36-41: SQLite fallback defeats the soft-delete slug reuse goal.SQLite 3.9+ supports partial indexes. The current fallback re-adds a plain unique constraint, which blocks slug reuse after soft delete—the exact problem this migration aims to solve.
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
7-40: Pint style violations still block CI (single quotes + brace position).
Please apply Pint (or adjust to project style) to resolve the lint failure.server-documentation/src/Services/MarkdownConverter.php (1)
292-345: Frontmatter round‑trip is lossy for quoted values.
addFrontmatter()emits quoted strings, butparseFrontmatter()never unquotes/unescapes them. This leaves titles like"A: B"with quotes after import and can break newline handling.🛠️ Suggested fix
- } elseif (is_string($value) && $this->needsYamlQuoting($value)) { - $value = '"' . addcslashes($value, '"\\') . '"'; + } elseif (is_string($value) && $this->needsYamlQuoting($value)) { + $value = '"' . str_replace( + ["\\", "\"", "\n"], + ["\\\\", "\\\"", "\\n"], + $value + ) . '"'; } @@ - if ($value === 'true') { + if ($value === 'true') { $value = true; } elseif ($value === 'false') { $value = false; + } elseif (preg_match('/^"(.*)"$/s', $value, $m)) { + $value = str_replace(['\\n', '\\"', '\\\\'], ["\n", '"', '\\'], $m[1]); + } elseif (preg_match("/^'(.*)'$/s", $value, $m)) { + $value = str_replace("''", "'", $m[1]); }server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php (1)
75-76: Empty-state strings refer to servers, not documents.
Line 75-76 uses server-centric translation keys in a documents relation manager; the copy will be misleading. Please switch to document-specific keys and add translations if missing.🧹 Suggested adjustment
- ->emptyStateHeading(trans('server-documentation::strings.relation_managers.no_servers_linked')) - ->emptyStateDescription(trans('server-documentation::strings.relation_managers.attach_servers_description')) + ->emptyStateHeading(trans('server-documentation::strings.relation_managers.no_documents_linked')) + ->emptyStateDescription(trans('server-documentation::strings.relation_managers.attach_documents_description'))server-documentation/src/ServerDocumentationPlugin.php (1)
39-41: Fix emptyboot()to satisfy Pint.
Line 39-41 is an empty body and CI shows Pint failing on single-line empty rules.🧹 Minimal fix
- public function boot(Panel $panel): void - { - } + public function boot(Panel $panel): void + { + // + }server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
23-38: LGTM!The
@property Document $recordannotation addresses the PHPStan type concerns from previous reviews. The mount and record resolution follow Filament'sInteractsWithRecordpattern correctly.server-documentation/src/Filament/Admin/Resources/DocumentResource.php (2)
23-24: LGTM!The table actions are now correctly imported from
Filament\Tables\Actionsnamespace.
146-149: PHPStan error:$is_globalproperty access.The
$record->is_globalaccess triggers a PHPStan error because theDocumentmodel lacks@propertyannotations for its Eloquent attributes. This requires adding PHPDoc annotations to theDocumentmodel class.Verify the Document model has the necessary
@propertyannotations:#!/bin/bash # Check Document model for `@property` annotations ast-grep --pattern $'/** * $$$ */ class Document extends Model { $$$ }'server-documentation/src/Services/DocumentService.php (1)
134-139:isServerAdminstill uses global permissions instead of server-scoped.This issue was flagged in a previous review but remains unaddressed. The method uses
$user->can('update server')and$user->can('create server')without passing$serveras context, meaning any user with global server management permissions becomes admin on all servers.🔧 Proposed fix
public function isServerAdmin(User $user, Server $server): bool { return $server->owner_id === $user->id || - $user->can('update server') || - $user->can('create server'); + $user->can('update server', $server) || + $user->can('create server', $server); }
🧹 Nitpick comments (7)
server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (1)
9-14: Migration appears redundant given the sequence.This migration adds a unique constraint on
slug, but migration000006immediately drops it (line 25) and replaces it with database-specific partial unique indexes. Since both migrations are new in this PR, consider either:
- Removing this migration entirely and having
000006work without expecting a pre-existing unique constraint, or- Consolidating the logic into a single migration.
Running these sequentially creates unnecessary schema churn and could cause confusion during incremental deployments.
server-documentation/resources/views/filament/pages/version-preview.blade.php (1)
27-29: UseMarkdownConverter::sanitizeHtml()instead of the Filament macro for consistency.The blade templates directly call
str($version->content)->sanitizeHtml(), which relies on Filament's macro. While this works in Filament pages, using the injectedMarkdownConverterservice provides a more consistent and maintainable approach—especially since the service already contains proper fallback sanitization logic that doesn't depend on external macros.🛠️ Suggested fix
<div class="prose prose-sm dark:prose-invert max-w-none"> - {!! str($version->content)->sanitizeHtml() !!} + {!! $this->markdownConverter->toHtml($version->content, sanitize: true) !!} </div>Consider injecting
MarkdownConverterinto the page component and using itstoHtml()method, which safely handles both markdown-to-HTML conversion and sanitization.server-documentation/src/Providers/ServerDocumentationServiceProvider.php (1)
27-33: Simplify singleton registrations by removing unused parameter.The
$appparameter is declared but unused in both singleton closures. This can be simplified.♻️ Proposed fix
- $this->app->singleton(DocumentService::class, function ($app) { + $this->app->singleton(DocumentService::class, function () { return new DocumentService(); }); - $this->app->singleton(MarkdownConverter::class, function ($app) { + $this->app->singleton(MarkdownConverter::class, function () { return new MarkdownConverter(); });server-documentation/src/Filament/Admin/Resources/DocumentResource.php (1)
66-78: Consider simplifying translation fallback pattern.The pattern checking if translation equals the key is verbose and repeated. Consider extracting to a helper or using Laravel's
trans_choicewith fallback, or leveraging__()with a default parameter.♻️ Example simplification
// Helper method approach protected static function transWithFallback(string $key, string $fallback): string { $translation = trans($key); return $translation !== $key ? $translation : $fallback; } // Then use: public static function getModelLabel(): string { return static::transWithFallback('server-documentation::strings.document.singular', 'Document'); }server-documentation/src/Services/DocumentService.php (1)
324-329: Consider making supported cache drivers configurable.The hardcoded list of tag-supporting drivers (
redis,memcached,dynamodb,array) may not cover all scenarios (e.g., custom drivers or future Laravel additions).♻️ Alternative approach
protected function cacheSupportsTagging(): bool { try { Cache::tags(['test']); return true; } catch (\BadMethodCallException $e) { return false; } }Or make it configurable:
protected function cacheSupportsTagging(): bool { return config('server-documentation.cache_supports_tagging', in_array(config('cache.default'), ['redis', 'memcached', 'dynamodb', 'array']) ); }server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (1)
26-68: Table configuration looks well-structured.The relation manager correctly configures:
- Reorderable pivot with
sort_order- Searchable/sortable columns for name and node
- Attach action with preloaded select and custom form for sort order
- Detach actions (single and bulk)
- Empty state with appropriate messaging
One minor consideration: the
sort_orderinput in the attach form (line 53-56) allows any integer including negatives. If negative values are undesirable, consider adding aminValue(0)constraint.♻️ Optional: Add minimum value constraint
TextInput::make('sort_order') ->numeric() ->default(0) + ->minValue(0) ->helperText(trans('server-documentation::strings.relation_managers.sort_order_helper')),server-documentation/src/Models/Document.php (1)
129-136: Consider performance impact of auto-pruning on every save.The
savedhook unconditionally callsclearDocumentCache()and conditionally callspruneVersions(). While cache clearing is necessary, callingpruneVersions()on every save (when enabled) could be expensive for documents with many versions, as it queries and potentially deletes records.Consider rate-limiting the pruning (e.g., only prune every N saves) or deferring it to a queued job.
♻️ Alternative: Queue the pruning operation
static::saved(function (Document $document) { app(DocumentService::class)->clearDocumentCache($document); app(DocumentService::class)->clearCountCache(); if (config('server-documentation.auto_prune_versions', false)) { - app(DocumentService::class)->pruneVersions($document); + dispatch(fn () => app(DocumentService::class)->pruneVersions($document))->afterCommit(); } });Or use a dedicated job class for better visibility and retry handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
server-documentation/docs/images/admin-create-document.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-documents-list.pngis excluded by!**/*.pngserver-documentation/docs/images/admin-edit-document.pngis excluded by!**/*.pngserver-documentation/docs/images/player-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-admin-view.pngis excluded by!**/*.pngserver-documentation/docs/images/server-mod-view.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-preview.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restore.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history-restored.pngis excluded by!**/*.pngserver-documentation/docs/images/version-history.pngis excluded by!**/*.png
📒 Files selected for processing (42)
server-documentation/LICENSEserver-documentation/README.mdserver-documentation/composer.jsonserver-documentation/config/server-documentation.phpserver-documentation/database/factories/DocumentFactory.phpserver-documentation/database/factories/DocumentVersionFactory.phpserver-documentation/database/migrations/2024_01_01_000001_create_documents_table.phpserver-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.phpserver-documentation/database/migrations/2024_01_01_000003_create_document_server_table.phpserver-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.phpserver-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.phpserver-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.phpserver-documentation/lang/en/strings.phpserver-documentation/plugin.jsonserver-documentation/resources/css/document-content.cssserver-documentation/resources/views/filament/pages/document-versions.blade.phpserver-documentation/resources/views/filament/pages/version-preview.blade.phpserver-documentation/resources/views/filament/partials/permission-guide.blade.phpserver-documentation/resources/views/filament/server/pages/documents.blade.phpserver-documentation/src/Enums/DocumentType.phpserver-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.phpserver-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.phpserver-documentation/src/Filament/Concerns/HasDocumentTableColumns.phpserver-documentation/src/Filament/Server/Pages/Documents.phpserver-documentation/src/Models/Document.phpserver-documentation/src/Models/DocumentVersion.phpserver-documentation/src/Policies/DocumentPolicy.phpserver-documentation/src/Providers/ServerDocumentationServiceProvider.phpserver-documentation/src/ServerDocumentationPlugin.phpserver-documentation/src/Services/DocumentService.phpserver-documentation/src/Services/MarkdownConverter.phpserver-documentation/tests/Pest.phpserver-documentation/tests/Unit/Enums/DocumentTypeTest.phpserver-documentation/tests/Unit/Models/DocumentTest.phpserver-documentation/tests/Unit/Policies/DocumentPolicyTest.phpserver-documentation/tests/Unit/Services/DocumentServiceTest.phpserver-documentation/tests/Unit/Services/MarkdownConverterTest.php
✅ Files skipped from review due to trivial changes (1)
- server-documentation/README.md
🚧 Files skipped from review as they are similar to previous changes (19)
- server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/EditDocument.php
- server-documentation/tests/Unit/Models/DocumentTest.php
- server-documentation/src/Enums/DocumentType.php
- server-documentation/LICENSE
- server-documentation/lang/en/strings.php
- server-documentation/src/Filament/Concerns/HasDocumentTableColumns.php
- server-documentation/tests/Unit/Services/MarkdownConverterTest.php
- server-documentation/tests/Pest.php
- server-documentation/src/Filament/Server/Pages/Documents.php
- server-documentation/resources/views/filament/pages/document-versions.blade.php
- server-documentation/src/Models/DocumentVersion.php
- server-documentation/database/factories/DocumentFactory.php
- server-documentation/composer.json
- server-documentation/plugin.json
- server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/CreateDocument.php
- server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php
- server-documentation/resources/views/filament/server/pages/documents.blade.php
- server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php
- server-documentation/config/server-documentation.php
🧰 Additional context used
🧬 Code graph analysis (11)
server-documentation/database/factories/DocumentVersionFactory.php (2)
server-documentation/src/Models/Document.php (1)
Document(21-314)server-documentation/src/Models/DocumentVersion.php (2)
DocumentVersion(13-58)document(44-47)
server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (2)
server-documentation/src/Services/MarkdownConverter.php (2)
MarkdownConverter(12-353)parseFrontmatter(324-352)server-documentation/src/Enums/DocumentType.php (2)
isValid(136-139)tryFromLegacy(124-131)
server-documentation/tests/Unit/Enums/DocumentTypeTest.php (1)
server-documentation/src/Enums/DocumentType.php (12)
hierarchyLevel(82-90)color(56-64)icon(69-77)tryFromLegacy(124-131)isValid(136-139)typesVisibleToLevel(105-119)isVisibleToLevel(95-98)options(146-154)simpleOptions(161-169)formatLabel(174-179)formatColor(184-189)formatIcon(194-199)
server-documentation/tests/Unit/Services/DocumentServiceTest.php (5)
server-documentation/src/Models/Document.php (4)
Document(21-314)createVersion(167-170)restoreVersion(172-175)versions(161-165)server-documentation/src/Services/DocumentService.php (8)
DocumentService(17-413)getAllowedTypesForUser(100-109)getUserHierarchyLevel(114-129)isServerAdmin(134-139)createVersion(239-254)restoreVersion(259-287)getDocumentCount(347-356)pruneVersions(369-394)server-documentation/src/Policies/DocumentPolicy.php (2)
create(34-37)update(42-45)server-documentation/src/Enums/DocumentType.php (1)
hierarchyLevel(82-90)server-documentation/src/Models/DocumentVersion.php (1)
document(44-47)
server-documentation/src/Providers/ServerDocumentationServiceProvider.php (3)
server-documentation/src/Models/Document.php (1)
Document(21-314)server-documentation/src/Policies/DocumentPolicy.php (1)
DocumentPolicy(12-101)server-documentation/src/Services/DocumentService.php (1)
DocumentService(17-413)
server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (2)
server-documentation/src/Models/Document.php (2)
Document(21-314)servers(153-159)server-documentation/src/Policies/DocumentPolicy.php (1)
create(34-37)
server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (4)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (1)
up(9-29)server-documentation/database/migrations/2024_01_01_000002_create_document_versions_table.php (1)
up(9-24)server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
up(9-27)server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (1)
up(10-47)
server-documentation/src/Models/Document.php (4)
server-documentation/database/factories/DocumentFactory.php (2)
DocumentFactory(14-98)published(70-73)server-documentation/src/Services/DocumentService.php (7)
DocumentService(17-413)generateChangeSummary(161-182)createVersionFromOriginal(188-233)clearDocumentCache(292-301)pruneVersions(369-394)createVersion(239-254)getAllowedTypesForUser(100-109)server-documentation/src/Models/DocumentVersion.php (2)
document(44-47)DocumentVersion(13-58)server-documentation/src/Enums/DocumentType.php (3)
LEGACY_ADMIN(25-38)tryFromLegacy(124-131)hierarchyLevel(82-90)
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php (1)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (2)
up(9-29)down(31-34)
server-documentation/database/migrations/2024_01_01_000005_add_unique_constraint_to_documents_slug.php (2)
server-documentation/database/migrations/2024_01_01_000001_create_documents_table.php (2)
up(9-29)down(31-34)server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (2)
up(10-47)down(49-81)
server-documentation/src/ServerDocumentationPlugin.php (2)
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (1)
DocumentResource(37-246)server-documentation/src/Providers/ServerDocumentationServiceProvider.php (2)
register(20-34)boot(36-74)
🪛 GitHub Actions: Lint
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[error] 1-1: Pint style issues detected (unknown specifics).
server-documentation/src/Services/DocumentService.php
[error] 1-1: Pint style issues detected: phpdoc_separation,…
server-documentation/src/Services/MarkdownConverter.php
[error] 1-1: Pint style issues detected: phpdoc_align
server-documentation/src/Models/Document.php
[error] 1-1: Pint style issues detected: class_attributes_separation.
server-documentation/database/migrations/2024_01_01_000004_update_documents_type_column.php
[error] 1-1: Pint style issues detected: class_definition, single_quote, braces_position.
server-documentation/src/ServerDocumentationPlugin.php
[error] 1-1: Pint style issues detected: single_line_empty_…
🪛 GitHub Check: PHPStan
server-documentation/src/Filament/Admin/Resources/DocumentResource.php
[failure] 217-217:
Call to static method make() on an unknown class Filament\Tables\Actions\EditAction.
[failure] 184-184:
Parameter #1 $view of function view expects view-string|null, string given.
[failure] 149-149:
Access to an undefined property Starter\ServerDocumentation\Models\Document::$is_global.
server-documentation/src/Filament/Admin/RelationManagers/DocumentsRelationManager.php
[failure] 73-73:
Call to static method make() on an unknown class Filament\Tables\Actions\DetachBulkAction.
[failure] 70-70:
Call to static method make() on an unknown class Filament\Tables\Actions\DetachAction.
[failure] 68-68:
Call to static method make() on an unknown class Filament\Tables\Actions\ViewAction.
[failure] 61-61:
Call to static method make() on an unknown class Filament\Tables\Actions\CreateAction.
[failure] 54-54:
Call to method getRecordSelect() on an unknown class Filament\Tables\Actions\AttachAction.
[failure] 53-53:
Parameter $action of anonymous function has invalid type Filament\Tables\Actions\AttachAction.
[failure] 51-51:
Call to static method make() on an unknown class Filament\Tables\Actions\AttachAction.
🪛 PHPMD (2.15.0)
server-documentation/src/Providers/ServerDocumentationServiceProvider.php
31-31: Avoid unused parameters such as '$app'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php
21-21: Avoid unused parameters such as '$ownerRecord'. (undefined)
(UnusedFormalParameter)
21-21: Avoid unused parameters such as '$pageClass'. (undefined)
(UnusedFormalParameter)
server-documentation/src/Policies/DocumentPolicy.php
26-26: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
42-42: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
58-58: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
server-documentation/src/ServerDocumentationPlugin.php
39-39: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (29)
server-documentation/database/migrations/2024_01_01_000006_add_performance_indexes_and_fix_slug_constraint.php (4)
12-17: LGTM!The performance indexes are well-chosen for the documented query patterns (filtering by publish status, type, global flag, and sorting). Using explicit index names improves maintainability.
28-32: LGTM!The generated column approach is a valid workaround for MySQL/MariaDB's lack of partial indexes. The
IF(deleted_at IS NULL, slug, NULL)expression correctly returnsNULLfor soft-deleted rows, and sinceNULLvalues aren't considered duplicates in unique indexes, this allows slug reuse after soft delete.
43-46: LGTM!Adding a unique constraint on
(document_id, version_number)is essential for data integrity. This provides database-level protection against duplicate version numbers, complementing the application-level 30-second debounce mentioned in the PR objectives.
70-73: Verify this aligns with migration 000005 decision.This restores a
unique('slug')constraint assuming migration 000005 originally added it. If you remove migration 000005 as suggested in my earlier comment, you'll need to also remove this restoration block, since there would be no prior unique constraint to restore.server-documentation/resources/css/document-content.css (1)
6-150: LGTM — cohesive document typography and content styling.
Scoped styles read cleanly and cover the core elements well.server-documentation/database/factories/DocumentVersionFactory.php (1)
21-46: LGTM — factory defaults and helper states are solid.
The defaults and state helpers look correct and consistent with the model.server-documentation/database/migrations/2024_01_01_000003_create_document_server_table.php (1)
11-19: UseforeignId()for theserver_idforeign key to ensure type alignment withservers.id.
The current approach usesunsignedInteger('server_id')with a manual foreign key constraint. Laravel'sforeignId()method automatically usesunsignedBigInteger, which aligns with the default ID type in modern Laravel applications. This prevents potential type mismatches on FK creation.🛠️ Suggested fix
- $table->unsignedInteger('server_id'); + $table->foreignId('server_id')->constrained('servers')->cascadeOnDelete(); $table->integer('sort_order')->default(0); $table->timestamps(); - $table->foreign('server_id')->references('id')->on('servers')->cascadeOnDelete(); $table->unique(['document_id', 'server_id']);server-documentation/tests/Unit/Enums/DocumentTypeTest.php (1)
7-105: Solid enum coverage.
Good breadth across hierarchy, legacy mapping, visibility, and formatting helpers.server-documentation/tests/Unit/Services/DocumentServiceTest.php (1)
18-231: Comprehensive service test coverage.
Versioning, restore, counts, and prune paths look well exercised.server-documentation/tests/Unit/Policies/DocumentPolicyTest.php (1)
12-115: Policy tests look solid.
Covers key permission tiers and viewOnServer scenarios cleanly.server-documentation/src/Providers/ServerDocumentationServiceProvider.php (2)
36-74: LGTM!The boot method properly loads plugin resources (migrations, views, translations), publishes config and assets, and registers the dynamic
documentsrelation on the Server model. TheresolveRelationUsingapproach is appropriate for plugin architecture.
87-110: LGTM!The gate definitions provide a clean implementation with root admin bypass and configurable explicit permissions mode. The fallback to server management permissions (
update server/create server) is reasonable for inheriting document access from server access.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ListDocuments.php (3)
85-94: LGTM!The
file_get_contentsfailure handling has been properly implemented with an early return and user notification.
100-129: LGTM!The import logic handles frontmatter parsing, title extraction with sensible fallbacks, slug uniqueness enforcement, and proper type normalization. The use of
filter_varwithFILTER_VALIDATE_BOOLEANcorrectly handles string boolean values from YAML frontmatter.
155-173: LGTM!The
normalizeDocumentTypemethod properly validates and normalizes document types using the enum'sisValidandtryFromLegacymethods, with appropriate logging for invalid types.server-documentation/src/Filament/Admin/Resources/DocumentResource/Pages/ViewDocumentVersions.php (1)
60-118: LGTM!The table configuration is well-structured with appropriate columns, sorting, and actions. The preview modal renders the version content, and the restore action uses confirmation with proper notification on success.
server-documentation/src/Filament/Admin/Resources/DocumentResource.php (2)
190-226: LGTM!The table configuration properly uses the shared column/filter methods from
HasDocumentTableColumnstrait and configures appropriate actions, sorting, and empty state messaging.
183-186: No actionable issue found.The code pattern at lines 183-186 is correct and follows Laravel conventions. The
view()->render()chain is the standard approach to render Blade templates, and wrapping the result inHtmlStringis appropriate for Filament components. This identical pattern is used consistently across multiple files (ViewDocumentVersions.php, ListDocuments.php) without issues. No actual PHPStan error evidence was found in the codebase or CI configuration to indicate this is a real problem.server-documentation/src/Policies/DocumentPolicy.php (2)
26-69: LGTM - PHPMD warnings are false positives.The
$documentparameters in these policy methods are required by Laravel's policy method signatures, even when not used in the implementation. The policy correctly delegates to the defined gates for admin panel permissions.
79-100: LGTM!The
viewOnServermethod correctly implements the 4-tier permission hierarchy with proper checks for:
- Root admin bypass with global/server association validation
- Published status requirement for non-admins
- Server-specific document attachment verification
- Type-based access via
DocumentService::getAllowedTypesForUserserver-documentation/src/Services/DocumentService.php (3)
52-93: LGTM!The document retrieval logic properly handles caching with both tag-supporting and non-tag drivers, combines server-attached and global documents while avoiding duplicates, and maintains ordering.
188-233: LGTM!The
createVersionFromOriginalmethod implements proper rate limiting with a 30-second debounce window, uses transactions withlockForUpdateto prevent race conditions, and handles both new version creation and existing version updates.
256-287: LGTM!The
restoreVersionmethod correctly usesupdateQuietlyto avoid triggering model events during restoration, then creates a proper version record to track the restore action.server-documentation/src/Filament/Admin/Resources/DocumentResource/RelationManagers/ServersRelationManager.php (2)
1-14: Imports are now correctly resolved.The previous issues with incorrect import namespaces have been addressed. Actions are now correctly imported from
Filament\Tables\Actions, and the unusedSchemaimport has been removed.
21-24: Unused parameters are required by the parent interface.The static analysis warning about unused
$ownerRecordand$pageClassparameters is a false positive. These parameters are part of Filament'sRelationManager::getTitle()signature and must be present even if not used in this implementation.server-documentation/src/Models/Document.php (4)
231-243: Search scope implementation is correct but consider full-text search for large datasets.The LIKE-based search works correctly and Laravel handles parameter escaping. However, for large document sets,
LIKE '%term%'queries won't use indexes efficiently.For better performance at scale, consider:
- Full-text search (MySQL FULLTEXT, PostgreSQL tsvector)
- Search service integration (Meilisearch, Algolia via Laravel Scout)
This is fine for initial implementation but worth noting for future optimization.
302-313: Slug generation correctly handles soft-deleted records.Good implementation detail: using
withTrashed()ensures uniqueness even against soft-deleted documents, preventing potential conflicts if a document is later restored.
143-165: Relationships are well-defined.The relationships correctly define:
authorandlastEditoras BelongsTo User with appropriate foreign keysserversas BelongsToMany with pivot data and default orderingversionsas HasMany with descending order for easy latest-first access
1-55: Model structure and properties are well-defined.The model correctly uses:
- Strict types declaration
- HasFactory and SoftDeletes traits
- Custom factory binding
- Appropriate fillable fields
- Proper blank line separation between class members per Laravel/Pint style guidelines
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"> | ||
| <strong>{{ trans('server-documentation::strings.labels.all_servers') }} Toggle:</strong> | ||
| </p> | ||
| <ul class="text-sm text-gray-600 dark:text-gray-300 space-y-1 list-disc list-inside"> | ||
| <li><strong>On</strong> → Document appears on every server</li> | ||
| <li><strong>Off</strong> → Must attach to specific servers</li> | ||
| </ul> | ||
|
|
||
| <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"><strong>Examples:</strong></p> | ||
| <ul class="text-sm text-gray-600 dark:text-gray-300 space-y-1 list-disc list-inside"> | ||
| <li><strong>Player + All Servers</strong> → Welcome guide everyone sees everywhere</li> | ||
| <li><strong>Player + Specific Server</strong> → Rules for one server only</li> | ||
| <li><strong>Server Admin + All Servers</strong> → Company-wide admin procedures</li> | ||
| <li><strong>Server Mod + Specific Server</strong> → Mod notes for one server</li> | ||
| </ul> |
There was a problem hiding this comment.
Localize hardcoded example text.
Line 41-55 uses raw English strings while surrounding content is translated; this will break i18n consistency.
🌍 Example refactor
- <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2">
- <strong>{{ trans('server-documentation::strings.labels.all_servers') }} Toggle:</strong>
- </p>
+ <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2">
+ <strong>{{ trans('server-documentation::strings.permission_guide.toggle_title') }}</strong>
+ </p>
...
- <li><strong>On</strong> → Document appears on every server</li>
- <li><strong>Off</strong> → Must attach to specific servers</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.toggle_on') }}</strong> → {{ trans('server-documentation::strings.permission_guide.toggle_on_desc') }}</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.toggle_off') }}</strong> → {{ trans('server-documentation::strings.permission_guide.toggle_off_desc') }}</li>
...
- <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"><strong>Examples:</strong></p>
+ <p class="text-sm text-gray-500 dark:text-gray-400 mt-4 mb-2"><strong>{{ trans('server-documentation::strings.permission_guide.examples_title') }}</strong></p>
...
- <li><strong>Player + All Servers</strong> → Welcome guide everyone sees everywhere</li>
- <li><strong>Player + Specific Server</strong> → Rules for one server only</li>
- <li><strong>Server Admin + All Servers</strong> → Company-wide admin procedures</li>
- <li><strong>Server Mod + Specific Server</strong> → Mod notes for one server</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.example_player_all') }}</strong> → {{ trans('server-documentation::strings.permission_guide.example_player_all_desc') }}</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.example_player_specific') }}</strong> → {{ trans('server-documentation::strings.permission_guide.example_player_specific_desc') }}</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.example_admin_all') }}</strong> → {{ trans('server-documentation::strings.permission_guide.example_admin_all_desc') }}</li>
+ <li><strong>{{ trans('server-documentation::strings.permission_guide.example_mod_specific') }}</strong> → {{ trans('server-documentation::strings.permission_guide.example_mod_specific_desc') }}</li>🤖 Prompt for AI Agents
In
`@server-documentation/resources/views/filament/partials/permission-guide.blade.php`
around lines 41 - 55, Replace the hardcoded English example text in the
permission-guide Blade view with translation keys to maintain i18n consistency:
replace the "On", "Off", "Examples:" label and each example sentence ("Welcome
guide everyone sees everywhere", "Rules for one server only", "Company-wide
admin procedures", "Mod notes for one server") with calls to the translation
helper (e.g., trans(...) or __(...)) using new or existing keys in your
server-documentation translation namespace so they match the surrounding
trans('server-documentation::strings...') usage; update the view strings in
permission-guide.blade.php (the list items and the <p> label) to use those
translation keys and add corresponding entries to the server-documentation
translation files.
| protected static function booted(): void | ||
| { | ||
| static::creating(function (Document $document) { | ||
| $document->uuid ??= Str::uuid()->toString(); | ||
| if ($document->slug === null) { | ||
| $document->slug = static::generateUniqueSlug($document->title); | ||
| } | ||
| if ($document->author_id === null && auth()->check()) { | ||
| $document->author_id = auth()->id(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Slug uniqueness issue has been addressed.
The previous concern about duplicate slug generation has been resolved with the generateUniqueSlug() method. However, there's a potential race condition: two concurrent requests creating documents with the same title could both pass the exists() check before either inserts, causing a constraint violation.
Consider wrapping slug generation in a database transaction with a retry mechanism, or handle the unique constraint exception gracefully.
🔧 Suggested improvement for race condition handling
protected static function booted(): void
{
static::creating(function (Document $document) {
$document->uuid ??= Str::uuid()->toString();
if ($document->slug === null) {
- $document->slug = static::generateUniqueSlug($document->title);
+ $document->slug = static::generateUniqueSlug($document->title, $document);
}
if ($document->author_id === null && auth()->check()) {
$document->author_id = auth()->id();
}
});Then update generateUniqueSlug to handle the race condition:
protected static function generateUniqueSlug(string $title, ?Document $document = null): string
{
$maxAttempts = 10;
$attempt = 0;
do {
$slug = Str::slug($title);
$originalSlug = $slug;
$counter = 1;
while (static::withTrashed()->where('slug', $slug)->exists()) {
$slug = $originalSlug . '-' . $counter++;
}
// Add random suffix on retry to avoid repeated collisions
if ($attempt > 0) {
$slug .= '-' . Str::random(4);
}
$attempt++;
} while ($attempt < $maxAttempts && static::withTrashed()->where('slug', $slug)->exists());
return $slug;
}Alternatively, catch the unique constraint exception in the model's save() or use database-level conflict resolution.
🤖 Prompt for AI Agents
In `@server-documentation/src/Models/Document.php` around lines 84 - 94, The
generateUniqueSlug() approach can still suffer a race: update Document::booted()
creating handler to retry slug creation on conflict by wrapping slug generation
and insert in a short loop/transaction and handling unique-constraint exceptions
from the DB; specifically, inside the static::creating(...) closure call
generateUniqueSlug($document->title) and attempt to persist, catching the unique
key exception (or using DB::transaction with SERIALIZABLE/isolation and retry up
to N times) and on retry call Document::generateUniqueSlug again (enhance
generateUniqueSlug to accept a retry attempt and append a short random suffix
when attempt>0); alternatively implement the catch-and-retry logic in
Document::save() to detect duplicate-slug SQL errors, regenerate the slug, and
retry the save a few times before bubbling the exception.
| /** | ||
| * Convert Markdown content to HTML. | ||
| * | ||
| * @param bool $sanitize Whether to sanitize the output HTML (default: true) | ||
| */ |
There was a problem hiding this comment.
Fix Pint phpdoc_align in this file.
CI reports phpdoc alignment issues; running Pint should resolve it.
🤖 Prompt for AI Agents
In `@server-documentation/src/Services/MarkdownConverter.php` around lines 43 -
47, The phpdoc block for the MarkdownConverter method (the "Convert Markdown
content to HTML." doc) is misaligned; run Pint or manually fix phpdoc_align by
aligning the asterisks and tag columns (e.g., `@param bool $sanitize` and the
description) to match project style; update the docblock in the
MarkdownConverter class/method so tags and descriptions are vertically aligned
and then run Pint to ensure no more phpdoc_align issues.
Filament v4 uses Filament\Actions\* for all action classes, not Filament\Tables\Actions\* which doesn't exist. Fixed imports in: - DocumentsRelationManager.php - DocumentResource.php - ViewDocumentVersions.php - ServersRelationManager.php Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Yesterday I was talking to a friend about exactly that :D Someone can read minds :) Thanks for the plugin :) |
You are most welcome. Its pretty close to ready. just working through the last of these CodeRabbit messages |
2 participants
Summary
A documentation management plugin for Pelican Panel that allows administrators to create, organize, and distribute documentation to server users with granular permission-based visibility.
Features
Security
Infrastructure
Test plan
php artisan test --filter=ServerDocumentationSummary by CodeRabbit
New Features
Permissions & Behavior
Documentation
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.