fix(eslint): stop prefer-global-this making the browser framing check unspellable - #36
Conversation
… unspellable `window.parent === window` is how DOM code asks "am I in an iframe?" before posting to a host. Under the shipped rule set that check had no valid spelling: window.parent === window -> unicorn/prefer-global-this parent === self -> unicorn/prefer-global-this globalThis.parent === globalThis -> unicorn/no-unnecessary-global-this parent === globalThis -> sonarjs/different-types-comparison The last one is not a style opinion. lib.dom types `parent`, `top`, `opener` and `MessageEvent.source` as `Window` while `globalThis` is `typeof globalThis`, so the type-aware rule correctly calls the comparison always-false. Consumers were left choosing between a suppression and an `as unknown as Window` bridge. `prefer-global-this` also contradicts `sonarjs/no-global-this`, which we already ship as an error: one demands the `globalThis` the other forbids. Turn `prefer-global-this` off and keep `no-unnecessary-global-this`, so `globalThis.parent` still trims to `parent` for plain member access.
📝 WalkthroughWalkthroughThe ESLint quality configuration disables ChangesBrowser framing rule configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/eslint/quality.js`:
- Around line 114-116: Update the comments describing the browser framing check
to use the correct polarity: window.parent === window identifies a top-level
window, not an embedded iframe. Revise the comments at src/eslint/quality.js
lines 114-116 and test/browser-framing-check.test.mjs lines 9-12; no behavior
changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1958dc11-c55f-4809-aab3-90e95cf4a270
📒 Files selected for processing (2)
src/eslint/quality.jstest/browser-framing-check.test.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/browser-framing-check.test.mjs (1)
41-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the final quality configuration.
These assertions verify only the last configured values for two rules. They do not lint
window.parent === windowwith the shippedqualityconfiguration. Another enabled rule or a later configuration entry could still reject the check while these assertions pass. Add an integration assertion that runs the framing source through the final flat configuration and verifies the expected diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/browser-framing-check.test.mjs` around lines 41 - 60, Extend test/browser-framing-check.test.mjs to lint the framing source through the shipped final quality configuration, rather than only resolving rule values. Add an integration assertion that checks window.parent === window produces the expected diagnostics, while preserving the existing resolve assertions for unicorn/no-unnecessary-global-this and unicorn/prefer-global-this.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/browser-framing-check.test.mjs`:
- Around line 41-60: Extend test/browser-framing-check.test.mjs to lint the
framing source through the shipped final quality configuration, rather than only
resolving rule values. Add an integration assertion that checks window.parent
=== window produces the expected diagnostics, while preserving the existing
resolve assertions for unicorn/no-unnecessary-global-this and
unicorn/prefer-global-this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29dc15b3-eb98-4619-83c0-af69f7e0641c
📒 Files selected for processing (2)
src/eslint/quality.jstest/browser-framing-check.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/eslint/quality.js
1 participant
Summary
The canonical browser framing check —
window.parent === window, how DOM code asks "am I inside an iframe?" — has no spelling that passes our shipped rule set. Every rewrite one rule accepts, another rejects. This turnsunicorn/prefer-global-thisoff so the check is writable again.The dead end
Each candidate spelling is rejected by a different shipped rule:
window.parent === windowunicorn/prefer-global-thisparent === selfunicorn/prefer-global-thisglobalThis.parent === globalThisunicorn/no-unnecessary-global-thisparent === globalThissonarjs/different-types-comparisonThe last row is not a style opinion.
lib.domtypesparent,top,opener, andMessageEvent.sourceasWindow, whileglobalThisistypeof globalThis. Those types do not overlap, so the type-aware rule is correct to call the comparison always-false — which is exactly why there is no legal spelling. In DOM codewindowis the typed global, not a legacy alias forglobalThis.Two rules already contradict each other
Independent of the DOM case, the preset ships both:
unicorn/prefer-global-this: "error"— demandsglobalThissonarjs/no-global-this: "error"— forbidsglobalThisOne rule requires what the other bans. Turning
prefer-global-thisoff makes the pair consistent.Resolution
unicorn/prefer-global-this: "off"in thequalityconcern, beside the other unicorn opinion rules that fight the stack.unicorn/no-unnecessary-global-thisstays aterror, so the useful half of the pair is kept:globalThis.parentstill trims toparentfor plain member access. Only the comparison case was unspellable.Why this matters to consumers
Without it, a consumer writing an iframe host bridge has two bad options: park a
unicorn/prefer-global-thissuppression in the baseline, or writeparent === (globalThis as unknown as Window)— a cast whose only purpose is to satisfy linting. Both hide a real, correct DOM idiom behind noise.Verification
eslint .clean.test/browser-framing-check.test.mjspins the behavior empirically rather than by assertion:window.parent === window, so the test fails if the conflict ever disappears upstream and this override becomes dead weight;prefer-global-thisoff andno-unnecessary-global-thisaterror;prefer-global-this/sonarjs/no-global-thiscontradiction so a future re-enable trips a test rather than a consumer's lint run.window.parent === windowlints clean where it previously errored.Risk
Low and narrow. The change only removes a stylistic preference for
globalThisoverwindow/self. No autofix behavior changes, no type-aware rule is touched, and the rule that does the genuinely useful cleanup stays on. Consumers currently suppressingunicorn/prefer-global-thiswill find those entries become obsolete and can be pruned at their next baseline refresh.Summary by CodeRabbit
Refactor
Tests