feat(KFLUXUI-875): add syntax support for log viewer#682
feat(KFLUXUI-875): add syntax support for log viewer#682testcara wants to merge 2 commits intokonflux-ci:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Dependencies package.json |
Added prismjs and @types/prismjs to dependencies for syntax highlighting support. |
Line Number Navigation src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx, src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts |
New LineNumberGutter component renders clickable line numbers with accessibility features; useLineNumberNavigation hook manages URL hash-based line selection, range highlighting, and shift-click range creation. |
Syntax Highlighting src/shared/components/virtualized-log-viewer/prism-log-language.ts, src/shared/components/virtualized-log-viewer/prism-log-theme.scss |
Custom Prism language definition for log tokenization (timestamps, log levels, key-value pairs, test results); SCSS theme applies PatternFly v5 colors to token types across light/dark modes. |
Content Rendering src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx |
Introduces lazy tokenization with per-line caching, token-aware rendering pipeline, gutter integration, line-height measurement, and scroll-to-highlight effect for hash-based navigation. |
Styling src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scss |
Added gutter container with sticky positioning, line-number cell styling, highlighted line backgrounds, and content column layout to support gutter + content side-by-side display. |
Exports src/shared/components/virtualized-log-viewer/index.ts |
Barrel exports for LineNumberGutter, VirtualizedLogContent, useLineNumberNavigation, and related prop types. |
Test Coverage src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx, src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx, src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx, src/shared/components/virtualized-log-viewer/__tests__/prism-log-language.spec.ts, src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts |
Comprehensive test suite covering gutter rendering, line-number click handling, hash navigation, tokenization behavior, search highlighting, and edge cases across virtualization and accessibility scenarios. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant LineNumberGutter as Line Number Gutter
participant useLineNumberNavigation as useLineNumberNavigation Hook
participant VirtualizedLogContent as VirtualizedLogContent
participant Prism
User->>LineNumberGutter: Click line number
LineNumberGutter->>useLineNumberNavigation: handleLineClick(lineNumber, event)
alt Normal Click
useLineNumberNavigation->>useLineNumberNavigation: Update hash to `#L`{lineNumber}
useLineNumberNavigation->>useLineNumberNavigation: Set highlightedLines = {start, end}
else Shift+Click with firstSelectedLine
useLineNumberNavigation->>useLineNumberNavigation: Create range from firstSelectedLine to lineNumber
useLineNumberNavigation->>useLineNumberNavigation: Update hash to `#L`{start}-L{end}
end
useLineNumberNavigation->>VirtualizedLogContent: Trigger re-render with new highlightedLines
VirtualizedLogContent->>Prism: tokenizeLine(lineText)
Prism->>VirtualizedLogContent: Return tokens
VirtualizedLogContent->>VirtualizedLogContent: Render tokens with highlight styling
VirtualizedLogContent->>VirtualizedLogContent: Scroll to highlighted line
VirtualizedLogContent->>User: Display highlighted content
sequenceDiagram
participant Browser
participant VirtualizedLogContent as VirtualizedLogContent
participant Prism
participant Cache as Tokenization Cache
Browser->>VirtualizedLogContent: Load with URL hash `#L10-L20`
VirtualizedLogContent->>VirtualizedLogContent: Parse hash to highlightedLines
VirtualizedLogContent->>Cache: Check if line tokens cached
alt Cache Hit
Cache->>VirtualizedLogContent: Return cached tokens
else Cache Miss
VirtualizedLogContent->>Prism: tokenizeLine(logText)
Prism->>VirtualizedLogContent: Return tokens
VirtualizedLogContent->>Cache: Store tokens
end
VirtualizedLogContent->>VirtualizedLogContent: Flatten token text for search
VirtualizedLogContent->>VirtualizedLogContent: Compute line matches from search
VirtualizedLogContent->>VirtualizedLogContent: Render token string with highlights
VirtualizedLogContent->>VirtualizedLogContent: Scroll to center line 10
VirtualizedLogContent->>Browser: Display highlighted lines with gutter
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
- Revert "feat(KFLUXUI-874): support url hash navigation for log viewer" #690: Directly reverts the virtualized-log-viewer additions (LineNumberGutter, VirtualizedLogContent, useLineNumberNavigation, tests, and exports) touched by this PR.
- feat(KFLUXUI-874): support url hash navigation for log viewer #671: Adds the same virtualized log viewer components (LineNumberGutter, VirtualizedLogViewer, useLineNumberNavigation) and line-number navigation infrastructure.
- feat(KFLUXUI-912): create customized log viewer to replace pf log viewer #626: Makes overlapping changes to VirtualizedLogContent, VirtualizedLogViewer, Prism integration, and virtualized scroll/search hooks.
Suggested labels
ok-to-test
Suggested reviewers
- sahil143
- rrosatti
- JoaoPedroPP
- janaki29
- marcin-michal
- milantaky
- rakshett
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'feat(KFLUXUI-875): add syntax support for log viewer' accurately describes the main feature added in this PR—Prism.js-based syntax highlighting for log viewers with support for timestamps, log levels, key-value pairs, and test results. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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.
🔧 Stylelint (17.3.0)
src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scss
ConfigurationError: Could not find "stylelint-config-recommended-scss". Do you need to install the package or use the "configBasedir" option?
at getModulePath (file:///usr/local/lib/node_modules/stylelint/lib/utils/getModulePath.mjs:29:9)
at loadExtendedConfig (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:241:21)
at extendConfig (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:208:25)
at augmentConfigBasic (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:73:26)
at augmentConfigFull (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:126:30)
at getConfigForFile (file:///usr/local/lib/node_modules/stylelint/lib/getConfigForFile.mjs:102:32)
at async resolveOptionValue (file:///usr/local/lib/node_modules/stylelint/lib/utils/resolveOptionValue.mjs:27:24)
at async standalone (file:///usr/local/lib/node_modules/stylelint/lib/standalone.mjs:202:19)
src/shared/components/virtualized-log-viewer/prism-log-theme.scss
ConfigurationError: Could not find "stylelint-config-recommended-scss". Do you need to install the package or use the "configBasedir" option?
at getModulePath (file:///usr/local/lib/node_modules/stylelint/lib/utils/getModulePath.mjs:29:9)
at loadExtendedConfig (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:241:21)
at extendConfig (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:208:25)
at augmentConfigBasic (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:73:26)
at augmentConfigFull (file:///usr/local/lib/node_modules/stylelint/lib/augmentConfig.mjs:126:30)
at getConfigForFile (file:///usr/local/lib/node_modules/stylelint/lib/getConfigForFile.mjs:102:32)
at async resolveOptionValue (file:///usr/local/lib/node_modules/stylelint/lib/utils/resolveOptionValue.mjs:27:24)
at async standalone (file:///usr/local/lib/node_modules/stylelint/lib/standalone.mjs:202:19)
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
142-142:⚠️ Potential issue | 🟡 MinorAlign
packageManagerversion with project coding guidelines.The
packageManagerfield specifiesyarn@4.12.0, which differs from the project's coding guidelines that requireyarn@4.6.0. This creates a version mismatch between documentation and implementation. Resolve by either:
- Reverting to
yarn@4.6.0to align with coding guidelines, or- Updating coding guidelines to reflect the intentional upgrade to
yarn@4.12.0Ensure consistency across all project documentation, including the DevContainer setup which currently references an outdated
Yarn 1.22.22.
🤖 Fix all issues with AI agents
In `@src/shared/components/pipeline-run-logs/logs/useLogViewerSearch.ts`:
- Around line 90-101: The computed scrolledRow in the useMemo for scrolledRow
should guard against rowInFocus.rowIndex being out of range after lines shrinks:
inside the useMemo (where scrolledRow, rowInFocus, autoScroll and lines.length
are used) check that rowInFocus.rowIndex is >= 0 and < lines.length before
returning rowInFocus.rowIndex + 1 (remember scrollToRow is 1-indexed); if it is
out of bounds fall back to the autoScroll branch (return lines.length) or 0 as
before. Ensure you update the dependency array stays correct (autoScroll,
lines.length, rowInFocus).
In `@src/shared/components/virtualized-log-viewer/useVirtualizedScroll.ts`:
- Around line 59-75: The effect unconditionally clears scrollToIndexRef.current
after initiating a scroll, which causes premature clearing when scrollToRow
changes between positive values; modify the effect so the reset of
scrollToIndexRef.current happens only in an else branch (i.e., when scrollToRow
is undefined or 0) rather than unconditionally, keep the existing timer-based
cleanup that sets it undefined after SCROLL_ANIMATION_DURATION_MS, and ensure
the returned cleanup still clears the timeout; update the block that references
scrollToRow, scrollToIndexRef, virtualizer, and SCROLL_ANIMATION_DURATION_MS
accordingly.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 142-149: The performance-warning threshold in
VirtualizedLogContent.tsx is inconsistent with the PR description: change the
hardcoded threshold in the dev-only tokenization timing check from 50ms to 10ms
(the block that computes duration = performance.now() - startTime and logs "Slow
tokenization") so the console.warn triggers when duration > 10; alternatively,
if 50ms is intended, update the PR description to state ">50ms" to match the
code.
🧹 Nitpick comments (11)
src/shared/components/virtualized-log-viewer/prism-log-language.ts (1)
63-67: Timestamp regex is comprehensive but consider edge case.The pattern handles common formats well. Note that bare time patterns like
\b\d{2}:\d{2}:\d{2}\bmay match times embedded in other contexts (e.g., version strings like1.23:45:67). This is likely acceptable given the log-file context, but worth monitoring if false positives appear.src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx (2)
77-95: ResizeObserver error suppression is documented but broad.The error handler catches and suppresses known browser limitations. The linked issue provides context. However, this global error handler could mask other legitimate errors during development.
Consider scoping this more narrowly or adding a counter to detect if suppression happens excessively:
Optional refinement
React.useEffect(() => { + let suppressionCount = 0; const handleError = (event: ErrorEvent) => { if ( event.message?.includes('ResizeObserver loop completed with undelivered notifications') || event.message?.includes('ResizeObserver loop limit exceeded') ) { + suppressionCount++; + if (process.env.NODE_ENV !== 'production' && suppressionCount > 100) { + console.warn('Excessive ResizeObserver errors suppressed'); + } event.preventDefault(); event.stopImmediatePropagation(); return true; } };
305-313: Double requestAnimationFrame may cause timing issues.Nested
requestAnimationFramecalls introduce a ~32ms delay (two frames). This works but is fragile. Consider using a single rAF with the virtualizer's ready state check, or document why two frames are required.src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx (1)
492-509: Prefer semantic queries for line-number links.These assertions can use role-based queries to align with the testing guidelines and improve resilience.
♻️ Suggested tweak
- const lineNumberLinks = document.querySelectorAll('.log-viewer__line-number'); + const lineNumberLinks = screen.getAllByRole('link', { + name: /jump to line \d+/i, + }); lineNumberLinks.forEach((link) => { expect(link).toHaveAttribute('aria-label'); expect(link.getAttribute('aria-label')).toMatch(/^Jump to line \d+$/); });As per coding guidelines, Prioritize semantic queries (
getByRole,getByLabelText) over implementation-detail queries (getByTestId,getByClassName) when selecting elements in tests.src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx (1)
29-44: Replace raw layout<div>with PatternFlyFlexItem.This keeps layout consistent with the repo’s PatternFly-only layout rule while still allowing absolute positioning.
♻️ Proposed change
-import { Flex } from '@patternfly/react-core'; +import { Flex, FlexItem } from '@patternfly/react-core'; ... - <div + <FlexItem key={`gutter-${virtualItem.key}`} className={`log-viewer__gutter-cell ${isHighlighted ? 'log-viewer__gutter-cell--highlighted' : ''}`} style={{ position: 'absolute', top: 0, left: 0, height: `${itemSize}px`, transform: `translateY(${virtualItem.start}px)`, }} > <a href={`#L${lineNumber}`} className="log-viewer__line-number" aria-label={`Jump to line ${lineNumber}`} onClick={(e) => { e.preventDefault(); onLineClick(lineNumber, e); }} data-line-number={lineNumber} > {lineNumber} </a> - </div> + </FlexItem>As per coding guidelines, NEVER use raw HTML layout tags (
,, ) or custom CSS layouts. Always use PatternFly layout components for all layout structures. src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx (1)
163-167: Consider usinguserEventinstead offireEventfor click interactions.Per project testing guidelines, async
userEvent.setup()is preferred over synchronousfireEventfor testing user actions. This provides more realistic event simulation.♻️ Suggested refactor
+import userEvent from '@testing-library/user-event'; + describe('Click Handling', () => { - it('should call onLineClick when line number is clicked', () => { + it('should call onLineClick when line number is clicked', async () => { + const user = userEvent.setup(); const onLineClick = jest.fn(); render(<LineNumberGutter {...defaultProps} onLineClick={onLineClick} />); const link = screen.getByText('1'); - fireEvent.click(link); + await user.click(link); expect(onLineClick).toHaveBeenCalledWith(1, expect.any(Object)); });As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.tsx (1)
21-28: Update outdated JSDoc comment.The comment states "Renders plain text (no syntax highlighting yet)" but this PR adds Prism-based syntax highlighting. The documentation should reflect the actual feature set.
📝 Suggested fix
/** * Virtualized log viewer using `@tanstack/react-virtual` * * Features: * - Uses `@tanstack/react-virtual` for virtualization - * - Renders plain text (no syntax highlighting yet) + * - Prism-based syntax highlighting for timestamps, log levels, and key=value pairs * - Drop-in replacement for PatternFly LogViewer + * - Search highlighting integration */src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts (1)
65-84: Consider dispatchinghashchangeevent afterpushStatefor consistency.
window.history.pushStatedoes not trigger thehashchangeevent. While the internal state is updated directly (which is correct), other parts of the application listening forhashchangewon't be notified. If cross-component synchronization via hash is expected, consider dispatching the event manually.💡 Optional enhancement
if (event.shiftKey && firstSelectedLine !== null) { const start = Math.min(firstSelectedLine, lineNumber); const end = Math.max(firstSelectedLine, lineNumber); const hash = `#L${start}-L${end}`; window.history.pushState(null, '', hash); + window.dispatchEvent(new HashChangeEvent('hashchange')); setHighlightedLines({ start, end }); setFirstSelectedLine(null); } else { const hash = `#L${lineNumber}`; window.history.pushState(null, '', hash); + window.dispatchEvent(new HashChangeEvent('hashchange')); setHighlightedLines({ start: lineNumber, end: lineNumber }); setFirstSelectedLine(lineNumber); }src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx (1)
88-111: Console suppression pattern is acceptable but consider centralizing.The eslint-disable comments suppress known harmless warnings during tests. While this works, consider moving this pattern to a shared test setup utility to avoid repetition across test files and reduce eslint-disable comments.
Based on learnings: "If code contains comments that suppress lint warnings or errors, remind contributors to fix the underlying issue rather than silencing the rule."
src/shared/components/pipeline-run-logs/logs/LogViewer.tsx (2)
194-203: MoveuseId()call to component top level.
React.useId()is called inline in JSX. While this works because it's unconditional, React hooks should be called at the top level of the component for clarity and adherence to the Rules of Hooks.♻️ Proposed fix
At the top of the component (around line 77):
const taskName = taskRun?.spec.taskRef?.name ?? taskRun?.metadata.name; const { effectiveTheme } = useTheme(); +const themeCheckboxId = React.useId(); const [logTheme, setLogTheme] = React.useState<'light' | 'dark'>('dark');Then update the Checkbox:
<Checkbox - id={React.useId()} + id={themeCheckboxId} label="Dark theme"
165-171: Raw div elements used for PatternFly CSS integration.The coding guidelines advise using PatternFly layout components instead of raw HTML. These divs use PatternFly's internal CSS class structure (
pf-v5-c-log-viewer,pf-v5-c-log-viewer__header) and require refs for fullscreen and height measurement functionality.If maintaining strict guideline compliance is important, consider wrapping content with PatternFly's
Stack/StackItemwhile preserving the necessary class names and refs. However, given the PatternFly CSS integration requirements, the current approach is acceptable.Also applies to: 272-279
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx`:
- Around line 1-7: Replace direct calls to testing-library's render in
LogViewer.spec.tsx with the repo's test harness helper
renderWithQueryClientAndRouter (imported from
'~/unit-test-utils/rendering-utils') so the component under test (LogViewer)
runs with React Query and Router context; update imports to remove or keep
screen, waitFor, act and userEvent as needed, wrap any existing render(...)
calls with renderWithQueryClientAndRouter(...) and ensure tests that rely on
router/query state (including those mocking useFullscreen, useTheme or
file-saver saveAs effects) continue to assert via screen and waitFor unchanged.
In `@src/shared/components/pipeline-run-logs/logs/LogViewer.tsx`:
- Around line 194-202: The call to React.useId() must be moved out of the JSX
and invoked at the top of the LogViewer component; create a const like
themeCheckboxId = React.useId() near the other top-level hooks in LogViewer
(after initial state/hooks) and then pass themeCheckboxId as the id prop to the
Checkbox instead of calling React.useId() inline; ensure the Checkbox still uses
isDisabled={effectiveTheme === 'dark'}, checked={logTheme === 'dark'} and
onClick={() => setLogTheme(prev => (prev === 'dark' ? 'light' : 'dark'))} so
behavior is unchanged.
In
`@src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx`:
- Around line 158-209: In the "Click Handling" tests for LineNumberGutter,
replace all uses of fireEvent.click and manual dispatchEvent with async user
interactions: create const user = userEvent.setup() at the start of each test
and use await user.click(element, { shiftKey: true }) (or without options) to
trigger clicks; for the "should prevent default link behavior on click" test,
spy on Event.prototype.preventDefault before the click and then await
user.click(link) to assert the spy was called. Update assertions that expect the
mouse event to use expect.objectContaining({ shiftKey: true }) as before and
keep the same onLineClick call checks (e.g., onLineClick, LineNumberGutter, and
test titles) while changing the interaction calls to await user.click.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scss`:
- Around line 79-81: The CSS rule for the highlighted log line
(.log-viewer__line--highlighted) has a stray double semicolon after the
background-color declaration; open the .log-viewer__line--highlighted rule in
VirtualizedLogViewer.scss and remove the extra semicolon so the declaration ends
with a single semicolon (i.e., only one semicolon after
var(--pf-v5-global--palette--gold-700)).
🧹 Nitpick comments (5)
src/shared/components/virtualized-log-viewer/__tests__/useVirtualizedScroll.spec.ts (2)
1-3: Use the shared animation constant and a path-alias import.Hard-coding
300risks drift if the hook’s animation duration changes. Also switch the hook import to a path alias to match repo standards. IfSCROLL_ANIMATION_DURATION_MSisn’t exported, consider exporting it for tests.🛠️ Suggested update
-import { useVirtualizedScroll } from '../useVirtualizedScroll'; +import { + SCROLL_ANIMATION_DURATION_MS, + useVirtualizedScroll, +} from '~/shared/components/virtualized-log-viewer/useVirtualizedScroll'; ... - jest.advanceTimersByTime(300); + jest.advanceTimersByTime(SCROLL_ANIMATION_DURATION_MS); ... - jest.advanceTimersByTime(300); + jest.advanceTimersByTime(SCROLL_ANIMATION_DURATION_MS); ... - jest.advanceTimersByTime(300); + jest.advanceTimersByTime(SCROLL_ANIMATION_DURATION_MS);As per coding guidelines: Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes.Also applies to: 191-193, 219-236
6-11: PrefercreateJestMockFunctionfor typed mocks.Use the shared helper for
scrollToIndex(and other mocks likeonScroll) to keep signatures type-safe.🧪 Example adjustment
import { Virtualizer } from '@tanstack/react-virtual'; import { renderHook } from '@testing-library/react-hooks'; +import { createJestMockFunction } from '~/unit-test-utils/common'; ... const createMockVirtualizer = ( scrollOffset = 0, ): Partial<Virtualizer<HTMLDivElement, Element>> => ({ scrollOffset, - scrollToIndex: jest.fn(), + scrollToIndex: createJestMockFunction< + Virtualizer<HTMLDivElement, Element>['scrollToIndex'] + >(), });Based on learnings: Applies to /tests//*.spec.{ts,tsx} : Use
createJestMockFunctionfrom~/unit-test-utils/commonfor type-safe mock functions with specific signatures.src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.tsx (1)
4-5: Prefer path-alias import for VirtualizedLogContent.This file is under
src/, so use the configured~alias instead of a relative import.♻️ Suggested update
-import { VirtualizedLogContent } from './VirtualizedLogContent'; +import { VirtualizedLogContent } from '~/shared/components/virtualized-log-viewer/VirtualizedLogContent';As per coding guidelines: Use absolute imports with configured path aliases (
~/components,~/types,~/k8s,~/utils,~/models,@routes).src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts (1)
57-79: Address the eslint-disable rather than suppressing the warning.The effect without a dependency array runs on every render, which is intentional to detect hash changes. However, suppressing
react-hooks/exhaustive-depsmasks a design decision that could confuse future maintainers.Consider extracting the hash-detection logic into an explicit polling pattern or documenting the rationale more prominently. If the current approach is the best tradeoff, the comment should explain why the rule is disabled rather than just what it does.
Based on learnings: code containing comments that suppress lint warnings should be fixed rather than silencing the rule.
src/shared/components/pipeline-run-logs/logs/LogViewer.tsx (1)
119-124: Consider removing the eslint-disable for no-console.The warning log for download errors is reasonable for debugging, but suppressing lint rules should be avoided. Consider using a structured logger or error boundary pattern instead of raw
console.warn.Based on learnings: code containing comments that suppress lint warnings should be fixed rather than silencing the rule.
src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx
Show resolved
Hide resolved
src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
==========================================
+ Coverage 85.94% 86.81% +0.87%
==========================================
Files 770 773 +3
Lines 58833 59339 +506
Branches 4779 5781 +1002
==========================================
+ Hits 50565 51517 +952
+ Misses 8217 7657 -560
- Partials 51 165 +114
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 112 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
testcara
requested review from
JoaoPedroPP,
StanislavJochman,
abhinandan13jan,
janaki29,
marcin-michal,
milantaky,
rakshett,
rrosatti and
sahil143
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/components/pipeline-run-logs/logs/LogViewer.tsx (1)
163-295:⚠️ Potential issue | 🟠 MajorReplace layout
<div>wrappers with PatternFly Stack/StackItem components.The main container and toolbar/content sections use flexbox for structural layout, which violates the guideline requiring PatternFly layout components. Convert to
StackandStackItem. Note: The resume button wrapper usesposition: absolutefor overlay positioning—while it can function as aStackItem, ensure the positioning and z-index behavior remain correct after refactoring.
StackandStackItemare not currently imported; add them to the PatternFly imports. The pattern is already used in similar components (VirtualizedLogViewer).
🤖 Fix all issues with AI agents
In `@src/shared/components/pipeline-run-logs/logs/LogViewer.tsx`:
- Around line 44-47: The ANSI escape regex is currently a regex literal using
the control character and disables the lint rule; replace the literal and the
eslint-disable comment by creating ANSI_ESCAPE_REGEX via the string-based RegExp
constructor (e.g., a string that escapes the ESC sequence and bracket/char
classes) with the same pattern and global flag, remove the
"eslint-disable-next-line no-control-regex" line, and keep the constant name
ANSI_ESCAPE_REGEX in LogViewer.tsx so existing usages remain unchanged.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 298-318: The effect that scrolls to highlighted lines should
include the stable virtualizer in its dependency list rather than suppressing
eslint; update the useEffect in VirtualizedLogContent (the effect that reads
highlightedLines.start and calls virtualizer.scrollToIndex) to add virtualizer
to the array of dependencies and remove the eslint-disable-next-line
react-hooks/exhaustive-deps comment, relying on virtualizer's stability from
useVirtualizer to avoid spurious re-runs.
🧹 Nitpick comments (1)
src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx (1)
322-385: Replace layout<div>wrappers with PatternFly layout components.The return block relies on multiple raw layout
<div>s. Please switch outer layout structure to PatternFly layout components (Stack/Flex/Split/Gallery) and keep raw<div>s only where virtualization requires them. As per coding guidelines:**/*.tsx: NEVER use raw HTML layout tags (,, ) or custom CSS layouts. Always use PatternFly layout components (Flex, Grid, Stack, Split, Level, Gallery, Bullseye) for responsive, accessible layouts.
src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const timestamp = tokens.find((t) => typeof t !== 'string' && t.type === 'timestamp'); | ||
| expect(timestamp).toBeDefined(); | ||
| expect(timestamp).toHaveProperty('content', '2026-02-02T10:51:43+00:00'); |
There was a problem hiding this comment.
just a nit but should not we have time format everewhere the same?
There was a problem hiding this comment.
I reviewed some logs, at least they do not always keep the same time format. Take the screenshot as one example:
There was a problem hiding this comment.
@sahil143 , Hi, Sahil, i consider Stanley's concern makes sense, for you, do we need to skip some format? Thank you!
JoaoPedroPP
left a comment
JoaoPedroPP
left a comment
There was a problem hiding this comment.
LGTM
UI is working as expected. Awesome output :)
Please take at the conflicts
Assisted-by: Claude
Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx (1)
1-7: UserenderWithQueryClientAndRouterfor these tests.This was previously flagged and still applies — the tests use bare
renderfrom@testing-library/reactinstead of the repo'srenderWithQueryClientAndRouterutility.As per coding guidelines: Use
renderWithQueryClientAndRouterfrom~/unit-test-utils/rendering-utilsfor rendering components with React Query and Router context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx` around lines 1 - 7, Tests in LogViewer.spec.tsx are using the bare render from `@testing-library/react` instead of the repo utility; replace imports and usage with renderWithQueryClientAndRouter from ~/unit-test-utils/rendering-utils. Specifically, remove or stop using the imported render symbol and import renderWithQueryClientAndRouter, then update all calls to render(...) to renderWithQueryClientAndRouter(...), ensuring the same props/children are passed so components like LogViewer get React Query and Router context during tests.src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx (1)
158-209: ReplacefireEvent.clickwithuserEventfor click interactions.This was previously flagged — the click handling tests should use
userEvent.setup()with async interactions instead of synchronousfireEvent.click.As per coding guidelines: Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx` around lines 158 - 209, Update the click interaction tests in LineNumberGutter.spec.tsx to use userEvent instead of fireEvent: import userEvent, create a user = userEvent.setup() inside each test (or shared beforeEach), replace fireEvent.click(...) with await user.click(...) and make the test functions async; ensure the mouse-event assertions that rely on modifier keys use await user.click(element, { shiftKey: true }) and that onLineClick assertions still check the same arguments (references: LineNumberGutter component tests, onLineClick handler, and screen.getByText('1'/'2'/'3') calls).src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx (1)
348-351:eslint-disable react-hooks/exhaustive-deps— previously flagged.The suppression on line 350 has already been reviewed. The
virtualizerinstance returned byuseVirtualizeris stable across renders, so it can be added to the dependency array to remove the suppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx` around lines 348 - 351, Remove the eslint-disable comment and include the stable virtualizer instance returned by useVirtualizer in the effect dependency array: update the effect in VirtualizedLogContent (the effect that currently depends on highlightedLines and lines.length) to depend on [highlightedLines, lines.length, virtualizer] so you can safely drop the eslint-disable-next-line react-hooks/exhaustive-deps; keep the rest of the effect logic unchanged.
🧹 Nitpick comments (6)
src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts (2)
30-49: ExtractgetHighlightedLinesoutside the component.This function has zero dependencies on component state or props — it only reads
window.location.hash. Extracting it to module scope avoids recreating it on every render and makes it reusable.♻️ Suggested refactor
+const LINE_NUMBER_RANGE_REGEX = /^#L(\d+)-L(\d+)$/; +const LINE_NUMBER_SINGLE_REGEX = /^#L(\d+)$/; + +const getHighlightedLines = (): HighlightedLineRange | null => { + const hash = window.location.hash; + const rangeMatch = hash.match(LINE_NUMBER_RANGE_REGEX); + if (rangeMatch) { + const start = parseInt(rangeMatch[1], 10); + const end = parseInt(rangeMatch[2], 10); + return { start: Math.min(start, end), end: Math.max(start, end) }; + } + const singleMatch = hash.match(LINE_NUMBER_SINGLE_REGEX); + if (singleMatch) { + const line = parseInt(singleMatch[1], 10); + return { start: line, end: line }; + } + return null; +}; + export const useLineNumberNavigation = (): UseLineNumberNavigationResult => { - // ... - const getHighlightedLines = (): HighlightedLineRange | null => { - const hash = window.location.hash; - const LINE_NUMBER_RANGE_REGEX = /^#L(\d+)-L(\d+)$/; - const LINE_NUMBER_SINGLE_REGEX = /^#L(\d+)$/; - // ... - };This also moves the regex compilation to module load time rather than per-call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts` around lines 30 - 49, Move the getHighlightedLines function out of the component to module scope so it is created once and the regexes are compiled at load time: define LINE_NUMBER_RANGE_REGEX and LINE_NUMBER_SINGLE_REGEX at module scope and implement a top-level function getHighlightedLines() that reads window.location.hash and returns a HighlightedLineRange | null (preserving the existing parsing logic and use of Math.min/Math.max for ranges); then import or call this module-level getHighlightedLines from the component instead of defining it inside the component render body.
57-79: Effect without dependency array runs on every render — intentional but worth a brief note.The no-deps effect is a deliberate choice to detect programmatic hash changes that bypass the
hashchangeevent (e.g., parent clearing the hash on view switch). The ref comparison on Line 67 correctly prevents unnecessary state updates and infinite loops.The
eslint-disable-next-line react-hooks/exhaustive-depson Line 59 suppresses the lint rule. Per repo convention, consider adding a more prominent inline comment explaining why the rule is disabled, or suppressing via a dedicated eslint config override for this specific pattern.Based on learnings: In the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors (e.g., eslint-disable), remind contributors to fix the underlying issue rather than silencing the rule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts` around lines 57 - 79, The effect in useLineNumberNavigation intentionally omits a dependency array and disables react-hooks/exhaustive-deps, but the inline suppression is terse; update the comment around the React.useEffect to clearly state that the no-deps behavior is intentional, why (to detect programmatic hash changes that bypass the hashchange event), and that lastHashRef.current prevents infinite loops; include the names getHighlightedLines, setHighlightedLines and setFirstSelectedLine in the explanation for clarity, and either expand the inline eslint-disable comment to justify the exception or add a targeted eslint rule override in the repo config for this pattern so contributors know this is intentional and not a hidden lint suppression.src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts (1)
400-418: Timing-based assertion may be flaky in CI.The
expect(duration).toBeLessThan(100)check on Line 413 depends on the test runner's execution speed. While 100ms is generous for 100 synchronous state updates, CI environments under heavy load can occasionally exceed this. Consider either increasing the threshold or converting to a non-timing assertion if this becomes flaky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts` around lines 400 - 418, The timing-based assertion in the test for useLineNumberNavigation is flaky (expect(duration).toBeLessThan(100)); remove or relax it — either delete the duration check altogether and only assert the final state (highlightedLines and firstSelectedLine) after invoking handleLineClick repeatedly, or increase the threshold to a much larger value (e.g., 500ms) to accommodate CI variability; keep the rest of the test (calls to result.current.handleLineClick and assertions on highlightedLines and firstSelectedLine) unchanged.src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx (1)
88-111: Multipleeslint-disablecomments suppress lint rules rather than addressing root causes.Lines 89, 91, 99, and 108 each suppress eslint rules. Per repo convention, contributors should fix the underlying issues rather than silencing lint rules. The
console.error/console.warnreferences can be captured without triggeringno-consoleby using a typed variable pattern or restructuring the mock setup.♻️ One approach to avoid the eslint-disable comments
- // eslint-disable-next-line no-console - const originalConsoleError = console.error; - // eslint-disable-next-line no-console - const originalConsoleWarn = console.warn; - - jest.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + const errorSpy = jest.spyOn(console, 'error'); + const originalError = errorSpy.getMockImplementation() ?? errorSpy.mock.calls; + errorSpy.mockImplementation((...args: unknown[]) => { const message = typeof args[0] === 'string' ? args[0] : String(args[0] ?? ''); if (message.includes('requestAnimationFrame') || message.includes('act(...)')) { return; } - // eslint-disable-next-line `@typescript-eslint/no-unsafe-argument`, `@typescript-eslint/no-explicit-any` - originalConsoleError(...(args as any[])); + errorSpy.mock.calls.push(args); });Alternatively, store the original implementations before any spying and reference them without triggering lint rules.
Based on learnings: In the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors (e.g., eslint-disable), remind contributors to fix the underlying issue rather than silencing the rule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx` around lines 88 - 111, The test currently uses multiple eslint-disable comments to silence no-console; instead remove those eslint-disable lines and capture the original console functions in properly typed variables before spying (e.g. declare const originalConsoleError: typeof console.error = console.error and const originalConsoleWarn: typeof console.warn = console.warn), then use jest.spyOn(console, 'error').mockImplementation(...) and jest.spyOn(console, 'warn').mockImplementation(...) to call those typed originals (originalConsoleError(...), originalConsoleWarn(...)) inside the mocks; update the mockImplementation blocks for console.error and console.warn to reference the typed originals and eliminate the eslint-disable comments.src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx (1)
37-42: Potential gutter/content misalignment with dynamic line heights.The gutter cell height is fixed at
itemSize(Line 41), but if the virtualizer uses dynamic measurement for content lines (e.g., when long lines wrap), the gutter cell height won't match the actual content row height. Consider usingvirtualItem.sizeinstead, which reflects the measured size aftermeasureElementruns.♻️ Suggested fix
style={{ position: 'absolute', top: 0, left: 0, - height: `${itemSize}px`, + height: `${virtualItem.size}px`, transform: `translateY(${virtualItem.start}px)`, }}This would also allow removing the
itemSizeprop entirely fromLineNumberGutterProps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx` around lines 37 - 42, The gutter currently sets its cell height to the static prop itemSize in LineNumberGutter (style height: `${itemSize}px`), which can desync from measured row heights; change the style to use virtualItem.size (height: `${virtualItem.size}px`) so the gutter matches the virtualized content, update any references in LineNumberGutterProps to stop relying on itemSize (and remove the prop if unused), and ensure render logic that positions the cell (transform/translateY using virtualItem.start) remains unchanged.src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx (1)
57-60:TokenizedLine | null— thenullbranch is never produced.
tokenizeLinereturns a non-null object on every path (empty line, error, success). The| nullmakesrenderLine's!lineObjguard (line 288) and thetokens.length === 0branch (lines 291–293) dead code.Consider either removing
| nullfrom the type and the unreachable guards, or adding an explicitnullreturn for a case that warrants it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx` around lines 57 - 60, The TokenizedLine type includes a nullable branch but tokenizeLine never returns null, so remove the unnecessary "| null" from the TokenizedLine type and then delete the unreachable guards in renderLine (remove the "!lineObj" check and the "tokens.length === 0" branch) to reflect that tokenizeLine always produces a TokenizedLine; alternatively, if you prefer to keep the guards, modify tokenizeLine to explicitly return null in the documented case—locate the TokenizedLine type declaration, the tokenizeLine function, and renderLine (which references lineObj and tokens) to apply the chosen fix consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 330-346: The cleanup calls cancelAnimationFrame(rafId2) but rafId2
may be unassigned if rafId1's callback hasn't run yet; change rafId2's type to
allow undefined (e.g., let rafId2: number | undefined) and in the returned
cleanup function only call cancelAnimationFrame for rafId2 if it is defined
(guard with != null or typeof !== 'undefined'); keep rafId1 handling as-is and
ensure the virtualizer.scrollToIndex call remains inside the nested
requestAnimationFrame callback.
- Around line 255-262: The rendering for Array tokens uses the parent's
tokenStart for every child, causing highlight misalignment; in the token.content
branch inside renderTokenRecursive, iterate children and track a running
childOffset (based on the string length of each child - use Prism.Token content
length when child is a token or string length when string) and call
renderTokenRecursive with tokenStart + childOffset for each child so each gets
its correct absolute start; update references to renderTokenRecursive,
token.content, and any helper that computes token string lengths (used by
renderTokenString) to compute offsets accordingly.
- Line 394: Replace the raw layout-only <div
className="log-viewer__content-column"> in VirtualizedLogContent with a
PatternFly layout component (e.g., Flex or Stack) to comply with the layout
guideline; keep the same className and any ARIA/props, and preserve the
absolutely-positioned virtual item children (the inline-styled items from
`@tanstack/react-virtual`) so their direct style injection and positioning
behavior are not altered. Ensure the new PatternFly component wraps the same
children and does not introduce extra layout styles that interfere with the
absolute positioning of the virtual rows.
---
Duplicate comments:
In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx`:
- Around line 1-7: Tests in LogViewer.spec.tsx are using the bare render from
`@testing-library/react` instead of the repo utility; replace imports and usage
with renderWithQueryClientAndRouter from ~/unit-test-utils/rendering-utils.
Specifically, remove or stop using the imported render symbol and import
renderWithQueryClientAndRouter, then update all calls to render(...) to
renderWithQueryClientAndRouter(...), ensuring the same props/children are passed
so components like LogViewer get React Query and Router context during tests.
In
`@src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx`:
- Around line 158-209: Update the click interaction tests in
LineNumberGutter.spec.tsx to use userEvent instead of fireEvent: import
userEvent, create a user = userEvent.setup() inside each test (or shared
beforeEach), replace fireEvent.click(...) with await user.click(...) and make
the test functions async; ensure the mouse-event assertions that rely on
modifier keys use await user.click(element, { shiftKey: true }) and that
onLineClick assertions still check the same arguments (references:
LineNumberGutter component tests, onLineClick handler, and
screen.getByText('1'/'2'/'3') calls).
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 348-351: Remove the eslint-disable comment and include the stable
virtualizer instance returned by useVirtualizer in the effect dependency array:
update the effect in VirtualizedLogContent (the effect that currently depends on
highlightedLines and lines.length) to depend on [highlightedLines, lines.length,
virtualizer] so you can safely drop the eslint-disable-next-line
react-hooks/exhaustive-deps; keep the rest of the effect logic unchanged.
---
Nitpick comments:
In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx`:
- Around line 88-111: The test currently uses multiple eslint-disable comments
to silence no-console; instead remove those eslint-disable lines and capture the
original console functions in properly typed variables before spying (e.g.
declare const originalConsoleError: typeof console.error = console.error and
const originalConsoleWarn: typeof console.warn = console.warn), then use
jest.spyOn(console, 'error').mockImplementation(...) and jest.spyOn(console,
'warn').mockImplementation(...) to call those typed originals
(originalConsoleError(...), originalConsoleWarn(...)) inside the mocks; update
the mockImplementation blocks for console.error and console.warn to reference
the typed originals and eliminate the eslint-disable comments.
In
`@src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts`:
- Around line 400-418: The timing-based assertion in the test for
useLineNumberNavigation is flaky (expect(duration).toBeLessThan(100)); remove or
relax it — either delete the duration check altogether and only assert the final
state (highlightedLines and firstSelectedLine) after invoking handleLineClick
repeatedly, or increase the threshold to a much larger value (e.g., 500ms) to
accommodate CI variability; keep the rest of the test (calls to
result.current.handleLineClick and assertions on highlightedLines and
firstSelectedLine) unchanged.
In `@src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx`:
- Around line 37-42: The gutter currently sets its cell height to the static
prop itemSize in LineNumberGutter (style height: `${itemSize}px`), which can
desync from measured row heights; change the style to use virtualItem.size
(height: `${virtualItem.size}px`) so the gutter matches the virtualized content,
update any references in LineNumberGutterProps to stop relying on itemSize (and
remove the prop if unused), and ensure render logic that positions the cell
(transform/translateY using virtualItem.start) remains unchanged.
In `@src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts`:
- Around line 30-49: Move the getHighlightedLines function out of the component
to module scope so it is created once and the regexes are compiled at load time:
define LINE_NUMBER_RANGE_REGEX and LINE_NUMBER_SINGLE_REGEX at module scope and
implement a top-level function getHighlightedLines() that reads
window.location.hash and returns a HighlightedLineRange | null (preserving the
existing parsing logic and use of Math.min/Math.max for ranges); then import or
call this module-level getHighlightedLines from the component instead of
defining it inside the component render body.
- Around line 57-79: The effect in useLineNumberNavigation intentionally omits a
dependency array and disables react-hooks/exhaustive-deps, but the inline
suppression is terse; update the comment around the React.useEffect to clearly
state that the no-deps behavior is intentional, why (to detect programmatic hash
changes that bypass the hashchange event), and that lastHashRef.current prevents
infinite loops; include the names getHighlightedLines, setHighlightedLines and
setFirstSelectedLine in the explanation for clarity, and either expand the
inline eslint-disable comment to justify the exception or add a targeted eslint
rule override in the repo config for this pattern so contributors know this is
intentional and not a hidden lint suppression.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 57-60: The TokenizedLine type includes a nullable branch but
tokenizeLine never returns null, so remove the unnecessary "| null" from the
TokenizedLine type and then delete the unreachable guards in renderLine (remove
the "!lineObj" check and the "tokens.length === 0" branch) to reflect that
tokenizeLine always produces a TokenizedLine; alternatively, if you prefer to
keep the guards, modify tokenizeLine to explicitly return null in the documented
case—locate the TokenizedLine type declaration, the tokenizeLine function, and
renderLine (which references lineObj and tokens) to apply the chosen fix
consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
package.jsonsrc/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsxsrc/shared/components/virtualized-log-viewer/LineNumberGutter.tsxsrc/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsxsrc/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scsssrc/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsxsrc/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsxsrc/shared/components/virtualized-log-viewer/__tests__/prism-log-language.spec.tssrc/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.tssrc/shared/components/virtualized-log-viewer/index.tssrc/shared/components/virtualized-log-viewer/prism-log-language.tssrc/shared/components/virtualized-log-viewer/prism-log-theme.scsssrc/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/shared/components/virtualized-log-viewer/prism-log-language.ts
- src/shared/components/virtualized-log-viewer/prism-log-theme.scss
- src/shared/components/virtualized-log-viewer/index.ts
- src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scss
| if (Array.isArray(token.content)) { | ||
| return ( | ||
| <span key={key} className={className}> | ||
| {token.content.map((t: string | Prism.Token, i: number) => | ||
| renderTokenRecursive(t, tokenStart, lineMatches, currentMatch, i, depth + 1), | ||
| )} | ||
| </span> | ||
| ); |
There was a problem hiding this comment.
Search highlight misalignment for tokens with array content.
When token.content is an Array, every child is passed the parent's tokenStart, not each child's own offset within the line. The second, third, ... sibling tokens are mapped at the wrong absolute position, so renderTokenString computes startInToken/endInToken relative to the wrong base, placing highlights at incorrect character positions.
🐛 Proposed fix — accumulate child offset
if (Array.isArray(token.content)) {
+ let childStart = tokenStart;
return (
<span key={key} className={className}>
{token.content.map((t: string | Prism.Token, i: number) => {
- renderTokenRecursive(t, tokenStart, lineMatches, currentMatch, i, depth + 1),
+ const rendered = renderTokenRecursive(t, childStart, lineMatches, currentMatch, i, depth + 1);
+ childStart += flattenTokenText(t).length;
+ return rendered;
})}
</span>
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`
around lines 255 - 262, The rendering for Array tokens uses the parent's
tokenStart for every child, causing highlight misalignment; in the token.content
branch inside renderTokenRecursive, iterate children and track a running
childOffset (based on the string length of each child - use Prism.Token content
length when child is a token or string length when string) and call
renderTokenRecursive with tokenStart + childOffset for each child so each gets
its correct absolute start; update references to renderTokenRecursive,
token.content, and any helper that computes token string lengths (used by
renderTokenString) to compute offsets accordingly.
| // Wait for next frame to ensure virtualizer is ready after state updates | ||
| let rafId2: number; | ||
|
|
||
| const rafId1 = requestAnimationFrame(() => { | ||
| rafId2 = requestAnimationFrame(() => { | ||
| virtualizer.scrollToIndex(scrollIndex, { | ||
| align: 'center', | ||
| behavior: 'smooth', | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| // Cleanup: cancel pending animation frames on unmount or dependency change | ||
| return () => { | ||
| cancelAnimationFrame(rafId1); | ||
| cancelAnimationFrame(rafId2); | ||
| }; |
There was a problem hiding this comment.
rafId2 may be read before assignment in the cleanup.
If the effect re-runs or the component unmounts before rafId1's callback fires, rafId2 is still undefined when cancelAnimationFrame(rafId2) is called. Calling cancelAnimationFrame(undefined) is a no-op in browsers, but the type annotation let rafId2: number is inaccurate and TypeScript strict-null checks may flag it.
🛡️ Proposed fix
- let rafId2: number;
+ let rafId2: number | undefined;
const rafId1 = requestAnimationFrame(() => {
rafId2 = requestAnimationFrame(() => {
virtualizer.scrollToIndex(scrollIndex, {
align: 'center',
behavior: 'smooth',
});
});
});
return () => {
cancelAnimationFrame(rafId1);
- cancelAnimationFrame(rafId2);
+ if (rafId2 !== undefined) cancelAnimationFrame(rafId2);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`
around lines 330 - 346, The cleanup calls cancelAnimationFrame(rafId2) but
rafId2 may be unassigned if rafId1's callback hasn't run yet; change rafId2's
type to allow undefined (e.g., let rafId2: number | undefined) and in the
returned cleanup function only call cancelAnimationFrame for rafId2 if it is
defined (guard with != null or typeof !== 'undefined'); keep rafId1 handling
as-is and ensure the virtualizer.scrollToIndex call remains inside the nested
requestAnimationFrame callback.
| /> | ||
|
|
||
| {/* Log content */} | ||
| <div className="log-viewer__content-column"> |
There was a problem hiding this comment.
New layout wrapper <div> violates the PatternFly layout guideline.
The log-viewer__content-column wrapper is a new layout-only element. As per coding guidelines, raw <div> tags should not be used for layout; a PatternFly Flex or Stack component should be used instead.
The absolutely-positioned virtual items inside (lines 399–414) are a legitimate exception since @tanstack/react-virtual requires direct style injection for positioning.
♻️ Suggested change
- <div className="log-viewer__content-column">
+ <Flex className="log-viewer__content-column" direction={{ default: 'column' }}>
{virtualItems.map((virtualItem) => {
// ...
})}
- </div>
+ </Flex>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx` at
line 394, Replace the raw layout-only <div
className="log-viewer__content-column"> in VirtualizedLogContent with a
PatternFly layout component (e.g., Flex or Stack) to comply with the layout
guideline; keep the same className and any ARIA/props, and preserve the
absolutely-positioned virtual item children (the inline-styled items from
`@tanstack/react-virtual`) so their direct style injection and positioning
behavior are not altered. Ensure the new PatternFly component wraps the same
children and does not introduce extra layout styles that interfere with the
absolute positioning of the virtual rows.
3 participants
Fixes
KFLUXUI-875
Description
Add syntax highlighting support to the log viewer using Prism.js to improve readability and visual parsing of log content, with performance optimizations for large log files.
Change details:
Syntax Highlighting Features
Performance Optimizations
Implementation Details
prism-log-language.ts)prism-log-theme.scss)Type of change
Screen shots / Gifs for design review
syntax.mp4
And other functions for log viewer:
otherfunction.mp4
How to test or reproduce?
Browser conformance:
Summary by CodeRabbit