feat: Global theming APIs#4559
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4559 +/- ##
==========================================
- Coverage 97.42% 97.42% -0.01%
==========================================
Files 938 938
Lines 29680 29716 +36
Branches 10788 10793 +5
==========================================
+ Hits 28917 28952 +35
- Misses 716 757 +41
+ Partials 47 7 -40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| interface WindowWithTheme extends Window { | ||
| [themeStorageKey]?: Theme; | ||
| } | ||
|
|
||
| function getTopWindow(): WindowWithTheme { | ||
| try { | ||
| if (window.top) { | ||
| return window.top as WindowWithTheme; | ||
| } | ||
| } catch { | ||
| // Cross-origin access error — fall back to current window. | ||
| } | ||
| return window as WindowWithTheme; | ||
| } |
There was a problem hiding this comment.
Is there a reason we go straight to the top vs using the findUpAPI that already exists for plugins?
We also use getTopWIndow in our ErrorBoundary component.
There is also a key piece missing here that both of those have: the cross-origin SecurityError check -- which is only thrown when accessing writable properties on the window directly. We need to add that to the first statement to make the catch have any effect.
function getTopWindow(): WindowWithTheme {
try {
if (window.top && window.top.document) { // <-- this forces cross-orign to return the security error here
return window.top as WindowWithTheme;
}
} catch {
// Cross-origin top — fall back to current window.
}
return window as WindowWithTheme;
Should we use a shared utility here? We could've prevented this bug (and any future fixes that we may need to make).
jkuelz
left a comment
There was a problem hiding this comment.
Looks good, just one comment on the getTopWindow fn.
Description
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md. YesCONTRIBUTING.md. YesSecurity
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.