Uh oh!
There was an error while loading. Please reload this page.
fix: pictique lazy scroll - #648
Conversation
📝 WalkthroughWalkthroughAdds paginated infinite-scroll to the Pictique feed: new pagination stores ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Home as Home Page
participant Store as Posts Store
participant API as Backend API
User->>Home: Open feed
Home->>Store: resetFeed()
Store-->>Store: clear posts, currentPage=0, hasMore=true
Home->>Store: fetchFeed(1,10,false)
activate Store
Store->>API: GET /posts?page=1&limit=10
API-->>Store: { posts, meta }
Store-->>Store: set posts, currentPage=1, hasMore=(meta.hasMore)
deactivate Store
Note over User,Home: User scrolls / viewport reaches sentinel
Home->>Home: IntersectionObserver fires (if hasMore & !isLoading/isLoadingMore)
Home->>Store: loadMoreFeed()
activate Store
Store-->>Store: isLoadingMore=true
Store->>API: GET /posts?page=2&limit=10
API-->>Store: { posts, meta }
Store-->>Store: append posts, currentPage=2, isLoadingMore=false, update hasMore
deactivate Store
Home->>User: Render appended posts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
platforms/pictique/src/lib/stores/posts.ts (1)
77-80: Simplify redundant array mapping.
images.map((img) => img)is an identity mapping that just creates a shallow copy. If a copy is needed,[...images]is clearer; otherwise, just useimagesdirectly.🔎 Suggested simplification
const response = await apiClient.post("/api/posts", { text, - images: images.map((img) => img),+ images, });platforms/pictique/src/routes/(protected)/home/+page.svelte (1)
62-89: Consider removing redundant scroll handler.Both the
IntersectionObserver(sentinel) and thisonScrollhandler triggerloadMoreFeed()under the same conditions. The observer is more efficient and already handles the use case. The scroll handler adds redundancy but also extra event overhead.If kept as a fallback for browser compatibility, this is fine. Otherwise, consider removing it to simplify the code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/pictique/src/lib/stores/posts.tsplatforms/pictique/src/routes/(protected)/home/+page.svelte
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (10)
platforms/pictique/src/lib/stores/posts.ts (5)
1-11: LGTM!The new stores (
isLoadingMore,currentPage,hasMore) are well-structured for managing pagination state alongside the existing stores.
16-20: LGTM!Clean reset function that properly initializes all pagination-related state.
22-59: LGTM!The pagination logic is well-implemented:
- Separate loading states for initial vs. append loads
- Robust response parsing for different API response structures
hasMorecalculation correctly handles edge cases (last page, partial results)- Error handling prevents infinite retry loops by setting
hasMoreto false
61-71: LGTM!The guard conditions properly prevent duplicate requests and the use of
get()is appropriate for reading store values in this async context.
92-101: LGTM!Clean error handling that properly propagates failures to the caller.
platforms/pictique/src/routes/(protected)/home/+page.svelte (5)
7-34: LGTM!Imports and state declarations are well-organized for the infinite scroll implementation.
35-60: LGTM!Well-implemented
IntersectionObserverpattern with proper cleanup. UsingrootMargin: '200px'for early triggering ensures smooth loading before the user reaches the bottom.
143-156: LGTM!Proper lifecycle management with passive scroll listener and thorough cleanup of both the listener and the debounce timeout.
158-178: LGTM!Good UX consideration to auto-load more content if the initial batch doesn't fill the viewport. The
setTimeoutapproach for waiting on DOM measurements is a common pattern.
183-234: LGTM!Well-structured conditional rendering:
- Initial loading only shows when no posts exist (prevents flicker on pagination)
- Clear feedback for "loading more" and "no more posts" states
- Sentinel element properly gated by
hasMore
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @platforms/pictique/src/routes/(protected)/home/+page.svelte:
- Around line 143-156: The $effect block that attaches and cleans up the scroll
listener (referencing listElement, onScroll, scrollTimeout, and $effect) should
be deleted if you remove the redundant onScroll handler; remove this entire
reactive effect to avoid attaching a now-nonexistent listener and cleaning up
unused scrollTimeout state.
- Around line 158-178: The current onMount block uses a 100ms setTimeout to
check listElement.scrollHeight and possibly call loadMoreFeed, which can run
before images load; remove that timeout-check block (the entire setTimeout and
its nested scrollHeight/clientHeight/hasMore/isLoading/isLoadingMore logic) and
rely on the existing IntersectionObserver sentinel to trigger loadMoreFeed when
needed; ensure resetFeed(), fetchFeed(1, 10, false) and fetchProfile() still run
on mount and keep loadMoreFeed, listElement, hasMore, isLoading, and
isLoadingMore intact for the sentinel to consult.
- Around line 200-246: The rollback uses a stale postIndex captured earlier
which can be invalid if the posts array changes; instead capture the post.id
(e.g., const targetPostId = post.id) when performing the optimistic update and
in the catch handler locate the post by id from the posts store (findIndex(p =>
p.id === targetPostId)) before reverting likedBy to originalLikedBy; update
references to postIndex in the like handler and rollback code accordingly and
keep toggleLike, posts.update, originalLikedBy and profile logic the same.
- Around line 35-60: The IntersectionObserver in sentinel currently sets root:
listElement which can be undefined and conflicts with the existing onScroll
logic; remove the root option from the observer options so it uses the viewport,
delete the onScroll function and its associated reactive effect that triggers
loadMoreFeed (keeping only any cleanup for scrollTimeout), and ensure sentinel
only calls loadMoreFeed() when entry.isIntersecting and the existing guards
(get(hasMore), get(isLoading), get(isLoadingMore)) pass.
♻️ Duplicate comments (1)
platforms/pictique/src/routes/(protected)/home/+page.svelte (1)
62-89: Redundant with IntersectionObserver sentinel.This scroll handler duplicates the loading logic already implemented by the IntersectionObserver sentinel function. Both mechanisms use a 200px threshold and can trigger
loadMoreFeed()simultaneously, potentially causing race conditions or duplicate requests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/pictique/src/routes/(protected)/home/+page.svelte
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (5)
platforms/pictique/src/routes/(protected)/home/+page.svelte (5)
7-24: LGTM!The new imports for pagination stores and
getutility are appropriate for implementing infinite scroll functionality.
27-33: LGTM!The new state variables for the sentinel element and scroll timeout are correctly typed and initialized.
183-183: LGTM!The refined loading condition correctly shows the initial loading message only when no posts exist yet, avoiding confusion with the "Loading more posts..." state.
261-269: LGTM!The conditional rendering for loading states is well-structured with mutually exclusive conditions that prevent conflicting UI states.
289-289: Minor: Class reordering has no functional impact.The reordering of Tailwind class tokens doesn't affect the rendered output.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
platforms/pictique/src/routes/(protected)/home/+page.svelte (3)
7-24: Unused import:currentPage
currentPageis imported but not referenced anywhere in this component. If it's only used internally byloadMoreFeed, consider removing it from the import list.🔎 Proposed fix
import { - currentPage, error, fetchFeed, hasMore, isLoading, isLoadingMore, loadMoreFeed, posts, resetFeed, toggleLike } from '$lib/stores/posts';
27-27: Unused binding:sentinelElementThe
sentinelElementvariable is bound on line 219 but never read. Since theuse:sentinelaction receives the node directly, this binding is unnecessary.🔎 Proposed fix
Remove the state declaration:
-let sentinelElement: HTMLElement | undefined = $state();And remove the binding on line 219:
-<li class="h-1 w-full" bind:this={sentinelElement} use:sentinel></li>+<li class="h-1 w-full" use:sentinel></li>
26-26: Unused binding:listElementWith the scroll handler removed (as suggested in past reviews),
listElementis now only bound but never read. Consider removing it.🔎 Proposed fix
Remove the declaration:
-let listElement: HTMLElement;And remove the binding on line 122:
-<ul bind:this={listElement} class="hide-scrollbar h-[100vh] overflow-auto">+<ul class="hide-scrollbar h-[100vh] overflow-auto">
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/pictique/src/routes/(protected)/home/+page.svelte
⏰ 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: build
🔇 Additional comments (6)
platforms/pictique/src/routes/(protected)/home/+page.svelte (6)
34-59: LGTM!The IntersectionObserver implementation is clean:
- Uses viewport as root (past issue addressed)
- Proper guard conditions before triggering load
- Correct cleanup via
destroy()
114-118: LGTM!The onMount logic is clean and addresses the PR objective of loading a small initial batch (~10 posts). The sentinel will handle additional loading if the viewport isn't filled.
123-123: LGTM!Correctly shows the initial loading state only when there are no posts yet, avoiding flicker during pagination.
140-197: Optimistic update implementation looks good.The critical issue from the past review (index-based rollback) has been properly addressed:
- Uses
targetPostIdfor reliable lookup in both update and rollback- Correctly captures
originalLikedBybefore mutation- Finds post by ID in both the optimistic update and error rollback paths
212-220: LGTM!The loading states and sentinel rendering are well-coordinated:
- Sentinel is hidden during loading, preventing duplicate triggers
- Clear feedback for "no more posts" state
- Conditions are mutually exclusive to avoid conflicting UI states
240-240: Minor class reordering - no functional change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
platforms/pictique/src/routes/(protected)/home/+page.svelte (1)
137-205: Good optimistic update implementation with proper ID-based rollback.The like handler correctly captures
targetPostIdupfront and usesfindIndexinside theposts.updatecallback during rollback, ensuring the correct post is reverted even if the array has changed. This addresses the previous feedback about index-based rollback being unreliable.Minor edge case: Rapid consecutive clicks could cause
originalLikedByto be captured at different points in time, leading to potential state inconsistencies if an earlier request fails after a later one modifies the same post. Consider adding a per-post loading flag or debouncing the like action if this becomes a UX issue.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/pictique/src/routes/(protected)/home/+page.svelte
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (6)
platforms/pictique/src/routes/(protected)/home/+page.svelte (6)
7-23: LGTM!The imports correctly bring in the pagination stores (
hasMore,isLoading,isLoadingMore) and functions (loadMoreFeed,resetFeed), along withgetfromsvelte/storefor accessing store values in non-reactive contexts like the IntersectionObserver callback.
31-56: Well-implemented sentinel action.The IntersectionObserver correctly uses the viewport as root (addressing the previous feedback), includes appropriate guard conditions to prevent duplicate loads, and properly cleans up via
observer.disconnect()in the destroy callback.
111-115: Clean initialization flow.The
onMountcorrectly resets the feed state and fetches the first page. Not awaitingfetchFeedallows profile loading to proceed in parallel. The sentinel observer will naturally trigger additional loads if the initial content doesn't fill the viewport.
119-120: LGTM!The loading condition correctly shows the loading message only during initial load when no posts exist yet, preserving existing content visibility during pagination.
220-228: LGTM!The conditional rendering is well-structured: loading indicator during pagination, end-of-feed message when exhausted, and sentinel element when more content is available. The sentinel's minimal height ensures it doesn't affect layout while still triggering the observer.
248-248: No functional change.Class reordering doesn't affect behavior or styling.
Description of change
Implements lazy load on scroll in pictique
Issue Number
closes#647
Type of change
How the change has been tested
Manual
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.