feat(KFLUXUI-874): support url hash navigation for log viewer#692
feat(KFLUXUI-874): support url hash navigation for log viewer#692testcara wants to merge 1 commit 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
|
| Cohort / File(s) | Summary |
|---|---|
Log Viewer Test Updates src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx |
Refactored test suite with improved React Testing Library practices: added act() wrapper for DOM interactions, simplified useFullscreen mock signature, suppressed console noise for benign warnings, and updated scroll event assertions to verify specific payload structure. |
Line Number Gutter Component src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx, src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx |
New component rendering clickable, accessible line numbers alongside virtual items with highlighting support; comprehensive test coverage includes rendering, positioning, highlighting, interactions, accessibility, and edge cases. |
Virtualized Log Viewer Integration src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx, src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx |
Updated VirtualizedLogContent to integrate LineNumberGutter, added useLineNumberNavigation hook integration, implemented auto-scroll to highlighted lines via double requestAnimationFrame, added line-level highlighting logic, and refactored layout to include gutter and content columns; test suite expanded with "Search Highlighting", "Line Number Gutter", and "Hash Navigation and Auto-scroll" test groups. |
Gutter Styling src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.scss |
Added GitHub-inspired line-number gutter styling with sticky positioning, dark-mode support, highlighting states, and content column layout; removed obsolete word-break rule. |
Line Navigation Hook src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts, src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts |
New hook managing hash-based line selection and range highlighting (#L123, #L10-L20 syntax), supporting single-click and shift-click interactions, with comprehensive test suite covering initialization, clicks, range selection, hash changes, and edge cases. |
Public Exports src/shared/components/virtualized-log-viewer/index.ts |
Expanded barrel exports to include LineNumberGutter component, useLineNumberNavigation hook, and associated type interfaces (LineNumberGutterProps, UseLineNumberNavigationResult, HighlightedLineRange). |
Sequence Diagram
sequenceDiagram
participant User
participant LineNumberGutter as LineNumberGutter<br/>(Component)
participant useLineNumberNav as useLineNumberNavigation<br/>(Hook)
participant BrowserHistory as Browser<br/>History/Hash
participant VirtualizedLogContent as VirtualizedLogContent<br/>(Component)
User->>LineNumberGutter: Click line number
LineNumberGutter->>useLineNumberNav: handleLineClick(lineNumber, event)
useLineNumberNav->>BrowserHistory: history.pushState + set location.hash
BrowserHistory-->>useLineNumberNav: hashchange event
useLineNumberNav->>useLineNumberNav: Parse hash, update highlightedLines
useLineNumberNav-->>VirtualizedLogContent: Return updated state (isLineHighlighted)
VirtualizedLogContent->>VirtualizedLogContent: Apply highlighting CSS class
VirtualizedLogContent->>VirtualizedLogContent: Auto-scroll to highlighted range
VirtualizedLogContent-->>User: Render highlighted line(s)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- feat(KFLUXUI-912): create customized log viewer to replace pf log viewer #626 — Main PR extends the virtualized log viewer added in this PR by introducing line-number gutter, useLineNumberNavigation hook, and VirtualizedLogContent integration.
- feat(KFLUXUI-874): support url hash navigation for log viewer #671 — Overlapping code changes adding the same virtualized log viewer features (LineNumberGutter, VirtualizedLogContent, useLineNumberNavigation) with matching tests and exports.
- Revert "feat(KFLUXUI-874): support url hash navigation for log viewer" #690 — Directly reverts the virtualized log viewer files and APIs (LineNumberGutter, useLineNumberNavigation, tests) introduced in this PR.
Suggested labels
ok-to-test
Suggested reviewers
- JoaoPedroPP
- rrosatti
- sahil143
- StanislavJochman
- rakshett
- janaki29
🚥 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-874): support url hash navigation for log viewer' clearly and concisely describes the main feature being added—hash-based URL navigation for the log viewer component, which aligns with the primary objective of implementing #L10 and #L10-L20 hash patterns for line selection and highlighting. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ 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)
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: 6
🤖 Fix all issues with AI agents
In `@src/shared/components/pipeline-run-logs/logs/LogViewer.tsx`:
- Around line 88-92: The current processedData useMemo replaces every '\r' with
'\n' which turns Windows CRLF ("\r\n") into double newlines; update the
replacement logic in the React.useMemo that computes processedData (the const
processedData = React.useMemo(...) block referencing data and ANSI_ESCAPE_REGEX)
to first normalize CRLF to a single '\n' and then convert any remaining lone
'\r' to '\n' (e.g., use a single regex that matches '\r\n' or lone '\r') before
stripping ANSI escape codes so CRLF sequences don't produce spurious blank
lines.
In
`@src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts`:
- Around line 400-418: The test uses a fragile timing assertion that can fail on
slower CI runners; update the spec in useLineNumberNavigation (the test case
calling result.current.handleLineClick) to remove or relax the performance.now()
check—either delete the duration measurement and the
expect(duration).toBeLessThan(100) assertion or replace it with a much higher
threshold or a different stable assertion (e.g., ensure final state via
highlightedLines and firstSelectedLine) while keeping the existing assertions on
result.current.highlightedLines and result.current.firstSelectedLine to verify
correct behavior after rapid clicks.
In
`@src/shared/components/virtualized-log-viewer/__tests__/useVirtualizedScroll.spec.ts`:
- Around line 1-3: Update the test to stop importing renderHook from the
deprecated package: replace the import of renderHook from
'@testing-library/react-hooks' with rendering utilities from
'@testing-library/react' (as done in the sibling test) so the test for
useVirtualizedScroll (and any uses of Virtualizer) uses renderHook from
'@testing-library/react'; ensure the import statement references renderHook from
'@testing-library/react' and remove the deprecated package import.
In
`@src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx`:
- Around line 349-363: The test may dereference listItems when scrollContainer
is null; ensure scrollContainer is asserted before querying: keep or move the
existing expect(scrollContainer).toBeInTheDocument() immediately before calling
scrollContainer.querySelectorAll, then replace
scrollContainer?.querySelectorAll('.pf-v5-c-log-viewer__list-item') with
scrollContainer.querySelectorAll(...) so listItems is always a defined NodeList;
use that NodeList.length safely when asserting and when building
itemsWithDataIndex (variables: scrollContainer, listItems, itemsWithDataIndex in
VirtualizedLogContent.spec.tsx).
In
`@src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogViewer.spec.tsx`:
- Around line 146-158: The test "should pass scroll callback to
VirtualizedLogContent" currently only asserts the local jest mock onScroll is
defined; update it to verify the callback is actually wired: render
VirtualizedLogViewer with the onScroll mock (as done), find the rendered
VirtualizedLogContent or its scroll container (e.g.,
querySelector('.pf-v5-c-log-viewer__list') or use getByTestId if available),
dispatch a scroll event on that element (or simulate scrolling via
fireEvent.scroll/element.scrollTop changes) and then assert that the onScroll
mock was called; if VirtualizedLogContent is a child component you can also spy
on its props by using a test double or React Testing Library's within to access
props to confirm onScroll was passed through.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 140-159: The effect that scrolls the virtualizer uses two nested
requestAnimationFrame calls but doesn't cancel them on unmount; change the
useEffect in VirtualizedLogContent to store the returned RAF ids (e.g., raf1 and
raf2), and in the cleanup callback call cancelAnimationFrame for any pending ids
and avoid calling virtualizer.scrollToIndex if the component is unmounted or if
virtualizer is not defined; keep the dependency on highlightedLines and ensure
the cleanup clears both nested frames to prevent scrollToIndex running on a
stale component or virtualizer reference.
🧹 Nitpick comments (16)
src/shared/components/pipeline-run-logs/logs/__tests__/useLogViewerSearch.spec.ts (1)
1-1: Use@testing-library/reactinstead of the deprecated@testing-library/react-hooks.
@testing-library/react-hooksis deprecated —renderHookandactare available directly from@testing-library/reactsince v13.1.0. The other test file in this PR (useAutoScrollWithResume.spec.ts) already uses the correct import.Proposed fix
-import { renderHook, act } from '@testing-library/react-hooks'; +import { renderHook, act } from '@testing-library/react';src/shared/components/virtualized-log-viewer/LineNumberGutter.tsx (1)
28-61: TheFlexwithdirection='column'has no effect on absolutely positioned children.The children are all
position: absolute, so the Flex column direction is inert. Consider using a plain<div>with thelog-viewer__gutterclass, or simply removedirection={{ default: 'column' }}since it's misleading. The raw<div>usage for inner cells is acceptable here given the pixel-precise absolute positioning required by virtualization.src/shared/components/virtualized-log-viewer/useVirtualizedScroll.ts (2)
37-56:onScrollin the dependency array may cause spurious effect executions.If the parent passes an unstable
onScrollcallback (new function reference each render), this effect re-fires on every render. SinceVirtualizedLogViewerpassesonScrolldirectly from props, stability depends on the consumer. Consider documenting this expectation or using a ref to hold the latest callback.Ref-based approach to decouple callback identity from effect trigger
+ const onScrollRef = React.useRef(onScroll); + onScrollRef.current = onScroll; + React.useEffect(() => { - if (!onScroll) return; + if (!onScrollRef.current) return; const scrollDirection: 'forward' | 'backward' = scrollOffset >= prevScrollOffset.current ? 'forward' : 'backward'; const scrollUpdateWasRequested = scrollToIndexRef.current === undefined; prevScrollOffset.current = scrollOffset; - onScroll({ + onScrollRef.current({ scrollDirection, scrollOffset, scrollUpdateWasRequested, }); - }, [scrollOffset, onScroll]); + }, [scrollOffset]);
44-47: The invertedscrollUpdateWasRequestedsemantics are documented but still confusing.The comment explains the inversion, which is good. Consider renaming the field in the interface (e.g.,
isUserScroll) to avoid perpetuating confusion, or at minimum ensure consuming code has matching comments.src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx (1)
158-209: Consider usinguserEvent.setup()for click interaction tests.The coding guidelines prefer
userEvent.setup()with async interactions over synchronousfireEvent. WhilefireEventworks here, switching touserEventwould be more consistent with the project conventions and better simulate real browser behavior (e.g., focus management).As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts (1)
4-9: MockReact.MouseEventviaObject.definePropertyis fragile but functional.The
createMouseEventhelper works for accessingevent.shiftKeybut would fail if the hook accessed other React synthetic event properties (e.g.,nativeEvent,persist). This is fine for the current usage. Just noting for future maintainability.src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogViewer.spec.tsx (1)
23-35:offsetHeight/offsetWidthoverrides onHTMLElement.prototypeare not restored inafterEach.These global overrides persist across tests in the same file and potentially across other test files in the same worker. Consider saving and restoring original property descriptors in
afterEachto prevent leakage.Save and restore property descriptors
+ let originalOffsetHeight: PropertyDescriptor | undefined; + let originalOffsetWidth: PropertyDescriptor | undefined; + beforeEach(() => { jest.clearAllMocks(); + originalOffsetHeight = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'offsetHeight'); + originalOffsetWidth = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'offsetWidth'); + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { configurable: true, value: 600, }); Object.defineProperty(HTMLElement.prototype, 'offsetWidth', { configurable: true, value: 800, }); }); + + afterEach(() => { + if (originalOffsetHeight) { + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', originalOffsetHeight); + } + if (originalOffsetWidth) { + Object.defineProperty(HTMLElement.prototype, 'offsetWidth', originalOffsetWidth); + } + });src/shared/components/virtualized-log-viewer/VirtualizedLogViewer.tsx (1)
38-47: The semantic naming ofcurrentSearchedItemCountis misleading but functionally correct. While the variable name suggests a "count" (implying 1-based numbering like "1st match"), it is actually used as a 0-based array index to accesssearchedWordIndexes. The implementation is correct and tests confirm proper behavior, but the naming could be improved for clarity. Consider renaming tocurrentMatchIndexfor consistency with how it's used locally inVirtualizedLogViewer.tsx.src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx (1)
123-128: Misleading test name.The test name says "should highlight when search text is less than 2 characters" but the test actually verifies that single-char search does produce highlights (
marks.length === 3). Consider renaming to clarify the intent, e.g., "should highlight matches even with single-character search text".src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx (3)
57-61: Suppressed lint rule — consider fixing the underlying cause.Line 158–159 disables
react-hooks/exhaustive-depsto omitvirtualizerfrom the dependency array. The comment states the reference is stable fromuseVirtualizer, which is correct for@tanstack/react-virtual. However, per repository conventions, suppressing lint rules should be a last resort.If the virtualizer reference is truly stable, adding it to the deps array would be harmless and would remove the need for the suppression.
98-134:renderLineis recreated on every render.This function closes over
searchText,escapedSearchText,searchRegex, andcurrentSearchMatch. It's called once per visible virtual item in themapon Line 203. Wrapping it inuseCallbackwith those dependencies would avoid recreating it on renders where only unrelated state (e.g., scroll offset) changes.This is a low-priority optimization since the virtual items themselves aren't memoized.
46-61:return truein addEventListener callback has no effect.Line 54 returns
true, butaddEventListenercallbacks don't use the return value for propagation control. The actual suppression is handled bypreventDefault()andstopImmediatePropagation()on lines 52–53. Thereturn trueis dead code.Proposed cleanup
event.preventDefault(); event.stopImmediatePropagation(); - return true; + return; }src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx (2)
88-111: Console suppression may hide real issues; multiple lint suppressions should be addressed.Blanket suppression of
requestAnimationFrameandact(...)errors (Line 96) could mask genuine missingact()wrapping issues in tests. Additionally, there are sixeslint-disablecomments in this block alone (lines 89, 91, 99, 100, 108, 109).Per repository conventions, prefer fixing the underlying issues rather than silencing lint rules. For the
act()warnings specifically, wrapping the renders/state updates inact()(already imported on Line 1) is the proper fix.Based on learnings: "In the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors (e.g., eslint-disable, ts-ignore, ts-expect-error), remind contributors to fix the underlying issue rather than silencing the rule."
364-409: Fullscreen mock returns a ref object instead of a callback ref.
useFullscreenreturns[boolean, (node: T) => void, () => void, boolean]where the second element is a callback ref. The mocks on lines 370 and 386 pass{ current: document.createElement('div') }(a ref object) instead. This works because React accepts both forms, but it doesn't accurately represent the hook's contract.src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts (2)
57-79: Effect running on every render with lint suppression.The
eslint-disableon Line 59 suppressesreact-hooks/exhaustive-depsfor an effect with no dependency array, meaning it fires on every render. The ref comparison on Line 67 keeps it cheap, but this is an unconventional pattern.Per repository conventions, prefer fixing the underlying issue rather than silencing lint rules. A cleaner alternative is to use only the
hashchangeevent listener (lines 82–95) combined with apopstatelistener for browser navigation, which would eliminate the need for a render-time poll.Based on learnings: "In the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors (e.g., eslint-disable, ts-ignore, ts-expect-error), remind contributors to fix the underlying issue rather than silencing the rule."
28-49: Consider extractinggetHighlightedLinesas a module-level utility.This function has zero React dependencies — it only reads
window.location.hashand returns a pure result. Defining it inside the hook means it's recreated on every render. Moving it outside the component scope would make it testable independently and slightly reduce per-render allocations.Proposed extraction
+const getHighlightedLines = (): HighlightedLineRange | null => { + const hash = window.location.hash; + const rangeMatch = hash.match(/^#L(\d+)-L(\d+)$/); + 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(/^#L(\d+)$/); + if (singleMatch) { + const line = parseInt(singleMatch[1], 10); + return { start: line, end: line }; + } + return null; +}; + export const useLineNumberNavigation = (): UseLineNumberNavigationResult => { const [firstSelectedLine, setFirstSelectedLine] = React.useState<number | null>(null); - - const getHighlightedLines = (): HighlightedLineRange | null => { - const hash = window.location.hash; - ... - };
src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts
Show resolved
Hide resolved
src/shared/components/virtualized-log-viewer/__tests__/useVirtualizedScroll.spec.ts
Show resolved
Hide resolved
src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogContent.spec.tsx
Show resolved
Hide resolved
src/shared/components/virtualized-log-viewer/__tests__/VirtualizedLogViewer.spec.tsx
Show resolved
Hide resolved
src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx
Outdated
Show resolved
Hide resolved
rrosatti
left a comment
rrosatti
left a comment
There was a problem hiding this comment.
I left a few comments, but overall code looks really good. Nice job :)
| const hash = window.location.hash; | ||
|
|
||
| // Match #L123-L456 (range) | ||
| const rangeMatch = hash.match(/^#L(\d+)-L(\d+)$/); |
There was a problem hiding this comment.
nit: I'd define the regex in a const, something like const LINE_NUMBER_RANGE_REGEX = /^#L(\d+)-L(\d+)$/; :)
| } | ||
|
|
||
| // Match #L123 (single line) | ||
| const singleMatch = hash.match(/^#L(\d+)$/); |
There was a problem hiding this comment.
nit: same here, something like LINE_NUMBER_SINGLE_REGEX
| const { highlightedLines, handleLineClick, isLineHighlighted } = useLineNumberNavigation(); | ||
|
|
||
| // Scroll to highlighted lines when hash changes or on initial load | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
I gave it a try, but it's not navigating to the selected start line on initial load 🤔
Screen.Recording.2026-02-09.at.9.33.02.mov
There was a problem hiding this comment.
curious question: wouldn't it be possible to use useVirtualizedScroll to handle this scroll to index logic? 🤔
There was a problem hiding this comment.
Nice catch! Thank you for testing this :-). fixed
Assisted-by: Claude
testcara
force-pushed
the
line-number
branch
from
8009308 to
9550e23
Compare
testcara
requested review from
JoaoPedroPP and
rakshett
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts (1)
400-413:⚠️ Potential issue | 🟡 MinorTiming-based performance assertion is flaky in CI.
Line 404-413’s duration check depends on host speed; consider removing or relaxing it while keeping the final state assertions.Suggested change
- const start = performance.now(); act(() => { for (let i = 1; i <= 100; i++) { result.current.handleLineClick(i, createMouseEvent(false)); } }); - const duration = performance.now() - start; - - // Should complete 100 clicks in reasonable time (< 100ms in test environment) - expect(duration).toBeLessThan(100);🤖 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 - 413, The timing-based assertion in the test for useLineNumberNavigation ("should efficiently handle rapid hash changes") is flaky; remove the duration-based expect (expect(duration).toBeLessThan(100)) and instead assert the final state after the simulated rapid clicks (e.g., that result.current reflects the last clicked line or that the URL/hash was updated appropriately by handleLineClick). Update the test to call result.current.handleLineClick in the loop, then after act() verify the final selected line/hash and any side effects, keeping useLineNumberNavigation and handleLineClick as the referenced symbols to locate the logic.
🧹 Nitpick comments (3)
src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx (3)
224-243: Scroll test assertion is trivially weak.The post-scroll assertion (
expect(scrollContainer).toBeInTheDocument()) was already true before theact()block and proves nothing about scroll behavior. If this test is intended to verify the component handles scroll without crashing, a comment to that effect is clearer than a meaningless DOM assertion. If it's meant to verifyonScrollis invoked, the providedonScrollmock at line 225 is never asserted.♻️ Suggested fix
if (scrollContainer) { act(() => { scrollContainer.scrollTop = 100; scrollContainer.dispatchEvent(new Event('scroll')); }); } - // Component should handle scroll events - expect(scrollContainer).toBeInTheDocument(); + // Component should invoke onScroll without throwing + expect(onScroll).toHaveBeenCalled();🤖 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 224 - 243, The test's post-scroll assertion is meaningless; update the test to assert the scroll handler was invoked instead of re-checking DOM presence: keep using LogViewer with allowAutoScroll and the onScroll jest.fn(), perform the act() that sets scrollContainer.scrollTop and dispatches the 'scroll' event, then replace the final expect(scrollContainer).toBeInTheDocument() with expect(onScroll).toHaveBeenCalled() (or expect(onScroll).toHaveBeenCalledTimes(1)) to verify the component handled the scroll; reference LogViewer, defaultProps, allowAutoScroll, onScroll, scrollContainer and act when making the change.
88-111: Suppressingact(...)warnings masks real async state issues.Filtering out
act(...)console errors globally inbeforeEachmeans any test that triggers a state update outside ofact()will silently swallow the warning. The correct fix is to wrap the offending interactions inact()(already done for scroll at lines 235-238), not to suppress the signal globally. This makes it harder to catch new violations introduced in future tests.If
requestAnimationFrameis the root cause (common in JSDOM), the correct suppression target iswindow.requestAnimationFrame, not the React warning:♻️ Suggested alternative
+ // Provide a synchronous rAF stub so virtualization doesn't warn + jest.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => { + cb(0); + return 0; + }); - jest.spyOn(console, 'error').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[])); - }); - - jest.spyOn(console, 'warn').mockImplementation((...args: unknown[]) => { - const message = typeof args[0] === 'string' ? args[0] : String(args[0] ?? ''); - if (message.includes('mobx-react-lite')) { - return; - } - // eslint-disable-next-line `@typescript-eslint/no-unsafe-argument`, `@typescript-eslint/no-explicit-any` - originalConsoleWarn(...(args as any[])); - });🤖 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 current test setup globally swallows React's "act(...)" warnings by filtering them in the console.error mock (see jest.spyOn(console, 'error') and originalConsoleError), which hides real async state update issues; remove the check that returns when message.includes('act(...)') from the console.error mock and instead ensure the specific test interactions causing the warning are wrapped with React's act() (the scroll interaction already wrapped around that area), or if JSDOM's requestAnimationFrame is the true noise, change suppression to mock window.requestAnimationFrame only (leave the console.warn mock for 'mobx-react-lite' as-is).
444-448: Prefer semantic query overqueryByTestId.Per project guidelines,
getByRole/getByLabelTextshould be prioritized overqueryByTestId. If the resume button has an accessible name or role, use that instead.- const resumeButton = queryByTestId('resume-log-stream'); + const resumeButton = screen.queryByRole('button', { name: /resume/i });🤖 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 444 - 448, In the 'should not show resume button initially' test for LogViewer, replace the non-semantic queryByTestId('resume-log-stream') with a semantic query (e.g. screen.queryByRole('button', { name: /resume/i }) or screen.queryByLabelText(/resume/i>) to reflect the button's accessible name/label); keep the assertion expect(...).not.toBeInTheDocument() and update any test setup (defaultProps) if needed to ensure the resume control exposes an accessible name so the semantic query can find it when present.
🤖 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/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx`:
- Around line 586-596: The test in LogViewer.spec.tsx couples to react-window
internals by asserting the full onScroll payload including
scrollUpdateWasRequested; update the test for LogViewer to avoid
implementation-specific fields by either asserting simply that onScroll was
called (expect(onScroll).toHaveBeenCalled()) or using a partial match for only
the meaningful fields (e.g.,
expect(onScroll).toHaveBeenCalledWith(expect.objectContaining({ scrollOffset: 0,
scrollDirection: 'forward' }))), keeping defaultProps and the LogViewer render
the same.
In
`@src/shared/components/virtualized-log-viewer/__tests__/LineNumberGutter.spec.tsx`:
- Around line 158-209: Replace direct DOM event usage with userEvent: import
userEvent from '@testing-library/user-event', make the three affected tests
async, create const user = userEvent.setup() inside each test, and replace
fireEvent.click(...) and manual dispatchEvent(...) with await
user.click(element) (use await user.click(element, { shiftKey: true }) for the
shiftKey case). Update the tests "should call onLineClick when line number is
clicked", "should prevent default link behavior on click", and "should pass
mouse event to onLineClick handler" to use this pattern when interacting with
the LineNumberGutter rendered links.
In
`@src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts`:
- Around line 1-2: Replace the relative import in the test with the project
absolute path alias: change the import of useLineNumberNavigation (referenced in
this spec) to use the `~` alias (e.g. import from
'~/shared/components/virtualized-log-viewer/useLineNumberNavigation') so the
test uses the configured absolute path alias instead of a relative path.
In `@src/shared/components/virtualized-log-viewer/VirtualizedLogContent.tsx`:
- Around line 3-4: The imports in VirtualizedLogContent.tsx use relative paths;
change them to the repo's path-alias style: replace "./LineNumberGutter" with
the appropriate alias import (e.g., "~/components/LineNumberGutter" or whichever
alias maps to that component) and "./useLineNumberNavigation" with its alias
(e.g., "~/components/useLineNumberNavigation" or the correct alias for hooks).
Update the import statements referencing the symbols LineNumberGutter and
useLineNumberNavigation accordingly so they follow the configured absolute path
aliases.
- Around line 170-171: Remove the eslint-disable comment and fix the dependency
list of the useEffect in the VirtualizedLogContent component: include every
variable and function referenced inside that effect instead of suppressing the
rule (for example, add highlightedLines plus the full lines array (lines), any
refs or handlers used such as listRef or containerRef, and any helper functions
like scrollToLine/scrollToHighlightedLines or onHighlightChange) so the effect
re-runs correctly when any of its inputs change; ensure you import/declare those
symbols or wrap stable functions with useCallback/useRef if you want them
excluded from the dependency array.
---
Duplicate comments:
In
`@src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts`:
- Around line 400-413: The timing-based assertion in the test for
useLineNumberNavigation ("should efficiently handle rapid hash changes") is
flaky; remove the duration-based expect (expect(duration).toBeLessThan(100)) and
instead assert the final state after the simulated rapid clicks (e.g., that
result.current reflects the last clicked line or that the URL/hash was updated
appropriately by handleLineClick). Update the test to call
result.current.handleLineClick in the loop, then after act() verify the final
selected line/hash and any side effects, keeping useLineNumberNavigation and
handleLineClick as the referenced symbols to locate the logic.
---
Nitpick comments:
In `@src/shared/components/pipeline-run-logs/logs/__tests__/LogViewer.spec.tsx`:
- Around line 224-243: The test's post-scroll assertion is meaningless; update
the test to assert the scroll handler was invoked instead of re-checking DOM
presence: keep using LogViewer with allowAutoScroll and the onScroll jest.fn(),
perform the act() that sets scrollContainer.scrollTop and dispatches the
'scroll' event, then replace the final
expect(scrollContainer).toBeInTheDocument() with
expect(onScroll).toHaveBeenCalled() (or
expect(onScroll).toHaveBeenCalledTimes(1)) to verify the component handled the
scroll; reference LogViewer, defaultProps, allowAutoScroll, onScroll,
scrollContainer and act when making the change.
- Around line 88-111: The current test setup globally swallows React's
"act(...)" warnings by filtering them in the console.error mock (see
jest.spyOn(console, 'error') and originalConsoleError), which hides real async
state update issues; remove the check that returns when
message.includes('act(...)') from the console.error mock and instead ensure the
specific test interactions causing the warning are wrapped with React's act()
(the scroll interaction already wrapped around that area), or if JSDOM's
requestAnimationFrame is the true noise, change suppression to mock
window.requestAnimationFrame only (leave the console.warn mock for
'mobx-react-lite' as-is).
- Around line 444-448: In the 'should not show resume button initially' test for
LogViewer, replace the non-semantic queryByTestId('resume-log-stream') with a
semantic query (e.g. screen.queryByRole('button', { name: /resume/i }) or
screen.queryByLabelText(/resume/i>) to reflect the button's accessible
name/label); keep the assertion expect(...).not.toBeInTheDocument() and update
any test setup (defaultProps) if needed to ensure the resume control exposes an
accessible name so the semantic query can find it when present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/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__/useLineNumberNavigation.spec.tssrc/shared/components/virtualized-log-viewer/index.tssrc/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/VirtualizedLogViewer.scss
- src/shared/components/virtualized-log-viewer/tests/VirtualizedLogContent.spec.tsx
- src/shared/components/virtualized-log-viewer/index.ts
- src/shared/components/virtualized-log-viewer/useLineNumberNavigation.ts
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
src/shared/components/virtualized-log-viewer/__tests__/useLineNumberNavigation.spec.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
==========================================
+ Coverage 85.94% 86.82% +0.88%
==========================================
Files 770 772 +2
Lines 58833 59090 +257
Branches 4779 6981 +2202
==========================================
+ Hits 50565 51306 +741
+ Misses 8217 7612 -605
- Partials 51 172 +121
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:
|
rrosatti
left a comment
rrosatti
left a comment
There was a problem hiding this comment.
LGTM! tested locally it's working fine. Amazing job! :)
| const { highlightedLines, handleLineClick, isLineHighlighted } = useLineNumberNavigation(); | ||
|
|
||
| // Scroll to highlighted lines when hash changes or on initial load | ||
| React.useEffect(() => { |
2 participants
Fixes
KFLUXUI-874
Description
Add hash-based line navigation to virtualized log viewer, enabling users to share URLs with highlighted log lines.
Details:
#L10(single line) and#L10-L20(range) formatsImplementation:
useLineNumberNavigationshook manages hash state and user interactionsLineNumberGuttercomponent renders clickable line numbersVirtualizedLogContentto highlight selected linesrequestAnimationFrameType of change
Screen shots / Gifs for design review
hash-nav.mp4
How to test or reproduce?
BTW, the url navigation is quite similar with github usages to share codes, so test with your experiences.
Browser conformance:
Summary by CodeRabbit
Release Notes
New Features
Tests