Merged
Conversation
* feat: add update page based on notion * style: update asynchronous pattern Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor: UpdatePage to use constants for default IDs --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
feat@timetable-infinite_scroll
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughThe updates introduce infinite scrolling to the lecture list in the timetable section, adjust search result limits, and add a new "UpdatePage" route and component that fetches and displays Notion content. Supporting changes include a new dependency ( Changes
Sequence Diagram(s)Infinite Scrolling in Lecture ListsequenceDiagram
participant User
participant Scroller
participant LectureListSection
participant Server
participant ReduxStore
User->>Scroller: Scrolls to bottom
Scroller->>LectureListSection: onScrollChange event
LectureListSection->>LectureListSection: _addLectureGroups()
LectureListSection->>Server: Fetch lectures (offset, limit)
Server-->>LectureListSection: Returns lecture data
LectureListSection->>ReduxStore: setListLecturesDispatch (append lectures)
LectureListSection->>LectureListSection: Update state with new lectures
Rendering Notion Page in UpdatePagesequenceDiagram
participant User
participant UpdatePage
participant NotionAPI
participant Browser
User->>UpdatePage: Navigates to /update?pageId=...
UpdatePage->>NotionAPI: Fetch Notion page data
NotionAPI-->>UpdatePage: Returns block map JSON
UpdatePage->>UpdatePage: Filter/process block map, extract space_id
UpdatePage->>Browser: Redirect if space_id is not official
UpdatePage->>User: Render Notion page content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new update page feature and enhances lecture search functionality with pagination support. The changes add the ability to display Notion-based content and implement infinite scrolling for lecture search results.
- Added a new UpdatePage component that renders Notion content with security validation
- Modified lecture search to use pagination with a reduced limit and infinite scrolling
- Added necessary dependencies and routing for the new functionality
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/UpdatePage.tsx | New component for displaying Notion-based update content with space ID validation |
| src/index.tsx | Added routing for the new update page |
| src/components/sections/timetable/lecturelist/LectureSearchSubSection.jsx | Reduced search limit and added pagination parameters |
| src/components/sections/timetable/lecturelist/LectureListSection.jsx | Added infinite scrolling functionality for lecture search results |
| src/components/sections/dictionary/courselist/CourseListSection.jsx | Removed unused reference and lifecycle method |
| package.json | Added react-notion dependency |
Comments suppressed due to low confidence (1)
package.json:49
- The react-notion package version ^0.10.0 appears to be non-existent. The latest available version is much lower (around 0.2.x).
"react-notion": "^0.10.0"
Comment on lines
+14
to
+29
| const query = new URLSearchParams(window.location.search); | ||
| const pageId = query.get('pageId') || DEFAULT_PAGE_ID; | ||
| const [spaceId, setSpaceId] = useState(''); | ||
|
|
||
| useEffect(() => { | ||
| const fetchNotionData = async () => { | ||
| const res = await fetch(`https://notion-api.splitbee.io/v1/page/${pageId}`); | ||
| const json = await res.json(); | ||
| Object.keys(json).forEach((key) => { | ||
| if (json[key].role === 'none') { | ||
| delete json[key]; | ||
| } else { | ||
| setSpaceId(json[key].value.space_id); | ||
| } | ||
| }); | ||
| setBlockMap(json); |
There was a problem hiding this comment.
The fetch operation lacks error handling. If the API request fails, the component will not provide feedback to the user about the failure.
Suggested change
| const query = new URLSearchParams(window.location.search); | |
| const pageId = query.get('pageId') || DEFAULT_PAGE_ID; | |
| const [spaceId, setSpaceId] = useState(''); | |
| useEffect(() => { | |
| const fetchNotionData = async () => { | |
| const res = await fetch(`https://notion-api.splitbee.io/v1/page/${pageId}`); | |
| const json = await res.json(); | |
| Object.keys(json).forEach((key) => { | |
| if (json[key].role === 'none') { | |
| delete json[key]; | |
| } else { | |
| setSpaceId(json[key].value.space_id); | |
| } | |
| }); | |
| setBlockMap(json); | |
| const [error, setError] = useState<string | null>(null); | |
| const query = new URLSearchParams(window.location.search); | |
| const pageId = query.get('pageId') || DEFAULT_PAGE_ID; | |
| const [spaceId, setSpaceId] = useState(''); | |
| useEffect(() => { | |
| const fetchNotionData = async () => { | |
| try { | |
| const res = await fetch(`https://notion-api.splitbee.io/v1/page/${pageId}`); | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch data: ${res.status} ${res.statusText}`); | |
| } | |
| const json = await res.json(); | |
| Object.keys(json).forEach((key) => { | |
| if (json[key].role === 'none') { | |
| delete json[key]; | |
| } else { | |
| setSpaceId(json[key].value.space_id); | |
| } | |
| }); | |
| setBlockMap(json); | |
| } catch (err: any) { | |
| setError(err.message || 'An error occurred while fetching data.'); | |
| } |
Comment on lines
+25
to
+28
| } else { | ||
| setSpaceId(json[key].value.space_id); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The spaceId is being set multiple times in the loop, potentially overwriting previous values. This could lead to inconsistent behavior if multiple blocks have different space_ids.
Suggested change
| } else { | |
| setSpaceId(json[key].value.space_id); | |
| } | |
| }); | |
| } | |
| }); | |
| // Set spaceId from the root block (pageId) | |
| if (json[pageId] && json[pageId].value && json[pageId].value.space_id) { | |
| setSpaceId(json[pageId].value.space_id); | |
| } |
| const option = { | ||
| ...lastSearchOption, | ||
| offset, | ||
| limit: offset + REFRESH_LIMIT, |
There was a problem hiding this comment.
The limit parameter should be REFRESH_LIMIT, not offset + REFRESH_LIMIT. The current implementation requests an increasing number of items instead of a fixed batch size.
Suggested change
| limit: offset + REFRESH_LIMIT, | |
| limit: REFRESH_LIMIT, |
| } | ||
|
|
||
| onScrollChange = (e) => { | ||
| const { scrollTop, scrollHeight, clientHeight } = e; |
There was a problem hiding this comment.
The destructuring is incorrect. Scroll event properties should be accessed from e.target, not directly from e.
Suggested change
| const { scrollTop, scrollHeight, clientHeight } = e; | |
| const { scrollTop, scrollHeight, clientHeight } = e.target; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores