-
Notifications
You must be signed in to change notification settings - Fork 0
Main roadmap instui11 #63
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
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
This PR updates the @instructure.ai/roadmap package to use InstUI version 10.25.1-pr-snapshot-1758747818199, along with implementing caching mechanisms and code improvements.
- Updates all InstUI dependencies to a new PR snapshot version
- Implements caching for roadmap and brand config API calls to improve performance
- Adds sourcemap generation and code quality improvements
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.mts | Enables sourcemap generation for better debugging |
| packages/roadmap/src/utils/getRoadmap.ts | Adds caching mechanism to prevent duplicate API calls |
| packages/roadmap/src/utils/getBrandConfig.ts | Implements caching and promise cleanup for brand config |
| packages/roadmap/src/components/tagList.tsx | Minor formatting fix for trailing comma |
| packages/roadmap/src/components/cardOverlay.tsx | Improves code style with arrow function syntax and nullish coalescing |
| packages/roadmap/src/App.tsx | Optimizes rendering with useMemo and separates resize event handling |
| packages/roadmap/public/themeEditor.js | Prevents duplicate event listeners using WeakMap |
| Multiple package.json files | Updates InstUI dependency versions to new snapshot |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| roadmapPromise = null; | ||
| resolve(result); | ||
| } | ||
| }; | ||
| window.addEventListener("message", handler); | ||
| }); |
Copilot
AI
Sep 29, 2025
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.
Setting roadmapPromise to null inside the promise handler creates a race condition. If multiple calls happen simultaneously, the second call might see roadmapPromise as null after the first resolves but before the second can use the cached result. Consider setting roadmapPromise to null after the promise resolves outside the handler.
| roadmapPromise = null; | |
| resolve(result); | |
| } | |
| }; | |
| window.addEventListener("message", handler); | |
| }); | |
| resolve(result); | |
| } | |
| }; | |
| window.addEventListener("message", handler); | |
| }); | |
| roadmapPromise.then(() => { | |
| roadmapPromise = null; | |
| }); |
| brandConfigPromise.finally(() => { | ||
| brandConfigPromise = null; | ||
| }); |
Copilot
AI
Sep 29, 2025
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.
The finally block correctly cleans up the promise reference, but there's still a potential race condition. If the promise rejects, cachedBrandConfig remains null but brandConfigPromise is reset, which could lead to multiple simultaneous requests on subsequent calls. Consider handling the rejection case to maintain consistent state.
@instructure.ai/roadmap