-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add prefetching support for external links in WebappButton component #2407
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request enhances the handling of external URLs in the WebappButton component by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant WB as WebappButton
participant PM as prefetchManager
U->>WB: Mouse enters external link
WB->>WB: Call isExternalURL() to verify URL
alt URL is external and prefetch enabled
WB->>PM: has(url)?
alt Not prefetched
PM-->>WB: false
WB->>PM: add(url)
WB->>U: Trigger prefetch logic
else Already prefetched
PM-->>WB: true
WB->>U: Continue without prefetch
end
else URL is internal
WB->>U: Render internal Link
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 (5)
src/features/common/WebappButton/index.tsx (4)
20-20
: Clarify usage of theprefetch
property
Consider adding a comment to delineate the difference between Next.js-specific prefetching vs. this custom approach for external URLs.
51-60
: Hover-based prefetch
This approach efficiently prevents multiple prefetch requests. Optionally, you could introduce a short hover delay for performance and bandwidth considerations.
62-82
: Potential accessibility issue
Nesting a<button>
inside an<a>
can confuse assistive technologies. Consider styling a single element (either<button>
or<a>
) to maintain consistent UX without nested interactive elements.
106-120
: Ignoringhref
withoutelementType='link'
This logic is valid. A comment clarifying that any providedhref
is disregarded unlesselementType='link'
might improve clarity for maintainers.src/utils/prefetchManager.ts (1)
1-6
: Effective prefetch manager
Using aSet
for tracking prefetched URLs is straightforward. Consider adding an eviction policy if URLs can accumulate over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/common/WebappButton/index.tsx
(4 hunks)src/features/projectsV2/ProjectSnippet/microComponents/ProjectInfoSection.tsx
(1 hunks)src/utils/prefetchManager.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
src/features/common/WebappButton/index.tsx (6)
3-3
: Good import for performance optimization
TheuseCallback
import helps optimize the event handler by memoizing the function reference. No changes needed here.
5-6
: Clear segregation of prefetch logic
The import ofprefetchManager
separates the prefetch logic neatly from the component, promoting maintainability.
33-33
: Consistent return type
ReturningReactElement
aligns with standard React conventions. This is good practice.
36-46
: Possible SSR caveat
window.location.host
might be undefined during SSR. Thetry/catch
prevents crashes, but consider ensuringisExternalURL
is only called client-side or handle SSR edge cases explicitly.
49-50
: External URL check
The logic for determining an external link is straightforward and correct.
91-93
: Conditional usage of Next.jsprefetch
The built-inprefetch
prop in Next.js works only for internal routes. Ensure this is expected behavior ifhref
points to an external resource.src/features/projectsV2/ProjectSnippet/microComponents/ProjectInfoSection.tsx (1)
101-101
: Explicitly enabling prefetch
Enablingprefetch={true}
ensures external link prefetching. IfdonateLink
is always external, this cleanly handles resource loading in advance.
Introduce prefetching functionality for external links in the WebappButton component, enhancing performance by preloading resources. A prefetch manager tracks prefetched URLs to avoid redundant requests.
Prefetch is currently added for Donate links across the Project List and Project Details pages.