Highlight whitespaces on renderer#78
Conversation
📝 WalkthroughWalkthroughAdds Unicode whitespace visualization and full-text highlighting utilities, integrates both transformations into ChangesHighlighting Features
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant RendererWrapper
participant visualizeInvisibleCharacters
participant highlightChildren
participant highlightSearchedFulltexts
Renderer->>RendererWrapper: pass children and wrapper props
RendererWrapper->>visualizeInvisibleCharacters: visualize invisible characters
visualizeInvisibleCharacters-->>RendererWrapper: processed children
RendererWrapper->>highlightChildren: apply fulltextHighlightTerms
highlightChildren->>highlightSearchedFulltexts: highlight text segments
highlightSearchedFulltexts-->>highlightChildren: span-wrapped matches
highlightChildren-->>RendererWrapper: highlighted children
RendererWrapper-->>Renderer: render processed content
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
screen-recording-2026-06-09-at-113507_iq4eegKJ.mp4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/abc/Renderer.js (1)
51-71:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
plain()should return plain data, not React elements.The
plain()method is expected to return primitive values or plain strings (based on its name and separation fromrender()), but lines 55-56 and 68-70 now callhighlightUnicodeChildren, which returns arrays of React elements for string inputs. This breaks the method's contract and will cause runtime errors in any code expecting plain values fromplain().🐛 Proposed fix: Remove highlighting from plain()
plain(key, value, schemaField) { // Render stringified component if value is a object if (typeof value === 'object') { try { - const highlightedValue = highlightUnicodeChildren(JSON.stringify(value)); - return highlightedValue; + return JSON.stringify(value); } catch(e) { console.warn('Failed to stringify the renderer value:', value, e); return value; } } // Convert bigint to string (to preserve precision and to display the value when no other renderer is applied) if (typeof value === "bigint") { value = value.toString(); } - const highlightedValue = highlightUnicodeChildren(value); - - return highlightedValue; + return value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abc/Renderer.js` around lines 51 - 71, The plain(key, value, schemaField) function must return primitive/plain string values but currently calls highlightUnicodeChildren (which returns React elements) for objects and strings; change plain() to return raw primitive values: for objects return JSON.stringify(value) (or the original value on stringify failure) and for bigint return value.toString(), then return that plain string without calling highlightUnicodeChildren; update references to highlightUnicodeChildren only in render() or UI-specific code, keeping plain() pure.
🧹 Nitpick comments (3)
src/abc/Renderer.scss (1)
3-7: 💤 Low valueSimplify vendor prefixes for modern browsers.
The
-khtml-user-select,-o-user-select, and-ms-user-selectprefixes are outdated as of 2026. Modern browsers support the standarduser-selectproperty, and at most-webkit-user-selectand-moz-user-selectmay be needed for older browser versions.♻️ Simplified CSS
.renderer-whitespace { color: var(--bs-secondary-color); + -webkit-user-select: none; + -moz-user-select: none; + user-select: none; - -webkit-user-select: none; - -khtml-user-select: none; - -moz-user-select: none; - -ms-user-select: none; - -o-user-select: none; - user-select: none; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abc/Renderer.scss` around lines 3 - 7, Remove the outdated vendor prefixes in the user-select rules in Renderer.scss: delete -khtml-user-select, -o-user-select, and -ms-user-select and ensure the block contains only the modern properties needed (keep -webkit-user-select and -moz-user-select if you need older compatibility, plus the standard user-select). Update the rule where those properties appear so the final set is: -webkit-user-select, -moz-user-select (optional), and user-select.src/utils/highlightWhitespaces.js (1)
62-98: ⚡ Quick winRemove unused
SPECIAL_UNICODE_RANGESand commented-out range check.
SPECIAL_UNICODE_RANGESis defined but never used because the range check on line 96 is commented out. If range checking is not needed for this feature, remove the constant and the dead code path to reduce confusion and maintenance burden.♻️ Proposed cleanup
-/** - * Inclusive [start, end] code point ranges. - * `@type` {[number, number][]} - */ -export const SPECIAL_UNICODE_RANGES = [ - // Whitespace - [0x2000, 0x200A], // En/Em quad, En/Em space, thin/hair space, etc. - - // Bidirectional control - [0x202A, 0x202E], // LRE, RLE, PDF, LRO, RLO - [0x2066, 0x2069], // LRI, RLI, FSI, PDI - - // C0 / C1 controls - [0x0001, 0x0008], // SOH–BS - [0x000E, 0x001F], // SO–US - [0x0080, 0x009F], // C1 controls (incl. NEL U+0085, CSI U+009B) - - // Other problematic - [0xFFF9, 0xFFFB], // Interlinear annotation - [0xFDD0, 0xFDEF], // Noncharacters - [0xE0000, 0xE007F], // Tag characters (ASCII smuggling) - [0xE0100, 0xE01EF], // Variation Selectors Supplement - [0xFE00, 0xFE0F], // Variation Selectors - [0xD800, 0xDFFF], // Surrogates (invalid standalone) - [0xE000, 0xF8FF], // Private Use Area (BMP) - [0xF0000, 0xFFFFD], // Private Use Area (supplementary) - [0x100000, 0x10FFFD], // Private Use Area (supplementary) - [0x2400, 0x2426], // Control Pictures (display glyphs) -]; - /** * `@param` {number} codePoint * `@returns` {boolean} */ export function isSpecialUnicodeCodePoint(codePoint) { - if (SPECIAL_UNICODE_CODE_POINTS.has(codePoint)) { - return true; - } - // return SPECIAL_UNICODE_RANGES.some(([start, end]) => codePoint >= start && codePoint <= end); - return false; + return SPECIAL_UNICODE_CODE_POINTS.has(codePoint); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/highlightWhitespaces.js` around lines 62 - 98, The file defines SPECIAL_UNICODE_RANGES but never uses it and has a commented-out range check in isSpecialUnicodeCodePoint; remove the dead constant and the commented-out line so isSpecialUnicodeCodePoint simply returns whether SPECIAL_UNICODE_CODE_POINTS has the codePoint (ensure SPECIAL_UNICODE_CODE_POINTS remains defined/ imported); if range checking is actually required instead, re-enable and use SPECIAL_UNICODE_RANGES in the function (e.g., iterate its [start,end] tuples) — choose one approach and delete the unused code path accordingly.src/components/DataTable2/DataTable2.jsx (1)
326-342: Optional refactor: explicit return is stylistic.The change from implicit to explicit return in the
mapcallback is purely stylistic and doesn't add functional value. The implicit return was already clear and concise. However, this change is acceptable and doesn't introduce issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/DataTable2/DataTable2.jsx` around lines 326 - 342, The rows.map callback was changed from an implicit to an explicit return which is purely stylistic; revert the callback in the JSX block that uses rows.map to the prior concise implicit-return form (the mapping that returns the <tr> directly) or, if you prefer to keep the explicit return, ensure consistency across DataTable2.jsx by updating other similar map callbacks (e.g., columns.map and rows.map usages) to the same style; touch the rows.map callback and related column rendering logic (column.render, loading branch) to restore consistent concise mapping style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/abc/Renderer.js`:
- Around line 37-47: The Renderer currently calls
highlightUnicodeChildren(value) before passing children into RendererWrapper,
causing duplicate highlighting because RendererWrapper already memoizes and
highlights its children; remove the call to highlightUnicodeChildren from
Renderer.render and pass the raw value as the child into <RendererWrapper> (keep
data-value={value} and data-key={key} and component prop) so only
RendererWrapper performs the highlighting logic.
In `@src/components/DataTable2/readme.md`:
- Around line 63-71: The fenced code block showing the DataTableCard2 usage
lacks a language specifier; update the opening fence to include a language
(e.g., change ``` to ```jsx) so the JSX snippet (DataTableCard2, props app,
columns, loader, header, disableFooter) is properly highlighted and linted in
the readme.
In `@src/utils/highlightWhitespaces.js`:
- Around line 8-9: The JSDoc type for SPECIAL_UNICODE_CODE_POINTS is wrong (says
`@type` {number[]}) while the value is a Set; update the comment above the export
of SPECIAL_UNICODE_CODE_POINTS to use the correct JSDoc type annotation `@type`
{Set<number>} so the doc matches the actual exported Set and tooling/linters
understand the correct type.
- Line 149: The span in src/utils/highlightWhitespaces.js is adding a dynamic
class name unicode-${hexUnicode} (alongside renderer-whitespace) that has no
matching CSS; remove the dynamic class and instead add a stable attribute (e.g.,
data-unicode or data-codepoint with the hexUnicode value) and update the
stylesheet to target the single .renderer-whitespace or the new data-* selector
for any per-code-point styling; locate the span generation in
highlightWhitespaces.js and replace the dynamic class with the data attribute,
then add/adjust CSS rules to use .renderer-whitespace or [data-unicode] rather
than unicode-XXXX.
---
Outside diff comments:
In `@src/abc/Renderer.js`:
- Around line 51-71: The plain(key, value, schemaField) function must return
primitive/plain string values but currently calls highlightUnicodeChildren
(which returns React elements) for objects and strings; change plain() to return
raw primitive values: for objects return JSON.stringify(value) (or the original
value on stringify failure) and for bigint return value.toString(), then return
that plain string without calling highlightUnicodeChildren; update references to
highlightUnicodeChildren only in render() or UI-specific code, keeping plain()
pure.
---
Nitpick comments:
In `@src/abc/Renderer.scss`:
- Around line 3-7: Remove the outdated vendor prefixes in the user-select rules
in Renderer.scss: delete -khtml-user-select, -o-user-select, and -ms-user-select
and ensure the block contains only the modern properties needed (keep
-webkit-user-select and -moz-user-select if you need older compatibility, plus
the standard user-select). Update the rule where those properties appear so the
final set is: -webkit-user-select, -moz-user-select (optional), and user-select.
In `@src/components/DataTable2/DataTable2.jsx`:
- Around line 326-342: The rows.map callback was changed from an implicit to an
explicit return which is purely stylistic; revert the callback in the JSX block
that uses rows.map to the prior concise implicit-return form (the mapping that
returns the <tr> directly) or, if you prefer to keep the explicit return, ensure
consistency across DataTable2.jsx by updating other similar map callbacks (e.g.,
columns.map and rows.map usages) to the same style; touch the rows.map callback
and related column rendering logic (column.render, loading branch) to restore
consistent concise mapping style.
In `@src/utils/highlightWhitespaces.js`:
- Around line 62-98: The file defines SPECIAL_UNICODE_RANGES but never uses it
and has a commented-out range check in isSpecialUnicodeCodePoint; remove the
dead constant and the commented-out line so isSpecialUnicodeCodePoint simply
returns whether SPECIAL_UNICODE_CODE_POINTS has the codePoint (ensure
SPECIAL_UNICODE_CODE_POINTS remains defined/ imported); if range checking is
actually required instead, re-enable and use SPECIAL_UNICODE_RANGES in the
function (e.g., iterate its [start,end] tuples) — choose one approach and delete
the unused code path accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5efed192-f77f-4ce8-a094-c9c6c2617d28
📒 Files selected for processing (6)
src/abc/Renderer.jssrc/abc/Renderer.scsssrc/components/DataTable2/DataTable2.jsxsrc/components/DataTable2/readme.mdsrc/components/RendererWrapper/RendererWrapper.jsxsrc/utils/highlightWhitespaces.js
| ``` | ||
| <DataTableCard2 | ||
| app={app} | ||
| columns={columns} | ||
| loader={loader} | ||
| header={<Header />} | ||
| hideFooter={true} | ||
| disableFooter={true} | ||
| /> | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block starting at line 63 should specify a language identifier for proper syntax highlighting and linting compliance.
📝 Proposed fix
-```
+```jsx
<DataTableCard2
app={app}
columns={columns}
loader={loader}
header={<Header />}
disableFooter={true}
/>
```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| <DataTableCard2 | |
| app={app} | |
| columns={columns} | |
| loader={loader} | |
| header={<Header />} | |
| hideFooter={true} | |
| disableFooter={true} | |
| /> | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/DataTable2/readme.md` around lines 63 - 71, The fenced code
block showing the DataTableCard2 usage lacks a language specifier; update the
opening fence to include a language (e.g., change ``` to ```jsx) so the JSX
snippet (DataTableCard2, props app, columns, loader, header, disableFooter) is
properly highlighted and linted in the readme.
Source: Linters/SAST tools
| /** @type {number[]} */ | ||
| export const SPECIAL_UNICODE_CODE_POINTS = new Set([ |
There was a problem hiding this comment.
Correct the JSDoc type annotation.
The JSDoc comment states @type {number[]} but SPECIAL_UNICODE_CODE_POINTS is instantiated as a Set, not an array.
📝 Suggested fix
-/** `@type` {number[]} */
+/** `@type` {Set<number>} */
export const SPECIAL_UNICODE_CODE_POINTS = new Set([📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** @type {number[]} */ | |
| export const SPECIAL_UNICODE_CODE_POINTS = new Set([ | |
| /** `@type` {Set<number>} */ | |
| export const SPECIAL_UNICODE_CODE_POINTS = new Set([ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/highlightWhitespaces.js` around lines 8 - 9, The JSDoc type for
SPECIAL_UNICODE_CODE_POINTS is wrong (says `@type` {number[]}) while the value is
a Set; update the comment above the export of SPECIAL_UNICODE_CODE_POINTS to use
the correct JSDoc type annotation `@type` {Set<number>} so the doc matches the
actual exported Set and tooling/linters understand the correct type.
| quotesOnKeys={false} | ||
| enableClipboard={false} | ||
| indentWidth={8} | ||
| src={value} |
|
|
||
| // Wrapper for datatable context | ||
| export function DataTableCard2({ columns, loader, loaderParams, header, className, initialLimit = 0, rowHeight = 38, disableParams = undefined, hideFooter = false, rowStyle }) { | ||
| export function DataTableCard2({ columns, loader, loaderParams, header, className, initialLimit = 0, rowHeight = 38, disableParams = undefined, disableFooter = false, rowStyle }) { |
There was a problem hiding this comment.
why there are changes to DataTable2? I dont think its necessary for the whitespaces highligher :)
| ``` | ||
|
|
||
| To remove footer from the DataTable2, `hideFooter={true}` should be set. | ||
| To remove footer from the DataTable2, `disableFooter={true}` should be set. |
There was a problem hiding this comment.
do it in other PR eventually
| if (SPECIAL_UNICODE_CODE_POINTS.has(codePoint)) { | ||
| return true; | ||
| } | ||
| // return SPECIAL_UNICODE_RANGES.some(([start, end]) => codePoint >= start && codePoint <= end); |
There was a problem hiding this comment.
remove it or write a commentary why it is commented in
44b7a3f to
1751ac7
Compare
1751ac7 to
64ce28b
Compare
|
Hi, I have mistakenly pushed commit from another branch, resolved it. Then I deleted double highlighting in renerer, deleted unnecessary styles and added a new condition (checking if in the end or in the start of the string are special unicode points before spaces to highlight that spaces too). You can see that case on the photo, in the end of the page are space + unicode 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/highlightWhitespaces.js`:
- Around line 101-115: The getAsciiSpaceEdges function has a surrogate pair
handling bug where it increments/decrements indices by 1 regardless of character
width, causing incorrect edge detection when supplementary plane characters are
present. Fix this by updating both the leading edge loop (where leadingEnd is
incremented) and trailing edge loop (where trailingStart is decremented) to
increment/decrement by the correct number of UTF-16 code units for each
character: use 2 code units for code points > 0xFFFF and 1 code unit otherwise.
Reference the correct handling already implemented in the main
highlightWhitespaces function at line 154 to ensure consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df1dac27-b1de-4155-8688-b063099523c2
📒 Files selected for processing (4)
src/abc/Renderer.jssrc/abc/Renderer.scsssrc/components/RendererWrapper/RendererWrapper.jsxsrc/utils/highlightWhitespaces.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/abc/Renderer.scss
- src/components/RendererWrapper/RendererWrapper.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/highlightRendererChildren.js`:
- Around line 24-29: The array handling in the `if (Array.isArray(children))`
branch only applies text highlighting to string nodes within the array but
ignores React elements nested inside it. Modify this block to recursively
process React elements in the array as well. Instead of only calling
`highlightFulltextInNodes`, map through the array children and for each child
element, recursively apply the highlighting logic (similar to how non-array
elements are handled) so that text descendants within React elements in the
array also receive fulltext highlighting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cffef1f-676c-4938-a2d8-29e5aa62d542
📒 Files selected for processing (6)
src/abc/Renderer.jssrc/abc/Renderer.scsssrc/index.jssrc/utils/highlightRendererChildren.jssrc/utils/highlightSearchedFulltexts.jssrc/utils/highlightWhitespaces.js
💤 Files with no reviewable changes (1)
- src/abc/Renderer.js
✅ Files skipped from review due to trivial changes (1)
- src/abc/Renderer.scss
Pe5h4
left a comment
Pe5h4
left a comment
There was a problem hiding this comment.
I think we may need to do some restructuralization. in utils create another folder named highlighting or similar, then put these highliting utils there
| }; | ||
|
|
||
| // Highlight searched fulltexts in the text by replacing matched terms with a span | ||
| export const highlightSearchedFulltexts = (text, searchValue, highlightClassName = 'bg-warning') => { |
There was a problem hiding this comment.
please have a look here and reuse the colors TeskaLabs/asab-webui-shell-lib#53
| } from './utils/highlightWhitespaces.js'; | ||
| export { highlightSearchedFulltexts } from './utils/highlightSearchedFulltexts.js'; | ||
| export { | ||
| highlightRendererChildren, |
There was a problem hiding this comment.
Why is it called highlightRendererChildren? I would focus on reuse, so it should not be named as such if we want to use it elsewhere as well
| export { | ||
| highlightRendererChildren, | ||
| createFulltextHighlightWrapper, | ||
| } from './utils/highlightRendererChildren.js'; |
There was a problem hiding this comment.
not sure if this is a viable name of the file. also use jsx filetype
There was a problem hiding this comment.
it this a viable name? it does not highlight, it just make the whitespaces visible, or not?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/highlightRendererChildren.js`:
- Around line 24-40: The array-handling branch in highlightRendererChildren is
re-processing spans created by highlightFulltextInNodes, causing redundant
recursive highlighting. Replace the current two-step flow in
highlightRendererChildren with a single-pass traversal that handles string nodes
and React elements directly, so highlighted <span
className='search-text-highlight'> nodes are not fed back into the recursion.
Keep the logic localized to highlightRendererChildren and its use of
highlightText/highlightFulltextInNodes behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be751106-e9ca-4c9c-918b-9b3bed57a2e5
📒 Files selected for processing (3)
src/utils/highlightRendererChildren.jssrc/utils/highlightSearchedFulltexts.jssrc/utils/highlightSearchedFulltexts.scss
✅ Files skipped from review due to trivial changes (1)
- src/utils/highlightSearchedFulltexts.scss
|
@Pe5h4 Hi, I have refactored code how you said (rename components and files, restructure architecture). But I don't understand you comment - please have a look here and reuse the colors TeskaLabs/asab-webui-shell-lib#53 I did that. That MR just change colors for searching on web highlighting (that's not a class or variable). Please, can you explain what do you mean |
| }; | ||
|
|
||
| // Wrap a renderer wrapper component with fulltext search highlighting | ||
| export const createFulltextHighlightWrapper = (BaseWrapper, fulltextHighlightTerms) => { |
There was a problem hiding this comment.
hmm, I am thinking here. wont this factory recreate the wrapper every time it is called? I mean even if you wrap it in the useMemo (as you have in your other MR) in lower level components, it may still recreate and therefore it is not ideal. Like we can loose some info/state of wrapped components because of this. can you validate that?
|
So, i reconsider the highlighting and remove wrapper for that, so it highlights just with function hightlightChildren. It avoids recreating the componnent |
| return children; | ||
| }; | ||
|
|
||
| export const createUnicodeVisualizeWrapper = (BaseWrapper) => { |
There was a problem hiding this comment.
I am a bit puzzled here and would like to understand the code more broadly. You have removed the highlighting factory, which was creating re-renders, but this factory is still here. It does the same thing... I am not really convinced over the implementation.
I understand that we need something which will identify the empty chars in the component and renders the marks, but is this really a solution here? I may be wrong, but on the same side, I am not convinced that your code is sustainable and that it works effectively without any unnecessary re-rendering and stuff either.
Also, why the visualization of the whitespaces is not default part of the RendererWrapper and needs to be set up in lower level?
|
You were right, i replace all logic here. |
Pe5h4
left a comment
Pe5h4
left a comment
There was a problem hiding this comment.
looks better, but I would advice a bit more updates here
| data-key={key} // Passing key (to eventually work with in the external wrapper) | ||
| component={params?.WrapperComponent || "span"} | ||
| >{value}</RendererWrapper>); | ||
| fulltextHighlightTerms={params?.fulltextHighlightTerms} |
There was a problem hiding this comment.
this would require a commentary. dont leave that uncommented, because devs then can only guess what is that. Also since its not a required value, I would maybe dont pass that at all (specifically), but you can pass the whole params
<RendererWrapper
{...(params ?? {})} // Passing wrapper params down to the children
data-value={value} // Passing value (to eventually work with in the external wrapper)
data-key={key} // Passing key (to eventually work with in the external wrapper)
component={params?.WrapperComponent || 'span'}
>{value}</RendererWrapper>
| export function RendererWrapper({ | ||
| children, | ||
| component: Component = 'span', // Default tag, can be overloaded | ||
| fulltextHighlightTerms, |
| const dataValue = rest?.['data-value']; | ||
|
|
||
| let processedChildren = visualizeUnicodeChildren(children); | ||
| if (fulltextHighlightTerms) { |
There was a problem hiding this comment.
// Validate if fulltext terms exist to proceed with highliting of a fulltext based search
const fulltextTerms = rest?.params?.fulltextTerms;
if (fulltextTerms) { ... }
There was a problem hiding this comment.
there wouldn't be params - const fulltextTerms = rest?.params?.fulltextTerms; just rest?.fulltextTerms;
Because of destructurization
| data-key={key} // Passing key (to eventually work with in the external wrapper) | ||
| component={params?.WrapperComponent || "span"} | ||
| >{value}</RendererWrapper>); | ||
| > |
| }) { | ||
| const dataValue = rest?.['data-value']; | ||
| let processedChildren = visualizeUnicodeChildren(children); | ||
|
|
There was a problem hiding this comment.
visualizeInvisibleCharacters would be a bit better name, also the folder name should change
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/textFormatting/highlightChildren.jsx`:
- Around line 59-64: Update the custom-component fallback in the
highlightChildren flow so React elements are returned unchanged rather than
replaced with highlightText output from nestedDataValue. Preserve
primitive-value highlighting for supported text content, and use the existing
children return path for custom components.
- Around line 30-33: Update highlightChildren to detect the visualization
wrapper’s no-highlight marker before recursing and return that element
unchanged. Add the same generic marker to the spans created by
visualizeInvisibleCharacters, ensuring searches cannot match or highlight their
rendered artifacts while ordinary elements retain the existing recursion
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09f3f935-d5db-4c3b-9047-9b27bd885ce7
📒 Files selected for processing (7)
src/abc/Renderer.jssrc/components/RendererWrapper/RendererWrapper.jsxsrc/index.jssrc/utils/textFormatting/highlightChildren.jsxsrc/utils/textFormatting/highlightSearchedFulltexts.jssrc/utils/textFormatting/highlightSearchedFulltexts.scsssrc/utils/textFormatting/visualizeInvisibleCharacters.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/abc/Renderer.js
There was a problem hiding this comment.
oh and can you please change the file extensions from js to jsx ? thanks
Ok - but I need some kind of briefing here ;-) |
@ateska oh, sorry, sure - this PR is about rendering a whitespaces/special characters. the reason for that is we (and users) want to be aware of them, so thats easier to locate them and fix these inconsitencies properly in parsers, etc.. This is to be used first in Discover screen, In @yanabel1996 will add more context here eventually |
ateska
left a comment
ateska
left a comment
There was a problem hiding this comment.
Can we have some kind of extendible unit test that will allow easy addition on all possible malicious strings .... so that we can evolve this?
|
|
||
| // Possible whitespace unicode code points | ||
| export const SPECIAL_UNICODE_CODE_POINTS = new Set([ | ||
| // Whitespace |
There was a problem hiding this comment.
I suggest to include all characters 0x00 - 0x31.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/textFormatting/highlightSearchedFulltexts.jsx (1)
38-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSimplify match detection by leveraging
splitcapturing group behavior.When using
String.prototype.splitwith a regex containing exactly one capturing group(...), the returned array strictly alternates between non-matched and matched segments. This guarantees every matched term will be placed at an odd index (1, 3, 5...), and every non-match at an even index (0, 2, 4...).You can completely remove the
highlightedTermsSet and thetoLowerCase()comparisons, which improves performance and simplifies the logic.♻️ Proposed refactor
- const highlightedTerms = new Set(terms.map((term) => term.toLowerCase())); - return sourceText.split(matchRegex).map((part, index) => { - if (highlightedTerms.has(part.toLowerCase())) { + if (index % 2 !== 0) { return ( <span key={`${part}-${index}`} className='search-text-highlight'> {part} </span> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/textFormatting/highlightSearchedFulltexts.jsx` around lines 38 - 48, In the sourceText.split(matchRegex) mapping, remove the highlightedTerms Set and lowercase comparisons, and determine matches by the segment index being odd, leveraging matchRegex’s single capturing group. Preserve the existing highlighted span and plain-text rendering for matched and non-matched segments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/textFormatting/visualizeInvisibleCharacters.jsx`:
- Around line 189-195: Update visualizeInvisibleCharacters to recursively
process array children, applying visualizeWhitespaces to each string or number
element while preserving the existing handling of scalar values and non-text
children. Use the function’s own recursive path so nested arrays are visualized
consistently.
---
Nitpick comments:
In `@src/utils/textFormatting/highlightSearchedFulltexts.jsx`:
- Around line 38-48: In the sourceText.split(matchRegex) mapping, remove the
highlightedTerms Set and lowercase comparisons, and determine matches by the
segment index being odd, leveraging matchRegex’s single capturing group.
Preserve the existing highlighted span and plain-text rendering for matched and
non-matched segments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96c772f2-d971-436e-aed5-9e66621989c9
📒 Files selected for processing (4)
CHANGELOG.mdpackage.jsonsrc/utils/textFormatting/highlightSearchedFulltexts.jsxsrc/utils/textFormatting/visualizeInvisibleCharacters.jsx
| export const visualizeInvisibleCharacters = (children) => { | ||
| if (typeof children === 'string' || typeof children === 'number') { | ||
| return visualizeWhitespaces(String(children)); | ||
| } | ||
|
|
||
| return children; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Support arrays of children to ensure whitespace visualization applies consistently.
If RendererWrapper receives an array of text nodes (e.g., ["Hello", " ", "World"]), visualizeInvisibleCharacters will currently return the array untouched, skipping the whitespace visualization.
Consider recursively processing arrays so that text fragments within them are correctly visualized.
🐛 Proposed fix
export const visualizeInvisibleCharacters = (children) => {
if (typeof children === 'string' || typeof children === 'number') {
return visualizeWhitespaces(String(children));
}
+
+ if (Array.isArray(children)) {
+ return React.Children.map(children, (child) => visualizeInvisibleCharacters(child));
+ }
return children;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const visualizeInvisibleCharacters = (children) => { | |
| if (typeof children === 'string' || typeof children === 'number') { | |
| return visualizeWhitespaces(String(children)); | |
| } | |
| return children; | |
| }; | |
| export const visualizeInvisibleCharacters = (children) => { | |
| if (typeof children === 'string' || typeof children === 'number') { | |
| return visualizeWhitespaces(String(children)); | |
| } | |
| if (Array.isArray(children)) { | |
| return React.Children.map(children, (child) => visualizeInvisibleCharacters(child)); | |
| } | |
| return children; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/textFormatting/visualizeInvisibleCharacters.jsx` around lines 189 -
195, Update visualizeInvisibleCharacters to recursively process array children,
applying visualizeWhitespaces to each string or number element while preserving
the existing handling of scalar values and non-text children. Use the function’s
own recursive path so nested arrays are visualized consistently.
|
@ateska i have added missing special characters and create the file with extendible tests. But I am not sure where to put that file. We have discussed with Franta about it and decided, that here is not a good place to store |
3 participants
I have implement highlighting white spaces, but i have a question. When we copy text should we have included whitespaces (i mean how it was before transforming visual). Now it copies only text without transforming whitespaces and whitespaces themselfs
Summary by CodeRabbit
New Features
Bug Fixes
Changelog