feat: remove preview script deps from url #1405
Conversation
WalkthroughThe changes restructure how material dependencies (scripts and styles) are managed and passed within the preview system. Dependency data is now fetched and aggregated explicitly, cached for efficiency, and no longer embedded in URL query parameters. The preview initialization and update logic is refactored to support this new dependency handling approach. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Preview
participant usePreviewData
participant MaterialSource
User->>Preview: Trigger preview initialization
Preview->>usePreviewData: loadInitialData()
usePreviewData->>MaterialSource: getMaterialDeps()
MaterialSource-->>usePreviewData: {scripts, styles}
usePreviewData->>usePreviewData: updatePreview({currentPage, ancestors, scripts, styles})
usePreviewData->>usePreviewData: getBasicData(basicFilesPromise, scripts)
usePreviewData-->>Preview: Render with appData, metaData, importMapData
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/common/js/preview.js (2)
244-245: Handleundefinedplatform to avoid leaking literal “undefined” into the URLWhen
platformIdis absent, the template literal will serialise the value as the string “undefined”, producing …&platform=undefined.
Consider coercing to an empty string or omitting the param altogether to keep URLs clean.- query += `&platform=${platform}` + if (platform) { + query += `&platform=${platform}` + }
263-265: Lazy-compute heavy dependency lists only when needed
getScriptAndStyleDeps()is called on every preview, but the result is only used in the (comparatively rare) history-preview branch below.
Pull the call inside theisHistorybranch (or memoise the helper) to avoid redundant work and large object allocations.packages/design-core/src/preview/src/preview/usePreviewData.ts (5)
38-63:updateUrlrefactor looks sound but consider reusingURLSearchParamsThe early‐return cases and parameter toggling logic are correct.
Minor: you parselocation.searchtwice (here and ingetPageOrBlockByApi). Caching theURLSearchParamsinstance would avoid repeated parsing when this function is called frequently.
208-218: Concurrent fetches can trigger duplicate network traffic
getAppDatacaches the response inpreviewState.appData, but if two calls happen before the first one resolves, the API will be hit twice.
You can avoid this by storing the pending promise or by using a simple mutex.-let previewState.appData = null +let previewState.appData: Promise<any> | null = null(then resolve/cache accordingly)
220-234: Edge-case: protect against falsy / malformed CSS entries
addScriptAndStylecurrently skips empty strings but not other falsy values (e.g.,null).
A small guard keeps thestylesset clean:- if (Array.isArray(pkg.css)) { - pkg.css.forEach((style: string) => { - if (style) { + if (Array.isArray(pkg.css)) { + pkg.css.forEach((style: string) => { + if (typeof style === 'string' && style.trim()) {
283-308: Unused promise element – clean up thePromise.allarray
basicFilesPromiseis awaited but its resolved value is ignored, which is confusing to readers.-const promises = [getAppData(), fetchMetaData(metaDataParams), getImportMap(scripts), basicFilesPromise] -const [appData, metaData, importMapData] = await Promise.all(promises) +await basicFilesPromise // ensure base files are ready +const [appData, metaData, importMapData] = await Promise.all([ + getAppData(), + fetchMetaData(metaDataParams), + getImportMap(scripts) +])This clarifies intent and avoids the “missing” fourth variable.
368-376: Type safety: expose an interface forupdatePreviewparams
updatePreviewnow expects{ currentPage, ancestors, scripts, styles }.
Exporting a dedicated interface keeps call sites honest and eases refactors.export interface IPreviewUpdateParams { currentPage: IPage ancestors: IPage[] scripts: Record<string, string> styles: string[] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
designer-demo/src/preview.js(1 hunks)packages/common/js/preview.js(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/design-core/src/preview/src/preview/usePreviewData.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
designer-demo/src/preview.js (1)
28-32: Verify bundle path & provide fallback for “material” resources
material: ['/mock/bundle.json']will 404 in production builds unless the file is shipped alongside the compiled assets.
Please double-check:
- The path is resolved relative to the built
index.html(or make it absolute).- The bundle is included in the build artefacts / CDN.
You may want to gate this behind
import.meta.env.DEVor move the mock into public assets.packages/design-core/src/preview/src/preview/Preview.vue (1)
60-66: Call-site updated correctly – good catch
updateUrlnow takes only thecurrentPageparameter, and the call site has been fixed accordingly.
No further action required.packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
433-437: Load-first render path looks goodInitial data now waits for material deps before the first render, ensuring import maps & styles are ready – nice improvement.
| const getMaterialDeps = async () => { | ||
| const bundleUrls = getMergeMeta('engine.config')?.material || [] | ||
| const materials = await Promise.allSettled( | ||
| bundleUrls.map((url: any) => (typeof url === 'string' ? getMetaApi(META_SERVICE.Http).get(url) : url)) | ||
| ) | ||
|
|
||
| const scripts = new Map<string, string>() | ||
| const styles = new Set<string>() | ||
| const appData = await getAppData() | ||
|
|
||
| if (Array.isArray(appData.utils)) { | ||
| appData.utils | ||
| .filter((item: Record<string, any>) => item.type === 'npm') | ||
| .forEach((item: Record<string, any>) => { | ||
| addScriptAndStyle(scripts, styles, { | ||
| package: item.content.package, | ||
| script: item.content.cdnLink, | ||
| css: item.content.cdnLink | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| materials | ||
| .filter((item) => item.status === 'fulfilled' && item.value.materials) | ||
| .map((item: any) => item.value.materials) | ||
| .forEach((item) => { | ||
| const { packages, components } = item | ||
|
|
||
| if (Array.isArray(packages)) { | ||
| packages.forEach((pkg) => { | ||
| addScriptAndStyle(scripts, styles, pkg) | ||
| }) | ||
| } | ||
|
|
||
| if (Array.isArray(components)) { | ||
| components.forEach((component) => { | ||
| addScriptAndStyle(scripts, styles, component.npm) | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| return { | ||
| scripts: Object.fromEntries(scripts), | ||
| styles: Array.from(styles) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Material dependency aggregation – consider error logging & timeouts
The silent Promise.allSettled pattern is good for resilience, but failed downloads are swallowed.
Adding minimal logging (in dev) or a timeout will help diagnose broken bundle URLs.
Also note that getMetaApi(META_SERVICE.Http).get should ideally be wrapped with an AbortController to prevent hanging previews when a bundle host is slow.
🤖 Prompt for AI Agents
In packages/design-core/src/preview/src/preview/usePreviewData.ts between lines
236 and 281, the Promise.allSettled call silently ignores failed downloads,
which can hide issues with bundle URLs. To fix this, add error logging for any
rejected promises to help diagnose failures. Additionally, wrap the
getMetaApi(META_SERVICE.Http).get calls with an AbortController to enforce a
timeout, preventing the preview from hanging if a bundle host is slow or
unresponsive.
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