-
-
Notifications
You must be signed in to change notification settings - Fork 96
feat: automatic refetch css override #1588
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
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a CSS override query key, removes the prior override URL construction, adds socket-triggered cache invalidation for CSS overrides, refactors the runtime stylesheet hook to fetch and inject CSS via React Query, and updates ViewLoader to rely on a new override-focused hook for render gating. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing touches🧪 Generate unit tests
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. Comment |
a709cde to
389d023
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/client/src/common/api/constants.ts(1 hunks)apps/client/src/common/hooks-query/useOverrideStylesheet.ts(1 hunks)apps/client/src/common/hooks/useRuntimeStylesheet.js(0 hunks)apps/client/src/common/utils/socket.ts(2 hunks)apps/client/src/features/viewers/lower-thirds/LowerThird.tsx(2 hunks)apps/client/src/views/ViewLoader.tsx(1 hunks)apps/server/src/api-data/assets/assets.controller.ts(3 hunks)apps/server/src/api-data/view-settings/viewSettings.controller.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/client/src/common/hooks/useRuntimeStylesheet.js
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/server/src/api-data/assets/assets.controller.ts (1)
apps/server/src/adapters/websocketAux.ts (1)
sendRefetch(7-12)
apps/client/src/common/utils/socket.ts (2)
apps/client/src/common/queryClient.ts (1)
ontimeQueryClient(3-9)apps/client/src/common/api/constants.ts (2)
VIEW_SETTINGS(15-15)CSS_OVERRIDE(16-16)
apps/client/src/views/ViewLoader.tsx (1)
apps/client/src/common/hooks-query/useOverrideStylesheet.ts (1)
useOverrideStylesheet(17-48)
apps/client/src/features/viewers/lower-thirds/LowerThird.tsx (1)
apps/client/src/common/hooks-query/useOverrideStylesheet.ts (1)
useOverrideStylesheet(17-48)
🔇 Additional comments (21)
apps/client/src/common/api/constants.ts (1)
16-16: New constant for CSS override query keyAdding
CSS_OVERRIDEconstant establishes a clear query key for managing the CSS override cache, which is used by the new automatic refetch system. This aligns with the existing pattern of defining query keys and follows good practices for constants management.apps/server/src/api-data/assets/assets.controller.ts (3)
4-4: Importing WebSocket notification utilityThe import of
sendRefetchis appropriate for implementing the notification system to alert clients when CSS overrides change.
27-27: Send notification on CSS override updateGood implementation of the CSS override notification after a successful update. This ensures clients are informed immediately when the CSS is changed, triggering automatic refetching.
40-40: Send notification on CSS restoreSimilar to the update path, this correctly sends a notification when CSS is restored to default. The consistent implementation across both modification endpoints ensures reliable updates for clients.
apps/client/src/common/utils/socket.ts (2)
4-4: Added constants for WebSocket handlerThe addition of
CSS_OVERRIDEandVIEW_SETTINGSto the imports is necessary for the expanded WebSocket message handling. This change correctly prepares the required constants.
215-219: Handle CSS and view settings refetch notificationsThe new conditional branches correctly handle WebSocket messages for invalidating the CSS override and view settings caches. This implementation completes the automatic refetch cycle by responding to server notifications and triggering the appropriate cache invalidations.
The branching logic follows the existing pattern in the file, maintaining consistency with how other refetch notifications are handled.
apps/client/src/views/ViewLoader.tsx (2)
3-3: Import new CSS override hookThe import change correctly switches to the new
useOverrideStylesheethook that centralizes CSS override logic.
8-8: Simplified implementation with new hookThe component now uses a single hook that handles all CSS override logic internally, simplifying this component's implementation while maintaining the same functionality. The
shouldRenderflag works as before to control the loading state.This is a good improvement as it encapsulates the CSS override logic in a dedicated hook, making the ViewLoader component cleaner and more focused on its primary responsibility.
apps/server/src/api-data/view-settings/viewSettings.controller.ts (3)
7-7: Good addition of the sendRefetch importThe import of the
sendRefetchfunction is correctly added to support the new functionality for notifying clients when CSS overrides change.
15-15: Good proactive caching of the previous stateStoring the previous
overrideStylesstate before making changes is a good practice to allow comparison later.
27-30: Properly implemented change detection for style overridesThe implementation correctly compares the old and new override states and triggers a notification only when there's an actual change, avoiding unnecessary network traffic.
apps/client/src/features/viewers/lower-thirds/LowerThird.tsx (4)
3-4: Modified import to remove ViewSettingsThe import has been correctly updated to remove the unneeded ViewSettings import, aligning with the new approach using the dedicated hook.
7-7: Added useOverrideStylesheet hook importThe new hook import looks good and matches the implementation pattern seen across the application.
54-55: Removed viewSettings from props destructuringCorrectly updated the props destructuring to remove the viewSettings param which is no longer needed.
59-59: Implemented useOverrideStylesheet hookThe hook is correctly implemented without arguments, simplifying the component by abstracting the CSS override logic.
apps/client/src/common/hooks-query/useOverrideStylesheet.ts (6)
1-6: Good imports and dependency setupThe necessary imports for React Query, constants, and the view settings hook are correctly defined.
7-8: Defined consistent ID for the style tagUsing a constant for the style tag ID is a good practice for consistency and maintainability.
9-15: Well-implemented fetch function with error handlingThe fetch function correctly handles the response and throws an appropriate error if the fetch fails.
17-26: Good React Query configurationThe React Query setup is robust with appropriate retry logic, placeholder data handling, and network mode settings.
28-32: Clean status tracking and view settings accessThe shouldRender flag and view settings extraction are implemented clearly.
47-48: Good return valueReturning the shouldRender flag provides a clean way for components to know when the stylesheet has been successfully loaded.
389d023 to
c1eaba0
Compare
cpvalente
left a comment
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.
Lets leave this for now. I dont think the trade off is right just yet
When users change the style of the page, the clients need to refresh. I think that is fair
Once we migrate to react 19 we get better tools for managing this and we can reconsider the entire solution
https://react.dev/blog/2024/12/05/react-19#support-for-metadata-tags
c1eaba0 to
114a282
Compare
114a282 to
26d5a0b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/client/src/common/api/constants.ts (2)
19-19: Add missing semicolon for consistency.All other exports in this file end with semicolons; keep this consistent to satisfy linters/formatters.
-export const CSS_OVERRIDE = ['cssOverride'] +export const CSS_OVERRIDE = ['cssOverride'];
19-19: Consider typing the query key as a readonly tuple.This prevents accidental mutation and improves TS inference, but only adopt if you plan to apply it consistently to all keys.
-export const CSS_OVERRIDE = ['cssOverride']; +export const CSS_OVERRIDE = ['cssOverride'] as const;apps/client/src/common/hooks/useRuntimeStylesheet.ts (1)
23-28: Update the docstring to match the new design (no path, content-driven).Avoid stale references to a “stylesheet path.”
/** - * When a view mounts or the stylesheet path changes we need to handle potentially loading a new stylesheet - * - if no path is given, ensure there is no stylesheet loaded - * - if a path is given, fetch the stylesheet and inject it into the document head + * When a view mounts or when the override flag/content changes, handle the runtime override stylesheet: + * - if override is disabled or content is missing, ensure no stylesheet is present + * - if enabled and content is available, inject or update a single <style> tag in document.head * @returns { shouldRender: boolean } - after the stylesheet is handled and the clients are ready to render */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/client/src/common/api/constants.ts(1 hunks)apps/client/src/common/hooks/useRuntimeStylesheet.ts(2 hunks)apps/client/src/common/utils/socket.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/client/src/common/utils/socket.ts
🧰 Additional context used
🪛 ast-grep (0.38.6)
apps/client/src/common/hooks/useRuntimeStylesheet.ts
[warning] 44-44: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: styleSheet.innerHTML = cssData
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 44-44: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: styleSheet.innerHTML = cssData
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-test
🔇 Additional comments (3)
apps/client/src/common/hooks/useRuntimeStylesheet.ts (3)
2-7: LGTM on adopting React Query for override fetching.Imports and integration look correct and align with the PR goal.
11-11: All useRuntimeStylesheet call sites now use the updated signature
No callers pass arguments or reference legacy override URL/path parameters.
15-21: No action needed:placeholderDatausage aligns with TanStack Query v5 signature.
|
since we are now in react 19, could we just use normal header tags? |
* fix: default import map * fix: update test
56c1f0e to
ff3d662
Compare
automatically tell users of the css override to re-fetch it when it is changed