-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
♻️ refactor: change discover page from rsc to SPA to improve performance #9828
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideThis PR migrates the Discover section from Next.js RSC to a client-side SPA using react-router-dom MemoryRouter. It replaces Next.js navigation and routing APIs with react-router hooks, restructures pages and layouts into SPA routes, adjusts URL paths and base segments, and introduces responsive layout handling for mobile vs desktop. Sequence diagram for navigation in Discover SPAsequenceDiagram
participant User as actor User
participant UI as Discover UI
participant Router as MemoryRouter
participant UrlSync as UrlSynchronizer
User->>UI: Clicks navigation link or button
UI->>Router: Calls useNavigate(link)
Router->>UI: Renders new route component
UI->>UrlSync: Location changes
UrlSync->>Browser: Updates browser URL (window.history.replaceState)
Browser->>User: Shows updated URL and page
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
There is too much information in the pull request to test. |
|
Thank you for raising your pull request and contributing to our Community |
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider replacing MemoryRouter with BrowserRouter (or HashRouter) to simplify URL handling and remove the need for manual URL synchronizer logic.
- The URL‐sync logic in UrlSynchronizer and getInitialPath is duplicated and could lead to subtle sync bugs—consolidate into a single effect or utility to avoid mismatches or infinite loops.
- You're using react-router Link for external URLs (with target="_blank"); switch to plain tags for those to prevent React Router from intercepting external navigations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing MemoryRouter with BrowserRouter (or HashRouter) to simplify URL handling and remove the need for manual URL synchronizer logic.
- The URL‐sync logic in UrlSynchronizer and getInitialPath is duplicated and could lead to subtle sync bugs—consolidate into a single effect or utility to avoid mismatches or infinite loops.
- You're using react-router Link for external URLs (with target="_blank"); switch to plain <a> tags for those to prevent React Router from intercepting external navigations.
## Individual Comments
### Comment 1
<location> `src/app/[variants]/(main)/discover/(list)/model/_layout/Desktop.tsx:7` </location>
<code_context>
import CategoryContainer from '../../../components/CategoryContainer';
import Category from '../features/Category';
-const Layout = async ({ children }: PropsWithChildren) => {
+const Layout = ({ children }: PropsWithChildren) => {
return (
<Flexbox gap={24} horizontal style={{ position: 'relative' }} width={'100%'}>
</code_context>
<issue_to_address>
**suggestion:** Removed unnecessary async from Layout component.
Removing 'async' clarifies the component's intent and aligns with React's best practices for component definitions.
</issue_to_address>
### Comment 2
<location> `src/app/[variants]/(main)/discover/features/useNav.tsx:18` </location>
<code_context>
const pathname = location.pathname;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {pathname} = location;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const { t } = useTranslation('discover'); | ||
|
|
||
| const activeKey = useMemo(() => { | ||
| const pathname = location.pathname; |
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.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const pathname = location.pathname; | |
| const {pathname} = location; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9828 +/- ##
=========================================
Coverage 84.55% 84.55%
=========================================
Files 944 944
Lines 64507 64507
Branches 7900 9284 +1384
=========================================
Hits 54541 54541
Misses 9966 9966
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🚀 Desktop App Build Completed!Version: Build Artifacts
Warning Note: This is a temporary build for testing purposes only. |
ceae476 to
5841900
Compare
arvinxx
left a comment
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.
lgtm now
|
❤️ Great PR @ONLY-yours ❤️ The growth of project is inseparable from user feedback and contribution, thanks for your contribution! If you are interesting with the lobehub developer community, please join our discord and then dm @arvinxx or @canisminor1990. They will invite you to our private developer channel. We are talking about the lobe-chat development or sharing ai newsletter around the world. |
### [Version 1.141.5](v1.141.4...v1.141.5) <sup>Released on **2025-10-22**</sup> #### ♻ Code Refactoring - **misc**: Change discover page from RSC to SPA to improve performance. <br/> <details> <summary><kbd>Improvements and Fixes</kbd></summary> #### Code refactoring * **misc**: Change discover page from RSC to SPA to improve performance, closes [#9828](#9828) ([b59ee0a](b59ee0a)) </details> <div align="right"> [](#readme-top) </div>
|
🎉 This PR is included in version 1.141.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [Version 1.135.0](v1.134.0...v1.135.0) <sup>Released on **2025-10-27**</sup> #### ♻ Code Refactoring - **misc**: Change discover page from RSC to SPA to improve performance. #### ✨ Features - **misc**: Use env to control clerk allow origin feature. #### 🐛 Bug Fixes - **misc**: Loadmore not work & navbar not show in pwa. #### 💄 Styles - **misc**: Adjust modal setting form styles for improved layout and responsiveness, Allow removal of `top_p` and similar request parameters, improve local system tools render, improve provider modal height when creating custom provider, Improvement for Agent Team After Alpha Launch [LOB-517], Unzip file when uploading in knowledge base [LOB-500], update i18n. <br/> <details> <summary><kbd>Improvements and Fixes</kbd></summary> #### Code refactoring * **misc**: Change discover page from RSC to SPA to improve performance, closes [lobehub#9828](https://github.com/jaworldwideorg/OneJA-Bot/issues/9828) ([b59ee0a](b59ee0a)) #### What's improved * **misc**: Use env to control clerk allow origin feature, closes [lobehub#9863](https://github.com/jaworldwideorg/OneJA-Bot/issues/9863) ([490fee0](490fee0)) #### What's fixed * **misc**: Loadmore not work & navbar not show in pwa, closes [lobehub#9855](https://github.com/jaworldwideorg/OneJA-Bot/issues/9855) ([411f875](411f875)) #### Styles * **misc**: Adjust modal setting form styles for improved layout and responsiveness, closes [lobehub#9890](https://github.com/jaworldwideorg/OneJA-Bot/issues/9890) ([1997ec5](1997ec5)) * **misc**: Allow removal of `top_p` and similar request parameters, closes [lobehub#9498](https://github.com/jaworldwideorg/OneJA-Bot/issues/9498) ([4c313ce](4c313ce)) * **misc**: Improve local system tools render, closes [lobehub#9853](https://github.com/jaworldwideorg/OneJA-Bot/issues/9853) ([295e8fc](295e8fc)) * **misc**: Improve provider modal height when creating custom provider, closes [lobehub#9870](https://github.com/jaworldwideorg/OneJA-Bot/issues/9870) ([55d92c0](55d92c0)) * **misc**: Improvement for Agent Team After Alpha Launch [LOB-517], closes [lobehub#9748](https://github.com/jaworldwideorg/OneJA-Bot/issues/9748) ([28245be](28245be)) * **misc**: Unzip file when uploading in knowledge base [LOB-500], closes [lobehub#9854](https://github.com/jaworldwideorg/OneJA-Bot/issues/9854) ([e568ce6](e568ce6)) * **misc**: Update i18n, closes [lobehub#9862](https://github.com/jaworldwideorg/OneJA-Bot/issues/9862) ([8d3bc91](8d3bc91)) </details> <div align="right"> [](#readme-top) </div>
💻 Change Type
🔀 Description of Change
📝 Additional Information
Summary by Sourcery
Convert the Discover section from Next.js server-rendered pages to a client-side SPA using React Router.
New Features:
Enhancements:
<Link to>/discoverprefix in internal routing while synchronizing browser historyBuild:
Chores: