feat: Add Book Info screen, richer metadata, and safer file-browser controls#1342
Closed
jpirnay wants to merge 18 commits into
Closed
feat: Add Book Info screen, richer metadata, and safer file-browser controls#1342jpirnay wants to merge 18 commits into
jpirnay wants to merge 18 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds series and description metadata end-to-end: OPF parsing, cache version bump, Epub getters, RecentBooks storage/serialization and call-site updates, a BookInfo UI and FileBrowser integration, theme rendering updates, input gating in ConfirmationActivity, and safer string deserialization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileBrowser as FileBrowser Activity
participant BookInfo as BookInfo Activity
participant FileSystem as File System
participant Parser as EPUB/XTC Parser
participant Renderer as Renderer
User->>FileBrowser: Right-press on EPUB/XTC entry
FileBrowser->>BookInfo: instantiate with filePath
BookInfo->>BookInfo: onEnter() → loadData()
BookInfo->>FileSystem: open file / read bytes
FileSystem-->>BookInfo: file data/handle
BookInfo->>Parser: parse metadata (title, author, series, seriesIndex, description)
Parser-->>BookInfo: metadata
BookInfo->>FileSystem: generate/read cover thumbnail
FileSystem-->>BookInfo: thumbnail
BookInfo->>Renderer: render(metadata + thumbnail + file size)
Renderer-->>User: display book info
User->>BookInfo: press Back
BookInfo->>FileBrowser: close / return
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/activities/home/BookInfoActivity.h (1)
3-5: Add direct#include <utility>to this header.The constructor on line 28 uses
std::move, which requires<utility>. Currently, the header relies on a transitive include fromActivity.h, which is brittle and violates explicit dependency requirements.♻️ Proposed fix
`#include` <string> +#include <utility> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/home/BookInfoActivity.h` around lines 3 - 5, Add an explicit direct include of <utility> to BookInfoActivity.h because the constructor (which calls std::move) relies on std::move; update the header by adding `#include` <utility> near the other includes so the use of std::move in the BookInfoActivity constructor is not dependent on transitive includes from Activity.h.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 16-68: stripHtml currently treats every '<' as the start of an
HTML tag, which removes literal angle-bracket text like "2 < 3"; update
stripHtml to only enter tag mode when the '<' is actually a tag start: when
encountering '<' in the loop inside stripHtml, peek ahead to the next non-space
character in html and if that character is alphabetic or one of '/', '!', '?'
(i.e., likely a tag or declaration) then set inTag = true and preserve the
word-separating space logic; otherwise append the literal '<' to result and do
not set inTag. Use the existing variables html, result, inTag and the loop index
i to implement the lookahead without changing other behavior.
- Around line 442-444: The parser currently appends unbounded data to
self->description when in state IN_BOOK_DESCRIPTION; add a hard cap (e.g.
define/use MAX_DESCRIPTION_LENGTH) and during parsing ensure you only append up
to the remaining allowed bytes: check self->description.size(), compute
remaining = MAX_DESCRIPTION_LENGTH - current_size, and append at most
min(remaining, len); if remaining is zero skip further appends (and optionally
mark a truncated flag). Update the IN_BOOK_DESCRIPTION handling to enforce this
cap so the parser never grows description past the safe limit.
- Around line 257-267: The parser currently treats any meta with property
"belongs-to-collection" and "group-position" independently; update the logic in
ContentOpfParser (the block handling metaProperty) to honor the refines
attribute and the target collection's collection-type: when you see a
"group-position" meta, resolve its refines="#id" (or similar) and only set
self->seriesIndex and state = IN_BOOK_SERIES_INDEX if that refines id matches a
previously parsed collection element that has collection-type="series"; likewise
when handling "belongs-to-collection" store the collection id and its
collection-type so that "group-position" can be matched to the same collection
id (instead of capturing the first occurrence blindly), and ensure you only set
self->series and state = IN_BOOK_SERIES for collections that are actually typed
as series.
In `@src/activities/home/FileBrowserActivity.cpp`:
- Around line 156-170: The code mutates the member variable basepath by
appending a trailing '/' even when opening a file; instead, keep basepath
immutable for the file-open branch: construct a local string (e.g. fullPath)
that is basepath plus a '/' only if needed and plus the entry (for files use
entry as-is, for directories keep the existing mutation+loadFiles logic), then
call onSelectBook(fullPath) without changing basepath; leave the directory
branch using basepath += entry.substr(...) / loadFiles() / selectorIndex = 0 /
requestUpdate() as-is.
In `@src/RecentBooksStore.cpp`:
- Around line 23-24: The migration is constructing RecentBook with the wrong
field order so old entries put coverBmpPath into series; update the migration
and RecentBooksStore::addBook usage so RecentBook is built as {path, title,
author, series, coverBmpPath} (not {path, title, author, coverBmpPath}), and
when handling legacy 4-field records map them to {path, title, author, ""
/*series*/, legacyCoverBmpPath} so the thumbnail stays in coverBmpPath and
series remains empty; search for RecentBook construction in RecentBooksStore
(the block that currently passes four values) and fix the argument positions and
legacy mapping accordingly.
---
Nitpick comments:
In `@src/activities/home/BookInfoActivity.h`:
- Around line 3-5: Add an explicit direct include of <utility> to
BookInfoActivity.h because the constructor (which calls std::move) relies on
std::move; update the header by adding `#include` <utility> near the other
includes so the use of std::move in the BookInfoActivity constructor is not
dependent on transitive includes from Activity.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e5681f5-ace3-4bba-b1c7-4c15f208def8
📒 Files selected for processing (21)
lib/Epub/Epub.cpplib/Epub/Epub.hlib/Epub/Epub/BookMetadataCache.cpplib/Epub/Epub/BookMetadataCache.hlib/Epub/Epub/parsers/ContentOpfParser.cpplib/Epub/Epub/parsers/ContentOpfParser.hlib/I18n/translations/english.yamlsrc/JsonSettingsIO.cppsrc/RecentBooksStore.cppsrc/RecentBooksStore.hsrc/activities/home/BookInfoActivity.cppsrc/activities/home/BookInfoActivity.hsrc/activities/home/FileBrowserActivity.cppsrc/activities/home/RecentBooksActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/TxtReaderActivity.cppsrc/activities/reader/XtcReaderActivity.cppsrc/activities/util/ConfirmationActivity.cppsrc/activities/util/ConfirmationActivity.hsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/reader/XtcReaderActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/home/FileBrowserActivity.cppsrc/activities/home/BookInfoActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/reader/XtcReaderActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/home/RecentBooksActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/JsonSettingsIO.cppsrc/activities/util/ConfirmationActivity.cppsrc/activities/reader/TxtReaderActivity.cppsrc/activities/home/FileBrowserActivity.cppsrc/components/themes/BaseTheme.cpplib/Epub/Epub/parsers/ContentOpfParser.cpplib/Epub/Epub.cpplib/Epub/Epub/BookMetadataCache.cppsrc/activities/home/BookInfoActivity.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/reader/XtcReaderActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/home/RecentBooksActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/JsonSettingsIO.cppsrc/activities/util/ConfirmationActivity.cppsrc/activities/reader/TxtReaderActivity.cppsrc/activities/home/FileBrowserActivity.cppsrc/components/themes/BaseTheme.cpplib/Epub/Epub/parsers/ContentOpfParser.cpplib/Epub/Epub.cpplib/Epub/Epub/BookMetadataCache.cppsrc/activities/home/BookInfoActivity.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-02-16T22:25:35.674Z
Learnt from: whyte-j
Repo: crosspoint-reader/crosspoint-reader PR: 733
File: lib/I18n/I18nKeys.h:271-272
Timestamp: 2026-02-16T22:25:35.674Z
Learning: In the crosspoint-reader i18n system (lib/I18n/), missing translation keys in non-English YAML files are automatically filled with English strings at build time by the gen_i18n.py script. All generated string arrays (STRINGS_EN, STRINGS_ES, etc.) have identical lengths, so runtime array indexing is safe without per-string fallback logic. The system prints "INFO: '{key}' missing in {lang_code}, using English fallback" during code generation when this occurs.
Applied to files:
lib/I18n/translations/english.yaml
📚 Learning: 2026-02-23T22:11:42.181Z
Learnt from: ariel-lindemann
Repo: crosspoint-reader/crosspoint-reader PR: 1133
File: lib/I18n/translations/finnish.yaml:92-92
Timestamp: 2026-02-23T22:11:42.181Z
Learning: In translation YAML files located in lib/I18n/translations/, literal percent signs (%) in strings (e.g., STR_HIDE_BATTERY, STR_GO_TO_PERCENT, STR_PERCENT_STEP_HINT) do not require escaping as %% and this convention is consistent across all language translations. Ensure that this pattern is followed for all YAML files in this directory to avoid introducing incorrect formatting markers in localized strings.
Applied to files:
lib/I18n/translations/english.yaml
📚 Learning: 2026-02-19T17:46:36.345Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:36.345Z
Learning: In src/activities/reader/EpubReaderActivity.cpp's renderContents() method, when uncached images exist, Phase 1 intentionally calls displayBuffer(forceFullRefresh) to perform a HALF_REFRESH (if needed), while Phase 2 intentionally calls renderer.displayBuffer() directly without forceFullRefresh. This is by design: Phase 1's refresh clears the screen properly to prevent ghosting, so Phase 2 can use a faster refresh mode for better performance. The grayscale anti-aliasing is handled separately after renderContents() via displayGrayBuffer().
Applied to files:
src/activities/home/RecentBooksActivity.cppsrc/activities/home/BookInfoActivity.cpp
📚 Learning: 2026-02-19T17:46:26.871Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:26.871Z
Learning: In EpubReaderActivity.cpp, renderContents() behavior is intentional: if uncached images exist, Phase 1 should call displayBuffer(forceFullRefresh) to perform a HALF_REFRESH and clear ghosting; Phase 2 should call renderer.displayBuffer() without forceFullRefresh for a faster refresh. Grayscale anti-aliasing is applied separately after renderContents() via displayGrayBuffer(). This guidance applies specifically to this file and situation.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/ConfirmationActivity.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
src/activities/home/FileBrowserActivity.cppsrc/activities/home/BookInfoActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/home/FileBrowserActivity.cpp
📚 Learning: 2026-03-02T21:18:25.387Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:137-150
Timestamp: 2026-03-02T21:18:25.387Z
Learning: In ChapterHtmlSlimParser.cpp, pendingAnchorId is a single string (not a vector) because EPUB footnote targets are virtually always block-level elements (<aside>, <p>, <div>) that trigger startNewTextBlock() and flush the anchor before overwrite can occur. Multiple consecutive inline IDs would resolve to the same completedPageCount anyway, and vector allocation overhead is unnecessary for ESP32-C3. Only inline IDs in practice are footnote back-references (<a id="ref1">), which are never navigation targets.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-02T09:58:47.539Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:238-246
Timestamp: 2026-03-02T09:58:47.539Z
Learning: In EPUB section files, typical sections contain approximately 20 footnote/note anchors plus a few sub-chapter anchors (max ~30-50 total) even in large chapters. While the code tracks all HTML id attributes, realistic anchor counts per section remain well under 255, making uint8_t sufficient for 99% of EPUBs, though uint16_t provides extra safety margin for edge cases.
Applied to files:
lib/Epub/Epub/BookMetadataCache.cpp
🔇 Additional comments (14)
src/activities/util/ConfirmationActivity.cpp (1)
12-14: Good neutral-state arming before dialog input.Resetting
inputArmedon enter and waiting for a clean input frame closes the inherited press/release loophole without changing the actual confirm/cancel handling.Also applies to: 59-71
src/RecentBooksStore.h (1)
35-36: Good API pressure for series propagation.Making
seriesa requiredaddBookargument forces each reader entry point to decide explicitly whether it has series metadata instead of silently dropping it.src/components/themes/lyra/LyraTheme.cpp (1)
490-509: Centering now accounts for the optional series line.Including
seriesHeightintotalBlockHeightkeeps the title block visually stable when series metadata is present.lib/Epub/Epub/BookMetadataCache.h (1)
17-19:⚠️ Potential issue | 🟠 MajorConfirm that
readStringvalidates incoming lengths before resize/allocation to prevent oversized allocations from malformed cache files.The
readStringimplementation inlib/Serialization/Serialization.h(lines 39-50 for both overloads) directly callss.resize(len)on auint32_tread from disk without any length bounds check. The newdescriptionfield atBookMetadataCache.cpp:395deserializes untrusted EPUB metadata via this function, creating a potential DoS vector if the cache file is malformed or tampered with.descriptioncan legitimately be much larger thantitleorauthorand should not be allowed to allocate unbounded memory.⛔ Skipped due to learnings
Learnt from: Uri-Tauber Repo: crosspoint-reader/crosspoint-reader PR: 1245 File: lib/Epub/Epub/Section.cpp:277-308 Timestamp: 2026-03-02T10:14:25.057Z Learning: In the crosspoint-reader codebase, `serialization::readString` is used at approximately 30 call sites (BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers, etc.) and performs unbounded `std::string::resize(len)` directly from disk data. Per-call-site validation is avoided in favor of potential future hardening at the `readString` source itself. Section cache files are versioned (SECTION_FILE_VERSION) and invalidated on parameter mismatches, reducing corruption risk for self-written cache files.src/JsonSettingsIO.cpp (1)
304-304: LGTM!The new
seriesfield serialization/deserialization follows the established pattern for otherRecentBookfields. The default empty string fallback ensures backward compatibility with existing JSON files that lack theserieskey.Also applies to: 329-329
lib/I18n/translations/english.yaml (1)
344-347: LGTM!The new translation keys follow the established naming conventions and provide appropriate English text. Based on learnings, the
gen_i18n.pyscript will automatically fill these keys with English fallback strings for non-English locales that haven't yet added translations.lib/Epub/Epub.h (1)
54-56: LGTM!The new accessor declarations follow the established pattern of existing metadata getters (
getTitle,getAuthor,getLanguage), maintaining consistent return types and naming conventions.src/activities/reader/TxtReaderActivity.cpp (1)
57-57: LGTM!The
addBookcall correctly adapts to the new 5-parameter signature. Empty strings for author, series, and thumbnail are appropriate since TXT files lack embedded metadata.src/activities/home/RecentBooksActivity.cpp (1)
103-109: LGTM!The subtitle composition logic cleanly handles all author/series combinations with appropriate fallbacks. The rendering layer (per
BaseTheme::drawList) will truncate long strings with ellipsis, so no overflow concerns.lib/Epub/Epub/BookMetadataCache.cpp (2)
12-12: Cache version bump correctly invalidates stale caches.Incrementing
BOOK_CACHE_VERSIONfrom 5 to 6 ensures existing cache files without the new metadata fields will be rebuilt, preventing deserialization mismatches.
118-121: LGTM!The serialization changes are well-structured:
metadataSizecorrectly accounts for 8 string fields (5 existing + 3 new) with their length prefixes- Write order (lines 136-138) matches read order (lines 393-395)
- The new fields are appended after existing ones, maintaining structural consistency
Also applies to: 136-138, 393-395
lib/Epub/Epub.cpp (2)
80-82: LGTM!The metadata propagation from
opfParsertobookMetadatafollows the established pattern for existing fields (title, author, language).
522-544: LGTM!The new getters follow the identical safe pattern established by
getTitle(),getAuthor(), andgetLanguage()— using a static empty string fallback when the cache is unavailable, ensuring safe reference returns.src/activities/reader/EpubReaderActivity.cpp (1)
106-108: LGTM!The series string construction correctly formats as "Series
#Index" (matching the PR screenshots like "The Lost Fleet#1.0") and gracefully handles edge cases where either series or seriesIndex is empty.
Contributor
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)
src/RecentBooksStore.cpp (1)
112-155:⚠️ Potential issue | 🔴 Critical
serialization::readStringpermits unbounded allocation from disk data—corruptrecent.bincan trigger DOS on startup.The migration functions deserialize multiple strings directly from disk via
serialization::readString, which reads a uint32_t length and unconditionally resizes the string to that size (lib/Serialization/Serialization.h:46–50). A corrupt or adversarially craftedrecent.binwith an extremely large length field can exhaust memory and crash the application during load. Add a maximum length bound enforced withinreadStringbefore deserializing untrusted files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecentBooksStore.cpp` around lines 112 - 155, The code currently calls serialization::readString directly from untrusted disk data in RecentBooksStore::loadFromBinaryFile which allows unbounded allocation; fix by adding a safe length check inside serialization::readString (introduce a MAX_STRING_LENGTH constant, read the uint32_t length with readPod, verify length <= MAX_STRING_LENGTH and refuse/return error if not) and update readString to surface failure instead of unconditionally resizing; then update RecentBooksStore::loadFromBinaryFile to handle a failed readString (abort loading the file or skip the entry and safely discard the rest) so corrupt/large lengths in recent.bin cannot trigger out-of-memory allocation.
♻️ Duplicate comments (1)
lib/Epub/Epub/parsers/ContentOpfParser.cpp (1)
23-36:⚠️ Potential issue | 🟡 MinorLiteral comparison operators still get stripped here.
Lines 25-28 still treat
x < yas a tag because the lookahead skips spaces and then accepts any following letter, and Line 35 drops a literal>even wheninTagis false. Escaped comparison text in descriptions will still get mangled.💡 Tighten the tag heuristic
- size_t j = i + 1; - while (j < html.size() && html[j] == ' ') ++j; + const size_t j = i + 1; if (j < html.size() && (isalpha(static_cast<unsigned char>(html[j])) || html[j] == '/' || html[j] == '!' || html[j] == '?')) { inTag = true; // Ensure words don't merge when a tag is removed if (!result.empty() && result.back() != ' ') result += ' '; } else { result += c; } - } else if (c == '>') { + } else if (c == '>' && inTag) { inTag = false; + } else if (c == '>') { + result += c; } else if (!inTag) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp` around lines 23 - 36, The current tag detection in ContentOpfParser.cpp misclassifies expressions like "x < y" because it skips spaces when peeking after '<' and also unconditionally swallows '>' characters; update the heuristic in the block that inspects c == '<' to require the immediate next character (no skipping of spaces) be a tag-start character (use html[i+1] after bounds check) before setting inTag and inserting a separating space into result, and change the c == '>' branch to only toggle inTag = false and drop the '>' when inTag is true (otherwise append '>' to result); reference variables/functions: c, i, j, html, inTag, result.
🧹 Nitpick comments (1)
src/RecentBooksStore.cpp (1)
43-52: Addseriesparameter toupdateBook()for consistency.
addBook()persistsseries, butupdateBook()doesn't accept it. While current call sites only useupdateBook()to clear the thumbnail path on generation failure, the signature inconsistency means future metadata refresh code could not update the series field on existing entries.♻️ Suggested change
-void RecentBooksStore::updateBook(const std::string& path, const std::string& title, const std::string& author, - const std::string& coverBmpPath) { +void RecentBooksStore::updateBook(const std::string& path, const std::string& title, const std::string& author, + const std::string& series, const std::string& coverBmpPath) { auto it = std::find_if(recentBooks.begin(), recentBooks.end(), [&](const RecentBook& book) { return book.path == path; }); if (it != recentBooks.end()) { RecentBook& book = *it; book.title = title; book.author = author; + book.series = series; book.coverBmpPath = coverBmpPath; saveToFile(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecentBooksStore.cpp` around lines 43 - 52, The updateBook signature and body in RecentBooksStore::updateBook must accept and persist the series field like addBook does: add a const std::string& series parameter to RecentBooksStore::updateBook, set book.series = series inside the existing branch that updates title/author/coverBmpPath, and ensure saveToFile() is still called; also update any callers of updateBook to pass through the series value (or empty string if unchanged) so existing entries can have their series metadata refreshed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 198-200: The parser currently re-enters IN_BOOK_DESCRIPTION for
every dc:description and appends into the single self->description, causing
multiple localized/alternate descriptions to be concatenated; change the
conditional that sets self->state to IN_BOOK_DESCRIPTION so it only does so when
no description has been captured yet (e.g., check self->description is
null/empty), mirroring the guard used for dc:title, so additional dc:description
elements are ignored rather than appended; update the block that tests
(self->state == IN_METADATA && strcmp(name, "dc:description") == 0) to include
this guard and leave existing behavior for the first description unchanged.
- Around line 253-259: The series and seriesIndex assignments accept unbounded
OPF data via metaContent (and text nodes); before assigning to self->series and
self->seriesIndex in ContentOpfParser (the branches where strcmp(metaName,
"calibre:series") and strcmp(metaName, "calibre:series_index") are checked),
truncate/cap the incoming metaContent to the same safe maximum used for
description parsing (i.e., apply the same small-cap/length-limit helper you used
for description) and then assign the capped string to self->series and
self->seriesIndex; apply the same capping fix to the other similar assignment
sites noted (the blocks around the other ranges mentioned).
---
Outside diff comments:
In `@src/RecentBooksStore.cpp`:
- Around line 112-155: The code currently calls serialization::readString
directly from untrusted disk data in RecentBooksStore::loadFromBinaryFile which
allows unbounded allocation; fix by adding a safe length check inside
serialization::readString (introduce a MAX_STRING_LENGTH constant, read the
uint32_t length with readPod, verify length <= MAX_STRING_LENGTH and
refuse/return error if not) and update readString to surface failure instead of
unconditionally resizing; then update RecentBooksStore::loadFromBinaryFile to
handle a failed readString (abort loading the file or skip the entry and safely
discard the rest) so corrupt/large lengths in recent.bin cannot trigger
out-of-memory allocation.
---
Duplicate comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 23-36: The current tag detection in ContentOpfParser.cpp
misclassifies expressions like "x < y" because it skips spaces when peeking
after '<' and also unconditionally swallows '>' characters; update the heuristic
in the block that inspects c == '<' to require the immediate next character (no
skipping of spaces) be a tag-start character (use html[i+1] after bounds check)
before setting inTag and inserting a separating space into result, and change
the c == '>' branch to only toggle inTag = false and drop the '>' when inTag is
true (otherwise append '>' to result); reference variables/functions: c, i, j,
html, inTag, result.
---
Nitpick comments:
In `@src/RecentBooksStore.cpp`:
- Around line 43-52: The updateBook signature and body in
RecentBooksStore::updateBook must accept and persist the series field like
addBook does: add a const std::string& series parameter to
RecentBooksStore::updateBook, set book.series = series inside the existing
branch that updates title/author/coverBmpPath, and ensure saveToFile() is still
called; also update any callers of updateBook to pass through the series value
(or empty string if unchanged) so existing entries can have their series
metadata refreshed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2601fb3f-9997-4509-8053-40db54ca1faf
📒 Files selected for processing (3)
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cppsrc/activities/home/FileBrowserActivity.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-03-02T21:18:25.387Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:137-150
Timestamp: 2026-03-02T21:18:25.387Z
Learning: In ChapterHtmlSlimParser.cpp, pendingAnchorId is a single string (not a vector) because EPUB footnote targets are virtually always block-level elements (<aside>, <p>, <div>) that trigger startNewTextBlock() and flush the anchor before overwrite can occur. Multiple consecutive inline IDs would resolve to the same completedPageCount anyway, and vector allocation overhead is unnecessary for ESP32-C3. Only inline IDs in practice are footnote back-references (<a id="ref1">), which are never navigation targets.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cppsrc/activities/home/FileBrowserActivity.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/activities/home/FileBrowserActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cppsrc/activities/home/FileBrowserActivity.cpp
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/home/FileBrowserActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/home/FileBrowserActivity.cpp
🔇 Additional comments (5)
src/activities/home/FileBrowserActivity.cpp (5)
156-175: LGTM - Previous review concern addressed.The file-open path now correctly uses a local
fullPathvariable (lines 170-172) instead of mutating thebasepathmember, while directory navigation appropriately updatesbasepathwithin the conditional block.
177-218: LGTM - Robust delete flow with proper confirmation.The implementation correctly:
- Guards against accidental deletion with long-press requirement and confirmation dialog
- Prevents directory deletion (line 185-187)
- Captures
fullPathby value in the lambda for safe async access- Handles selector bounds correctly after deletion (lines 200-204)
The hidden gesture design is intentional per PR objectives.
220-231: LGTM - Clean BookInfoActivity integration.Correctly guards against non-EPUB/XTC files and directories, constructs the full path without mutating
basepath, and the result handler appropriately refreshes the display. PerBookInfoActivity::loadData(), this is safe to invoke from the file browser as it only reads metadata.
233-253: LGTM - Standard list navigation.The
buttonNavigatorpattern with capturedlistSizeandpageItemsvalues is consistent with the codebase conventions for list navigation.
285-294: LGTM - Button hints correctly reflect new controls.The dynamic label mapping properly shows:
- Context-aware Back/Home based on directory depth
- Info hint only for supported file types (mirroring line 224's condition)
- Empty btn3 keeps the delete gesture appropriately hidden
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/Serialization/Serialization.h (1)
39-40: Consider whether 4096 bytes is sufficient for all fields.Book descriptions (read via
BookMetadataCache.cppline 395) can legitimately exceed 4096 bytes. A typical multi-paragraph synopsis could be 5-10 KB.This limit provides good protection against malformed/malicious data, but you may want to either:
- Increase to ~16KB or 32KB to accommodate longer descriptions, or
- Document that descriptions will be silently truncated/rejected.
Based on learnings: this hardening at the
readStringsource is the preferred approach over per-call-site validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Serialization/Serialization.h` around lines 39 - 40, The current MAX_STRING_LENGTH constexpr (in Serialization::Serialization.h) is 4096 which is too small for book descriptions read via BookMetadataCache.cpp (e.g., readString); either raise the limit to a larger value (suggest 16384 or 32768) to accommodate multi‑KB synopses, or add a clear comment/docstring on MAX_STRING_LENGTH noting that strings above the limit will be truncated/rejected and that callers (like readString/BookMetadataCache) must handle that behavior; update the constant value and add a brief explanatory comment next to MAX_STRING_LENGTH and ensure readString’s use of the constant is the single source of truth for length validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 244-283: The parser currently only switches to
IN_BOOK_SERIES/IN_BOOK_SERIES_INDEX when seeing metaProperty, so
self-closing/meta with content="..." (metaContent) is ignored; update the block
handling metaProperty in ContentOpfParser.cpp so that if metaProperty ==
"belongs-to-collection" and metaContent is non-null you set self->series
(truncate to MAX_DESCRIPTION_LENGTH if needed) instead of only changing state,
and likewise if metaProperty == "group-position" and metaContent is non-null set
self->seriesIndex (truncate) instead of relying on character data; keep the
existing behavior fallback that sets self->state = IN_BOOK_SERIES or
IN_BOOK_SERIES_INDEX and returns when metaContent is null, and use the same
emptiness checks (self->series.empty(), self->seriesIndex.empty()) to avoid
overwriting existing values.
In `@lib/Serialization/Serialization.h`:
- Around line 50-57: In readString, when readPod(file, len) yields len >
MAX_STRING_LENGTH you must advance the file pointer by len to avoid corrupting
subsequent reads; update the early-return path in readString to call
file.seekCur(len) (and handle its failure if it returns an error) before
returning false so the file remains aligned; reference readString, readPod,
MAX_STRING_LENGTH and FsFile::seekCur when making this change.
- Around line 41-48: The readString function reads the length with readPod but
returns false when len > MAX_STRING_LENGTH without skipping the payload, leaving
the stream misaligned; modify readString(std::istream&, std::string&) to, after
reading len and detecting len > MAX_STRING_LENGTH, advance the stream by len
(e.g., use is.seekg(len, std::ios::cur) and handle failure) before returning
false so subsequent reads remain aligned; likewise update the FsFile overload to
call file.seekCur(len) when len is oversized; keep the existing behavior of not
resizing/reading into s and ensure any error from seeking is handled/propagated
consistently with readPod failure semantics.
In `@src/RecentBooksStore.cpp`:
- Around line 129-145: Deserialize the file into a temporary container instead
of mutating recentBooks as you parse: create a local vector (e.g.,
tmpRecentBooks), push_back parsed RecentBook entries into tmpRecentBooks
(including the getDataFromBook fallback branch where title/author are read), and
only swap or move tmpRecentBooks into the member recentBooks after the full read
loop completes without error. Ensure existing error paths still close inputFile
and return false without touching recentBooks; apply the same temporary-buffer
pattern to the other parsing block referenced (lines ~160-173) so partial parses
never overwrite the live recentBooks.
---
Nitpick comments:
In `@lib/Serialization/Serialization.h`:
- Around line 39-40: The current MAX_STRING_LENGTH constexpr (in
Serialization::Serialization.h) is 4096 which is too small for book descriptions
read via BookMetadataCache.cpp (e.g., readString); either raise the limit to a
larger value (suggest 16384 or 32768) to accommodate multi‑KB synopses, or add a
clear comment/docstring on MAX_STRING_LENGTH noting that strings above the limit
will be truncated/rejected and that callers (like readString/BookMetadataCache)
must handle that behavior; update the constant value and add a brief explanatory
comment next to MAX_STRING_LENGTH and ensure readString’s use of the constant is
the single source of truth for length validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8154428-188e-4329-89bb-4e5986a2dd74
📒 Files selected for processing (5)
lib/Epub/Epub/parsers/ContentOpfParser.cpplib/Serialization/Serialization.hsrc/RecentBooksStore.cppsrc/RecentBooksStore.hsrc/activities/home/HomeActivity.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/RecentBooksStore.h
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-03-02T21:18:25.387Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:137-150
Timestamp: 2026-03-02T21:18:25.387Z
Learning: In ChapterHtmlSlimParser.cpp, pendingAnchorId is a single string (not a vector) because EPUB footnote targets are virtually always block-level elements (<aside>, <p>, <div>) that trigger startNewTextBlock() and flush the anchor before overwrite can occur. Multiple consecutive inline IDs would resolve to the same completedPageCount anyway, and vector allocation overhead is unnecessary for ESP32-C3. Only inline IDs in practice are footnote back-references (<a id="ref1">), which are never navigation targets.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cppsrc/activities/home/HomeActivity.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cppsrc/activities/home/HomeActivity.cpp
📚 Learning: 2026-03-02T10:14:25.057Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:25.057Z
Learning: In the crosspoint-reader codebase, `serialization::readString` is used at approximately 30 call sites (BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers, etc.) and performs unbounded `std::string::resize(len)` directly from disk data. Per-call-site validation is avoided in favor of potential future hardening at the `readString` source itself. Section cache files are versioned (SECTION_FILE_VERSION) and invalidated on parameter mismatches, reducing corruption risk for self-written cache files.
Applied to files:
lib/Serialization/Serialization.h
🔇 Additional comments (4)
src/activities/home/HomeActivity.cpp (1)
77-77: Nice propagation ofserieswhen clearing stale covers.Keeping
book.seriesin bothupdateBookcalls preserves the richer recent-book subtitles even when thumbnail generation fails.Also applies to: 94-94
lib/Epub/Epub/parsers/ContentOpfParser.cpp (1)
201-207: Good hardening of description parsing.The first-description guard plus the bounded append closes off the earlier concatenation/memory-growth edge cases cleanly.
Also applies to: 457-462, 519-523
src/RecentBooksStore.cpp (2)
23-24: Series metadata is propagated consistently.Both write paths now keep
RecentBook::seriesaligned with the expanded struct layout.Also applies to: 33-33, 43-44, 51-52
77-79: The new field is initialized cleanly across backends.EPUB/XTC/text/default paths all construct
RecentBookwith an explicitseriesvalue, which avoids partially initialized entries during migration and refresh.Also applies to: 83-88
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/RecentBooksStore.cpp (1)
119-124:⚠️ Potential issue | 🔴 CriticalAdd failure checks for header reads before branching on
versionandcount.The
serialization::readPod()function returnsvoidand provides no error status on short reads, leaving destination variables uninitialized when the file is truncated. Lines 119 and 124 readversionandcountwithout any checks, then immediately branch and callreserve()based on these potentially-uninitialized values. While subsequentreadString()calls properly check return values, these POD header reads should be validated before use to prevent undefined behavior on corrupted files.
♻️ Duplicate comments (1)
lib/Serialization/Serialization.h (1)
56-59:⚠️ Potential issue | 🔴 Critical
seekCur(len)with unbounded offset is unsafe on ESP32 SdFat—apply a cap or require return-value checks.While file-position alignment is needed (most call sites don't check the return value), calling
seekCurwith a potentiallyUINT32_MAXoffset produces undefined behavior on ESP32 SdFat. Either cap the seek to a reasonable maximum, or refactor all call sites to properly check the return value and close the file on failure, eliminating the need for seekCur.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Serialization/Serialization.h` around lines 56 - 59, The call to file.seekCur(len) with an unbounded len is unsafe on ESP32 SdFat; update the code in Serialization.h to avoid passing a potentially UINT32_MAX offset to seekCur by either capping len to a safe maximum (e.g., min(len, SAFE_SEEK_MAX)) before calling seekCur or by removing the silent seek and instead checking the return value of seekCur and handling failure (close the file and return false) so callers no longer rely on an unchecked seek. Modify the branch that currently uses MAX_STRING_LENGTH and seekCur to perform the cap or to perform a checked seek+error path, referencing the symbols seekCur, len, and MAX_STRING_LENGTH so callers remain aligned or fail-cleanly.
🧹 Nitpick comments (1)
lib/Epub/Epub/parsers/ContentOpfParser.cpp (1)
263-266: Consider trimming whitespace when assigning frommetaContent.The character-data path applies
trim()inendElement(lines 533, 539), but direct assignments from thecontent=attribute don't. If an EPUB hascontent=" Series Name ", the leading/trailing whitespace would be preserved.💡 Consistent trim for attribute-based values
} else if (strcmp(metaName, "calibre:series") == 0 && self->series.empty()) { - self->series = std::string(metaContent).substr(0, MAX_DESCRIPTION_LENGTH); + self->series = trim(std::string(metaContent).substr(0, MAX_DESCRIPTION_LENGTH)); } else if (strcmp(metaName, "calibre:series_index") == 0 && self->seriesIndex.empty()) { - self->seriesIndex = std::string(metaContent).substr(0, MAX_DESCRIPTION_LENGTH); + self->seriesIndex = trim(std::string(metaContent).substr(0, MAX_DESCRIPTION_LENGTH)); }Apply the same pattern to lines 276 and 284.
Also applies to: 276-276, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp` around lines 263 - 266, The assignments to self->series and self->seriesIndex use std::string(metaContent).substr(...) without trimming, so attribute values like " Series Name " keep surrounding whitespace; update the two assignment sites (the statements setting self->series and self->seriesIndex from metaContent) to apply the same trimming used elsewhere (the trim() used in endElement) before taking substr/MAX_DESCRIPTION_LENGTH so the stored values are whitespace-trimmed and length-limited (keep using MAX_DESCRIPTION_LENGTH and metaContent/series/seriesIndex identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/RecentBooksStore.cpp`:
- Around line 77-79: The migration is currently allowing blank-title records to
be carried from v1/v2 into recent.json; in the code path that uses
getDataFromBook() and constructs RecentBook (and where the v2 fallback is
applied), change the logic to base the fallback/skip decision on
book.title.empty() and drop any record that still has an empty title before
pushing into tmpRecentBooks or before committing tmpRecentBooks to the final
recent list; update the branch that applies the v2 stored fallback (the code
that creates RecentBook from epub.getTitle()/getAuthor()/getSeries) to skip
adding the record when book.title.empty(), and apply the same check in the other
migration branch that handles older records so no blank-title entries are
persisted.
---
Duplicate comments:
In `@lib/Serialization/Serialization.h`:
- Around line 56-59: The call to file.seekCur(len) with an unbounded len is
unsafe on ESP32 SdFat; update the code in Serialization.h to avoid passing a
potentially UINT32_MAX offset to seekCur by either capping len to a safe maximum
(e.g., min(len, SAFE_SEEK_MAX)) before calling seekCur or by removing the silent
seek and instead checking the return value of seekCur and handling failure
(close the file and return false) so callers no longer rely on an unchecked
seek. Modify the branch that currently uses MAX_STRING_LENGTH and seekCur to
perform the cap or to perform a checked seek+error path, referencing the symbols
seekCur, len, and MAX_STRING_LENGTH so callers remain aligned or fail-cleanly.
---
Nitpick comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 263-266: The assignments to self->series and self->seriesIndex use
std::string(metaContent).substr(...) without trimming, so attribute values like
" Series Name " keep surrounding whitespace; update the two assignment sites
(the statements setting self->series and self->seriesIndex from metaContent) to
apply the same trimming used elsewhere (the trim() used in endElement) before
taking substr/MAX_DESCRIPTION_LENGTH so the stored values are whitespace-trimmed
and length-limited (keep using MAX_DESCRIPTION_LENGTH and
metaContent/series/seriesIndex identifiers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58e98930-e94c-4574-8d8d-8be88dd77323
📒 Files selected for processing (3)
lib/Epub/Epub/parsers/ContentOpfParser.cpplib/Serialization/Serialization.hsrc/RecentBooksStore.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-03-02T21:18:25.387Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:137-150
Timestamp: 2026-03-02T21:18:25.387Z
Learning: In ChapterHtmlSlimParser.cpp, pendingAnchorId is a single string (not a vector) because EPUB footnote targets are virtually always block-level elements (<aside>, <p>, <div>) that trigger startNewTextBlock() and flush the anchor before overwrite can occur. Multiple consecutive inline IDs would resolve to the same completedPageCount anyway, and vector allocation overhead is unnecessary for ESP32-C3. Only inline IDs in practice are footnote back-references (<a id="ref1">), which are never navigation targets.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-08T10:24:28.111Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1342
File: lib/Serialization/Serialization.h:50-57
Timestamp: 2026-03-08T10:24:28.111Z
Learning: In crosspoint-reader/crosspoint-reader, every call site of `serialization::readString` (lib/Serialization/Serialization.h) checks the bool return value and immediately closes the file and returns false on failure. There is therefore no need to advance the file pointer (e.g. via seekCur) when rejecting an oversized string, because no subsequent read ever occurs after a failure. Additionally, calling seekCur with a potentially UINT32_MAX-sized offset on the ESP32 SdFat implementation is unsafe and should be avoided.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-03-02T10:14:25.057Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:25.057Z
Learning: In the crosspoint-reader codebase, `serialization::readString` is used at approximately 30 call sites (BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers, etc.) and performs unbounded `std::string::resize(len)` directly from disk data. Per-call-site validation is avoided in favor of potential future hardening at the `readString` source itself. Section cache files are versioned (SECTION_FILE_VERSION) and invalidated on parameter mismatches, reducing corruption risk for self-written cache files.
Applied to files:
lib/Serialization/Serialization.h
📚 Learning: 2026-03-08T10:24:28.111Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1342
File: lib/Serialization/Serialization.h:50-57
Timestamp: 2026-03-08T10:24:28.111Z
Learning: In lib/Serialization/Serialization.h, ensure that every call site of serialization::readString checks the boolean return value and, on failure, immediately closes the file and returns false (no further reads). Do not advance the file pointer (no seekCur) after a failure, since no subsequent read occurs. Also, avoid calling seekCur with potentially UINT32_MAX-sized offsets on ESP32 SdFat, as it is unsafe. When reviewing related code, verify that error handling paths terminate reads promptly and do not perform risky pointer/offset adjustments after a failure.
Applied to files:
lib/Serialization/Serialization.h
📚 Learning: 2026-03-07T14:16:34.225Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1320
File: lib/JpegToBmpConverter/JpegToBmpConverter.cpp:281-308
Timestamp: 2026-03-07T14:16:34.225Z
Learning: In the crosspoint-reader/crosspoint-reader codebase (ESP32 Arduino environment), exceptions are disabled. Plain `new` calls `abort()` on OOM rather than throwing `std::bad_alloc`. Prefer `new(std::nothrow)` for non-critical allocations so that OOM returns `nullptr` and can be handled gracefully. Do not raise exception-safety concerns in code reviews for this repository.
Applied to files:
lib/Serialization/Serialization.hsrc/RecentBooksStore.cpp
🔇 Additional comments (8)
lib/Epub/Epub/parsers/ContentOpfParser.cpp (5)
17-81: Literal angle-bracket handling looks correct now.The lookahead (lines 23-33) properly distinguishes HTML tags from literal
<characters by checking if the next character is alphabetic,/,!, or?. Text like2 < 3will now preserve the<as expected. This addresses the previous review concern.
201-207: First-occurrence guard added.The
self->description.empty()check ensures only the firstdc:descriptionelement is captured, avoiding concatenation of localized/alternate variants. This addresses the previous review feedback.
464-486: Bounds checking correctly caps metadata growth.All three fields (
description,series,seriesIndex) now enforceMAX_DESCRIPTION_LENGTHduring character data accumulation, preventing unbounded allocation from malformed EPUBs. This aligns with the codebase guideline to defend against unbounded growth when reading from disk data.
526-542: Post-processing sanitization looks good.Applying
stripHtml()to description andtrim()to series/seriesIndex at element close ensures clean final values regardless of how the character data was chunked by the parser.
83-93: Simple and correct trim implementation.Handles all common whitespace characters (space, newline, carriage return, tab) from both ends.
lib/Serialization/Serialization.h (2)
39-40: LGTM!The 4KB cap is a reasonable safety limit for metadata strings while still accommodating typical EPUB content.
41-51: LGTM!The length validation with stream realignment and bool return value provides a safer deserialization path for the
std::istreamvariant.src/RecentBooksStore.cpp (1)
23-24: Series propagation looks good.
addBook()andupdateBook()both carry the newseriesfield through the sameRecentBookshape, so inserts and refreshes stay consistent.Also applies to: 33-33, 43-52
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ContentOpfParser.cpp`:
- Around line 262-265: The code currently constructs std::string(metaContent)
and then substrs to MAX_DESCRIPTION_LENGTH, which allows an untrusted huge
attribute to be copied into a temporary; change the construction so you only
copy a bounded prefix into the temporary string (e.g. build the std::string from
metaContent with a length limited to MAX_DESCRIPTION_LENGTH or
std::min(strlen(metaContent), MAX_DESCRIPTION_LENGTH)), then call trim() and any
substrings on that bounded string; apply this fix for the assignments to
self->series and self->seriesIndex (and the other similar branches mentioned
around the same area) and keep using MAX_DESCRIPTION_LENGTH, trim, and
metaContent to locate the spots.
In `@src/RecentBooksStore.cpp`:
- Around line 137-149: The v2 legacy title/author fields are only read when both
book.title and book.author are empty, causing unread bytes on subsequent
iterations and missing the fallback case; in the function handling recent.bin
parsing (use symbols: book, version, serialization::readString, inputFile,
tmpRecentBooks, path, i) always call serialization::readString(inputFile, title)
and serialization::readString(inputFile, author) for every entry when version ==
2 to consume the legacy fields, check read success and log/return on failure,
then decide whether to push tmpRecentBooks using the live book values or, if
book.title is empty (or book.title.empty() && !author.empty()), use the legacy
title/author as the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cbfea06-ee5a-4152-bb5f-86c3e720346f
📒 Files selected for processing (3)
lib/Epub/Epub/parsers/ContentOpfParser.cpplib/Serialization/Serialization.hsrc/RecentBooksStore.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/Serialization/Serialization.h
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-03-02T21:18:25.387Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:137-150
Timestamp: 2026-03-02T21:18:25.387Z
Learning: In ChapterHtmlSlimParser.cpp, pendingAnchorId is a single string (not a vector) because EPUB footnote targets are virtually always block-level elements (<aside>, <p>, <div>) that trigger startNewTextBlock() and flush the anchor before overwrite can occur. Multiple consecutive inline IDs would resolve to the same completedPageCount anyway, and vector allocation overhead is unnecessary for ESP32-C3. Only inline IDs in practice are footnote back-references (<a id="ref1">), which are never navigation targets.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cpp
📚 Learning: 2026-03-08T10:24:28.111Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1342
File: lib/Serialization/Serialization.h:50-57
Timestamp: 2026-03-08T10:24:28.111Z
Learning: In crosspoint-reader/crosspoint-reader, every call site of `serialization::readString` (lib/Serialization/Serialization.h) checks the bool return value and immediately closes the file and returns false on failure. There is therefore no need to advance the file pointer (e.g. via seekCur) when rejecting an oversized string, because no subsequent read ever occurs after a failure. Additionally, calling seekCur with a potentially UINT32_MAX-sized offset on the ESP32 SdFat implementation is unsafe and should be avoided.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/RecentBooksStore.cpp
📚 Learning: 2026-03-07T14:16:34.225Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1320
File: lib/JpegToBmpConverter/JpegToBmpConverter.cpp:281-308
Timestamp: 2026-03-07T14:16:34.225Z
Learning: In the crosspoint-reader/crosspoint-reader codebase (ESP32 Arduino environment), exceptions are disabled. Plain `new` calls `abort()` on OOM rather than throwing `std::bad_alloc`. Prefer `new(std::nothrow)` for non-critical allocations so that OOM returns `nullptr` and can be handled gracefully. Do not raise exception-safety concerns in code reviews for this repository.
Applied to files:
src/RecentBooksStore.cpp
Contributor
|
Thanks!! |
# Conflicts: # src/activities/home/FileBrowserActivity.cpp
pablohc
added a commit
to pablohc/crosspoint-reader
that referenced
this pull request
Apr 5, 2026
…ontrols - New BookInfoActivity for EPUB/XTC files: title, author, series, description, file size, cover thumbnail. Accessible from file browser (Right button) and recent books list. - Expanded EPUB metadata: series, seriesIndex, description parsed from OPF (calibre:series, calibre:series_index, EPUB3 belongs-to-collection). - BookMetadataCache bumped to v6 with 3 new fields. - RecentBooksStore gains series field, propagated to addBook/updateBook. - ContentOpfParser: stripHtml helper for description, length-bounded capture. - Serialization: readString returns bool, MAX_STRING_LENGTH (4096) guard. - File browser remap: Back=parent/home, Confirm=open, Left long=delete, Right=info. ButtonNavigator updated to onRelease/onContinuous API. - ConfirmationActivity hardened: arms input only after inherited events are cleared, preventing accidental auto-confirm. - Home/recent views show richer subtitle (author + series). Credits: Based on PR crosspoint-reader#1342 by @jpirnay - all praise goes to him.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What is the goal of this PR? This PR adds a new Book Info experience for EPUB/XTC files and improves metadata handling across the app.
It also updates file-browser button behavior and hardens delete confirmation to prevent accidental actions.
What changes are included?
Added
BookInfoActivityfor EPUB/XTC files. Accessible from the file browser via the Right button (Info).Displays:
Expanded EPUB metadata support
Extended metadata extraction/persistence to include: series, seriesIndex and description
Recents metadata improvements
Recent book entries now store/display series information. Home/recent views now show richer subtitle info (author + series when present). Reader entry points updated to populate series data when adding to recents.
File browser input remap + hidden delete gesture
Updated controls:
Fixed accidental auto-confirm/cancel edge case by arming confirmation input only after inherited button events are cleared.
Prevents the press/release that opened the dialog from being reused as confirmation.
Additional Context
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? YES