-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: filter highlighting #499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { state } from './state.js'; | ||
| import { escapeHtml, formatCellValueAsText } from './utils.js'; | ||
| import { escapeHtml, formatCellValueAsText, appendHighlightedText, buildHighlightMatcher } from './utils.js'; | ||
| import { getRowId, getCellValue } from './data-utils.js'; | ||
| import { syncSelectionDOM } from './grid-selection.js'; | ||
|
|
||
|
|
@@ -106,6 +106,12 @@ function createTableBody(orderedColumns, columnIndexMap, pinnedColumnOffsets, ro | |
| ...state.gridData.map((row, idx) => ({ idx, rowId: getRowId(row, idx) })).filter(r => !state.pinnedRowIds.has(r.rowId)) | ||
| ]; | ||
|
|
||
| // Precompute one highlight matcher per column (depends on the global filter + | ||
| // that column's filter, not on the row), so we don't rebuild a RegExp per cell. | ||
| const columnMatchers = orderedColumns.map(col => | ||
| buildHighlightMatcher([state.filterQuery, state.columnFilters[col.name]]) | ||
| ); | ||
|
|
||
| const fragment = document.createDocumentFragment(); | ||
|
|
||
| for (const { idx: rowIdx, rowId } of orderedRowIndices) { | ||
|
|
@@ -175,9 +181,9 @@ function createTableBody(orderedColumns, columnIndexMap, pinnedColumnOffsets, ro | |
|
|
||
| const textSpan = document.createElement('span'); | ||
| textSpan.className = 'cell-text'; | ||
| // Use textContent for security (prevents XSS). | ||
| // formatCellValueAsText returns unescaped text suitable for textContent. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This scans the formatted display string for every cell, including synthetic text from Useful? React with 👍 / 👎. |
||
| td.appendChild(textSpan); | ||
|
|
||
| if (hasContent) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,66 @@ export function escapeHtml(str) { | |
| .replace(/'/g, '''); | ||
| } | ||
|
|
||
| /** | ||
| * Escape a string for safe use inside a RegExp pattern. | ||
| */ | ||
| function escapeRegExp(str) { | ||
| return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } | ||
|
|
||
| /** | ||
| * Build a reusable case-insensitive RegExp that matches any of the given filter | ||
| * terms, or null when there are no active terms. Compile this once per column | ||
| * and reuse the matcher across every cell rather than rebuilding it per cell. | ||
| * | ||
| * Terms are de-duplicated and sorted longest-first: regex alternation matches | ||
| * the first listed alternative that fits, so without this a shorter term that is | ||
| * a prefix of a longer one (e.g. "cat" vs "category") would shadow the longer | ||
| * match and only highlight the prefix. | ||
| */ | ||
| export function buildHighlightMatcher(terms) { | ||
| const seen = new Set(); | ||
| for (const t of terms) { | ||
| const trimmed = t && t.trim(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a table has a column named like Useful? React with 👍 / 👎. |
||
| if (trimmed) seen.add(trimmed); | ||
| } | ||
| if (seen.size === 0) return null; | ||
| const ordered = [...seen].sort((a, b) => b.length - a.length); | ||
| return new RegExp(`(${ordered.map(escapeRegExp).join('|')})`, 'gi'); | ||
| } | ||
|
Comment on lines
+35
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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');
} |
||
|
|
||
| /** | ||
| * Append text to a parent element, wrapping matches of a precompiled `matcher` | ||
| * (from buildHighlightMatcher) in <mark class="cell-highlight"> spans. Uses DOM | ||
| * text nodes (never innerHTML) so untrusted cell content can never be interpreted | ||
| * as markup. When `matcher` is null, the text is appended verbatim (fast path). | ||
| */ | ||
| export function appendHighlightedText(parentEl, text, matcher) { | ||
| if (!matcher) { | ||
| parentEl.appendChild(document.createTextNode(text)); | ||
| return; | ||
| } | ||
|
|
||
| // The matcher is shared across cells; reset its state before scanning. | ||
| matcher.lastIndex = 0; | ||
| let lastIndex = 0; | ||
| let match; | ||
| while ((match = matcher.exec(text)) !== null) { | ||
| if (match.index > lastIndex) { | ||
| parentEl.appendChild(document.createTextNode(text.slice(lastIndex, match.index))); | ||
| } | ||
| const mark = document.createElement('mark'); | ||
| mark.className = 'cell-highlight'; | ||
| mark.textContent = match[0]; | ||
| parentEl.appendChild(mark); | ||
| lastIndex = match.index + match[0].length; | ||
| if (match[0].length === 0) matcher.lastIndex++; // guard against zero-length matches | ||
| } | ||
| if (lastIndex < text.length) { | ||
| parentEl.appendChild(document.createTextNode(text.slice(lastIndex))); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validate and sanitize a rowid for use in SQL queries. | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wires highlighting only into the full grid render, but cells are also rewritten by
updateCellDomincore/ui/modules/edit.js, which still restores plaintextContent. 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 👍 / 👎.