fix: load more manual click - #244
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesScroll-aware pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Page
participant PageScrollContext
participant LoadMoreButton
participant useInView
participant WithdrawalList
Page->>PageScrollContext: provide Scrollable root
LoadMoreButton->>PageScrollContext: read scroll root
LoadMoreButton->>useInView: observe with 200px root margin
useInView-->>LoadMoreButton: report inView
LoadMoreButton->>WithdrawalList: invoke onClick when not loading
WithdrawalList-->>LoadMoreButton: provide isFetchingNextPage
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
The symptom in the Before video (all remaining pages loading instantly on scroll) seems to come from two implementation defects rather than the auto-scroll pattern itself:
Would you be open to trying a fix that keeps the pattern and addresses the root cause instead? Something like: // LoadMoreButton.tsx
useEffect(() => {
if (inView && !disabled) onClick()
}, [inView, disabled, onClick])// BridgeHistory.tsx
const loadMore = useCallback(() => setPage((page) => page + 1), [])
// WithdrawalList.tsx
const loadMore = useCallback(() => fetchNextPage(), [fetchNextPage])This should make loading trigger only when the intersection state actually changes, preserving the scroll UX. Could you test whether this resolves the instant-load issue on your end? |
Yes, I came across the same root causes. I considered fixing the effect directly, but decided dropping auto-scroll entirely was the better call. Separately, auto-triggering the instant the button glimpses into view isn't very intuitive. I went with manual click since that's the typical pattern in Initia apps, it's more aesthetic, and it's simpler to reason about long-term. Let me know which direction you would like to go. Do you think we should just have auto-scroll and just get rid of the button? |
Deploying interwovenkit-testnet with
|
| Latest commit: |
c069851
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ab03340d.interwovenkit-testnet.pages.dev |
| Branch Preview URL: | https://fix-loadmore-manual-click.interwovenkit-testnet.pages.dev |
Deploying interwovenkit with
|
| Latest commit: |
c069851
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4d6d2dd2.interwovenkit.pages.dev |
| Branch Preview URL: | https://fix-loadmore-manual-click.interwovenkit.pages.dev |
Deploying interwovenkit-staging with
|
| Latest commit: |
c069851
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca125f6d.interwovenkit-staging.pages.dev |
| Branch Preview URL: | https://fix-loadmore-manual-click.interwovenkit-staging.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/interwovenkit-react/src/components/LoadMoreButton.tsx`:
- Around line 1-2: Remove the useInView observer and useEffect/useEffectEvent
machinery from LoadMoreButton, including the viewport-margin configuration and
effect that invokes onClick. Preserve the button’s existing onClick handler so
loading occurs only through manual clicks, while retaining the disabled
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a09425c-0541-4d5a-9cc4-90594e7c8568
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/interwovenkit-react/src/components/LoadMoreButton.tsx
…ss a11y/spacing review feedback
- Guard clicks instead of disabling so focus and aria-busy survive - Rename the disabled prop to isLoading to match its render behavior - Remove the :disabled color; --gray-6 was under 1.5:1 in both themes - Correct the comment on why the click fallback is needed
Before
LoadMoreButtonusedreact-intersection-observerto auto-fetch all remaining pages instantly upon scrolling into view. Because it triggered immediately, the button's text could flash on screen for no functional reason before vanishing.before.mov
After
LoadMoreButtonis now a deterministic, manually clicked button, aligning with standard UX patterns across Initia applications.suggested-update.mov
Summary by CodeRabbit
New Features
Bug Fixes