feat: implement cell match navigation via Enter key#500
Conversation
|
@yukina3230 is attempting to deploy a commit to the zknpr's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a filter match navigation feature that allows users to cycle through matching cells using Enter and Shift+Enter in both global and column filters, complete with a match counter UI and cell highlighting. The review feedback highlights several critical improvements: handling MouseEvent arguments when filters are triggered via click events, preventing concurrent data loads during rapid filter submissions, calling event.preventDefault() on Enter keydown events, defensively handling non-string return values from cell formatting to avoid runtime errors, and correcting a CSS z-index conflict that causes active matches to override pinned cell layering.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export async function applyGlobalFilter(direction = 1) { | ||
| const value = document.getElementById('filterInput').value; | ||
| if (value !== state.filterQuery) { | ||
| state.filterQuery = value; | ||
| state.currentPageIndex = 0; | ||
| loadTableData(); | ||
| resetMatchNav(); | ||
| await loadTableData(); | ||
| persistState(); | ||
| }, 300); | ||
| } | ||
| navigateMatches('global', direction); | ||
| } |
There was a problem hiding this comment.
When applyGlobalFilter is registered as a click event listener on btnApplyFilter, the browser passes the MouseEvent object as the first argument (direction). This causes direction to be an object, leading to NaN when performing index calculations in navigateMatches. Additionally, we should defensively check if filterInput exists and prevent concurrent re-fetches if state.isLoadingData is true.
export async function applyGlobalFilter(direction = 1) {
if (state.isLoadingData) return;
const input = document.getElementById('filterInput');
if (!input) return;
const value = input.value;
const dir = typeof direction === 'number' ? direction : 1;
if (value !== state.filterQuery) {
state.filterQuery = value;
state.currentPageIndex = 0;
resetMatchNav();
await loadTableData();
persistState();
}
navigateMatches('global', dir);
}| export async function applyColumnFilter(columnName, direction = 1) { | ||
| const input = document.querySelector(`.column-filter[data-column="${columnName}"]`); | ||
| if (input) { | ||
| if (!input) return; | ||
|
|
||
| const changed = input.value !== (state.columnFilters[columnName] || ''); | ||
| if (changed) { | ||
| state.columnFilters[columnName] = input.value; | ||
| state.currentPageIndex = 0; | ||
| loadTableData(); | ||
| resetMatchNav(); | ||
| await loadTableData(); | ||
| // loadTableData() rebuilds the header, so the input we focused is gone. | ||
| // Re-focus the freshly rendered one and place the caret at the end. | ||
| const newInput = document.querySelector(`.column-filter[data-column="${columnName}"]`); | ||
| if (newInput) { | ||
| newInput.focus(); | ||
| newInput.setSelectionRange(newInput.value.length, newInput.value.length); | ||
| } | ||
| } | ||
| navigateMatches(columnName, direction); | ||
| } |
There was a problem hiding this comment.
To prevent race conditions and redundant database queries when the user rapidly presses Enter, we should check if state.isLoadingData is true and return early. We should also ensure direction is a number to prevent any potential type coercion issues.
export async function applyColumnFilter(columnName, direction = 1) {
if (state.isLoadingData) return;
const input = document.querySelector(`.column-filter[data-column="${columnName}"]`);
if (!input) return;
const dir = typeof direction === 'number' ? direction : 1;
const changed = input.value !== (state.columnFilters[columnName] || '');
if (changed) {
state.columnFilters[columnName] = input.value;
state.currentPageIndex = 0;
resetMatchNav();
await loadTableData();
// loadTableData() rebuilds the header, so the input we focused is gone.
// Re-focus the freshly rendered one and place the caret at the end.
const newInput = document.querySelector(`.column-filter[data-column="${columnName}"]`);
if (newInput) {
newInput.focus();
newInput.setSelectionRange(newInput.value.length, newInput.value.length);
}
}
navigateMatches(columnName, dir);
}| export function onFilterEnter(event) { | ||
| // Enter jumps to the next match, Shift+Enter to the previous one. | ||
| if (event.key === 'Enter') applyGlobalFilter(event.shiftKey ? -1 : 1); | ||
| } |
There was a problem hiding this comment.
It is a good practice to call event.preventDefault() when handling the Enter keydown event to prevent any default browser actions (such as accidental form submissions or unexpected page reloads).
export function onFilterEnter(event) {
// Enter jumps to the next match, Shift+Enter to the previous one.
if (event.key === 'Enter') {
event.preventDefault();
applyGlobalFilter(event.shiftKey ? -1 : 1);
}
}| export function onColumnFilterKeydown(event, columnName) { | ||
| if (event.key === 'Enter') { | ||
| applyColumnFilter(columnName); | ||
| } | ||
| // Enter jumps to the next match, Shift+Enter to the previous one. | ||
| if (event.key === 'Enter') applyColumnFilter(columnName, event.shiftKey ? -1 : 1); | ||
| } |
There was a problem hiding this comment.
Call event.preventDefault() when handling the Enter keydown event to prevent default browser actions.
export function onColumnFilterKeydown(event, columnName) {
// Enter jumps to the next match, Shift+Enter to the previous one.
if (event.key === 'Enter') {
event.preventDefault();
applyColumnFilter(columnName, event.shiftKey ? -1 : 1);
}
}| const value = getCellValue(row, colIdx); | ||
| const text = formatCellValueAsText(value, col.type, state.dateFormat, col.name); | ||
| if (text.toLowerCase().includes(term)) { | ||
| matches.push({ rowIdx, colIdx }); | ||
| } |
There was a problem hiding this comment.
If formatCellValueAsText returns a non-string value (e.g., null, undefined, or a number), calling .toLowerCase() directly on it will throw a TypeError and crash the application. We should defensively convert the formatted value to a string.
const value = getCellValue(row, colIdx);
const rawText = formatCellValueAsText(value, col.type, state.dateFormat, col.name);
const text = typeof rawText === 'string' ? rawText : String(rawText || '');
if (text.toLowerCase().includes(term)) {
matches.push({ rowIdx, colIdx });
}| .data-cell.active-match-cell { | ||
| outline: 2px solid var(--accent-color); | ||
| outline-offset: -2px; | ||
| z-index: 3; | ||
| } |
There was a problem hiding this comment.
Since .data-cell.active-match-cell has z-index: 3 and is declared later in the stylesheet than .data-cell.pinned (which has z-index: 5), it will override the z-index of pinned cells to 3. This causes pinned cells to lose their correct stacking order when they become the active match. We should add a rule to preserve the z-index of pinned cells.
.data-cell.active-match-cell {
outline: 2px solid var(--accent-color);
outline-offset: -2px;
z-index: 3;
}
.data-cell.pinned.active-match-cell {
z-index: 5;
}
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cfd7ddb5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function initGridControls() { | ||
| document.getElementById('filterInput')?.addEventListener('keyup', onFilterChange); | ||
| document.getElementById('filterInput')?.addEventListener('keydown', onFilterEnter); | ||
| document.getElementById('btnApplyFilter')?.addEventListener('click', applyGlobalFilter); |
There was a problem hiding this comment.
When the toolbar Search button is clicked, addEventListener passes the MouseEvent into applyGlobalFilter as the direction argument. That makes a fresh click search start at the last match (MouseEvent !== 1), and repeated clicks hit currentIndex + direction in navigateMatches, coercing the event into a string and producing NaN, so the counter/active cell stop working. Use a wrapper such as () => applyGlobalFilter(1) for this click path.
Useful? React with 👍 / 👎.
| ? state.matchNav.matches[state.matchNav.currentIndex] | ||
| : null; | ||
|
|
There was a problem hiding this comment.
This renders an active match solely from state.matchNav, but selecting another table clears filterQuery/columnFilters without resetting matchNav. After navigating matches in one table and then selecting a different table, the new grid can highlight the same row/column index even though the filter box is empty (and the old counter remains), so table switches show a stale match. Reset the match navigation state when table/filter state is cleared, or guard this active match on the current active term.
Useful? React with 👍 / 👎.
| import { formatCellValueAsText } from './utils.js'; | ||
|
|
||
| function activeTerm(scope) { | ||
| return (scope === 'global' ? state.filterQuery : state.columnFilters[scope] || '').trim().toLowerCase(); |
There was a problem hiding this comment.
When a table has a column literally named global, column-filter navigation for that column is treated as the toolbar global search because this sentinel is compared directly with column names. In that case activeTerm() reads state.filterQuery instead of state.columnFilters.global, and the matching code also scans every column, so Enter on the global column filter shows no/incorrect match navigation. Store the scope as a distinct type or reserve a sentinel that cannot collide with user column names.
Useful? React with 👍 / 👎.
| const cacheValid = state.matchNav.scope === scope | ||
| && state.matchNav.term === term | ||
| && state.matchNav.matches.length > 0; |
There was a problem hiding this comment.
After matches are cached, this validity check only considers scope and term, but computeMatches() depends on the current rendered cell text via state.gridData and state.dateFormat. If a user edits a matched cell or changes the Date Format control and then presses Enter with the same filter term, navigation can keep jumping to/counting cells whose current text no longer matches. Include a data/display version in the cache key or reset match navigation when cell data or date formatting changes.
Useful? React with 👍 / 👎.
- btnApplyFilter passed the click MouseEvent as `direction`, making navigateMatches compute NaN (first click jumped to the last match, a second wedged navigation). Bind `() => applyGlobalFilter(1)` and normalize direction to ±1 in navigateMatches. - Guard applyGlobalFilter/applyColumnFilter on state.isGridReloading so the toolbar filter (which bypasses the #gridContainer guards) can't stack a concurrent refetch or act on the stale grid. - preventDefault() on the global + column filter Enter handlers. - match-nav: String(formatCellValueAsText(...)) before toLowerCase() to avoid a throw on non-string formatter output. - onDateFormatChange now resetMatchNav() — the cached matches/active highlight are stale once the displayed text changes. - viewer.css: keep a pinned cell's z-index (5) when it is the active match, instead of dropping to the active-match z-index (3). - Remove the now-dead state.filterTimer (the old debounced onFilterChange was replaced by Enter-to-submit). Regenerated bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zknpr
force-pushed
the
feat/filter-match-navigation
branch
from
0cfd7dd to
94451b4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94451b4173
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event.key === 'Enter') { | ||
| event.preventDefault(); | ||
| applyGlobalFilter(event.shiftKey ? -1 : 1); |
There was a problem hiding this comment.
When a user is composing text with an IME in the filter input, the Enter key used to confirm the candidate can arrive as key === 'Enter'; this path treats it as a filter submit, prevents the composition confirmation, and starts a grid reload before the composed text is committed. Guarding on event.isComposing (and the same column-filter Enter handler below) avoids breaking Japanese/Chinese/Korean input.
Useful? React with 👍 / 👎.
The Enter that confirms an IME candidate (Japanese/Chinese/Korean) fires as key==='Enter'; with the new preventDefault the global and column filter handlers were submitting the filter and starting a reload before the composed text committed, breaking CJK input. Guard both Enter handlers on !event.isComposing. Regenerated bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32040944c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (value !== state.filterQuery) { | ||
| state.filterQuery = value; | ||
| state.currentPageIndex = 0; | ||
| loadTableData(); | ||
| resetMatchNav(); | ||
| await loadTableData(); |
There was a problem hiding this comment.
If loadTableData() fails or times out, it catches the error and leaves the error view in place, but this path has already made the submitted value look applied. A subsequent Enter/Search with the same query skips the refetch and only calls navigateMatches, so users cannot retry a transient failed global filter without editing the text first; the column-filter path has the same changed-before-success pattern.
Useful? React with 👍 / 👎.
applyGlobalFilter/applyColumnFilter set the filter term before awaiting loadTableData, which swallows errors. After a failed/timed-out fetch the term looked applied, so re-submitting the same query skipped the refetch (value === state.filterQuery) and only navigated — the user couldn't retry without editing the text. loadTableData now returns true on success / false on a genuine (non-superseded) failure. Both apply paths revert the term (global) or restore/delete the prior column value on false, so the same query re-runs the fetch. Regenerated bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b5c64453a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loadTableData(); | ||
| resetMatchNav(); | ||
| const ok = await loadTableData(); | ||
| if (ok === false) { |
There was a problem hiding this comment.
When loadTableData() is superseded by a newer toolbar load it resolves undefined, but this check only stops explicit false failures, so a slow filter submit can resume after the user clicks pagination/page-size and call navigateMatches against the stale grid while the newer load is still in flight; the newer render can then show a stale active match/counter on the wrong page. Treat only ok === true as applied before persisting/navigating, and apply the same guard to the column-filter path below.
Useful? React with 👍 / 👎.
A filter load superseded by a newer toolbar load (pagination/page-size/ table switch) resolves undefined; the previous check only stopped explicit false, so a slow submit could resume and navigateMatches against the stale grid while the newer load was still rendering, showing a stale active match/counter on the wrong page. Treat only ok === true as applied: false still reverts the term (retry), undefined now returns without navigating. Applied to both global and column filter paths. Regenerated bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for this, @yukina3230! 🙏 Enter-to-navigate with the match counter is a great addition, and it pairs really nicely with your highlighting PR. Merged into That's four solid PRs in one batch — thank you for all of them! 🎉 |
2 participants
Description
Enter-to-navigate functionality for matching grid cells in both global search and column filters..active-match-cellborder and tracks progress via a dynamic counter.Enterapplies the filter (backend fetch), while subsequent presses cycle focus locally without redundant re-queries.Type of Change
Checklist
npm testpasses locallynpm run buildcompletes without errorstextContentrendering, strict CSP)Screenshots
nav.webm