From 193b494c099307188efa2f7c76a13dbd2ef7a764 Mon Sep 17 00:00:00 2001 From: Gabriel Bernal Date: Wed, 16 Sep 2026 16:41:52 +0200 Subject: [PATCH] fix: handle direction and sorting accurately for load more requests Signed-off-by: Gabriel Bernal --- web/cypress/e2e/integration/logs-alerts.cy.ts | 2 +- .../e2e/integration/logs-detail-page.cy.ts | 14 ++++-- .../e2e/integration/logs-dev-page.cy.ts | 23 ++++++---- web/cypress/e2e/integration/logs-page.cy.ts | 16 +++++-- web/jest.config.js | 3 +- web/package.json | 2 +- .../__tests__/logs-table-load-more.spec.ts | 46 +++++++++++++++++++ web/src/__tests__/value-utils.spec.ts | 28 +++++++++++ web/src/components/logs-table-utils.ts | 36 +++++++++++++++ web/src/components/logs-table.tsx | 8 +++- web/src/hooks/useLogs.ts | 16 +------ web/src/value-utils.ts | 18 +++++++- web/tsconfig.spec.json | 8 ++++ 13 files changed, 185 insertions(+), 35 deletions(-) create mode 100644 web/src/__tests__/logs-table-load-more.spec.ts create mode 100644 web/src/components/logs-table-utils.ts create mode 100644 web/tsconfig.spec.json diff --git a/web/cypress/e2e/integration/logs-alerts.cy.ts b/web/cypress/e2e/integration/logs-alerts.cy.ts index d5bef4b60..416dd62df 100644 --- a/web/cypress/e2e/integration/logs-alerts.cy.ts +++ b/web/cypress/e2e/integration/logs-alerts.cy.ts @@ -6,7 +6,7 @@ import { } from '../../fixtures/query-range-fixtures'; Cypress.Keyboard.defaults({ - keystrokeDelay: 15, + keystrokeDelay: 40, }); const LOGS_ALERTS_PAGE_URL = '/monitoring/alerts/test-alert'; diff --git a/web/cypress/e2e/integration/logs-detail-page.cy.ts b/web/cypress/e2e/integration/logs-detail-page.cy.ts index a4f81ff47..edd7af4b1 100644 --- a/web/cypress/e2e/integration/logs-detail-page.cy.ts +++ b/web/cypress/e2e/integration/logs-detail-page.cy.ts @@ -5,7 +5,7 @@ import { } from '../../fixtures/query-range-fixtures'; Cypress.Keyboard.defaults({ - keystrokeDelay: 15, + keystrokeDelay: 40, }); const LOGS_DETAIL_PAGE_URL = '/k8s/ns/my-namespace/pods/test-pod-name'; @@ -63,7 +63,6 @@ describe('Logs Detail Page', () => { .type('{selectAll}') .type('{ job = "some_job" }', { parseSpecialCharSequences: false, - delay: 1, }) .type('{enter}'); }); @@ -213,9 +212,10 @@ describe('Logs Detail Page', () => { ); }); + cy.intercept(QUERY_RANGE_MATRIX_URL_MATCH, queryRangeMatrixValidResponse()).as('executeMatrix'); cy.byTestID(TestIds.ExecuteQueryButton).click(); - cy.wait('@queryRangeMatrix'); + cy.wait('@executeMatrix'); cy.byTestID(TestIds.LogsMetrics).should('exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.disabled'); @@ -230,9 +230,15 @@ describe('Logs Detail Page', () => { }); }); + // Re-alias so the wait targets this execution's streams request rather than a + // stale one (initial load or histogram toggle) still held by the shared alias. + cy.intercept( + QUERY_RANGE_STREAMS_URL_MATCH, + queryRangeStreamsValidResponse({ message: TEST_MESSAGE }), + ).as('executeStreams'); cy.byTestID(TestIds.ExecuteQueryButton).click(); - cy.wait('@queryRangeStreams'); + cy.wait('@executeStreams'); cy.byTestID(TestIds.LogsMetrics).should('not.exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.enabled'); diff --git a/web/cypress/e2e/integration/logs-dev-page.cy.ts b/web/cypress/e2e/integration/logs-dev-page.cy.ts index 42aa08308..6f411230a 100644 --- a/web/cypress/e2e/integration/logs-dev-page.cy.ts +++ b/web/cypress/e2e/integration/logs-dev-page.cy.ts @@ -6,7 +6,7 @@ import { import { podsLabelValuesResponse } from '../../fixtures/resource-api-fixtures'; Cypress.Keyboard.defaults({ - keystrokeDelay: 15, + keystrokeDelay: 40, }); const LOGS_DEV_PAGE_URL = '/dev-monitoring/ns/my-namespace/logs'; @@ -106,7 +106,6 @@ describe('Logs Dev Page', () => { .type('{selectAll}') .type('{ job = "some_job" }', { parseSpecialCharSequences: false, - delay: 1, }) .type('{enter}'); }); @@ -406,13 +405,15 @@ describe('Logs Dev Page', () => { cy.intercept( QUERY_RANGE_STREAMS_URL_MATCH, queryRangeStreamsValidResponse({ message: TEST_MESSAGE }), - ); + ).as('queryRangeStreams'); cy.intercept(QUERY_RANGE_MATRIX_URL_MATCH, queryRangeMatrixValidResponse()).as( 'queryRangeMatrix', ); cy.visit(LOGS_DEV_PAGE_URL); + cy.wait('@queryRangeStreams'); + cy.byTestID(TestIds.ToggleHistogramButton).click(); cy.byTestID(TestIds.LogsHistogram) @@ -431,14 +432,14 @@ describe('Logs Dev Page', () => { 'sum by (level) (count_over_time({ kubernetes_namespace_name="my-namespace" })[10m])', { parseSpecialCharSequences: false, - delay: 1, }, ); }); - cy.byTestID(TestIds.ExecuteQueryButton).click({ force: true }); + cy.intercept(QUERY_RANGE_MATRIX_URL_MATCH, queryRangeMatrixValidResponse()).as('executeMatrix'); + cy.byTestID(TestIds.ExecuteQueryButton).click(); - cy.wait('@queryRangeMatrix'); + cy.wait('@executeMatrix'); cy.byTestID(TestIds.LogsMetrics).should('exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.disabled'); @@ -450,11 +451,17 @@ describe('Logs Dev Page', () => { .type('{backspace}') .type('{ kubernetes_namespace_name="my-namespace" }', { parseSpecialCharSequences: false, - delay: 1, }); }); - cy.byTestID(TestIds.ExecuteQueryButton).click({ force: true }); + cy.intercept( + QUERY_RANGE_STREAMS_URL_MATCH, + queryRangeStreamsValidResponse({ message: TEST_MESSAGE }), + ).as('executeStreams'); + cy.byTestID(TestIds.ExecuteQueryButton).click(); + + cy.wait('@executeStreams'); + cy.byTestID(TestIds.LogsMetrics).should('not.exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.enabled'); cy.byTestID(TestIds.ToggleHistogramButton).click(); diff --git a/web/cypress/e2e/integration/logs-page.cy.ts b/web/cypress/e2e/integration/logs-page.cy.ts index f85e0a4ee..1f4dcc5e1 100644 --- a/web/cypress/e2e/integration/logs-page.cy.ts +++ b/web/cypress/e2e/integration/logs-page.cy.ts @@ -18,7 +18,7 @@ import { formatTimeRange } from '../../../src/time-range'; import { configResponse } from '../../fixtures/backend-fixtures'; Cypress.Keyboard.defaults({ - keystrokeDelay: 15, + keystrokeDelay: 40, }); const LOGS_PAGE_URL = '/monitoring/logs'; @@ -314,7 +314,6 @@ describe('Logs Page', () => { .type('{selectAll}') .type('{ job = "some_job" }', { parseSpecialCharSequences: false, - delay: 1, }) .type('{enter}'); }); @@ -708,9 +707,13 @@ describe('Logs Page', () => { ); }); + // Re-alias immediately before the click so the wait targets the request this + // execution triggers, not the histogram's own `sum(...)` request captured by + // the shared `@queryRangeMatrix` alias. + cy.intercept(QUERY_RANGE_MATRIX_URL_MATCH, queryRangeMatrixValidResponse()).as('executeMatrix'); cy.byTestID(TestIds.ExecuteQueryButton).click(); - cy.wait('@queryRangeMatrix'); + cy.wait('@executeMatrix'); cy.byTestID(TestIds.LogsMetrics).should('exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.disabled'); @@ -725,9 +728,14 @@ describe('Logs Page', () => { }); }); + // Re-alias so the wait targets this execution's streams request rather than a + // stale one (initial load or histogram toggle) still held by the shared alias. + cy.intercept(QUERY_RANGE_STREAMS_URL_MATCH, queryRangeStreamsWithMessage()).as( + 'executeStreams', + ); cy.byTestID(TestIds.ExecuteQueryButton).click(); - cy.wait('@queryRangeStreams'); + cy.wait('@executeStreams'); cy.byTestID(TestIds.LogsMetrics).should('not.exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.enabled'); diff --git a/web/jest.config.js b/web/jest.config.js index b062ac436..af434d575 100644 --- a/web/jest.config.js +++ b/web/jest.config.js @@ -4,7 +4,8 @@ module.exports = { preset: 'ts-jest', testEnvironment: 'node', transform: { - '^.+\\.js$': 'ts-jest', + '^.+\\.tsx?$': ['ts-jest', { tsconfig: 'tsconfig.spec.json' }], + '^.+\\.js$': ['ts-jest', { tsconfig: 'tsconfig.spec.json' }], }, transformIgnorePatterns: ['node_modules/(?!(@openshift-console|@patternfly))'], moduleNameMapper: { diff --git a/web/package.json b/web/package.json index 06fdff93c..8a2d17b70 100644 --- a/web/package.json +++ b/web/package.json @@ -23,7 +23,7 @@ "cypress:run": "cypress run", "cypress:run:ci": "NO_COLOR=1 cypress run --spec \"cypress/e2e/integration/*.cy.ts\" --browser electron", "test": "concurrently -n \"unit,e2e\" --kill-others-on-fail \"npm run test:unit\" \"npm run test:e2e\"", - "test:unit": "TZ=UTC jest --config jest.config.js", + "test:unit": "TZ=UTC jest --config jest.config.js --runInBand --ci", "test:e2e": "./scripts/run-cypress.sh", "test:unit:watch": "TZ=UTC jest --config jest.config.js --watch", "test:unit:coverage": "TZ=UTC jest --config jest.config.js --coverage", diff --git a/web/src/__tests__/logs-table-load-more.spec.ts b/web/src/__tests__/logs-table-load-more.spec.ts new file mode 100644 index 000000000..252cc388d --- /dev/null +++ b/web/src/__tests__/logs-table-load-more.spec.ts @@ -0,0 +1,46 @@ +import { getLoadMoreTimestamp, getTimestampBounds } from '../components/logs-table-utils'; +import { LogTableData } from '../logs.types'; +import { getPaginationRange } from '../value-utils'; + +const messageSortedData = [ + { rawTimestamp: '300' }, + { rawTimestamp: '100' }, + { rawTimestamp: '200' }, +] as Array; + +describe('load-more boundary selection (table -> hook)', () => { + it('anchors are the chronological oldest/newest, independent of row (Message) order', () => { + const bounds = getTimestampBounds(messageSortedData); + + expect(bounds).toEqual({ oldest: '100', newest: '300' }); + }); + + it('paginates from the oldest visible timestamp when loading backward, even with Message sorting', () => { + const bounds = getTimestampBounds(messageSortedData); + const lastTimestampNs = getLoadMoreTimestamp(bounds, 'backward'); + + // Oldest visible entry, not the final row of the message-sorted table. + expect(lastTimestampNs).toBe('100'); + + const { endNs } = getPaginationRange(lastTimestampNs as string, 'backward'); + expect(endNs).toBe(String(100n - 1n)); + }); + + it('paginates from the newest visible timestamp when loading forward, even with Message sorting', () => { + const bounds = getTimestampBounds(messageSortedData); + const lastTimestampNs = getLoadMoreTimestamp(bounds, 'forward'); + + // Newest visible entry, not the final row of the message-sorted table. + expect(lastTimestampNs).toBe('300'); + + const { startNs } = getPaginationRange(lastTimestampNs as string, 'forward'); + expect(startNs).toBe(String(300n + 1n)); + }); + + it('returns no anchor when there is no data to paginate from', () => { + const bounds = getTimestampBounds([]); + + expect(bounds).toEqual({ oldest: undefined, newest: undefined }); + expect(getLoadMoreTimestamp(bounds, 'backward')).toBeUndefined(); + }); +}); diff --git a/web/src/__tests__/value-utils.spec.ts b/web/src/__tests__/value-utils.spec.ts index 22fd816cf..f359ef21e 100644 --- a/web/src/__tests__/value-utils.spec.ts +++ b/web/src/__tests__/value-utils.spec.ts @@ -5,6 +5,8 @@ import { valueWithScalePrefix, capitalize, msToNs, + getPaginationRange, + ONE_HOUR_IN_NS, } from '../value-utils'; describe('value utils', () => { @@ -52,4 +54,30 @@ describe('value utils', () => { expect(msToNs(1000.7)).toBe('1001000000'); expect(msToNs(1000.3)).toBe('1000000000'); }); + + describe('getPaginationRange', () => { + const lastTs = '1666003060000000000'; + const last = BigInt(lastTs); + + it('uses an exclusive upper bound when paginating backward to avoid overlap', () => { + const { startNs, endNs } = getPaginationRange(lastTs, 'backward'); + + // end must exclude the oldest visible entry (last - 1ns), not re-include it + expect(endNs).toBe(String(last - 1n)); + expect(startNs).toBe(String(last - ONE_HOUR_IN_NS)); + }); + + it('uses an exclusive lower bound when paginating forward to avoid overlap', () => { + const { startNs, endNs } = getPaginationRange(lastTs, 'forward'); + + expect(startNs).toBe(String(last + 1n)); + expect(endNs).toBe(String(last + ONE_HOUR_IN_NS)); + }); + + it('honors a custom span', () => { + const span = 2n * ONE_HOUR_IN_NS; + expect(getPaginationRange(lastTs, 'backward', span).startNs).toBe(String(last - span)); + expect(getPaginationRange(lastTs, 'forward', span).endNs).toBe(String(last + span)); + }); + }); }); diff --git a/web/src/components/logs-table-utils.ts b/web/src/components/logs-table-utils.ts new file mode 100644 index 000000000..34eca1ae5 --- /dev/null +++ b/web/src/components/logs-table-utils.ts @@ -0,0 +1,36 @@ +import { Direction, LogTableData } from '../logs.types'; + +export interface TimestampBounds { + oldest?: string; + newest?: string; +} + +/** + * Computes the chronological anchors (oldest and newest raw timestamps) for a + * set of table rows ragardless of the sorting. + */ +export const getTimestampBounds = ( + data: Array>, +): TimestampBounds => { + let oldest: string | undefined; + let newest: string | undefined; + + for (const { rawTimestamp } of data) { + const timestamp = BigInt(rawTimestamp); + + if (oldest === undefined || timestamp < BigInt(oldest)) { + oldest = rawTimestamp; + } + + if (newest === undefined || timestamp > BigInt(newest)) { + newest = rawTimestamp; + } + } + + return { oldest, newest }; +}; + +export const getLoadMoreTimestamp = ( + bounds: TimestampBounds, + direction?: Direction, +): string | undefined => (direction === 'forward' ? bounds.newest : bounds.oldest); diff --git a/web/src/components/logs-table.tsx b/web/src/components/logs-table.tsx index 2c78169fc..a5f2a401b 100644 --- a/web/src/components/logs-table.tsx +++ b/web/src/components/logs-table.tsx @@ -30,6 +30,7 @@ import { severityFromString } from '../severity'; import { numericComparator, bigIntDifference } from '../sort-utils'; import { TestIds } from '../test-ids'; import { LogDetail } from './log-detail'; +import { getLoadMoreTimestamp, getTimestampBounds } from './logs-table-utils'; import './logs-table.css'; import { StatsTable } from './stats-table'; import { TableData, VirtualizedLogsTable } from './virtualized-logs-table'; @@ -357,8 +358,13 @@ export const LogsTable: FC> = ({ const dataIsEmpty = sortedData.length === 0; + const timestampBounds = useMemo(() => getTimestampBounds(tableData), [tableData]); + const handleLoadMore = () => { - onLoadMore?.(tableData[tableData.length - 1].rawTimestamp); + const lastTimestampNs = getLoadMoreTimestamp(timestampBounds, direction); + if (lastTimestampNs !== undefined) { + onLoadMore?.(lastTimestampNs); + } }; const RowComponent = useMemo( diff --git a/web/src/hooks/useLogs.ts b/web/src/hooks/useLogs.ts index a1d5b376d..854196219 100644 --- a/web/src/hooks/useLogs.ts +++ b/web/src/hooks/useLogs.ts @@ -20,7 +20,7 @@ import { validateQueryRangeResponse, } from '../loki-client'; import { intervalFromTimeRange, numericTimeRange, timeRangeFromDuration } from '../time-range'; -import { msToNs } from '../value-utils'; +import { getPaginationRange, msToNs } from '../value-utils'; import { LogQLQuery } from '../logql-query'; import { LogsContext } from './LogsConfigProvider'; @@ -344,19 +344,7 @@ export const useLogs = ( currentQuery.current = query; currentDirection.current = direction ?? currentDirection.current; - const lastTs = BigInt(lastTimestampNs); - const oneHourNs = 3_600_000_000_000n; - - let startNs: string; - let endNs: string; - - if (currentDirection.current === 'forward') { - startNs = String(lastTs + 1n); - endNs = String(lastTs + oneHourNs); - } else { - startNs = String(lastTs - oneHourNs); - endNs = String(lastTs); - } + const { startNs, endNs } = getPaginationRange(lastTimestampNs, currentDirection.current); dispatch({ type: 'moreLogsRequest' }); diff --git a/web/src/value-utils.ts b/web/src/value-utils.ts index 1cb193c8d..68789aec9 100644 --- a/web/src/value-utils.ts +++ b/web/src/value-utils.ts @@ -1,4 +1,20 @@ -import { DEFAULT_SCHEMA, Schema, SchemaConfig } from './logs.types'; +import { DEFAULT_SCHEMA, Direction, Schema, SchemaConfig } from './logs.types'; + +export const ONE_HOUR_IN_NS = 3_600_000_000_000n; + +export const getPaginationRange = ( + lastTimestampNs: string, + direction: Direction, + spanNs: bigint = ONE_HOUR_IN_NS, +): { startNs: string; endNs: string } => { + const lastTs = BigInt(lastTimestampNs); + + if (direction === 'forward') { + return { startNs: String(lastTs + 1n), endNs: String(lastTs + spanNs) }; + } + + return { startNs: String(lastTs - spanNs), endNs: String(lastTs - 1n) }; +}; /** * Converts a value into a string with scale prefix diff --git a/web/tsconfig.spec.json b/web/tsconfig.spec.json new file mode 100644 index 000000000..c3394980a --- /dev/null +++ b/web/tsconfig.spec.json @@ -0,0 +1,8 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + // Test-only: skip full type-checking during ts-jest transpilation to cut + // CPU/memory. Type errors are still enforced by `npm run lint:tsc`. + "isolatedModules": true + } +}