Phase 23 — Settings Page Minimal (CONV-364–380)#295
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Email is rendered as disabled/readonly input; saveProfile() never touches the email field, enforced by existing tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a complete user settings management system. Users can now manage their account profile and converter preferences through a dedicated settings page, with user-specific defaults automatically applied to converter options throughout the application. ChangesUser Account and Converter Preferences Settings
Sequence DiagramsequenceDiagram
participant User as Authenticated User
participant Form as AccountSettingsForm
participant Resolver as UserConverterDefaultsResolver
participant Dashboard as DashboardConverter
participant DB as User Model
User->>Form: access /settings
Form->>DB: mount() - load user data
DB-->>Form: name, email, settings
Form->>Form: initialize form fields with mount() data
Form-->>User: render account-settings-form template
User->>Form: update name and submit
Form->>Form: validate name (required)
Form->>DB: saveProfile() - persist trimmed name
Form->>User: dispatch settings-saved (profile)
Form-->>User: show "Profile settings saved"
User->>Dashboard: navigate to converter
Dashboard->>Resolver: initializeOptionsFromSchema()
Resolver->>DB: get user settings.conversion
Resolver->>Resolver: apply user preferences or config defaults
Resolver-->>Dashboard: resolved defaults with user overrides
Dashboard-->>User: render converter with user defaults
User->>Form: update imageQuality and removeMetadata
Form->>Form: validate imageQuality against allowed values
Form->>Form: validate removeMetadata as boolean
Form->>DB: saveConversionPreferences() - update settings.conversion
Form->>User: dispatch settings-saved (conversion)
Form-->>User: show "Conversion preferences saved"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/Feature/Converters/ConverterOptionsDefaultsTest.php (1)
52-67: ⚡ Quick winAdd a regression test for invalid stored
image_qualityfallback.There’s good coverage for missing preferences, but not for malformed persisted values. Add one case asserting invalid stored quality falls back to
converter.user_defaults.image_quality.Suggested test case
+it('falls back to system image quality when stored preference is invalid', function () { + $user = User::factory()->create([ + 'settings' => [ + 'conversion' => [ + 'image_quality' => 'ultra', + 'remove_metadata' => true, + ], + ], + ]); + + $schema = [ + ['key' => 'quality', 'type' => 'segmented', 'label' => 'Quality', 'default' => 'high', 'options' => []], + ['key' => 'remove_metadata', 'type' => 'toggle', 'label' => 'Remove metadata', 'default' => true], + ]; + + $defaults = app(UserConverterDefaultsResolver::class)->apply($user, $schema); + + expect($defaults['quality'])->toBe(config('converter.user_defaults.image_quality')); + expect($defaults['remove_metadata'])->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/Converters/ConverterOptionsDefaultsTest.php` around lines 52 - 67, Add a regression test to ensure malformed persisted image_quality falls back to the system default: in tests/Feature/Converters/ConverterOptionsDefaultsTest.php add a case similar to the existing "uses system defaults when user has no conversion preferences" but create the User with settings including an invalid 'image_quality' (e.g. 'image_quality' => 'invalid') and assert that UserConverterDefaultsResolver::apply($user, $schema) returns quality equal to config('converter.user_defaults.image_quality'); reference the resolver class name UserConverterDefaultsResolver and the schema keys 'quality' and 'remove_metadata' to mirror existing test structure.tests/Unit/Settings/UserConversionPreferencesTest.php (1)
5-18: ⚡ Quick winAssert concrete default values, not only key presence.
Current checks can miss contract drift (e.g., wrong default quality or metadata default). Consider asserting exact values and that the configured default is included in the allowed list.
Suggested patch
it('has default conversion preferences schema', function () { $defaults = config('converter.user_defaults'); - expect($defaults)->toHaveKey('image_quality'); - expect($defaults)->toHaveKey('remove_metadata'); + expect($defaults['image_quality'])->toBe('high'); + expect($defaults['remove_metadata'])->toBeTrue(); }); it('has allowed image quality values', function () { $allowed = config('converter.allowed_image_quality_values'); expect($allowed)->toContain('medium'); expect($allowed)->toContain('high'); expect($allowed)->toContain('best'); + expect($allowed)->toContain(config('converter.user_defaults.image_quality')); });🤖 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/Unit/Settings/UserConversionPreferencesTest.php` around lines 5 - 18, Update the tests to assert concrete default values instead of only key presence: in the "has default conversion preferences schema" test, assert that $defaults['image_quality'] equals the expected default string (e.g. 'medium' or whatever your canonical default is) and that $defaults['remove_metadata'] equals the expected boolean; in the "has allowed image quality values" test, assert that the configured default image quality (from config('converter.user_defaults.image_quality') or $defaults['image_quality']) is contained in $allowed in addition to checking the allowed set contains 'medium', 'high', 'best'. Ensure you reference the config keys 'converter.user_defaults' and 'converter.allowed_image_quality_values' and variables $defaults and $allowed when making these assertions.app/Support/Converters/UserConverterDefaultsResolver.php (1)
24-49: ⚡ Quick winNormalize persisted
image_qualityagainst allowed config values before applying defaults.Unsupported stored values currently flow straight into
quality, which can initialize options to an invalid state. Fallback toconverter.user_defaults.image_qualitywhen the stored value is not allowed.Suggested patch
- $imageQuality = data_get( + $imageQuality = data_get( $settings, 'conversion.image_quality', config('converter.user_defaults.image_quality') ); + $allowedImageQualities = config('converter.allowed_image_quality_values', []); + if (! is_string($imageQuality) || ! in_array($imageQuality, $allowedImageQualities, true)) { + $imageQuality = config('converter.user_defaults.image_quality'); + }🤖 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/Support/Converters/UserConverterDefaultsResolver.php` around lines 24 - 49, Persisted image_quality may contain values not allowed by config and currently gets assigned into $defaults['quality']; validate $imageQuality against the allowed set (e.g. config('converter.user_defaults.allowed_image_qualities') or similar constant) after you retrieve it and if not in that allowed array replace it with the canonical default (config('converter.user_defaults.image_quality')). Update the code around the $imageQuality variable and before you set $defaults['quality'] so that the normalized/validated value is used; reference the $imageQuality variable, the $defaults array, and the 'quality' schema key in UserConverterDefaultsResolver.php when making this change.tests/Feature/Livewire/AccountSettingsFormTest.php (1)
44-52: ⚡ Quick winAdd a whitespace-only name validation test.
Current coverage checks
''but not' ', which is the risky path with trim-before-save behavior.Test addition example
+it('rejects whitespace-only profile name', function () { + $user = User::factory()->create(); + + Livewire::actingAs($user) + ->test(AccountSettingsForm::class) + ->set('name', ' ') + ->call('saveProfile') + ->assertHasErrors(['name']); +});🤖 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/Livewire/AccountSettingsFormTest.php` around lines 44 - 52, Add a new test in AccountSettingsFormTest that mirrors the existing requires profile name test but sets the Livewire component's 'name' property to a whitespace-only string (e.g., ' ') before calling saveProfile and assertHasErrors(['name' => 'required']); this ensures AccountSettingsForm::class saveProfile trimming/validation is exercised for whitespace-only input.
🤖 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.
Inline comments:
In `@app/Livewire/AccountSettingsForm.php`:
- Around line 45-85: The Livewire methods saveProfile and
saveConversionPreferences contain domain logic and persistence that must be
moved into Actions; create two actions (e.g., UpdateUserProfileAction and
UpdateConversionPreferencesAction) that accept the validated payload and perform
trimming, applying settings, and saving the authenticated user (including
handling settings array/keys and validation rules like Rule::in for image
quality), then update the Livewire methods to only run validation, call the
corresponding Action (inject/resolve the action and pass the validated data),
set the component messages (profileSavedMessage/preferencesSavedMessage) and
dispatch('settings-saved', ...) — remove all direct forceFill/save and settings
mutation from the Livewire component so persistence lives in app/Actions/*.
- Around line 47-55: The current validation allows whitespace-only names because
'required' accepts " " and trim() then stores an empty string; update the
validation in AccountSettingsForm.php (the call to $this->validate() that
produces $validated) to reject whitespace-only input — e.g. add a rule for
'name' such as 'regex:/\S/' or 'not_regex:/^\s*$/' so pure-whitespace fails
validation, and keep using trim($validated['name']) when calling
$user->forceFill([...])->save() to persist the cleaned value.
In `@resources/views/livewire/account-settings-form.blade.php`:
- Around line 67-69: Replace the hardcoded <option> entries in
account-settings-form.blade.php with a loop that consumes a server-provided list
(e.g., $imageQualities or $imageQualityOptions) exposed by the Livewire
component; add a property or computed getter (e.g., imageQualities or
getImageQualityOptionsProperty) in the component class to return the allowed
value=>label pairs from the backend/config/validator, then in the Blade view
iterate over that array to render <option value="{{ $key }}">{{ $label
}}</option>; ensure the Livewire property name you choose matches the view
binding so UI and backend validation/config remain in sync.
---
Nitpick comments:
In `@app/Support/Converters/UserConverterDefaultsResolver.php`:
- Around line 24-49: Persisted image_quality may contain values not allowed by
config and currently gets assigned into $defaults['quality']; validate
$imageQuality against the allowed set (e.g.
config('converter.user_defaults.allowed_image_qualities') or similar constant)
after you retrieve it and if not in that allowed array replace it with the
canonical default (config('converter.user_defaults.image_quality')). Update the
code around the $imageQuality variable and before you set $defaults['quality']
so that the normalized/validated value is used; reference the $imageQuality
variable, the $defaults array, and the 'quality' schema key in
UserConverterDefaultsResolver.php when making this change.
In `@tests/Feature/Converters/ConverterOptionsDefaultsTest.php`:
- Around line 52-67: Add a regression test to ensure malformed persisted
image_quality falls back to the system default: in
tests/Feature/Converters/ConverterOptionsDefaultsTest.php add a case similar to
the existing "uses system defaults when user has no conversion preferences" but
create the User with settings including an invalid 'image_quality' (e.g.
'image_quality' => 'invalid') and assert that
UserConverterDefaultsResolver::apply($user, $schema) returns quality equal to
config('converter.user_defaults.image_quality'); reference the resolver class
name UserConverterDefaultsResolver and the schema keys 'quality' and
'remove_metadata' to mirror existing test structure.
In `@tests/Feature/Livewire/AccountSettingsFormTest.php`:
- Around line 44-52: Add a new test in AccountSettingsFormTest that mirrors the
existing requires profile name test but sets the Livewire component's 'name'
property to a whitespace-only string (e.g., ' ') before calling saveProfile
and assertHasErrors(['name' => 'required']); this ensures
AccountSettingsForm::class saveProfile trimming/validation is exercised for
whitespace-only input.
In `@tests/Unit/Settings/UserConversionPreferencesTest.php`:
- Around line 5-18: Update the tests to assert concrete default values instead
of only key presence: in the "has default conversion preferences schema" test,
assert that $defaults['image_quality'] equals the expected default string (e.g.
'medium' or whatever your canonical default is) and that
$defaults['remove_metadata'] equals the expected boolean; in the "has allowed
image quality values" test, assert that the configured default image quality
(from config('converter.user_defaults.image_quality') or
$defaults['image_quality']) is contained in $allowed in addition to checking the
allowed set contains 'medium', 'high', 'best'. Ensure you reference the config
keys 'converter.user_defaults' and 'converter.allowed_image_quality_values' and
variables $defaults and $allowed when making these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68e0f625-7982-49cb-9487-a306fa0c08fb
📒 Files selected for processing (11)
app/Livewire/AccountSettingsForm.phpapp/Livewire/Dashboard/DashboardConverter.phpapp/Support/Converters/UserConverterDefaultsResolver.phpconfig/converter.phpresources/views/livewire/account-settings-form.blade.phpresources/views/settings/index.blade.phproutes/web.phptests/Feature/Converters/ConverterOptionsDefaultsTest.phptests/Feature/Livewire/AccountSettingsFormTest.phptests/Feature/Settings/SettingsPageTest.phptests/Unit/Settings/UserConversionPreferencesTest.php
- Extract persistence into UpdateProfileNameAction and UpdateConversionPreferencesAction; Livewire component now only validates, calls action, and updates UI state - Reject whitespace-only names with not_regex:/^\s*$/ rule (required passes " ") - Expose imageQualityOptions() from component; view iterates instead of hardcoding <option> tags - Guard invalid persisted image_quality in UserConverterDefaultsResolver against config drift - Add tests: whitespace-only name rejection, invalid persisted quality fallback, concrete value assertions in unit tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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
/settingspage with profile name editing and read-only email displayusers.settingsJSONDashboardConverterviaUserConverterDefaultsResolverso converter forms open with the user's saved defaultsAccountSettingsFormScope (Phase 23 only)
This PR deliberately excludes: password change, email change, 2FA, billing settings, API key management, device/session management.
Test plan
/settings— redirected to/login/settingsmedium/high/best)users.settings['conversion']composer testpasses (695 tests)composer lintpassesnpm run buildpasses🤖 Generated with Claude Code
Summary by cubic
Adds a minimal /settings page for authenticated users to edit display name and set default conversion preferences that pre-fill converter forms; email is read-only. Completes Phase 23 (CONV-364–380) with validation hardening and cleaner persistence.
New Features
App\Support\Converters\UserConverterDefaultsResolverconverter.user_defaultsandallowed_image_quality_valuesRefactors
UpdateProfileNameActionandUpdateConversionPreferencesActionimage_qualityvalues to system default in resolverWritten for commit 61ea415. Summary will update on new commits.
Summary by CodeRabbit
Release Notes