feat: add synchronized reading progress for bookmarks #2302
Conversation
Track and restore reading position in reader mode across web and mobile. Uses text character offsets (matching the highlights pattern) to find the first visible paragraph and scroll back to it on return. - Add readingProgressOffset field to bookmarkLinks table - Add updateReadingProgress tRPC mutation - Create shared useReadingProgress hook with auto-save on unload - Integrate in web full-screen reader (/reader/[bookmarkId]) - Integrate in mobile reader mode via WebView injectedJavaScript - Only tracks progress for bookmark owners 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements per-user reading progress for link bookmarks: DB migration, API mutation and query augmentation, shared DOM/WebView utilities and bundle generation, client hook and component integrations (web and mobile), and tests; adds a pre-commit check to validate the generated WebView bundle. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,json,css,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/mobile/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
🔇 Additional comments (4)
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: 2
🧹 Nitpick comments (7)
packages/trpc/routers/bookmarks.ts (1)
587-604: Consider validating bookmark type before updating.The mutation updates
bookmarkLinkswithout verifying the bookmark is actually a LINK type. If called on a TEXT or ASSET bookmark, the update will succeed but affect zero rows (since no matchingbookmarkLinks.idexists), which is silent and may hide bugs.🔎 Suggested improvement
updateReadingProgress: authedProcedure .input( z.object({ bookmarkId: z.string(), readingProgressOffset: z.number().int().nonnegative(), readingProgressAnchor: z.string().max(ANCHOR_TEXT_MAX_LENGTH).nullish(), }), ) .use(ensureBookmarkOwnership) .mutation(async ({ input, ctx }) => { - await ctx.db + const result = await ctx.db .update(bookmarkLinks) .set({ readingProgressOffset: input.readingProgressOffset, readingProgressAnchor: input.readingProgressAnchor ?? null, }) .where(eq(bookmarkLinks.id, input.bookmarkId)); + + if (result.changes === 0) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Reading progress can only be set for link bookmarks", + }); + } }),apps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsx (1)
169-170: Non-null assertion on ref may cause issues if accessed before mount.The
useImperativeHandlewith an empty dependency array andcontentRef.current!assertion could returnnullif the parent accesses the ref before the component mounts. Consider returning the ref conditionally or addingcontentRef.currentto the dependency array.🔎 Suggested improvement
// Expose the content div ref to parent components - useImperativeHandle(ref, () => contentRef.current!, []); + useImperativeHandle( + ref, + () => contentRef.current as HTMLDivElement, + [contentRef.current], + );Alternatively, if the parent component always accesses the ref after mount (which seems to be the case with the reading progress hooks), the current implementation is acceptable.
apps/web/components/dashboard/preview/ReaderView.tsx (1)
91-96: Minor: Redundant fallback forhtmlContent.On line 96,
cachedContent || ""is unnecessary since we're inside theelsebranch wherecachedContentis already truthy (line 90 checks!cachedContent).Suggested fix
content = ( <BookmarkHTMLHighlighter ref={ref} className={className} style={style} - htmlContent={cachedContent || ""} + htmlContent={cachedContent} highlights={highlights?.highlights ?? []}packages/shared/utils/reading-progress-dom.ts (1)
78-95: Edge case: Last paragraph may not be captured when scrolled to bottom.When the user has scrolled past all paragraphs (all paragraph tops are above
viewportTop), and no paragraph spans the viewport top,topParagraphremainsnull. This can happen when viewing the last section of content.Consider capturing the last paragraph when no paragraph is at or below the viewport top:
Suggested fix
+ let lastParagraph: Element | null = null; + for (const paragraph of paragraphs) { const rect = paragraph.getBoundingClientRect(); + lastParagraph = paragraph; // If this paragraph's top is at or below the viewport top, it's our target if (rect.top >= viewportTop) { topParagraph = paragraph; break; } // If this paragraph spans the viewport top (started above, ends below), use it if (rect.top < viewportTop && rect.bottom > viewportTop) { topParagraph = paragraph; break; } } - if (!topParagraph) return null; + if (!topParagraph) { + // If all paragraphs are above viewport, use the last one + topParagraph = lastParagraph; + } + + if (!topParagraph) return null;packages/shared/utils/reading-progress-core.ts (1)
85-94: Fuzzy matching may miss valid matches for short anchors.The fuzzy fallback requires
anchor.length >= 20(line 90), but if a paragraph's text is shorter than 20 characters, the original anchor would also be short, and this condition would never match. This could cause restoration failures for short paragraphs.Consider also checking if the paragraph anchor starts with the saved anchor for shorter texts:
Suggested enhancement
// Fuzzy fallback: check if first 20 chars match for (const paragraph of paragraphs) { const paragraphAnchor = extractAnchorText(paragraph); if ( - paragraphAnchor.slice(0, 20) === anchor.slice(0, 20) && - anchor.length >= 20 + (paragraphAnchor.slice(0, 20) === anchor.slice(0, 20) && + anchor.length >= 20) || + (anchor.length < 20 && paragraphAnchor.startsWith(anchor)) ) { return paragraph; } }apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
168-183: Potential issue:saveProgresscalled during cleanup may use stale refs.In the cleanup function (line 178-182),
saveProgress()is called aftersubscription.remove(). However, at this point, React may have already started unmounting, andcurrentPosition.currentcould be accessed in an unstable state.Additionally,
saveProgressis in the dependency array, which could cause the effect to re-run unnecessarily whensaveProgresschanges (though it's memoized withuseCallback).Consider calling save before removing the subscription:
Suggested fix
useEffect(() => { if (!isOwner) return; const subscription = AppState.addEventListener("change", (status) => { if (status === "background" || status === "inactive") { saveProgress(); } }); return () => { - // Save on unmount - saveProgress(); subscription.remove(); + // Save on unmount - call after removing listener to avoid double-save race + saveProgress(); }; }, [isOwner, saveProgress]);Actually, the order is fine since we want to save before fully cleaning up. The real concern is that
saveProgressaccesses refs. This should work correctly since refs persist through cleanup. The current implementation looks acceptable.packages/shared-react/hooks/reading-progress.ts (1)
222-225: Intentional missing dependency array for ref updates.The effect at lines 222-225 intentionally omits the dependency array to run on every render, keeping
savePositionRefandrestorePositionRefcurrent. This is a valid pattern for avoiding stale closures in event handlers.Consider adding a comment to clarify this is intentional:
Suggested clarification
useEffect(() => { savePositionRef.current = progress.savePosition; restorePositionRef.current = progress.restorePosition; - }); + }); // Intentionally no deps - always sync refs on every render
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.husky/pre-commitapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsxapps/web/components/dashboard/preview/BookmarkPreview.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/db/drizzle/0073_add_reading_progress_offset.sqlpackages/db/drizzle/meta/0073_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/open-api/karakeep-openapi-spec.jsonpackages/shared-react/hooks/reading-progress.tspackages/shared/index.tspackages/shared/package.jsonpackages/shared/scripts/build-webview-js.tspackages/shared/types/bookmarks.tspackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-webview.generated.tspackages/trpc/models/bookmarks.tspackages/trpc/routers/bookmarks.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/db/schema.tspackages/shared/types/bookmarks.tspackages/shared/scripts/build-webview-js.tsapps/web/components/dashboard/preview/BookmarkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxpackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared/utils/reading-progress-webview.generated.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/db/schema.tspackages/shared/types/bookmarks.tspackages/shared/scripts/build-webview-js.tsapps/web/components/dashboard/preview/BookmarkPreview.tsxpackages/db/drizzle/meta/_journal.jsonapps/web/app/reader/[bookmarkId]/page.tsxpackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/open-api/karakeep-openapi-spec.jsonpackages/shared/utils/reading-progress-webview.generated.tspackages/shared/package.json
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/db/schema.tspackages/shared/types/bookmarks.tspackages/shared/scripts/build-webview-js.tsapps/web/components/dashboard/preview/BookmarkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxpackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared/utils/reading-progress-webview.generated.ts
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for database schema and migrations in the db package
Files:
packages/db/schema.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/types/bookmarks.tspackages/shared/scripts/build-webview-js.tspackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-webview.generated.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/preview/BookmarkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsx
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.ts
packages/shared-react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place shared React components and hooks in
packages/shared-react
Files:
packages/shared-react/hooks/reading-progress.ts
apps/mobile/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Expo for developing the mobile application
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Run `pnpm typecheck` to verify TypeScript type safety across the codebase
Applied to files:
.husky/pre-commit
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Start the web application using `pnpm web` and background workers using `pnpm workers`
Applied to files:
.husky/pre-commit
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Use pnpm as the package manager for all dependency installations
Applied to files:
.husky/pre-commit
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Lint code using oxlint and fix issues with `pnpm lint:fix`
Applied to files:
.husky/pre-commit
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/shared/**/*.{ts,tsx} : Organize shared code and types in the `packages/shared` directory for use across packages
Applied to files:
packages/shared/package.json
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Use Next.js for building the main web application
Applied to files:
packages/shared/package.json
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use Vitest for writing and running tests
Applied to files:
packages/shared/package.json
🧬 Code graph analysis (4)
apps/web/components/dashboard/preview/BookmarkPreview.tsx (1)
apps/web/components/ui/tabs.tsx (1)
TabsList(54-54)
packages/shared/utils/reading-progress-dom.ts (1)
packages/shared/utils/reading-progress-core.ts (4)
ReadingPosition(11-14)PARAGRAPH_SELECTOR_STRING(28-28)extractAnchorText(58-61)normalizeTextLength(50-52)
apps/web/components/dashboard/preview/LinkContentSection.tsx (1)
packages/shared-react/hooks/reading-progress.ts (1)
useReadingProgressAutoSave(211-449)
apps/web/components/dashboard/preview/ReaderView.tsx (2)
packages/db/schema.ts (1)
highlights(318-348)packages/shared-react/hooks/highlights.ts (3)
useCreateHighlight(3-17)useUpdateHighlight(19-33)useDeleteHighlight(35-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (29)
.husky/pre-commit (1)
4-4: LGTM! Pre-commit validation for WebView bundle.Adding the WebView JS validation step ensures the generated reading-progress bundle stays synchronized with source changes, preventing stale artifacts from being committed.
packages/db/drizzle/meta/_journal.json (1)
516-522: LGTM! Migration journal entry added correctly.The journal entry for the reading progress migration follows the standard Drizzle format and is properly sequenced.
packages/db/drizzle/0073_add_reading_progress_offset.sql (1)
1-2: LGTM! Database migration adds reading progress columns.The migration correctly adds nullable columns for reading progress tracking. Making these nullable is appropriate for a new feature that won't have values for existing bookmarks.
packages/db/schema.ts (1)
256-259: LGTM! Schema updated with reading progress fields.The schema changes align with the migration and include helpful inline comments. Making the fields nullable is the right choice for backward compatibility.
packages/open-api/karakeep-openapi-spec.json (1)
221-230: LGTM! OpenAPI spec extended with reading progress fields.The new properties are correctly defined as optional and nullable, with appropriate constraints (minimum: 0 for offset). This aligns with the database schema and TypeScript types.
packages/shared/types/bookmarks.ts (1)
58-59: LGTM! TypeScript types extended with reading progress fields.The Zod schema definitions correctly mirror the database schema and OpenAPI spec. The validation constraints (integer, non-negative) are appropriate for a character offset value.
apps/web/components/dashboard/preview/LinkContentSection.tsx (4)
1-3: LGTM! Import additions support reading progress tracking.The new imports (
useRefanduseReadingProgressAutoSave) are correctly sourced and necessary for the reading progress integration.Also applies to: 37-37
118-118: LGTM! Content ref properly initialized.The
contentRefis correctly typed and will be used to track the scrollable container for reading progress.
132-143: LGTM! Reading progress auto-save correctly integrated.The auto-save setup properly:
- Derives initial position from bookmark content
- Gates tracking by ownership and section type
- Passes all required parameters to the hook
The
enabledcondition (isOwner && section === "cached") correctly limits tracking to the reader view where the user is the owner, avoiding progress tracking in archive/screenshot/video modes.
160-160: LGTM! Ref correctly forwarded to ReaderView.The
contentRefis properly forwarded to theReaderViewcomponent, enabling the auto-save hook to track scroll position within the content container.Based on learnings, the component integration follows React best practices for ref forwarding.
apps/web/app/reader/[bookmarkId]/page.tsx (1)
38-55: LGTM! Reading progress auto-save integration is well-implemented.The ownership check (
enabled: isOwner) correctly ensures only bookmark owners can track progress. The initial offset/anchor extraction properly handles the LINK type guard, and the hook integration follows the expected pattern.packages/shared/package.json (1)
25-32: LGTM! Build tooling additions are well-structured.The scripts follow a good pattern with generate, watch, and check modes. The esbuild dependency is appropriate for bundling the WebView script.
packages/trpc/models/bookmarks.ts (1)
188-189: LGTM! Reading progress fields correctly propagated to bookmark content.The fields are consistently added in both the single-bookmark normalization path (
toZodSchema) and the multi-bookmark loading path, ensuring proper API surface coverage.packages/shared/utils/reading-progress-webview.generated.ts (1)
1-19: LGTM! Auto-generated WebView script is well-documented.Clear header comments indicate the file should not be edited manually and provide regeneration instructions. The exposed API surface is documented in the JSDoc comment.
packages/shared/scripts/build-webview-js.ts (1)
32-43: LGTM! esbuild configuration is appropriate for WebView injection.The IIFE format with
__readingProgressglobal name, ES2015 target for broad compatibility, and minification are all correct choices for React Native WebView injection.packages/shared/utils/reading-progress-webview-src.ts (2)
74-85: The fallback return at line 84 may produce unexpected results.If
topParagraph.contains(node)never returns true during the TreeWalker traversal (which shouldn't happen iftopParagraphis withincontainer), line 84 returns the total text length as the offset. This fallback is logically correct but indicates an edge case that might warrant a defensive check.Consider whether this edge case can occur in practice (e.g., if
topParagraphis a descendant ofcontainerbut has no text nodes). If so, returningnullmight be more appropriate than returning a potentially incorrect offset.
34-65: LGTM! WebView-specific reading position logic is well-implemented.The function correctly handles finding the topmost visible paragraph by checking both "at or below viewport" and "spanning viewport" cases. The TreeWalker-based offset calculation is appropriate for WebView contexts.
apps/web/components/dashboard/preview/BookmarkHtmlHighlighter.tsx (1)
151-166: LGTM! forwardRef conversion enables reading progress tracking.The conversion to
forwardRefwithuseImperativeHandlecorrectly exposes the internal content div, enabling parent components to integrate with the reading progress hooks.apps/web/components/dashboard/preview/ReaderView.tsx (2)
22-39: LGTM on component structure and data fetching.The forwardRef pattern is correctly implemented, and the query with
selecttransformer properly extractshtmlContentfor LINK bookmarks.
41-81: Mutation handlers are well-structured with consistent toast feedback.The three highlight mutation hooks follow a consistent pattern with success/error toasts. Cache invalidation is handled by the underlying hooks from
@karakeep/shared-react/hooks/highlights.packages/shared/utils/reading-progress-dom.ts (1)
30-53: LGTM on scrollable parent detection.The implementation correctly handles:
- Standard overflow-based scrolling
- Radix ScrollArea viewports via data attribute
- Window-level scrolling fallback
Good defensive approach checking both
isCandidateandhasScrollContent.packages/shared/utils/reading-progress-core.ts (2)
40-45: LGTM on text normalization.The normalization correctly handles newlines, tabs, and multiple spaces, ensuring consistent offset calculation across different HTML formatting.
103-168: LGTM on scrollToReadingPosition implementation.The dual-strategy approach (anchor-first with offset fallback) is robust. The TreeWalker traversal correctly finds the enclosing paragraph element and scrolls to it. Returning a boolean success flag is useful for callers to know if restoration succeeded.
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (2)
49-97: Well-structured WebView injection script.The script correctly:
- Embeds the shared READING_PROGRESS_WEBVIEW_JS bundle
- Extracts functions from the IIFE global
- Restores initial position with a delay for content loading
- Debounces scroll events (500ms)
- Has periodic backup reporting (10s interval)
The
true;at the end satisfies WebView's injectedJavaScript requirements.
201-209: LGTM on conditional script injection.The ownership check ensures only bookmark owners have progress tracking enabled. Non-owners receive a no-op
"true;"script, which is efficient.packages/shared-react/hooks/reading-progress.ts (4)
19-47: Good coordination mechanism for restoration across instances.The module-level
restorationClaimedMap with 5-second expiry effectively handles:
- React StrictMode double-mounting
- Multiple component instances viewing the same bookmark
The claim/release pattern prevents duplicate scroll restorations.
253-274: Robust scroll listener setup with Radix ScrollArea support.The logic correctly identifies the scrollable parent (using the shared
findScrollableParent) and attaches scroll listeners with{ passive: true }for performance. Window vs. container scrolling is properly distinguished.
376-446: Well-designed restoration flow with RAF polling.The implementation handles several edge cases well:
- Visibility check before claiming lock (line 398)
- Layout readiness detection (lines 407-411)
- Lock release on cleanup (lines 440-443)
- Cancellation via
state.cancelledobjectThe 120-attempt limit (~2s at 60fps) provides a reasonable timeout for content loading.
358-367: Mutation calls during unmount are safe and will not be dropped.The code correctly calls
saveCurrentProgress()during cleanup. With React Query v5 and TRPC v11, callingmutate()queues the request immediately and sends it—the request completes even if the component unmounts before receiving the response. The mutation request will not be dropped, and the database update will succeed. TheonSuccesscallback won't fire if the component unmounts before the response arrives, but that's expected behavior and doesn't prevent the data from being saved.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/shared/scripts/build-webview-js.ts">
<violation number="1" location="packages/shared/scripts/build-webview-js.ts:102">
P2: Watch mode only monitors the entry file, not its imported dependency `reading-progress-core.ts`. Changes to the core module won't trigger regeneration. Consider also watching the core file, using esbuild's built-in watch API, or documenting this limitation.</violation>
</file>
<file name="packages/shared/utils/reading-progress-webview-src.ts">
<violation number="1" location="packages/shared/utils/reading-progress-webview-src.ts:79">
P2: If `topParagraph` has no text content (empty paragraph or only non-text elements), the TreeWalker loop completes without returning, and the final `return { offset, anchor }` returns the total document text length as the offset, which is incorrect. Consider returning `null` to indicate the position couldn't be calculated.</violation>
</file>
<file name="packages/shared-react/hooks/reading-progress.ts">
<violation number="1" location="packages/shared-react/hooks/reading-progress.ts:243">
P2: The scroll handler calls `getReadingPosition()` on every scroll event without throttling. This function performs expensive DOM operations (querySelectorAll, TreeWalker traversal) that could cause jank during smooth scrolling. Consider adding a throttle (e.g., 100-200ms) since the position only needs to be captured for save-on-unload scenarios, not real-time tracking.</violation>
</file>
<file name="packages/trpc/routers/bookmarks.ts">
<violation number="1" location="packages/trpc/routers/bookmarks.ts:597">
P2: Missing check for `result.changes == 0` after updating `bookmarkLinks`. If called on a non-link bookmark, the update silently does nothing. The `updateBookmark` mutation follows a pattern of checking if link-specific updates affected any rows and throwing an error if not.</violation>
</file>
<file name="packages/shared/utils/reading-progress-dom.ts">
<violation number="1" location="packages/shared/utils/reading-progress-dom.ts:115">
P2: If `topParagraph` contains no text nodes (e.g., empty or contains only images), this returns the total text length of the entire container instead of the position of the paragraph. This would cause scroll restoration to jump to the wrong location. Consider returning `null` here to indicate the position couldn't be determined.</violation>
</file>
<file name="apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx">
<violation number="1" location="apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx:113">
P1: Race condition: `currentUser` may still be loading when the WebView renders, causing `isOwner` to be `false` and the reading progress script to not be injected. Since `injectedJavaScript` only runs once on page load, the script won't be injected even after `currentUser` loads. Consider waiting for `currentUser` to be loaded (check `isLoading` from `useWhoAmI`) before rendering the WebView, or handle the case where `currentUser` is undefined by not determining ownership until it's available.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shared/scripts/build-webview-js.ts (2)
46-46: Consider defensive check for outputFiles.While esbuild typically throws before returning when builds fail, adding a null check would make the code more defensive and provide clearer errors if esbuild behavior changes.
🔎 Optional defensive check
+ if (!result.outputFiles?.[0]) { + throw new Error("esbuild did not produce any output files"); + } const jsCode = result.outputFiles[0].text;
96-123: Watch mode now correctly monitors both source and core files.This implementation resolves the concerns raised in previous reviews—both
SRC_FILEandCORE_FILEare now watched (lines 103 and 113), ensuring changes to dependencies trigger rebuilds.
Optional: Refactor duplicate watch handlers.
The two
fs.watchblocks (lines 103-112 and 113-122) contain nearly identical logic. Consider extracting a helper function to reduce duplication.🔎 Optional refactoring to reduce duplication
+ const watchFile = (filePath: string, label: string) => { + fs.watch(filePath, async (eventType) => { + if (eventType === "change") { + console.log(`\n${label} changed, rebuilding...`); + try { + await build(); + } catch (err) { + console.error("Build error:", err); + } + } + }); + }; + - // Watch for changes - fs.watch(SRC_FILE, async (eventType) => { - if (eventType === "change") { - console.log("\nSource file changed, rebuilding..."); - try { - await build(); - } catch (err) { - console.error("Build error:", err); - } - } - }); - fs.watch(CORE_FILE, async (eventType) => { - if (eventType === "change") { - console.log("\nCore file changed, rebuilding..."); - try { - await build(); - } catch (err) { - console.error("Build error:", err); - } - } - }); + watchFile(SRC_FILE, "Source file"); + watchFile(CORE_FILE, "Core file");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shared/scripts/build-webview-js.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/shared/scripts/build-webview-js.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/shared/scripts/build-webview-js.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/shared/scripts/build-webview-js.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/scripts/build-webview-js.ts
🔇 Additional comments (2)
packages/shared/scripts/build-webview-js.ts (2)
1-28: LGTM! Clean setup with proper paths.The shebang, imports, and path constants are well-structured. The inclusion of
CORE_FILEat line 24 sets up dependency tracking for the watch mode.
125-135: LGTM! Proper error handling for both modes.The main execution block correctly branches between watch and build modes, with appropriate error handling and exit codes for failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
237-237: Consider the flash when initial offset is 0 for owners.When
isOwneris true butinitialOffsetis 0 (new bookmark, never scrolled), the body doesn't getopacity: 0initially. This is correct behavior since no scroll restoration is needed, but the script at line 83 still setsopacity: 1. This is harmless but could be simplified by only setting opacity in the CSS when restoration is actually needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared-react/hooks/reading-progress.tspackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-webview.generated.tspackages/trpc/routers/bookmarks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared-react/hooks/reading-progress.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/trpc/routers/bookmarks.tspackages/shared/utils/reading-progress-webview.generated.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/trpc/routers/bookmarks.tspackages/shared/utils/reading-progress-webview.generated.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/trpc/routers/bookmarks.tspackages/shared/utils/reading-progress-webview.generated.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.ts
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/utils/reading-progress-webview.generated.tspackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.ts
apps/mobile/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Expo for developing the mobile application
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx
🧬 Code graph analysis (4)
packages/trpc/routers/bookmarks.ts (4)
packages/trpc/index.ts (1)
authedProcedure(100-121)packages/shared/utils/reading-progress-core.ts (1)
ANCHOR_TEXT_MAX_LENGTH(34-34)packages/trpc/models/assets.ts (1)
ensureBookmarkOwnership(200-206)packages/db/schema.ts (1)
bookmarkLinks(229-262)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (4)
packages/shared/utils/reading-progress-webview.generated.ts (1)
READING_PROGRESS_WEBVIEW_JS(18-19)packages/shared/types/bookmarks.ts (1)
ZBookmark(125-125)apps/mobile/lib/useColorScheme.tsx (1)
useColorScheme(46-46)packages/shared-react/hooks/users.ts (1)
useWhoAmI(35-37)
packages/shared/utils/reading-progress-core.ts (3)
packages/shared-react/hooks/reading-progress.ts (1)
ReadingPosition(62-62)packages/shared/utils/reading-progress-dom.ts (2)
ReadingPosition(12-12)scrollToReadingPosition(13-13)packages/shared/utils/reading-progress-webview-src.ts (1)
scrollToReadingPosition(18-18)
packages/shared/utils/reading-progress-webview-src.ts (2)
packages/shared/utils/reading-progress-dom.ts (2)
getReadingPosition(52-65)ReadingPosition(12-12)packages/shared/utils/reading-progress-core.ts (2)
ReadingPosition(11-14)getReadingPositionWithViewport(107-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (12)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (3)
49-102: Well-structured WebView script injection.The
buildReadingProgressScriptfunction cleanly encapsulates the reading progress logic for WebView injection. The throttled scroll listener (150ms) and periodic backup reporting (10s interval) provide good balance between responsiveness and performance.
175-190: Cleanup handles both unmount and background transitions correctly.The effect properly saves progress on app background/inactive state changes and on unmount, with correct subscription cleanup. Note that
saveProgressis called synchronously in the cleanup function, which works here since it's a fire-and-forget mutation call.
192-192: Race condition fix confirmed.The loading check now includes
isUserLoading, ensuring the WebView won't render beforecurrentUseris available. This addresses the previous review concern about ownership determination.packages/trpc/routers/bookmarks.ts (1)
587-610: Clean implementation of updateReadingProgress mutation.The mutation properly validates input (nonnegative integer offset, max-length anchor), enforces ownership via middleware, and now correctly throws
NOT_FOUNDwhen updating a non-link bookmark (addressing the previous review concern). Not triggering webhooks or search reindex is appropriate since this is metadata rather than content.packages/shared/utils/reading-progress-webview.generated.ts (1)
1-19: Auto-generated file with clear documentation.The file header clearly indicates it's auto-generated, provides the regeneration command, lists source files, and documents the exposed API surface. The pre-commit hook mentioned in the PR ensures this stays synchronized with the source.
packages/shared/utils/reading-progress-webview-src.ts (1)
1-29: Clean WebView-specific adapter module.The module correctly adapts the core utilities for WebView context by hardcoding
viewportTop=0for window-level scrolling. The re-export ofscrollToReadingPositionprovides a consistent API surface for the bundled WebView script.packages/shared/utils/reading-progress-dom.ts (2)
20-43: Robust scrollable parent detection with Radix support.The
findScrollableParentfunction handles standard overflow-based scrolling, Radix ScrollArea components (viadata-radix-scroll-area-viewportattribute), and falls back todocument.documentElementfor window-level scrolling. ThehasScrollContentcheck correctly prevents false positives from elements that have overflow styles but no actual scrollable content.
52-64: Correct viewport reference calculation for nested scrolling.The function properly distinguishes between window-level scrolling (viewportTop=0) and container scrolling (uses container's bounding rect top). This ensures accurate position tracking in both standard pages and components like Radix ScrollArea.
packages/shared/utils/reading-progress-core.ts (4)
11-34: Well-designed core interface and constants.The
ReadingPositioninterface withoffsetandanchorprovides a dual-strategy approach for reliable position tracking. The paragraph selectors cover semantic block elements appropriately, andANCHOR_TEXT_MAX_LENGTH = 50provides enough context for verification while staying compact.
67-97: Fuzzy anchor matching adds resilience to content changes.The two-pass matching strategy (exact match first, then fuzzy on first 20 characters) provides tolerance for minor content edits while maintaining accuracy. The
anchor.length >= 20guard prevents false matches on very short paragraphs.
107-159: Correct handling of empty paragraphs.The TreeWalker loop now properly returns
nullat line 158 whentopParagraphcontains no text nodes, addressing the previous review concern about returning incorrect offsets for empty paragraphs or paragraphs with only non-text elements.
165-230: Robust dual-strategy scroll restoration.The anchor-based strategy provides reliable restoration even after content reflows, while the offset-based fallback handles cases where anchors don't match. The parent traversal logic (lines 205-217) correctly finds the enclosing paragraph element for proper scroll alignment.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/trpc/routers/bookmarks.ts">
<violation number="1" location="packages/trpc/routers/bookmarks.ts:606">
P2: Inconsistent error code with existing patterns. Other similar checks in this file use `BAD_REQUEST` with a descriptive message when the bookmark exists but is the wrong type. Consider using `BAD_REQUEST` with a message like "Attempting to set reading progress for non-link type bookmark" to match the established pattern and provide clearer feedback to API consumers.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/trpc/routers/bookmarks.ts (1)
588-613: Consider validating bookmark type before saving reading progress.The mutation uses
ensureBookmarkAccesswhich validates the bookmark exists and user has access, but doesn't verify the bookmark is of typeLINK. While writing progress for non-LINK bookmarks won't cause errors (data goes touserReadingProgresstable), it creates orphaned data that will never be used since reading progress is only retrieved and displayed for LINK bookmarks.This is a minor concern since the client controls when this is called, but for API completeness you may want to add type validation.
🔎 Optional: Add bookmark type validation
updateReadingProgress: authedProcedure .input( z.object({ bookmarkId: z.string(), readingProgressOffset: z.number().int().nonnegative(), readingProgressAnchor: z.string().max(ANCHOR_TEXT_MAX_LENGTH).nullish(), }), ) .use(ensureBookmarkAccess) .mutation(async ({ input, ctx }) => { + // Reading progress only applies to link bookmarks + const bookmark = await ctx.db.query.bookmarks.findFirst({ + where: eq(bookmarks.id, input.bookmarkId), + columns: { type: true }, + }); + if (bookmark?.type !== BookmarkTypes.LINK) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Reading progress can only be set for link bookmarks", + }); + } + await ctx.db .insert(userReadingProgress)apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
37-101: Well-structured reading progress script with good documentation.The IIFE pattern, scroll throttling, and opacity handling for restoration are well-implemented.
One minor note: the
setIntervalon line 96 is never explicitly cleared. While this isn't a leak (the JS context is destroyed with the WebView), adding cleanup would be more explicit:Optional: Add cleanup for interval
+ var intervalId = setInterval(reportProgress, 10000); - setInterval(reportProgress, 10000); + + // Clean up on page unload (defensive) + window.addEventListener('pagehide', function() { + clearInterval(intervalId); + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxpackages/db/drizzle/0073_user_reading_progress_table.sqlpackages/db/drizzle/meta/0073_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/trpc/models/bookmarks.tspackages/trpc/routers/bookmarks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/models/bookmarks.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/db/drizzle/meta/_journal.jsonapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/db/schema.tspackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/db/schema.tspackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/db/schema.tspackages/trpc/routers/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
apps/mobile/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Expo for developing the mobile application
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for database schema and migrations in the db package
Files:
packages/db/schema.ts
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/preview/LinkContentSection.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-16T13:02:08.919Z
Learnt from: MohamedBassem
Repo: karakeep-app/karakeep PR: 2141
File: packages/db/schema.ts:160-165
Timestamp: 2025-11-16T13:02:08.919Z
Learning: In this codebase using Drizzle ORM with SQLite, enum definitions (e.g., `text("status", { enum: ["value1", "value2"] })`) only provide TypeScript-level type safety and do not require database migrations when values are added or modified. SQLite stores these as TEXT columns without database-level CHECK constraints, so schema changes to enums don't need corresponding migration files.
Applied to files:
packages/db/schema.ts
🧬 Code graph analysis (1)
apps/web/components/dashboard/preview/LinkContentSection.tsx (1)
packages/shared-react/hooks/reading-progress.ts (1)
useReadingProgressAutoSave(214-459)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (9)
apps/web/components/dashboard/preview/LinkContentSection.tsx (2)
132-143: LGTM! Reading progress auto-save integration is well-structured.The hook is correctly called unconditionally at the component level with the
enabledflag controlling behavior. Initial progress values are properly extracted from bookmark content, and thecontainerRefis appropriately passed for scroll tracking.
158-170: The scroll detection is already properly implemented for nested ScrollArea. ThefindScrollableParentfunction explicitly checks for Radix ScrollArea's viewport using thedata-radix-scroll-area-viewportattribute (not just overflow-based detection), verifies scrollable content exists before returning, and only falls back todocument.documentElementif no scrollable ancestor is found. The hook correctly uses this and adds event listeners to the identified scroll parent.Likely an incorrect or invalid review comment.
packages/db/drizzle/0073_user_reading_progress_table.sql (1)
1-14: LGTM! Well-designed migration with appropriate constraints and indexes.The table structure correctly models per-user reading progress with:
- Proper cascade deletes for data integrity
- Unique composite index enforcing one progress record per user-bookmark pair
- Individual indexes supporting efficient lookups by bookmark or user
packages/db/drizzle/meta/_journal.json (1)
515-522: LGTM! Migration journal entry correctly appended.The new entry follows the established pattern with sequential index and matching tag.
packages/db/schema.ts (2)
345-369: LGTM! Schema definition follows established patterns.The
userReadingProgresstable correctly:
- Uses
createId()for primary key generation- Defines appropriate foreign keys with cascade deletes
- Includes the unique constraint for per-user-per-bookmark progress
- Uses the existing
modifiedAtField()helper for automatic timestamp updates
1121-1134: LGTM! Relations properly defined.The relations correctly link reading progress records to both bookmarks and users, enabling efficient Drizzle ORM relational queries.
packages/trpc/routers/bookmarks.ts (1)
624-645: LGTM! Reading progress retrieval is correctly implemented.The implementation properly:
- Fetches progress only for the current authenticated user
- Only augments LINK-type bookmarks with progress data
- Defaults to
nullwhen no progress existsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (2)
171-184: LGTM!The AppState subscription correctly handles saving progress when the app goes to background. The effect cleanup is well-designed: when
saveProgresschanges (e.g., bookmark navigation), cleanup runs with the old closure, correctly saving progress for the previous bookmark before switching.
262-263: This concern has already been addressed in the codebase.The
htmlContentis sanitized server-side using DOMPurify before storage. Inapps/workers/workers/crawlerWorker.ts, the crawler extracts readable content and sanitizes it withDOMPurify.sanitize()(line 670), storing only the purified HTML. This sanitized content is then stored in the database and retrieved for display in the WebView, ensuring malicious scripts cannot execute within the WebView context.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx">
<violation number="1" location="apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx:138">
P2: Removing `isOwner` check contradicts the PR's stated design ("Ownership-Based Tracking") and creates inconsistency with the web version which still uses `enabled: isOwner`. If this is intentional (per-user progress for all users with access), the PR description should be updated and the web version should match. If not, the ownership check should be restored.</violation>
</file>
<file name="apps/web/components/dashboard/preview/LinkContentSection.tsx">
<violation number="1" location="apps/web/components/dashboard/preview/LinkContentSection.tsx:142">
P1: This removes the ownership check that prevents non-owners from modifying reading progress. The PR description explicitly states 'Reading progress only tracked for bookmark owners' and 'Progress is NOT saved when viewing someone else's bookmark in a shared list.' Restore the `isOwner` condition to prevent unauthorized modifications.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shared/utils/reading-progress-core.test.ts (1)
9-41: Comprehensive coverage for normalizeText.The test suite covers the core scenarios well, including edge cases (empty, whitespace-only) and mixed whitespace types. Test names are descriptive and assertions are clear.
Optional: Consider adding edge case tests for special characters
If the reading progress feature will handle international content or special characters, you might add tests for:
test("handles non-breaking spaces", () => { expect(normalizeText("hello\u00A0\u00A0world")).toBe("hello world"); }); test("handles unicode whitespace characters", () => { expect(normalizeText("hello\u2003world")).toBe("hello world"); // em space });However, the current coverage is adequate for the typical use case.
packages/trpc/routers/bookmarks.test.ts (1)
938-1060: Consider adding optional edge case tests.The current test coverage is solid for typical usage. To further strengthen the test suite, you could optionally add tests for:
- Input validation edge cases: negative offsets, very large offsets (e.g.,
Number.MAX_SAFE_INTEGER), extremely long anchor text (e.g., 1000+ characters), empty string anchor- Explicit optional anchor test: Currently line 1055 implicitly tests optional anchor by omitting it, but an explicit test with a comment documenting this behavior would be valuable
- TEXT bookmark response verification: Verify that
getBookmarkfor TEXT bookmarks doesn't includereadingProgressOffset/readingProgressAnchorfields (complementing the existing test that validates saving is rejected)These additions would provide more comprehensive coverage of boundary conditions and edge cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/reader/[bookmarkId]/page.tsxpackages/shared/utils/reading-progress-core.test.tspackages/trpc/routers/bookmarks.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/reader/[bookmarkId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/trpc/routers/bookmarks.test.tspackages/shared/utils/reading-progress-core.test.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/trpc/routers/bookmarks.test.tspackages/shared/utils/reading-progress-core.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/trpc/routers/bookmarks.test.tspackages/shared/utils/reading-progress-core.test.ts
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for writing and running tests
Files:
packages/trpc/routers/bookmarks.test.tspackages/shared/utils/reading-progress-core.test.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/utils/reading-progress-core.test.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/e2e_tests/**/*.{ts,tsx,test,spec} : Write end-to-end tests in the `packages/e2e_tests` directory
Applied to files:
packages/shared/utils/reading-progress-core.test.ts
🔇 Additional comments (4)
packages/shared/utils/reading-progress-core.test.ts (3)
1-7: LGTM! Clean test structure and imports.The test file follows Vitest conventions and coding guidelines. Imports are minimal and appropriate.
43-60: LGTM! Adequate coverage for normalizeTextLength.The test suite appropriately verifies the function returns the length of normalized text, including edge cases for empty and whitespace-only inputs.
62-66: LGTM! Appropriate constant verification.Testing that the constant equals 50 is a reasonable sanity check to catch unintended changes.
packages/trpc/routers/bookmarks.test.ts (1)
937-1238: Excellent test coverage for reading progress functionality!The test suite comprehensively validates the reading progress feature across all critical scenarios:
- Basic CRUD operations (save, retrieve, update)
- Per-user isolation for both independent bookmarks and shared bookmarks
- Validation rules (LINK-only restriction)
- Access control and authorization
- Collaboration workflows (owner and collaborator independence)
- Data integrity (cascading deletion)
The tests are well-structured, follow existing patterns in the file, and align perfectly with the PR objectives. The use of type guards with assertions (
assert(retrieved.content.type === BookmarkTypes.LINK)) is particularly good for maintaining type safety in the test assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared-react/hooks/reading-progress.ts (1)
115-134: Consider more robust retry for dynamically visible containers.The single 300ms retry (lines 132-134) may miss containers that become visible later (e.g., tab panels or modals opened after load). While the effect re-runs when
containerRefchanges, the same ref becoming visible after 300ms won't re-attach listeners.💡 Alternative approach
Use a short polling interval or
MutationObserver/IntersectionObserverto detect when a hidden container becomes visible, then attach listeners. However, given that most UI frameworks update refs when showing/hiding elements, the current approach may suffice for typical use cases.apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
126-134: Consider the trade-off of query invalidation after progress save.The
onSuccesshandler invalidates and refetches the bookmark query, but since the WebView's HTML content doesn't change, the WebView won't reload. This refetch keeps the cache consistent (beneficial for multiple views or components), but it does introduce unnecessary network overhead. The current approach prioritizes consistency, which is reasonable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxpackages/shared-react/hooks/reading-progress.tspackages/shared/utils/reading-progress-dom.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/components/dashboard/preview/LinkContentSection.tsx
- apps/web/app/reader/[bookmarkId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/utils/reading-progress-dom.tspackages/shared-react/hooks/reading-progress.ts
apps/mobile/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Expo for developing the mobile application
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/utils/reading-progress-dom.ts
packages/shared-react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place shared React components and hooks in
packages/shared-react
Files:
packages/shared-react/hooks/reading-progress.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/shared-react/**/*.{ts,tsx} : Place shared React components and hooks in `packages/shared-react`
Applied to files:
packages/shared-react/hooks/reading-progress.ts
🧬 Code graph analysis (3)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (2)
packages/shared/utils/reading-progress-webview.generated.ts (1)
READING_PROGRESS_WEBVIEW_JS(18-19)packages/shared/types/bookmarks.ts (1)
ZBookmark(125-125)
packages/shared/utils/reading-progress-dom.ts (2)
packages/shared/utils/reading-progress-webview-src.ts (1)
getReadingPosition(24-29)packages/shared/utils/reading-progress-core.ts (2)
ReadingPosition(11-14)getReadingPositionWithViewport(107-159)
packages/shared-react/hooks/reading-progress.ts (2)
packages/shared/utils/reading-progress-dom.ts (5)
ReadingPosition(12-12)getReadingPosition(64-77)isElementVisible(21-25)findScrollableParent(32-55)scrollToReadingPosition(13-13)packages/shared/utils/reading-progress-core.ts (2)
ReadingPosition(11-14)scrollToReadingPosition(165-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (6)
packages/shared/utils/reading-progress-dom.ts (1)
1-77: LGTM! Clean DOM utility module.The implementation correctly delegates to core utilities while handling web-specific concerns (Radix ScrollArea detection, nested scrolling). Previous edge-case issues with empty paragraphs have been addressed in the core module.
packages/shared-react/hooks/reading-progress.ts (1)
46-199: Well-structured hook with proper effect separation.The three-effect organization (scroll tracking, auto-save, restoration) is clean and maintainable. Throttling (150ms) addresses previous performance concerns. The ref-based callback pattern (lines 83-86) correctly avoids effect dependency issues for the save function.
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (4)
48-101: Well-structured reading progress injection script.The function effectively combines the shared reading-progress core with platform-specific WebView messaging. The throttled scroll listener (150ms) and periodic backup (10s) provide a good balance between responsiveness and performance.
168-181: AppState lifecycle handling is correctly implemented.The effect properly saves progress when the app backgrounds and on unmount. The
saveProgressdependency may cause the effect to re-run, but sincebookmark.idandupdateProgressare stable, this should be rare in practice.
199-202: Reading progress fields accessed safely with nullish coalescing.The code accesses
readingProgressOffsetandreadingProgressAnchoron the content object returned by the query withincludeContent: true. The nullish coalescing provides safe defaults. If TypeScript doesn't error here, the API query return type correctly includes these fields (likely merged from the userReadingProgress table as mentioned in the PR summary).
207-276: WebView integration is well-implemented with good UX considerations.The conditional opacity (line 226) prevents content flash during scroll restoration, and the injected script (lines 272-273) properly integrates reading progress tracking with WebView messaging. The restoration flow is smooth and consistent with the web implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shared/utils/reading-progress-core.ts (2)
16-28: Consider exportingPARAGRAPH_SELECTORSfor extensibility.While these constants are currently used only internally, exporting
PARAGRAPH_SELECTORS(and optionallyPARAGRAPH_SELECTOR_STRING) would allow consuming code to reference the same selector set when extending functionality or writing tests that need to verify paragraph detection logic.Proposed change
-const PARAGRAPH_SELECTORS = [ +export const PARAGRAPH_SELECTORS = [ "p", "h1", "h2", "h3", "h4", "h5", "h6", "li", "blockquote", ]; -const PARAGRAPH_SELECTOR_STRING = PARAGRAPH_SELECTORS.join(", "); +export const PARAGRAPH_SELECTOR_STRING = PARAGRAPH_SELECTORS.join(", ");
149-162: Fuzzy matching is restricted to anchors ≥ 20 characters.The current fuzzy-match logic only activates when
anchor.length >= 20. For shorter anchors (10–19 chars), even small content changes will cause restoration to fall back to pure offset-based scrolling, which is less accurate. Consider lowering the threshold (e.g., 10 characters) or using a proportional similarity check to handle moderately short anchors more gracefully.Example: flexible threshold
- // Fuzzy fallback: check if first 20 chars match + // Fuzzy fallback: check if first 20 chars match (for anchors >= 10 chars) for (const paragraph of paragraphs) { const paragraphAnchor = normalizeText(paragraph.textContent ?? "").slice( 0, ANCHOR_TEXT_MAX_LENGTH, ); + const compareLength = Math.min(20, anchor.length); if ( - paragraphAnchor.slice(0, 20) === anchor.slice(0, 20) && - anchor.length >= 20 + anchor.length >= 10 && + paragraphAnchor.slice(0, compareLength) === anchor.slice(0, compareLength) ) { paragraph.scrollIntoView({ behavior, block: "start" }); return true; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-webview.generated.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/utils/reading-progress-webview.generated.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/shared/utils/reading-progress-core.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/shared/utils/reading-progress-core.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/shared/utils/reading-progress-core.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/utils/reading-progress-core.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
packages/shared/utils/reading-progress-core.ts (3)
40-52: LGTM!The text normalization logic correctly handles whitespace collapsing for consistent offset calculations across different HTML formatting. The two-step regex approach (convert special chars → collapse spaces → trim) is clear and correct.
62-117: LGTM!The viewport-based position detection logic is well-structured:
- Correctly identifies the paragraph at the viewport top using two complementary conditions
- TreeWalker-based offset calculation properly accumulates text length up to the target paragraph
- Handles edge cases (no paragraphs, empty paragraphs) with appropriate null returns
165-210: LGTM!The offset-based fallback traversal correctly mirrors the position calculation logic:
- TreeWalker iterates text nodes in document order
- Normalized length ensures consistent offset calculations
- Parent-chain walk finds the appropriate paragraph element to scroll
- Graceful fallback to parent element if no paragraph is found
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/components/dashboard/preview/LinkContentSection.tsx (1)
154-154: Restore ownership check to prevent unauthorized tracking.Line 154 enables reading progress tracking for all users viewing the cached section, not just bookmark owners. This contradicts the PR's stated design ("Reading progress only tracked for bookmark owners") and will cause non-owners to track scroll position and attempt unauthorized saves, generating backend errors and wasting client resources.
🔎 Proposed fix
- enabled: section === "cached", // Only track in cached/reader view + enabled: isOwner && section === "cached", // Only track in cached/reader view
🧹 Nitpick comments (1)
packages/shared-react/hooks/reading-progress.ts (1)
236-289: Remove unusedbookmarkIdfrom restoration effect dependencies.The restoration effect (lines 236-289) includes
bookmarkIdin its dependency array (line 286) but never references it within the effect body. This triggers unnecessary re-runs whenbookmarkIdchanges, though in practice this is unlikely to occur during a component's lifecycle.🔎 Proposed fix
}, [ enabled, initialOffset, contentReady, - bookmarkId, initialAnchor, containerRef, ]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared-react/hooks/reading-progress.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxpackages/shared-react/hooks/reading-progress.tsapps/web/components/dashboard/preview/ReaderView.tsx
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
apps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxpackages/shared-react/hooks/reading-progress.tsapps/web/components/dashboard/preview/ReaderView.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxpackages/shared-react/hooks/reading-progress.tsapps/web/components/dashboard/preview/ReaderView.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/LinkContentSection.tsxapps/web/components/dashboard/preview/ReaderView.tsx
packages/shared-react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place shared React components and hooks in
packages/shared-react
Files:
packages/shared-react/hooks/reading-progress.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/shared-react/**/*.{ts,tsx} : Place shared React components and hooks in `packages/shared-react`
Applied to files:
packages/shared-react/hooks/reading-progress.ts
🧬 Code graph analysis (3)
apps/web/app/reader/[bookmarkId]/page.tsx (2)
packages/shared-react/hooks/reading-progress.ts (1)
useReadingProgress(62-292)packages/shared/types/readers.ts (1)
READER_FONT_FAMILIES(11-15)
apps/web/components/dashboard/preview/LinkContentSection.tsx (1)
packages/shared-react/hooks/reading-progress.ts (1)
useReadingProgress(62-292)
packages/shared-react/hooks/reading-progress.ts (3)
packages/shared/utils/reading-progress-core.ts (1)
ReadingPosition(11-14)packages/shared/utils/reading-progress-dom.ts (4)
ReadingPosition(12-12)getReadingPosition(64-77)isElementVisible(21-25)findScrollableParent(32-55)packages/shared/utils/reading-progress-webview-src.ts (1)
getReadingPosition(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/shared-react/hooks/reading-progress.ts">
<violation number="1" location="packages/shared-react/hooks/reading-progress.ts:92">
P2: Missing `bookmarkId` in sync effect dependencies may cause `isReady` to remain `true` when switching between bookmarks that both need restoration. Since `needsRestoration` stays `true` when both bookmarks have `initialOffset`, the sync effect won't reset `isReady` to `false`, potentially causing a flash of unscrolled content.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…p into evan/reading-progress
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/components/dashboard/preview/LinkContentSection.tsx (1)
165-165: Ownership check still missing.This is the same issue flagged in the previous review. The
enabledprop should include theisOwnercheck to prevent non-owners from saving reading progress when viewing shared bookmarks, as stated in the PR objectives.🔎 Restore the ownership check
- enabled: section === "cached", // Only track in cached/reader view + enabled: isOwner && section === "cached", // Only track in cached/reader view
🧹 Nitpick comments (1)
packages/trpc/models/bookmarks.ts (1)
475-477: Intentional omission for batch loading performance.Excluding reading progress from
loadMultiavoids N+1 queries when fetching multiple bookmarks. Per-user reading progress is fetched only in the singulargetBookmarkendpoint. Consider adding a brief comment explaining this design choice for future maintainers.Optional: Enhanced documentation
// Reading progress is fetched separately per-user in getBookmark +// Note: Intentionally excluded from batch queries (loadMulti) for performance readingProgressOffset: null, readingProgressAnchor: null,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/web/components/dashboard/preview/LinkContentSection.tsxpackages/db/drizzle/0074_user_reading_progress_table.sqlpackages/db/drizzle/meta/0074_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/open-api/karakeep-openapi-spec.jsonpackages/shared/types/bookmarks.tspackages/trpc/models/bookmarks.tspackages/trpc/routers/bookmarks.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/shared/types/bookmarks.ts
- packages/db/schema.ts
- packages/db/drizzle/meta/_journal.json
- packages/open-api/karakeep-openapi-spec.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tsapps/web/components/dashboard/preview/LinkContentSection.tsx
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/preview/LinkContentSection.tsx
🧬 Code graph analysis (1)
packages/trpc/routers/bookmarks.ts (3)
packages/trpc/index.ts (1)
authedProcedure(100-121)packages/shared/utils/reading-progress-core.ts (1)
ANCHOR_TEXT_MAX_LENGTH(34-34)packages/db/schema.ts (1)
userReadingProgress(359-383)
🔇 Additional comments (7)
packages/db/drizzle/0074_user_reading_progress_table.sql (1)
1-14: LGTM! Well-designed schema for per-user reading progress.The migration correctly establishes:
- CASCADE deletion for both foreign keys (bookmark and user cleanup)
- Unique constraint on (bookmarkId, userId) preventing duplicate entries
- Appropriate indexes for query performance
packages/trpc/models/bookmarks.ts (1)
189-191: Correct pattern for per-user reading progress.Initializing to null here is appropriate since
toZodSchemadoesn't have per-user context. The reading progress is populated separately in thegetBookmarkendpoint where user context is available.packages/trpc/routers/bookmarks.ts (3)
17-17: LGTM! Appropriate imports for reading progress feature.Also applies to: 47-47
590-626: Excellent implementation of reading progress persistence.The mutation correctly:
- Validates input with appropriate constraints (nonnegative offset, max anchor length)
- Uses
ensureBookmarkAccessto allow collaborators to save their own progress- Validates LINK bookmark type before persisting (addresses past review feedback)
- Uses upsert pattern to handle both new and updated progress
- Avoids updating bookmark's
modifiedAt(correct, as reading progress is user-specific state)
637-658: Per-user reading progress correctly integrated.The implementation properly:
- Fetches the bookmark first to ensure access control
- Conditionally queries reading progress only for LINK types
- Uses indexed columns (bookmarkId, userId) for efficient lookup
- Populates user-specific progress on the content object
The separate query is acceptable for single bookmark fetches and maintains clean separation between bookmark data and per-user state.
apps/web/components/dashboard/preview/LinkContentSection.tsx (2)
190-191: This concern is already addressed in the implementation.The
useReadingProgresshook already has proper fallback logic (lines 239-242) to prevent content from being hidden indefinitely. It usesrequestAnimationFrameto attempt restoration, and explicitly callssetIsReady(true)regardless of success with a comment stating "Mark ready regardless of success to avoid permanent hidden state." Additionally, the cleanup function (lines 247-248) ensuresisReadyis set to true on unmount to prevent a stuck hidden state.
155-157: PropertiesreadingProgressOffsetandreadingProgressAnchorare properly typed as nullable.Both properties are defined on the
ZBookmarkedLinktype asz.number().int().nonnegative().nullish()andz.string().nullish()respectively, and theuseReadingProgresshook correctly accepts them as optional nullable parameters (number | nullandstring | null). The hook safely handles undefined values via truthiness checks when determining whether restoration is needed.
|
Moving back to draft mode, I found some ways to improve the performance and reliability of this feature |
|
I ended up deciding not to over-optimize things too early, I have some follow-up PRs I want to add (like live webpage progress tracking/restoring) but I'll address any changes to the reading progress tracking I need to make then. I did add percentage tracking, though, in advance of adding rules engine support. |
Greptile SummaryImplements synchronized reading progress tracking across web and mobile platforms using character offsets with anchor text verification. The implementation uses a three-layer architecture: platform-agnostic core utilities, platform-specific modules (DOM for web with Radix ScrollArea support, WebView for mobile), and React/React Native integration layers. Reading progress is stored in a separate Key Implementation Details:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Component as React Component<br/>(ReaderView/LinkContentSection)
participant Hook as useReadingProgress Hook
participant DOM as reading-progress-dom.ts
participant Core as reading-progress-core.ts
participant TRPC as TRPC Client
participant API as bookmarks.updateReadingProgress
participant DB as userReadingProgress Table
Note over User,DB: Initial Load & Position Restoration
User->>Component: Opens bookmark in reader view
Component->>TRPC: getBookmark(bookmarkId, includeContent: true)
TRPC->>DB: Query bookmark + reading progress
DB-->>TRPC: bookmark with readingProgressOffset/Anchor
TRPC-->>Component: bookmark data
Component->>Hook: useReadingProgress(initialOffset, initialAnchor)
Hook->>Hook: Wait for contentReady signal
Component->>Component: Content loaded
Component->>Hook: setContentReady(true)
Hook->>DOM: findScrollableParent(container)
DOM-->>Hook: scrollParent element
Hook->>Core: scrollToReadingPosition(offset, anchor)
Core->>Core: Try anchor text match first
Core->>Core: Fallback to offset-based lookup
Core->>DOM: element.scrollIntoView()
Hook->>Component: setIsReady(true)
Component->>Component: Show content (visibility: visible)
Note over User,DB: Scrolling & Progress Tracking
User->>Component: Scrolls through article
Component->>Hook: scroll event (throttled 150ms)
Hook->>DOM: getReadingPosition(container)
DOM->>Core: getReadingPositionWithViewport()
Core->>Core: Find paragraph at viewport top
Core->>Core: Calculate offset via TreeWalker
Core->>Core: Extract anchor text (50 chars)
Core->>Core: Calculate percentage
Core-->>Hook: {offset, anchor, percent}
Hook->>Hook: Update lastKnownPositionRef
Note over User,DB: Auto-Save on Visibility Change
User->>Component: Navigates away / closes tab
Component->>Hook: visibilitychange / beforeunload event
Hook->>DOM: getReadingPosition(container)
DOM-->>Hook: Current position
Hook->>Hook: Check if offset changed
Hook->>TRPC: updateReadingProgress(offset, anchor, percent)
TRPC->>API: mutation(bookmarkId, progress data)
API->>API: Validate bookmark is LINK type
API->>DB: INSERT ... ON CONFLICT DO UPDATE
DB-->>API: Success
API-->>TRPC: Success
TRPC->>TRPC: Invalidate bookmark cache
|
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
178-191: Update handler to capturepercentif added to the message.If you fix the script to include
percent, update this handler accordingly:🔎 Proposed update
const handleMessage = useCallback((event: WebViewMessageEvent) => { try { const data = JSON.parse(event.nativeEvent.data); if (data.type === "SCROLL_PROGRESS" && typeof data.offset === "number") { currentPosition.current = { offset: data.offset, anchor: typeof data.anchor === "string" ? data.anchor : "", + percent: typeof data.percent === "number" ? data.percent : null, }; } } catch (error) { console.warn("[ReadingProgress] Failed to parse WebView message:", error); } }, []);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/web/app/reader/[bookmarkId]/page.tsxapps/web/components/dashboard/preview/ReaderView.tsxpackages/db/drizzle/0076_user_reading_progress.sqlpackages/db/drizzle/meta/0076_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/open-api/karakeep-openapi-spec.jsonpackages/shared-react/hooks/reading-progress.tspackages/shared/types/bookmarks.tspackages/shared/utils/reading-progress-core.tspackages/shared/utils/reading-progress-dom.tspackages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-webview.generated.tspackages/trpc/models/bookmarks.tspackages/trpc/routers/bookmarks.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/shared/utils/reading-progress-webview.generated.ts
- packages/open-api/karakeep-openapi-spec.json
- packages/db/drizzle/meta/_journal.json
- apps/web/app/reader/[bookmarkId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/db/schema.tsapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tspackages/shared/utils/reading-progress-core.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/types/bookmarks.tspackages/shared-react/hooks/reading-progress.tspackages/shared/utils/reading-progress-dom.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/db/schema.tsapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tspackages/shared/utils/reading-progress-core.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/types/bookmarks.tspackages/shared-react/hooks/reading-progress.tspackages/shared/utils/reading-progress-dom.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/db/schema.tsapps/web/components/dashboard/preview/ReaderView.tsxpackages/shared/utils/reading-progress-webview-src.tspackages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.tspackages/shared/utils/reading-progress-core.tsapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxpackages/shared/types/bookmarks.tspackages/shared-react/hooks/reading-progress.tspackages/shared/utils/reading-progress-dom.ts
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for database schema and migrations in the db package
Files:
packages/db/schema.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/preview/ReaderView.tsx
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/utils/reading-progress-webview-src.tspackages/shared/utils/reading-progress-core.tspackages/shared/types/bookmarks.tspackages/shared/utils/reading-progress-dom.ts
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/bookmarks.tspackages/trpc/models/bookmarks.ts
apps/mobile/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Expo for developing the mobile application
Files:
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx
packages/shared-react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place shared React components and hooks in
packages/shared-react
Files:
packages/shared-react/hooks/reading-progress.ts
🧠 Learnings (1)
📚 Learning: 2025-11-16T13:02:08.919Z
Learnt from: MohamedBassem
Repo: karakeep-app/karakeep PR: 2141
File: packages/db/schema.ts:160-165
Timestamp: 2025-11-16T13:02:08.919Z
Learning: In this codebase using Drizzle ORM with SQLite, enum definitions (e.g., `text("status", { enum: ["value1", "value2"] })`) only provide TypeScript-level type safety and do not require database migrations when values are added or modified. SQLite stores these as TEXT columns without database-level CHECK constraints, so schema changes to enums don't need corresponding migration files.
Applied to files:
packages/db/schema.ts
🧬 Code graph analysis (3)
apps/web/components/dashboard/preview/ReaderView.tsx (2)
packages/db/schema.ts (1)
highlights(327-357)packages/shared-react/hooks/highlights.ts (3)
useCreateHighlight(3-17)useUpdateHighlight(19-33)useDeleteHighlight(35-49)
packages/trpc/routers/bookmarks.ts (2)
packages/shared/utils/reading-progress-core.ts (1)
ANCHOR_TEXT_MAX_LENGTH(35-35)packages/db/schema.ts (1)
userReadingProgress(359-384)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
packages/shared/utils/reading-progress-webview.generated.ts (1)
READING_PROGRESS_WEBVIEW_JS(14-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (9)
packages/shared/types/bookmarks.ts (1)
61-63: LGTM! Well-defined schema for reading progress fields.The field constraints are appropriate:
readingProgressOffsetas nonnegative integer for character positionreadingProgressPercentbounded to 0-100- All fields are nullish for backward compatibility
Note: The anchor string length is validated server-side via
ANCHOR_TEXT_MAX_LENGTHin the mutation input, which is acceptable since this schema is primarily for output/response validation.packages/db/schema.ts (2)
359-384: LGTM! Well-designed per-user reading progress table.The schema correctly implements:
- Composite unique constraint on
(bookmarkId, userId)enabling efficient upserts- Cascade deletes on both FKs for automatic cleanup
- Appropriate indexes for lookup performance
readingProgressOffsetas NOT NULL (required when saving), while anchor and percent are optional
1137-1149: Relations correctly defined.The bidirectional relations to
bookmarksandusersare properly established.packages/shared/utils/reading-progress-webview-src.ts (1)
1-37: LGTM! Clean WebView-specific wrapper.The implementation correctly:
- Delegates to the shared core logic for consistency between platforms
- Assumes window-level scrolling (appropriate for WebView)
- Documents the differences from the DOM version clearly
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (2)
193-206: LGTM! Proper lifecycle handling for progress persistence.The implementation correctly:
- Saves progress when app goes to background/inactive
- Saves progress on component unmount
- Cleans up the AppState subscription
248-251: Good UX pattern for scroll restoration.Setting
opacity: 0initially and revealing after scroll restoration prevents the visual jump. This is a nice touch for user experience.packages/db/drizzle/0076_user_reading_progress.sql (1)
1-15: LGTM! Migration correctly reflects the schema.The SQL migration properly creates:
- Table with all required columns
- Foreign key constraints with cascade deletes
- Individual indexes on
bookmarkIdanduserIdfor efficient lookups- Unique composite index on
(bookmarkId, userId)for upsert operationspackages/trpc/routers/bookmarks.ts (2)
583-622: LGTM! Well-implemented reading progress mutation.The implementation:
- Uses
ensureBookmarkAccessappropriately to allow any user with access to track their own progress- Validates the bookmark is a LINK type with a clear error message
- Uses the correct upsert pattern targeting the unique
(bookmarkId, userId)constraint- Properly handles nullable fields for anchor and percent
633-656: LGTM! Per-user progress retrieval in getBookmark.The implementation correctly fetches the current user's reading progress and populates all three fields (
readingProgressOffset,readingProgressAnchor,readingProgressPercent) from theuserReadingProgresstable.
1 participant
Summary
Track and restore reading position in reader mode across web and mobile. Uses text character offsets with anchor text verification to find the first visible paragraph and scroll back to it on return.
Closes #1423
Note on unsupported views
This PR only covers the reader view for now.
Archived views
Adding support for archived views would require enabling JavaScript execution in the archive view iframes. While SingleFile, the new Karakeep extension, and the parsers all default to sanitizing JavaScript when saving archived HTML, there is still a chance someone archives something with malicious JS code in it. I didn't want to be the one to make this call, however I'm eager to figure out how we can add support for reading progress to the archived views while minimizing the risk to users.
Live webpages - mobile
Adding support for live webpages in the mobile app should be very easy. Like with the reader view on mobile, this is encapsulated in a mobile webview so we can just inject a contentScript and the logic should be pretty much the same.
Live webpages - web
For live webpage on the web, I think this would be a great feature for the Karakeep browser extension! It can host the same contentScript, with a lookup to see if a page is in your bookmarks. If so, it can autoscroll to your last known position!
Architectural Decisions
Three-Layer Code Organization: Core utilities (
reading-progress-core.ts) contain platform-agnostic logic shared between web and mobile. Platform-specific modules (reading-progress-dom.ts,reading-progress-webview-src.ts) extend the core with environment-specific implementations. Adds a generator script for transpiling the core logic into stringified JavaScript to inject into the Mobile WebView (and into iframes in the future).Dual-Strategy Position Restoration: Uses anchor text (~50 chars of paragraph content) as primary lookup, with character offset as fallback. This makes restoration resilient to minor content changes while still working when anchor text isn't available.
Text Normalization: Collapses whitespace to single spaces before calculating offsets, ensuring consistent character counting regardless of HTML formatting variations.
Content-Ready Signal with RAF Fallback: Parent components signal when content is ready for restoration via the
contentReadyprop. The hook attempts restoration immediately when signaled, then uses a singlerequestAnimationFrameas a fallback if the layout hasn't been calculated yet. This avoids polling while ensuring restoration works even when the scroll parent needs one more paint cycle to have scrollable height.Persist on Unload: Only persists changes to the DB when the view is unloaded or blurred (or when the app closes).
Separate Reading Progress Table: Reading progress is stored in a dedicated
userReadingProgresstable rather than as columns on the bookmarks table. This allows each user to track their own reading progress independently when viewing shared bookmarks.Platform-Specific Scrolling: Web version handles nested scrolling (including Radix ScrollArea detection), while mobile assumes window-level scrolling only.
Testing Steps
Web - Full-Screen Reader
Web - Preview Panel
Mobile