Skip to content

fix: defer inline scripts in HTML preview so Chart.js and CDN scripts load before chart code runs (#1228)#1247

Open
spider-yamet wants to merge 11 commits intoeigent-ai:mainfrom
spider-yamet:fix/render-images-fail-render-in-HTML
Open

fix: defer inline scripts in HTML preview so Chart.js and CDN scripts load before chart code runs (#1228)#1247
spider-yamet wants to merge 11 commits intoeigent-ai:mainfrom
spider-yamet:fix/render-images-fail-render-in-HTML

Conversation

@spider-yamet
Copy link
Contributor

Summary

Fixes charts and script-driven visuals not rendering when opening HTML reports (e.g. in_progress_priority_impact_report.html) in the file viewer. Inline scripts were running before external scripts (e.g. Chart.js from CDN), so Chart was undefined and canvas charts did not draw.

Changes

  • src/lib/htmlFontStyles.ts: Added deferInlineScriptsUntilLoad(html). When the document contains at least one <script src="...">, it wraps inline <script> content in window.addEventListener('load', ...) so it runs after all resources (including CDN scripts) have loaded.
  • src/components/Folder/index.tsx: Uses deferInlineScriptsUntilLoad when building HTML for the iframe before setting srcdoc, so chart-initialization code runs only after Chart.js is available.

Testing

  • Open an HTML report that uses Chart.js (or similar CDN script) with inline chart code in the Folder/file viewer. Charts should render in the preview.
  • HTML without external scripts is unchanged (no wrapping).

Closes #1228

@spider-yamet
Copy link
Contributor Author

@Wendong-Fan @bytecii @4pmtong @Pakchoioioi could you please review the PR?

Best Regards

Copy link
Collaborator

@bytecii bytecii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

hasExternal = true;
break;
}
idx = lower.indexOf('<script', idx + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all script tags are js scripts right? for example, type="module" and type="application/ld+json"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that some script attributes will be removed of the script such as crossorigin etc?

const content = html.slice(contentStart, contentEnd);
if (!hasSrc && content.trim().length > 0) {
const escaped = content.replace(/<\/script>/gi, '<\\/script>');
result += `<script>window.addEventListener('load',function(){${escaped}});</script>`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use DOMContentLoaded instead of load? Instead of waiting all things such as images?

@spider-yamet
Copy link
Contributor Author

@bytecii Let me summarize the update.

1. Not all script tags are JS
Added isClassicInlineJs(attrs) so we only wrap classic inline JS.
Scripts with type="module" or type="application/ld+json" are left as-is (no wrapping).
Only classic scripts (no type or text/javascript) are deferred.
2. Preserve script attributes
Wrapped output now keeps the original opening tag: we use openTag (from <script through >) and emit ${openTag}window.addEventListener(...)</script>.
Attributes like crossorigin, type="text/javascript", etc. are preserved on deferred scripts.
3. DOMContentLoaded instead of load
Replaced window.addEventListener('load', ...) with window.addEventListener('DOMContentLoaded', ...) so we don’t wait for images/resources, only for the DOM and synchronous scripts (e.g. CDN script) to be ready.

Copy link
Collaborator

@bytecii bytecii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM but it will be good if others can take a quick look at this @Wendong-Fan

@spider-yamet
Copy link
Contributor Author

@Wendong-Fan @bytecii Could you please check PR status?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Bug: Images fail to render within HTML content

2 participants