Skip to content

Conversation

@jduerholt
Copy link
Collaborator

Motivation

Fix the bug outlined here: #3033

Tests are not yet adapted, first, I want to get your opinion on the fix ;)

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Unit tests, not yet adapted.

@meta-cla meta-cla bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 2, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.98%. Comparing base (2591f32) to head (b3d4537).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##              main    #3034      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          216      216              
  Lines        20497    20531      +34     
===========================================
+ Hits         20497    20527      +30     
- Misses           0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hvarfner
Copy link
Contributor

hvarfner commented Oct 2, 2025

Hi @jduerholt ,

Thanks for spotting and fixing this. I haven't looked into it yet, but I'll start right away!

Copy link
Contributor

@saitcakmak saitcakmak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. I'll leave it to @hvarfner to give it a final look before merging.

Copy link
Contributor

@hvarfner hvarfner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great solution to me! Did you have a test or two for this in the works, too?

@jduerholt
Copy link
Collaborator Author

I will file some tests over the weekend ;)

@meta-codesync
Copy link

meta-codesync bot commented Oct 2, 2025

@esantorella has imported this pull request. If you are a Meta employee, you can view this in D83766789.

@jduerholt
Copy link
Collaborator Author

Hi @hvarfner, I added a test that checks for the desired behavior. I was only wondering regarding one data structure issue, see my comment in the code ;) Best, Johannes

@facebook-github-bot
Copy link
Contributor

@saitcakmak merged this pull request in 3ec2ab0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants