Skip to content

[Dashboard] Add locate mode to log viewer with match navigation#63504

Open
Rruop wants to merge 1 commit into
ray-project:masterfrom
Rruop:dashboard-log-locate-mode
Open

[Dashboard] Add locate mode to log viewer with match navigation#63504
Rruop wants to merge 1 commit into
ray-project:masterfrom
Rruop:dashboard-log-locate-mode

Conversation

@Rruop
Copy link
Copy Markdown

@Rruop Rruop commented May 19, 2026

Why are these changes needed?

The current log search in Ray Dashboard only supports filter mode, which shows only lines matching the keyword. This makes it difficult to understand the context around matching lines — for example, what happened immediately before/after an error.

This PR adds a "Locate" mode (as the new default) that:

  • Shows all log lines with keyword matches highlighted in-place
  • Displays a match count indicator (e.g., "3 / 15")
  • Supports prev/next navigation between matches via buttons and keyboard shortcuts
  • Keeps the existing filter mode available via a toggle switch

Motivation

When debugging distributed Ray applications, users often search for keywords like ERROR, timeout, or actor names in logs. With filter-only behavior:

  1. Context is lost — the error line is shown without the preceding lines that explain the cause
  2. Navigation is manual — users must clear the search and scroll to see surrounding lines
  3. No match count — users cannot tell how many matches exist or track their position

This is the standard search behavior in tools like VS Code, Chrome DevTools, and less.

UI Design

┌─────────────────────────────────────────────────────────────────┐
│ [🔍 Keyword]  [3 / 15] [▲] [▼]   Filter Mode: [OFF]  Reverse  │
├─────────────────────────────────────────────────────────────────┤
│  line 41: normal log line                                       │
│  line 42: normal log line                                       │
│▶ line 43: log with [KEYWORD] highlighted  ← current match      │
│  line 44: normal log line                                       │
│  line 45: another [KEYWORD] match (subtle highlight)            │
│  line 46: normal log line                                       │
└─────────────────────────────────────────────────────────────────┘

Keyboard shortcuts:

  • Enter — Jump to next match
  • Shift+Enter — Jump to previous match

Related issue number

N/A (new feature)

Checks

  • I've signed the CLA
  • I've run scripts/format.sh to lint the changes in this PR
  • I've included any doc changes needed for https://docs.ray.io/en/master/
  • I've made sure the tests are passing
  • Testing Strategy:
    • TypeScript compilation passes (npx tsc --noEmit)
    • Manual testing with Ray Dashboard (screenshots to be added)

The current log search only supports filter mode which hides non-matching
lines, losing important context. This adds a "locate" mode (new default)
that shows all lines with keyword highlighting and prev/next navigation.

- Add searchMode prop to LogVirtualView: "locate" (default) or "filter"
- Locate mode displays all lines, highlights matches with background color
- Add match count indicator (e.g., "3 / 15") and prev/next buttons
- Support Enter/Shift+Enter keyboard shortcuts for navigation
- Keep existing filter mode accessible via a toggle switch

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a 'locate' search mode to the log viewer, enabling users to navigate through keyword matches within the full log context rather than just filtering lines. Key additions include navigation controls (up/down buttons and keyboard shortcuts), match highlighting, and a match counter. Feedback focuses on fixing navigation and auto-scroll logic to correctly handle the 'revert' prop for reversed logs. Performance optimizations were also suggested to avoid redundant regex processing and to clean up repeated index calculations in the item renderer.

Comment on lines +260 to +262
const logIndex = matchIndices[matchIndex];
if (el.current) {
el.current.scrollToItem(logIndex, "center");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The scrollToItem call does not account for the revert prop. When logs are displayed in reverse order, the list index (visual position) is the inverse of the log array index. This will cause the viewer to scroll to the wrong line when navigating matches in reverse mode.

          const logIndex = matchIndices[matchIndex];
          const listIndex = revert ? logs.length - 1 - logIndex : logIndex;
          if (el.current) {
            el.current.scrollToItem(listIndex, "center");
          }

Comment on lines +421 to +423
setTimeout(() => {
el.current?.scrollToItem(indices[0], "center");
}, 0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the navigation logic, the initial auto-scroll to the first match needs to account for the revert prop to ensure it scrolls to the correct visual position when logs are reversed.

              setTimeout(() => {
                const listIndex = revert ? filtered.length - 1 - indices[0] : indices[0];
                el.current?.scrollToItem(listIndex, "center");
              }, 0);

Comment on lines +381 to +385
const allLines = originContent.map((e, i) => ({
i,
origin: e,
time: (e?.match(timeReg) || [""])[0],
}));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Regex matching for timestamps is performed on every line even when no time filters (startTime or endTime) are active. For large log files (up to 50,000 lines), this unnecessary processing can impact performance. It's better to only perform the match if at least one time filter is provided.

Suggested change
const allLines = originContent.map((e, i) => ({
i,
origin: e,
time: (e?.match(timeReg) || [""])[0],
}));
const allLines = originContent.map((e, i) => ({
i,
origin: e,
time: (startTime || endTime) ? (e?.match(timeReg) || [""])[0] : "",
}));

Comment on lines +279 to +281
const { i, origin } = logs[revert ? logs.length - 1 - index : index];
const isMatch = searchMode === "locate" && keywords && matchIndexSet.has(revert ? logs.length - 1 - index : index);
const isCurrentMatch = isMatch && matchIndices[currentMatchIndex] === (revert ? logs.length - 1 - index : index);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The log index calculation revert ? logs.length - 1 - index : index is repeated three times in the itemRenderer. Calculating it once improves readability and slightly improves performance.

Suggested change
const { i, origin } = logs[revert ? logs.length - 1 - index : index];
const isMatch = searchMode === "locate" && keywords && matchIndexSet.has(revert ? logs.length - 1 - index : index);
const isCurrentMatch = isMatch && matchIndices[currentMatchIndex] === (revert ? logs.length - 1 - index : index);
const logIndex = revert ? logs.length - 1 - index : index;
const { i, origin } = logs[logIndex];
const isMatch = searchMode === "locate" && keywords && matchIndexSet.has(logIndex);
const isCurrentMatch = isMatch && matchIndices[currentMatchIndex] === logIndex;

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit aff6177. Configure here.

setCurrentMatchIndex(matchIndex);
const logIndex = matchIndices[matchIndex];
if (el.current) {
el.current.scrollToItem(logIndex, "center");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scroll-to-match ignores reverse mode, navigates to wrong position

Medium Severity

scrollToMatch passes logIndex (an index into the logs array) directly to scrollToItem, but when revert is true, the visual row index differs from the array index. The itemRenderer maps visual index to array index via logs[revert ? logs.length - 1 - index : index], so scrolling to array index N requires visual index logs.length - 1 - N in reverse mode. Without this conversion, match navigation scrolls to the wrong row when Reverse is enabled.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit aff6177. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant