-
Notifications
You must be signed in to change notification settings - Fork 16
feat(URL): Support seeking logs by timestamp via URL parameter (resolves #117). #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5477d0a
4a8aeb7
7f5cc7f
54ef03c
6c2b45a
255f3e3
3972319
5ecdaab
f1f93f7
683350a
7b72d93
0cc3649
a81d570
e8e58f5
206b01f
07ef527
6c910d4
bc16de2
e04dadb
3c051e8
32de344
127d56b
e5725c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -491,26 +491,32 @@ class LogFileManager { | |
*/ | ||
#getCursorData (cursor: CursorType, numActiveEvents: number): CursorData { | ||
const {code, args} = cursor; | ||
switch (code) { | ||
case CURSOR_CODE.PAGE_NUM: | ||
return getPageNumCursorData( | ||
args.pageNum, | ||
args.eventPositionOnPage, | ||
numActiveEvents, | ||
this.#pageSize, | ||
); | ||
case CURSOR_CODE.LAST_EVENT: | ||
return getLastEventCursorData(numActiveEvents, this.#pageSize); | ||
case CURSOR_CODE.EVENT_NUM: | ||
return getEventNumCursorData( | ||
args.eventNum, | ||
numActiveEvents, | ||
this.#pageSize, | ||
this.#decoder.getFilteredLogEventMap(), | ||
); | ||
default: | ||
throw new Error(`Unsupported cursor type: ${code}`); | ||
|
||
let eventNum: number = 0; | ||
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; | ||
} | ||
} | ||
Comment on lines
+496
to
512
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Unhandled cursor codes & zero-event fallback can break pagination
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
|
||
|
||
return getEventNumCursorData( | ||
eventNum, | ||
numActiveEvents, | ||
this.#pageSize, | ||
this.#decoder.getFilteredLogEventMap(), | ||
); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -106,6 +106,10 @@ class ClpIrDecoder implements Decoder { | |||||||||||||||||||||||||||||||
return this.#streamReader.getFilteredLogEventMap(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
findNearestLogEventByTimestamp (timestamp: number): Nullable<number> { | ||||||||||||||||||||||||||||||||
return this.#streamReader.findNearestLogEventByTimestamp(BigInt(timestamp)); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+109
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainEnsure type-safety & handle out-of-range values in
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 🏁 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.ts Length of output: 2862 🏁 Script executed: #!/bin/bash
set -e
echo "=== Context around findNearestLogEventByTimestamp in JsonlDecoder ==="
rg -C3 -n "findNearestLogEventByTimestamp" src/services/decoders/JsonlDecoder/index.ts Length of output: 472 Validate To prevent runtime errors and inaccurate conversions, ensure the • File: 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 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
setLogLevelFilter (logLevelFilter: LogLevelFilter): boolean { | ||||||||||||||||||||||||||||||||
this.#streamReader.filterLogEvents(logLevelFilter); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -123,6 +123,32 @@ class JsonlDecoder implements Decoder { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return results; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+135
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🧰 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
low = mid + 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
high = mid - 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// corner case: all log events have timestamps >= timestamp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (0 > high) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return high; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+126
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🧰 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Parses each line from the data array and buffers it internally. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ enum SEARCH_PARAM_NAMES { | |||||||||||||||||||||
enum HASH_PARAM_NAMES { | ||||||||||||||||||||||
LOG_EVENT_NUM = "logEventNum", | ||||||||||||||||||||||
IS_PRETTIFIED = "isPrettified", | ||||||||||||||||||||||
TIMESTAMP = "timestamp", | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
interface UrlSearchParams { | ||||||||||||||||||||||
|
@@ -17,6 +18,7 @@ interface UrlSearchParams { | |||||||||||||||||||||
interface UrlHashParams { | ||||||||||||||||||||||
[HASH_PARAM_NAMES.IS_PRETTIFIED]: boolean; | ||||||||||||||||||||||
[HASH_PARAM_NAMES.LOG_EVENT_NUM]: number; | ||||||||||||||||||||||
[HASH_PARAM_NAMES.TIMESTAMP]: number; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
18
to
22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Mark it optional (or -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
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||
|
||||||||||||||||||||||
type UrlSearchParamUpdatesType = { | ||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 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:
This would ensure that timestampRef always reflects the latest timestamp value.
📝 Committable suggestion
🤖 Prompt for AI Agents