refactor: Extract upper-bound binary search from findNearestLogEventByTimestamp into a function (resolves #343).#347
Conversation
…estamp log event search
|
Warning Rate limit exceeded@Henry8192 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughReplaced JsonlDecoder's manual binary search with a new generic utility Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JsonlDecoder
participant MathUtils
Client->>JsonlDecoder: findNearestLogEventByTimestamp(timestamp)
JsonlDecoder->>MathUtils: upperBoundBinarySearch(this.#logEvents, timestamp, get: e => e.timestamp.valueOf())
MathUtils-->>JsonlDecoder: index or null
JsonlDecoder-->>Client: index or null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/services/decoders/JsonlDecoder/index.ts(2 hunks)src/utils/math.ts(2 hunks)test/utils/math.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (4)
📓 Common learnings
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
test/utils/math.test.ts (3)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-10-10T20:26:31.573Z
Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
src/services/decoders/JsonlDecoder/index.ts (8)
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-10-10T20:26:31.573Z
Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-10-08T15:52:50.753Z
Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-09-26T08:12:44.589Z
Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-10-08T15:52:50.753Z
Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-09-26T08:12:44.589Z
Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/decoders/ClpIrDecoder.ts:42-42
Timestamp: 2024-11-18T01:35:35.812Z
Learning: The interface `YscopeFormatterOptionsType` is defined in `src/typings/formatters.ts` and is used to type the options passed to `YscopeFormatter`, ensuring type safety in constructor calls like `new YscopeFormatter({ formatString: decoderOptions.formatString })`.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx:1-7
Timestamp: 2024-10-28T18:40:29.816Z
Learning: In the y-scope/yscope-log-viewer project, importing components via destructuring from '@mui/joy' is acceptable, as the package size impact has been analyzed and found acceptable.
src/utils/math.ts (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
🧬 Code Graph Analysis (3)
test/utils/math.test.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(69-69)
src/services/decoders/JsonlDecoder/index.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(69-69)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🪛 Biome (1.9.4)
src/utils/math.ts
[error] 52-52: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 54-54: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (7)
src/utils/math.ts (3)
1-1: LGTM: Import statement correctly adds Nullable type.The import statement properly brings in the
Nullabletype from the common typings module, which is used in the function return type.
69-69: LGTM: Export statement correctly includes the new function.The export statement properly adds the new
upperBoundBinarySearchfunction to the module exports.
43-45: Consider using explicit boolean comparison per coding guidelines.The coding guidelines prefer
false == <expression>rather than!<expression>.Apply this diff to align with coding guidelines:
- if (highIdx < lowIdx || "undefined" === typeof (get(highIdx))) { + if (highIdx < lowIdx || false === (typeof get(highIdx) !== "undefined")) { return null; }However, the current implementation is more readable. Consider whether strict adherence to this guideline improves code clarity in this case.
⛔ Skipped due to learnings
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#84 File: new-log-viewer/src/contexts/NotificationContextProvider/index.tsx:0-0 Timestamp: 2024-10-11T19:37:56.597Z Learning: In our TypeScript codebase with strong typing, variables are strictly typed and cannot be `undefined` where `null` is expected, so checking for `undefined` in null checks is unnecessary.test/utils/math.test.ts (2)
5-5: LGTM: Import statement correctly adds the new function.The import statement properly includes the
upperBoundBinarySearchfunction from the math utils module.
55-103: Excellent comprehensive test coverage for the binary search function.The test suite thoroughly covers all important scenarios:
✅ Edge cases handled:
- Empty collections with invalid ranges
- Single-element arrays
- Values outside the array bounds
✅ Boundary conditions tested:
- First and last element matches
- Values between elements
- Duplicate element handling
✅ Algorithm correctness verified:
- All test expectations align with the expected binary search upper bound behavior
- Test data includes negative numbers, duplicates, and various array sizes
The test cases correctly validate that the function returns:
nullfor invalid ranges0when all elements are greater than the search value- The correct index for the last element ≤ the search value
src/services/decoders/JsonlDecoder/index.ts (2)
21-21: LGTM: Import statement correctly adds the utility function.The import statement properly brings in the
upperBoundBinarySearchfunction from the math utils module.
142-149: Excellent refactoring that simplifies the timestamp search logic.The refactoring successfully replaces the manual binary search implementation with the new utility function. The implementation is much cleaner and more maintainable:
✅ Correct parameter mapping:
get: (idx) => this.#logEvents[idx]?.timestamp.valueOf()- Proper accessor function with safe navigationlowIdx: 0- Correct starting indexhighIdx: this.#logEvents.length - 1- Correct ending indexupperboundValue: timestamp- Correct search target✅ Behavior preservation:
- Returns the same type (
Nullable<number>)- Maintains the same upper bound binary search semantics
- Handles edge cases (empty arrays, out-of-bounds values) correctly
The use of optional chaining (
?.) in the getter function is particularly good as it handles potential undefined log events gracefully.
Henry8192
changed the title
upperboundBinarySearch and refactor timestamp log event search (resolves #343).upperboundBinarySearch and refactor timestamp log event search (resolves #343).
quinntaylormitchell
left a comment
quinntaylormitchell
left a comment
There was a problem hiding this comment.
PR is looking good Henry; a few high-level comments for you as the code itself looks pretty good:
- I would flesh out your “Description” section. I would A) communicate the purpose for the PR, B) say whether or not you changed any logic (or if it’s just a pure refactor), and C) provide a link to issue #343.
- In the “Validation performed” section, the link you provided does go directly to a log result, but I’m not sure how that link by itself provides validation for the PR. On the page I see when I follow the link, I don’t see the “query” you’re referring to. I ran the query
2015-03-24T01:09:myself and it successfully returned all 37 of the logs on page 38 that occurred during 1:09am, so maybe you can put that in the validation section (with the link) if you’d like to flesh it out. - I agree with the rabbit re. improving the detail in the docstring for the function, but parameters like
lowIdxandhighIdxare self-explanatory and don't need explanation. - I kind of agree with the rabbit re. avoiding parameter reassignment, but I also think it’s good that you stuck to the conventions about naming 0-based index variables with the
Idxsuffix. @kirkrodrigues or @junhaoliao what do you think?
Just to clarify, that link is to query timestamp, which is not related to query strings (that's searching for keywords). By adding This link is just a check to prove this existing feature is working. Hope this would clarify your questions. |
discussed with @kirkrodrigues and he says it's fine if we mention this in the pr title. |
|
@quinntaylormitchell thanks for the review! I've reworded the descriptions in |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/math.ts (1)
26-37: Documentation needs completion and clarification.The JSDoc comment has several issues that were previously identified but haven't been addressed:
- The
@returnsection is incomplete - it doesn't describe the actual return value- Parameter descriptions are missing for most parameters
- The null return case needs clearer documentation per previous feedback
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/typings/decoders.ts(1 hunks)src/utils/math.ts(2 hunks)test/utils/math.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/typings/decoders.tstest/utils/math.test.tssrc/utils/math.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.802Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-10-10T20:26:31.573Z
Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
src/typings/decoders.ts (1)
Learnt from: Henry8192
PR: #80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the #searchChunk method of LogFileManager.ts, checking typeof match.index === "number" correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
test/utils/math.test.ts (4)
Learnt from: junhaoliao
PR: #115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The clamp function in src/utils/math.ts correctly handles cases where max is less than min by returning the upper boundary.
Learnt from: Henry8192
PR: #80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the #searchChunk method of LogFileManager.ts, checking typeof match.index === "number" correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
Learnt from: Henry8192
PR: #107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx:9-12
Timestamp: 2024-10-28T18:39:28.680Z
Learning: In the Result component (Result.tsx), if end < start, the for loop won't run and it will return nothing, so additional validation for matchRange indices is not necessary.
Learnt from: Henry8192
PR: #80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-10-10T20:26:31.573Z
Learning: When a function like #searchChunkAndScheduleNext requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
src/utils/math.ts (10)
Learnt from: junhaoliao
PR: #115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The clamp function in src/utils/math.ts correctly handles cases where max is less than min by returning the upper boundary.
Learnt from: Henry8192
PR: #347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.802Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Learnt from: Henry8192
PR: #80
File: new-log-viewer/src/services/LogFileManager.ts:297-313
Timestamp: 2024-10-09T02:08:56.053Z
Learning: In the #searchChunk method of LogFileManager.ts, checking typeof match.index === "number" correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
Learnt from: Henry8192
PR: #196
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx:55-58
Timestamp: 2025-03-03T18:10:02.451Z
Learning: In the y-scope/yscope-log-viewer repository, the team prefers using the ! operator to flip boolean values (e.g., !isRegex) rather than the pattern false == <expression> that's mentioned in the coding guidelines.
Learnt from: junhaoliao
PR: #224
File: src/contexts/states/viewStore.ts:108-109
Timestamp: 2025-05-09T01:07:32.803Z
Learning: For the viewStore in the yscope-log-viewer project, it's more appropriate to suppress the max-lines-per-function ESLint rule than to split the store into slices due to the tight coupling and cohesive nature of its functions.
Learnt from: Henry8192
PR: #152
File: src/components/AppController.tsx:42-42
Timestamp: 2025-07-15T16:29:02.609Z
Learning: In the y-scope/yscope-log-viewer repository, the team never uses == for comparisons, preferring strict equality (===) instead. This overrides the coding guidelines that suggest using false == <expression> rather than !<expression>.
Learnt from: Henry8192
PR: #107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx:1-7
Timestamp: 2024-10-28T18:40:29.816Z
Learning: In the y-scope/yscope-log-viewer project, importing components via destructuring from '@mui/joy' is acceptable, as the package size impact has been analyzed and found acceptable.
Learnt from: junhaoliao
PR: #285
File: package-lock.json:33-33
Timestamp: 2025-05-23T15:19:35.710Z
Learning: For the y-scope/yscope-log-viewer and y-scope/clp repositories, the team has decided to lock the "eslint-config-yscope" dependency at a specific version (not "latest" or "*"). When a new version is released, the version string will be updated explicitly in each referencing repository.
Learnt from: junhaoliao
PR: #94
File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118
Timestamp: 2024-10-19T03:33:29.578Z
Learning: In new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx, when using useEffect to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (getPanelWidth, PANEL_CLIP_THRESHOLD_IN_PIXELS, and tabListRef) are constants or refs declared outside the functional component and do not change.
Learnt from: Henry8192
PR: #80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-10-10T20:26:31.573Z
Learning: When a function like #searchChunkAndScheduleNext requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
🧬 Code Graph Analysis (2)
test/utils/math.test.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(70-70)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🔇 Additional comments (7)
src/utils/math.ts (4)
1-2: LGTM: Clean import addition.The import of
Nullabletype is appropriate for the function's return type annotation.
48-57: Parameter reassignment acknowledged per team preference.The static analysis correctly identifies parameter reassignment, but based on previous discussion, the team prefers this implementation style for binary search algorithms to avoid unnecessary local variables.
59-65: Logic correctly handles edge case.The corner case handling for when all values are greater than or equal to
upperboundValueis implemented correctly. WhenhighIdxbecomes negative after the loop, returning0represents the first index as documented.
70-70: Export addition is appropriate.The function is correctly added to the exports list for external usage.
src/typings/decoders.ts (1)
131-131: Documentation update correctly reflects the interface contract.The clarification that the method returns "null if there are no log events" properly aligns with the refactored implementation that uses
upperBoundBinarySearch, which can return null for invalid ranges or empty collections.test/utils/math.test.ts (2)
5-5: Import addition is correct.The import of
upperBoundBinarySearchis properly added to the existing import statement.
55-103: Comprehensive test coverage with good edge case handling.The test suite covers all critical scenarios:
- Empty collections (both valid and invalid ranges)
- Single-element arrays
- Multiple elements with duplicates
- Boundary conditions (first/last elements)
- Values that don't exist in the array
- Upper bound matching unique and duplicate values
The test data
[-5, -3, 0, 1, 1, 8, 8, 8, 100]provides good coverage with negative numbers, zero, duplicates, and a large positive value. The assertions correctly verify the expected behaviour of finding the last index whereelement <= upperboundValue.
Henry8192
force-pushed
the
extract-binary-search
branch
from
776298b to
7461712
Compare
quinntaylormitchell
left a comment
quinntaylormitchell
left a comment
There was a problem hiding this comment.
Looks good Henry - left a comment on a change that I think is outside the scope of the PR, but other than that I think you're good to go.
quinntaylormitchell
left a comment
quinntaylormitchell
left a comment
There was a problem hiding this comment.
Looks good to me!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/utils/math.ts (3)
48-56: Fix variable names in the comment; keep parameter reassignment as per project preference.The inline comment references
low/high, but the code useslowIdx/highIdx.Apply this diff:
- // `mid` is guaranteed to be within bounds since `low <= high`. + // `mid` is guaranteed to be within bounds since `lowIdx <= highIdx`.Note: Maintaining parameter reassignment is acceptable here given the stated team preference to avoid introducing local variables for binary search bounds.
44-46: Guard negativelowIdxto align with the documented “invalid indices → null” behaviour.Currently,
lowIdx < 0proceeds into the loop and may produce a non-null result. Either document minimum preconditions or treat negativelowIdxas invalid.Apply this diff:
- if (highIdx < lowIdx || "undefined" === typeof (get(highIdx))) { + if (highIdx < lowIdx || 0 > lowIdx || "undefined" === typeof get(highIdx)) { return null; }
26-37: Fix typos and fully specify the contract (ascending sort, inclusive indices, null semantics).Clarify the doc to avoid ambiguity and fix
upperboundValuecasing.Apply this diff:
/** - * Finds the last element in a sorted collection that is less than or equal to `upperboundValue`. If - * all elements in the collection are greater than `upperboundValue`, returns the first index of the - * collection (i.e., `0`). + * Finds the index of the last element in a sorted (ascending) collection that is less than or equal + * to `upperBoundValue`. If all elements in the collection are greater than `upperBoundValue`, + * returns the first index of the collection (i.e., `0`). * - * @param get Function to access elements in the collection by index. - * @param lowIdx - * @param highIdx - * @param upperBoundValue - * @return The index of the last element less than or equal to `upperboundValue`, `0` if all - * elements are greater, or `null` if the collection is empty or the indices are invalid. + * @param get Function to access elements in the collection by index. Must return `undefined` for + * out-of-range indices. + * @param lowIdx The starting index (inclusive) of the search range. + * @param highIdx The ending index (inclusive) of the search range. + * @param upperBoundValue The value to search for. + * @return The index of the last element <= `upperBoundValue`, `0` if all elements are greater, + * or `null` if the collection is empty or the indices are invalid. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/utils/math.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/utils/math.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
src/utils/math.ts
📚 Learning: 2025-07-24T21:28:56.838Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Applied to files:
src/utils/math.ts
🧬 Code Graph Analysis (1)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🔇 Additional comments (2)
src/utils/math.ts (2)
1-1: LGTM: correctNullableimport location.Import path and type usage look right for this codebase.
70-70: LGTM: export added in the public utils surface.Good to see this utility reusable elsewhere.
src/utils/math.ts
Outdated
| const upperBoundBinarySearch = <T>( | ||
| get: (index: number) => T, | ||
| lowIdx: number, | ||
| highIdx: number, | ||
| upperBoundValue: T, | ||
| ): Nullable<number> => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type-safety: constrain T or accept a comparator to ensure valid <= comparisons.
get(mid) <= upperBoundValue assumes T is comparable with <=. To make this safe and self-documenting, either constrain T to a comparable type (e.g., number) or accept a comparator.
Minimal change (matches current usage with timestamps as numbers):
-const upperBoundBinarySearch = <T>(
+const upperBoundBinarySearch = <T extends number>(
get: (index: number) => T,
lowIdx: number,
highIdx: number,
upperBoundValue: T,
): Nullable<number> => {Alternatively (if you want generic support), introduce a comparator:
const upperBoundBinarySearchBy = <T>(
get: (index: number) => T,
lowIdx: number,
highIdx: number,
upperBoundValue: T,
cmp: (a: T, b: T) => number,
): Nullable<number> => {
if (highIdx < lowIdx || "undefined" === typeof get(highIdx)) return null;
while (lowIdx <= highIdx) {
const mid = Math.floor((lowIdx + highIdx) / 2);
if (cmp(get(mid), upperBoundValue) <= 0) lowIdx = mid + 1;
else highIdx = mid - 1;
}
return 0 > highIdx ? 0 : highIdx;
};🤖 Prompt for AI Agents
In src/utils/math.ts around lines 38 to 43, the binary search uses get(mid) <=
upperBoundValue on a generic T which is unsafe; either constrain T to a
comparable type (e.g., change signature to <T extends number>) so numeric
comparisons are type-safe, or add a comparator argument (e.g., cmp: (a: T, b: T)
=> number), replace the <= check with cmp(get(mid), upperBoundValue) <= 0, and
update the return logic and all callers accordingly (or expose a new
upperBoundBinarySearchBy name if you want to keep the old API).
…d high bounds; improve documentation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/utils/math.ts (3)
60-63: Typo: “upperboundValue” → “upperBoundValue”.Keeps terminology consistent with the parameter name and docstring.
Apply this diff:
- // corner case: all values >= upperboundValue + // Corner case: all values >= upperBoundValue
26-36: Docstring: correct inaccuracies and clarify contract.The function no longer accepts index bounds, so “indices are invalid” is misleading. Also clarify sort order, the return semantics, and parameter naming to match the implementation.
Apply this diff:
/** - * Finds the last element in a sorted collection that is less than or equal to `upperBoundValue`. If - * all elements in the collection are greater than `upperBoundValue`, returns the first index of the - * collection (i.e., `0`). + * Finds the index of the last element in a sorted collection whose mapped value is less than + * or equal to `upperBoundValue`. If all elements are greater than `upperBoundValue`, returns + * the first index of the collection (i.e., `0`). Returns `null` if the collection is empty. + * + * Assumes the array is sorted in ascending order by the value returned from `valueFn`. * - * @param get Function to access an element (or its property) in the collection. - * @param array - * @param upperBoundValue - * @return The index of the last element less than or equal to `upperBoundValue`, `0` if all - * elements are greater, or `null` if the collection is empty or the indices are invalid. + * @param valueFn Function that maps an element to a comparable number. + * @param array The sorted collection to search. + * @param upperBoundValue The numeric target value for the upper-bound search. + * @return The index of the last element <= `upperBoundValue`, `0` if all elements are greater, + * or `null` if the collection is empty. */
37-41: Type-safety: constrain the comparable type to number (or accept a comparator).
<=on a genericTis not type-safe and will fail type-checking unlessTis constrained. Given current usage (timestamps), constrainingTtonumberis simplest and self-documenting.Apply this diff:
-const upperBoundBinarySearch = <T, U>( - get: (logEvent: U) => T, +const upperBoundBinarySearch = <T extends number, U>( + valueFn: (item: U) => T, array: U[], upperBoundValue: T, ): Nullable<number> => {And update the usage inside the function:
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - if (get(array[mid]!) <= upperBoundValue) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + if (valueFn(array[mid]!) <= upperBoundValue) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/services/decoders/JsonlDecoder/index.ts(2 hunks)src/utils/math.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/services/decoders/JsonlDecoder/index.tssrc/utils/math.ts
🧠 Learnings (2)
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
src/utils/math.ts
📚 Learning: 2025-07-24T21:28:56.838Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Applied to files:
src/utils/math.ts
🧬 Code Graph Analysis (2)
src/services/decoders/JsonlDecoder/index.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(71-71)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🔇 Additional comments (4)
src/services/decoders/JsonlDecoder/index.ts (2)
21-21: Import looks good; reuse over reimplementation.Pulling in the shared utility reduces duplication and centralizes the search logic.
142-148: Refactor to shared binary search is correct and maintains semantics.Passing a numeric timestamp extractor and delegating to the utility matches the intended “last ≤ target, else 0; null if empty” behaviour.
src/utils/math.ts (2)
1-1: Import of Nullable is correct.Matches usage and keeps types explicit.
68-72: Explicitly exporting the new utility is good.Keeps the module’s public surface clear and consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/math.ts (2)
60-63: Fix typo in comment: “upperboundValue” → “upperBoundValue”.Keeps naming consistent with the parameter.
- // corner case: all values >= upperboundValue + // corner case: all values >= upperBoundValue
37-41: Constrain generic T to number (or add a comparator) —<=on unconstrained T is unsafeUsing
<=on an unconstrained generic T will fail under strict TS (TS2365). I verified the call sites — they all use numeric values — so the minimal, safe fix is to constrain T to number.Files to update / check:
- src/utils/math.ts — function signature (around lines 37–41)
- src/services/decoders/JsonlDecoder/index.ts — call uses timestamp.valueOf() (lines ~143–146)
- test/utils/math.test.ts — tests use numeric arrays (lines ~60–102)
Suggested minimal change:
-const upperBoundBinarySearch = <T, U>( +const upperBoundBinarySearch = <T extends number, U>( get: (logEvent: U) => T, array: U[], upperBoundValue: T, ): Nullable<number> => {If you need generic support for non-numeric types, add a comparator-based variant (e.g.
upperBoundBinarySearchBy) instead of using the<=operator directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/utils/math.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/utils/math.ts
🧠 Learnings (2)
📚 Learning: 2025-07-24T21:28:56.838Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Applied to files:
src/utils/math.ts
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
src/utils/math.ts
🧬 Code Graph Analysis (1)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🔇 Additional comments (4)
src/utils/math.ts (4)
48-58: Binary-search core looks correct and handles duplicates (upper bound) properly.The loop invariant and updates correctly implement the upper bound (last index <= target). Good use of mid calculation and boundary updates.
1-1: Import is correct and consistent with localNullableusage.No issues found.
71-71: Export looks good.Utility is properly exposed alongside existing exports.
41-46: upperBoundBinarySearch — docstring already documents the whole-collection return behaviourThe function's docstring already states that it returns 0 when all elements are greater than
upperBoundValueand the implementation has no sub-range parameters, so no change is strictly required. If you'd like an explicit sentence saying "this searches the entire collection (no sub-range parameters) and returning 0 is by design", I can add it.
- File: src/utils/math.ts — upperBoundBinarySearch docstring (above the function) — currently documents the "returns 0" behaviour.
…undBinarySearch refactor.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/utils/math.ts (3)
37-41: Type-safety: constrain to numbers or accept a comparatorUsing
<=on genericTis not type-safe. Since current usage maps to numbers (timestamps), either constrain to numbers or introduce a comparator.Option A (minimal, matches current usage):
-const upperBoundBinarySearch = <T, U>( - get: (arrayElement: U) => T, +const upperBoundBinarySearch = <U>( + get: (arrayElement: U) => number, array: U[], - upperBoundValue: T, + upperBoundValue: number, ): Nullable<number> => {Option B (generic comparator-based variant):
const upperBoundBinarySearchBy = <T, U>( get: (arrayElement: U) => T, array: U[], upperBoundValue: T, cmp: (a: T, b: T) => number, ): Nullable<number> => { if (0 === array.length) return null; let lowIdx = 0; let highIdx = array.length - 1; while (lowIdx <= highIdx) { const mid = Math.floor((lowIdx + highIdx) / 2); // eslint-disable-next-line @typescript-eslint/no-non-null-assertion if (cmp(get(array[mid]!), upperBoundValue) <= 0) lowIdx = mid + 1; else highIdx = mid - 1; } return 0 > highIdx ? 0 : highIdx; };
60-63: Fix comment casing: “upperBoundValue”Minor nit for consistency.
- // corner case: all values >= upperboundValue + // Corner case: all values >= upperBoundValue
26-36: Update JSDoc: remove obsolete “indices” wording; clarify contract and preconditionThe docstring predates the simplified API (no low/high indices) and still references “indices are invalid”. Also, parameter descriptions are sparse and the return description can be clearer.
Apply this diff to tighten the documentation:
/** - * Finds the last element in a sorted collection that is less than or equal to `upperBoundValue`. If - * all elements in the collection are greater than `upperBoundValue`, returns the first index of the - * collection (i.e., `0`). + * Finds the index of the last element in a sorted collection whose mapped value is less than or + * equal to `upperBoundValue`. If all elements in the collection are greater than `upperBoundValue`, + * returns the first index of the collection (i.e., `0`). + * + * Precondition: `array` is sorted in non-decreasing order by the value returned from `get`. * - * @param get Function to access an element (or its property) in the collection. - * @param array - * @param upperBoundValue - * @return The index of the last element less than or equal to `upperBoundValue`, `0` if all - * elements are greater, or `null` if the collection is empty or the indices are invalid. + * @param get Function that maps an element to a comparable value used for ordering. + * @param array The sorted collection to search. + * @param upperBoundValue The target value to upper-bound against. + * @return The index of the last element <= `upperBoundValue`, `0` if all elements are greater, + * or `null` if the collection is empty. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/utils/math.ts(2 hunks)test/utils/math.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
test/utils/math.test.tssrc/utils/math.ts
🧠 Learnings (2)
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
test/utils/math.test.tssrc/utils/math.ts
📚 Learning: 2025-07-24T21:28:56.838Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Applied to files:
src/utils/math.ts
🧬 Code Graph Analysis (1)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
🔇 Additional comments (2)
test/utils/math.test.ts (1)
55-113: Solid coverage and edge-case handling for upperBoundBinarySearchGood breadth of cases: empty arrays, single-element arrays, boundaries, non-matches, and duplicate-elements returning the last match. Assertions align with the intended “upper bound” semantics.
src/utils/math.ts (1)
45-66: Binary search implementation looks correct and efficientClear loop invariants, correct mid calculation, and correct return semantics for “upper bound” (last index <= target; 0 when all are greater). The non-null assertion is justified by the bounds check.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/utils/math.ts (3)
37-41: Constrain the comparable type (or accept a comparator) to make<=type-safeUsing
<=on a genericTrelies on inference and can accept unintended types. Two options:
- Minimal: constrain
Ttonumber(current usage with timestamps).- Generic-friendly: add a comparator.
Minimal change:
-const upperBoundBinarySearch = <T, U>( +const upperBoundBinarySearch = <T extends number, U>( array: U[], upperBoundValue: T, get: (arrayElement: U) => T, ): Nullable<number> => {If you want broader support, I can provide a
upperBoundBinarySearchByvariant with a comparator without breaking this API.
60-63: Fix casing in the inline comment:upperBoundValueTypo in the comment.
- // corner case: all values >= upperboundValue + // corner case: all values >= upperBoundValue
26-36: Doc precision: say “index of the last element” and call out non-decreasing orderSmall clarity tweaks:
- The function returns an index, so the lead sentence should say “Finds the index of the last element…”.
- Since duplicates are supported, “non-decreasing order” is clearer than “ascending”.
Apply this diff to the JSDoc:
- * Finds the last element in a sorted collection that is less than or equal to `upperBoundValue`. If - * all elements in the collection are greater than `upperBoundValue`, returns the first index of the - * collection (i.e., `0`). + * Finds the index of the last element in a sorted collection that is less than or equal to + * `upperBoundValue`. If all elements in the collection are greater than `upperBoundValue`, + * returns the first index of the collection (i.e., `0`). @@ - * @param array The entire collection in ascending order. + * @param array The entire collection in non-decreasing (ascending with duplicates) order.test/utils/math.test.ts (2)
59-66: Remove duplicate empty-array test and reword name (indices no longer apply)Both tests assert the same thing and reference low/high indices that the function no longer accepts. Keep a single, accurate test name.
- test("returns null if collection is empty with lowIdx < highIdx", () => { + test("returns null for empty array", () => { const result = upperBoundBinarySearch(emptyArray, 5, (arrayElement) => arrayElement); expect(result).toBeNull(); }); - test("returns null if collection is empty with highIdx < lowIdx", () => { - const result = upperBoundBinarySearch(emptyArray, 5, (arrayElement) => arrayElement); - expect(result).toBeNull(); - });
67-111: Unify casing in test descriptions: “upperBoundValue”Align test descriptions with implementation/JSDoc.
- test("returns the first index if upperboundValue < all elements in array", () => { + test("returns the first index if upperBoundValue < all elements in array", () => { @@ - test("returns the first index if upperboundValue < element in singleElementArray", () => { + test("returns the first index if upperBoundValue < element in singleElementArray", () => { @@ - test("returns the last index if upperboundValue > all elements in array", () => { + test("returns the last index if upperBoundValue > all elements in array", () => { @@ - test("returns the last index if upperboundValue > element in singleElementArray", () => { + test("returns the last index if upperBoundValue > element in singleElementArray", () => { @@ - test("returns the correct index if upperboundValue doesn't match any elements", () => { + test("returns the correct index if upperBoundValue doesn't match any elements", () => { @@ - test("returns the correct index if upperboundValue matches a unique element", () => { + test("returns the correct index if upperBoundValue matches a unique element", () => { @@ - test("returns the last matched index if upperboundValue matches a duplicated element", () => { + test("returns the last matched index if upperBoundValue matches a duplicated element", () => { @@ - test("returns index 0 if upperboundValue matches the first element", () => { + test("returns index 0 if upperBoundValue matches the first element", () => { @@ - test("returns the last index if upperboundValue matches the last element", () => { + test("returns the last index if upperBoundValue matches the last element", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/services/decoders/JsonlDecoder/index.ts(2 hunks)src/utils/math.ts(2 hunks)test/utils/math.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/utils/math.tssrc/services/decoders/JsonlDecoder/index.tstest/utils/math.test.ts
🧠 Learnings (2)
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
src/utils/math.tstest/utils/math.test.ts
📚 Learning: 2025-07-24T21:28:56.838Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#347
File: src/utils/math.ts:47-56
Timestamp: 2025-07-24T21:28:56.838Z
Learning: In the y-scope/yscope-log-viewer codebase, Henry8192 prefers not to introduce local variables to avoid parameter reassignment in binary search implementations, considering the additional constants unnecessary.
Applied to files:
src/utils/math.ts
🧬 Code Graph Analysis (3)
src/utils/math.ts (1)
src/typings/common.ts (1)
Nullable(6-6)
src/services/decoders/JsonlDecoder/index.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(71-71)
test/utils/math.test.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(71-71)
🔇 Additional comments (1)
src/utils/math.ts (1)
42-46: LGTM: explicit empty-collection handlingReturning
nullfor an empty array aligns with the JSDoc and makes the contract unambiguous.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/utils/math.test.ts (1)
63-63: Fix casing in test descriptions: use “upperBoundValue”Keep test names consistent with the API and JSDoc. This was previously suggested and appears still pending here.
- test("returns the first index if upperboundValue < all elements in array", () => { + test("returns the first index if upperBoundValue < all elements in array", () => { - test("returns the first index if upperboundValue < element in singleElementArray", () => { + test("returns the first index if upperBoundValue < element in singleElementArray", () => { - test("returns the last index if upperboundValue > all elements in array", () => { + test("returns the last index if upperBoundValue > all elements in array", () => { - test("returns the last index if upperboundValue > element in singleElementArray", () => { + test("returns the last index if upperBoundValue > element in singleElementArray", () => { - test("returns the correct index if upperboundValue doesn't match any elements", () => { + test("returns the correct index if upperBoundValue doesn't match any elements", () => { - test("returns the correct index if upperboundValue matches a unique element", () => { + test("returns the correct index if upperBoundValue matches a unique element", () => { - test("returns the last matched index if upperboundValue matches a duplicated element", () => { + test("returns the last matched index if upperBoundValue matches a duplicated element", () => { - test("returns index 0 if upperboundValue matches the first element", () => { + test("returns index 0 if upperBoundValue matches the first element", () => { - test("returns the last index if upperboundValue matches the last element", () => { + test("returns the last index if upperBoundValue matches the last element", () => {Also applies to: 67-67, 76-76, 80-80, 89-89, 93-93, 97-97, 101-101, 105-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/utils/math.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
test/utils/math.test.ts
🧠 Learnings (1)
📚 Learning: 2024-11-10T21:25:24.592Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#115
File: test/utils/math.test.ts:0-0
Timestamp: 2024-11-10T21:25:24.592Z
Learning: The `clamp` function in `src/utils/math.ts` correctly handles cases where `max` is less than `min` by returning the upper boundary.
Applied to files:
test/utils/math.test.ts
🧬 Code Graph Analysis (1)
test/utils/math.test.ts (1)
src/utils/math.ts (1)
upperBoundBinarySearch(71-71)
🔇 Additional comments (1)
test/utils/math.test.ts (1)
5-5: LGTM: Correctly importing upperBoundBinarySearchImport path and named export usage look correct.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
refactor: Extract upper-bound binary search from `findNearestLogEventByTimestamp` into a function (resolves #343).
upperboundBinarySearch and refactor timestamp log event search (resolves #343).findNearestLogEventByTimestamp into a function (resolves #343).
3 participants
Description
This pull request refactors the
findNearestLogEventByTimestampmethod in theJsonlDecoderclass by introducing a reusable utility function,upperBoundBinarySearch, to simplify and optimize the binary search logic. Additionally, it includes unit tests for the new utility function. Below is a summary of the most important changes:Refactoring and Optimization:
findNearestLogEventByTimestampcore binary search logic inJsonlDecoderwas refactored to use the newly introducedupperBoundBinarySearchutility function, replacing the inline binary search implementation. This improves code readability and reusability.Testing:
Added comprehensive unit tests for
upperBoundBinarySearchintest/utils/math.test.ts, covering edge cases such as empty arrays, single-element arrays, and arrays with duplicate values.The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.
Validation performed
npx jest test/utils/math.test.tsSummary by CodeRabbit
Refactor
New Features
Tests
Documentation