It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?

I can add that , so in general more caption edge cases

I just checked the test for sorting, and it shows that the captions are not sorted. After rechecking the code, I realized that the captions are implemented as a getter, which means it’s a read-only operation.

I have two solutions in mind:

1- Modify the public-facing controller by adding a getter to retrieve the sorted captions. However, based on my observations, it’s not recommended to change the interface, so this solution might not be valid.

2- Ensure that all ClosedCaptionFile classes return sorted captions. I will implement this solution for now, and if the public-facing approach turns out to be better, we can always revert to it.

Edit: fix number 2 is not clean since it doesnt enforce the sorting so i am going with the first fix

tests implemented and improved the logic for the binary search since it was incorrect in some cases

for this test test('captions are sorted correctly on initialization'
this test can be ignored and if thats the case we can remove the new public member to the class (my main issue is that i was not able to expose a private member only for testing) if someone can point me to the right direction that will help a lot

From triage: what's the status of this PR? Is it ready for re-review?

From triage: what's the status of this PR? Is it ready for re-review?

Yes it's ready for a re-review

I switched the logic to use whenComplete to only call the sort once on the future completion, I hope that matches what you meant, if not I currently don't know how to solve the event listener call to avoid resorting on every event fired,

the plan in my mind currently to add a flag so I know if the current file was sorted or not, but don't know if its good or not.

@abdelaziz-mahdy Are you still planning on updating this PR to address the review comments above?

@abdelaziz-mahdy Are you still planning on updating this PR to address the review comments above?

Sadly I have been quite busy lately, and not easy to figure out, so I cant fix it yet

Marking as a draft pending updates.

Greetings from stale PR triage! 👋
Is this change still on your radar?

Review comments addressed. Ready for re-review.

Changes Made:

1. Optimized caption lookup

• Added local variable in _getCaptionAt to avoid repeated force-unwraps
• Removed unnecessary validity check after binary search (compare function guarantees correctness)

2. Fixed caching strategy

• Refactored setClosedCaptionFile to always sort when explicitly setting a new file
• Updated _updateClosedCaptionWithFuture to only sort on first initialization (_sortedCaptions == null)
• Prevents expensive re-sorting on every event listener call

3. Fixed test structure

• Unnested incorrectly placed tests

4. Added edge case tests

Added 5 new tests covering binary search edge cases:

• Exact caption start/end time boundaries
• Gaps between captions
• Single caption scenario
• Overlapping captions

The caching optimization ensures captions are only sorted when the file actually changes (initialization or explicit setClosedCaptionFile call), avoiding performance issues.

Sorry for the wait, this one slid through my process