feat(search): improve filename normalization for better auto results#414
feat(search): improve filename normalization for better auto results#414
Conversation
- Add regex pattern to remove 'read by' / 'narrated by' with narrator names - Add regex pattern to remove part/volume/book/chapter indicators - Add regex pattern to remove years in parentheses/brackets and standalone 4-digit years - Add unit tests for normalize_name method Improves ASIN auto-matching by cleaning up common audiobook filename patterns.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds ARIA roles/attributes and keyboard/focus handling to the book tabs UI and refines SearchTool.normalize_name to strip narrator credits, part/volume indicators, and years; new unit tests cover the normalization behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (3 passed)
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: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
importer/templates/base.htmlimporter/templates/book_list.htmlimporter/templates/book_tabs.htmlutils/search_tools.pyutils/tests.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ 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/base.htmlutils/search_tools.pyutils/tests.pyimporter/templates/book_tabs.htmlimporter/templates/book_list.html
🧬 Code graph analysis (1)
utils/tests.py (1)
utils/search_tools.py (1)
normalize_name(61-106)
🪛 Ruff (0.15.1)
utils/tests.py
[warning] 292-292: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 293-293: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 294-294: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 295-295: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 300-300: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 301-301: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 302-302: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 303-303: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 308-308: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 309-309: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 310-310: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 315-315: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 316-316: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
🔇 Additional comments (2)
importer/templates/book_list.html (1)
30-32: LGTM!The CSS class application correctly enables word-wrapping for long book descriptions. The styling integration with
base.htmlis appropriate.utils/tests.py (1)
289-295: Regarding Ruff PT009 warnings: this is a false positive.The static analysis hints suggest using plain
assertinstead ofassertIn/assertNotIn. However, this is a DjangoTestCasewhich extendsunittest.TestCase. The unittest-style assertions provide better error messages on failure (showing actual vs expected values) and are idiomatic for this test framework.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/base.html`:
- Around line 207-213: The .book-description CSS block contains redundant and
deprecated properties: remove the legacy "word-wrap" declaration, replace the
deprecated "word-break: break-word" with "overflow-wrap: anywhere" (or adjust
overflow-wrap if already present) and drop the unnecessary "max-height: none"
since it's the default; update the .book-description rule to keep only the
needed properties (e.g., overflow-wrap / hyphens and any other necessary rules)
to eliminate redundancy and deprecated usage.
In `@importer/templates/book_tabs.html`:
- Around line 6-8: Add ARIA tab semantics: on the tab anchors (inside elements
with IDs done-tab, processing-tab, error-tab) add role="tab" and ensure an
aria-selected attribute is present and updated by the openTab function; when
openTab switches active tabs it should set aria-selected="true" on the newly
active anchor and aria-selected="false" on the others (in addition to toggling
the "is-active" class) so screen readers get correct selected state.
In `@utils/search_tools.py`:
- Around line 97-98: The current re.sub call assigning to name uses a two-part
regex `[\(\[]\s*\d{4}\s*[\)\]]|\b(19|20)\d{2}\b` but the bracket-case is dead
because earlier code already strips bracketed content (`\[[^"]*\]`) and later
removes all special chars (`[^\w\s]`); either simplify the expression to only
remove standalone years by replacing the pattern with `\b(19|20)\d{2}\b`, or
move this year-removal line to run before the bracket/special-character
stripping so the `[\(\[]\s*\d{4}\s*[\)\]]` branch can match — update the re.sub
call that assigns to name accordingly.
- Around line 86-89: The current re.sub call that cleans narrator phrases (the
assignment to the variable name using re.sub for the pattern matching
"read|narrated by") uses a greedy [^\[\]]+ which can remove unrelated trailing
title text; update that re.sub invocation to use a non-greedy or word-limited
pattern that only captures the narrator name (for example, match a few words
after "by" or use a non-greedy quantifier and stop at end/extra spacing) and
remove the redundant bracket-exclusion (since brackets are already stripped
earlier); locate the re.sub that removes "read by"/"narrated by" and replace the
pattern accordingly so only the narrator token(s) are removed and not the rest
of the title.
In `@utils/tests.py`:
- Around line 282-316: Add unit tests to TestSearchToolNormalizeName to cover
missing normalization cases: add a test method for the "narrated by" variant
(call self.tool.normalize_name on "Great Book narrated by Jane Doe" and assert
neither "narrated by" nor "jane doe" appear), a test for standalone years (e.g.,
"Book Title 2020 Edition" asserting "2020" is removed but "edition" remains), a
test for volume/chapter indicators (e.g., "Epic Saga Volume 2" asserting "volume
2" is removed and title words remain), and an edge-case test for trailing text
after a narrator (e.g., "Great Book read by John Smith The Sequel" asserting
that "sequel" is preserved) so normalize_name is exercised against the
greedy-regex bug.
- Remove deprecated CSS properties (overflow-wrap, word-wrap, word-break) and replace with modern overflow-wrap: anywhere - Add ARIA role attributes (role=tablist, role=tab, role=tabpanel) for proper accessibility tree - Add aria-selected state management in JavaScript for screen readers - Add aria-controls and aria-labelledby relationships for tab panels Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Limit narrator name capture to 2-3 words to prevent over-matching - Preserve trailing text after narrator patterns (e.g., sequels) - Add unit tests for narrator removal, year removal, and volume indicators Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
importer/templates/base.htmlimporter/templates/book_tabs.htmlimporter/templates/book_tabs.jsutils/search_tools.pyutils/tests.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/book_tabs.js
**/*
⚙️ 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/book_tabs.jsimporter/templates/book_tabs.htmlimporter/templates/base.htmlutils/tests.pyutils/search_tools.py
🪛 Ruff (0.15.1)
utils/tests.py
[warning] 292-292: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 293-293: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 294-294: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 295-295: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 300-300: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 301-301: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 302-302: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 303-303: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 308-308: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 309-309: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 310-310: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 315-315: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 316-316: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 321-321: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 322-322: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 323-323: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 324-324: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 329-329: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 330-330: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 331-331: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 332-332: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 337-337: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 338-338: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 339-339: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 340-340: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 345-345: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 346-346: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
[warning] 347-347: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 348-348: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
[warning] 349-349: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
🔇 Additional comments (5)
importer/templates/book_tabs.js (2)
14-22: ARIA selected-state sync looks correct.Nice update to keep
aria-selectedaligned with the active tab.
1-23: Remove reference to non-existent AGENTS.md file.The AGENTS.md file referenced in this review does not exist in the repository. The actual project uses Biome for linting (configured in biome.json with recommended rules enabled) and Node.js test runner. No specific coverage thresholds (85%+ statements/lines/functions, 80%+ branches) are documented in the project standards.
If coverage requirements should be enforced, they need to be explicitly configured in package.json or a dedicated test configuration file.
Likely an incorrect or invalid review comment.
utils/search_tools.py (1)
86-99: Normalization additions look good.The narrator and part/volume cleanup improves title matching without overreaching.
utils/tests.py (1)
282-349: Solid test coverage for new normalization behavior.Good additions covering narrator, part/volume, and year cases, plus trailing-text preservation.
importer/templates/base.html (1)
207-210: CSS addition looks good.The
book-descriptionclass provides sensible wrapping without extra baggage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/book_tabs.html`:
- Around line 6-8: The tab anchors (ids done-tab, processing-tab, error-tab) are
not keyboard-focusable because they lack href/tabindex; update each <a> to
include either a usable href (e.g. href="#") or explicit tabindex attributes so
they enter the tab order (set tabindex="0" for the active tab and tabindex="-1"
for inactive tabs), and ensure the click handler still calls
openTab('done'/'processing'/'error'); also update the openTab function to set
the active/inactive tabindex values (and call event.preventDefault() if you use
href="#") so keyboard users can focus and navigate tabs properly.
In `@utils/search_tools.py`:
- Around line 100-101: The current regex that strips years into the variable
name can produce an empty string for titles that are just a year (the re.sub
call assigning to name); change the flow so you first compute the stripped value
(e.g., stripped = re.sub(...)) and if stripped.strip() is empty then retain the
original name (or detect that the original is a pure year and keep it) instead
of overwriting name with an empty string — update the assignment around the
re.sub in utils/search_tools.py so name is only replaced when stripped is
non-empty.
…ty string bug - Add href and tabindex attributes to tab anchors in book_tabs.html - Update openTab() to manage tabindex and focus for keyboard accessibility - Fix empty string bug when year removal would result in empty title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
importer/templates/book_tabs.htmlimporter/templates/book_tabs.jsutils/search_tools.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/book_tabs.js
**/*
⚙️ 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/book_tabs.jsutils/search_tools.pyimporter/templates/book_tabs.html
🔇 Additional comments (3)
utils/search_tools.py (1)
93-99: Part/volume indicator cleanup looks solid.
The pattern is clear, scoped, and consistent with the existing normalization flow.importer/templates/book_tabs.js (1)
15-25: ARIA selected + tabindex synchronization is correct.
This keeps visual and accessibility states aligned when switching tabs.importer/templates/book_tabs.html (1)
4-4: Good addition ofrole="tablist".
Matches the ARIA tabs pattern and improves screen reader context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/book_tabs.html`:
- Around line 6-8: The tab anchors use href="#" which causes navigation unless
prevented; update the click handling so openTab accepts the event and calls
event.preventDefault(). Change the onclick invocations for the tab elements (ids
done-tab, processing-tab, error-tab) to pass the event and the tab name (e.g.,
onclick="openTab(event, 'done')"), and update the openTab function to accept
(event, tabName) and invoke event.preventDefault() at the start before
performing tab switching logic.
- Around line 13-19: The tab panels (divs with id="done", id="processing",
id="error") currently set aria-labelledby to the parent <li> instead of the
actionable tab element; update the corresponding <a> elements that have
role="tab" by adding unique ids (e.g. done-tab-anchor, processing-tab-anchor,
error-tab-anchor) and change each panel's aria-labelledby to reference those
anchor ids (replace existing aria-labelledby values on the divs for "done",
"processing", and "error"); ensure the <a> ids are unique and match exactly the
new aria-labelledby values.
In `@utils/search_tools.py`:
- Around line 86-92: The regex that strips narrator credits in
utils/search_tools.py currently requires 2–3 words after "by" (pattern using
\w+(?:\s+\w+){1,2}), so single-word narrators like "read by Cher" are missed;
update the pattern used in the re.sub that assigns to name (the block that
removes "read by"/"narrated by" patterns) to allow 1–3 words after "by" by
changing the quantifier to {0,2} (i.e., make the group optional for additional
words) so single-word names are matched and removed.
---
Duplicate comments:
In `@utils/search_tools.py`:
- Around line 100-104: The bracketed-year alternative in the regex used when
cleaning `name` (the pattern `[\(\[]\s*\d{4}\s*[\)\]]`) is dead because
punctuation was already removed earlier; either remove that bracketed-year
branch from the `re.sub` (leave just the `\b(19|20)\d{2}\b` year matcher) or
move the year-stripping step so it runs before the punctuation stripping logic
earlier in this function; update references to `name_before_year_removal` and
`name` accordingly so the fallback behavior still preserves the original when
stripping produces an empty string.
Resolved conflicts: - base.html: use develop's book-description CSS (more comprehensive) - book_tabs.html: keep accessibility fix from feature branch
- Fix tab click handling with event.preventDefault() to prevent navigation
- Add proper aria-labelledby references to tab anchors
- Update narrator regex to match single-word names (changed {1,2} to {0,2})
- Remove dead bracketed-year pattern from year regex
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
importer/templates/book_tabs.htmlimporter/templates/book_tabs.jsutils/search_tools.py
🧰 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/book_tabs.htmlimporter/templates/book_tabs.jsutils/search_tools.py
**/*.{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/book_tabs.js
🔇 Additional comments (7)
utils/search_tools.py (3)
86-92: LGTM! Narrator pattern correctly handles single-word names.The regex
{0,2}quantifier properly allows 1-3 word narrator names (e.g., "read by Cher", "read by John Smith", "read by Mary Jane Watson").
93-99: LGTM! Part/volume indicator removal is well-designed.The regex correctly handles various formats like "Part 1", "Volume 2", "Book 3 of 5", and "Chapter 10" with appropriate optional spacing and case insensitivity.
100-104: LGTM! Year removal correctly preserves year-only titles.The implementation properly guards against erasing legitimate year-only titles (e.g., "1984") by checking if removal would result in an empty string and preserving the original in that case.
importer/templates/book_tabs.js (2)
1-3: LGTM! Properly prevents default anchor navigation.The
event.preventDefault()call correctly prevents thehref="#"from causing page jumps when tabs are clicked.
17-27: LGTM! ARIA state management follows accessibility best practices.The implementation correctly updates
aria-selectedandtabindexattributes, and properly focuses the active tab anchor for keyboard navigation.importer/templates/book_tabs.html (2)
6-8: LGTM! Tab links implement proper ARIA tabs pattern.The implementation correctly includes:
role="tab"with unique IDs for anchorsaria-selectedandtabindexfor roving tabindex patternaria-controlslinking to panel IDs- Descriptive
aria-labelwith book counts
13-21: LGTM! Tab panels correctly reference their controlling tabs.The
aria-labelledbyattributes now properly reference the anchor elements withrole="tab"(e.g.,done-tab-anchor), which is the correct ARIA pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/book_tabs.html`:
- Line 4: The data-default attribute in the div
(importer/templates/book_tabs.html) is missing quotes around the template
variable; update the element that currently contains data-default={{
default_view }} to include quotes around the template expression
(data-default="{{ default_view }}") so the attribute value is valid HTML and
handles special characters in default_view correctly.
In `@importer/templates/book_tabs.js`:
- Around line 14-15: The current tab-showing code uses
document.getElementById(tabId) and document.getElementById(`${tabId}-tab`)
without null checks, which can throw if an element is missing; update the code
that sets style.display and classList (the lines referencing tabId and
`${tabId}-tab`) to first store each lookup in a local variable, check for
truthiness (e.g., if (tabElem) ...) and only then set style.display = "block"
and call classList.add("is-active"); optionally log a warning if an expected
element is not found to aid debugging.
- Around line 30-32: The current load handler reads const defaultTab =
document.querySelector(".tabs").dataset.default and immediately calls
openTab(..., defaultTab) which can pass undefined if data-default is missing;
update the handler to guard against a missing dataset.default by checking
document.querySelector(".tabs") exists and that dataset.default is truthy, and
if not choose a safe fallback (e.g., the id of the first tab button or a known
default string) before calling openTab; reference the defaultTab variable, the
querySelector(".tabs") call, and the openTab(...) invocation when making the
change.
- Quote data-default attribute to handle special characters correctly - Add null checks for document.getElementById calls with console warnings - Guard against missing data-default with fallback to first tab - Add trailing newline for style consistency
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
importer/templates/book_tabs.htmlimporter/templates/book_tabs.js
🧰 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/book_tabs.htmlimporter/templates/book_tabs.js
**/*.{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/book_tabs.js
🔇 Additional comments (3)
importer/templates/book_tabs.js (2)
1-25: Defensive null checks and warnings are solid.You avoid null dereferences and provide helpful diagnostics when elements are missing.
41-59: Fallback default-tab selection is robust.The staged fallback with warnings prevents runtime errors when
.tabsordata-defaultare missing.importer/templates/book_tabs.html (1)
4-19: ARIA wiring and metadata look consistent.Tablist roles,
aria-controls/aria-labelledbylinkage, and quoteddata-defaultimprove semantics and robustness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/book_tabs.js`:
- Around line 27-36: The current roving tabindex (in the tabAnchors.forEach
block) sets inactive tabs to tabindex="-1" but lacks keyboard handlers, so
implement arrow-key navigation: add a keydown listener on each anchor (the
tabAnchors NodeList / anchor elements) that handles
ArrowLeft/ArrowRight/Home/End to move focus to the previous/next/first/last tab,
and on focus update aria-selected and tabindex values for all anchors (set the
active anchor to '0' and others to '-1') to maintain the roving pattern;
alternatively, if you prefer not to implement arrow-key behavior, change the
code that sets tabindex to always set tabindex="0" for all anchors so every tab
remains keyboard-focusable.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
importer/templates/book_tabs.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/book_tabs.js
**/*
⚙️ 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/book_tabs.js
🔇 Additional comments (1)
importer/templates/book_tabs.js (1)
71-96: LGTM! Robust fallback logic for default tab selection.The implementation properly handles edge cases:
- Missing
.tabscontainer with warning- Missing
data-defaultattribute with fallback to first tab- Ultimate fallback to
"done"if no tabs existThe keydown handlers are correctly attached to enable keyboard navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importer/templates/book_tabs.js`:
- Around line 66-68: The code calls nextTab.getAttribute('aria-controls') and
passes the result (tabId) directly to openTab, which can be null; update the
logic around tabAnchors/nextIndex (where nextTab is retrieved) to check that
nextTab and its aria-controls attribute are non-null before calling openTab — if
tabId is falsy, skip calling openTab (or select the next valid anchor) and
optionally log or handle the missing attribute; reference the nextTab variable,
the getAttribute('aria-controls') call, and the openTab(...) invocation when
making the change.
- Around line 27-37: The code unconditionally calls anchor.focus() inside the
tab update loop which can steal focus on page load; modify openTab to accept a
user-initiated flag (or the original event) and only call anchor.focus() when
that flag/event indicates a real user interaction (e.g., userInitiated === true
or event.isTrusted). Update callers: when wiring tab clicks pass true (or the
click event), and when invoking openTab on window load pass false (or null) so
focus is not set. Keep references: change function openTab(...), the
tabAnchors.forEach(...) block where anchor.focus() is called, and all places
that call openTab (click handler and window load).
| const tabAnchors = document.querySelectorAll('.tab a[role="tab"]'); | ||
| tabAnchors.forEach(anchor => { | ||
| if (anchor.getAttribute('aria-controls') === tabId) { | ||
| anchor.setAttribute('aria-selected', 'true'); | ||
| anchor.setAttribute('tabindex', '0'); | ||
| anchor.focus(); | ||
| } else { | ||
| anchor.setAttribute('aria-selected', 'false'); | ||
| anchor.setAttribute('tabindex', '-1'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unconditional focus() may disrupt user on page load.
When openTab is called during window load (line 90), anchor.focus() will steal focus from wherever the user may have been, which can be jarring for keyboard and screen reader users. Consider only focusing when the call originates from a user interaction.
♻️ Proposed fix to conditionally focus based on user interaction
-function openTab(event, tabId) {
+function openTab(event, tabId, shouldFocus = true) {
event.preventDefault();Then in the ARIA update block:
if (anchor.getAttribute('aria-controls') === tabId) {
anchor.setAttribute('aria-selected', 'true');
anchor.setAttribute('tabindex', '0');
- anchor.focus();
+ if (shouldFocus) {
+ anchor.focus();
+ }And update the load call:
- openTab({ preventDefault: () => {} }, defaultTab);
+ openTab({ preventDefault: () => {} }, defaultTab, false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@importer/templates/book_tabs.js` around lines 27 - 37, The code
unconditionally calls anchor.focus() inside the tab update loop which can steal
focus on page load; modify openTab to accept a user-initiated flag (or the
original event) and only call anchor.focus() when that flag/event indicates a
real user interaction (e.g., userInitiated === true or event.isTrusted). Update
callers: when wiring tab clicks pass true (or the click event), and when
invoking openTab on window load pass false (or null) so focus is not set. Keep
references: change function openTab(...), the tabAnchors.forEach(...) block
where anchor.focus() is called, and all places that call openTab (click handler
and window load).
| const nextTab = tabAnchors[nextIndex]; | ||
| const tabId = nextTab.getAttribute('aria-controls'); | ||
| openTab({ preventDefault: () => {} }, tabId); |
There was a problem hiding this comment.
Add null check for missing aria-controls attribute.
If a tab anchor lacks the aria-controls attribute, getAttribute returns null, and openTab will be called with tabId = null. This would cause the tab content lookup to fail silently.
🛡️ Proposed defensive fix
const nextTab = tabAnchors[nextIndex];
const tabId = nextTab.getAttribute('aria-controls');
+ if (!tabId) {
+ console.warn('Tab anchor missing aria-controls attribute');
+ return;
+ }
openTab({ preventDefault: () => {} }, tabId);📝 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.
| const nextTab = tabAnchors[nextIndex]; | |
| const tabId = nextTab.getAttribute('aria-controls'); | |
| openTab({ preventDefault: () => {} }, tabId); | |
| const nextTab = tabAnchors[nextIndex]; | |
| const tabId = nextTab.getAttribute('aria-controls'); | |
| if (!tabId) { | |
| console.warn('Tab anchor missing aria-controls attribute'); | |
| return; | |
| } | |
| openTab({ preventDefault: () => {} }, tabId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@importer/templates/book_tabs.js` around lines 66 - 68, The code calls
nextTab.getAttribute('aria-controls') and passes the result (tabId) directly to
openTab, which can be null; update the logic around tabAnchors/nextIndex (where
nextTab is retrieved) to check that nextTab and its aria-controls attribute are
non-null before calling openTab — if tabId is falsy, skip calling openTab (or
select the next valid anchor) and optionally log or handle the missing
attribute; reference the nextTab variable, the getAttribute('aria-controls')
call, and the openTab(...) invocation when making the change.
1 participant
Summary
Improves the
normalize_namemethod inSearchToolto clean up search strings more effectively, resulting in better ASIN auto-search results for audiobooks.Changes
utils/search_tools.pyutils/tests.pyTestSearchToolNormalizeNametest class with 4 test methods covering all new patternsTest Results
All tests pass:
Related Issue
#159
Summary by CodeRabbit
New Features
Tests