-
Notifications
You must be signed in to change notification settings - Fork 343
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
fix: adjust the preview of the current page to schema data #1095
base: refactor/develop
Are you sure you want to change the base?
fix: adjust the preview of the current page to schema data #1095
Conversation
WalkthroughThe pull request updates function signatures in the page composable. In Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main.vue (preview)
participant U as usePage.js (getFamily)
participant H as usePage.js (handlePageDetail)
M->>U: Call getFamily(params)
U->>H: Call handlePageDetail(familyPages, params)
H-->>U: Return updated family pages
U-->>M: Return ancestors based on updated details
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 (
|
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: 2
🧹 Nitpick comments (1)
packages/plugins/page/src/composable/usePage.js (1)
398-403
: Add defensive programming for schema access.The schema access could be more defensive to prevent potential runtime errors.
- if (currentPage.id && currentPage.pageInfo?.schema && page.id === currentPage.id) { - page.page_content = currentPage.pageInfo?.schema + if (currentPage?.id && currentPage?.pageInfo?.schema && page?.id === currentPage.id) { + page.page_content = currentPage.pageInfo.schema
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/page/src/composable/usePage.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/plugins/page/src/composable/usePage.js (1)
435-447
: Improve parameter naming for better clarity.The parameter name
previewParams
is not descriptive enough of its structure. Based on the previous review comments, this parameter contains framework, platform, and pageInfo.-const getFamily = async (previewParams) => { +const getFamily = async (pageContext) => { if (pageSettingState.pages.length === 0) { await getPageList() } - const familyPages = getAncestorsRecursively(previewParams.id) + const familyPages = getAncestorsRecursively(pageContext.id) .filter((item) => item.isPage) .reverse() - await handlePageDetail(familyPages, previewParams) + await handlePageDetail(familyPages, pageContext) return familyPages }
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 (2)
packages/plugins/page/src/composable/usePage.js (2)
417-421
: Rename function for better clarity.The function name
updateParentId
doesn't fully convey that it's fixing invalid parent references.-const updateParentId = (page, pages, index, ROOT_ID) => { +const fixInvalidParentReference = (page, pages, index, ROOT_ID) => {
439-451
: Improve parameter naming for better clarity.The parameter name
previewParams
is too generic. Based on its usage, it represents the current page's context.-const getFamily = async (previewParams) => { +const getFamily = async (currentPageContext) => { if (pageSettingState.pages.length === 0) { await getPageList() } - const familyPages = getAncestorsRecursively(previewParams.id) + const familyPages = getAncestorsRecursively(currentPageContext.id) .filter((item) => item.isPage) .reverse() - await handlePageDetail(familyPages, previewParams) + await handlePageDetail(familyPages, currentPageContext)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/page/src/composable/usePage.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/page/src/composable/usePage.js (3)
423-437
: 🛠️ Refactor suggestionAdd error handling for Promise.all operation.
The concurrent execution could fail silently if any of the promises reject.
if (pages.length > 0) { + try { pages = await Promise.all( pages.map(async (page, index) => { page = updatePageContent(page, currentPage) page = await fetchPageDetailIfNeeded(page) - page = updateParentId(page, pages, index, ROOT_ID) + page = fixInvalidParentReference(page, pages, index, ROOT_ID) return page }) ) + } catch (error) { + console.error('Failed to process page details:', error) + throw error // Re-throw to allow caller to handle the error + } }Likely invalid or redundant comment.
398-403
: 🛠️ Refactor suggestionAdd optional chaining for safer property access.
The deep property access could throw if
pageInfo
is undefined.- if (currentPage.id && currentPage.pageInfo?.schema && page.id === currentPage.id) { - page.page_content = currentPage.pageInfo?.schema + if (currentPage?.id && currentPage.pageInfo?.schema && page.id === currentPage.id) { + page.page_content = currentPage.pageInfo.schemaLikely invalid or redundant comment.
405-415
: 🛠️ Refactor suggestionImprove error handling in fetchPageDetailIfNeeded.
The function silently swallows errors and sets an empty object. This could hide issues and make debugging difficult.
try { const pageDetail = await http.fetchPageDetail(page.id) page.page_content = pageDetail.page_content } catch (error) { + console.error(`Failed to fetch page detail for page ${page.id}:`, error) page.page_content = {} }
Likely invalid or redundant comment.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
页面预览显示内容为上一次保存的内容
Issue Number: N/A
What is the new behavior?
预览逻辑调整为当前页面获取schema数据,保持以前不需要保存就可以预览的逻辑
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit