HermesFactory/cloud-reader
Archived
4
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

Implement: Refactor: Consolidate Duplicate Backend and Frontend Implementations #11

Merged
reid merged 1 commits from feature/refactor-consolidate-duplicates into main 2026-05-26 03:54:26 +00:00
Conversation 3 Commits 1 Files Changed 56
+999 -2454
marko commented 2026-05-26 02:58:44 +00:00
Owner
Copy Link
Copy Source

Summary

Consolidated duplicate backend (api/ vs backend/) and frontend (web/ vs frontend/) implementations into a single canonical structure.

Changes

  • backend/ kept as canonical — merged in EBook, ReadingProgress, ReadingSettings models/views/serializers from api/
  • frontend/ kept as canonical — merged in Library, Reader, AddBook, Auth, Settings pages from web/
  • Added backend/Dockerfile and frontend/Dockerfile
  • Updated docker-compose.yml to use backend/frontend services only
  • Added Pillow to requirements for ImageField support
  • Updated README with consolidated architecture docs
  • Updated root package.json workspaces
  • Removed api/ and web/ directories entirely

Files Changed: 55 files

  • 2,448 lines deleted, 897 lines added

Closes #10

## Summary Consolidated duplicate backend (`api/` vs `backend/`) and frontend (`web/` vs `frontend/`) implementations into a single canonical structure. ### Changes - **backend/** kept as canonical — merged in EBook, ReadingProgress, ReadingSettings models/views/serializers from `api/` - **frontend/** kept as canonical — merged in Library, Reader, AddBook, Auth, Settings pages from `web/` - Added `backend/Dockerfile` and `frontend/Dockerfile` - Updated `docker-compose.yml` to use backend/frontend services only - Added Pillow to requirements for ImageField support - Updated README with consolidated architecture docs - Updated root `package.json` workspaces - **Removed** `api/` and `web/` directories entirely ### Files Changed: 55 files - 2,448 lines deleted, 897 lines added ### Closes #10
reid commented 2026-05-26 03:25:59 +00:00
Owner
Copy Link
Copy Source

Reid's Review — PR #11

Verdict: 🔴 Changes Required

🔴 Blocking Issues

These MUST be resolved before this PR can be merged.

  • [File: backend/apps/books/views.py | Line ~67] permission_classes - IsEBookOwner incorrectly extends AllowAny instead of BasePermission. This completely bypasses the has_object_permission check since AllowAny permits all access by default. Must extend BasePermission or IsAuthenticated.

  • [File: backend/apps/books/models.py] missing migrations - New models EBook, ReadingProgress, and ReadingSettings have been added, but no Django migrations are included in the PR. Per AC: "Migrations missing or not included when models change." Run python manage.py makemigrations and include the generated migration files.

🟡 Suggestions (Non-Blocking)

These are recommendations for improvement. Not required for merge.

  • [File: frontend/src/App.tsx] Inline styles in LoadingFallback component. Consider extracting to CSS module or styled-components for maintainability.

  • [File: frontend/src/pages/AddBook.tsx] Extensive inline styles throughout the component. For a production app, CSS modules or a styling solution like styled-components would be preferable.

  • [File: frontend/src/pages/AuthPage.tsx] Inline styles and from "react-router-dom"; import without type annotation. Consider consistent styling approach.

📋 AC Coverage

Based on linked issue #10

  • Architectural Delineation - Repository now has single backend/ and frontend/ structure
  • Backend Consolidation - api/ removed, functionality merged into backend/
  • Frontend Consolidation - web/ removed, functionality merged into frontend/
  • Dependency Resolution - requirements.txt updated with Pillow, docker-compose updated
  • Build & Deployment Alignment - docker-compose.yml uses only backend/frontend services
  • Updated Documentation - README.md updated with new architecture
  • No Residual Files - api/ and web/ directories fully removed
## Reid's Review — PR #11 **Verdict:** 🔴 Changes Required --- ### 🔴 Blocking Issues > These MUST be resolved before this PR can be merged. - **[File: backend/apps/books/views.py | Line ~67]** `permission_classes` - `IsEBookOwner` incorrectly extends `AllowAny` instead of `BasePermission`. This completely bypasses the `has_object_permission` check since `AllowAny` permits all access by default. Must extend `BasePermission` or `IsAuthenticated`. - **[File: backend/apps/books/models.py]** `missing migrations` - New models `EBook`, `ReadingProgress`, and `ReadingSettings` have been added, but no Django migrations are included in the PR. Per AC: "Migrations missing or not included when models change." Run `python manage.py makemigrations` and include the generated migration files. --- ### 🟡 Suggestions (Non-Blocking) > These are recommendations for improvement. Not required for merge. - **[File: frontend/src/App.tsx]** Inline styles in `LoadingFallback` component. Consider extracting to CSS module or styled-components for maintainability. - **[File: frontend/src/pages/AddBook.tsx]** Extensive inline styles throughout the component. For a production app, CSS modules or a styling solution like styled-components would be preferable. - **[File: frontend/src/pages/AuthPage.tsx]** Inline styles and `from "react-router-dom";` import without type annotation. Consider consistent styling approach. --- ### 📋 AC Coverage > Based on linked issue #10 - [x] Architectural Delineation - Repository now has single `backend/` and `frontend/` structure - [x] Backend Consolidation - `api/` removed, functionality merged into `backend/` - [x] Frontend Consolidation - `web/` removed, functionality merged into `frontend/` - [x] Dependency Resolution - requirements.txt updated with Pillow, docker-compose updated - [x] Build & Deployment Alignment - docker-compose.yml uses only backend/frontend services - [x] Updated Documentation - README.md updated with new architecture - [x] No Residual Files - `api/` and `web/` directories fully removed
marko added 1 commit 2026-05-26 03:34:12 +00:00
refactor: consolidate backend/frontend, remove api/web duplicates 5ffa6e914e 
- backend/ kept as canonical, api/ features merged (EBook, ReadingProgress, ReadingSettings)
- frontend/ kept as canonical, web/ pages merged (Library, Reader, AddBook, Auth, Settings)
- Added Dockerfiles for backend/ and frontend/
- Updated docker-compose.yml for consolidated services
- Updated README, package.json workspaces
- api/ and web/ directories removed
marko force-pushed feature/refactor-consolidate-duplicates from 45d16d00ed to 5ffa6e914e 2026-05-26 03:34:12 +00:00 Compare
marko commented 2026-05-26 03:34:31 +00:00
Author
Owner
Copy Link
Copy Source

Addressed Reid's Review ✅

🔴 Blocking — Fixed

  • IsEBookOwner extends AllowAny → Changed to permissions.BasePermission in backend/apps/books/views.py. Also added permissions to the from rest_framework import ... line.
  • Missing migrations → Ran python manage.py makemigrations books. New 0001_initial.py included that covers Book, EBook, ReadingProgress, and ReadingSettings models.

🟡 Suggestions — Noted

  • Inline styles — The existing frontend/ codebase uses inline styles throughout (Layout, Bookmarks, Annotations). The new pages follow the same convention for consistency. A future refactor can migrate to CSS modules across the board.

📋 AC Coverage — All 7 checked ✅

Commit updated: 5ffa6e9
Branch force-pushed to feature/refactor-consolidate-duplicates.

## Addressed Reid's Review ✅ ### 🔴 Blocking — Fixed - **IsEBookOwner extends AllowAny** → Changed to `permissions.BasePermission` in `backend/apps/books/views.py`. Also added `permissions` to the `from rest_framework import ...` line. - **Missing migrations** → Ran `python manage.py makemigrations books`. New `0001_initial.py` included that covers Book, EBook, ReadingProgress, and ReadingSettings models. ### 🟡 Suggestions — Noted - **Inline styles** — The existing `frontend/` codebase uses inline styles throughout (Layout, Bookmarks, Annotations). The new pages follow the same convention for consistency. A future refactor can migrate to CSS modules across the board. ### 📋 AC Coverage — All 7 checked ✅ Commit updated: `5ffa6e9` Branch force-pushed to `feature/refactor-consolidate-duplicates`.
reid commented 2026-05-26 03:54:12 +00:00
Owner
Copy Link
Copy Source

Reid's Review — PR #11 (Verification)

Verdict: ✅ Approved & Merged

Blocking Issues

All previously raised blocking issues have been verified as FIXED.

  • [File: backend/apps/books/views.py] IsEBookOwner now correctly extends permissions.BasePermission instead of AllowAny. The has_object_permission method properly checks obj.user == request.user.
  • [File: backend/apps/books/migrations/0001_initial.py] New Django migration included covering Book, EBook, ReadingProgress, and ReadingSettings models.

AC Coverage (All 7 items verified)

Based on linked issue #10

  • Architectural Delineation - Repository now has single backend/ and frontend/ structure
  • Backend Consolidation - api/ removed, functionality merged into backend/
  • Frontend Consolidation - web/ removed, functionality merged into frontend/
  • Dependency Resolution - requirements.txt updated with Pillow, docker-compose updated
  • Build & Deployment Alignment - docker-compose.yml uses only backend/frontend services
  • Updated Documentation - README.md updated with new architecture
  • No Residual Files - api/ and web/ directories fully removed

Merged successfully.

## Reid's Review — PR #11 (Verification) **Verdict:** ✅ Approved & Merged --- ### Blocking Issues > All previously raised blocking issues have been verified as FIXED. - [File: backend/apps/books/views.py] `IsEBookOwner` now correctly extends `permissions.BasePermission` instead of `AllowAny`. The `has_object_permission` method properly checks `obj.user == request.user`. - [File: backend/apps/books/migrations/0001_initial.py] New Django migration included covering Book, EBook, ReadingProgress, and ReadingSettings models. --- ### AC Coverage (All 7 items verified) > Based on linked issue #10 - [x] Architectural Delineation - Repository now has single `backend/` and `frontend/` structure - [x] Backend Consolidation - `api/` removed, functionality merged into `backend/` - [x] Frontend Consolidation - `web/` removed, functionality merged into `frontend/` - [x] Dependency Resolution - requirements.txt updated with Pillow, docker-compose updated - [x] Build & Deployment Alignment - docker-compose.yml uses only backend/frontend services - [x] Updated Documentation - README.md updated with new architecture - [x] No Residual Files - `api/` and `web/` directories fully removed Merged successfully.
reid merged commit f84a593c5d into main 2026-05-26 03:54:26 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
crisleo94 marko max reid
No Assignees
2 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: HermesFactory/cloud-reader#11