Feat/toggle backups enabled from index#369
Merged
David-Crty merged 6 commits intoJun 10, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a toggle control to enable or disable backups directly from the database server index view. A new Livewire method authorizes and flips the ChangesBackups Toggle from Index
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/Livewire/DatabaseServer/Show.php (1)
66-71: ⚡ Quick winConsider adding user feedback after toggling backups.
Other actions in this component emit success toast messages using the
Toasttrait (e.g.,success()at line 109). Adding a confirmation message after toggling backups would improve UX by confirming the action succeeded.💬 Suggested addition
public function toggleBackupsEnabled(): void { $this->authorize('viewForm', $this->server); $this->server->update(['backups_enabled' => ! $this->server->backups_enabled]); + + $this->success( + $this->server->backups_enabled + ? __('Backups enabled successfully!') + : __('Backups disabled successfully!') + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/DatabaseServer/Show.php` around lines 66 - 71, The toggleBackupsEnabled method currently flips backups_enabled but gives no user feedback; after the authorization and $this->server->update(...) call in toggleBackupsEnabled, call the component's Toast trait success() method to emit a confirmation message (e.g., "Backups enabled" or "Backups disabled" depending on new state) so users see a toast; locate the toggleBackupsEnabled function and use $this->server->backups_enabled to determine which message to pass to success().app/Livewire/DatabaseServer/Index.php (1)
163-170: ⚡ Quick winConsider adding user feedback after toggling backups.
Other actions in this component (e.g.,
delete()at line 111,clear()at line 68) emit success toast messages using theToasttrait. Adding a similar confirmation message after toggling backups would improve UX by confirming the action succeeded.💬 Suggested addition
public function toggleBackupsEnabled(string $id): void { $server = DatabaseServer::findOrFail($id); $this->authorize('viewForm', $server); $server->update(['backups_enabled' => ! $server->backups_enabled]); + + $this->success( + $server->backups_enabled + ? __('Backups enabled successfully!') + : __('Backups disabled successfully!') + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/DatabaseServer/Index.php` around lines 163 - 170, The toggleBackupsEnabled method currently flips the backups_enabled flag but gives no user feedback; update toggleBackupsEnabled (in the Livewire DatabaseServer Index component) to call the same Toast trait success emitter used by delete() and clear() after $server->update(...) to display a confirmation message (e.g., “Backups enabled” or “Backups disabled” based on the new state), keeping the authorization and update logic intact.tests/Feature/DatabaseServer/IndexTest.php (1)
110-119: 💤 Low valueConsider aligning test names with actual assertions.
Both test names reference "admin" but don't explicitly verify the user's role or the
viewFormpermission. While the tests exercise the toggle correctly, naming them more generically (e.g., "user can disable/enable backups from the index") would match the actual assertions. Alternatively, add an explicit role assertion if admin-specific behavior is intended.Also applies to: 121-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/IndexTest.php` around lines 110 - 119, The test names say "admin" but the tests only create a generic User and don't assert admin role or grant the viewForm permission; either rename the two tests (the ones invoking Livewire::actingAs($user)->test(Index::class)->call('toggleBackupsEnabled', $server->id)) to "user can disable/enable backups from the index" to match the assertions, or make them actually exercise admin-specific behavior by creating/granting an admin role or the viewForm permission on the User (e.g., create an admin user or attach the viewForm permission) before calling Index::class->toggleBackupsEnabled so the test name and behavior align.tests/Feature/DatabaseServer/ShowTest.php (1)
86-95: 💤 Low valueConsider aligning test names with actual assertions.
Both test names reference "admin" but don't explicitly verify the user's role or the
viewFormpermission. While the tests exercise the toggle correctly, naming them more generically (e.g., "user can disable/enable backups from the show page") would match the actual assertions. Alternatively, add an explicit role assertion if admin-specific behavior is intended.Also applies to: 97-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/ShowTest.php` around lines 86 - 95, The test names claim "admin" but the setup uses a generic User::factory()->create() and doesn't assert admin role or the viewForm permission; either rename the test descriptions to "user can disable/enable backups from the show page" to match the current assertions (tests using Show::class and calling toggleBackupsEnabled/toggleBackupsEnabled), or modify the setup to create/assign an admin user and assert the permission: e.g. create the user with admin credentials/role (replace User::factory()->create() with an admin factory/role assignment) and optionally assert the viewForm permission before calling Livewire::actingAs(...)->test(Show::class)->call('toggleBackupsEnabled') so the test name and behavior align (apply the same change to the paired test around 97-106).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Livewire/DatabaseServer/Index.php`:
- Around line 163-170: The toggleBackupsEnabled method currently flips the
backups_enabled flag but gives no user feedback; update toggleBackupsEnabled (in
the Livewire DatabaseServer Index component) to call the same Toast trait
success emitter used by delete() and clear() after $server->update(...) to
display a confirmation message (e.g., “Backups enabled” or “Backups disabled”
based on the new state), keeping the authorization and update logic intact.
In `@app/Livewire/DatabaseServer/Show.php`:
- Around line 66-71: The toggleBackupsEnabled method currently flips
backups_enabled but gives no user feedback; after the authorization and
$this->server->update(...) call in toggleBackupsEnabled, call the component's
Toast trait success() method to emit a confirmation message (e.g., "Backups
enabled" or "Backups disabled" depending on new state) so users see a toast;
locate the toggleBackupsEnabled function and use $this->server->backups_enabled
to determine which message to pass to success().
In `@tests/Feature/DatabaseServer/IndexTest.php`:
- Around line 110-119: The test names say "admin" but the tests only create a
generic User and don't assert admin role or grant the viewForm permission;
either rename the two tests (the ones invoking
Livewire::actingAs($user)->test(Index::class)->call('toggleBackupsEnabled',
$server->id)) to "user can disable/enable backups from the index" to match the
assertions, or make them actually exercise admin-specific behavior by
creating/granting an admin role or the viewForm permission on the User (e.g.,
create an admin user or attach the viewForm permission) before calling
Index::class->toggleBackupsEnabled so the test name and behavior align.
In `@tests/Feature/DatabaseServer/ShowTest.php`:
- Around line 86-95: The test names claim "admin" but the setup uses a generic
User::factory()->create() and doesn't assert admin role or the viewForm
permission; either rename the test descriptions to "user can disable/enable
backups from the show page" to match the current assertions (tests using
Show::class and calling toggleBackupsEnabled/toggleBackupsEnabled), or modify
the setup to create/assign an admin user and assert the permission: e.g. create
the user with admin credentials/role (replace User::factory()->create() with an
admin factory/role assignment) and optionally assert the viewForm permission
before calling
Livewire::actingAs(...)->test(Show::class)->call('toggleBackupsEnabled') so the
test name and behavior align (apply the same change to the paired test around
97-106).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51468386-8f76-43e8-b265-dd172bbbd1bd
📒 Files selected for processing (8)
app/Livewire/DatabaseServer/Index.phpapp/Livewire/DatabaseServer/Show.phplang/es.jsonlang/fr.jsonresources/views/livewire/database-server/index.blade.phpresources/views/livewire/database-server/show.blade.phptests/Feature/DatabaseServer/IndexTest.phptests/Feature/DatabaseServer/ShowTest.php
📜 Review details
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/database-server/show.blade.phpresources/views/livewire/database-server/index.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/database-server/show.blade.phpresources/views/livewire/database-server/index.blade.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
resources/views/livewire/database-server/show.blade.phptests/Feature/DatabaseServer/IndexTest.phpapp/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.phpresources/views/livewire/database-server/index.blade.phptests/Feature/DatabaseServer/ShowTest.php
📚 Learning: 2026-05-13T11:17:00.425Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 308
File: app/Livewire/Restore/Modal.php:353-363
Timestamp: 2026-05-13T11:17:00.425Z
Learning: In this project (David-Crty/databasement), when comparing a column to a value inside Eloquent/Laravel query closures or builder chains, keep the intentional pattern `whereRaw('column = ?', [$value])` instead of changing it to the idiomatic `->where('column', $value)`. This is used to avoid Larastan errors about unrecognized model properties on generic `Builder<Model>` closures. Do not suggest replacing `whereRaw` equality checks with `where()` in reviews if the existing `whereRaw('col = ?', [...])` form is being used for this purpose (PHPStan currently passes with 0 errors using this pattern).
Applied to files:
resources/views/livewire/database-server/show.blade.phptests/Feature/DatabaseServer/IndexTest.phpapp/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.phpresources/views/livewire/database-server/index.blade.phptests/Feature/DatabaseServer/ShowTest.php
📚 Learning: 2026-05-17T06:24:34.175Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 319
File: resources/views/livewire/dashboard/job-status-grid.blade.php:18-19
Timestamp: 2026-05-17T06:24:34.175Z
Learning: In this project’s Blade templates, do not recommend adding PHP null-safe `?->` operators to the `databaseServer` relation coming from `Snapshot` or to the `targetServer` relation coming from `Restore`. Both `snapshots.database_server_id` and `restores.target_server_id` are non-nullable `char(26)` columns with database-enforced cascade-on-delete foreign keys to `database_servers`, so the related servers are guaranteed by the DB once the parent relation is present. Only use `?->` on the parent relation itself (e.g., `$job->snapshot?->` or `$job->restore?->`), since a `BackupJob` is exclusively one of those. After null-guarding the parent, accessing `->databaseServer->name` or `->targetServer->name` directly is intentional and should not be changed.
Applied to files:
resources/views/livewire/database-server/show.blade.phpresources/views/livewire/database-server/index.blade.php
📚 Learning: 2026-05-17T06:30:13.129Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 319
File: resources/views/livewire/database-server/index.blade.php:134-140
Timestamp: 2026-05-17T06:30:13.129Z
Learning: In this repository’s Blade views, preserve the established Blade Icons convention for database backup/restore: use `bi.database-fill-up` for backup and `bi.database-fill-down` for restore (Blade Icons namespace), and do not flag or suggest replacing these with Heroicons when reviewing files such as Livewire database/snapshot/restore views. The project intentionally uses these `bi.*` glyphs because Heroicons does not provide an equivalent “database with directional arrow” icon, and Mary UI/Blade Icons supports multiple namespaces beyond Heroicons.
Applied to files:
resources/views/livewire/database-server/show.blade.phpresources/views/livewire/database-server/index.blade.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.phptests/Feature/DatabaseServer/ShowTest.php
📚 Learning: 2026-04-09T13:59:25.873Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: tests/Feature/ConfigurationTest.php:165-176
Timestamp: 2026-04-09T13:59:25.873Z
Learning: In this repository, `Notification::fake()` is already performed globally in the base test class (`tests/TestCase.php`) within `setUp()` before each test. When reviewing individual test files under `tests/`, do not flag missing `Notification::fake()` calls, since they are handled by the shared base test setup.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.phptests/Feature/DatabaseServer/ShowTest.php
📚 Learning: 2026-06-03T15:14:32.705Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 199
File: tests/Feature/Livewire/DatabaseServer/IndexTest.php:55-63
Timestamp: 2026-06-03T15:14:32.705Z
Learning: In David-Crty/databasement, `AppConfig` is DB-backed and declares `app.adminer_enabled` with a default of `false` via `AppConfigService::CONFIG`. When reviewing PHP tests that use database refresh/isolation between runs (so no prior `AppConfig::set()` can leak), do not flag tests that assert the “Adminer disabled” path for omitting an explicit `AppConfig::set('app.adminer_enabled', false)`. Adding that `set()` would be redundant scaffolding as the default is already deterministic under test isolation.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.phptests/Feature/DatabaseServer/ShowTest.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-13T11:17:00.425Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 308
File: app/Livewire/Restore/Modal.php:353-363
Timestamp: 2026-05-13T11:17:00.425Z
Learning: In the David-Crty/databasement repository, do not flag the use of explicit `Illuminate\Database\Eloquent\Builder $q` (e.g., `fn (Builder $q) => ...`) as the parameter type for query-constraint closures used in Eloquent query methods (including callbacks passed to `whereHas`, `when`, and `where*`). This explicit typehint is an intentional project-wide convention (used in `app/Queries/*` and Livewire components) to allow Larastan to type-check the closure body, so reviews should treat `Builder $q` typehints in these query closures as acceptable.
Applied to files:
app/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-17T06:21:44.287Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 319
File: app/Livewire/DatabaseServer/Show.php:37-38
Timestamp: 2026-05-17T06:21:44.287Z
Learning: When reviewing Livewire 4 components in this project, do not flag missing validation annotations (e.g., `#[Validate('boolean')]` / `$this->validate()`) for `public` PHP primitive-typed properties (e.g., `public bool $keepFiles`, `public int $limit`, `public string $name`) when Livewire hydrates them and the value is used only for a straightforward assignment into a simple model property. Livewire will coerce or reject the incoming value during hydration, so adding redundant validation would create review noise and can risk `ValidationException` regressions in destructive action handlers. Do flag validation/escaping when the property value is used in a riskier context (e.g., query/SQL construction, file/path handling, or shell/command execution), or when it is not the direct input for a simple model-property assignment.
Applied to files:
app/Livewire/DatabaseServer/Show.phpapp/Livewire/DatabaseServer/Index.php
🔇 Additional comments (4)
resources/views/livewire/database-server/index.blade.php (1)
153-160: LGTM!resources/views/livewire/database-server/show.blade.php (1)
99-107: LGTM!lang/es.json (1)
210-210: LGTM!Also applies to: 242-242
lang/fr.json (1)
210-210: LGTM!Also applies to: 242-242
- Replace viewForm with update authorization (viewForm allows demo users, which must not mutate backups_enabled) - Remove the toggle from the show page; keep it on the index only - Consolidate toggle tests and add demo-user authorization test
Owner
|
Pushed 8651572 with a few adjustments:
Thanks for the contribution! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Feature/DatabaseServer/IndexTest.php (1)
110-125: ⚡ Quick winConsider asserting the success toast message.
The test validates that
backups_enabledpersists correctly but doesn't verify that the success toast is displayed with the appropriate message ("Backups enabled successfully!" or "Backups disabled successfully!").✨ Optional enhancement to verify toast messages
test('user can toggle backups for a server from the index', function () { $user = User::factory()->create(); $server = DatabaseServer::factory()->create(['backups_enabled' => true]); Livewire::actingAs($user) ->test(Index::class) - ->call('toggleBackupsEnabled', $server->id); + ->call('toggleBackupsEnabled', $server->id) + ->assertDispatched('toast', title: 'Backups disabled successfully!'); expect($server->fresh()->backups_enabled)->toBeFalse(); Livewire::actingAs($user) ->test(Index::class) - ->call('toggleBackupsEnabled', $server->id); + ->call('toggleBackupsEnabled', $server->id) + ->assertDispatched('toast', title: 'Backups enabled successfully!'); expect($server->fresh()->backups_enabled)->toBeTrue(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/IndexTest.php` around lines 110 - 125, Extend the test 'user can toggle backups for a server from the index' to also assert the Livewire success toast messages after each toggle: when calling Index::toggleBackupsEnabled($server->id) assert the component emitted or session contains the expected success message string ("Backups disabled successfully!" after first call and "Backups enabled successfully!" after second call). Locate the test using the function name and the Livewire::test(Index::class)->call('toggleBackupsEnabled', ...) calls and add assertions for the emitted toast event or session/flash message that your Index component uses to show success toasts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Feature/DatabaseServer/IndexTest.php`:
- Around line 110-125: Extend the test 'user can toggle backups for a server
from the index' to also assert the Livewire success toast messages after each
toggle: when calling Index::toggleBackupsEnabled($server->id) assert the
component emitted or session contains the expected success message string
("Backups disabled successfully!" after first call and "Backups enabled
successfully!" after second call). Locate the test using the function name and
the Livewire::test(Index::class)->call('toggleBackupsEnabled', ...) calls and
add assertions for the emitted toast event or session/flash message that your
Index component uses to show success toasts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a423de58-b79a-458a-af0f-50b7629c7ca3
📒 Files selected for processing (3)
app/Livewire/DatabaseServer/Index.phpresources/views/livewire/database-server/index.blade.phptests/Feature/DatabaseServer/IndexTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/livewire/database-server/index.blade.php
📜 Review details
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.php
📚 Learning: 2026-04-09T13:59:25.873Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: tests/Feature/ConfigurationTest.php:165-176
Timestamp: 2026-04-09T13:59:25.873Z
Learning: In this repository, `Notification::fake()` is already performed globally in the base test class (`tests/TestCase.php`) within `setUp()` before each test. When reviewing individual test files under `tests/`, do not flag missing `Notification::fake()` calls, since they are handled by the shared base test setup.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.php
📚 Learning: 2026-06-03T15:14:32.705Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 199
File: tests/Feature/Livewire/DatabaseServer/IndexTest.php:55-63
Timestamp: 2026-06-03T15:14:32.705Z
Learning: In David-Crty/databasement, `AppConfig` is DB-backed and declares `app.adminer_enabled` with a default of `false` via `AppConfigService::CONFIG`. When reviewing PHP tests that use database refresh/isolation between runs (so no prior `AppConfig::set()` can leak), do not flag tests that assert the “Adminer disabled” path for omitting an explicit `AppConfig::set('app.adminer_enabled', false)`. Adding that `set()` would be redundant scaffolding as the default is already deterministic under test isolation.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
tests/Feature/DatabaseServer/IndexTest.phpapp/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-13T11:17:00.425Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 308
File: app/Livewire/Restore/Modal.php:353-363
Timestamp: 2026-05-13T11:17:00.425Z
Learning: In this project (David-Crty/databasement), when comparing a column to a value inside Eloquent/Laravel query closures or builder chains, keep the intentional pattern `whereRaw('column = ?', [$value])` instead of changing it to the idiomatic `->where('column', $value)`. This is used to avoid Larastan errors about unrecognized model properties on generic `Builder<Model>` closures. Do not suggest replacing `whereRaw` equality checks with `where()` in reviews if the existing `whereRaw('col = ?', [...])` form is being used for this purpose (PHPStan currently passes with 0 errors using this pattern).
Applied to files:
tests/Feature/DatabaseServer/IndexTest.phpapp/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-13T11:17:00.425Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 308
File: app/Livewire/Restore/Modal.php:353-363
Timestamp: 2026-05-13T11:17:00.425Z
Learning: In the David-Crty/databasement repository, do not flag the use of explicit `Illuminate\Database\Eloquent\Builder $q` (e.g., `fn (Builder $q) => ...`) as the parameter type for query-constraint closures used in Eloquent query methods (including callbacks passed to `whereHas`, `when`, and `where*`). This explicit typehint is an intentional project-wide convention (used in `app/Queries/*` and Livewire components) to allow Larastan to type-check the closure body, so reviews should treat `Builder $q` typehints in these query closures as acceptable.
Applied to files:
app/Livewire/DatabaseServer/Index.php
📚 Learning: 2026-05-17T06:21:44.287Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 319
File: app/Livewire/DatabaseServer/Show.php:37-38
Timestamp: 2026-05-17T06:21:44.287Z
Learning: When reviewing Livewire 4 components in this project, do not flag missing validation annotations (e.g., `#[Validate('boolean')]` / `$this->validate()`) for `public` PHP primitive-typed properties (e.g., `public bool $keepFiles`, `public int $limit`, `public string $name`) when Livewire hydrates them and the value is used only for a straightforward assignment into a simple model property. Livewire will coerce or reject the incoming value during hydration, so adding redundant validation would create review noise and can risk `ValidationException` regressions in destructive action handlers. Do flag validation/escaping when the property value is used in a riskier context (e.g., query/SQL construction, file/path handling, or shell/command execution), or when it is not the direct input for a simple model-property assignment.
Applied to files:
app/Livewire/DatabaseServer/Index.php
🔇 Additional comments (2)
app/Livewire/DatabaseServer/Index.php (1)
163-176: LGTM!tests/Feature/DatabaseServer/IndexTest.php (1)
110-125: Add/locate demo-user authorization coverage fortoggleBackupsEnabled
tests/Feature/DatabaseServer/IndexTest.phpalready checks that the toggle flipsbackups_enabled, but the “demo-user authorization test” mentioned in the commit message isn’t present intests/fortoggleBackupsEnabled+demo(search returned no matches). Add the demo-user authorization-failure assertion (or point to the existing test if it uses different terminology).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #369 +/- ##
=========================================
Coverage 89.89% 89.90%
- Complexity 2515 2517 +2
=========================================
Files 213 213
Lines 8886 8894 +8
=========================================
+ Hits 7988 7996 +8
Misses 898 898 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Thanks for the review and the fixes! |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #368 .
Adds an Enable Backup / Disable Backup toggle to the 3-dots actions menu on the Database Servers index and show page. When clicked:
backups_enabledflag is flipped immediately via atoggleBackupsEnabled()Livewire actionviewFormpolicy gateThe
backups_enabledcolumn already existed ondatabase_serversand was already reflected in the index view. No migration needed.Test plan
make test— 1034 passedmake phpstan— cleanmake lint-fix— cleanSummary by CodeRabbit
New Features
Internationalization
Tests