feat(reader): chapter navigation, appearance system, and reading stats#440
Open
RXWatcher wants to merge 67 commits into
Open
feat(reader): chapter navigation, appearance system, and reading stats#440RXWatcher wants to merge 67 commits into
RXWatcher wants to merge 67 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The chapter label should only render when extent is non-null, ensuring that extent:null hides both the chapter band and label by construction. Updated test to verify the label is absent when extent is null. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Header shrank from two rows to one when the progress slider moved into ReaderFooter, but the page wrapper (min-h-screen) and <main> (h-[calc(100vh-6rem)]) still assumed the old 6rem of chrome. Comics (header + main, no footer) showed a blank strip; prose (header + main + footer) overflowed the viewport. Switch to a flex column (h-screen flex-col, main flex-1 min-h-0) so header/footer keep natural heights and main absorbs the remainder in both layouts, without a hardcoded constant that has to be re-tuned whenever chrome changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chapterExtent indexed sectionFractions[i], sectionFractions[i+1], and sectionFractions[1] without checking for undefined, which noUncheckedIndexedAccess flags as number | undefined and fails tsc --noEmit -p tsconfig.app.json. Hoist the boundary read into a guarded local and continue when it's undefined; end already falls back with ??. No behavior change, existing tests unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Task 5 attached tap-zone handling as onPointerUp on the outer <section data-reader-surface> and chrome-reappear as a window mousemove listener, but foliate renders prose in a same-origin iframe that fills the surface and its events never bubble out. Extend the same content-doc listener bridge FoliateBookReader already uses for selection (attachSelectionListeners) to also forward pointerup/ mousemove from each content doc, translated into outer viewport coordinates via the frame element's getBoundingClientRect(). EbookReader now shares its tap-zone and edge-reveal logic between the margin path and this new content-iframe path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reading-ruler grip inside [data-reader-surface] never called event.stopPropagation() on pointerup/pointercancel, so releasing a drag bubbled to the section's tap handler. The grip sits at right-2, so xRatio≈1 resolved to a next-page tap on every ruler release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReaderFooter prefers locationInfo.fraction over readerProgress, but handleProgressScrub only updated readerProgress. If the last relocate reported a different fraction, the bar could revert to it until the next relocate event landed. Scrubbing now also patches locationInfo optimistically, preserving its sectionIndex/tocLabel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Escape handler only chained shortcuts-overlay then chrome, so pressing Escape while the side panel (Contents/Search/Notes/Settings) was open skipped straight past it and toggled chrome instead. Escape now closes the topmost open layer: overlay, then panel, then chrome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReaderShortcutsOverlay autofocused its Close button but never trapped Tab/Shift+Tab, so focus could escape the modal into the page behind it. Adds a generic focus trap (mirrors the pattern already used by SubtitleSearchModal) over whatever is focusable inside the dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The listed shortcuts described "b" as "Toggle bookmark", but the header bookmark button (and the bound handler) only ever creates one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
typeof detail.fraction === "number" is true for NaN, letting a non-finite fraction reach onLocationChange (and the Math.min/max clamp, which then propagates NaN). Layers Number.isFinite on top of the typeof narrowing needed for the Math calls. Also declares getSectionFractions on FoliateViewElement directly and drops the inline cast at the imperative handle that stood in for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
profile_id comes straight from the client-supplied X-Profile-Id header (RequireProfile only checks non-empty) and reached filepath.Join/filepath.Glob unvalidated in the reader-fonts upload/serve/delete handlers, allowing path traversal via a header like `X-Profile-Id: ../../../../tmp/evil`. Add safeReaderFontPathComponent and call it before any store or filesystem operation in every handler. Also: fix the /ebooks route group so reader-fonts routes no longer disappear when the ebook reader handler is nil, best-effort remove a partially written blob on upload failure, and tighten filename sanitization to reject the same unsafe characters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Relaxed sanitizeReaderFontFilename to preserve spaces, parentheses, and non-ASCII characters in user-uploaded font display names - Now only strips NUL and control characters (0x00-0x1F, 0x7F), trims whitespace, and caps at 255 runes - Font blobs remain content-addressed as <id>.<format>, so the strict charset restriction is unnecessary for security - TDD: Added test cases for "Literata Bold (2).ttf", "Times New Roman.ttf", and Cyrillic filenames to verify names/filenames are preserved - Also added HandleList coverage to path-traversal regression test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix TS2532 build error in ReadingHeatmap.heatmapBuckets: destructure days[0] before use so noUncheckedIndexedAccess doesn't flag it, since a length check alone doesn't narrow indexed array access.
DailyRollup only returns days with sessions, but heatmapBuckets padded leading blanks from the first day's weekday and then laid days into consecutive grid cells, silently compressing any gap day and shifting every later day's weekday row. Add an exported densifyDays(days, from, to) helper that fills every missing date in the range with a zero-second entry before bucketing. ReadingStats now computes the same default range the server uses (today UTC, minus 365 days) and passes it explicitly to both the history query and the heatmap, so client and server always agree on the range being densified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
isComicFormat derives from the loaded item's file versions, so while useCatalogItemDetail is still pending it defaults to false — the same value a prose book would have. The heartbeat hook's contentId and useBookReadingStats were both gated on isComicFormat alone, so a keypress during that loading window could fire a stats GET and create a spurious reading session for a book that turns out to be a comic. Add readerKindKnown = Boolean(item) and require it alongside !isComicFormat before either tracker sees a contentId. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds reader appearance customization, chapter-aware navigation, reading-session analytics, motivation aggregates, and a reading-stats dashboard. The change includes custom font APIs and storage, theme and layout migration, reader footer and shortcuts, timezone-aware statistics, goals, achievements, and related tests. ChangesReader appearance and custom fonts
Reader page navigation
Reading statistics and motivation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant EbookReader
participant HeartbeatHook
participant ReadingSessionsHandler
participant ReadingMotivationHandler
participant ReadingStatsPage
EbookReader->>HeartbeatHook: record activity and fraction
HeartbeatHook->>ReadingSessionsHandler: send heartbeat
ReadingSessionsHandler-->>ReadingStatsPage: provide history aggregates
ReadingMotivationHandler-->>ReadingStatsPage: provide motivation aggregates
ReadingStatsPage->>ReadingMotivationHandler: save reading goals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@internal/api/handlers/reading_sessions.go`:
- Around line 625-631: Update the reading-stats aggregation query in the handler
containing the rows Query call so date_trunc operates on started_at converted
with AT TIME ZONE 'UTC'. Keep the existing grouping, ordering, filters, and
duration sum unchanged while ensuring daily buckets are UTC-based.
In `@web/src/pages/EbookReader.tsx`:
- Around line 557-573: Update dispatchSurfaceTap to derive hasSelection from the
current live iframe selection at tap time rather than the potentially stale
React selection state. Reuse the existing selection source or helper used by the
iframe selection update, and preserve the tapZoneAction behavior while ensuring
pointer release after a new selection is treated as selected.
In `@web/src/reader/readerNavigation.ts`:
- Line 43: Update the shortcut entry for key "b" in the reader navigation
shortcuts so its description indicates toggling the bookmark rather than only
adding one, while leaving the key binding unchanged.
🪄 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: c3074dbc-e1cc-46da-859e-26a1edc24e71
📒 Files selected for processing (42)
cmd/silo/main.godocs/superpowers/plans/2026-07-20-reader-appearance.mddocs/superpowers/plans/2026-07-20-reader-page-navigation.mddocs/superpowers/plans/2026-07-20-reading-stats-foundation.mddocs/superpowers/specs/2026-07-20-reader-appearance-design.mddocs/superpowers/specs/2026-07-20-reader-page-navigation-design.mddocs/superpowers/specs/2026-07-20-reading-stats-foundation-design.mdinternal/api/handlers/reader_fonts.gointernal/api/handlers/reader_fonts_test.gointernal/api/handlers/reading_sessions.gointernal/api/handlers/reading_sessions_test.gointernal/api/router.gomigrations/sql/20260720190000_reader_fonts.sqlmigrations/sql/20260720210000_reading_sessions.sqlweb/src/App.tsxweb/src/components/AppSidebar.tsxweb/src/components/stats/ReadingHeatmap.test.tsxweb/src/components/stats/ReadingHeatmap.tsxweb/src/hooks/queries/keys.tsweb/src/hooks/queries/readingStats.tsweb/src/hooks/useReadingHeartbeat.test.tsweb/src/hooks/useReadingHeartbeat.tsweb/src/lib/formatDuration.test.tsweb/src/lib/formatDuration.tsweb/src/pages/EbookReader.test.tsxweb/src/pages/EbookReader.tsxweb/src/pages/ReadingStats.test.tsxweb/src/pages/ReadingStats.tsxweb/src/reader/FoliateBookReader.component.test.tsxweb/src/reader/FoliateBookReader.test.tsweb/src/reader/FoliateBookReader.tsxweb/src/reader/ReaderFooter.test.tsxweb/src/reader/ReaderFooter.tsxweb/src/reader/ReaderShortcutsOverlay.test.tsxweb/src/reader/ReaderShortcutsOverlay.tsxweb/src/reader/ebookReaderApi.tsweb/src/reader/readerFontsApi.test.tsweb/src/reader/readerFontsApi.tsweb/src/reader/readerNavigation.test.tsweb/src/reader/readerNavigation.tsweb/src/reader/readerThemes.test.tsweb/src/reader/readerThemes.ts
DailyRollup's date_trunc('day', started_at) truncated in whatever
timezone the DB session happened to be in, which the pool never sets
explicitly, making the day boundary nondeterministic. Wrap the column
in AT TIME ZONE 'UTC' first so bucketing is pinned to UTC regardless
of session config. TotalsSince's since boundaries are already
computed in Go as UTC time.Time and compared directly (no
date_trunc), so they were already stable and needed no change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Finishing a text selection could still page-turn: FoliateBookReader deferred its onSelectionChange update with setTimeout(0), so the pointerup ending a selection dispatched to EbookReader's dispatchSurfaceTap while React's `selection` state still read stale (often null). Add a synchronous hasLiveSelectionRef inside FoliateBookReader, updated in the selectionchange/pointerup/keyup listeners ahead of that deferral, and expose it via hasLiveSelection() on the imperative handle. dispatchSurfaceTap now ORs the React state with this live signal before deciding whether a tap should page-turn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The `b` shortcut and header bookmark button always created a new bookmark annotation, even when one already existed at the current location, contradicting the "Toggle bookmark" shortcut description. Rename handleCreateBookmark to handleToggleBookmark: it now looks for an existing kind="bookmark" annotation whose location exactly matches the one it would create, and deletes it (same path the Notes panel uses) instead of creating a duplicate. Restore the READER_SHORTCUTS "b" description and the header button's aria-label/title to "Toggle bookmark" to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…imezone Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Goal saves now go through useSaveReadingGoals (useMutation + invalidateQueries on ebookKeys.readingMotivation()), only advance the saved-value ref after a successful PUT, and surface an inline error (cleared on the next successful save) instead of failing silently. Motivation sections now render off useReadingMotivation's own data, independent of the reading-history query's loading/error gate. The motivation query key also includes tz, matching readingHistory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HandleHistory derived "today" from h.Now()'s UTC-clock Year/Month/Day fields before applying the requester's timezone, so today/week/month totals and the default `to` boundary silently shifted by a day for non-UTC users during their local 00:00-02:00 window (e.g. UTC+2). Mirror the motivation handler's now.In(loc) pattern instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The night-owl accumulation reused hourBucket's DNA "night" bucket (22-04), but the badge's own spec window is midnight-5am. Sessions between 22:00 and midnight were wrongly credited toward the badge while sessions between 04:00 and 05:00 were wrongly excluded. Accumulate against a dedicated nightOwlEndHour bound instead; hourBucket itself (used for the DNA night bucket) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reading-motivation response surfaced every genre individually, which sprawls for wide-genre libraries. Collapse the display list to the top 8 by seconds plus a trailing "other" bucket summing the rest, in the handler after diversity_score/genre-hopper's distinct-genre count are already computed from the full, uncollapsed list so neither is understated by the collapse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReadingStats.tsx derived its default history range from a fixed todayUTC(), disagreeing with the server-side fix in the previous commit whenever the viewer's local calendar day differs from UTC's. Derive `to` from the viewer's IANA timezone instead (via a small en-CA-locale Intl.DateTimeFormat trick), so the client's default range and the server's now agree on which day is "today". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reorder the reading-stats page to the spec order (totals row, then streak/goals/achievements/DNA, then the activity/books/sessions detail), while keeping the motivation sections' independence from the history query's loading/error state — they now render unconditionally rather than merely being unaffected by a gate they sat above. Add a Flame icon to the streak card's day count, matching today_qualified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
web/src/components/stats/MotivationSections.tsx (1)
241-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate error text with inputs via
aria-describedby.
aria-invalidis set, but the error<p>isn't programmatically linked to the field, so screen readers announce the invalid state without the reason. Give each error paragraph anidand reference it from the input.♿ Proposed association
<Input id="goal-books-per-year" inputMode="numeric" value={booksInput} aria-invalid={booksError != null} + aria-describedby={booksError ? "goal-books-per-year-error" : undefined} onChange={(e) => setBooksInput(e.target.value)} onBlur={() => void commitBooks()} /> - {booksError ? <p className="text-destructive text-xs">{booksError}</p> : null} + {booksError ? ( + <p id="goal-books-per-year-error" className="text-destructive text-xs"> + {booksError} + </p> + ) : null}Apply the same pattern to the hours field.
🤖 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 `@web/src/components/stats/MotivationSections.tsx` around lines 241 - 268, Associate each validation message with its corresponding input: add a unique id to the booksError and hoursError paragraphs, then set the matching aria-describedby on the goal-books-per-year and goal-hours-per-year Input elements. Keep the associations aligned with their respective fields and preserve the existing conditional error rendering.internal/api/handlers/reading_motivation.go (1)
616-620: 🚀 Performance & Scalability | 🔵 TrivialAll-time session load per motivation request may not scale.
SessionsSince(ctx, userID, profileID, time.Time{})pulls every session a profile has ever recorded on eachGET /reading-motivation, then recomputes streaks/DNA/achievements in memory. For long-term heavy readers this grows unbounded in row count and per-request memory/CPU. Consider precomputed daily/monthly rollups (thereading_sessionsday-bucket totals you already derive) or a server-side cache keyed by profile+tz+day, so the hot path reads a bounded aggregate rather than the full history.🤖 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 `@internal/api/handlers/reading_motivation.go` around lines 616 - 620, Replace the unbounded all-time SessionsSince call in the motivation request handler with a bounded aggregate source, such as precomputed daily/monthly reading-session rollups or a cache keyed by profile, timezone, and day. Update the downstream streak, DNA, and achievement calculations to consume that aggregate while preserving their existing behavior.
🤖 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 `@docs/superpowers/plans/2026-07-20-reading-motivation.md`:
- Line 35: Update the ReadingMotivationStore method signatures and
PGReadingMotivationStore implementation to use the repository’s identity types
consistently: context.Context, numeric userID, and string profileID for every
method, including GetGoals, PutGoals, AchievedAt, PersistAchievement,
SessionsSince, FinishedBooksInRange, GenreSeconds, and AuthorSeconds. Ensure all
declarations reflect the same typed contract.
- Around line 226-237: The dashboard handler must not load all session history
since the epoch for each request. Update the session aggregation design around
the handler and the GenreSeconds/AuthorSeconds queries to compute lifetime
totals and required aggregates in SQL or dedicated aggregate queries, fetching
raw sessions only for calculations that genuinely require them while preserving
the existing dashboard outputs.
- Around line 170-174: Extend TestEvaluateAchievements to cover the finisher
eligibility boundary where FinishedWithHighRead is derived: verify 94.99%
completion is rejected and exactly 95.00% is accepted, preserving the specified
inclusive >=95% behavior.
- Line 109: Update the Goals request-shape example to use a fenced Go block
instead of an inline code span, and show a valid putGoalsRequest struct with
pointer fields and correctly formatted JSON tags for books_per_year and
hours_per_year.
In `@docs/superpowers/specs/2026-07-20-reading-motivation-design.md`:
- Around line 61-65: Make reading-motivation reads side-effect-free by removing
GET-time achievement persistence from the motivation endpoint. In
docs/superpowers/specs/2026-07-20-reading-motivation-design.md lines 61-65,
revise the achievement flow so persistence occurs through a separate mutation
flow rather than during the motivation endpoint call. In
docs/superpowers/plans/2026-07-20-reading-motivation.md lines 201-203, remove
PersistAchievement writes from GET handling and update the endpoint contract and
tests accordingly.
---
Nitpick comments:
In `@internal/api/handlers/reading_motivation.go`:
- Around line 616-620: Replace the unbounded all-time SessionsSince call in the
motivation request handler with a bounded aggregate source, such as precomputed
daily/monthly reading-session rollups or a cache keyed by profile, timezone, and
day. Update the downstream streak, DNA, and achievement calculations to consume
that aggregate while preserving their existing behavior.
In `@web/src/components/stats/MotivationSections.tsx`:
- Around line 241-268: Associate each validation message with its corresponding
input: add a unique id to the booksError and hoursError paragraphs, then set the
matching aria-describedby on the goal-books-per-year and goal-hours-per-year
Input elements. Keep the associations aligned with their respective fields and
preserve the existing conditional error rendering.
🪄 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: 1b49bce4-d812-4976-8199-c223bce55eb7
📒 Files selected for processing (21)
docs/superpowers/plans/2026-07-20-reading-motivation.mddocs/superpowers/specs/2026-07-20-reading-motivation-design.mdinternal/api/handlers/reading_motivation.gointernal/api/handlers/reading_motivation_test.gointernal/api/handlers/reading_sessions.gointernal/api/handlers/reading_sessions_test.gointernal/api/router.gomigrations/sql/20260720230000_reading_motivation.sqlweb/src/components/stats/MotivationSections.test.tsxweb/src/components/stats/MotivationSections.tsxweb/src/hooks/queries/keys.tsweb/src/hooks/queries/readingStats.test.tsweb/src/hooks/queries/readingStats.tsweb/src/pages/EbookReader.test.tsxweb/src/pages/EbookReader.tsxweb/src/pages/ReadingStats.test.tsxweb/src/pages/ReadingStats.tsxweb/src/reader/FoliateBookReader.component.test.tsxweb/src/reader/FoliateBookReader.tsxweb/src/reader/readerNavigation.test.tsweb/src/reader/readerNavigation.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- web/src/reader/readerNavigation.ts
- web/src/reader/readerNavigation.test.ts
- web/src/pages/ReadingStats.tsx
- internal/api/router.go
- web/src/reader/FoliateBookReader.component.test.tsx
- internal/api/handlers/reading_sessions_test.go
- internal/api/handlers/reading_sessions.go
- web/src/pages/EbookReader.tsx
- web/src/reader/FoliateBookReader.tsx
- web/src/pages/EbookReader.test.tsx
Comment on lines
+170
to
+174
| func TestEvaluateAchievements(t *testing.T) | ||
| // each of the 18 at its boundary: satisfied at exactly the threshold, unsatisfied just below | ||
| // (3600s→first-hour, 7200s single session→marathon, streak 3/7/30/100, books 1/10/50, | ||
| // 36000s night→night-owl, 36000s early→early-bird, 72000s weekend→weekender, | ||
| // 5 genres→genre-hopper, 36000s one book→deep-diver, FinishedWithHighRead→finisher). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the finisher’s 95% boundary directly.
The specification requires >= 95%, but these tests only pass a precomputed FinishedWithHighRead boolean. Add cases proving 94.99% is rejected and 95.00% is accepted where that boolean is derived.
🤖 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 `@docs/superpowers/plans/2026-07-20-reading-motivation.md` around lines 170 -
174, Extend TestEvaluateAchievements to cover the finisher eligibility boundary
where FinishedWithHighRead is derived: verify 94.99% completion is rejected and
exactly 95.00% is accepted, preserving the specified inclusive >=95% behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Three stacked reader improvements, modeled on the strongest ideas from comparable ebook apps (BookOrbit et al.) and adapted to silo's architecture. Each was spec'd, planned, TDD-implemented, and adversarially reviewed; all three are running on a production install.
1. Page navigation
←/→,Home/Endchapter jumps,t/s/b/f,?shortcuts overlay,Escoverlay→panel→chrome precedence) with an editable-target guard.2. Appearance
themevalues migrate and keep being written for one release.reader_fontstable + four additive/api/v1/ebooks/reader-fontsendpoints (magic-byte validation, 5 MB / 10-per-profile caps,SILO_READER_FONTS_DIR), fonts delivered into the reader via authenticated blob fetch → object URL (CSS@font-facecan't carry auth headers; no API URLs ever enter content CSS). Profile path components are strictly validated before any filesystem use.spread), justification toggle, settings panel regrouped Theme/Typography/Layout.3. Reading stats (foundation)
/reading-statspage: 12-month heatmap (client-densified daily rollups), today/week/month/all-time totals, top books, session timeline. All rollups computed by query; UTC day boundaries.Migrations:
20260720190000_reader_fonts.sql,20260720210000_reading_sessions.sql.Specs and plans included under
docs/superpowers/{specs,plans}/2026-07-20-*.Review-hardened items worth reviewer attention
X-Profile-Idreachingfilepath.Join— caught in review, fixed with strict component validation + regression tests on all four font handlers.@font-faceauth-unreachability — caught in whole-branch review, fixed via blob URLs.API surface
Additive only: new endpoints under
/api/v1/ebooks(reader-fonts CRUD/file, reading-heartbeat, reading-stats × 2); task/result contracts untouched.Tests
pnpm build, lint, format clean.go test ./internal/api/handlers,go vet,gofmt,make migrate-validate,make verify-local-pathsclean.AI-use disclosure
Implemented and reviewed with Claude Code (spec → plan → TDD subagents with per-task and whole-branch adversarial review).
🤖 Generated with Claude Code
Summary by CodeRabbit