feat: add design system, improve layouts, and enhance importer UX#420
feat: add design system, improve layouts, and enhance importer UX#420djdembeck wants to merge 4 commits into
Conversation
…able - PRODUCT.md: product register, users, brand personality, design principles - DESIGN.md: visual system (The Organized Shelf), color palette, typography, elevation, components, and do's/don'ts per Stitch DESIGN.md spec - .gitignore: add .impeccable directory
…ments Harden: - Add RetryBookView endpoint (POST /api/retry-book/) to re-queue errored books - Add 'Retry this book' button on error books in book list - Add import confirmation modal showing selected directory count - Add prefers-reduced-motion support for spinner, progress, and folder-loading animations Clarify: - Add explanation text when Match submit is disabled (ASIN selection required) - Improve no-match-found message: 'No automatic match found. Try Custom Search.' - Add ASIN explanation help text for first-time users on Match page - Replace empty state 'Nothing to see here' with actionable message Polish: - Standardize modal button order: cancel left, confirm right - Relabel remove confirmation: 'Keep item'/'Remove item' instead of 'Yes'/'No' - Add aria-label, role, tabindex, and keyboard handler to remove icon - Fix invalid Bulma class is-three-quarter → is-three-quarters
Replace flat Bulma columns + nested box cards with semantic grid layout: - Grid: cover | metadata columns, description spans full width below - Group metadata into people (authors, narrators, series) and details (length, type, release, language, publisher) with border separators - Hide empty author/narrator rows with conditional template logic - Cover image: aspect-ratio 2/3, object-fit cover, onerror class fallback - Responsive: 160px cover desktop, 100px tablet, stacked mobile - Remove nested .box wrapper from book_tabs (cards-in-cards) - Add 4pt spacing scale, BEM-structured CSS classes in base.html
…ne styles - Add :root CSS custom properties for all design tokens (colors, spacing, z-index, radii, typography, transitions) aligned with DESIGN.md - Replace all hard-coded color values with var() references - Fix text contrast on gold pre-loader: #4a4a4a → #2a2200 (6.87:1 → 12.27:1) - Remove 13-line inline style duplication on #pre-loader element - Replace z-index: 9999 with semantic --z-overlay token - Add focus-visible indicators on breadcrumb nav links - Add aria-label to primary and breadcrumb nav landmarks - Hoist spacing custom properties from .book-entry to :root - Add line-height: 1.25 to pre-loader h2 for consistency - Move max-width: 75ch to .book-description base rule
📝 WalkthroughWalkthroughThis PR establishes design system and product documentation, refactors the UI with design tokens and accessibility improvements, adds a client/server book retry flow, implements an import confirmation modal with reduced-motion support, and enhances form validation and keyboard navigation for the match workflow. ChangesDesign System, UI Refactoring, and Retry Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 `@DESIGN.md`:
- Line 106: Fix the broken Unicode character in the DESIGN.md sentence under the
"Character: Single-family" paragraph by replacing the fragment "without引入ing a
second typeface" with a correct English phrase such as "without introducing a
second typeface" (or "without needing a second typeface"); locate the line
containing "Character: Single-family type with weight contrast" and update the
offending string to the corrected wording to remove the stray non‑ASCII
characters.
In `@importer/templates/book_list.html`:
- Line 21: Extract the inline author/narrator concatenation into a reusable
template filter: create a filter named format_names (registered in
importer/templatetags) that accepts an iterable of person objects, builds "First
Last" strings, and returns "" for empty, a single name, "A and B" for two, or
"A, B, ... and Z" for >2; then replace the inline logic in the template (where
book.authors.all and book.narrators.all are used) with {{
book.authors.all|format_names }} and {{ book.narrators.all|format_names }}
respectively to remove duplication and simplify the template.
- Line 70: The retry button's dynamic text updates (elements with class
"retry-book-btn" in book_list.html and updated by scripts in book_tabs.html
around lines 53/58/63) are not announced to screen readers; add an accessible
live region by either adding aria-live="polite" to the <button
class="retry-book-btn"> or placing a nearby element (e.g., <span
class="retry-status" aria-live="polite">) that your JavaScript updates instead
of only changing the button label, and update the scripts in book_tabs.html to
set textContent on that live region (or update both label and aria-live region)
so status changes are announced to assistive tech.
In `@importer/templates/book_tabs.html`:
- Around line 34-67: Extract the inline click handler into a testable module
(e.g., create book_retry.js exporting async function retryBook(asin, button))
and move the document.addEventListener logic in book_tabs.html to import and
call that function; inside retryBook use getCSRFToken(), fetch the "{% url
\"api-retry-book\" %}" POST, parse non-2xx response bodies and include response
error text or exception message in the button/UI (instead of the generic 'Retry
failed'), and return a structured result ({success: boolean, error?: string}) so
callers can update the UI in-place (update status classes/text based on the
returned result) rather than calling setTimeout(location.reload()) to preserve
unsaved state and enable unit tests for retryBook and its error branches.
In `@importer/templates/importer.html`:
- Around line 118-135: The modal lacks keyboard accessibility for the ESC key;
add a keydown listener in importer.js that detects e.key === 'Escape' (or e.code
=== 'Escape') and, when the element with id "import-confirm-modal" has the
active class (e.g., contains 'is-active'), calls closeImportConfirmation() to
close it; ensure the listener is registered once (and removed if you tear down
the UI) so pressing ESC while the modal is open triggers
closeImportConfirmation() and does not interfere with other keyboard handlers.
- Line 125: The modal buttons lack explicit type attributes and may default to
type="submit"; update each modal <button> used for UI actions (e.g., the button
with class="delete" that calls closeImportConfirmation(), and the other modal
buttons at the nearby lines) to include type="button" to prevent accidental form
submission if the DOM changes; locate buttons by their class or onclick handlers
(e.g., class="delete" and onclick="closeImportConfirmation()") and add
type="button" to each.
In `@importer/templates/importer.js`:
- Around line 501-503: The current loose selection of the form via the `form`
variable using a method-based selector can pick the wrong form; update the
template and selection so it's specific: add a unique id to the import form
(e.g., "import-form") and replace the query logic that sets the `form` variable
to use that id lookup; alternatively, if you prefer not to add an id, change the
lookup to find the form that contains the directory inputs (select the input
named "input_dir" and call closest form) or select by a more specific
action-based selector (form whose action contains "import"); update the code
paths that reference `form` accordingly so the correct form is always submitted.
- Around line 477-504: The three new functions openImportConfirmation,
closeImportConfirmation, and confirmImport are not exported and thus untestable;
update the module.exports object (the export block near module.exports = { ... }
in this file) to include these three function names so tests can import them—add
openImportConfirmation, closeImportConfirmation, and confirmImport to the
existing exported functions list following the same ordering/style as other
exported helpers.
In `@importer/templates/match.html`:
- Line 121: The Cancel button (and other modal buttons using class "button" with
onclick handlers such as the one calling closeSearchPanel()) lacks an explicit
type and will default to submit; update each modal button element to include
type="button" (e.g., the Cancel button and the other modal buttons around the
same area) so they do not trigger form submission when clicked.
- Line 60: The onkeydown handler for the remove button only triggers on Enter;
update it to also handle the Space key so keyboard users get native-like button
behavior: modify the onkeydown attribute that calls
openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{
forloop.counter|add:'-1' }}') to detect event.key === 'Enter' OR event.key === '
' (or event.code === 'Space') and call openRemoveConfirmationModal, and call
event.preventDefault() for the Space key to avoid page scrolling; ensure the
same update is applied to both onclick/onkeydown handlers referencing
openRemoveConfirmationModal and the same template variables.
In `@importer/templates/match.js`:
- Around line 140-144: The loop over document.querySelectorAll(".asin-select")
uses forEach with a return inside its callback which only exits the callback,
causing unnecessary iterations after hasValues is set to false; change the logic
to use Array.some() (or a for...of) to stop iterating once an invalid select
(select.value.length != 10) is found and set hasValues accordingly—locate the
block that iterates over ".asin-select" and the hasValues variable and replace
the forEach callback with an early-exiting construct so the loop stops on first
invalid value.
In `@importer/views.py`:
- Around line 305-307: Wrap the multi-step status mutation on book.status
(setting book.status.status and book.status.message, then calling
book.status.save()) in an atomic transaction to ensure the update is performed
atomically; use Django's transaction.atomic() around the operations that modify
the Status model (the book.status object) so the two assignments and save()
happen inside one DB transaction and cannot be partially applied or interleaved
by concurrent requests.
- Around line 294-313: The RetryBookView.post must enforce auth, CSRF, ASIN
format, and guard against duplicate queueing: make RetryBookView inherit
LoginRequiredMixin and apply `@method_decorator`(csrf_protect) to dispatch;
validate asin with a regex like r'^[A-Za-z0-9]{10}$' and return 400 for invalid
format; load Book.objects.select_for_update() inside a transaction.atomic() to
check book.status.status and only proceed if it's not already
StatusChoices.PROCESSING (return 409 if it is); to prevent duplicate task
enqueueing use a short-lived distributed lock or cache key (e.g.,
cache.add(f"retry-lock:{asin}", True, timeout=60)) before calling
m4b_merge_task.delay(asin) and clear the lock after scheduling; update
book.status.message and book.status.status = StatusChoices.PROCESSING and save
only after acquiring the lock and confirming status transition succeeded.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 271393ee-ca69-4f3a-a737-90954f3bdbfb
⛔ Files ignored due to path filters (1)
node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (12)
.gitignoreDESIGN.mdPRODUCT.mdimporter/templates/base.htmlimporter/templates/book_list.htmlimporter/templates/book_tabs.htmlimporter/templates/importer.htmlimporter/templates/importer.jsimporter/templates/match.htmlimporter/templates/match.jsimporter/urls.pyimporter/views.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Please review focusing on:
- Code quality and consistency with existing patterns
- Adherence to TypeScript best practices
- Test coverage for any new code
- Security implications of changes
- Performance considerations
- Documentation updates where needed
This project follows the AGENTS.md repository standards. Please ensure:
- Conventional commit format is followed
- Unit tests are included with proper coverage
- No breaking changes are introduced without proper discussion
Files:
importer/templates/importer.htmlimporter/urls.pyimporter/templates/match.htmlimporter/views.pyPRODUCT.mdimporter/templates/importer.jsDESIGN.mdimporter/templates/book_tabs.htmlimporter/templates/match.jsimporter/templates/book_list.htmlimporter/templates/base.html
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (Custom checks)
**/*.{ts,tsx,js,jsx}: PR must follow all standards in AGENTS.md including test coverage thresholds (85%+ statements/lines/functions, 80%+ branches)
PR must pass linting according to AGENTS.md standards
Files:
importer/templates/importer.jsimporter/templates/match.js
🪛 HTMLHint (1.9.2)
importer/templates/importer.html
[warning] 125-125: The type attribute must be present on elements.
(button-type-require)
[warning] 131-131: The type attribute must be present on
elements.(button-type-require)
[warning] 132-132: The type attribute must be present on
elements.(button-type-require)
importer/templates/match.html
[warning] 121-121: The type attribute must be present on elements.
(button-type-require)
[warning] 139-139: The type attribute must be present on
elements.(button-type-require)
[warning] 140-140: The type attribute must be present on
elements.(button-type-require)
🪛 LanguageTool
PRODUCT.md
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ns, bouncing animations, toy-like UI. - No cold enterprise tools: sterile dense da...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
DESIGN.md
[style] ~116-~116: In this context, consider an alternative for the overused word “looks”.
Context: ... needs a reason stronger than "it looks nice." The weight gap between label (600) an...
(LOOKS_NICE)
[style] ~172-~172: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ns, links) and progress indicators. - Do maintain ≥4.5:1 contrast for body tex...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~175-~175: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... white on gold is elevation enough. - Do keep button labels as verb + object: ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~176-~176: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., "Save settings", "Custom Search". - Do use word break / overflow wrap on boo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~177-~177: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...long file paths (already in place). - Do support reduced motion: suppress spin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~184-~184: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e illustrations, toy-like elements. - Don't create cold enterprise layouts: de...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e form walls, admin-panel monotony. - Don't use Brand Gold as text color on wh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
DESIGN.md
[warning] 83-83: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 86-86: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 108-108: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 128-128: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 135-135: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 142-142: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 147-147: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 153-153: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 157-157: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 162-162: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 169-169: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 179-179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (16)
.gitignore (1)
15-15: LGTM!importer/templates/base.html (6)
19-56: LGTM!
76-253: Excellent pre-loader implementation with accessibility support.The pre-loader CSS demonstrates strong adherence to the design system (DESIGN.md) with proper token usage, semantic z-index scale, and comprehensive reduced-motion support. The contrast choice for text on gold backgrounds (--color-body-on-gold:
#2a2200) ensures WCAG compliance.Source: Coding guidelines
255-389: LGTM!
391-443: Strong accessibility implementation.The focus indicators, responsive behavior, and reduced-motion support align well with the accessibility requirements specified in PRODUCT.md and DESIGN.md. The prefers-reduced-motion media query properly disables all animations while preserving functionality.
Source: Coding guidelines
487-487: LGTM!
504-504: LGTM!importer/templates/importer.html (2)
62-66: LGTM!
112-112: LGTM!importer/templates/book_tabs.html (1)
29-32: LGTM!importer/urls.py (1)
16-17: LGTM!importer/templates/book_list.html (1)
2-2: Avoid hiding ERROR books behind{% if book.asin %}
importer/views.py’sMatchView.postrejects missing/blank ASINs (if not asin: return JsonResponse({"error": "ASIN required"}, status=400)), so in the normal flow ERROR books should have an ASIN and won’t be hidden by{% if book.asin %}. If any other code path can createBookrows with an empty/falseyasin, ERROR entries could still disappear; consider rendering the ERROR state even whenasinis empty.importer/templates/match.html (2)
48-48: LGTM!
77-77: LGTM!importer/templates/match.js (2)
93-95: LGTM!
137-156: ASIN validation correctly enforces 10-character requirement.The client-side validation logic matches the server-side
BookManager.book_asin_validatorcontract, which requires exactly 10 characters per ASIN. The submit button gating and help-element toggling provide clear user feedback before form submission.
| **Body Font:** System sans-serif stack (BlinkMacSystemFont, -apple-system, Segoe UI, Roboto, Oxygen, Ubuntu, Cantarell, Fira Sans, Droid Sans, Helvetica Neue, sans-serif) | ||
| **Display/Label Font:** Same family. Hierarchy through weight and size, not through typeface switches. | ||
|
|
||
| **Character:** Single-family type with weight contrast. The system font reads as familiar and native on every platform, which is the right feel for a self-hosted tool. Weight contrast (400 vs 600) and size steps create hierarchy without引入ing a second typeface. |
There was a problem hiding this comment.
Fix typo: broken character in text.
Line 106 contains "without引入ing a second typeface" with a broken Unicode character. This should read "without introducing a second typeface" or "without needing a second typeface."
✏️ Proposed fix
-**Character:** Single-family type with weight contrast. The system font reads as familiar and native on every platform, which is the right feel for a self-hosted tool. Weight contrast (400 vs 600) and size steps create hierarchy without引入ing a second typeface.
+**Character:** Single-family type with weight contrast. The system font reads as familiar and native on every platform, which is the right feel for a self-hosted tool. Weight contrast (400 vs 600) and size steps create hierarchy without introducing a second typeface.📝 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.
| **Character:** Single-family type with weight contrast. The system font reads as familiar and native on every platform, which is the right feel for a self-hosted tool. Weight contrast (400 vs 600) and size steps create hierarchy without引入ing a second typeface. | |
| **Character:** Single-family type with weight contrast. The system font reads as familiar and native on every platform, which is the right feel for a self-hosted tool. Weight contrast (400 vs 600) and size steps create hierarchy without introducing a second typeface. |
🤖 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 `@DESIGN.md` at line 106, Fix the broken Unicode character in the DESIGN.md
sentence under the "Character: Single-family" paragraph by replacing the
fragment "without引入ing a second typeface" with a correct English phrase such as
"without introducing a second typeface" (or "without needing a second
typeface"); locate the line containing "Character: Single-family type with
weight contrast" and update the offending string to the corrected wording to
remove the stray non‑ASCII characters.
| <div class="book-entry__row"> | ||
| <dt>Author{{ book.authors.all|pluralize }}</dt> | ||
| <dd> | ||
| {% for author in book.authors.all %}{% if not forloop.first %}{% if forloop.last %} and {% else %}, {% endif %}{% endif %}{{ author.first_name }} {{ author.last_name }}{% endfor %} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider extracting author/narrator name formatting to a template filter.
The inline template logic for concatenating author and narrator names with commas and "and" is complex and duplicated. Moving this to a custom template filter would improve readability and maintainability.
♻️ Example template filter
Create a template filter in importer/templatetags/:
`@register.filter`
def format_names(persons):
"""Format a QuerySet of persons (authors/narrators) as a comma-separated list with 'and'."""
names = [f"{p.first_name} {p.last_name}" for p in persons]
if len(names) == 0:
return ""
elif len(names) == 1:
return names[0]
elif len(names) == 2:
return f"{names[0]} and {names[1]}"
else:
return ", ".join(names[:-1]) + f" and {names[-1]}"Then use: {{ book.authors.all|format_names }}
Also applies to: 29-29
🤖 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 `@importer/templates/book_list.html` at line 21, Extract the inline
author/narrator concatenation into a reusable template filter: create a filter
named format_names (registered in importer/templatetags) that accepts an
iterable of person objects, builds "First Last" strings, and returns "" for
empty, a single name, "A and B" for two, or "A, B, ... and Z" for >2; then
replace the inline logic in the template (where book.authors.all and
book.narrators.all are used) with {{ book.authors.all|format_names }} and {{
book.narrators.all|format_names }} respectively to remove duplication and
simplify the template.
| {% if book.status.status == 'Error' %} | ||
| <div class="book-entry__error"> | ||
| <p>{{ book.status.message }}</p> | ||
| <button class="button is-small is-link retry-book-btn" data-asin="{{ book.asin }}" type="button">Retry this book</button> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Verify accessibility of dynamically updated retry button.
The retry button text is updated by JavaScript in book_tabs.html (lines 53, 58, 63) to "Retrying...", "Retry failed", etc. These dynamic text changes should be announced to screen readers. Consider adding aria-live="polite" to the button or wrapping status messages in a live region.
♿ Proposed accessibility enhancement
- <button class="button is-small is-link retry-book-btn" data-asin="{{ book.asin }}" type="button">Retry this book</button>
+ <button class="button is-small is-link retry-book-btn" data-asin="{{ book.asin }}" type="button" aria-live="polite">Retry this book</button>📝 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.
| <button class="button is-small is-link retry-book-btn" data-asin="{{ book.asin }}" type="button">Retry this book</button> | |
| <button class="button is-small is-link retry-book-btn" data-asin="{{ book.asin }}" type="button" aria-live="polite">Retry this book</button> |
🤖 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 `@importer/templates/book_list.html` at line 70, The retry button's dynamic
text updates (elements with class "retry-book-btn" in book_list.html and updated
by scripts in book_tabs.html around lines 53/58/63) are not announced to screen
readers; add an accessible live region by either adding aria-live="polite" to
the <button class="retry-book-btn"> or placing a nearby element (e.g., <span
class="retry-status" aria-live="polite">) that your JavaScript updates instead
of only changing the button label, and update the scripts in book_tabs.html to
set textContent on that live region (or update both label and aria-live region)
so status changes are announced to assistive tech.
| document.addEventListener('click', async function(e) { | ||
| const btn = e.target.closest('.retry-book-btn'); | ||
| if (!btn) return; | ||
|
|
||
| const asin = btn.dataset.asin; | ||
| btn.classList.add('is-loading'); | ||
| btn.disabled = true; | ||
|
|
||
| try { | ||
| const resp = await fetch('{% url "api-retry-book" %}', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'X-CSRFToken': getCSRFToken(), | ||
| 'Content-Type': 'application/x-www-form-urlencoded' | ||
| }, | ||
| body: 'asin=' + encodeURIComponent(asin) | ||
| }); | ||
|
|
||
| if (resp.ok) { | ||
| btn.textContent = 'Retrying...'; | ||
| btn.classList.remove('is-loading'); | ||
| btn.classList.add('is-success'); | ||
| setTimeout(function() { location.reload(); }, 1500); | ||
| } else { | ||
| btn.textContent = 'Retry failed'; | ||
| btn.classList.remove('is-loading'); | ||
| btn.classList.add('is-danger'); | ||
| } | ||
| } catch (err) { | ||
| btn.textContent = 'Retry failed'; | ||
| btn.classList.remove('is-loading'); | ||
| btn.classList.add('is-danger'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Consider extracting retry logic to a testable module and improving error feedback.
The inline retry handler is functional but has several considerations:
- Testing: Per coding guidelines, JavaScript should have 85%+ test coverage. Inline scripts are difficult to test. Consider moving to
book_tabs.jsor a separate module. - UX: Line 56 reloads the entire page after 1.5 seconds. This discards any unsaved state and interrupts user flow. Consider updating the UI in-place instead.
- Error feedback: Lines 58, 63 display generic "Retry failed" without details. Users won't know if the failure is temporary or requires action.
♻️ Suggested improvements
Option 1: Move to testable module
// book_retry.js
export async function retryBook(asin, button) {
// ... existing logic ...
// Return status instead of reloading
return { success: true };
}Option 2: Improve error handling
} else {
- btn.textContent = 'Retry failed';
+ const errorMsg = await resp.text();
+ btn.textContent = 'Retry failed';
+ btn.setAttribute('title', errorMsg);
btn.classList.remove('is-loading');
btn.classList.add('is-danger');
}Option 3: Update UI in-place instead of reload
if (resp.ok) {
btn.textContent = 'Retrying...';
btn.classList.remove('is-loading');
btn.classList.add('is-success');
- setTimeout(function() { location.reload(); }, 1500);
+ // Update book entry to show processing state
+ const bookEntry = btn.closest('.book-entry');
+ bookEntry.querySelector('.book-entry__error').style.display = 'none';
}🤖 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 `@importer/templates/book_tabs.html` around lines 34 - 67, Extract the inline
click handler into a testable module (e.g., create book_retry.js exporting async
function retryBook(asin, button)) and move the document.addEventListener logic
in book_tabs.html to import and call that function; inside retryBook use
getCSRFToken(), fetch the "{% url \"api-retry-book\" %}" POST, parse non-2xx
response bodies and include response error text or exception message in the
button/UI (instead of the generic 'Retry failed'), and return a structured
result ({success: boolean, error?: string}) so callers can update the UI
in-place (update status classes/text based on the returned result) rather than
calling setTimeout(location.reload()) to preserve unsaved state and enable unit
tests for retryBook and its error branches.
Source: Coding guidelines
|
|
||
| <!-- Import confirmation modal --> | ||
| <div class="modal" id="import-confirm-modal"> | ||
| <div class="modal-background" onclick="closeImportConfirmation()"></div> | ||
| <div class="modal-card"> | ||
| <header class="modal-card-head"> | ||
| <p class="modal-card-title">Confirm import</p> | ||
| <button class="delete" aria-label="close" onclick="closeImportConfirmation()"></button> | ||
| </header> | ||
| <section class="modal-card-body"> | ||
| <p id="import-confirm-text">You've selected 0 directories. Import will begin and files will be matched against Audible metadata.</p> | ||
| </section> | ||
| <footer class="modal-card-foot"> | ||
| <button class="button" onclick="closeImportConfirmation()">Go back</button> | ||
| <button class="button is-link" id="import-confirm-btn" onclick="confirmImport()">Start import</button> | ||
| </footer> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding keyboard accessibility for ESC key.
The modal currently supports mouse/click interactions but lacks ESC key handling to close the modal, which is a standard accessibility pattern. Users expect to be able to dismiss modals with the ESC key.
💡 Implementation suggestion
Add a keydown event listener in importer.js that closes the modal when ESC is pressed while the modal is active:
// Add to importer.js
document.addEventListener('keydown', (e) => {
if (e.key === 'Escape') {
const modal = document.getElementById('import-confirm-modal');
if (modal && modal.classList.contains('is-active')) {
closeImportConfirmation();
}
}
});🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 125-125: The type attribute must be present on elements.
(button-type-require)
[warning] 131-131: The type attribute must be present on
elements.(button-type-require)
[warning] 132-132: The type attribute must be present on
elements.(button-type-require)
🤖 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 `@importer/templates/importer.html` around lines 118 - 135, The modal lacks
keyboard accessibility for the ESC key; add a keydown listener in importer.js
that detects e.key === 'Escape' (or e.code === 'Escape') and, when the element
with id "import-confirm-modal" has the active class (e.g., contains
'is-active'), calls closeImportConfirmation() to close it; ensure the listener
is registered once (and removed if you tear down the UI) so pressing ESC while
the modal is open triggers closeImportConfirmation() and does not interfere with
other keyboard handlers.
| <div class="field is-flex is-flex-direction-column"> | ||
| <div class="control is-inline-flex is-align-items-center"> | ||
| <i class="fa fa-times mr-2" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i> | ||
| <i class="fa fa-times mr-2" role="button" tabindex="0" aria-label="Remove {{ input.src_path|basename }}" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')" onkeydown="if(event.key==='Enter')openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding Space key support for complete keyboard accessibility.
The onkeydown handler currently only responds to the Enter key. Accessibility best practices recommend that interactive elements with role="button" should also respond to the Space key, matching native button behavior.
♿ Suggested enhancement
-<i class="fa fa-times mr-2" role="button" tabindex="0" aria-label="Remove {{ input.src_path|basename }}" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')" onkeydown="if(event.key==='Enter')openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i>
+<i class="fa fa-times mr-2" role="button" tabindex="0" aria-label="Remove {{ input.src_path|basename }}" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')" onkeydown="if(event.key==='Enter'||event.key===' ')openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i>📝 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.
| <i class="fa fa-times mr-2" role="button" tabindex="0" aria-label="Remove {{ input.src_path|basename }}" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')" onkeydown="if(event.key==='Enter')openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i> | |
| <i class="fa fa-times mr-2" role="button" tabindex="0" aria-label="Remove {{ input.src_path|basename }}" onclick="openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')" onkeydown="if(event.key==='Enter'||event.key===' ')openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{ forloop.counter|add:'-1' }}')"></i> |
🤖 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 `@importer/templates/match.html` at line 60, The onkeydown handler for the
remove button only triggers on Enter; update it to also handle the Space key so
keyboard users get native-like button behavior: modify the onkeydown attribute
that calls openRemoveConfirmationModal('{{ input.src_path|escapejs }}', '{{
forloop.counter|add:'-1' }}') to detect event.key === 'Enter' OR event.key === '
' (or event.code === 'Space') and call openRemoveConfirmationModal, and call
event.preventDefault() for the Space key to avoid page scrolling; ensure the
same update is applied to both onclick/onkeydown handlers referencing
openRemoveConfirmationModal and the same template variables.
| No results found for this search... | ||
| </div> | ||
| <div style="padding-left: 10px;"> | ||
| <button class="button" onclick="closeSearchPanel()">Cancel</button> |
There was a problem hiding this comment.
Add explicit type="button" attributes to modal buttons.
Buttons without an explicit type attribute default to type="submit", which can trigger unintended form submissions when the modal is rendered within or near a form context.
🐛 Proposed fix
- <button class="button" onclick="closeSearchPanel()">Cancel</button>
+ <button type="button" class="button" onclick="closeSearchPanel()">Cancel</button>
<button class="button is-link"
onclick="searchAsin(document.getElementById('title').value, document.getElementById('author').value, document.getElementById('keywords').value)">Search</button>- <button class="button" onclick="closeRemoveConfirmationModal()">Keep item</button>
- <button class="button is-danger" id="remove-column-button">Remove item</button>
+ <button type="button" class="button" onclick="closeRemoveConfirmationModal()">Keep item</button>
+ <button type="button" class="button is-danger" id="remove-column-button">Remove item</button>Also applies to: 139-140
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 121-121: The type attribute must be present on elements.
(button-type-require)
🤖 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 `@importer/templates/match.html` at line 121, The Cancel button (and other
modal buttons using class "button" with onclick handlers such as the one calling
closeSearchPanel()) lacks an explicit type and will default to submit; update
each modal button element to include type="button" (e.g., the Cancel button and
the other modal buttons around the same area) so they do not trigger form
submission when clicked.
Source: Linters/SAST tools
| document.querySelectorAll(".asin-select").forEach(function(select) { | ||
| if (select.value.length != 10) { | ||
| hasValues = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using Array.some() for clearer early-exit semantics.
The return statement inside forEach only exits the current callback iteration, not the entire loop. While the logic works correctly (once hasValues is set to false, it remains false), the code continues iterating unnecessarily. Using Array.some() or a for...of loop would more clearly express the intent to stop checking after finding the first invalid select.
♻️ Optional refactor for clarity and efficiency
function checkAllSelectsHaveValue() {
- var hasValues = true;
-
- document.querySelectorAll(".asin-select").forEach(function(select) {
- if (select.value.length != 10) {
- hasValues = false;
- return;
- }
- });
+ const hasInvalidSelect = Array.from(document.querySelectorAll(".asin-select")).some(function(select) {
+ return select.value.length != 10;
+ });
var submitBtn = document.getElementById("match-form-submit");
var helpEl = document.getElementById("match-submit-help");
- if (!hasValues) {
+ if (hasInvalidSelect) {
submitBtn.disabled = true;
if (helpEl) helpEl.style.display = 'block';
} else {🤖 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 `@importer/templates/match.js` around lines 140 - 144, The loop over
document.querySelectorAll(".asin-select") uses forEach with a return inside its
callback which only exits the callback, causing unnecessary iterations after
hasValues is set to false; change the logic to use Array.some() (or a for...of)
to stop iterating once an invalid select (select.value.length != 10) is found
and set hasValues accordingly—locate the block that iterates over ".asin-select"
and the hasValues variable and replace the forEach callback with an
early-exiting construct so the loop stops on first invalid value.
| class RetryBookView(View): | ||
| def post(self, request): | ||
| asin = request.POST.get("asin") | ||
| if not asin: | ||
| return JsonResponse({"error": "ASIN required"}, status=400) | ||
| try: | ||
| book = Book.objects.get(asin=asin) | ||
| except Book.DoesNotExist: | ||
| return JsonResponse({"error": "Book not found"}, status=404) | ||
|
|
||
| # Reset status to Processing | ||
| book.status.status = StatusChoices.PROCESSING | ||
| book.status.message = "Retrying..." | ||
| book.status.save() | ||
|
|
||
| # Re-queue the Celery task | ||
| m4b_merge_task.delay(asin) | ||
|
|
||
| return JsonResponse({"status": "success", "asin": asin}) | ||
|
|
There was a problem hiding this comment.
CRITICAL: Add authentication, CSRF protection, and prevent duplicate task queueing.
The RetryBookView endpoint has several security and reliability concerns:
- No authentication: Any unauthenticated user can retry any book by ASIN. This is a security risk if the application is exposed publicly.
- CSRF protection: While Django's CSRF middleware typically protects POST requests, class-based views require explicit decorator or mixin. Verify that
CsrfViewMiddlewareis active. - Duplicate tasks: The view doesn't check if a task is already queued or processing for this ASIN. Multiple clicks or concurrent requests could queue duplicate tasks, wasting resources and potentially causing race conditions.
- Status validation: The view doesn't check the current status before retrying. It will retry even if the book is already
PROCESSING. - No ASIN validation: The view doesn't validate the ASIN format (should be 10 alphanumeric characters based on typical Audible ASINs).
🔒 Proposed fixes
+from django.contrib.auth.mixins import LoginRequiredMixin
+from django.views.decorators.csrf import csrf_protect
+from django.utils.decorators import method_decorator
+from django.core.cache import cache
+
+@method_decorator(csrf_protect, name='dispatch')
-class RetryBookView(View):
+class RetryBookView(LoginRequiredMixin, View):
def post(self, request):
asin = request.POST.get("asin")
if not asin:
return JsonResponse({"error": "ASIN required"}, status=400)
+
+ # Validate ASIN format (10 alphanumeric characters)
+ if not (len(asin) == 10 and asin.isalnum()):
+ return JsonResponse({"error": "Invalid ASIN format"}, status=400)
+
+ # Check for duplicate in-flight retry using cache
+ cache_key = f"retry_lock_{asin}"
+ if cache.get(cache_key):
+ return JsonResponse({"error": "Retry already in progress"}, status=429)
+
try:
book = Book.objects.get(asin=asin)
except Book.DoesNotExist:
return JsonResponse({"error": "Book not found"}, status=404)
+
+ # Only retry if in ERROR state
+ if book.status.status != StatusChoices.ERROR:
+ return JsonResponse(
+ {"error": f"Cannot retry book in {book.status.status} state"},
+ status=400
+ )
- # Reset status to Processing
- book.status.status = StatusChoices.PROCESSING
- book.status.message = "Retrying..."
- book.status.save()
+ # Use transaction to ensure atomic status update
+ from django.db import transaction
+ with transaction.atomic():
+ book.status.status = StatusChoices.PROCESSING
+ book.status.message = "Retrying..."
+ book.status.save()
+
+ # Set retry lock for 5 minutes
+ cache.set(cache_key, True, timeout=300)
- # Re-queue the Celery task
- m4b_merge_task.delay(asin)
+ # Re-queue the Celery task
+ m4b_merge_task.delay(asin)
return JsonResponse({"status": "success", "asin": asin})🤖 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 `@importer/views.py` around lines 294 - 313, The RetryBookView.post must
enforce auth, CSRF, ASIN format, and guard against duplicate queueing: make
RetryBookView inherit LoginRequiredMixin and apply
`@method_decorator`(csrf_protect) to dispatch; validate asin with a regex like
r'^[A-Za-z0-9]{10}$' and return 400 for invalid format; load
Book.objects.select_for_update() inside a transaction.atomic() to check
book.status.status and only proceed if it's not already StatusChoices.PROCESSING
(return 409 if it is); to prevent duplicate task enqueueing use a short-lived
distributed lock or cache key (e.g., cache.add(f"retry-lock:{asin}", True,
timeout=60)) before calling m4b_merge_task.delay(asin) and clear the lock after
scheduling; update book.status.message and book.status.status =
StatusChoices.PROCESSING and save only after acquiring the lock and confirming
status transition succeeded.
| book.status.status = StatusChoices.PROCESSING | ||
| book.status.message = "Retrying..." | ||
| book.status.save() |
There was a problem hiding this comment.
Use atomic transaction for status update.
The status update happens in two steps (setting status and message, then calling save()). If the application is interrupted between these operations or if there's a concurrent request, the status could be inconsistent. Wrap the update in a transaction as shown above.
🤖 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 `@importer/views.py` around lines 305 - 307, Wrap the multi-step status
mutation on book.status (setting book.status.status and book.status.message,
then calling book.status.save()) in an atomic transaction to ensure the update
is performed atomically; use Django's transaction.atomic() around the operations
that modify the Status model (the book.status object) so the two assignments and
save() happen inside one DB transaction and cannot be partially applied or
interleaved by concurrent requests.
1 participant
Summary
Extract design tokens from inline styles, add comprehensive design system documentation (DESIGN.md, PRODUCT.md), restructure book_list.html with grid layout for improved hierarchy, and add error retry with import confirmation modal for better UX.
Spirit/Intent
Establish a maintainable design system foundation while improving accessibility, visual hierarchy, and user experience across the importer workflow.
Key Changes
Risks
Summary by CodeRabbit
New Features
Documentation
Accessibility