Apply suggestions from code review

Co-authored-by: kirkrodrigues <[email protected]>

new-log-viewer/src/services/LogFileManager.ts

@@ -214,17 +214,16 @@ class LogFileManager {
     }
 
     /**
-     * Changes the current log level filter and updates the page boundaries.
+     * Sets the log level filter.
      *
-     * @param logLevelFilter Array of selected log levels
-     * @throws {Error} If changing the log level filter is not successful
+     * @param logLevelFilter
+     * @throws {Error} If changing the log level filter couldn't be set.
      */
     changeLogLevelFilter (logLevelFilter: LogLevelFilter) {
         const result: boolean = this.#decoder.changeLogLevelFilter(logLevelFilter);
 
         if (false === result) {
-            throw new Error(`Error changing log level filter as feature not yet
-                implemented for this file type on new log viewer`);
+            throw new Error(`Failed to set log level filter for current decoder.`);
         }
 
         if (logLevelFilter) {
@@ -235,9 +234,8 @@ class LogFileManager {
     }
 
     /**
-     * Computes logEventNum page boundaries based on current filter. Sets two arrays of page
-     * boundaries. The first array contains the number of first log event on each page. The
-     * second array contains the number last log event on each page.
+     * Computes the log event number at the beginning and end of each page, accounting for the level
+     * filter.
      */
     #computeFilteredPageBoundaries () {
         this.#firstLogEventNumOnPage.length = 0;
@@ -250,21 +248,15 @@ class LogFileManager {
             const firstLogEventOnPageIdx: number = filteredLogEventsIndices[i] as number;
             this.#firstLogEventNumOnPage.push(1 + firstLogEventOnPageIdx);
 
-            // Need to minus one from page size to get correct index into filtered log events.
-            let lastPageIdx: number = i + this.#pageSize - 1;
-
-            // Guard to prevent indexing out of array on last page.
-            if (lastPageIdx >= this.#numFilteredEvents) {
-                lastPageIdx = this.#numFilteredEvents - 1;
-            }
-
-            const lastLogEventOnPageIdx: number = filteredLogEventsIndices[lastPageIdx] as number;
+            const j = Math.min(i + this.#pageSize - 1, this.#numFilteredEvents - 1);
+            const lastLogEventOnPageIdx: number = filteredLogEventsIndices[j] as number;
             this.#lastLogEventNumOnPage.push(1 + lastLogEventOnPageIdx);
         }
     }
 
     /**
-     * Computes logEventNum page boundaries with assuming no filter.
+     * Computes the log event number at the beginning and end of each page, assuming the events
+     * aren't filtered.
      */
     #computeUnfilteredPageBoundaries () {
         this.#firstLogEventNumOnPage.length = 0;

new-log-viewer/src/services/MainWorker.ts

@@ -79,8 +79,8 @@ onmessage = async (ev: MessageEvent<MainWorkerReqMessage>) => {
                 if (null === LOG_FILE_MANAGER) {
                     throw new Error("Log file manager hasn't been initialized");
                 }
-                LOG_FILE_MANAGER.changeLogLevelFilter(args.logLevelFilter);
 
+                LOG_FILE_MANAGER.changeLogLevelFilter(args.logLevelFilter);
                 postResp(WORKER_RESP_CODE.VIEW_INFO, {
                     numFilteredEvents: LOG_FILE_MANAGER.numFilteredEvents,
                     firstLogEventNumPerPage: LOG_FILE_MANAGER.firstLogEventNumPerPage,

new-log-viewer/src/services/decoders/ClpIrDecoder.ts

@@ -34,7 +34,7 @@ class ClpIrDecoder implements Decoder {
 
     getFilteredLogEvents (): number[] {
         // eslint-disable-next-line no-warning-comments
-        // TODO: fix this after log level filtering is implemented in clp-ffi-js
+        // TODO: Update this after log level filtering is implemented in clp-ffi-js
         return Array.from({length: this.#streamReader.getNumEventsBuffered()}, (_, index) => index);
     }
 
@@ -48,7 +48,7 @@ class ClpIrDecoder implements Decoder {
     // eslint-disable-next-line @typescript-eslint/no-unused-vars, class-methods-use-this
     changeLogLevelFilter (logLevelFilter: LogLevelFilter): boolean {
         // eslint-disable-next-line no-warning-comments
-        // TODO fix this after log level filtering is implemented in clp-ffi-js
+        // TODO: Update this after log level filtering is implemented in clp-ffi-js
         return false;
     }
 
@@ -57,6 +57,7 @@ class ClpIrDecoder implements Decoder {
     }
 
     decodeFilteredRange (beginIdx: number, endIdx: number): Nullable<DecodeResultType[]> {
+        // TODO: Update after log level filtering is implemented in clp-ffi-js
         return this.#streamReader.decodeRange(beginIdx, endIdx);
     }
 }

new-log-viewer/src/services/decoders/JsonlDecoder.ts

@@ -69,7 +69,7 @@ class JsonlDecoder implements Decoder {
 
     buildIdx (beginIdx: number, endIdx: number): Nullable<LogEventCount> {
         if (0 !== beginIdx || endIdx !== LOG_EVENT_FILE_END_IDX) {
-            throw new Error("Partial range deserialization is not yet supported.");
+            throw new Error("Partial range deserialization is currently unsupported.");
         }
 
         this.#deserialize();
@@ -101,12 +101,12 @@ class JsonlDecoder implements Decoder {
     }
 
     /**
-     * Decodes JSON log events from the filtered log events array or unfiltered
-     * based on the value of useFilter.
+     * Decodes log events from the filtered or unfiltered log events array based on the value of
+     * `useFilter`.
      *
      * @param beginIdx
      * @param endIdx
-     * @param useFilter Whether to use filtered or unfiltered log event array
+     * @param useFilter Whether to decode from the filtered or unfiltered log events array.
      * @return The decoded log events on success or null if any log event in the range doesn't exist
      * (e.g., the range exceeds the number of log events in the file).
      */
@@ -144,7 +144,7 @@ class JsonlDecoder implements Decoder {
                 message = `${this.#invalidLogEventIdxToRawLine.get(filteredIdx)}\n`;
                 logLevel = LOG_LEVEL.NONE;
             } else {
-                // Explicit cast since typescript thinks `#logEvents[logEventIdx]` can be undefined,
+                // Explicit cast since typescript thinks `#logEvents[filteredIdx]` can be undefined,
                 // but it shouldn't be since the index comes from a class-internal filter.
                 const logEvent: JsonLogEvent = this.#logEvents[filteredIdx] as JsonLogEvent;
 
@@ -173,6 +173,7 @@ class JsonlDecoder implements Decoder {
         if (null === this.#dataArray) {
             return;
         }
+
         const text = JsonlDecoder.#textDecoder.decode(this.#dataArray);
         let beginIdx = 0;
         while (beginIdx < text.length) {
@@ -214,15 +215,14 @@ class JsonlDecoder implements Decoder {
     }
 
     /**
-     * Creates an array containing indexes of logs which match the user selected levels. The
-     * previous array is removed and a new one is created on each call.
+     * Computes and saves the indices of the log events that match the log level filter.
      *
-     * @param logLevelFilter Array of selected log levels
+     * @param logLevelFilter
      */
     #filterLogs (logLevelFilter: LogLevelFilter) {
         this.#filteredLogIndices.length = 0;
 
-        if (!logLevelFilter) {
+        if (null === logLevelFilter) {
             return;
         }
 

new-log-viewer/src/services/formatters/LogbackFormatter.ts

@@ -55,7 +55,7 @@ class LogbackFormatter implements Formatter {
      * Formats the given log event.
      *
      * @param logEvent
-     * @return The log event's timestamp and the formatted string.
+     * @return The formatted string.
      */
     formatLogEvent (logEvent: JsonLogEvent): string {
         const {fields, timestamp} = logEvent;

new-log-viewer/src/typings/decoders.ts

@@ -58,8 +58,6 @@ interface Decoder {
     getEstimatedNumEvents(): number;
 
     /**
-     * Retrieves the filtered log events indices, which is usually based on log level.
-     *
      * @return Indices of the filtered events.
      */
     getFilteredLogEvents(): number[];
@@ -84,8 +82,8 @@ interface Decoder {
     buildIdx(beginIdx: number, endIdx: number): Nullable<LogEventCount>;
 
     /**
-     * Decodes filtered log events (i.e. only includes events are included in current filter) in
-     * the range `[beginIdx, endIdx)`.
+     * Decodes filtered log events (i.e. only events included by the current filter) in the range
+     * `[beginIdx, endIdx)`.
      *
      * @param beginIdx
      * @param endIdx
@@ -95,7 +93,8 @@ interface Decoder {
     decodeFilteredRange(beginIdx: number, endIdx: number): Nullable<DecodeResultType[]>;
 
     /**
-     * Decodes the all log events in the range `[beginIdx, endIdx)` irrespective of filter applied.
+     * Decodes all log events in the range `[beginIdx, endIdx)`, ignoring any filter that may have
+     * been set.
      *
      * @param beginIdx
      * @param endIdx