Repository navigation
Read document outlines in the chapter menu - #13
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds chapter extraction for several book formats, records outline depth and chapter page positions, and loads chapter metadata asynchronously for pane headers and menus. ChangesChapter extraction and navigation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor User
participant Model
participant BookChapters as Book.Chapters
User->>Model: Open chapter menu
Model->>BookChapters: Load chapters asynchronously
BookChapters-->>Model: Return chapters and error
Model->>Model: Update matching pane cache
Model-->>User: Refresh chapter menu
Merge Risk: ⚪ Minimal · up to The chapter-navigation changes have no established merge-blocking issue and are ready to merge subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Chapter destinations remain validated, displayed titles are sanitized, and results from replaced books are rejected. Automatic extraction adds background parsing work, but no introduced security defect was established. Converter isolation and cancellation of archive metadata reads remain incompletely characterized. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func openDOC(path string) (*Book, error) { | ||
| out, err := runTool("antiword", "-w", "0", "-m", "UTF-8.txt", path) | ||
| out, err := runTool("antiword", "-x", "db", path) | ||
| if err != nil { |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/book/tool.go
Line: 172
Comment:
**DOC headings lack integration coverage** The new conversion relies on antiword’s DocBook output to detect headings, but the real-tool test checks only plain paragraphs; the nested-heading test uses hand-written XML. Add a DOC fixture with headings so a change in antiword’s output cannot silently break the chapter menu.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
The real antiword test covers the converter invocation and paragraph output; DocBook fixtures cover nested headings and page destinations. A binary DOC fixture with heading styles remains a coverage gap.
| out.name.Local = "em" | ||
| if n.attr("role") == "bold" { | ||
| out.name.Local = "strong" | ||
| } |
There was a problem hiding this comment.
🟡 Medium book/doc.go:34
emphasis elements with role="underline" or role="strikethrough" are rendered as <em>, so Antiword documents using those Word styles lose their underline/strikethrough formatting and appear italic instead. Handle these roles separately by emitting <u> and <s> (while retaining <strong> for bold).
- out.name.Local = "em"
- if n.attr("role") == "bold" {
- out.name.Local = "strong"
- }
+ switch n.attr("role") {
+ case "bold":
+ out.name.Local = "strong"
+ case "underline":
+ out.name.Local = "u"
+ case "strikethrough":
+ out.name.Local = "s"
+ }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/book/doc.go around lines 34-37:
`emphasis` elements with `role="underline"` or `role="strikethrough"` are rendered as `<em>`, so Antiword documents using those Word styles lose their underline/strikethrough formatting and appear italic instead. Handle these roles separately by emitting `<u>` and `<s>` (while retaining `<strong>` for `bold`).
Evidence trail:
Reviewed commit 05d8a7a: `internal/book/doc.go:33-37`; `internal/book/tool.go:169-180`; `internal/book/epub_render.go:388-390`; `internal/book/epub_layout.go:31-39,304-313`. Antiword implementation: https://github.com/grobian/antiword/blob/main/xml.c#L824-L888
There was a problem hiding this comment.
Only italic emphasis now selects em; bold selects strong. Underline and strikethrough stay plain text because the shared layout does not draw those decorations. Added a role-mapping regression test.
| switch n.attr("role") { | ||
| case "", "italic": | ||
| out.name.Local = "em" | ||
| case "bold": | ||
| out.name.Local = "strong" | ||
| } |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/book/doc.go
Line: 34-39
Comment:
**DOC text loses decoration** Underlined and struck-through DOC text now becomes a plain span. The renderer applies no decoration to spans, so these runs lose their visual distinction. In particular, struck-through text can appear to be current text.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
DOC previously used plain-text output, which did not display underline or strikethrough. Chapter extraction now preserves headings and the supported bold/italic styles. Drawing other decorations requires a separate change to the shared text layout; this keeps the prior plain-text behavior rather than substituting italics.
There was a problem hiding this comment.
That’s fair. Since the previous DOC path rendered plain text and did not preserve underline or strikethrough either, this change does not introduce a regression; limiting decoration support to the existing bold/italic mappings is intentional. I’m withdrawing this finding.
Read PDF and DJVU outlines and EPUB navigation tables in the Tab chapter menu. Preserve nested entries, source order, and page destinations.
Summary by CodeRabbit
Note
Read document outlines in the chapter menu across PDF, EPUB, DOCX, DJVU, DOC, and FB2
Extracts real table-of-contents outlines from supported formats and shows them as an indented chapter menu instead of relying on ComicInfo or folder markers.
Chapter.Depth, and the menu indents entries by depth in source outline order.Book.CurrentChapter, which picks the nearest preceding chapter with later equal-page entries winning ties.Book.Chaptersnow returnsos.ErrClosedfor closed books, DOC loading requires antiword DocBook output (plain line-per-paragraph output is gone), and PDF/DJVU tests now needpdftohtml/djvusedinstalled.Macroscope summarized 4711d79.
The PR appears safe to merge, with a non-blocking DOC formatting regression and an existing test-coverage gap.
Findings
Fix with agent prompt
Summary
The PR adds nested chapter extraction across document formats and loads chapter metadata asynchronously for the menu. Since the previous review, it also changes how DOC emphasis roles are converted.
Reviews (4) · Last reviewed commit: "Avoid treating DOC text decorations as i..."