ReadingPage — full-screen reading view with chapter content, swipe/keyboard navigation
TableOfContents — slide-in left drawer listing all chapters with current highlighted
ReadingSettingsPanel — slide-in right drawer with Theme presets (Sepia/Dark/Light/Paper), Font family, Font size slider, Line height slider, Margin slider, Brightness slider, Orientation lock
These MUST be resolved before this PR can be merged.
[File: backend/apps/reader/models.py] Missing migrations for new model
The ReadingSettings model was added but no migration file was included. Django requires migrations to be committed alongside model changes. Run python manage.py makemigrations apps.reader and include the generated migration.
[File: backend/apps/books/models.py] Missing Chapter and ReadingProgress models
The spec documents Chapter and ReadingProgress models that should exist under apps.books, but no changes to this file are in the diff. The frontend API client (frontend/src/api/reader.ts) and web/src/api/reader.ts call /api/books/${bookId}/chapters/ and /api/books/${bookId}/progress/ endpoints that will fail without these models.
[File: backend/apps/books/views.py | backend/apps/books/urls.py] Missing chapter and progress endpoints
The frontend API client calls:
GET /api/books/{id}/chapters/ (list TOC)
GET /api/books/{id}/chapters/{number}/ (chapter content)
GET /api/books/{id}/progress/ (get progress)
PUT /api/books/{id}/progress/ (update progress)
None of these endpoints exist in the backend diff. Without these, the reading view cannot function.
[File: frontend/src/pages/ReadingPage.tsx | web/src/pages/ReadingPage.tsx] XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses dangerouslySetInnerHTML={{ __html: currentChapter.content }} to render chapter content. This is a potential XSS vulnerability if the content contains user-supplied or untrusted HTML. Either:
Sanitize the HTML with a library like DOMPurify before rendering, or
Configure a Content Security Policy header on the backend to mitigate XSS
🟡 Suggestions (Non-Blocking)
These are recommendations for improvement. Not required for merge.
[File: frontend/src/reader.css | web/src/reader.css] Typo in media query
Line 52: @media (orientation: landascape) should be @media (orientation: landscape). The typo means landscape orientation styles will never apply.
[File: frontend/src/App.tsx] Duplicate ReadingPage definition
Line 13 defines ReadingPage lazy-loaded from ReadingPage export, but there's already a ReaderPage imported from pages/Reader. This creates confusion between two similarly-named components. Consider renaming or consolidating.
[File: frontend/src/pages/ReadingPage.tsx] Unused book state variable
The book state is fetched via booksApi.getBook(bookId) but the result is never used (the component works with settings and chapters from hooks). The book.title reference on line 78 will crash if book is null while other data has loaded.
📋 AC Coverage
Based on docs/001-customizable-mobile-reading-experience.md
Chapter model — Spec requires apps.books.models.Chapter but not implemented
ReadingProgress model — Spec requires apps.books.models.ReadingProgress but not implemented
ReadingSettings model — Implemented in apps/reader/models.py
Chapters endpoint — /api/books/{id}/chapters/ missing from backend
Chapter content endpoint — /api/books/{id}/chapters/{number}/ missing from backend
Reading progress endpoints — /api/books/{id}/progress/ missing from backend
Frontend ReadingPage — Implemented with chapter navigation
TableOfContents component — Implemented
ReadingSettingsPanel component — Implemented
useReadingSettings hook — Implemented
useChapters hook — Implemented
useReadingProgress hook — Implemented
## Reid's Review — PR #25
**Verdict:** 🔴 Changes Required
---
### 🔴 Blocking Issues
> These MUST be resolved before this PR can be merged.
- **[File: backend/apps/reader/models.py]** Missing migrations for new model
The `ReadingSettings` model was added but no migration file was included. Django requires migrations to be committed alongside model changes. Run `python manage.py makemigrations apps.reader` and include the generated migration.
- **[File: backend/apps/books/models.py]** Missing Chapter and ReadingProgress models
The spec documents `Chapter` and `ReadingProgress` models that should exist under `apps.books`, but no changes to this file are in the diff. The frontend API client (`frontend/src/api/reader.ts`) and `web/src/api/reader.ts` call `/api/books/${bookId}/chapters/` and `/api/books/${bookId}/progress/` endpoints that will fail without these models.
- **[File: backend/apps/books/views.py | backend/apps/books/urls.py]** Missing chapter and progress endpoints
The frontend API client calls:
- `GET /api/books/{id}/chapters/` (list TOC)
- `GET /api/books/{id}/chapters/{number}/` (chapter content)
- `GET /api/books/{id}/progress/` (get progress)
- `PUT /api/books/{id}/progress/` (update progress)
None of these endpoints exist in the backend diff. Without these, the reading view cannot function.
- **[File: frontend/src/pages/ReadingPage.tsx | web/src/pages/ReadingPage.tsx]** XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses `dangerouslySetInnerHTML={{ __html: currentChapter.content }}` to render chapter content. This is a potential XSS vulnerability if the content contains user-supplied or untrusted HTML. Either:
1. Sanitize the HTML with a library like DOMPurify before rendering, or
2. Configure a Content Security Policy header on the backend to mitigate XSS
---
### 🟡 Suggestions (Non-Blocking)
> These are recommendations for improvement. Not required for merge.
- **[File: frontend/src/reader.css | web/src/reader.css]** Typo in media query
Line 52: `@media (orientation: landascape)` should be `@media (orientation: landscape)`. The typo means landscape orientation styles will never apply.
- **[File: frontend/src/App.tsx]** Duplicate ReadingPage definition
Line 13 defines `ReadingPage` lazy-loaded from `ReadingPage` export, but there's already a `ReaderPage` imported from `pages/Reader`. This creates confusion between two similarly-named components. Consider renaming or consolidating.
- **[File: frontend/src/pages/ReadingPage.tsx]** Unused `book` state variable
The `book` state is fetched via `booksApi.getBook(bookId)` but the result is never used (the component works with `settings` and `chapters` from hooks). The `book.title` reference on line 78 will crash if `book` is null while other data has loaded.
---
### 📋 AC Coverage
> Based on docs/001-customizable-mobile-reading-experience.md
- [ ] **Chapter model** — Spec requires `apps.books.models.Chapter` but not implemented
- [ ] **ReadingProgress model** — Spec requires `apps.books.models.ReadingProgress` but not implemented
- [x] ReadingSettings model — Implemented in `apps/reader/models.py`
- [ ] **Chapters endpoint** — `/api/books/{id}/chapters/` missing from backend
- [ ] **Chapter content endpoint** — `/api/books/{id}/chapters/{number}/` missing from backend
- [ ] **Reading progress endpoints** — `/api/books/{id}/progress/` missing from backend
- [x] Reader settings endpoints — `/api/reader/settings/` implemented correctly
- [x] Frontend ReadingPage — Implemented with chapter navigation
- [x] TableOfContents component — Implemented
- [x] ReadingSettingsPanel component — Implemented
- [x] useReadingSettings hook — Implemented
- [x] useChapters hook — Implemented
- [x] useReadingProgress hook — Implemented
These MUST be resolved before this PR can be merged.
[File: web/src/pages/ReadingPage.tsx] XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses dangerouslySetInnerHTML={{ __html: currentChapter.content }} without sanitizing the content. The frontend version (frontend/src/pages/ReadingPage.tsx) correctly added DOMPurify, but this web/ version does not. Either add DOMPurify sanitization or configure CSP headers on the backend.
[File: web/src/reader.css] Typo in media query
Line ~52: @media (orientation: landascape) should be @media (orientation: landscape). The typo means landscape orientation styles will never apply on the web version.
[File: web/src/api/reader.ts] API endpoint mismatch
The API client calls /api/books/${bookId}/chapters/ and /api/books/${bookId}/progress/ but the existing EBookViewSet endpoints use /api/books/ebooks/${bookId}/toc/ and /api/books/ebooks/${bookId}/progress/. Either:
Update the web/api/reader.ts to use the correct endpoints matching the backend, or
Add new endpoints to EBookViewSet with the /chapters/ and /progress/ paths
🟡 Suggestions (Non-Blocking)
These are recommendations for improvement. Not required for merge.
[File: frontend/src/pages/ReadingPage.tsx] Unused book state variable
The book state is fetched via booksApi.getBook(bookId) but only book?.title is used in the toolbar. Consider whether the book fetch is necessary or if title can come from another source.
[File: frontend/src/App.tsx | web/src/App.tsx] Duplicate ReadingPage/ReaderPage components
Two similar components exist (ReaderPage and ReadingPage). The code comments reference both /reader/:id and /read/:id routes. Consider consolidating or clarifying the naming.
✅ Fixed Issues
Issues from first review that have been addressed.
ReadingSettings model — Now has migrations included
BookChapter model — Added to apps/books/models.py
ReadingProgress model fields — Added device_id, device_name, version
XSS vulnerability in frontend — DOMPurify sanitization added
CSS typo in frontend — Fixed to landscape
Backend endpoints — toc and content actions exist in EBookViewSet
📋 AC Coverage
Based on docs/001-customizable-mobile-reading-experience.md
ReadingSettings model — Implemented in apps/reader/models.py with all spec fields
Frontend ReadingPage — Implemented with DOMPurify sanitization
TableOfContents component — Implemented
ReadingSettingsPanel component — Implemented
useReadingSettings hook — Implemented
useChapters hook — Implemented with API mapping
useReadingProgress hook — Implemented with API mapping
## Reid's Review — PR #25 (Updated)
**Verdict:** 🔴 Changes Required
---
### 🔴 Blocking Issues
> These MUST be resolved before this PR can be merged.
- **[File: web/src/pages/ReadingPage.tsx]** XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses `dangerouslySetInnerHTML={{ __html: currentChapter.content }}` without sanitizing the content. The frontend version (`frontend/src/pages/ReadingPage.tsx`) correctly added DOMPurify, but this `web/` version does not. Either add DOMPurify sanitization or configure CSP headers on the backend.
- **[File: web/src/reader.css]** Typo in media query
Line ~52: `@media (orientation: landascape)` should be `@media (orientation: landscape)`. The typo means landscape orientation styles will never apply on the web version.
- **[File: web/src/api/reader.ts]** API endpoint mismatch
The API client calls `/api/books/${bookId}/chapters/` and `/api/books/${bookId}/progress/` but the existing EBookViewSet endpoints use `/api/books/ebooks/${bookId}/toc/` and `/api/books/ebooks/${bookId}/progress/`. Either:
1. Update the web/api/reader.ts to use the correct endpoints matching the backend, or
2. Add new endpoints to EBookViewSet with the `/chapters/` and `/progress/` paths
---
### 🟡 Suggestions (Non-Blocking)
> These are recommendations for improvement. Not required for merge.
- **[File: frontend/src/pages/ReadingPage.tsx]** Unused `book` state variable
The `book` state is fetched via `booksApi.getBook(bookId)` but only `book?.title` is used in the toolbar. Consider whether the book fetch is necessary or if title can come from another source.
- **[File: frontend/src/App.tsx | web/src/App.tsx]** Duplicate ReadingPage/ReaderPage components
Two similar components exist (`ReaderPage` and `ReadingPage`). The code comments reference both `/reader/:id` and `/read/:id` routes. Consider consolidating or clarifying the naming.
---
### ✅ Fixed Issues
> Issues from first review that have been addressed.
- [x] ReadingSettings model — Now has migrations included
- [x] BookChapter model — Added to apps/books/models.py
- [x] ReadingProgress model fields — Added device_id, device_name, version
- [x] XSS vulnerability in frontend — DOMPurify sanitization added
- [x] CSS typo in frontend — Fixed to `landscape`
- [x] Backend endpoints — toc and content actions exist in EBookViewSet
---
### 📋 AC Coverage
> Based on docs/001-customizable-mobile-reading-experience.md
- [x] ReadingSettings model — Implemented in `apps/reader/models.py` with all spec fields
- [x] Chapters endpoint — EBookViewSet.toc exists (GET /api/books/ebooks/{id}/toc/)
- [x] Chapter content endpoint — EBookViewSet.content exists (GET /api/books/ebooks/{id}/content/?page=N)
- [x] Reading progress endpoints — EBookViewSet.progress exists (GET/PUT /api/books/ebooks/{id}/progress/)
- [x] Reading settings endpoints — `/api/reader/settings/` implemented correctly
- [x] Frontend ReadingPage — Implemented with DOMPurify sanitization
- [x] TableOfContents component — Implemented
- [x] ReadingSettingsPanel component — Implemented
- [x] useReadingSettings hook — Implemented
- [x] useChapters hook — Implemented with API mapping
- [x] useReadingProgress hook — Implemented with API mapping
Reid's second review flagged that the old web/ files still had:
- XSS vulnerability (no DOMPurify)
- CSS typo (landascape)
- API endpoint mismatches
All fix: delete web/ entirely. The correct, sanitized, fixed versions
already live in frontend/src/.
crisleo94
approved these changes 2026-05-29 13:48:02 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
crisleo94
Summary
Full customizable mobile reading experience with reading view, font/themes settings, table of contents, and orientation support.
Backend Changes
apps/books/models.py) — stores chapter title, number, and content per book with unique constraint on (book, number)apps/books/models.py) — tracks user's current chapter, position, and percentage per bookapps/reader/models.py) — OneToOne with User, stores font_family, font_size, line_height, margin_width, colors, brightness, orientation_lock, themeapps/readerapp — serializers with validation, GET/PUT/PATCH view for settings auto-creation/api/books/{id}/chapters/(TOC),/api/books/{id}/chapters/{number}/(content),/api/books/{id}/progress/(GET/PUT)/api/reader/settings/Frontend Changes
Docs
docs/001-customizable-mobile-reading-experience.md— full spec with models, API contracts, component tree, types, theme presetsRun migrations after merge:
python manage.py makemigrations && python manage.py migrateReid's Review — PR #25
Verdict: 🔴 Changes Required
🔴 Blocking Issues
[File: backend/apps/reader/models.py] Missing migrations for new model
The
ReadingSettingsmodel was added but no migration file was included. Django requires migrations to be committed alongside model changes. Runpython manage.py makemigrations apps.readerand include the generated migration.[File: backend/apps/books/models.py] Missing Chapter and ReadingProgress models
The spec documents
ChapterandReadingProgressmodels that should exist underapps.books, but no changes to this file are in the diff. The frontend API client (frontend/src/api/reader.ts) andweb/src/api/reader.tscall/api/books/${bookId}/chapters/and/api/books/${bookId}/progress/endpoints that will fail without these models.[File: backend/apps/books/views.py | backend/apps/books/urls.py] Missing chapter and progress endpoints
The frontend API client calls:
GET /api/books/{id}/chapters/(list TOC)GET /api/books/{id}/chapters/{number}/(chapter content)GET /api/books/{id}/progress/(get progress)PUT /api/books/{id}/progress/(update progress)None of these endpoints exist in the backend diff. Without these, the reading view cannot function.
[File: frontend/src/pages/ReadingPage.tsx | web/src/pages/ReadingPage.tsx] XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses
dangerouslySetInnerHTML={{ __html: currentChapter.content }}to render chapter content. This is a potential XSS vulnerability if the content contains user-supplied or untrusted HTML. Either:🟡 Suggestions (Non-Blocking)
[File: frontend/src/reader.css | web/src/reader.css] Typo in media query
Line 52:
@media (orientation: landascape)should be@media (orientation: landscape). The typo means landscape orientation styles will never apply.[File: frontend/src/App.tsx] Duplicate ReadingPage definition
Line 13 defines
ReadingPagelazy-loaded fromReadingPageexport, but there's already aReaderPageimported frompages/Reader. This creates confusion between two similarly-named components. Consider renaming or consolidating.[File: frontend/src/pages/ReadingPage.tsx] Unused
bookstate variableThe
bookstate is fetched viabooksApi.getBook(bookId)but the result is never used (the component works withsettingsandchaptersfrom hooks). Thebook.titlereference on line 78 will crash ifbookis null while other data has loaded.📋 AC Coverage
apps.books.models.Chapterbut not implementedapps.books.models.ReadingProgressbut not implementedapps/reader/models.py/api/books/{id}/chapters/missing from backend/api/books/{id}/chapters/{number}/missing from backend/api/books/{id}/progress/missing from backend/api/reader/settings/implemented correctlyReid's Review — PR #25 (Updated)
Verdict: 🔴 Changes Required
🔴 Blocking Issues
[File: web/src/pages/ReadingPage.tsx] XSS vulnerability — dangerouslySetInnerHTML without sanitization
The code uses
dangerouslySetInnerHTML={{ __html: currentChapter.content }}without sanitizing the content. The frontend version (frontend/src/pages/ReadingPage.tsx) correctly added DOMPurify, but thisweb/version does not. Either add DOMPurify sanitization or configure CSP headers on the backend.[File: web/src/reader.css] Typo in media query
Line ~52:
@media (orientation: landascape)should be@media (orientation: landscape). The typo means landscape orientation styles will never apply on the web version.[File: web/src/api/reader.ts] API endpoint mismatch
The API client calls
/api/books/${bookId}/chapters/and/api/books/${bookId}/progress/but the existing EBookViewSet endpoints use/api/books/ebooks/${bookId}/toc/and/api/books/ebooks/${bookId}/progress/. Either:/chapters/and/progress/paths🟡 Suggestions (Non-Blocking)
[File: frontend/src/pages/ReadingPage.tsx] Unused
bookstate variableThe
bookstate is fetched viabooksApi.getBook(bookId)but onlybook?.titleis used in the toolbar. Consider whether the book fetch is necessary or if title can come from another source.[File: frontend/src/App.tsx | web/src/App.tsx] Duplicate ReadingPage/ReaderPage components
Two similar components exist (
ReaderPageandReadingPage). The code comments reference both/reader/:idand/read/:idroutes. Consider consolidating or clarifying the naming.✅ Fixed Issues
landscape📋 AC Coverage
apps/reader/models.pywith all spec fields/api/reader/settings/implemented correctly