Conversation
## Summary - Fixes critical bug where tag renames (especially case-only changes) don't persist after page refresh - Adds comprehensive debug logging for all tag operations to improve observability ## Details ### Bug Fixes **Tag Rename Race Condition:** - Tag renames would appear successful but revert on page refresh - Root cause 1: Calibre file watcher was syncing back old data after rename completed - Root cause 2: Calibre's `tags` table uses `COLLATE NOCASE` for case-insensitive storage, but our code was doing case-sensitive lookups - Solution: Suspend watcher during operations + use case-insensitive tag lookup + update tag case when it changes **Watcher Debounce Issue:** - Suspending watcher didn't clear pending debounce timer - Solution: Clear timer when suspending to prevent queued events from firing ### Enhancements **Debug Logging:** - Added operation-specific logging prefixes: `[RENAME]`, `[MERGE]`, `[DELETE]`, `[BULK_DELETE]`, `[WATCHER]` - Detailed step-by-step debug logs for troubleshooting - Completion messages at INFO level - Consistent logging pattern across all tag operations ## Files Changed - `lib/calibre-watcher.ts` - Improved suspend/resume + detailed logging - `lib/db/calibre-write.ts` - Case-insensitive tag lookup + case update logic - `lib/services/book.service.ts` - Debug logging for all tag operations ## Testing - ✅ All 56 tests pass - ✅ Linting clean - ✅ TypeScript compilation successful
## Problem Fixes #181 Users accessing Tome over HTTP in production mode (e.g., local networks, Docker deployments without TLS) cannot log in. The authentication cookie's `secure` flag was hardcoded to `process.env.NODE_ENV === "production"`, which assumes production deployments always use HTTPS. When `secure: true` is set on a cookie over HTTP (except localhost/127.0.0.1), browsers reject the cookie. This caused: - **Correct password**: Page stays on login screen (cookie rejected, redirect loop) - **Incorrect password**: Error shown correctly (no cookie involved) The issue was reproduced with a user accessing Tome at `http://192.168.1.158:3000` in production mode. ## Solution Auto-detect the connection protocol from the request instead of relying on `NODE_ENV`: 1. Added `isSecureConnection()` helper that checks: - `x-forwarded-proto` header first (for reverse proxies/load balancers) - Request URL protocol as fallback 2. Updated `createAuthResponse()` to accept the request and use protocol detection 3. Updated login route to pass request to `createAuthResponse()` ## Changes - `lib/auth.ts` - Added protocol detection logic and updated `createAuthResponse()` signature - `app/api/auth/login/route.ts` - Pass request to `createAuthResponse()` - `__tests__/lib/auth.test.ts` - Added 4 comprehensive tests covering HTTP, HTTPS, and proxy scenarios - `package.json` - Bump version to 0.3.1 ## Testing - All tests passing ✓ - New tests verify correct behavior for: - Direct HTTPS requests (secure: true) - Direct HTTP requests (secure: false) - HTTP requests with `x-forwarded-proto: https` (secure: true) - HTTP requests with `x-forwarded-proto: http` (secure: false) ## Test Plan - [ ] Deploy to production mode - [ ] Access over HTTP (e.g., `http://192.168.1.x:3000`) - [ ] Verify login works with correct password - [ ] Verify error shows with incorrect password - [ ] If using reverse proxy, verify `x-forwarded-proto` header is respected 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
## Summary This PR implements a comprehensive refactoring of book completion workflows, introducing auto-completion at 100% progress, a new API client architecture, and service layer consolidation. The changes fix critical bugs with backdated completions, eliminate code duplication, and establish patterns for future development. ## Key Features ### 1. Auto-Completion at 100% Progress ✨ **Fixes #175** - Books automatically transition to "read" status when progress reaches 100%, using the progress date (not today's date) as the completion date. **Benefits:** - Fixes backdated completion bug - More semantic: 100% progress = finished - Single source of truth for completion - Eliminates duplicate completion flows ### 2. Hybrid API Client Architecture 🏗️ **New:** Type-safe API client layer with domain-based organization **Added:** - `BaseApiClient` with comprehensive error handling, timeouts, type safety - `bookApi` domain helper with typed methods - `ApiError` class with structured error details - Foundation for migrating 38 endpoints across 12 domains **Benefits:** - Full TypeScript type safety for all API calls - IDE autocomplete for methods and request/response shapes - Standardized error handling - Easier to test (mock `bookApi` instead of `fetch`) - Single place for auth headers, retry logic, interceptors **Architecture:** ``` React Hooks → Domain API Objects (bookApi) → BaseApiClient → Fetch API ``` See **ADR-010** for detailed rationale. ### 3. Service Layer Consolidation 🔧 **Major refactoring:** Moved all book status transition logic into `SessionService` **Changes:** - Added composable methods to `SessionService`: - `ensureReadingStatus()` - Status transitions - `create100PercentProgress()` - Progress with auto-completion - `updateBookRating()` - Rating updates with Calibre sync - `updateSessionReview()` - Review updates - `markAsRead()` - Unified orchestration method - Simplified `useBookStatus` hook from 534 to 270 lines (49% reduction) - Created `/api/books/:id/mark-as-read` endpoint - Updated rating API to use service layer **Benefits:** - Business logic independently testable without React - API routes and UI hooks use same validated logic - Future features (CLI tools) can reuse methods - Clearer separation: service = business logic, hook = UI concerns See **ADR-011** for comprehensive documentation. ### 4. Complete Book Workflow 📚 **New:** Dedicated workflow for marking books as read from Want to Read/Read Next **Added:** - `CompleteBookModal` - Collect all data upfront (dates, pages, rating, review) - `/api/books/:id/complete` endpoint - Single transaction for complete workflow - Separates "complete from non-reading" vs "finish currently reading" flows **Benefits:** - Better UX for each scenario - All completion data in one step - Consistent behavior across status transitions ## Bug Fixes 1. ✅ Backdated completion dates now preserved correctly (#175) 2. ✅ Reviews saved to reading session table (not book table) 3. ✅ 100% progress log created when marking as read from any status 4. ✅ Duplicate progress creation prevented in FinishBookModal 5. ✅ Reading history refreshes immediately after auto-completion 6. ✅ Client-side bundling issue resolved (no server imports in hooks) 7. ✅ Skip button in modal now properly marks as read without rating ## User Flows ### Scenario 1: Log Progress to 100% (with backdating) ``` 1. User logs progress to 100% with Dec 15 date 2. ✅ Progress service auto-changes status to "read" with completedDate = Dec 15 3. Modal appears for optional rating/review 4. User adds rating → saved to session 5. ✅ completedDate remains Dec 15 (BUG FIXED!) ``` ### Scenario 2: Mark as Read from Want to Read ``` 1. User clicks status dropdown → "Read" 2. CompleteBookModal appears with all fields 3. User enters start date, end date, pages read, rating, review 4. All saved in single transaction 5. Book marked as read with correct dates ``` ### Scenario 3: Manual "Mark as Read" from Reading ``` 1. User clicks "Read" status while actively reading 2. System creates 100% progress (auto-completes) 3. FinishBookModal appears for rating/review 4. User can skip or add rating/review ``` ## Architecture Changes ### New Files Created - `lib/api/base-client.ts` - Base API client with error handling - `lib/api/domains/book/api.ts` - Book domain API methods - `lib/api/domains/book/types.ts` - TypeScript types - `app/api/books/[id]/complete/route.ts` - Complete book endpoint - `app/api/books/[id]/mark-as-read/route.ts` - Mark as read endpoint - `components/CompleteBookModal.tsx` - New completion modal ### Enhanced Components - `useBookStatus.ts` - Simplified by 49% (534 → 270 lines) - `session.service.ts` - Added 9 new composable methods - `progress.service.ts` - Auto-completion logic - `FinishBookModal.tsx` - Updated UX (title, button text) ### Test Coverage - **Added:** 62 new/updated tests - **Total:** 1,848 tests passing (up from 1,830) - **Coverage:** SessionService 100% functions, 95.73% lines - **Coverage:** BaseApiClient 92% ## Documentation ### New ADRs - **ADR-010:** Hybrid API Client Architecture - Decision rationale and alternatives considered - Architecture patterns and benefits - Migration strategy for remaining endpoints - **ADR-011:** Status Transition Service Consolidation - Complete refactoring documentation - Phase-by-phase breakdown with commit references - Architecture diagrams and metrics - Migration guide for developers ### New Test Documentation - **AUTO_COMPLETION_TEST_PLAN.md** - 34+ test scenarios - **AUTO_COMPLETION_TEST_SUMMARY.md** - Executive summary ## Breaking Changes None - all existing flows maintained with backward compatibility. ## Testing ```bash # Run full test suite bun test # Build verification bun run build ``` ## Metrics - **Code Reduction:** 49% in useBookStatus hook (534 → 270 lines) - **Code Addition:** 6,308 insertions (includes tests + docs) - **Test Growth:** +62 new/updated tests (+18 from 1,830 to 1,848) - **Files Changed:** 32 files - **Commits:** 20 commits across 4 major refactoring phases ## Future Enhancements **Backdating Modal for Historical Books** (tracked separately): - When marking book as read with NO existing progress - Allow user to specify start/end dates explicitly - Useful for importing historical reading data ## Related Issues Fixes #175 --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
#191) ## Summary Fixes a follow-up issue from PR #190 where users still couldn't create a 2026 goal after the page was fixed to default to showing 2025. ## Problem After PR #190 made the page default to showing 2025 (the most recent year with a goal), a new issue emerged: - Users viewing 2025 couldn't create a goal for 2026 - The year dropdown only showed years that already had goals - The form didn't respect the selected year from the parent component ## Root Cause 1. `availableYears` only included years from existing goals (2026 was missing from dropdown) 2. `ReadingGoalForm` always used `currentYear` for create mode, ignoring UI context 3. User saw "Create Goal for 2025" but form tried to create for 2026 (or vice versa) ## Solution - **Always include current year in dropdown**, even if no goal exists yet - **Pass `selectedYear` prop to form** to respect user's year selection - **Update validation** to only block future years (allows current/past years per API) ## Changes - `components/GoalsPagePanel.tsx`: - Modified `availableYears` to always include current year - Pass `selectedYear` to `ReadingGoalForm` - `components/ReadingGoalForm.tsx`: - Accept `selectedYear` prop - Use it for create mode instead of always defaulting to `currentYear` - Fix validation to match API (block future years only) ## User Flow (Fixed) 1. User lands on /goals → sees 2025 (has goal) 2. Dropdown shows both 2025 and 2026 3. User selects 2026 → sees "No goal set for 2026" 4. Clicks "Create Goal for 2026" → modal opens with correct year 5. Form submits for 2026 ✅ ## Related - Depends on: #190 - Fixes regression introduced by: #190
## Summary - Remove Pino logger from middleware to resolve production 500 errors when `LOG_DEST` environment variable is set - Simplify middleware to focus solely on authentication logic ## Problem When `LOG_DEST` is set to a file path in production, the application returns 500 errors with the following: ``` TypeError: e$().destination is not a function ``` This occurs because: 1. Pino's `destination()` function requires Node.js filesystem APIs 2. Next.js middleware runs in the Edge Runtime by default (even in Next.js 16) 3. Edge Runtime does not support Node.js `fs` module operations 4. The logger module attempts to create file destinations at module load time, causing the bundle to fail ## Solution Remove logger from middleware entirely because: - Middleware only had 2 debug-level log statements - Authentication flow can be observed through HTTP status codes and container logs - Middleware should be lightweight (Next.js best practice) - API routes and server components still have full logging with file destination support ## Future Work During the Next.js 16 migration, we can: - Migrate from `middleware.ts` to `proxy.ts` (new convention) - Optionally configure Node.js runtime for proxy if logging is truly needed - Evaluate if proxy logging provides enough value to justify the performance trade-off ## Testing - [x] Removed logger imports and calls from middleware.ts - [x] Verified middleware logic remains intact - [x] Confirmed change only affects middleware (API routes retain full logging) ## Files Changed - `middleware.ts`: Removed logger import and debug calls, simplified auth flow
## Summary
Standardizes loading states across all tag management modals on the
`/tags` page, fixing a critical bug where modals would close immediately
before async operations completed, preventing users from seeing loading
feedback.
## Changes
### 🐛 Critical Bug Fix
- **Fixed**: Modals were closing immediately when users clicked confirm,
before async operations completed
- **Result**: Modals now stay open during async operations and display
loading spinners
- Removed premature `onClose()` calls from all modal confirm handlers
### ✨ Loading State Improvements
- **Loading button text**: Buttons now show dynamic text during
operations:
- "Delete Tag" → "Deleting..."
- "Merge Tags" → "Merging..."
- "Rename" → "Renaming..."
- "Remove Tag" → "Removing..."
- "Delete X Tags" → "Deleting..."
- **Disabled buttons during loading**: Cancel and X buttons are disabled
to prevent dismissal mid-operation
- **Loading spinner**: BaseModal displays spinner in body when
`loading={true}`
### 🔄 Error Handling
- Modals now stay open when errors occur
- Users can see error toast and retry the operation
- Modal only closes on successful completion
- Moved modal close logic from `finally` block into `try` block (only on
success)
### 📦 RemoveTagFromBook Modal
- Fixed async handling in both `TagDetailPanel` and
`TagDetailBottomSheet`
- Each `BookCardSimple` component manages its own loading state
- Properly handles async removal with error recovery
- Modal stays open if removal fails, allowing retry
## Files Modified (9 files)
- `components/BaseModal.tsx` - Disabled X button during loading
- `components/TagManagement/DeleteTagModal.tsx` - Removed premature
close, added loading text
- `components/TagManagement/MergeTagsModal.tsx` - Removed premature
close, added loading text
- `components/TagManagement/RenameTagModal.tsx` - Removed premature
close, added loading text
- `components/TagManagement/RemoveTagFromBookModal.tsx` - Removed
premature close, added loading text
- `components/TagManagement/BulkDeleteTagsModal.tsx` - Added loading
text, disabled cancel button
- `components/TagManagement/TagDetailPanel.tsx` - Added async handling
for book card removal
- `components/TagManagement/TagDetailBottomSheet.tsx` - Added async
handling for book card removal
- `app/tags/page.tsx` - Updated 4 handlers to only close modals on
success
## User Experience
### Before
- User clicks "Delete Tag"
- Modal closes immediately
- User sees no feedback
- Tag disappears after network request completes (if successful)
- No way to retry on error
### After
- User clicks "Delete Tag"
- Button text changes to "Deleting..."
- Button is disabled
- Cancel/X button is disabled
- Modal body shows loading spinner
- On success: Modal closes, success toast appears
- On error: Modal stays open, error toast appears, user can retry
## Testing
Manual testing recommended for:
- [ ] Delete tag - verify spinner shows, modal closes on success
- [ ] Merge tags - verify spinner shows, modal closes on success
- [ ] Rename tag - verify spinner shows, modal closes on success
- [ ] Bulk delete tags - verify spinner shows, modal closes on success
- [ ] Remove tag from book - verify spinner shows in both panel and
bottom sheet
- [ ] Error cases - disconnect network, verify modal stays open and user
can retry
## Related
Addresses feedback about inconsistent loading states on the `/tags`
page, especially for modals.
## Summary
- Implement multi-stream logging to write logs to both stdout and file
destinations
- Fixes issue where logs were only visible in log files but not in
Portainer container logs
## Problem
When `LOG_DEST` is configured, Pino was writing logs exclusively to the
file destination, which meant:
- ❌ Logs were NOT visible in Portainer (which shows stdout/stderr)
- ✅ Logs were only available in the file
This made real-time monitoring difficult since you had to access the log
file directly instead of using the container log viewer.
## Solution
Configure Pino to use `multistream()` to write to **both** stdout and
file simultaneously:
1. **Always** write to stdout (for container log viewers like Portainer)
2. **Conditionally** write to file when `LOG_DEST` is set (for
persistence)
This is the industry-standard approach for production logging systems.
## Changes
- Updated `lib/logger.ts` to use `pino.multistream()`
- Created array of stream destinations:
- stdout: Always included
- file: Included when `LOG_DEST` env var is set
- Maintains backward compatibility with all existing logging features
## Benefits
✅ **Real-time visibility**: Logs appear in Portainer/Docker logs
instantly
✅ **Persistent logs**: File logging still works for historical analysis
✅ **No trade-offs**: Get both stdout and file logging simultaneously
✅ **Standard practice**: This is how most production systems handle
logging
✅ **Zero breaking changes**: All existing functionality preserved
## Testing
- [x] Tested locally with `LOG_DEST=/tmp/test.log`
- [x] Verified logs appear on stdout
- [x] Verified logs are written to file
- [x] Server starts without errors
## Configuration
No changes needed - this works with existing env vars:
- `LOG_DEST`: Optional file path for persistent logs
- `LOG_LEVEL`: Controls verbosity (trace, debug, info, warn, error)
- `LOG_PRETTY`: Pretty-print logs in development
- `LOG_ENABLED`: Enable/disable logging
## Example Output
**Stdout (visible in Portainer):**
```json
{"level":"info","time":1767364620665,"msg":"Starting application..."}
```
**File (persistent):**
```json
{"level":"info","time":1767364620665,"msg":"Starting application..."}
```
Both destinations receive identical log entries.
Fix production error where pino.multistream() was being called in browser bundles, causing 'multistream is not a function' errors. Pino's multistream is a Node.js-only feature and cannot be used in client-side code. Changes: - Detect server environment (Node.js/Bun) vs browser using process.versions.node - Only use pino.multistream() on server-side when LOG_DEST is configured - Fall back to standard pino logger for server without LOG_DEST - Use pino browser mode for client-side (logs to console) This ensures: - Server-side: Logs go to both stdout (Portainer) AND file (when LOG_DEST set) - Client-side: Logs work in browser without errors - No breaking changes to existing functionality
Updated funding options and added Buy Me a Coffee username.
## Summary
Implements comprehensive partial success feedback for tag operations,
allowing operations to continue and report detailed results even when
some Calibre sync failures occur.
Previously, if 84 of 85 tag updates succeeded, the entire operation
would fail with a generic error. Now, users see detailed feedback
showing which books succeeded, which failed, and why.
## Changes
### Backend (Calibre & Service Layer)
- **`lib/db/calibre-write.ts`**: Updated `batchUpdateCalibreTags()` to
return `CalibreBatchResult` with detailed failure information instead of
just a success count
- **`lib/services/calibre.service.ts`**: Updated interface to reflect
new return type
- **`lib/services/tag.service.ts`**: Refactored all tag operations to:
- Continue with Tome DB updates even when some Calibre updates fail
- Collect and enrich failure details with book titles for better UX
- Return `TagOperationResult` with comprehensive success/failure
breakdown
- Only throw errors when ALL operations fail (not partial failures)
### Backend (API Routes)
Updated all tag operation endpoints to return new response structure:
- **`app/api/tags/merge/route.ts`** - Tag merge operations
- **`app/api/tags/[tagName]/route.ts`** - Rename (PATCH) and delete
(DELETE) operations
- **`app/api/tags/bulk-delete/route.ts`** - Bulk delete operations
New response format:
```typescript
{
success: boolean, // true if no failures
partialSuccess: boolean, // true if some succeeded, some failed
totalBooks: number,
successCount: number,
failureCount: number,
calibreFailures?: Array<{ calibreId, bookId?, title?, error }>,
tomeFailures?: Array<{ bookId, error }>,
// operation-specific fields...
}
```
### Frontend (UI Components)
- **`types/tag-operations.ts`**: Added shared TypeScript types for
operation results
- **`components/TagManagement/TagOperationResults.tsx`**: New component
for displaying detailed operation results with:
- Success/partial success/failure status indicators
- Expandable list of failed books with titles and error messages
- "Copy Error Report" button for troubleshooting
- Informative notes about successful vs failed updates
- **Updated all modal components** to support "results mode":
- `MergeTagsModal.tsx`
- `RenameTagModal.tsx`
- `DeleteTagModal.tsx`
- `BulkDeleteTagsModal.tsx`
Each modal now:
- Accepts `result` prop to display operation results inline
- Switches between "input mode" and "results mode"
- Keeps modal open to show detailed results on partial success
- Changes action buttons to "Close" when showing results
- **`hooks/useTagManagement.ts`**: Updated to return
`TagOperationResult` and refresh data even on partial success
- **`app/tags/page.tsx`**: Implemented smart toast notifications:
- ✅ **Full success**: Green toast + auto-close modal
- ⚠️ **Partial success**: Yellow warning toast + keep modal open with
details
- ❌ **Complete failure**: Red error toast + keep modal open with details
### Tests
Updated 7 test files to validate new behavior:
- `__tests__/lib/calibre-write.test.ts` - Calibre layer tests
- `__tests__/services/tag.service.test.ts` - Service layer tests
- `__tests__/api/tags.test.ts` - API route tests
- `__tests__/api/tags-bulk-delete.test.ts`
- `__tests__/api/tags-delete.test.ts`
- `__tests__/api/tags-get-patch.test.ts`
- `__tests__/api/tags-merge.test.ts`
All mocks and assertions updated to handle new `TagOperationResult`
structure.
## Example User Experience
### Before
User merges 85 tags → 84 succeed, 1 fails → ❌ "Failed to merge tags"
(generic error, no details)
### After
User merges 85 tags → 84 succeed, 1 fails → ⚠️ Warning toast: "Merged 84
of 85 books. 1 failed - see details in modal"
Modal shows:
- ✅ Successfully merged 84 of 85 books into "target-tag"
- 📋 Failed Books (1) - expandable list showing:
- Book title: "Example Book"
- Calibre ID: 123
- Error: "Calibre database locked"
- 📋 Copy Error Report button
## Testing
- ✅ All 1945 tests passing
- ✅ Build succeeds
- ✅ TypeScript compilation successful
- Manual testing recommended for UX validation
## Files Changed
- 21 files changed
- 1328 insertions, 387 deletions
## Breaking Changes
None - API endpoints maintain backward compatibility by still returning
error responses for complete failures.
…ons (#199) ## Summary Adds 14 comprehensive test cases to cover partial success scenarios for tag operations that were implemented in PR #197. ## What This PR Adds This PR adds thorough test coverage for the partial success functionality implemented in #197, specifically testing the critical behavior that **Tome DB updates ALL books even when some Calibre syncs fail**. ### Service Layer Tests (8 new tests in `tag.service.test.ts`) For each operation (rename, delete, merge, bulkDelete), added: 1. **Partial failure handling test**: Verifies operations continue and Tome DB is updated for ALL books even when some Calibre updates fail 2. **Failure enrichment test**: Verifies `calibreFailures` array contains enriched data (calibreId, bookId, title, error) ### API Layer Tests (4 new tests across route files) For each endpoint, added: - Tests HTTP 200 responses with `partialSuccess: true` - Verifies correct `successCount`, `failureCount`, `totalBooks` - Verifies `calibreFailures` array exists with enriched data - Confirms ALL books in Tome DB were updated Test files updated: - `__tests__/api/tags-get-patch.test.ts` (PATCH rename) - `__tests__/api/tags-delete.test.ts` (DELETE) - `__tests__/api/tags-merge.test.ts` (POST merge) - `__tests__/api/tags-bulk-delete.test.ts` (POST bulk-delete) - `__tests__/services/tag.service.test.ts` (service layer) ## Key Testing Improvements - ✅ Fixes TypeScript type annotations for mock failures arrays - ✅ Uses `mockReturnValueOnce()` to avoid cross-test interference - ✅ Verifies both result structure AND database state - ✅ Uses distinct book titles to verify enrichment works correctly ## Test Results ``` 1957 tests pass 0 failures ``` ## Why This Matters While PR #197 implemented the partial success functionality and updated existing tests to handle the new return types, it lacked explicit test cases for **partial success scenarios** (when some Calibre updates succeed and others fail). This PR ensures that critical behavior is properly tested: - ✅ Operations don't throw errors on partial failures - ✅ Tome DB is always updated, even for books that fail Calibre sync - ✅ Failure information is properly enriched with book details - ✅ API responses correctly indicate partial success ## Related - Builds on #197 (already merged) - Closes testing gaps identified after #197 implementation
…200) ## Summary This PR enhances the tags page user experience with two key improvements: 1. **Scroll position preservation** - Users maintain their position in the tag list when performing operations (rename, delete, merge) 2. **View books in Select Multiple mode** - Users can click tags to view their books while in checkbox mode, with clearly separated click areas ## Changes ### Scroll Position Preservation - Add scroll position tracking to TagList component via forwardRef - Add before/after refetch callbacks to useTagManagement and useTagBooks hooks - Connect scroll preservation in tags page using refs and callbacks - Use requestAnimationFrame for smooth scroll restoration after DOM updates ### Select Multiple Mode Improvements - Separate checkbox click area from tag content click area - Wrap checkbox in container with stopPropagation to prevent conflicts - Move onClick handler to content area (tag name, icon, book count) - Add conditional cursor styling based on checkbox mode - Checkbox clicks only toggle selection, content clicks open tag detail ## User Experience Impact **Scroll Preservation:** - ✅ No more losing your place after tag operations - ✅ Seamless workflow when working with tags further down the list - ✅ Works across all tag operations (rename, delete, merge, bulk operations) **Select Multiple Mode:** - ✅ Click checkbox to select tags for merge/delete - ✅ Click tag content to view which books have that tag - ✅ No accidental tag opening when trying to select - ✅ Makes informed decisions about merging/deleting easier ## Technical Details **Files changed:** 5 files **Lines added:** +125 **Lines removed:** -34 - `app/tags/page.tsx` - Coordinate scroll restoration - `components/TagManagement/TagList.tsx` - Scroll position tracking - `components/TagManagement/TagItem.tsx` - Separated click areas - `hooks/useTagBooks.ts` - Refetch callbacks - `hooks/useTagManagement.ts` - Refetch callbacks ## Testing - ✅ TypeScript compilation passes - ✅ ESLint passes with no warnings - ✅ Scroll position preserved across operations - ✅ Click areas properly separated in checkbox mode - ✅ Works on desktop and mobile layouts
…s modal (#201) ## Summary - Add `allowBackdropClose` prop to BaseModal to prevent accidental dismissal during editing - Make log progress modal full-height on mobile for better screen utilization
…tic generation errors
…er tag operations (#203)
The delete button on book cards in the tags page was using hover states (opacity-0 group-hover:opacity-100) which don't work reliably on iPad and other touch devices. Changed to always visible with a color transition on interaction instead.
Strengthen code coverage requirements to maintain quality bar: - Reduce project threshold from 1% to 0.3% to catch coverage regressions - Set patch target to 80% (matching current project baseline of 81%) - Set patch threshold to 0% to prevent coverage degradation in new code Rationale: At 81% project coverage, the previous 1% threshold was too permissive and could mask significant coverage drops. New code should meet or exceed the existing quality standard. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
## Summary - Fixes the mobile bottomsheet remaining visible behind the "Book Completed!" modal when logging progress to 100% ## Root Cause Z-index stacking conflict: the `BottomSheet` renders at `z-[100]`/`z-[101]` while `FinishBookModal` (via `BaseModal` + `createPortal`) renders at `z-50`. When progress hit 100%, `onClose()` was intentionally skipped to "let FinishBookModal appear" — but the completion modal was actually rendered **behind** the bottomsheet. ## Fix Call `onClose()` in `LogProgressModal.tsx` to dismiss the bottomsheet before showing the completion modal, matching the desktop behavior where `BaseModal` closes and `FinishBookModal` replaces it. **One-line change** in `components/Modals/LogProgressModal.tsx:109`. ## Testing All 3796 tests pass.
## Summary - Fixes inconsistent progress input width when switching between "Page" and "Percentage" modes in the Log Progress modal - The progress input field and date column would shift widths due to nested flex items inheriting content-based intrinsic minimum widths (`min-width: auto`) - Changes the date column from `flex-1` to a fixed `md:w-48` for better proportions since it only needs to fit a date value ## Changes - `components/BookDetail/BookProgress.tsx`: - Add `min-w-0` to progress column and number inputs to override browser intrinsic sizing - Add `shrink-0` to dropdown wrapper div to prevent it from collapsing - Change date column from `flex-1` to fixed `md:w-48 md:shrink-0`
moved to wiki
## Summary - Adds server-side validation to reject progress entries where `currentPage` exceeds `book.totalPages`, preventing corrupted progress data (e.g., logging 1111 pages for a 111-page book) - Validation lives in `calculateProgressMetrics()` so both `logProgress()` and `updateProgress()` paths are covered with a single check - API routes return HTTP 400 with a clear error message: *"Page X exceeds the book's total of Y pages. Please check your input or update the book's page count."* ## Changes **Production code (3 files):** - `lib/services/progress.service.ts` — Added bounds check in `calculateProgressMetrics()` - `app/api/books/[id]/progress/route.ts` — Catch "exceeds" errors → 400 response - `app/api/books/[id]/progress/[progressId]/route.ts` — Catch "exceeds" errors → 400 response **Tests (3 files, 12 new tests):** - `__tests__/integration/services/progress.service.test.ts` — 8 tests covering log + update page bounds - `__tests__/e2e/api/progress/progress.test.ts` — 3 E2E tests for POST endpoint - `__tests__/e2e/api/progress/progress-route-coverage.test.ts` — 1 E2E test for PATCH endpoint ## Design Decisions - **Single validation point:** `calculateProgressMetrics()` is called by both create and update flows, so one check covers everything - **Books without totalPages:** Validation is skipped when `totalPages` is null/undefined (some Calibre books lack page counts) - **Hard reject:** Throws an error rather than silently capping — protects user data integrity per the project constitution ## Testing All 3808 tests pass across 180 test files with zero regressions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…#347) ## Summary Consolidates ~300 lines of duplicate streak calculation logic from `lib/streaks.ts` into `StreakService`, establishing it as the single source of truth for all streak operations. ## Motivation - **Eliminates technical debt** from obsolete Bun workaround (Bun was removed in PR #230 on Jan 6, 2026) - **Fixes timezone bug divergence** between duplicate implementations - **Prevents future bugs** from maintaining logic in two places - **Aligns with project constitution** (DRY principle, service layer pattern) ## Changes ### Code Changes - ✅ Added `getActivityCalendar()` method to StreakService for feature parity - ✅ Fixed critical timezone conversion bug in `StreakService.updateStreaks()` - ✅ Migrated 4 production files to use StreakService: - `app/api/streaks/route.ts` - `app/api/stats/activity/route.ts` - `lib/services/session.service.ts` - `lib/db/seeders/index.ts` - ✅ Deleted `lib/streaks.ts` (400 lines eliminated) - ✅ Removed obsolete Bun workaround documentation - ✅ Fixed 2 dynamic imports in StreakService ### Test Changes - ✅ Updated test files to remove `lib/streaks` references - ✅ Removed obsolete mocks for deleted module - ✅ Simplified tests to use real StreakService implementation ### Documentation Changes - ✅ Updated `docs/ARCHITECTURE.md` (3 references) - ✅ Updated `.specify/memory/patterns.md` (4 examples) ## File Statistics - **11 files modified** - **1 file deleted** (`lib/streaks.ts`) - **Net change**: -455 lines (+119 / -574) ## Testing - ✅ All **3731 tests passing** (no regressions) - ✅ Clean build with zero errors - ✅ Verified zero remaining imports to deleted module ## Bug Fixes Included **Critical timezone bug in `StreakService.updateStreaks()`**: - **Problem**: Was not converting timezone-aware dates back to UTC before storing - **Impact**: Could cause incorrect streak dates when threshold changes mid-day - **Fix**: Now properly converts via `fromZonedTime()` before database storage - **Lines**: `lib/services/streak.service.ts:288-320` ## Related Issues - Fixes #346 - Depends on #345 (merged - timezone fix in lib/streaks.ts) ## Implementation Plan Detailed 8-phase, 27-task implementation breakdown available at `docs/plans/consolidate-streak-logic.md` ## Review Notes This PR has **zero breaking changes** - all API contracts are preserved. The consolidation is purely internal refactoring that eliminates duplicate code and fixes a latent bug in the process. --------- Co-authored-by: Mason Fox <masonfox@pop-os.lan> Co-authored-by: Mason Fox <masonfox@pop-os.tail3f4b5.ts.net>
…rvice lib/streaks.ts is being removed in PR #368 and replaced by the new StreakService singleton. This commit updates all test files that were still mocking the old module to use the new service layer path. Changes: - Update 6 test files to mock @/lib/services/streak.service - Update mock structure to match streakService singleton pattern - All tests verified passing after changes This prevents test failures when lib/streaks.ts is deleted. Resolves module resolution error identified by Copilot code review.
The test suite was renamed to accurately reflect that it now tests StreakService rather than the legacy lib/streaks.ts module. This keeps test output and search results accurate. Resolves Copilot feedback about outdated suite naming.
## Summary Fixes a critical bug where the SQLite AUTOINCREMENT sequence was leaking during Calibre sync operations, causing the sequence to advance unnecessarily on UPDATE operations. ### Root Cause Drizzle ORM's `onConflictDoUpdate()` causes SQLite to increment the AUTOINCREMENT sequence even when performing updates (not just inserts). The query attempts an INSERT first (incrementing sequence), then switches to UPDATE on conflict. The sequence increment is not rolled back. ### Impact - **Before fix**: Sequence advanced by ~851 IDs on every sync cycle (even for updates) - **Production database**: 894 books with IDs 1-636,267, but sequence at 645,512 - **ID waste**: 99.86% waste (644,618 leaked IDs out of 645,512 total) ### Solution **Refactored to split INSERT/UPDATE operations for better architecture:** 1. **New `bulkInsert()` method** - Only performs INSERT operations - Sequence advances once per book (correct behavior) 2. **New `bulkUpdate()` method** - Only performs UPDATE operations - Sequence does NOT advance (correct behavior) 3. **Removed `bulkUpsert()`** - No longer used anywhere - Had performance issues (850+ redundant existence checks per sync) - Caller already knew which books exist via `findAllByCalibreIds()` - Clean removal with no breaking changes 4. **Updated sync service** to call `bulkInsert()` and `bulkUpdate()` separately - Eliminates ~850 redundant queries per sync - Uses existing `existingBooksMap` from line 200 (no extra queries) - Clear separation of concerns (sync service orchestrates, repository executes) ### Architectural Benefits ✅ **Separation of concerns** - Sync service decides insert vs update, repository executes ✅ **No redundant work** - Eliminates 850+ redundant existence checks per sync ✅ **Single responsibility** - Each method does one thing well ✅ **Self-documenting** - Clear intent: `bulkInsert()` vs `bulkUpdate()` ✅ **Performance** - Fewer database queries overall ✅ **Cleaner codebase** - Removed unused, problematic method ### Testing **Fresh database (before fix):** - Sync 1: sequence = 851 ✅ - Sync 2: sequence = 1,702 (+851) ❌ Bug! - Sync 3: sequence = 2,553 (+851) ❌ Bug! **Fresh database (after fix):** - 5 consecutive syncs: sequence stable at 851 ✅ - 0% waste maintained across all syncs ✅ **Test suite:** - Created 5 regression tests in `book-repository-autoincrement.test.ts` - `bulkInsert()` advances sequence correctly - `bulkUpdate()` does NOT advance sequence (critical check) - Multiple update cycles never leak - Mixed insert/update operations - Production scenario: 851 books regression test - Fixed test isolation using unique `calibreId` ranges per test - All 3,822 tests passing ✅ ### Design Decision: No Migration We decided **not** to include a companion migration to reset the sequence on existing databases because: - The gap between MAX(id) and sequence is harmless (SQLite supports 9.2 quintillion IDs) - Avoiding migration complexity reduces maintenance burden and risk - The bug fix itself prevents future leaks, which is what matters Existing databases will have a one-time jump in the sequence (e.g., 636,267 → 645,513) on the next sync, then the sequence will stay aligned with MAX(id) going forward. ### Files Changed - `lib/repositories/book.repository.ts` - Added `bulkInsert()`, `bulkUpdate()`, removed `bulkUpsert()` - `lib/sync-service.ts` - Split insert/update operations, eliminated redundant queries - `__tests__/integration/repositories/books/book-repository-autoincrement.test.ts` - 5 regression tests ### Before & After **Before (fresh database):** ``` Sync 1: 851 books → sequence = 851 Sync 2: 851 books → sequence = 1,702 (leak!) Sync 3: 851 books → sequence = 2,553 (leak!) ``` **After (fresh database):** ``` Sync 1: 851 books → sequence = 851 Sync 2: 851 books → sequence = 851 (stable) Sync 3: 851 books → sequence = 851 (stable) ```
- moved to global
- Add proper teardownTestDatabase() call in afterAll hook to prevent resource leaks - Make beforeAll, beforeEach, and afterAll hooks async with proper await - Import teardownTestDatabase and add missing beforeAll/afterAll imports - Follows standard pattern from other integration tests Addresses GitHub Copilot feedback on PR #371: - r2813552750: Missing teardown prevents SQLite handles from closing - r2813552725: Missing await on async setup/clear causes test flakiness
## Summary
Fixes a bug where users see an error toast "Failed to create streak"
when first enabling streak tracking, even though the streak is
successfully created. Also refactors the streak onboarding flow to match
the Goals pattern for better architectural consistency.
## Problem
When users enable streak tracking:
1. A server action calls `redirect("/streak")` which throws a
`NEXT_REDIRECT` error internally
2. This error bubbles up to the client-side try-catch block in
`StreakOnboarding`
3. The error toast is triggered even though the streak is created
successfully
4. The 303 redirect response is followed correctly, but the user sees a
confusing error message
## Solution
### Phase 1: Quick Fix (bb1dbf4)
- Removed server-side `redirect()` from server action
- Added client-side `router.refresh()` call after successful enable
- Eliminates the NEXT_REDIRECT error without architectural changes
### Phase 2: Architectural Refactor (bb6cda9)
- Refactored streak onboarding to match the Goals pattern (React Query +
API routes)
- Removed the only `"use server"` directive in the codebase for
consistency
- Created consistent architecture:
- Added `enableStreak()` API method with proper types
- Added `enableStreakMutation` to useStreak hook with toast
notifications
- Created `StreakPagePanel` client wrapper component (matches
GoalsPagePanel)
- Simplified `StreakOnboarding` to use hooks directly (no props)
- Refactored `app/streak/page.tsx` to server component pattern
- Updated all tests to mock the useStreak hook
## Changes
**Files Modified (7 total):**
- `/lib/api/domains/streak/types.ts` - Added
EnableStreakRequest/Response types
- `/lib/api/domains/streak/api.ts` - Added enableStreak() method
- `/hooks/useStreak.ts` - Added enable/disable mutation with React Query
- `/components/Streaks/StreakOnboarding.tsx` - Refactored to use
useStreak hook
- `/components/Streaks/StreakPagePanel.tsx` - NEW: Client wrapper
component
- `/app/streak/page.tsx` - Simplified to server component with client
wrapper
- `/__tests__/components/StreakOnboarding.test.tsx` - Updated to mock
hook
## Benefits
1. **Consistency**: Matches Goals pattern used throughout the codebase
2. **No server actions**: Removes confusing "use server" directive
3. **Better UX**: No more error toasts on successful operations
4. **Type safety**: Full TypeScript coverage from API to UI
5. **Better state management**: React Query handles caching and
invalidation
6. **Testability**: Easy to mock and test with established patterns
## Testing
- ✅ All **3815 tests passing**
- ✅ Build completes successfully (no TypeScript errors)
- ✅ Follows established architectural patterns from Goals flow
## Related
- Fixes the only usage of "use server" directive in the codebase
- Aligns with architecture defined in `.specify/memory/patterns.md`
## Summary Fixes #372 - DNF (Did Not Finish) books are no longer counted toward annual reading goals. This PR adds status filtering to the reading goals repository to ensure only books with `status='read'` are counted, bringing Goals page behavior into alignment with the Stats page. ## Problem When a user marked a book as DNF and set a completed date, it was incorrectly counted toward their annual reading goal. This was inconsistent with the Stats page, which correctly excluded DNF books. ### Root Cause The Goals repository was missing a status filter in its counting queries. It counted ANY session with a `completedDate`, regardless of status: - ❌ **Before**: Counted DNF, reading, to-read, AND read sessions - ✅ **After**: Only counts sessions with `status = "read"` ## Changes ### Repository Layer (`lib/repositories/reading-goals.repository.ts`) Added `eq(readingSessions.status, "read")` filter to 4 methods: - `getBooksCompletedInYear()` - Annual goal progress counting - `getYearsWithCompletedBooks()` - Year dropdown filtering - `getBooksCompletedByMonth()` - Monthly breakdown view - `getBooksByCompletionYear()` - Completed books list ### Test Coverage (`__tests__/integration/repositories/reading-goals.repository.test.ts`) Added comprehensive test suite with 8 new test cases: - ✅ Verify DNF books are excluded from all 4 methods - ✅ Verify other non-read statuses (reading, to-read) are also excluded - ✅ Verify only `status='read'` counts toward goals ## Test Results All **3,843 tests pass** including 8 new tests. No regressions introduced. ## Impact **Before this fix:** - DNF books incorrectly counted toward reading goals - Goals page showed inflated progress - Inconsistent with Stats page behavior **After this fix:** - ✅ DNF books NOT counted toward reading goals - ✅ Only books with `status='read'` count - ✅ Goals page matches Stats page behavior - ✅ Accurate goal progress tracking ## Related This aligns with the previous consolidation effort in commit `18666cc` which created shared `ReadingStatsService` for Stats/Goals consistency. This PR completes that alignment by adding the missing status filter. Co-authored-by: OpenCode <opencode@anomalyco.ai>
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #374 +/- ##
=======================================
Coverage 78.00% 78.01%
=======================================
Files 162 162
Lines 7307 7308 +1
Branches 1763 1765 +2
=======================================
+ Hits 5700 5701 +1
Misses 1139 1139
Partials 468 468
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Release bump to v0.6.5 that consolidates recent fixes: enforce ADR-014 date-string handling, correct reading goal counting by excluding non-read sessions (DNF/etc.), and refactor streak onboarding to a React Query + API route pattern.
Changes:
- Enforce ADR-014 across services/validation by comparing/sorting YYYY-MM-DD strings instead of constructing
Dateobjects. - Update reading-goals repository queries to count only
status = "read"sessions and add integration coverage for DNF exclusion. - Refactor streak enable flow away from server-action redirect into a typed API client +
useStreakmutation, with component/test updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Bumps package version to 0.6.5. |
| lib/services/session.service.ts | Updates completed-session sorting to lexicographic date-string comparison (ADR-014). |
| lib/services/progress.service.ts | Passes date strings into validation and replaces date sorting/comparison with string ops. |
| lib/services/progress-validation.ts | Changes validation API to accept date strings and uses shared formatDate() for display. |
| lib/repositories/reading-goals.repository.ts | Filters reading-goal completion queries to status="read" (excludes DNF/etc.). |
| lib/api/domains/streak/types.ts | Adds request/response types for enabling/disabling streak tracking. |
| lib/api/domains/streak/api.ts | Adds enableStreak() PATCH helper for /api/streak. |
| hooks/useStreak.ts | Adds React Query mutation for enable/disable streak with cache invalidation + toasts. |
| docs/ADRs/ADR-014-DATE-STRING-STORAGE.md | Documents common pitfalls and correct patterns for ADR-014 compliance. |
| components/Streaks/StreakPagePanel.tsx | New client wrapper to render onboarding vs analytics/error state consistently. |
| components/Streaks/StreakOnboarding.tsx | Refactors onboarding to call useStreak().enableStreak directly. |
| app/streak/page.tsx | Refactors page to server component + client panel; removes server-action redirect path. |
| tests/utils/dateHelpers.test.ts | Minor formatting-only import change. |
| tests/unit/api-clients/streak.api.test.ts | Adds unit tests for streakApi.enableStreak. |
| tests/integration/services/progress-validation.test.ts | Updates integration tests to pass YYYY-MM-DD strings into validation. |
| tests/integration/repositories/reading-goals.repository.test.ts | Adds integration coverage ensuring DNF/non-read statuses are excluded from goals. |
| tests/hooks/useStreak.test.ts | Adds hook-level tests for enable/disable streak mutation behavior. |
| tests/components/StreakOnboarding.test.tsx | Updates component tests to mock useStreak instead of prop-based enable flow. |
Comment on lines
177
to
185
| * Get all books completed in a specific year | ||
| * Returns books with their completion dates, ordered by completion date descending | ||
| * Only returns sessions with status='read' (excludes DNF, reading, etc.) | ||
| */ | ||
| /** | ||
| * Get books completed in a specific year | ||
| * Returns books with completedDate as string (YYYY-MM-DD) | ||
| * Only returns sessions with status='read' (excludes DNF, reading, etc.) | ||
| */ |
There was a problem hiding this comment.
There are two consecutive JSDoc blocks above getBooksByCompletionYear(). This looks like an accidental duplicate and makes the docs harder to maintain—please merge into a single block.
Suggested change
| * Get all books completed in a specific year | |
| * Returns books with their completion dates, ordered by completion date descending | |
| * Only returns sessions with status='read' (excludes DNF, reading, etc.) | |
| */ | |
| /** | |
| * Get books completed in a specific year | |
| * Returns books with completedDate as string (YYYY-MM-DD) | |
| * Only returns sessions with status='read' (excludes DNF, reading, etc.) | |
| */ | |
| * Get all books completed in a specific year. | |
| * Returns books with their completion dates as strings (YYYY-MM-DD), | |
| * ordered by completion date descending. | |
| * Only returns sessions with status='read' (excludes DNF, reading, etc.). | |
| */ |
|
|
||
| **Commits:** | ||
| - Branch: `fix/adr-014-date-violations` | ||
| - PR: [Link to PR once created] |
There was a problem hiding this comment.
The ADR includes a placeholder PR: [Link to PR once created]. Since this is in a release PR, please either replace it with the actual PR link(s) or remove the placeholder to avoid stale documentation.
Suggested change
| - PR: [Link to PR once created] |
Comment on lines
+169
to
+175
| data: { | ||
| streakEnabled: true, | ||
| currentStreak: 0, | ||
| longestStreak: 0, | ||
| dailyReadingThreshold: 10, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The mocked API response uses dailyReadingThreshold, but the streak schema/API response field is dailyThreshold (see streaks schema and /api/streak route). Using the wrong property name in test fixtures makes the test less representative and can mask type/contract regressions.
Comment on lines
+205
to
+210
| success: true, | ||
| data: { | ||
| streakEnabled: true, | ||
| dailyReadingThreshold: 5, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Same issue here: the enable-streak test fixture uses dailyReadingThreshold but the contract appears to use dailyThreshold. Updating the fixture to match the real field name will keep this test aligned with the actual API payload/response shape.
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.
Release v0.6.5
This release includes important bug fixes and improvements to date handling, reading goals, and streak functionality.
Key Changes
Bug Fixes
Improvements
Testing
This release builds on v0.6.4 with critical fixes to ensure data integrity and accurate tracking.