feat(Utility): improve addScript method#8116
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefines the dynamic script-loading utility to distinguish between successful and failed loads using attribute-based state and cleans up the polling logic accordingly. Sequence diagram for updated addScript script-loading flowsequenceDiagram
participant Caller
participant addScript
participant Document
participant scriptElement
participant Timer
participant Console
Caller->>addScript: addScript(content)
addScript->>Document: querySelectorAll
alt scriptElement exists
addScript->>scriptElement: setAttribute(src, content)
else no scriptElement
addScript->>Document: createElement(script)
addScript->>scriptElement: setAttribute(src, content)
addScript->>Document: appendChild(scriptElement)
end
addScript->>scriptElement: onload = () => setAttribute(loaded, 'true')
addScript->>scriptElement: onerror = () => setAttribute(loaded, 'error')
addScript->>Timer: setInterval(handler, 20)
loop polling
Timer->>scriptElement: getAttribute(loaded)
alt state is 'true'
addScript->>Timer: clearInterval
addScript-->>Caller: resolve()
else state is 'error'
addScript->>Timer: clearInterval
addScript->>Console: console.error
addScript-->>Caller: resolve()
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Instead of polling the
loadedattribute withsetInterval, consider resolving the promise directly in theonload/onerrorhandlers and cleaning up event listeners to avoid unnecessary timers and potential hanging if neither event fires. - Right now the promise resolves both on success and error; if callers need to distinguish failures, consider rejecting on error or returning a more explicit result object so error handling can be done upstream.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of polling the `loaded` attribute with `setInterval`, consider resolving the promise directly in the `onload`/`onerror` handlers and cleaning up event listeners to avoid unnecessary timers and potential hanging if neither event fires.
- Right now the promise resolves both on success and error; if callers need to distinguish failures, consider rejecting on error or returning a more explicit result object so error handling can be done upstream.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates the dynamic script-loading helper in utility.js to track load state using string markers and to handle load failures without hanging indefinitely.
Changes:
- Track script load state via
loaded="true"/loaded="error"attributes. - Add
onerrorhandling and log a message when script loading fails. - Simplify the returned promise to always resolve (no explicit rejection path).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8116 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 766 766
Lines 34204 34204
Branches 4695 4695
=========================================
Hits 34204 34204
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Link issues
fixes #8115
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve the dynamic script loading utility to track and handle load success or failure states more robustly.
Bug Fixes:
Enhancements: