feat: add highlight words feature for messages and user preferences #6809
Conversation
WalkthroughAdds user-configurable highlight keywords through Preferences, exposes highlights via MarkdownContext, forwards highlights through Message/RoomView to Markdown, implements case-insensitive, escaped-word highlighting in Plain renderer, adds UI to edit/save highlights, tests, and a Storybook story. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Prefs as UserPreferencesView
participant Server
participant Room as RoomView
participant Msg as Message Container
participant MD as Markdown (Provider)
participant Plain as Plain Component
User->>Prefs: Edit highlight terms
Prefs->>Prefs: update local highlights & dirty flag
Prefs->>Server: saveUserPreferences(highlights)
Server-->>Prefs: save response
Prefs->>Server: getUserPreferences()
Server-->>Prefs: confirmed persisted highlights
Note over Room,Msg: Message rendering path
Room->>Msg: pass highlights (user.settings.preferences.highlights)
Msg->>Msg: compute isHighlighted (prop || contains any term)
Msg->>MD: provide highlights via context
MD->>Plain: highlights available
Plain->>Plain: normalize & escape terms, build regex, split text
Plain->>Plain: render segments with highlight styling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/containers/message/Content.tsx (1)
89-115: Addhighlightsto the memo comparison.The memo comparison function doesn't check for
highlightsprop changes. When user highlight preferences update, the Content component won't re-render, causing stale highlighting.Apply this diff to fix the comparison:
}, (prevProps, nextProps) => { if (prevProps.isTemp !== nextProps.isTemp) { return false; } if (prevProps.msg !== nextProps.msg) { return false; } if (prevProps.type !== nextProps.type) { return false; } if (prevProps.isEncrypted !== nextProps.isEncrypted) { return false; } if (prevProps.isIgnored !== nextProps.isIgnored) { return false; } if (!dequal(prevProps.md, nextProps.md)) { return false; } if (!dequal(prevProps.mentions, nextProps.mentions)) { return false; } if (!dequal(prevProps.channels, nextProps.channels)) { return false; } + if (!dequal(prevProps.highlights, nextProps.highlights)) { + return false; + } return true; }app/containers/message/index.tsx (1)
101-146: AddhighlightstoshouldComponentUpdatecheck.The component won't re-render when
highlightsprop changes, causing stale rendering when users update their highlight preferences.Apply this diff:
shouldComponentUpdate(nextProps: IMessageContainerProps, nextState: IMessageContainerState) { const { isManualUnignored } = this.state; const { threadBadgeColor, isIgnored, highlighted, previousItem, autoTranslateRoom, autoTranslateLanguage, isBeingEdited, showUnreadSeparator, - dateSeparator + dateSeparator, + highlights } = this.props; if (nextProps.showUnreadSeparator !== showUnreadSeparator) { return true; } if (nextProps.dateSeparator !== dateSeparator) { return true; } if (nextProps.highlighted !== highlighted) { return true; } + if (nextProps.highlights !== highlights) { + return true; + }Note: This uses reference equality. If you need deep comparison, use
!dequal(nextProps.highlights, highlights)instead.
🧹 Nitpick comments (3)
app/containers/markdown/components/Plain.tsx (2)
16-16: Remove unusedthemevariable.The ESLint warning is correct—
themeis destructured but never used in the component.Apply this diff:
- const { colors, theme } = useTheme(); + const { colors } = useTheme();
21-48: Consider extracting the plain text rendering to reduce duplication.The same JSX block is repeated three times for early returns when highlights are empty or invalid.
Apply this diff to eliminate duplication:
const Plain = ({ value }: IPlainProps): React.ReactElement => { const { colors } = useTheme(); const { highlights = [] } = useContext(MarkdownContext as any); const text = (value ?? '').toString(); + + const renderPlainText = () => ( + <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> + {text} + </Text> + ); if (!highlights || !highlights.length) { - return ( - <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> - {text} - </Text> - ); + return renderPlainText(); } // prepare case-insensitive set of highlight words const words = highlights.map((w: any) => w?.toString().trim()).filter(Boolean); if (!words.length) { - return ( - <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> - {text} - </Text> - ); + return renderPlainText(); } const wordsLower = new Set(words.map(w => w.toLowerCase())); // build regex to split and keep matched parts; guard pattern const pattern = words.map(escapeRegExp).filter(Boolean).join('|'); if (!pattern) { - return ( - <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> - {text} - </Text> - ); + return renderPlainText(); }app/i18n/locales/en.json (1)
642-647: Highlight-related i18n keys look good; consider consolidating duplicate description stringsThe new keys for highlights are clear and consistent with existing naming. If only one of
Highlight_Words_DescriptionorHighlights_Descriptionends up being used in the UI, you might later consolidate to a single key to avoid duplication, but this is purely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
app/containers/markdown/components/Plain.tsx(1 hunks)app/containers/markdown/contexts/MarkdownContext.ts(2 hunks)app/containers/markdown/index.tsx(3 hunks)app/containers/message/Content.tsx(1 hunks)app/containers/message/index.tsx(4 hunks)app/containers/message/interfaces.ts(1 hunks)app/definitions/IUser.ts(1 hunks)app/i18n/locales/en.json(1 hunks)app/views/RoomView/index.tsx(1 hunks)app/views/UserPreferencesView/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/markdown/components/Plain.tsx (1)
app/theme.tsx (1)
useTheme(29-29)
app/views/UserPreferencesView/index.tsx (5)
app/actions/login.ts (1)
setUser(91-96)app/definitions/IUser.ts (1)
IUser(128-152)app/lib/services/restApi.ts (3)
setUserPreferences(315-317)saveUserPreferences(649-651)getUserPreferences(367-369)app/lib/methods/helpers/showToast.ts (1)
showToast(4-4)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
🪛 ESLint
app/containers/markdown/components/Plain.tsx
[error] 16-16: 'theme' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (13)
app/views/RoomView/index.tsx (1)
1376-1376: LGTM! Clean pass-through of user highlight preferences.The optional chaining safely handles cases where settings or preferences might be undefined, and the prop is correctly forwarded to the Message component.
app/definitions/IUser.ts (1)
115-115: LGTM! Type definition is correct.The optional
highlights?: string[]field appropriately extends the notification preferences interface and maintains backward compatibility.app/containers/message/Content.tsx (1)
70-70: Forwarding highlights to Markdown is correct.The prop is correctly passed through to enable downstream highlighting.
app/containers/message/interfaces.ts (1)
73-73: LGTM! Interface extension is correct.The optional
highlightsproperty appropriately extends IMessageContent to support the highlighting feature.app/containers/markdown/contexts/MarkdownContext.ts (1)
13-13: LGTM! Context definition is correct.The highlights field is properly added to both the interface and default state, enabling downstream components to access highlight preferences.
Also applies to: 22-23
app/containers/markdown/components/Plain.tsx (1)
13-72: Highlighting implementation is well-designed.The regex-based approach correctly:
- Escapes special regex characters
- Performs case-insensitive matching
- Uses theme-aware colors with fallbacks
- Efficiently checks matches using a Set
app/containers/message/index.tsx (2)
66-66: LGTM! Props interface correctly extended.The
highlightsprop is appropriately added to support the highlighting feature.
431-433: Highlighting logic correctly combines prop and keyword-based highlighting.The implementation properly handles both explicit
highlightedprop and dynamic keyword matching, using case-insensitive comparison for consistency with the Plain component.app/containers/markdown/index.tsx (1)
35-35: LGTM! Markdown component correctly propagates highlights.The prop is properly defined, defaulted to an empty array, and passed through to the context provider for downstream consumption.
Also applies to: 49-49, 73-73
app/views/UserPreferencesView/index.tsx (4)
2-21: New imports are consistent and all usedThe added React hooks, REST helpers, toast helper, and UI components (FormTextInput, Button) are all used in this file and align with existing patterns; no issues here.
33-42: Highlight state initialization and sync from user settings look correctUsing
initialHighlightRefplushighlights/dirtystate and syncing them in auseEffectkeyed onsettings?.preferences?.highlightsis a reasonable way to keep the input in sync with server/user changes without losing in-progress edits.
56-62: toggleMessageParser optimistic update + setUserPreferences(id, …) looks goodThe optimistic
setUserdispatch that merges intosettings.preferencesand the switch tosetUserPreferences(id, { enableMessageParserEarlyAdoption: value })align with the REST helper’s signature and keep client and server in sync. This change looks correct.
125-130: Updated alsoSendThreadToChannel optimistic update pattern is solidMerging
paraminto...settings?.preferencesinside the optimisticsetUserdispatch avoids dropping other preference keys and is consistent with how you handleenableMessageParserEarlyAdoption. This change looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/views/UserPreferencesView/index.tsx (2)
145-146: Good fix to merge thread preferences; adjust comment wording (optional)Merging
...settings?.preferences, ...paramin thesetAlsoSendThreadToChanneldispatch now preserves other preference keys, which is the right behavior. Note that the comment still says “optimistic update”, but the dispatch happens after the awaitedsaveUserPreferencescall, so it’s technically a post-save sync rather than an optimistic update; you may want to rename the comment for clarity.
201-237: Highlights UI wiring is correct; consider centralizing save logic and adding a saving flagThe new Highlights section is nicely integrated:
- Input is bound to
highlightsanddirty.- Blur and Save button both route through
saveHighlights(highlights).- Placeholder and description use I18n keys, keeping localization consistent.
- The Save button is only shown when
dirtyis true, which matches user expectations.Two optional polish points:
Centralize “unchanged” detection
The blur handler re-parses and compares highlights to detectunchanged, duplicating logic fromsaveHighlights. You can simplify by always callingsaveHighlights(highlights)on blur and lettingsaveHighlights’ ownunchangedcheck decide whether to no-op.Optional: track a
savingflag to avoid concurrent saves
A simpleconst [saving, setSaving] = useState(false)inside this component and short-circuitingsaveHighlightswhensavingis true would prevent overlapping save/verify cycles and let you disable the Save button and/or show loading state while the request is in flight.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/UserPreferencesView/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/UserPreferencesView/index.tsx (5)
app/actions/login.ts (1)
setUser(91-96)app/definitions/IUser.ts (1)
IUser(128-152)app/lib/services/restApi.ts (3)
setUserPreferences(315-317)saveUserPreferences(649-651)getUserPreferences(367-369)app/lib/methods/helpers/showToast.ts (1)
showToast(4-4)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
🔇 Additional comments (3)
app/views/UserPreferencesView/index.tsx (3)
2-2: New imports for highlights feature are consistent and fully usedThe added imports (
useRef, REST helpers,showToast,FormTextInput, andButton) line up with the new highlights behavior and UI; nothing appears unused or mis-wired.Also applies to: 13-14, 19-20
33-42: Highlight local state and sync effect look solidUsing
initialHighlightRef,highlights, anddirtyplus theuseEffectkeyed onsettings?.preferences?.highlightsgives a clean source-of-truth flow and correctly resets local edits when server/user settings change. No changes needed here.
58-62: Optimistic update for message parser preference is correctly merged and shapedMerging
enableMessageParserEarlyAdoptionintosettings?.preferencesbefore callingsetUserPreferences(id, { enableMessageParserEarlyAdoption: value })avoids clobbering other preference keys and uses theuserId + datashape expected by the REST helper. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/views/UserPreferencesView/index.tsx (2)
83-86: Simplify the unchanged check.The condition
unchanged && !dirtyis confusing. If the parsed data is unchanged from the server value, the save should be skipped regardless of the UIdirtyflag. Including!dirtyin the condition means a save could proceed whendirtyis true even if data is unchanged (though theunchangedcheck prevents that). The logic is clearer and more maintainable if you simply checkunchangedalone.Apply this diff to simplify:
const unchanged = JSON.stringify(current) === JSON.stringify(words); - if (unchanged && !dirty) { - // No change, skip network/save and toasts + if (unchanged) { + // No change detected, skip save + setDirty(false); return; }
125-125: Baseline drift: normalize the baseline string to match the canonical form.This issue was flagged in the past review and remains unresolved. Setting
initialHighlightRef.current = valueuses the raw input string, which may differ from the canonical form (e.g.,"foo,bar"vs"foo, bar"). This causes the dirty check to use a baseline that doesn't match the rendered value, making the dirty state feel inconsistent.Apply this diff to normalize the baseline to the canonical form:
if (JSON.stringify(sortA) === JSON.stringify(sortB)) { - initialHighlightRef.current = value; + const normalized = words.join(', '); + initialHighlightRef.current = normalized; + setHighlights(normalized); setDirty(false); showToast(I18n.t('Highlights_saved_successfully'));Based on past review comments
🧹 Nitpick comments (2)
app/views/UserPreferencesView/index.tsx (2)
145-154: Clarify comment: this is a confirmed update, not optimistic.The comment on Line 148 says "optimistic update" but the dispatch happens after the successful API call (Line 147), not before. This is actually a confirmed/reactive update pattern. While the implementation is correct and safe, consider updating the comment to avoid confusion:
await saveUserPreferences(param); - // optimistic update merging into preferences + // Update local state after successful save dispatch(setUser({ settings: { ...settings, preferences: { ...settings?.preferences, ...param } } } as Partial<IUser>));
228-228: Consider extracting inline style to avoid recreating the object on every render.The inline style object
{ alignSelf: 'center', marginTop: 15}will be recreated on every render. While unlikely to cause noticeable performance issues in this case, extracting it to a constant or styles object follows best practices.Extract to a constant outside the component:
const styles = StyleSheet.create({ saveButton: { alignSelf: 'center', marginTop: 15 } });Then use:
- style={{ alignSelf: 'center', marginTop: 15}} + style={styles.saveButton}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/UserPreferencesView/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/UserPreferencesView/index.tsx (5)
app/actions/login.ts (1)
setUser(91-96)app/definitions/IUser.ts (1)
IUser(128-152)app/lib/services/restApi.ts (3)
setUserPreferences(315-317)saveUserPreferences(649-651)getUserPreferences(367-369)app/lib/methods/helpers/showToast.ts (1)
showToast(4-4)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
🔇 Additional comments (2)
app/views/UserPreferencesView/index.tsx (2)
33-42: LGTM! State synchronization pattern is sound.The ref and state initialization correctly tracks the baseline highlights value, and the
useEffectproperly resets both the ref and state when the server data changes, ensuring the UI stays in sync with external updates.
56-65: LGTM! Optimistic update pattern is correctly implemented.The function properly dispatches the optimistic state update using the
setUseraction creator before making the API call, with a well-structured payload.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/UserPreferencesView/index.tsx (1)
67-71: Fix preferences merge intoggleConvertAsciiToEmojito avoid clobbering other settingsIn
toggleConvertAsciiToEmoji, the optimistic update builds:dispatch(setUser({ settings: { ...settings, preferences: { convertAsciiEmoji: value } } } as Partial<IUser>));This replaces the entire
settings.preferencesobject with onlyconvertAsciiEmoji, dropping other preference fields (including the newhighlights,enableMessageParserEarlyAdoption, etc.) until the next full user sync.To keep all preferences intact (and consistent with the other toggles), spread the existing preferences:
- dispatch(setUser({ settings: { ...settings, preferences: { convertAsciiEmoji: value } } } as Partial<IUser>)); + dispatch(setUser({ + settings: { + ...settings, + preferences: { + ...settings?.preferences, + convertAsciiEmoji: value + } + } + } as Partial<IUser>));This avoids accidental loss of other settings in the Redux user slice.
♻️ Duplicate comments (1)
app/views/UserPreferencesView/index.tsx (1)
33-42: Keep highlights baseline and visible value in sync after saves (normalization)The combination of
initialHighlightRef/highlights(Lines 33‑42) andsaveHighlights(Lines 76‑140) works, but the baseline string can drift from the canonical version shown in the input:
useEffectnormalizes fromsettings?.preferences?.highlights?.join(', ').saveHighlightsverifies via normalized arrays, but on success setsinitialHighlightRef.current = valuewithout updatinghighlightsor normalizing the string.If the server (or the join logic) formats the string differently than what the user typed (spaces, ordering), the ref and the visible value can diverge, which makes the dirty check (
value !== initialHighlightRef.current) less intuitive.To keep everything aligned, consider normalizing once and using that everywhere on success:
- if (JSON.stringify(sortA) === JSON.stringify(sortB)) { - initialHighlightRef.current = value; - setDirty(false); - showToast(I18n.t('Highlights_saved_successfully')); - } + if (JSON.stringify(sortA) === JSON.stringify(sortB)) { + const normalized = words.join(', '); + initialHighlightRef.current = normalized; + setHighlights(normalized); + setDirty(false); + showToast(I18n.t('Highlights_saved_successfully')); + }Optionally, you can also drop the
!dirtypart of the early‑return and letunchangedalone gate the no‑op path, sinceunchangedis already computed from the normalized arrays.Also applies to: 76-140
🧹 Nitpick comments (3)
app/views/UserPreferencesView/index.tsx (3)
56-62: Message parser toggle flow looks good; consider rollback on failure (optional)The updated
toggleMessageParsernow:
- Optimistically merges the flag into
settings.preferencesviasetUser.- Calls
setUserPreferences(id, { enableMessageParserEarlyAdoption: value })with the correct payload shape.That’s a solid improvement. If you want to be extra defensive, you could capture the previous value and roll back the optimistic update when the API call fails, but this is optional given current UX.
76-140:saveHighlightslogic is robust; consider small hardening and cleanupOverall the flow (parse/normalize → optimistic
setUser→saveUserPreferences→getUserPreferencesverification → toasts) is solid. A few optional refinements:
- Deduplicate words: currently
"foo, foo"produces['foo', 'foo']. If the server dedupes, you’ll get a mismatch and show a failure toast even though data is effectively saved. A simpleArray.from(new Set(words))before save would avoid that.- Avoid double saves: since
saveHighlightsis called on blur and via the Save button, a tap on Save may trigger two calls in quick succession. Tracking anisSavingflag (ref/state) and early‑returning when a save is in flight would prevent duplicate requests and duplicate toasts.- Centralize “unchanged” handling: you already compute
unchangedfrom normalized arrays; relying solely on that (and notdirty) for the no‑op path would simplify reasoning and reduce coupling to the UI’s local dirty flag.These are polish items; the current implementation should work functionally.
201-231: Highlights UI wiring is correct; minor UX/a11y and double‑save refinementsThe Highlights section correctly binds the input to
highlights/dirty, callssaveHighlightson blur and via the Save button, and uses i18n keys for placeholder/description. A couple of small improvements:
Provide an explicit label for the text input (a11y/clarity)
FormTextInputsupports alabelandaccessibilityLabel. Right now the only textual cue directly attached to the input is the placeholder. Passing the same “Highlights” label improves clarity and screen‑reader behavior:- <FormTextInput - value={highlights} + <FormTextInput + label={I18n.t('Highlights')} + value={highlights} onChangeText={value => { setHighlights(value); setDirty(value !== initialHighlightRef.current); }}Avoid potential duplicate saves on Save tap
BecauseonBlurand the Save button both callsaveHighlights(highlights), tapping Save may trigger two saves (blur + press). If you adopt anisSavingflag insaveHighlights(as mentioned earlier), you can early‑return when a save is already in progress and avoid double API calls/toasts.These are non‑blocking refinements; the current behavior is functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/UserPreferencesView/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/UserPreferencesView/index.tsx (5)
app/actions/login.ts (1)
setUser(91-96)app/definitions/IUser.ts (1)
IUser(128-152)app/lib/services/restApi.ts (3)
setUserPreferences(315-317)saveUserPreferences(649-651)getUserPreferences(367-369)app/lib/methods/helpers/showToast.ts (1)
showToast(4-4)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
🔇 Additional comments (2)
app/views/UserPreferencesView/index.tsx (2)
2-20: New imports are coherent with usageThe added imports (React hooks, rest API helpers, showToast, FormTextInput, Button) are all used in this file and wired consistently with existing patterns; no issues here.
142-147: Thread‑to‑channel preference optimistic merge looks correctThe updated
setAlsoSendThreadToChannelmergesparamintosettings.preferencesviasetUserafter a successfulsaveUserPreferencescall, preserving other preference keys (includinghighlights). This is consistent with the other preference update paths and looks good.
DSingh0304
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
Hey! Thank you for your contribution. Here are a few changes:
- Lint is failing.
- E2E tests are missing.
- Storybook is missing.
Please add screenshots to the PR description and include Storybook cases for message-validation scenarios, such as highlighted messages with bold, italic and code. We need to make sure all cases are working correctly.
|
Yeah, sure I will make the changes and update the PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/containers/markdown/HighlightWords.stories.tsx (1)
30-36: Story demonstrates expected behavior but implementation doesn't match (see Plain.tsx critical issue).Line 34's comment states "only exact words defined will match", expecting "rocket" to NOT highlight within "rockets" or "rocketing". However, the current implementation in Plain.tsx lacks word boundaries and will incorrectly match partial words. This has been flagged as a critical issue in the Plain.tsx review.
Once the word boundary fix is applied in Plain.tsx (adding
\baround the pattern), this story will correctly demonstrate the expected behavior.
🧹 Nitpick comments (3)
app/containers/markdown/components/Plain.tsx (3)
17-17: Replaceanywith proper MarkdownContext type.Using
anybypasses TypeScript's type checking. Define or import the proper context type to ensure type safety.Apply this diff:
- const { highlights = [] } = useContext<any>(MarkdownContext); + const { highlights = [] } = useContext(MarkdownContext);If MarkdownContext doesn't have proper typing, consider adding an interface for its value shape.
21-48: Consider consolidating duplicate return blocks.The three identical early-return blocks (lines 21-27, 31-37, 42-48) could be consolidated by moving validation checks together at the top and rendering the default text after all checks.
Example consolidation:
const text = (value ?? '').toString(); - if (!highlights || !highlights.length) { - return ( - <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> - {text} - </Text> - ); - } - // prepare case-insensitive set of highlight words const words = highlights.map((w: any) => w?.toString().trim()).filter(Boolean); - if (!words.length) { - return ( - <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> - {text} - </Text> - ); - } - const wordsLower = new Set(words.map((w: string) => w.toLowerCase())); // build regex to split and keep matched parts; guard pattern const pattern = words.map(escapeRegExp).filter(Boolean).join('|'); - if (!pattern) { + + if (!highlights?.length || !words.length || !pattern) { return ( <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> {text} </Text> ); } + const re = new RegExp(`(${pattern})`, 'ig');
49-50: Consider memoizing regex compilation for better performance.The regex is compiled on every render. In message-heavy views (like chat rooms), this could impact performance. Consider using
useMemoto cache the compiled regex when the highlights array reference doesn't change.Example optimization:
const re = useMemo(() => { const words = highlights.map((w: any) => w?.toString().trim()).filter(Boolean); if (!words.length) return null; const pattern = words.map(escapeRegExp).filter(Boolean).join('|'); if (!pattern) return null; return new RegExp(`\\b(${pattern})\\b`, 'ig'); }, [highlights]); if (!re) { return ( <Text accessibilityLabel={text} style={[styles.plainText, { color: colors.fontDefault }]}> {text} </Text> ); } const parts = text.split(re); // ... rest of logicNote: Using array index as key (lines 63, 68) is acceptable here since the parts array is derived from a deterministic split operation and won't be reordered.
Also applies to: 58-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/containers/markdown/HighlightWords.stories.tsx(1 hunks)app/containers/markdown/components/Plain.test.tsx(1 hunks)app/containers/markdown/components/Plain.tsx(1 hunks)app/views/RoomView/index.tsx(1 hunks)app/views/UserPreferencesView/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/UserPreferencesView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/containers/markdown/components/Plain.test.tsx (2)
app/lib/constants/colors.ts (1)
colors(280-302)app/theme.tsx (1)
ThemeContext(18-18)
app/containers/markdown/components/Plain.tsx (1)
app/theme.tsx (1)
useTheme(29-29)
app/containers/markdown/HighlightWords.stories.tsx (1)
app/lib/constants/colors.ts (1)
themes(304-304)
🔇 Additional comments (6)
app/containers/markdown/components/Plain.tsx (2)
1-1: LGTM: Imports and regex escaping helper are correct.The added imports support the highlighting feature, and the
escapeRegExphelper properly escapes all regex special characters to prevent injection issues when building the pattern from user-provided highlight words.Also applies to: 7-7, 13-13
53-54: LGTM: Theme-aware colors with appropriate fallbacks.The highlight colors correctly use theme tokens (
statusBackgroundDanger,statusFontDanger) with sensible fallbacks, ensuring consistent theming across light and dark modes.app/containers/markdown/HighlightWords.stories.tsx (2)
1-17: LGTM: Story setup is appropriate.The imports, theme selection, and styling are correctly configured for a Storybook story. Hardcoding the
'light'theme (line 9) is acceptable for demonstration purposes.
19-28: LGTM: Story metadata is correctly structured.The title and NavigationContainer decorator are appropriate for demonstrating the Markdown highlighting feature.
app/containers/markdown/components/Plain.test.tsx (2)
1-15: LGTM: Test setup and helper are well-structured.The imports and
getBackgroundhelper correctly handle React Native's style arrays to extract the background color for assertions.
76-90: LGTM: No-highlights test correctly validates early return.This test properly verifies that when no highlights are configured, the full text is rendered without creating separate highlighted nodes, confirming the early-return optimization works correctly.
…es and improved regex matching
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/containers/markdown/components/Plain.tsx (1)
55-55: Word boundaries correctly implemented.The regex construction now includes word boundaries (
\b) at both ends of the pattern, addressing the critical issue from the previous review. The pattern(\\b(?:${pattern})\\b)ensures that highlight words match only as complete words, not as substrings within larger words (e.g., "rocket" won't match within "rockets").The use of a non-capturing group
(?:...)for the alternation is correct and prevents duplicate entries when splitting.
🧹 Nitpick comments (2)
app/containers/markdown/components/Plain.tsx (2)
17-17: Replaceanywith proper MarkdownContext type.The
useContext<any>(MarkdownContext)call defeats TypeScript's type checking. Based on the AI summary,MarkdownContextexportsIMarkdownContextwhich includes thehighlightsfield.Apply this diff to use the proper type:
- const { highlights = [] } = useContext<any>(MarkdownContext); + const { highlights = [] } = useContext(MarkdownContext);TypeScript can infer the correct type from
MarkdownContextautomatically. If explicit typing is needed, import and useIMarkdownContextfrom the context file.
30-44: Remove unnecessaryanytype annotations.Lines 30 and 44 use
(w: any)in the map callbacks. Sincehighlightsis typed asstring[]in the context, these can use(w: string)for better type safety.Apply this diff:
- const words = highlights.map((w: any) => w?.toString().trim()).filter(Boolean); + const words = highlights.map((w: string) => w.trim()).filter(Boolean);Since
wis already a string, the optional chaining andtoString()call are unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/containers/markdown/components/Plain.test.tsx(1 hunks)app/containers/markdown/components/Plain.tsx(1 hunks)app/definitions/IUser.ts(2 hunks)app/views/RoomView/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/containers/markdown/components/Plain.test.tsx
- app/views/RoomView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/markdown/components/Plain.tsx (1)
app/theme.tsx (1)
useTheme(29-29)
🔇 Additional comments (5)
app/definitions/IUser.ts (2)
115-115: LGTM! Highlights field correctly added.The
highlightsfield is properly typed as an optional string array, matching the web version's API contract and maintaining backward compatibility through the optional modifier.
102-104: Type change is backward compatible—all preference accesses use properly typed keys.Verification confirms no code in the codebase accesses
preferenceswith arbitrary string keys. All usages reference typed properties likehighlights(fromINotificationPreferences) andconvertAsciiEmoji(fromIMessagePreferences), which are defined in the interface intersection. The type change is safe and will not break existing code.app/containers/markdown/components/Plain.tsx (3)
13-13: LGTM! Regex escaping is correct.The
escapeRegExphelper properly escapes all special regex metacharacters, ensuring highlight words with special characters are treated as literals.
21-37: LGTM! Defensive checks prevent errors.The multiple early-return guards (lines 21-27, 31-37) ensure robust handling of empty or invalid highlight data, preventing downstream errors while maintaining the existing plain-text rendering path.
58-76: LGTM! Highlight rendering is correct and theme-aware.The rendering implementation properly:
- Uses theme colors with safe fallbacks (lines 59-60)
- Preserves accessibility with the original text label (line 63)
- Applies case-insensitive matching (line 66)
- Conditionally styles matched segments with danger background/text colors
The index-based keys are acceptable here since the entire text re-renders when highlights change.
|
Hey, I have updated the PR and uploaded the screenshots. Please review once. |
2 participants
Proposed changes
This PR implements the highlight words feature in the Rocket.Chat React Native app, bringing it in sync with the web version. Users can now set highlight words in their preferences, and matching words in messages will be highlighted with a red background.
Key Changes:
highlights) matching web versionHow to test or reproduce
##Screenshots
Screen Recording
Screencast.from.2025-11-17.22-34-57.webm
Types of changes
Checklist
Further comments
Files Modified:
app/views/UserPreferencesView/index.tsx- Added highlights UI and save logicapp/containers/markdown/components/Plain.tsx- Implemented text highlightingapp/containers/markdown/contexts/MarkdownContext.ts- Added highlights contextapp/containers/markdown/index.tsx- Passed highlights to contextapp/containers/message/Content.tsx- Propagated highlights to Markdownapp/containers/message/index.tsx- Added highlight detection logicapp/containers/message/interfaces.ts- Added highlights propapp/definitions/IUser.ts- Extended notification preferencesapp/i18n/locales/en.json- Added translation keysapp/views/RoomView/index.tsx- Passed highlights from user settingsTesting Results:
✅ All existing tests pass
✅ Markdown rendering tests: 11/11 passed
✅ No breaking changes to existing functionality
Closes: #6371
This feature aligns the mobile app with the Rocket.Chat web version's highlight words functionality.
Summary by CodeRabbit
New Features
UI
Localization
Tests
Documentation