feat(URL): Support seeking logs by timestamp via URL parameter (resolves #117). #152
Conversation
WalkthroughThis change introduces support for seeking logs by timestamp via a URL parameter. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AppController
participant StateContextProvider
participant LogFileManager
participant Decoder
Browser->>AppController: URL contains timestamp param
AppController->>StateContextProvider: Provide timestamp from context
StateContextProvider->>LogFileManager: loadPageByCursor({code: TIMESTAMP, args: {timestamp}})
LogFileManager->>Decoder: findNearestLogEventByTimestamp(timestamp)
Decoder-->>LogFileManager: index of nearest log event
LogFileManager->>StateContextProvider: Return page data for event
StateContextProvider->>AppController: Update state with new page
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Henry8192
changed the title
|
@Henry8192 can we address the linter checks before we proceed further? If the ClpIrDecoder return error is the blocker, we can wait for y-scope/clp-ffi-js#42 to be merged first. |
# Conflicts: # src/typings/url.ts
…avior by returning the nearest log event index instead of invalid log event index
…rect value; Correct findNearestLogEventByTimestamp signature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/typings/decoders.ts (1)
102-119: Consider clarifying the return value for no-match cases.The documentation should explicitly state what value is returned when no matching log event is found. Based on the past review discussion, it would be helpful to document whether it returns null or -1 in such cases.
/** * Finds the log event, L, where if we assume: * * - the collection of log events is sorted in chronological order; * - and we insert a marker log event, M, with timestamp `timestamp` into the collection (if log * events with timestamp `timestamp` already exist in the collection, M should be inserted * after them). * * L is the event just before M, if M is not the first event in the collection; otherwise L is * the event just after M. * * NOTE: If the collection of log events isn't in chronological order, this method has undefined * behaviour. * + * @return The index of the log event L, or null if no matching log event is found. * - * @param timestamp - * @return The index of the log event L. + * @param timestamp The timestamp to search for. */src/services/decoders/JsonlDecoder/index.ts (1)
125-144: Consider adding input validation and improving type safety.The binary search implementation is efficient, but could benefit from some improvements:
- Add validation for negative timestamps
- Extract the log event access into a separate constant to improve readability and type safety
Apply this diff to improve the implementation:
findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { + if (timestamp < 0) { + return null; + } let low = 0; let high = this.#logEvents.length - 1; if (high < low) { return null; } while (low <= high) { const mid = Math.floor((low + high) / 2); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); + const midLogEvent = this.#logEvents[mid]; + const midTimestamp = midLogEvent.timestamp.valueOf(); if (midTimestamp <= timestamp) { low = mid + 1; } else { high = mid - 1; } } return high; }src/contexts/UrlContextProvider.tsx (1)
208-219: Consider improving readability of the parameter array declaration.The array declaration could be more readable by using proper line breaks and alignment.
Apply this diff to improve readability:
- const numberHashParamNames = [HASH_PARAM_NAMES.LOG_EVENT_NUM, - HASH_PARAM_NAMES.TIMESTAMP]; + const numberHashParamNames = [ + HASH_PARAM_NAMES.LOG_EVENT_NUM, + HASH_PARAM_NAMES.TIMESTAMP, + ];src/services/LogFileManager/index.ts (1)
434-462: Consider adding validation and improving error handling.The cursor data handling for timestamps could benefit from some improvements:
- Add validation for negative timestamps
- Consider adding error handling for the case when no event is found
Apply this diff to improve the implementation:
} else { + if (args.timestamp < 0) { + throw new Error("Timestamp cannot be negative"); + } const eventIdx = this.#decoder.findNearestLogEventByTimestamp(args.timestamp); if (null !== eventIdx) { eventNum = eventIdx + 1; + } else { + throw new Error("No log event found for the given timestamp"); } }src/contexts/StateContextProvider.tsx (1)
488-498: Consider adding error handling for invalid timestamps.The timestamp effect should handle invalid timestamp values gracefully.
Apply this diff to improve error handling:
if (null === mainWorkerRef.current || null === timestamp) { return; } + if (timestamp < 0) { + console.error("Invalid timestamp value:", timestamp); + updateWindowUrlHashParams({timestamp: null}); + return; + } + loadPageByCursor(mainWorkerRef.current, { code: CURSOR_CODE.TIMESTAMP, args: {timestamp: timestamp}, }); updateWindowUrlHashParams({timestamp: null});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.json(1 hunks)src/contexts/StateContextProvider.tsx(5 hunks)src/contexts/UrlContextProvider.tsx(2 hunks)src/services/LogFileManager/index.ts(1 hunks)src/services/decoders/ClpIrDecoder.ts(1 hunks)src/services/decoders/JsonlDecoder/index.ts(3 hunks)src/typings/decoders.ts(1 hunks)src/typings/logs.ts(1 hunks)src/typings/url.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/typings/logs.tssrc/services/decoders/ClpIrDecoder.tssrc/typings/url.tssrc/typings/decoders.tssrc/contexts/UrlContextProvider.tsxsrc/services/decoders/JsonlDecoder/index.tssrc/services/LogFileManager/index.tssrc/contexts/StateContextProvider.tsx
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/typings/logs.tssrc/services/decoders/ClpIrDecoder.tssrc/typings/url.tssrc/typings/decoders.tssrc/contexts/UrlContextProvider.tsxsrc/services/decoders/JsonlDecoder/index.tssrc/services/LogFileManager/index.tssrc/contexts/StateContextProvider.tsx🔇 Additional comments (10)
src/typings/url.ts (1)
9-10: LGTM! The changes align well with the PR objectives.The addition of the
TIMESTAMPenum value and thetimestampproperty inUrlHashParamsinterface provides proper typing support for querying log events by timestamp via URL hash parameters.Also applies to: 18-19
src/typings/logs.ts (1)
46-46: LGTM! The constant is properly defined using BigInt.The use of BigInt for
INVALID_TIMESTAMP_VALUEaligns with the timestamp handling in the codebase, particularly with theDecodeResulttype that uses BigInt for timestamps.src/typings/decoders.ts (1)
102-119: LGTM! The method is well-documented and properly typed.The method signature and documentation provide clear guidance on the chronological order requirement and the marker log event concept.
src/services/decoders/ClpIrDecoder.ts (1)
80-82: LGTM! The implementation is clean and handles type conversion properly.The method correctly converts the number timestamp to BigInt before passing it to the stream reader, maintaining consistency with the underlying implementation.
src/services/decoders/JsonlDecoder/index.ts (1)
238-256: LGTM! Timestamp type change improves precision.The change from
numbertobigintfor timestamp handling is a good improvement as it prevents potential precision loss when dealing with large timestamp values.src/contexts/UrlContextProvider.tsx (1)
32-35: LGTM! Default state properly initialized.The addition of
HASH_PARAM_NAMES.TIMESTAMPtoURL_HASH_PARAMS_DEFAULTwith a null default value is consistent with the existing pattern.src/contexts/StateContextProvider.tsx (3)
250-276: LGTM! Proper state management setup.The addition of timestamp handling in the context and ref setup follows the established patterns.
464-468: LGTM! Consistent ref synchronization.The useEffect hook for timestamp synchronization follows the same pattern as other ref synchronizations.
541-546: LGTM! Consistent cursor handling.The addition of timestamp cursor handling in the file loading logic is consistent with the existing event number cursor handling.
package.json (1)
33-33: Dependency Version Bump for clp-ffi-js
The dependency version forclp-ffi-jshas been updated from^0.3.4to^0.3.5. This change supports the new timestamp handling features as outlined in the PR objectives. Please ensure that all modules relying on this dependency are tested against this updated version for compatibility.
|
Just woke up for a bug in find nearst log event by timestamp before I go to sleep again: if all log events' timestamps are larger than target timestamp, return high would overflow. Need to check that condition before the return. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/decoders/JsonlDecoder/index.ts (1)
134-135: 🛠️ Refactor suggestionExtract variable to preserve type-checking.
The non-null assertion disables type-checking for the entire line. Let's extract the log event into a variable first.
Apply this diff to preserve type-checking:
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); + const midLogEvent = this.#logEvents[mid]; + const midTimestamp = midLogEvent.timestamp.valueOf();
🧹 Nitpick comments (2)
src/services/decoders/JsonlDecoder/index.ts (2)
144-146: Define constant for magic number and improve corner case handling.The magic number
0is used for both comparison and return value. Let's define a constant and improve the corner case handling.Apply this diff to improve readability and maintainability:
+ static readonly #FIRST_INDEX = 0; + findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { - let low = 0; + let low = JsonlDecoder.#FIRST_INDEX; let high = this.#logEvents.length - 1; if (high < low) { return null; } // ... binary search implementation ... // corner case: all log events have timestamps >= timestamp - if (0 > high) { - return 0; + if (JsonlDecoder.#FIRST_INDEX > high) { + return JsonlDecoder.#FIRST_INDEX; }
128-130: Align with coding guidelines by usingfalse == <expression>.Per coding guidelines, prefer
false == <expression>over!<expression>.Apply this diff to align with coding guidelines:
- if (high < low) { + if (false == (high >= low)) { return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/decoders/JsonlDecoder/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/JsonlDecoder/index.ts
🔇 Additional comments (1)
src/services/decoders/JsonlDecoder/index.ts (1)
243-243: LGTM! Consistent timestamp type handling.The changes ensure consistent handling of timestamps using
biginttype, which aligns with the PR objectives.Also applies to: 260-260
Henry8192
changed the title
# Conflicts: # package-lock.json # package.json
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/decoders/JsonlDecoder/index.ts (1)
140-140: Consider extracting the log event to a local variable.Instead of using a non-null assertion with an eslint-disable comment, you could extract the log event to a local variable with a type assertion for better readability.
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); + const midLogEvent = this.#logEvents[mid] as LogEvent; + const midTimestamp = midLogEvent.timestamp.valueOf();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/decoders/ClpIrDecoder/index.ts(1 hunks)src/services/decoders/JsonlDecoder/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/ClpIrDecoder/index.tssrc/services/decoders/JsonlDecoder/index.ts
🔇 Additional comments (4)
src/services/decoders/JsonlDecoder/index.ts (3)
130-154: The binary search implementation looks good.The implementation correctly finds the nearest log event with a timestamp less than or equal to the target. The corner case handling for when all timestamps are greater than the target is properly addressed.
149-151: Edge case handling is correct.Good job handling the edge case where all log events have timestamps greater than or equal to the target timestamp. This prevents potential issues when no events with smaller timestamps exist.
254-254: Appropriate type change from number to bigint.Changing the timestamp type from number to bigint is a good improvement that allows handling larger timestamp values without precision loss.
Also applies to: 271-271
src/services/decoders/ClpIrDecoder/index.ts (1)
109-111:✅ Verification successful
Implementation correctly delegates to the stream reader.
The method properly converts the number timestamp to BigInt before delegating to the stream reader's implementation. This ensures consistent timestamp handling across different decoder implementations.
🏁 Script executed:
#!/bin/bash # Verify if the clp-ffi-js implementation handles edge cases correctly # Check for edge case handling in clp-ffi-js implementation rg -A 10 "findNearestLogEventByTimestamp" --type tsLength of output: 3971
Approval: Timestamp conversion and delegation confirmed.
The implementation correctly converts the number timestamp to BigInt and delegates this value to the stream reader, ensuring consistent handling across decoders. The repository search confirms that similar practices are employed elsewhere in the codebase (e.g. in the JsonlDecoder), and no edge case issues have been identified.
…ll add a state management tool and shrink the lines.
# Conflicts: # src/contexts/StateContextProvider.tsx # src/typings/url.ts
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/contexts/UrlContextProvider.tsx(2 hunks)src/services/LogFileManager/index.ts(1 hunks)src/services/decoders/ClpIrDecoder/index.ts(1 hunks)src/services/decoders/JsonlDecoder/index.ts(3 hunks)src/typings/url.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/JsonlDecoder/index.tssrc/typings/url.tssrc/services/decoders/ClpIrDecoder/index.tssrc/services/LogFileManager/index.tssrc/contexts/UrlContextProvider.tsx
🧬 Code Graph Analysis (2)
src/services/LogFileManager/index.ts (2)
src/typings/worker.ts (1)
CURSOR_CODE(100-100)src/services/LogFileManager/utils.ts (3)
getPageNumCursorData(157-157)getLastEventCursorData(156-156)getEventNumCursorData(155-155)
src/contexts/UrlContextProvider.tsx (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
🪛 Biome (1.9.4)
src/services/decoders/JsonlDecoder/index.ts
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/services/LogFileManager/index.ts
[error] 495-495: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
🔇 Additional comments (5)
src/services/LogFileManager/index.ts (1)
495-495: Remove redundant type annotation flagged by Biome
eventNum’s type is obvious from its initialiser. Dropping the annotation fixes thenoInferrableTypeslinter error.- let eventNum: number = 0; + let eventNum = 0;(This is a small lint fix; no functional impact.)
🧰 Tools
🪛 Biome (1.9.4)
[error] 495-495: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/contexts/UrlContextProvider.tsx (2)
32-35: LGTM: New timestamp parameter added.The addition of the
TIMESTAMPparameter to the default hash parameters is consistent with the feature being implemented, enabling log seeking by timestamp.
214-227: Improved code organization for number parameter handling.Refactoring the number parameter parsing into a loop over an array of parameter names is a cleaner approach that makes the code more maintainable and extensible for future parameters.
The implementation matches the suggestion from the previous review while maintaining the coding guidelines of preferring
null !== hashParamover!hashParam.src/services/decoders/JsonlDecoder/index.ts (2)
250-251: LGTM: Type change to bigint is appropriate.Changing the timestamp type from
numbertobigintaligns with the timestamp handling changes throughout the codebase.
267-268: LGTM: Proper conversion to BigInt.The conversion of the timestamp to BigInt is correctly implemented, ensuring consistency with the return type of the method.
| findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | ||
| return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure type-safety & handle out-of-range values in findNearestLogEventByTimestamp
-
The public API now only accepts a
number, but you immediately cast it toBigInt.
a. Callers could mistakenly pass anumberthat is not an integer or exceedsNumber.MAX_SAFE_INTEGER, producing an inaccurateBigIntor runtime throw.
b. Upstream (Decoderinterface) may already expose the parameter asbigint; keeping the converter here silently widens the contract. -
No validation is performed for negative or
NaNtimestamps—BigInt(NaN)will throw at runtime.
A small defensive wrapper prevents both issues:
findNearestLogEventByTimestamp (timestamp: number): Nullable<number> {
- return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp));
+ if (!Number.isFinite(timestamp) || timestamp < 0 || !Number.isInteger(timestamp)) {
+ return null;
+ }
+ return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp));
}Optionally, change the signature to bigint to avoid repeated casting at every call-site.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== All occurrences of findNearestLogEventByTimestamp in src ==="
rg "findNearestLogEventByTimestamp" -n src
echo -e "\n=== Context around #streamReader in ClpIrDecoder ==="
rg -C3 -n "#streamReader" src/services/decoders/ClpIrDecoder/index.tsLength of output: 2862
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Context around findNearestLogEventByTimestamp in JsonlDecoder ==="
rg -C3 -n "findNearestLogEventByTimestamp" src/services/decoders/JsonlDecoder/index.tsLength of output: 472
Validate timestamp before casting to BigInt in ClpIrDecoder.findNearestLogEventByTimestamp
To prevent runtime errors and inaccurate conversions, ensure the timestamp is a non-negative, finite integer within JavaScript’s safe range before calling the FFI:
• File: src/services/decoders/ClpIrDecoder/index.ts
• Lines: 109–111
Proposed change:
findNearestLogEventByTimestamp(timestamp: number): Nullable<number> {
- return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp));
+ // Reject NaN, non-integers, negatives or values beyond Number.MAX_SAFE_INTEGER
+ if (
+ !Number.isFinite(timestamp) ||
+ !Number.isInteger(timestamp) ||
+ timestamp < 0 ||
+ timestamp > Number.MAX_SAFE_INTEGER
+ ) {
+ return null;
+ }
+ return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp));
}• This guards against BigInt(NaN)/BigInt(1.5)/overflow errors.
• The Decoder interface currently uses number, so we retain that signature here.
• Optionally, once all implementations can support it, you may revisit the interface to accept bigint directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | |
| return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); | |
| } | |
| findNearestLogEventByTimestamp(timestamp: number): Nullable<number> { | |
| // Reject NaN, non-integers, negatives or values beyond Number.MAX_SAFE_INTEGER | |
| if ( | |
| !Number.isFinite(timestamp) || | |
| !Number.isInteger(timestamp) || | |
| timestamp < 0 || | |
| timestamp > Number.MAX_SAFE_INTEGER | |
| ) { | |
| return null; | |
| } | |
| return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); | |
| } |
🤖 Prompt for AI Agents
In src/services/decoders/ClpIrDecoder/index.ts at lines 109 to 111, the method
findNearestLogEventByTimestamp currently casts the input number to BigInt
without validation, which can cause runtime errors or inaccurate conversions for
non-integer, negative, NaN, or out-of-range values. Fix this by adding
validation to ensure the timestamp is a non-negative, finite integer within the
safe integer range before casting to BigInt. If the validation fails, handle the
error appropriately or return null. Retain the method signature using number as
per the Decoder interface.
| interface UrlHashParams { | ||
| [HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean; | ||
| [HASH_PARAM_NAMES.LOG_EVENT_NUM]: number; | ||
| [HASH_PARAM_NAMES.TIMESTAMP]: number; | ||
| } |
There was a problem hiding this comment.
timestamp should be optional in UrlHashParams
UrlHashParams now requires TIMESTAMP, making every consumer supply a value even when no timestamp cursor is used. This breaks existing code that only cares about logEventNum or isPrettified.
Mark it optional (or Nullable) just like the update helpers already expect:
-interface UrlHashParams {
- [HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean;
- [HASH_PARAM_NAMES.LOG_EVENT_NUM]: number;
- [HASH_PARAM_NAMES.TIMESTAMP]: number;
-}
+interface UrlHashParams {
+ [HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean;
+ [HASH_PARAM_NAMES.LOG_EVENT_NUM]: number;
+ [HASH_PARAM_NAMES.TIMESTAMP]?: number; // optional
+}This keeps type-checks happy and matches real-world URL usage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface UrlHashParams { | |
| [HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean; | |
| [HASH_PARAM_NAMES.LOG_EVENT_NUM]: number; | |
| [HASH_PARAM_NAMES.TIMESTAMP]: number; | |
| } | |
| interface UrlHashParams { | |
| [HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean; | |
| [HASH_PARAM_NAMES.LOG_EVENT_NUM]: number; | |
| [HASH_PARAM_NAMES.TIMESTAMP]?: number; // optional | |
| } |
🤖 Prompt for AI Agents
In src/typings/url.ts around lines 18 to 22, the TIMESTAMP property in the
UrlHashParams interface is currently required, which forces all consumers to
provide a timestamp even when it's not used. To fix this, mark the TIMESTAMP
property as optional by adding a question mark after the key, making it
consistent with how update helpers expect it and allowing consumers to omit it
when unnecessary.
| if (CURSOR_CODE.PAGE_NUM === code) { | ||
| return getPageNumCursorData( | ||
| args.pageNum, | ||
| args.eventPositionOnPage, | ||
| numActiveEvents, | ||
| this.#pageSize, | ||
| ); | ||
| } else if (CURSOR_CODE.LAST_EVENT === code) { | ||
| return getLastEventCursorData(numActiveEvents, this.#pageSize); | ||
| } else if (CURSOR_CODE.EVENT_NUM === code) { | ||
| ({eventNum} = args); | ||
| } else { | ||
| const eventIdx = this.#decoder.findNearestLogEventByTimestamp(args.timestamp); | ||
| if (null !== eventIdx) { | ||
| eventNum = eventIdx + 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unhandled cursor codes & zero-event fallback can break pagination
-
The
elsebranch executes whenevercode≠PAGE_NUM | LAST_EVENT | EVENT_NUM, yet you unconditionally readargs.timestamp.
• If an entirely different cursor code is introduced later, this will crash withargs.timestamp is undefined.
• Make the branch explicit (CURSOR_CODE.TIMESTAMP) and throw for truly unknown codes. -
When no event is found for the timestamp,
eventNumstays0.
•getEventNumCursorDataexpects event numbers ≥ 1; passing0will yield incorrect page offsets or throw.
Suggested robust handling:
- } else {
- const eventIdx = this.#decoder.findNearestLogEventByTimestamp(args.timestamp);
- if (null !== eventIdx) {
- eventNum = eventIdx + 1;
- }
+ } else if (CURSOR_CODE.TIMESTAMP === code) {
+ const eventIdx = this.#decoder.findNearestLogEventByTimestamp(args.timestamp);
+ if (null === eventIdx) {
+ // Fallback: first event
+ return getPageNumCursorData(1, 0, numActiveEvents, this.#pageSize);
+ }
+ eventNum = eventIdx + 1;
+ } else {
+ throw new Error(`Unsupported cursor code: ${code as string}`);
}This removes undefined behaviour, restores the original “fail-fast” philosophy, and clarifies intent.
🤖 Prompt for AI Agents
In src/services/LogFileManager/index.ts around lines 496 to 512, the else branch
currently assumes any unknown cursor code means a timestamp lookup, which can
cause crashes if args.timestamp is undefined for new cursor codes. Refactor this
by explicitly checking if code equals CURSOR_CODE.TIMESTAMP before accessing
args.timestamp, and throw an error for any truly unknown cursor codes to fail
fast. Additionally, handle the case where no event is found for the timestamp by
throwing or returning early instead of allowing eventNum to remain 0, since
getEventNumCursorData requires eventNum ≥ 1 to avoid incorrect pagination.
| findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | ||
| let low = 0; | ||
| let high = this.#logEvents.length - 1; | ||
| if (high < low) { | ||
| return null; | ||
| } | ||
|
|
||
| while (low <= high) { | ||
| const mid = Math.floor((low + high) / 2); | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); | ||
| if (midTimestamp <= timestamp) { | ||
| low = mid + 1; | ||
| } else { | ||
| high = mid - 1; | ||
| } | ||
| } | ||
|
|
||
| // corner case: all log events have timestamps >= timestamp | ||
| if (0 > high) { | ||
| return 0; | ||
| } | ||
|
|
||
| return high; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Binary search implementation looks correct, but could be optimized.
The method correctly implements a binary search to find the nearest log event by timestamp, handling important edge cases like empty logs and when all timestamps are greater than the target.
Could be slightly optimized by reducing duplicate bounds checks:
findNearestLogEventByTimestamp (timestamp: number): Nullable<number> {
let low = 0;
let high = this.#logEvents.length - 1;
if (high < low) {
return null;
}
while (low <= high) {
const mid = Math.floor((low + high) / 2);
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
- const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf();
+ // We've checked bounds so this array access is safe
+ const midLogEvent = this.#logEvents[mid];
+ const midTimestamp = midLogEvent.timestamp.valueOf();
if (midTimestamp <= timestamp) {
low = mid + 1;
} else {
high = mid - 1;
}
}
// corner case: all log events have timestamps >= timestamp
if (0 > high) {
return 0;
}
return high;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | |
| let low = 0; | |
| let high = this.#logEvents.length - 1; | |
| if (high < low) { | |
| return null; | |
| } | |
| while (low <= high) { | |
| const mid = Math.floor((low + high) / 2); | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); | |
| if (midTimestamp <= timestamp) { | |
| low = mid + 1; | |
| } else { | |
| high = mid - 1; | |
| } | |
| } | |
| // corner case: all log events have timestamps >= timestamp | |
| if (0 > high) { | |
| return 0; | |
| } | |
| return high; | |
| } | |
| findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | |
| let low = 0; | |
| let high = this.#logEvents.length - 1; | |
| if (high < low) { | |
| return null; | |
| } | |
| while (low <= high) { | |
| const mid = Math.floor((low + high) / 2); | |
| // We've checked bounds so this array access is safe | |
| const midLogEvent = this.#logEvents[mid]; | |
| const midTimestamp = midLogEvent.timestamp.valueOf(); | |
| if (midTimestamp <= timestamp) { | |
| low = mid + 1; | |
| } else { | |
| high = mid - 1; | |
| } | |
| } | |
| // corner case: all log events have timestamps >= timestamp | |
| if (0 > high) { | |
| return 0; | |
| } | |
| return high; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents
In src/services/decoders/JsonlDecoder/index.ts around lines 126 to 150, optimize
the binary search by reducing duplicate boundary checks inside the loop.
Refactor the while loop conditions and updates to minimize redundant comparisons
of low and high bounds, ensuring the logic still correctly finds the nearest log
event by timestamp while improving efficiency.
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); | ||
| if (midTimestamp <= timestamp) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Address the non-null assertion warning.
The non-null assertion is flagged by the static analysis tool. While the current implementation includes a comment to suppress the warning, it would be better to refactor to avoid the non-null assertion entirely.
const mid = Math.floor((low + high) / 2);
-// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf();
+// We've already checked bounds, so this access is safe
+const midLogEvent = this.#logEvents[mid];
+const midTimestamp = midLogEvent.timestamp.valueOf();This approach is cleaner than using a non-null assertion and makes the code's safety more explicit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); | |
| if (midTimestamp <= timestamp) { | |
| const mid = Math.floor((low + high) / 2); | |
| // We've already checked bounds, so this access is safe | |
| const midLogEvent = this.#logEvents[mid]; | |
| const midTimestamp = midLogEvent.timestamp.valueOf(); | |
| if (midTimestamp <= timestamp) { | |
| // … | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents
In src/services/decoders/JsonlDecoder/index.ts around lines 135 to 137, avoid
using the non-null assertion operator on this.#logEvents[mid]. Instead, add an
explicit check to ensure this.#logEvents[mid] is not null or undefined before
accessing its timestamp property. This makes the code safer and removes the need
for the eslint-disable comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/decoders/ClpIrDecoder/index.ts (1)
109-111: Validatetimestampbefore casting to BigIntThe method currently casts the input
timestamptoBigIntwithout any validation, which can throw runtime errors if the input is invalid (e.g., NaN, non-integers, negatives, or values beyond JavaScript's safe integer range).Adding validation would make this method more robust:
findNearestLogEventByTimestamp(timestamp: number): Nullable<number> { - return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); + // Reject NaN, non-integers, negatives or values beyond Number.MAX_SAFE_INTEGER + if ( + !Number.isFinite(timestamp) || + !Number.isInteger(timestamp) || + timestamp < 0 || + timestamp > Number.MAX_SAFE_INTEGER + ) { + return null; + } + return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); }src/services/decoders/JsonlDecoder/index.ts (1)
135-137: Address the non-null assertion warningThe non-null assertion (!) usage is flagged by static analysis. While you've added a comment to disable the linter, it would be better to refactor to avoid the non-null assertion entirely.
const mid = Math.floor((low + high) / 2); -// eslint-disable-next-line @typescript-eslint/no-non-null-assertion -const midTimestamp = this.#logEvents[mid]!.timestamp.valueOf(); +// We've already checked bounds, so this access is safe +const midLogEvent = this.#logEvents[mid]; +const midTimestamp = midLogEvent.timestamp.valueOf();This approach is cleaner than using a non-null assertion and makes the code's safety more explicit.
🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/components/AppController.tsx(4 hunks)src/services/decoders/ClpIrDecoder/index.ts(1 hunks)src/services/decoders/JsonlDecoder/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/ClpIrDecoder/index.tssrc/services/decoders/JsonlDecoder/index.tssrc/components/AppController.tsx
🧬 Code Graph Analysis (1)
src/services/decoders/JsonlDecoder/index.ts (1)
src/typings/common.ts (1)
Nullable(10-10)
🪛 Biome (1.9.4)
src/services/decoders/JsonlDecoder/index.ts
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🔇 Additional comments (4)
src/services/decoders/JsonlDecoder/index.ts (1)
126-150: Binary search implementation looks correctThe method correctly implements binary search to find the nearest log event by timestamp, handling edge cases like empty logs array and when all timestamps are greater than the target.
🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/components/AppController.tsx (3)
85-85: LGTM: Added timestamp to URL context extractionCorrectly extracting timestamp from the UrlContext for use in the component.
101-101: LGTM: Added timestampRefAppropriate use of useRef to maintain reference to the timestamp value across renders.
210-215: LGTM: Added timestamp-based cursor handlingThe code correctly overrides the cursor type to use timestamp when available, giving it precedence over event number navigation.
| useEffect(() => { | ||
| if (null === timestamp) { | ||
| return; | ||
| } | ||
|
|
||
| (async () => { | ||
| const cursor: CursorType = { | ||
| code: CURSOR_CODE.TIMESTAMP, | ||
| args: {timestamp: timestamp}, | ||
| }; | ||
|
|
||
| const pageData = await logFileManagerProxy.loadPage(cursor, isPrettifiedRef.current); | ||
| updatePageData(pageData); | ||
| })().catch((e: unknown) => { | ||
| console.error(e); | ||
| postPopUp({ | ||
| level: LOG_LEVEL.ERROR, | ||
| message: String(e), | ||
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | ||
| title: "Action failed", | ||
| }); | ||
| }); | ||
| updateWindowUrlHashParams({timestamp: null}); | ||
| }, [ | ||
| logFileManagerProxy, | ||
| postPopUp, | ||
| updatePageData, | ||
| timestamp, | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement timestamp tracking similar to logEventNum
The effect correctly handles timestamp changes by loading the corresponding page and clearing the timestamp from URL. However, unlike logEventNum, there's no code to update timestampRef when timestamp changes.
Consider adding an effect to synchronize timestampRef with timestamp, similar to how logEventNumRef is synchronized:
+// Synchronize `timestampRef` with `timestamp`.
+useEffect(() => {
+ timestampRef.current = timestamp;
+}, [timestamp]);This would ensure that timestampRef always reflects the latest timestamp value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (null === timestamp) { | |
| return; | |
| } | |
| (async () => { | |
| const cursor: CursorType = { | |
| code: CURSOR_CODE.TIMESTAMP, | |
| args: {timestamp: timestamp}, | |
| }; | |
| const pageData = await logFileManagerProxy.loadPage(cursor, isPrettifiedRef.current); | |
| updatePageData(pageData); | |
| })().catch((e: unknown) => { | |
| console.error(e); | |
| postPopUp({ | |
| level: LOG_LEVEL.ERROR, | |
| message: String(e), | |
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | |
| title: "Action failed", | |
| }); | |
| }); | |
| updateWindowUrlHashParams({timestamp: null}); | |
| }, [ | |
| logFileManagerProxy, | |
| postPopUp, | |
| updatePageData, | |
| timestamp, | |
| ]); | |
| useEffect(() => { | |
| if (null === timestamp) { | |
| return; | |
| } | |
| (async () => { | |
| const cursor: CursorType = { | |
| code: CURSOR_CODE.TIMESTAMP, | |
| args: {timestamp: timestamp}, | |
| }; | |
| const pageData = await logFileManagerProxy.loadPage(cursor, isPrettifiedRef.current); | |
| updatePageData(pageData); | |
| })().catch((e: unknown) => { | |
| console.error(e); | |
| postPopUp({ | |
| level: LOG_LEVEL.ERROR, | |
| message: String(e), | |
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | |
| title: "Action failed", | |
| }); | |
| }); | |
| updateWindowUrlHashParams({timestamp: null}); | |
| }, [ | |
| logFileManagerProxy, | |
| postPopUp, | |
| updatePageData, | |
| timestamp, | |
| ]); | |
| // Synchronize `timestampRef` with `timestamp`. | |
| useEffect(() => { | |
| timestampRef.current = timestamp; | |
| }, [timestamp]); |
🤖 Prompt for AI Agents
In src/components/AppController.tsx around lines 124 to 152, the useEffect
handles timestamp changes but does not update timestampRef to reflect the latest
timestamp value. To fix this, add a separate useEffect that listens to changes
in timestamp and updates timestampRef.current accordingly, similar to how
logEventNumRef is synchronized. This ensures timestampRef always holds the
current timestamp value.
Description
Closes #117.
Implements JSONL & CLPIR
findNearestLogEventByTimestampfeature:src/services/decoders/JsonlDecoder/index.ts.findNearestLogEventByTimestampAPI is here. We use this API insrc/services/decoders/ClpIrDecoder.ts.Here's an example of searching log events with timestamp Mar 28 2023 04:01:18.594 GMT+0000:
localhost:3010/?filePath=http://localhost:3010/test/example.jsonl#timestamp=1679976078594Here, the hash parameter
#timestamp=1679976078594is the unix timestamp of our target in miliseconds. This would trigger a search in log events with timestamp closest to1679976078594. After the search is done, it would jump to that log event, and clear the#timestampparameter in the URL.Validation performed
Performed search with timestamp of
1679976078593, 1679976078594, 1679976078595 and 1679976078596with example.jsonl.zip (please unzip it).Summary by CodeRabbit