feat(settings): add adaptive cover display preferences#169
Merged
neonsolstice merged 1 commit intoMay 31, 2026
Merged
Conversation
Allow users to choose how cover artwork is fitted so mixed aspect ratios render cleanly in cards, lists, carousels, and widgets. Persist display preferences through shared types and user-preferences endpoints so these settings stay consistent across sessions and devices. Closes bookorbit#152
📝 WalkthroughWalkthroughThis PR introduces a centralized display preferences system enabling multiple book cover display modes. It adds server synchronization for display settings, a new ChangesDisplay Settings & Cover Rendering Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
|
github-actions
Bot
added
server
![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 (1)
client/src/features/settings/AppearanceLayoutSettings.vue (1)
52-67: 💤 Low valueConsider
aria-pressedon the segmented toggle buttons.The scope (
synced/per-view) and shape (circle/square) selectors are styled toggle buttons but don't expose their selected state to assistive tech. Adding:aria-pressed="coverSizeScope === 'synced'"(and equivalents) conveys the active option.Also applies to: 206-221
🤖 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 `@client/src/features/settings/AppearanceLayoutSettings.vue` around lines 52 - 67, The segmented toggle buttons (the cover size scope buttons using coverSizeScope and setCoverSizeScope) do not expose their selected state to assistive tech; update each option button to include an aria-pressed binding (e.g., :aria-pressed="coverSizeScope === 'synced'" for the synced button and analogous for 'per-view') so the active state is conveyed; apply the same pattern to the shape selector buttons referenced by the shape state (the circle/square controls) so each button binds aria-pressed to its corresponding comparison of the shape state.
🤖 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 `@client/src/features/settings/__tests__/AppearanceSettings.spec.ts`:
- Around line 20-53: The mock objects are inconsistent: displaySnapshot includes
gridGap, viewMode, and tableDensity but displayRefs omits them, which can break
tests vs production for the useDisplaySettings composable; update displayRefs to
include refs for gridGap (ref(28)), viewMode (ref('grid')), and tableDensity
(ref('comfortable')) so the mock return shape matches displaySnapshot and the
real composable (check the displaySnapshot and displayRefs constants and the
useDisplaySettings mock).
In `@client/src/features/settings/AppearanceLayoutSettings.vue`:
- Around line 83-92: The range sliders lack programmatic labels; for each slider
(e.g., the portrait cover size slider bound to portraitCoverSize with
`@input`="handlePortraitCoverSizeInput" and :disabled="!syncModeEnabled") add an
accessible name by either giving the input an id and pairing it with the
existing visible text via a <label for="..."> or by adding an appropriate
aria-label/aria-labelledby value that references the visible heading. Repeat the
same pattern for the other four sliders (square cover size, portrait grid gap,
square grid gap, author cover size) so every <input type="range"> has a
descriptive accessible label tied to its visible text.
In `@client/src/features/settings/AppearancePreferenceStorage.vue`:
- Around line 91-122: The two storage-mode card DIVs (the one using syncEnabled
and the one using isDemoRestrictedAccount) are not keyboard accessible; replace
each outer <div> with a semantic <button type="button"> that preserves the
existing :class and content and keeps the `@click` handlers (handleDeviceMode and
handleAccountMode) so keyboard/Enter/Space work; for the account card bind
:disabled="isDemoRestrictedAccount" so it becomes non-interactive when
restricted and adjust the class logic to account for the disabled state (keep
visual styles like cursor-not-allowed and opacity via classes when
isDemoRestrictedAccount is true), ensuring focus styles remain available and no
additional click handling fires when disabled.
---
Nitpick comments:
In `@client/src/features/settings/AppearanceLayoutSettings.vue`:
- Around line 52-67: The segmented toggle buttons (the cover size scope buttons
using coverSizeScope and setCoverSizeScope) do not expose their selected state
to assistive tech; update each option button to include an aria-pressed binding
(e.g., :aria-pressed="coverSizeScope === 'synced'" for the synced button and
analogous for 'per-view') so the active state is conveyed; apply the same
pattern to the shape selector buttons referenced by the shape state (the
circle/square controls) so each button binds aria-pressed to its corresponding
comparison of the shape state.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc81b24f-7ef8-4a63-a39d-2589e921005a
📒 Files selected for processing (44)
client/src/assets/theme/cover-effects.cssclient/src/composables/__tests__/useDisplaySettings.spec.tsclient/src/composables/__tests__/useDisplaySettingsSync.spec.tsclient/src/composables/useDisplaySettings.tsclient/src/composables/useDisplaySettingsSync.tsclient/src/features/auth/composables/useAuth.tsclient/src/features/book/components/BookCoverArtwork.vueclient/src/features/book/components/BookCoverCard.test.tsclient/src/features/book/components/BookCoverCard.vueclient/src/features/book/components/BookCoverSurface.vueclient/src/features/book/components/BookListRow.vueclient/src/features/book/components/BookQuickView.vueclient/src/features/book/components/__tests__/BookCoverArtwork.spec.tsclient/src/features/book/components/__tests__/BookCoverSurface.spec.tsclient/src/features/book/components/__tests__/BookQuickView.spec.tsclient/src/features/book/components/detail/BookCarousel.test.tsclient/src/features/book/components/detail/BookCarousel.vueclient/src/features/book/components/detail/tabs/DetailsTab.vueclient/src/features/book/components/detail/tabs/__tests__/DetailsTab.spec.tsclient/src/features/book/components/table/BookTableCoverCell.vueclient/src/features/book/lib/cover-aspect-ratio.test.tsclient/src/features/book/lib/cover-aspect-ratio.tsclient/src/features/dashboard/components/widgets/CurrentlyReadingWidget.vueclient/src/features/dashboard/components/widgets/HighlightOfTheDayWidget.vueclient/src/features/dashboard/components/widgets/LongWaitWidget.vueclient/src/features/dashboard/components/widgets/NeglectedGemsWidget.vueclient/src/features/settings/AppearanceBehaviorSettings.vueclient/src/features/settings/AppearanceBookCoverSettings.vueclient/src/features/settings/AppearanceLayoutSettings.vueclient/src/features/settings/AppearancePreferenceStorage.vueclient/src/features/settings/AppearanceSettings.vueclient/src/features/settings/AppearanceThemeSettings.vueclient/src/features/settings/SettingsHeader.vueclient/src/features/settings/__tests__/AppearanceSettings.spec.tsclient/src/features/settings/__tests__/SettingsHeader.spec.tsclient/src/features/settings/lib/__tests__/appearance-tabs.spec.tsclient/src/features/settings/lib/appearance-tabs.tsclient/src/router/index.tspackages/types/src/display-preferences.tspackages/types/src/index.tsserver/src/modules/user-preferences/user-preferences.controller.test.tsserver/src/modules/user-preferences/user-preferences.controller.tsserver/src/modules/user-preferences/user-preferences.service.test.tsserver/src/modules/user-preferences/user-preferences.service.ts
Comment on lines
+20
to
+53
| const displaySnapshot = { | ||
| portraitCoverSize: 180, | ||
| squareCoverSize: 180, | ||
| coverSizeScope: 'per-view', | ||
| gridGap: 28, | ||
| portraitGridGap: 12, | ||
| squareGridGap: 12, | ||
| viewMode: 'grid', | ||
| cardOverlays: [], | ||
| smartScopeFilterExpanded: false, | ||
| authorCoverSize: 120, | ||
| authorCoverShape: 'circle', | ||
| tableZebraStriping: true, | ||
| tableDensity: 'comfortable', | ||
| bookSpineOverlay: 'off', | ||
| bookShadowStrength: 'default', | ||
| bookCoverDisplayMode: 'blurred-fit', | ||
| } | ||
|
|
||
| const displayRefs = { | ||
| portraitCoverSize: ref(180), | ||
| squareCoverSize: ref(180), | ||
| coverSizeScope: ref('per-view'), | ||
| portraitGridGap: ref(12), | ||
| squareGridGap: ref(12), | ||
| cardOverlays: ref([]), | ||
| smartScopeFilterExpanded: ref(false), | ||
| authorCoverSize: ref(120), | ||
| authorCoverShape: ref('circle'), | ||
| tableZebraStriping: ref(true), | ||
| bookSpineOverlay: ref('off'), | ||
| bookShadowStrength: ref('default'), | ||
| bookCoverDisplayMode: ref('blurred-fit'), | ||
| } |
There was a problem hiding this comment.
Verify consistency between displaySnapshot and displayRefs.
The displaySnapshot object (lines 20-37) includes gridGap, viewMode, and tableDensity, but the displayRefs object (lines 39-53) does not include refs for these three fields. If the production useDisplaySettings composable exposes these fields, tests may pass while production code fails when accessing displayRefs.gridGap, displayRefs.viewMode, or displayRefs.tableDensity.
Ensure the mock's return value matches the actual composable's interface.
🤖 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 `@client/src/features/settings/__tests__/AppearanceSettings.spec.ts` around
lines 20 - 53, The mock objects are inconsistent: displaySnapshot includes
gridGap, viewMode, and tableDensity but displayRefs omits them, which can break
tests vs production for the useDisplaySettings composable; update displayRefs to
include refs for gridGap (ref(28)), viewMode (ref('grid')), and tableDensity
(ref('comfortable')) so the mock return shape matches displaySnapshot and the
real composable (check the displaySnapshot and displayRefs constants and the
useDisplaySettings mock).
Comment on lines
+83
to
+92
| <input | ||
| :value="portraitCoverSize" | ||
| type="range" | ||
| min="100" | ||
| max="280" | ||
| step="10" | ||
| class="w-full accent-primary cursor-pointer" | ||
| :disabled="!syncModeEnabled" | ||
| @input="handlePortraitCoverSizeInput" | ||
| /> |
There was a problem hiding this comment.
Range sliders lack an accessible name.
The <input type="range"> controls have no programmatically associated label (no aria-label, and the visible "Cover size"/heading text lives in sibling <div>s without for/id linkage). Screen-reader users will hear an unnamed "slider". This applies to all five sliders in this file (portrait/square cover size, portrait/square grid gap, author cover size).
♿ Proposed fix (apply the same pattern to each slider)
<input
:value="portraitCoverSize"
type="range"
+ aria-label="Portrait cover size"
min="100"
max="280"
step="10"
class="w-full accent-primary cursor-pointer"
:disabled="!syncModeEnabled"
`@input`="handlePortraitCoverSizeInput"
/>📝 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.
Suggested change
| <input | |
| :value="portraitCoverSize" | |
| type="range" | |
| min="100" | |
| max="280" | |
| step="10" | |
| class="w-full accent-primary cursor-pointer" | |
| :disabled="!syncModeEnabled" | |
| @input="handlePortraitCoverSizeInput" | |
| /> | |
| <input | |
| :value="portraitCoverSize" | |
| type="range" | |
| aria-label="Portrait cover size" | |
| min="100" | |
| max="280" | |
| step="10" | |
| class="w-full accent-primary cursor-pointer" | |
| :disabled="!syncModeEnabled" | |
| `@input`="handlePortraitCoverSizeInput" | |
| /> |
🤖 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 `@client/src/features/settings/AppearanceLayoutSettings.vue` around lines 83 -
92, The range sliders lack programmatic labels; for each slider (e.g., the
portrait cover size slider bound to portraitCoverSize with
`@input`="handlePortraitCoverSizeInput" and :disabled="!syncModeEnabled") add an
accessible name by either giving the input an id and pairing it with the
existing visible text via a <label for="..."> or by adding an appropriate
aria-label/aria-labelledby value that references the visible heading. Repeat the
same pattern for the other four sliders (square cover size, portrait grid gap,
square grid gap, author cover size) so every <input type="range"> has a
descriptive accessible label tied to its visible text.
Comment on lines
+91
to
+122
| <div | ||
| class="flex items-start gap-4 px-4 py-3.5 md:px-5 md:py-4 rounded-lg border-2 cursor-pointer transition-colors" | ||
| :class="!syncEnabled ? 'border-primary bg-primary/5' : 'border-border bg-card hover:border-muted-foreground/30'" | ||
| @click="handleDeviceMode" | ||
| > | ||
| <div | ||
| class="mt-0.5 flex items-center justify-center w-8 h-8 rounded-lg shrink-0 transition-colors" | ||
| :class="!syncEnabled ? 'bg-primary/15 text-primary' : 'bg-muted text-muted-foreground'" | ||
| > | ||
| <Monitor :size="16" /> | ||
| </div> | ||
| <div class="flex-1 min-w-0"> | ||
| <div class="flex items-center gap-2 mb-1"> | ||
| <span class="settings-label">This device only</span> | ||
| <span v-if="!syncEnabled" class="text-xs font-semibold uppercase tracking-wide px-1.5 py-0.5 rounded bg-primary/15 text-primary"> | ||
| Active | ||
| </span> | ||
| </div> | ||
| <span class="block text-xs text-muted-foreground leading-relaxed"> | ||
| Preferences stay in your browser. Best if you want a different look on different devices. | ||
| </span> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div | ||
| class="flex items-start gap-4 px-4 py-3.5 md:px-5 md:py-4 rounded-lg border-2 transition-colors" | ||
| :class="[ | ||
| isDemoRestrictedAccount ? 'border-border bg-card opacity-50 cursor-not-allowed' : 'cursor-pointer', | ||
| !isDemoRestrictedAccount && (syncEnabled ? 'border-primary bg-primary/5' : 'border-border bg-card hover:border-muted-foreground/30'), | ||
| ]" | ||
| @click="handleAccountMode" | ||
| > |
There was a problem hiding this comment.
Mode selection cards are not keyboard operable.
Both storage-mode options are plain <div>s with @click; they have no role, tabindex, or keydown handler, so keyboard-only and screen-reader users cannot select a storage mode. This blocks task completion for those users. Also, the "My account" card still fires handleAccountMode on click when isDemoRestrictedAccount (only styled cursor-not-allowed).
Prefer native <button type="button"> (with :disabled for the demo-restricted case), which gives focus, Enter/Space activation, and disabled semantics for free.
♿ Sketch: device-mode card as a button
- <div
+ <button
+ type="button"
class="flex items-start gap-4 px-4 py-3.5 md:px-5 md:py-4 rounded-lg border-2 cursor-pointer transition-colors"
:class="!syncEnabled ? 'border-primary bg-primary/5' : 'border-border bg-card hover:border-muted-foreground/30'"
`@click`="handleDeviceMode"
>
...
- </div>
+ </button>Apply the same for the account card, adding :disabled="isDemoRestrictedAccount" so it is non-interactive when restricted.
🤖 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 `@client/src/features/settings/AppearancePreferenceStorage.vue` around lines 91
- 122, The two storage-mode card DIVs (the one using syncEnabled and the one
using isDemoRestrictedAccount) are not keyboard accessible; replace each outer
<div> with a semantic <button type="button"> that preserves the existing :class
and content and keeps the `@click` handlers (handleDeviceMode and
handleAccountMode) so keyboard/Enter/Space work; for the account card bind
:disabled="isDemoRestrictedAccount" so it becomes non-interactive when
restricted and adjust the class logic to account for the disabled state (keep
visual styles like cursor-not-allowed and opacity via classes when
isDemoRestrictedAccount is true), ensuring focus styles remain available and no
additional click handling fires when disabled.
This was referenced Jun 1, 2026
Closed
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
github-actions
Bot
added
the
released
11 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.
What does this PR do?
Adds adaptive cover display preferences to cleanly render book cover artwork of mixed aspect ratios across all views.
This PR includes:
AppearanceSettings.vueinto subtabs (AppearanceThemeSettings,AppearanceBookCoverSettings,AppearanceLayoutSettings,AppearanceBehaviorSettings,AppearancePreferenceStorage).useDisplaySettings(local state persistence) anduseDisplaySettingsSync(bi-directional preference syncing with server).BookCoverArtworkcomponent that supportsblurred-fit,fill-crop, andnatural-bottomdisplay modes, replacing ad-hoc cover display handling across cards, lists, tables, carousels, and widgets./api/v1/user-preferences/display.Closes #152
How did you test this?
pnpm run verify:fastlint:check+typecheck)pnpm run testScreenshots
Anything non-obvious in the diff?
blurred-fitmode draws a blurred background panel under the aspect-ratio-locked book cover to seamlessly avoid empty margins.Checklist
.env, personal configs)Summary by CodeRabbit
Release Notes
New Features
Improvements