feat: in-book search for epubs#1
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds EPUB in-book search with version-42 cache records, streaming matching, reader search UI/highlighting, locale strings, docs, tests, and CI wiring. ChangesIn-book EPUB search with v42 section cache
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
🤖 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 `@lib/Epub/Epub/Section.cpp`:
- Around line 830-833: The page-count read path in the Section logic returns
count even when the seek or read fails, which can propagate an uninitialized
value into search route resolution. Update the page-count helper in Section.cpp
to check the seek/read outcome before using count, and return nullopt on any
failure instead of returning a possibly invalid value. Keep the fix localized to
the page-count reading logic that uses f.seek and serialization::readPod.
- Around line 728-752: The corrupt-cache early returns in pageContainsText leave
the search file open and searchHeaderReady set, which can break downstream cache
invalidation and retries. Before each std::nullopt return in this block, call
the same cleanup helper used elsewhere in the search flow to close/reset the
search state, and apply it consistently to the later seek/read/length failure
paths as well so no stale file or header state is retained.
- Around line 560-567: Reject page indexes that are beyond the header’s page
count before computing or reading the LUT entry in Section::deserializePage. Add
an explicit upper-bound guard for currentPage against the actual page count used
by this method (or load that count locally from the header if pageCount may not
be initialized), so Page::deserialize() cannot treat later metadata as a page
entry.
In `@lib/Epub/Epub/Section.h`:
- Around line 126-134: Expose a chapter-boundary reset in the pageContainsText
API so carried KMP state cannot span chapter breaks inside the same spine.
Update the Section::pageContainsText contract and any related call sites/types
to accept a boundary/reset signal or equivalent marker, and ensure matched is
reset whenever a chapter transition occurs, not just at scan start, spine
change, or wrap. Locate the change around Section::pageContainsText and the code
that threads matched through page scans, and make the reset behavior explicit in
the public interface and its callers.
In `@lib/I18n/translations/polish.yaml`:
- Line 278: The Polish no-results translation string is misspelled in the
STR_NO_SEARCH_RESULTS entry. Update the value in the translation file from the
incorrect wording to the correct “Brak pasujących wyników.” so the search
empty-state text is rendered properly.
In `@lib/I18n/translations/ukrainian.yaml`:
- Line 285: The in-progress search message has a typo in the Ukrainian
translation; update the STR_SEARCHING_BOOK entry in ukrainian.yaml to use the
correct word form “книзі” instead of “кнізі”. Keep the rest of the translation
unchanged and verify the key still reads naturally in the search status text.
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 3424-3437: The search route in EpubReaderActivity::search should
not use footnote-preview pagination when activeFootnotePreview is true, because
section->currentPage may point to the _fn_ preview cache instead of the normal
chapter cache. Update the initiatedFromPage calculation so it is derived from
the normal chapter pagination only when building
EpubReaderSearchActivity::SearchRoute::make, and keep find-next logic aligned
with the real chapter page rather than the preview page.
- Around line 4516-4626: `EpubReaderActivity::drawSearchHighlights` only
searches the current page text, so it misses search hits that started on the
previous page and continue onto this one. Update the matching logic to
incorporate cross-page search state, likely by using the same carried-over match
information that `pageContainsText()` relies on, and highlight the current page
portion when a boundary-spanning match lands here. Keep the local normalization
and rendering flow, but extend the match-range detection so
`searchHighlightMatchRanges` can include results that begin before the current
page while still referencing `lastSearchQuery`, `lastSearchResultSpine`, and
`lastSearchResultPage` for locating the active result.
- Around line 3400-3415: The search flow in EpubReaderActivity should pause
reading-pace tracking while the keyboard/search UI is active, since the current
startActivityForResult path calls KeyboardEntryActivity and then
EpubReaderSearchActivity without the pause/resume handling used elsewhere. Wrap
the search launch and its callback in pauseReadingPaceTimer() and
resumeReadingPaceTimer() around the existing logic, using the same pattern as
other reader UI transitions in this class so the timer is suspended during user
input and book-wide search.
- Around line 1073-1092: The viewport calculation in calculateReaderViewport()
must match the render path used by computeReaderViewportLayout(), since search
cache pagination is currently drifting from render pagination. Update
calculateReaderViewport() to include the same publisher-page left margin, top
clock reservation, and status-bar padding logic as render(), so
launchBookSearch() applies match.page against the same layout that was used to
build the cache.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21af0bb8-b52e-4c40-889d-c4617ba224f6
📒 Files selected for processing (42)
docs/file-formats.mddocs/search-architecture.mdlib/Epub/Epub/AsciiCase.hlib/Epub/Epub/Page.cpplib/Epub/Epub/Page.hlib/Epub/Epub/Section.cpplib/Epub/Epub/Section.hlib/Epub/Epub/css/CssParser.cpplib/I18n/translations/belarusian.yamllib/I18n/translations/catalan.yamllib/I18n/translations/czech.yamllib/I18n/translations/danish.yamllib/I18n/translations/dutch.yamllib/I18n/translations/english.yamllib/I18n/translations/finnish.yamllib/I18n/translations/french.yamllib/I18n/translations/german.yamllib/I18n/translations/hebrew.yamllib/I18n/translations/hungarian.yamllib/I18n/translations/italian.yamllib/I18n/translations/kazakh.yamllib/I18n/translations/lithuanian.yamllib/I18n/translations/polish.yamllib/I18n/translations/portuguese.yamllib/I18n/translations/romanian.yamllib/I18n/translations/russian.yamllib/I18n/translations/slovak.yamllib/I18n/translations/slovenian.yamllib/I18n/translations/spanish.yamllib/I18n/translations/swedish.yamllib/I18n/translations/turkish.yamllib/I18n/translations/ukrainian.yamllib/I18n/translations/valencian.yamllib/I18n/translations/vietnamese.yamlsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderMenuActivity.cppsrc/activities/reader/EpubReaderMenuActivity.hsrc/activities/reader/EpubReaderSearchActivity.cppsrc/activities/reader/EpubReaderSearchActivity.hsrc/activities/reader/ReaderUtils.hsrc/activities/util/KeyboardEntryActivity.cpp
…pageContainsText API
…mer in EpubReaderActivity
The search feature was previously checking the EPUB's Table of Contents (TOC) on every single page scan to see if it landed on a "chapter boundary." This resulted in significant I/O overhead because reading a TOC entry involved opening and seeking within the section cache file on the SD card. However, since a TOC anchor inside a single file (spine item) is not actually a discontinuity in the text flow, this entire operation was unnecessary.
Moves the KMP compilation, normalization, and matching state out of Section and into a dedicated SearchMatcher class to simplify state tracking across chapter boundaries.
… KMP - Replaces per-page SD card scanning with `Section::scanForward` which batches LUT reads and streams text records sequentially. - Deduplicates KMP highlight loop in `EpubReaderActivity` by sharing `SearchMatcher`. - Simplifies `EpubReaderSearchActivity` state machine and implements cooperative 50-page chunking to maintain device responsiveness during long searches.
Change the drawSearchHighlights priming pass to only scan the immediately preceding page instead of the entire chapter up to the current page. The KMP state machine can only carry over up to MAX_QUERY_BYTES (255) anyway, so one page is enough, changing O(N) chapter scanning to O(1).
Introduce a lightweight UTF-8 decoder into SearchMatcher to strip common Latin diacritics during KMP execution. This allows base ASCII queries (e.g., "cafe") to successfully match text with accented characters (e.g., "café"). To preserve accurate highlight rendering boundaries when multi-byte UTF-8 sequences match single-byte ASCII pattern characters, SearchMatcher now maintains a ring buffer of raw byte widths and returns the total byte length of the completed match.
The intermediate search progress frames correctly use HalDisplay::FAST_REFRESH to avoid screen flashing during long scans. However, when the search transitions to a terminal state (NotFound/Error), a FULL_REFRESH is needed to clear any accumulated ghosting from the progress updates before the user dismisses the overlay.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/Epub/Epub/SearchMatcher.cpp`:
- Around line 155-183: In SearchMatcher::feed, unsupported Unicode text is
currently converted to '?' and pending separator bytes are always folded into
the reported match width, which can create false matches and include leading
whitespace/hyphens in highlights. Update the normalization/matching path so
unsupported codepoints are skipped or otherwise excluded from matching instead
of becoming '?', keeping behavior aligned with normalizeSearchQuery, and only
accumulate separator bytes into the returned width once a real matched character
sequence has started. Use the existing symbols feed, stripLatinDiacritics,
pendingSeparatorBytes, matchByteWidths, and matched to keep the fix localized.
In `@src/activities/reader/EpubReaderSearchActivity.cpp`:
- Around line 220-235: Restore the full matcher state before retrying the
cache-repair scan in EpubReaderSearchActivity::scanForward handling. The current
retry path only resets matcher.matched, but scanForward() can also mutate
decoder/separator/width state, so the second attempt may be inconsistent. Save
the complete matcher state before the first scan, then fully restore it before
calling section.scanForward() again after section.resetForSpine(),
section.clearCache(), and ensureSectionLoaded().
- Around line 57-64: The query handling in
EpubReaderSearchActivity::EpubReaderSearchActivity currently truncates oversized
input before calling matcher.compile(), which can turn an invalid search into a
prefix search. Validate the input length first and reject queries that exceed
the buffer before copying anything, then only store and compile the query when
it fits; keep the fail-closed behavior in the constructor by checking the query
size around the existing strncpy/query buffer logic.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 3ffc6826-99dd-4bb9-88be-491f2e953b75
📒 Files selected for processing (7)
lib/Epub/Epub/SearchMatcher.cpplib/Epub/Epub/SearchMatcher.hlib/Epub/Epub/Section.cpplib/Epub/Epub/Section.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderSearchActivity.cppsrc/activities/reader/EpubReaderSearchActivity.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activities/reader/EpubReaderActivity.cpp
… conditionally in SearchMatcher::feed
…vent invalid truncated queries
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/activities/reader/EpubReaderSearchActivity.cpp (1)
59-62: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the length probe before validating the query.
strlen(query)walks to the first NUL before the 64-byte limit is enforced. Since this constructor is already designed to fail closed independently of the caller, use a bounded probe and reject when the input does not fit. This is adjacent to the earlier truncation fix.🐛 Proposed fix
- const size_t len = strlen(query); - if (len <= SearchMatcher::MAX_QUERY_BYTES) { - strncpy(this->query.data(), query, this->query.size() - 1); - this->query[this->query.size() - 1] = '\0'; + const size_t len = strnlen(query, this->query.size()); + if (len <= SearchMatcher::MAX_QUERY_BYTES) { + memcpy(this->query.data(), query, len); + this->query[len] = '\0';🤖 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 `@src/activities/reader/EpubReaderSearchActivity.cpp` around lines 59 - 62, The query validation in EpubReaderSearchActivity’s constructor still uses an unbounded strlen probe before applying the MAX_QUERY_BYTES check. Update the constructor logic to use a bounded length check on query and reject inputs that do not fit instead of scanning past the limit; keep the existing copy/truncation handling around this query assignment path consistent with SearchMatcher::MAX_QUERY_BYTES and this->query.lib/Epub/Epub/SearchMatcher.cpp (1)
168-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecompute separator width after KMP fallback.
Line 168 folds
pendingSeparatorBytesinto the current byte before lines 177-180 decide whether that byte extends the current partial match or starts a new one after fallback. For queryabovera---ab, the secondabecomes the new match start but still carries the separators from the failed prefix, so the returned raw width/highlight includes text before the actual match. This is the same leading-separator class previously flagged.🐛 Proposed fix
- uint8_t totalBytesForThisChar = utf8BytesConsumed + pendingSeparatorBytes; + const uint8_t separatorBytes = pendingSeparatorBytes; pendingSeparatorBytes = 0; const uint8_t value = static_cast<uint8_t>(norm); - // Track raw byte width of this valid character in the circular buffer - matchByteWidths[widthBufferHead] = totalBytesForThisChar; - widthBufferHead = (widthBufferHead + 1) % MAX_QUERY_BYTES; - while (matched > 0 && value != pattern[matched]) { matched = prefix[matched - 1]; } if (value == pattern[matched]) { + const uint8_t totalBytesForThisChar = utf8BytesConsumed + (matched > 0 ? separatorBytes : 0); + // Track raw byte width of this valid character in the circular buffer + matchByteWidths[widthBufferHead] = totalBytesForThisChar; + widthBufferHead = (widthBufferHead + 1) % MAX_QUERY_BYTES; ++matched;🤖 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 `@lib/Epub/Epub/SearchMatcher.cpp` around lines 168 - 180, The KMP fallback in SearchMatcher::match is reusing pendingSeparatorBytes as part of the current character before the match state is finalized, so a restarted match can incorrectly inherit separators from the failed prefix. Update the logic around totalBytesForThisChar, matchByteWidths, and the matched/prefix fallback so separator width is only committed to the actual matched span after the final match decision, and reset or reassign pendingSeparatorBytes when the match restarts.
🤖 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.
Duplicate comments:
In `@lib/Epub/Epub/SearchMatcher.cpp`:
- Around line 168-180: The KMP fallback in SearchMatcher::match is reusing
pendingSeparatorBytes as part of the current character before the match state is
finalized, so a restarted match can incorrectly inherit separators from the
failed prefix. Update the logic around totalBytesForThisChar, matchByteWidths,
and the matched/prefix fallback so separator width is only committed to the
actual matched span after the final match decision, and reset or reassign
pendingSeparatorBytes when the match restarts.
In `@src/activities/reader/EpubReaderSearchActivity.cpp`:
- Around line 59-62: The query validation in EpubReaderSearchActivity’s
constructor still uses an unbounded strlen probe before applying the
MAX_QUERY_BYTES check. Update the constructor logic to use a bounded length
check on query and reject inputs that do not fit instead of scanning past the
limit; keep the existing copy/truncation handling around this query assignment
path consistent with SearchMatcher::MAX_QUERY_BYTES and this->query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03d8b9b5-5b9a-46fe-93ee-d3d782c98346
📒 Files selected for processing (2)
lib/Epub/Epub/SearchMatcher.cppsrc/activities/reader/EpubReaderSearchActivity.cpp
- Maps ß to ss, æ to ae, and typographic ligatures (ff, fi, fl, ffi, ffl) to their multi-character equivalents. - Packs the folded characters into a single 32-bit integer returned by stripLatinDiacritics. - Unpacks and feeds each resulting byte iteratively to the KMP matcher state machine. - Attributes the full UTF-8 source byte width to the first mapped character in the sequence, allowing the match width to perfectly correspond to the original multi-byte sequence in the text without disrupting highlighting.
- Reflects that diacritic stripping and multi-character folding are now implemented. - Removes the 'future extension' item for Unicode case-fold support since it is now completed using zero-RAM code logic. - Updates the limitations to specify that case/diacritic folding covers ASCII and supported Latin characters.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/Epub/Epub/SearchMatcher.cpp (1)
171-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBreak partial matches on unsupported code points.
norm == 0currently leavesmatchedintact, so unsupported text can bridge a match even though only spaces/hyphens are documented as ignored. For example, queryabcan match acrossaЖb. Reset the active match state here.🐛 Proposed fix
if (norm == 0) { + matched = 0; + pendingSeparatorBytes = 0; + utf8BytesConsumed = 0; return 0; }🤖 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 `@lib/Epub/Epub/SearchMatcher.cpp` around lines 171 - 173, Reset the active match state in SearchMatcher when normalized value is zero so unsupported code points cannot bridge partial matches; in the norm == 0 branch of the matching logic, clear the current matched state before returning so only the documented ignored separators (spaces/hyphens) can be skipped.
🤖 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 `@lib/Epub/Epub/SearchMatcher.cpp`:
- Around line 199-215: The width accounting in SearchMatcher.cpp is using
per-emitted-byte widths, so matches that begin inside folded glyph expansions
can lose the original character width or collapse to zero. Update the
match-width tracking around the normalization/matching path by associating each
emitted normalized byte with its source codepoint width, then when a match
completes in the matching logic that updates matchByteWidths, widthBufferHead,
matched, and totalWidthReturn, sum unique source-codepoint widths instead of raw
byte widths so folded sequences like ß/fi still contribute once to the
highlighted span.
---
Duplicate comments:
In `@lib/Epub/Epub/SearchMatcher.cpp`:
- Around line 171-173: Reset the active match state in SearchMatcher when
normalized value is zero so unsupported code points cannot bridge partial
matches; in the norm == 0 branch of the matching logic, clear the current
matched state before returning so only the documented ignored separators
(spaces/hyphens) can be skipped.
🪄 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 Plus
Run ID: c1b4d02b-2f64-4d43-8758-04e2a792fe71
📒 Files selected for processing (2)
lib/Epub/Epub/SearchMatcher.cppsrc/activities/reader/EpubReaderSearchActivity.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Line 7: Reword the in-book search release note in CHANGELOG.md to avoid
claiming whole-word or word-boundary matching, since the current matcher still
does normalized substring matching and only supports hyphenation-aware behavior.
Update the description to emphasize that search finds words or phrases in the
current EPUB and handles words split across lines or pages by hyphenation,
without stating that matching fully respects word boundaries.
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 63-77: The query equivalence check in searchQueriesEquivalent is
only folding ASCII case, so it misses matches that SearchMatcher already
normalizes (for example accented variants), causing repeated searches to restart
incorrectly. Update this gate to compare the normalized query bytes produced by
the same normalization path used for searching, and use that normalized
representation to decide whether the new query is equivalent to the previous
one.
🪄 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 Plus
Run ID: 0e3341b0-4eb0-4cd7-ba61-42924e51fb88
📒 Files selected for processing (4)
CHANGELOG.mddocs/search-architecture.mdlib/Epub/Epub/SearchMatcher.cppsrc/activities/reader/EpubReaderActivity.cpp
The same-query check only folded ASCII case, so queries the matcher treats as identical (Latin diacritics, ligatures, hyphen/space fuzzing) were judged different and find-next restarted the scan instead of continuing. Expose SearchMatcher::queriesEquivalent() which compares queries by the same normalizeSearchQuery() path compile() uses, and call it from the reader so the gate can no longer drift from match behavior.
The matcher ran a plain KMP substring scan, so "cat" matched inside "category" even though the search feature advertises word-boundary matching. Gate matches on word boundaries: the leading boundary is checked at completion via a small per-character ring recording whether each significant byte was preceded by a word char; the trailing boundary is resolved on the next significant byte (a word char rejects, a boundary confirms) or at the end of a record, which is itself a word boundary since records hold whole space-separated words with no trailing separator. A completed match is held as tentative until its trailing boundary is known; the span is carried in the matcher so it survives the matcher copies the chunked scan makes. Hyphenation stays fuzzy: dropped bytes (hyphens, unmapped codepoints, line-break separator spaces) remain transparent for boundary purposes, and a boundary is only required on a pattern edge that is itself a word char (regex \b semantics).
Add a GoogleTest suite that drives SearchMatcher through the same feeding contract Section::scanForward uses (page-boundary space, tentative/confirm protocol, end-of-record and empty-record boundaries), so the tests exercise the real scan integration rather than feed() alone. Covers substring rejection, whole-word spans, punctuation/record-edge boundaries, regex-\b query edges, phrases, case/diacritic folding, the ss-expansion boundary guard, hard and line-break hyphenation, cross-page matches, CJK transparency, KMP self-overlap, and first-occurrence selection.
The cmake target omitted lib/EpdFont/EpdFontFamily.cpp, so it failed to link (undefined EpdFontFamily::getTextDimensions / getFallbackCodepoint) even though run_differential_rounding_test.sh compiles that source. Add it so the whole gtest suite builds, which the new CI job requires.
Add a unit-tests job that configures, builds, and runs the CMake/ GoogleTest suite under test/ with ctest, and make it a required check via test-status. Nothing ran these suites in CI before, so regressions in the matcher, parsers, and font/rounding logic could land unnoticed.
This search branch had accumulated an unrelated refinement of the existing "/Read folder" move feature, spanning commits 9656a6c and 7d9717c. Restore main's behavior so the branch stays scoped to search: - BookMoveUtils::buildReadFolderDestination no longer returns "" when no free "name (N)" slot exists (reverted to main). - Drop the now-unneeded empty-destination guards in BookActions.cpp and EpubReaderActivity::moveFinishedBookToReadFolder. The dead duplicate of the helper that 9656a6c removed from EpubReaderActivity stays removed: it is unused and behavior-neutral, so re-adding it would only restore dead code.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/Epub/Epub/SectionSearch.cpp (1)
143-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMap validated short reads to
IoError, notCorruptCache.Both reads happen after the offset/length checks have already proven the record is structurally inside the cache file. If either read still comes up short, that is the same transient I/O class you already return for the LUT read above, and
EpubReaderSearchActivitywill currently treat it as corruption and rebuild/drop a valid cache.Suggested fix
- if (file.read(reinterpret_cast<uint8_t*>(&remaining), sizeof(remaining)) != sizeof(remaining) || - remaining > lutOffset - searchTextOffset - sizeof(uint32_t)) { + if (file.read(reinterpret_cast<uint8_t*>(&remaining), sizeof(remaining)) != sizeof(remaining)) { + LOG_ERR("SCT", "Search failed: could not read text record length"); + closeSearchState(); + return {ScanStatus::IoError, -1}; + } + if (remaining > lutOffset - searchTextOffset - sizeof(uint32_t)) { LOG_ERR("SCT", "Search failed: invalid text record length"); closeSearchState(); return {ScanStatus::CorruptCache, -1}; } @@ - if (file.read(buffer.data(), chunkSize) != chunkSize) { - LOG_ERR("SCT", "Search failed: truncated text record"); + if (file.read(buffer.data(), chunkSize) != chunkSize) { + LOG_ERR("SCT", "Search failed: could not read text record"); closeSearchState(); - return {ScanStatus::CorruptCache, -1}; + return {ScanStatus::IoError, -1}; }Also applies to: 183-186
🤖 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 `@lib/Epub/Epub/SectionSearch.cpp` around lines 143 - 146, In SectionSearch::searchText, the short-read checks for the text record payload are currently mapped to CorruptCache even though the offsets/lengths have already validated the record, so change those failure paths to return IoError like the LUT read handling above. Update the read-and-validate logic around the remaining-text length read and the subsequent payload read so any unexpected short read is treated as a transient I/O failure, preserving the existing LOG_ERR context but using the same error class as the earlier file reads.
🤖 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 @.github/workflows/ci.yml:
- Around line 122-124: The checkout step is not hardened: actions/checkout is
still referenced by tag and persist-credentials is not disabled. Update the
checkout configuration in the CI workflow to set persist-credentials to false
and pin actions/checkout to a full commit SHA in this step so the location is
easy to find and the token is not retained in .git/config.
In `@test/search_matcher/SearchMatcherTest.cpp`:
- Around line 210-214: Add a regression case in
SearchMatcherQuery::EquivalenceIgnoresCaseAndFuzz to cover Unicode normalization
equivalence, since it currently only checks ASCII case and hyphen fuzz. Extend
the test to assert SearchMatcher::queriesEquivalent returns true for a
composed/decomposed NFC/NFD pair so the normalization contract used by find-next
is locked in.
---
Outside diff comments:
In `@lib/Epub/Epub/SectionSearch.cpp`:
- Around line 143-146: In SectionSearch::searchText, the short-read checks for
the text record payload are currently mapped to CorruptCache even though the
offsets/lengths have already validated the record, so change those failure paths
to return IoError like the LUT read handling above. Update the read-and-validate
logic around the remaining-text length read and the subsequent payload read so
any unexpected short read is treated as a transient I/O failure, preserving the
existing LOG_ERR context but using the same error class as the earlier file
reads.
🪄 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 Plus
Run ID: a2589c1f-6647-433a-8134-1c23bb64f911
📒 Files selected for processing (11)
.github/workflows/ci.ymlCHANGELOG.mddocs/search-architecture.mdlib/Epub/Epub/SearchMatcher.cpplib/Epub/Epub/SearchMatcher.hlib/Epub/Epub/SectionSearch.cppsrc/activities/reader/EpubReaderActivity.cpptest/CMakeLists.txttest/differential_rounding/CMakeLists.txttest/search_matcher/CMakeLists.txttest/search_matcher/SearchMatcherTest.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/activities/reader/EpubReaderSearchActivity.cpp`:
- Around line 328-340: Seed lastProgressRepaintMs after the initial 0% repaint
in EpubReaderSearchActivity::onEnter or the first search-state paint path so the
timing gate starts from that update instead of 0. Update the search progress
repaint logic in the state == SearchState::Searching block to set
lastProgressRepaintMs when the initial percentage is drawn, ensuring
requestUpdate in the progress path does not immediately trigger a second
FAST_REFRESH on the next nonzero percent change.
🪄 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 Plus
Run ID: bb3e6b29-2416-4163-9b66-d44153d329ae
📒 Files selected for processing (6)
.github/workflows/ci.ymldocs/search-architecture.mdlib/Epub/Epub/Section.hlib/Epub/Epub/SectionSearch.cppsrc/activities/reader/EpubReaderSearchActivity.cppsrc/activities/reader/EpubReaderSearchActivity.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What is the goal of this PR?
What changes are included?
crossink_search_demo.mp4
I've tested searching with the X4 emulator and flashed the build onto my X3 successfully.
Additional Context
See In-Book Search Architecture for more context, but I've copied the trade-offs section here:
Indexingpopup is shown while it runs so a cold-cache search does not appear frozen.Section::scanForward()reports the match's byte span within the page's search-text record, andSearchHighlightermaps that span to the page's words at render time. The match is located once by the scan rather than re-derived by a second matcher, so the highlighter never re-normalizes text or re-reads the cache. A match that began on the previous page reports a span clamped to the page start, so its visible tail still highlights without re-scanning the previous page."cat"does not match inside"category"(see Matching algorithm). Other punctuation-glyph differences (curly vs straight quotes, em dash, the ellipsis character vs three dots) are not normalized and can still cause a miss, and a match split across a chapter (spine) boundary is not joined.Summary by CodeRabbit