Add learner-defined custom themes to the EPUB reader

[rtibbles]

Summary

Brings the EPUB reader's custom themes feature — originally a GSoC contribution — to merge-readiness on the current codebase. Alongside the six fixed themes, learners can create, edit, and delete their own themes (background, text, and link colors), picked via a color picker and persisted per-device in localStorage.

The feature was functional in the GSoC branch; this branch modernises and hardens it.

State	Screenshot
Add / edit theme modal
Color picker
Custom theme applied (My themes list)
Dark fixed theme — accessible link color

References

Continues the custom-themes work from the GSoC-era epub-custom-themes branch (merge base ~July 2023). No tracking issue was filed for the original work.

Reviewer guidance

How to test: Open any EPUB and open the reader settings (sliders icon). Under Themes, switch the six fixed themes — check link contrast on the dark ones (Black / Grey / Yellow). Under My themes (signed in), add a theme: name it, pick background/text/link colors, save; confirm it applies, appears in the list, and that rename/edit and delete work.

Deliberate convention departure: the six fixed themes are hardcoded hex in EpubConstants.js rather than $themeTokens/$themePalette, so EPUB reading palettes don't shift with Kolibri's app theme. Intentional, but it diverges from the "use theme tokens" guideline and is worth a deliberate decision.

One maintenance note: the alwan accessibility patch in ColorPickerModal.vue keys off the library's internal class/attribute names, so an alwan upgrade could silently turn it into a no-op.

AI usage

Used Claude Code throughout. It reviewed the GSoC-derived branch and, under my direction, modernised and hardened it — converting the components from Options (and mixed) API to Composition API, extracting persistence into the useCustomThemes composable, and adding stable theme ids, accessible link colors, CSS-ellipsis names, an alwan accessibility patch, and the test suite — then drove a headless browser to verify rendering and run axe audits on the new UI. I made the design decisions (theme identity, hardcoded vs token colors, auditing color-picker alternatives before keeping alwan) and critically reviewed and revised its output.

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.5.dev184_gd45132331.d20260613.pex
Windows Installer (EXE)	kolibri-0.19.5.dev184+gd45132331.d20260613-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.5.dev184+gd45132331-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.5.dev184+gd45132331.d20260613.dmg
Android Package (APK)	kolibri-0.19.5.dev184+gd45132331-0.2.0-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.5.dev184+gd45132331.zip
TAR file	kolibri-0.19.5.dev184+gd45132331.d20260613.tar.gz
WHL file	kolibri-0.19.5.dev184+gd45132331.d20260613-py3-none-any.whl

Smoke test screenshot

Download screenshot

[rtibblesbot]

Clean, well-tested implementation of learner-defined EPUB themes. The composable design — stable UUID identity, localStorage via Lockr, isDuplicateName with self-exclusion in edit mode — is solid, and resolveTheme's two-phase backward-compatibility resolution (by id, then by name for pre-id snapshots) shows careful thought. Test coverage is thorough across all new files, with tests driving actual localStorage (not mocked) and using real translated strings.

Three substantive concerns are noted below as suggestions. CI is passing. Visual inspection ran from the four screenshots in the PR: the add modal, color picker, applied custom theme with truncated name, and dark fixed theme with accessible link color all look correct and complete.

frontend review: The custom-themes feature is well-structured overall: composable design is clean, resolveTheme handles backward compatibility gracefully, all new components have tests using Vue Testing Library with proper role/label queries, and the proactive Alwan accessibility patching shows good defensive a11y instinct. A few frontend-convention issues are worth addressing before merge.

Two important findings: AddEditCustomThemeModal uses float: left for its three-column color-picker layout (project convention is KGrid/flex, and float has RTL implications even with RTLCSS auto-flip), and SettingsSideBar adds three new reactive variables via data() despite the PR simultaneously introducing setup() — per AGENTS.md the project discourages propagating Options API state to new code, and refs in setup() are the natural fit here since the composable returns are already in setup() and Vue auto-unwraps refs on this so all the existing this.* method accesses work without changes.

Several suggestions around ColorPickerModal: the alwan instance is not destroyed on unmount (low risk since the DOM element is removed, but worth adding alwan.destroy() in onUnmounted for hygiene), the accessibility patch runs document.querySelectorAll('svg[aria-role]') globally rather than scoped to the modal's DOM node, and the #color-picker hardcoded id would be cleaner as a Vue ref. The default theme name myTheme${index} is user-visible and not translated.

[praise] Praise: comprehensive test coverage with idiomatic Vue Testing Library usage

All five new components and the composable have dedicated spec files. Tests query by role and aria-label (not component internals), mock only at the hard boundary (Alwan / useUser), and cover edge cases like the duplicate-name exclusion in edit mode, empty-state focus management, and the Alwan hex-extraction behaviour. The useCustomThemes suite in particular tests all four CRUD operations plus the persistence round-trip through Lockr.

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran a phased review pipeline over the pull request diff:

• Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
• Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
• Specialized frontend/backend review passes applied framework-specific lenses where those files changed
• For UI changes: manual QA and an accessibility audit against a live dev server, when available
• Discarded findings citing files outside the diff; checked CI status and linked issue acceptance criteria
• Calibrated the final verdict deterministically from finding severities, CI status, and QA evidence

[rtibblesbot]

[suggestion] Sign-in gate on a localStorage-only operation is inconsistent

handleSubmit blocks saving unless isUserLoggedIn.value is true, but themes are persisted to device localStorage — no server call, no authentication needed. At the same time, logged-out users can see and apply any custom themes already in storage; only creation is gated. The UX gap is: the "Add new theme" button is visible, the user fills the form, then is told to sign in.

If the intent is that custom themes belong to a signed-in identity, the button should be hidden or disabled (with a tooltip) for logged-out users so the constraint is communicated before they do work. If anonymous creation is actually fine (localStorage is device-local), remove the isUserLoggedIn guard and the signIn alert string. As written, the feature is confusing in both directions.

[rtibbles]

Removed the guard entirely — dropped isUserLoggedIn/useUser, the UiAlert, and the signIn string. Custom themes are device-local localStorage, so anonymous creation is fine.

[rtibblesbot]

[suggestion] Newly added state and methods in SettingsSideBar use Options API while setup() is already there

The PR adds setup() to SettingsSideBar for the composable, but the new local state (themeToDelete, editingTheme, newThemeName) and the new methods (createTheme, updateTheme, deleteTheme, startEditCustomTheme, cancelAdd, cancelEdit, generateNewThemeName) are written in data() / methods:. The AGENTS.md guideline is "Composition API, not Options API" for new code. Since you're already touching this file, moving those ref(null) state variables and their methods into setup() would complete the migration and keep the component consistent. ($trs and the existing computed: block can stay for now.)

[rtibbles]

Done — themeToDelete/editingTheme/newThemeName are now ref()s in setup() and the data() block is gone. The methods: are untouched since Vue auto-unwraps refs on this.

[rtibblesbot]

[suggestion] Hardcoded #cccccc border colors in AddEditCustomThemeModal styles

.theme-preview (line 298) and .theme-color-button (line 317) both use border: 1px solid #cccccc. AGENTS.md: "Never use raw color values." These are UI chrome inside the modal, not EPUB-reader content, so the deliberate-hardcoding rationale from the PR description doesn't apply. Use $themeTokens.fineLine (already used elsewhere in EpubRendererIndex) so the border adapts if the host Kolibri theme changes.

[rtibbles]

Done — both borders now use $themeTokens.fineLine, bound inline the way EpubRendererIndex already does it (preview via :style, swatches via a borderColor arg on themeColorOptionStyles).

[rtibblesbot]

[nitpick] UiAlert inline style should move to the style block

:style="{ margin: '8px 24px 12px 24px', width: 'auto' }" is static and non-directional — nothing here needs to be inline. AGENTS.md: "Non-dynamic styles go in <style> blocks." Extract to a CSS class.

[rtibbles]

Moot now — the UiAlert was removed entirely along with the sign-in guard (see the localStorage thread).

[rtibblesbot]

[nitpick] border-color: black !important on Delete/Edit buttons

.theme-settings-button forces border-color: black !important. These are sidebar control buttons, not EPUB-content elements, so the "hardcoded EPUB colors" rationale doesn't apply. $themeTokens.text (or $themePalette.grey.v_700) would give a dark border that adapts correctly. !important to override KButton is also fragile against future KDS updates.

[rtibbles]

Went further on this one — dropped the custom border entirely rather than tokenising it. Edit/Delete are now stock flat KButtons (no .theme-settings-button, no !important), with Edit as the primary action and ordered before Delete. The row is now a flex layout so the buttons align with the swatch on all sizes.

[rtibblesbot]

[suggestion] patchAlwanAccessibility patches the entire document, not just this modal

document.querySelectorAll('svg[aria-role]') runs against the whole document, which could inadvertently patch SVGs outside this modal if any other component happened to use the (invalid) aria-role attribute. Scope the query to the modal's root element instead. In setup(), create a ref for the modal container and pass it into patchAlwanAccessibility:

function patchAlwanAccessibility(root) {
  nextTick(() => {
    const palette = root.querySelector('.alwan__palette');
    ...
    root.querySelectorAll('svg[aria-role]').forEach(...);
  });
}

[rtibbles]

Done — patchAlwanAccessibility(root) now takes a pickerRoot template ref and queries within it (root.querySelectorAll('svg[aria-role]')) instead of document.

[rtibblesbot]

[nitpick] Hardcoded id='color-picker' should be a Vue ref

Using a hardcoded id to mount Alwan (new Alwan('#color-picker', ...)) requires a globally-unique DOM id. While the v-if guard prevents two instances from being mounted simultaneously, the idiomatic Vue pattern is to use a template ref and pass the element directly to Alwan:

<div ref="colorPickerEl"></div>

new Alwan(colorPickerEl.value, { ... })

This also removes the need for the scoped <div id="color-picker"> and makes the intent clear.

[rtibbles]

Done — replaced the hardcoded id with colorPickerEl/pickerBox template refs; Alwan is constructed with the elements directly (new Alwan(colorPickerEl.value, { target: pickerBox.value, ... })).

[rtibblesbot]

[nitpick] Test queries preview element by CSS class name

container.querySelector('.theme-preview') couples the test to an implementation detail (the CSS class name). The preview <div> has no semantic role, but adding role="img" and aria-label would make it queryable via screen.getByRole('img', { name: customThemePreview$() }) — or at minimum use screen.getByRole('region') with an aria-label. This would align with the project's prefer-semantic-queries convention.

[rtibbles]

Done — the preview now has role="img" + :aria-label="customThemePreview$()", and the test uses screen.getByRole('img', { name: customThemePreview$() }).

[rtibblesbot]

[praise] Excellent: resolveTheme handles theme identity migration cleanly

The id-first, name-fallback design in resolveTheme is a clean solution to the migration problem: existing localStorage snapshots without ids are still resolved correctly by name, while custom themes (uuid ids not present in THEMES) are returned untouched. The accompanying tests cover all four cases (id match, name fallback, custom uuid, same-name-as-fixed). Well done.

[rtibblesbot]

[praise] Excellent: proactive Alwan accessibility patching

patchAlwanAccessibility() identifies and corrects two known axe failures in the third-party library's generated markup (aria-prohibited-attr on the spectrum div, aria-valid-attr on control icons) and fixes them once on mount. The inline comment explains both the defects and their fixes clearly — this is exactly the kind of defensive a11y work that prevents regressions from slipping through.

[rtibblesbot]

Prior-finding status:

RESOLVED

• Sign-in gate on localStorage-only operation — isUserLoggedIn check and UiAlert removed entirely; anonymous theme creation now allowed.
• Newly added state uses Options API data() despite setup() being present (thread 1) — data() block removed; themeToDelete, editingTheme, newThemeName moved to ref() in setup().
• Hardcoded #cccccc border colors in AddEditCustomThemeModal styles — Both borders now use $themeTokens.fineLine via :style binding and themeColorOptionStyles.
• UiAlert inline style should move to style block — Moot — UiAlert removed together with the sign-in guard.
• border-color: black !important on Delete/Edit buttons — Custom border and .theme-settings-button class removed; stock flat-button KButtons used.
• getCurrentInstance() to access $coreOutline — getCurrentInstance() removed; themeStyle now accepts coreOutline as a parameter, called as themeStyle($coreOutline) from template.
• height: 6vh for color-swatch buttons — Changed to height: 48px.
• float: left used for three-column color-picker layout — Replaced with display: flex on .color-select-container and flex: 1 on .theme-option-container.
• New reactive state added via data() despite setup() being present (thread 2) — Same fix as thread 1 — data() removed, state in ref() in setup(). Methods remain in methods: (acknowledged by author as correct via auto-unwrap).
• Default theme name myTheme${index} is not translated — defaultThemeName string added to customThemeStrings.js with {index} ICU param; generateNewThemeName() uses it.
• Alwan instance not destroyed on component unmount — onUnmounted(() => { if (alwanInstance) { alwanInstance.destroy(); } }) added at ColorPickerModal.vue:69.
• patchAlwanAccessibility patches the entire document — Function now accepts root parameter (pickerRoot.value); all DOM queries scoped to the modal container.
• Hardcoded id='color-picker' should be a Vue ref — colorPickerEl Vue ref used; Alwan instantiated with colorPickerEl.value directly.
• Test queries preview element by CSS class name — Preview div now has role='img' and aria-label; test uses screen.getByRole('img', { name: customThemePreview$() }).

Every finding from the prior review has been addressed — the author resolved all fourteen code issues (sign-in gate removed, state moved to ref() in setup(), colors tokenised, layout converted from float to flex, alwan instance cleaned up on unmount, a11y patch scoped to the modal root, Vue refs replacing the hardcoded DOM id, and semantic test queries replacing CSS-class selectors) and acknowledged the methods: Options-API interop explanation. CI is green (only the Raspberry Pi image build is still in progress — unrelated to this feature). Screenshots show a visually complete and consistent UI: the add/edit modal renders the preview with live colours, the colour picker modal is correctly titled per the field being edited, the custom theme list shows truncated names with accessible flat-button Edit/Delete actions, and the dark fixed themes display legible link colours.

[praise] All prior findings resolved

All fourteen prior findings were addressed in the current HEAD. Detailed resolution status is in the resolutions array below. No new issues found during this review pass.

ℹ️ This was an automatic code-only delta review triggered by new commits. Specialized frontend/backend passes and manual QA were skipped — re-request a review to run the full pipeline.

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:

• Retrieved prior bot reviews deterministically via the GitHub API
• Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
• Only raised NEW findings for newly introduced code
• Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
• Calibrated the final verdict deterministically from finding severities, CI status, and QA evidence

[rtibblesbot]

suggestion: customThemeStrings.$tr(...) breaks the $-suffix destructuring convention used everywhere else in this PR (and in the codebase). The $tr method is an implementation detail of the translator; callers should use the named $-suffix functions so usage is visible to i18n tooling that tracks string call-sites by that pattern.

Prefer explicit destructuring:

const { titleSelectBackground$, titleSelectText$, titleSelectLink$, titleSelectColor$ } = customThemeStrings;
const title = computed(() => {
  if (props.colorPicker === 'backgroundColor') return titleSelectBackground$();
  if (props.colorPicker === 'textColor') return titleSelectText$();
  if (props.colorPicker === 'linkColor') return titleSelectLink$();
  return titleSelectColor$();
});

[rtibblesbot]

suggestion: .$refs.button is KButton's internal DOM-element ref and is not part of its public API. All four focus-management sites (createTheme, updateTheme, deleteTheme/cancelAdd, cancelEdit) reach through it. If KButton exposes a focus() method directly on the component instance, call that instead — e.g. this.$refs.addCustomThemeButton.focus(). Worth a quick check against the KDS source or docs before this merges.

[rtibblesbot]

suggestion: The docblock correctly documents the per-call-instance design ("Each consumer reads the current state on creation; localStorage is the shared source of truth"), so the intent is clear. Worth noting the alternative for future readers: lifting customThemes to module scope would make all consumers share a single reactive ref and remove the dependency on localStorage-sync timing entirely:

const customThemes = ref(Lockr.get(CUSTOM_THEMES_STORAGE_KEY) || {});

export default function useCustomThemes() {
  // all callers now share the same ref
  ...
}

Not a required change — the current approach is safe given focus-lock prevents concurrent mutations — but worth considering if this composable gains more consumers.

[rtibblesbot]

suggestion: The spec covers initial rendering and fixed-theme/custom-theme selection well, but the new orchestration behaviour added to SettingsSideBar — opening AddEditCustomThemeModal, creating/editing/deleting themes, and the nextTick focus-management chains (createTheme, updateTheme, deleteTheme, cancelAdd, cancelEdit) — has no tests at this level. These paths are the glue of the feature; a wrong ref path or broken composable call would slip through the current suite.

Since Alwan is already stubbed here, a few integration tests are tractable. For example:

it('opens the add-theme modal when "Add new theme" is clicked', async () => {
  renderSettingsSideBar();
  await fireEvent.click(screen.getByRole('button', { name: addNewTheme$() }));
  expect(screen.getByRole('dialog')).toBeInTheDocument();
});

[rtibblesbot]

nitpick: container.querySelector('.truncate') couples the test to the CSS class name. The behaviour being verified (full name present in DOM rather than JS-truncated) can be asserted without the implementation detail:

expect(
  screen.getByRole('button', { name: setCustomTheme$({ themeName: 'aReallyLongThemeName' }) }),
).toBeInTheDocument();

This also exercises the accessible-name contract, which is the user-facing behaviour.

[rtibblesbot]

nitpick: Every other spec file that stubs Alwan (SettingsSideBar.spec.js, AddEditCustomThemeModal.spec.js, ColorPickerModal.spec.js) includes destroy() {} alongside on() {}. This stub omits it. Current tests don't render the component so it doesn't matter today, but adding destroy() {} keeps the pattern consistent and prevents a surprise if a future test renders EpubRendererIndex and triggers the onUnmounted cleanup.