feat: filter highlighting#499
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 secure, XSS-safe text highlighting for grid cells based on active search/filter terms, and updates the build script to use function replacers to prevent regex substitution issues during bundling. The review feedback suggests sorting the highlight terms by length in descending order to prevent shorter terms from shadowing longer terms in the generated regular expression.
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 function buildHighlightMatcher(terms) { | ||
| const escaped = []; | ||
| for (const t of terms) { | ||
| const trimmed = t && t.trim(); | ||
| if (trimmed) escaped.push(escapeRegExp(trimmed)); | ||
| } | ||
| if (escaped.length === 0) return null; | ||
| return new RegExp(`(${escaped.join('|')})`, 'gi'); | ||
| } |
There was a problem hiding this comment.
When constructing a regular expression with alternations (e.g., (term1|term2)), shorter terms can shadow longer terms if they share a prefix and the shorter term appears first (for example, (cat|category) matching against 'category' will only match 'cat'). To prevent this prefix shadowing issue and avoid redundant duplicate terms, we should deduplicate the terms and sort them by length in descending order before joining them.
export function buildHighlightMatcher(terms) {
const uniqueEscaped = new Set();
for (const t of terms) {
const trimmed = t && t.trim();
if (trimmed) {
uniqueEscaped.add(escapeRegExp(trimmed));
}
}
if (uniqueEscaped.size === 0) return null;
const sorted = Array.from(uniqueEscaped).sort((a, b) => b.length - a.length);
return new RegExp('(' + sorted.join('|') + ')', 'gi');
}
|
@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: a951b49d1f
ℹ️ 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 buildHighlightMatcher(terms) { | ||
| const escaped = []; | ||
| for (const t of terms) { | ||
| const trimmed = t && t.trim(); |
There was a problem hiding this comment.
When a table has a column named like toString or constructor, state.columnFilters is still a plain {} with Object prototype, and the new grid-render caller passes state.columnFilters[col.name] into this helper even when no filter is set. In that case t can be an inherited function instead of a string, so calling trim() throws and the grid fails to render for that table; use an own-property lookup at the call site or a string type guard before trimming.
Useful? React with 👍 / 👎.
| textSpan.textContent = displayValue; | ||
| // Use DOM text nodes (never innerHTML) for security (prevents XSS). | ||
| // formatCellValueAsText returns unescaped text suitable for textContent/text nodes. | ||
| appendHighlightedText(textSpan, displayValue, columnMatchers[displayColIdx]); |
There was a problem hiding this comment.
This wires highlighting only into the full grid render, but cells are also rewritten by updateCellDom in core/ui/modules/edit.js, which still restores plain textContent. With an active filter, simply double-clicking a highlighted cell and pressing Escape goes through the cancel path and removes the highlight until a later full render, so matched cells become visually inconsistent; reuse the same highlighter when restoring edited cells.
Useful? React with 👍 / 👎.
| textSpan.textContent = displayValue; | ||
| // Use DOM text nodes (never innerHTML) for security (prevents XSS). | ||
| // formatCellValueAsText returns unescaped text suitable for textContent/text nodes. | ||
| appendHighlightedText(textSpan, displayValue, columnMatchers[displayColIdx]); |
There was a problem hiding this comment.
This scans the formatted display string for every cell, including synthetic text from formatCellValueAsText such as NULL, [BLOB], and the appended ... for long strings. When a global search matches the row via another column, queries like null, blob, or . can highlight these placeholders even though the SQL filter ran against the actual column values and did not match those cells; skip placeholder/generated text or match against the raw value before marking it.
Useful? React with 👍 / 👎.
buildHighlightMatcher joined active filter terms into a single alternation in arbitrary order. Regex alternation is first-match, so a shorter term that is a prefix of a longer one (e.g. global filter "cat" + a column filter "category") would shadow the longer match and only highlight "cat". De-duplicate the terms and sort them longest-first so the longest applicable term wins. Regenerated bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zknpr
force-pushed
the
feat/filter-highlighting
branch
from
a951b49 to
b508c4e
Compare
Press Enter in the global or a column filter to jump between cells whose displayed text contains the active term, cycling with Enter/Shift+Enter, a current/total counter, and an outlined active-match cell. Initial Enter applies the filter (one fetch); subsequent presses cycle locally without re-querying. Match navigation resets on sort/page/page-size/filter-text/date-format changes. Review hardening (Gemini + 4 Codex rounds): fixed the btnApplyFilter MouseEvent-as-direction crash; guarded the toolbar filter against concurrent reloads (state.isGridReloading); preventDefault + IME (isComposing) handling on Enter; String() around formatter output in match scan; failed/superseded filter-submit lifecycle (loadTableData returns success; only a fully-applied load navigates, failures revert for retry); pinned active-match z-index; removed dead filterTimer. Composes with #498/#499/#501. tsc + 454 unit tests green. Co-authored-by: yukina3230 <75545944+yukina3230@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for this, @yukina3230! 🙏 The highlighting is done exactly the right way — DOM text nodes + |
2 participants
Description
appendHighlightedText()intorenderDataGrid()cell rendering, covering both global search (state.filterQuery) and per-column filters.innerHTML) to ensure cell content remains 100% XSS-safe.viewer.cssvia the standard VS CodefindMatchHighlightBackgroundtoken.Type of Change
Checklist
npm testpasses locallynpm run buildcompletes without errorstextContentrendering, strict CSP)Screenshots