Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughReformatted an existing Yarn Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
🧹 Nitpick comments (2)
package.json (2)
53-54: Redundant@react-aria/i18nresolution entries — consolidate.Lines 53 and 54 both map to the exact same patched target (
@react-aria/i18n@npm%3A3.12.5#...-435edff786.patch). If only^3.12.5is actually resolved in the dep graph now, the nightly-tagged entry on line 53 is dead config and can be dropped; otherwise keep both but add a brief comment explaining why two descriptor keys are needed. Worth verifying viayarn why@react-aria/i18n`` before removing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 53 - 54, The package.json contains two resolution keys for the same patched target: "@react-aria/i18n@npm:^3.0.0-nightly-fb28ab3b4-241024" and "@react-aria/i18n@npm:^3.12.5" both pointing to the same patch; run `yarn why `@react-aria/i18n`` to confirm which descriptor is actually used, then either remove the dead/nightly key ("@react-aria/i18n@npm:^3.0.0-nightly-fb28ab3b4-241024") if unused, or keep both but add a short comment explaining why both descriptors are required; update the resolutions section to only include necessary keys that map to "patch:`@react-aria/i18n`@npm%3A3.12.5#~/.yarn/patches/@react-aria-i18n-npm-3.12.5-435edff786.patch".
55-55: Track upstream and add a maintenance note.Carrying a patch against
@react-aria/overlays@3.25.0means every bump to this dep will need a manual patch refresh (or it will silently fail to apply). Recommendations:
- Open/link an upstream issue or PR in
adobe/react-spectrumreferencing SSGA-9 so this patch can be retired.- Add a short
README/header comment inside the.patchfile explaining what it fixes and the upstream tracking link, so the next person bumping react-aria knows whether to drop it.- Consider pinning the descriptor more tightly (e.g.
~3.25.0) to avoid a future^bump silently picking up a version the patch no longer cleanly applies to.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 55, The package.json entry carrying a patch for "@react-aria/overlays@npm:^3.25.0" should be documented and made easier to maintain: open an upstream issue/PR in the adobe/react-spectrum repo referencing SSGA-9 asking to accept or upstream the fix so the local patch can be retired; add a short header comment at the top of the existing patch file named ".yarn/patches/@react-aria-overlays-npm-3.25.0-2628866e6e.patch" describing what it fixes and a link to the upstream issue/PR; and tighten the package descriptor in package.json from "^3.25.0" to a more conservative range (e.g. "~3.25.0" or an exact pin) so future dependency bumps don’t silently break patch application.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 55: The patch correctly computes actualWindow in
getWindowAndVisualViewport (using targetNode?.ownerDocument.defaultView) but
still reads visualViewport from the global window; update the visualViewport
assignment to use actualWindow instead of window so listeners subscribe to the
correct viewport for overlays in popups/iframes—i.e., inside
getWindowAndVisualViewport, set visualViewport = actualWindow?.visualViewport ||
null and ensure all references to visualViewport come from that returned value
used by the overlay code (functions/methods that call
getWindowAndVisualViewport).
---
Nitpick comments:
In `@package.json`:
- Around line 53-54: The package.json contains two resolution keys for the same
patched target: "@react-aria/i18n@npm:^3.0.0-nightly-fb28ab3b4-241024" and
"@react-aria/i18n@npm:^3.12.5" both pointing to the same patch; run `yarn why
`@react-aria/i18n`` to confirm which descriptor is actually used, then either
remove the dead/nightly key
("@react-aria/i18n@npm:^3.0.0-nightly-fb28ab3b4-241024") if unused, or keep both
but add a short comment explaining why both descriptors are required; update the
resolutions section to only include necessary keys that map to
"patch:`@react-aria/i18n`@npm%3A3.12.5#~/.yarn/patches/@react-aria-i18n-npm-3.12.5-435edff786.patch".
- Line 55: The package.json entry carrying a patch for
"@react-aria/overlays@npm:^3.25.0" should be documented and made easier to
maintain: open an upstream issue/PR in the adobe/react-spectrum repo referencing
SSGA-9 asking to accept or upstream the fix so the local patch can be retired;
add a short header comment at the top of the existing patch file named
".yarn/patches/@react-aria-overlays-npm-3.25.0-2628866e6e.patch" describing what
it fixes and a link to the upstream issue/PR; and tighten the package descriptor
in package.json from "^3.25.0" to a more conservative range (e.g. "~3.25.0" or
an exact pin) so future dependency bumps don’t silently break patch application.
🪄 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
Run ID: bba6eea1-04fa-4a70-b1c3-37fb3913d82b
⛔ Files ignored due to path filters (2)
.yarn/patches/@react-aria-overlays-npm-3.25.0-2628866e6e.patchis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
package.json
gabriellsh
force-pushed
the
patch/useOverlayPosition
branch
from
d3e3b07 to
6a26b00
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40284 +/- ##
========================================
Coverage 69.80% 69.80%
========================================
Files 3296 3296
Lines 119173 119173
Branches 21435 21446 +11
========================================
+ Hits 83183 83187 +4
Misses 32684 32684
+ Partials 3306 3302 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
gabriellsh
changed the title
1 participant
Proposed changes (including videos or screenshots)
This PR patches
useOverlayPositionhook fromreact-ariapackage in order to get the correctdocumentandwindowreference of where the element is being used (popup window for example), instead of relying in the globalwindowobject, which can be different from the actual window that the element is being rendered on (such as a popup window with React portals).Issue(s)
SSGA-9
Steps to test or reproduce
Further comments
Summary by CodeRabbit