-
-
Notifications
You must be signed in to change notification settings - Fork 4
Load less entries - esp. on mobile #2033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdjusted the entries list to import an in-house IsMobile hook and changed queryOptions.count from a fixed 10,000 to a conditional value: 1,000 when IsMobile.value is true, otherwise 5,000. No other logic paths were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/viewer/src/project/browse/EntriesList.svelte (1)
78-78: Good performance optimization for mobile devices.The conditional entry count (1,000 for mobile, 5,000 for desktop, down from 10,000) should improve initial load performance while accepting the trade-off of constrained scrolling.
Consider extracting the magic numbers as named constants for easier maintenance:
+const MAX_ENTRIES_MOBILE = 1_000; +const MAX_ENTRIES_DESKTOP = 5_000; + const fetchCurrentEntries = useDebounce(async (silent = false) => { if (!miniLcmApi) return []; if (!silent) loadingUndebounced = true; try { const queryOptions: IQueryOptions = { - count: IsMobile.value ? 1_000 : 5_000, + count: IsMobile.value ? MAX_ENTRIES_MOBILE : MAX_ENTRIES_DESKTOP, offset: 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/viewer/src/project/browse/EntriesList.svelte(2 hunks)
⏰ 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). (6)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: check-and-lint
🔇 Additional comments (1)
frontend/viewer/src/project/browse/EntriesList.svelte (1)
22-22: Verify that IsMobile is reactive and consider its scope.Ensure the
IsMobilehook properly reacts to viewport changes (e.g., device rotation, browser resizing). Note thatIsMobileis not included in theentriesResourcedependencies (line 100-102), so changes toIsMobile.valuealone won't trigger a refetch. This means if a user resizes from desktop to mobile, they'll still have 5,000 entries loaded rather than 1,000.Consider whether this behavior is acceptable or if
IsMobileshould be added to the resource dependencies:const entriesResource = resource( - () => ({ search, sort, gridifyFilter, miniLcmApi }), + () => ({ search, sort, gridifyFilter, miniLcmApi, isMobile: IsMobile.value }), async () => await fetchCurrentEntries());Note: Adding
IsMobileto dependencies would cause refetches on viewport changes, which could be disruptive if users are actively browsing.
Part of #2032
I'd love to have bullet-proof lazy-loading/infinite-scrolling, but not today.
This feels like a safe improvement.
The biggest problem with loading less entries, is that we can only scroll to entries that were loaded.
I think for now that's less important than performance. Especially on mobile.