You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The conversation: I proposed the scope in this comment before writing anything, including two possible implementations and three questions. No maintainer has replied yet, so what follows is the scope I proposed, not a scope anyone agreed to — I'm opening it as a concrete starting point and I'm happy to cut it back or re-do it the other way if you'd prefer.
What it covers
Of the issue's acceptance criteria:
Highlight searched words inside message content
Works for plain text, links, bold/italic text
Rocket.Chat-like highlight — uses the existing warning / warningForeground theme tokens rather than a hard-coded yellow, so it survives dark mode
How
Message bodies render as a parsed token tree (<Markup tokens={md} />), and every text leaf in that tree — including the text inside BoldSpan, ItalicSpan and link labels — bottoms out in PlainSpan. So the highlight lives in exactly one place:
markups/src/elements/highlightSegments.js — a pure function splitting a string around case-insensitive occurrences of a term.
markups/src/elements/PlainSpan.js — reads highlight from the existing MarkupInteractionContext and wraps matches in <mark>.
react/src/context/SearchHighlightContext.js — a new context only SearchMessages provides, carrying the term that produced the current results.
Markdown.js passes it into the markup context; SearchMessages.js provides it.
Nothing changes outside search. With no term, PlainSpan returns <>{contents}</> exactly as it does today — same early return, same output — so the main message list, threads and every other consumer of markups are untouched.
Out of scope (as proposed)
Highlighting in the main message list or threads, code blocks and inline code, file names in the Files view, fuzzy/regex matching, and jump-to-message from a result (that's #1057).
Video/Screenshots
No screenshot yet — this needs a running Rocket.Chat server with indexed messages to show meaningfully. Happy to add one if you'd like it before review.
PR Test Details
highlightSegments is covered by 7 tests: no term, whitespace-only term, no occurrence, match at start/middle/end, every occurrence rather than just the first, case-insensitive matching that preserves original casing, and that the segments rejoin to exactly the original string.
One caveat worth flagging: packages/react has no runnable test setup on develop — babel.config.js sets modules: false unconditionally, so jest fails with "Cannot use import statement outside a module" on any test file. #1376 fixes that. I ran these 7 tests locally with that change applied (all pass) but deliberately did not include it here, to keep this diff to the feature. packages/markups has no test runner at all (no type: module, bundled on publish), which is why the test sits in react and imports the helper across packages.
Also verified: yarn lint clean on every changed file, and yarn build in packages/react exits 0 (dist/cjs + dist/esm produced).
@Spiral-Memory — would you have a moment to look at this one, or point me at whoever owns that area?
Being upfront about where it stands: I proposed the scope on #1043 first and nobody has replied yet, so this is the scope I proposed, not one anyone agreed to. I'd rather be told it's the wrong shape now than have it sit.
The part I'd most want pushback on: the highlight lives in PlainSpan, which every message in the app renders through. I gated it behind a context that only SearchMessages provides, so with no search term PlainSpan returns <>{contents}</> exactly as it does today and nothing outside search changes — but that's still a shared file, and if you'd rather it never touched markups at all, say so and I'll redo it contained inside the search view, or close this.
Lint is clean, yarn build passes, and the pure helper has 7 tests. Happy to cut the scope down to whatever you'd actually merge.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Highlight searched terms in search results
Implements #1043.
The conversation: I proposed the scope in this comment before writing anything, including two possible implementations and three questions. No maintainer has replied yet, so what follows is the scope I proposed, not a scope anyone agreed to — I'm opening it as a concrete starting point and I'm happy to cut it back or re-do it the other way if you'd prefer.
What it covers
Of the issue's acceptance criteria:
warning/warningForegroundtheme tokens rather than a hard-coded yellow, so it survives dark modeHow
Message bodies render as a parsed token tree (
<Markup tokens={md} />), and every text leaf in that tree — including the text insideBoldSpan,ItalicSpanand link labels — bottoms out inPlainSpan. So the highlight lives in exactly one place:markups/src/elements/highlightSegments.js— a pure function splitting a string around case-insensitive occurrences of a term.markups/src/elements/PlainSpan.js— readshighlightfrom the existingMarkupInteractionContextand wraps matches in<mark>.react/src/context/SearchHighlightContext.js— a new context onlySearchMessagesprovides, carrying the term that produced the current results.Markdown.jspasses it into the markup context;SearchMessages.jsprovides it.Nothing changes outside search. With no term,
PlainSpanreturns<>{contents}</>exactly as it does today — same early return, same output — so the main message list, threads and every other consumer ofmarkupsare untouched.Out of scope (as proposed)
Highlighting in the main message list or threads, code blocks and inline code, file names in the Files view, fuzzy/regex matching, and jump-to-message from a result (that's #1057).
Video/Screenshots
No screenshot yet — this needs a running Rocket.Chat server with indexed messages to show meaningfully. Happy to add one if you'd like it before review.
PR Test Details
highlightSegmentsis covered by 7 tests: no term, whitespace-only term, no occurrence, match at start/middle/end, every occurrence rather than just the first, case-insensitive matching that preserves original casing, and that the segments rejoin to exactly the original string.One caveat worth flagging:
packages/reacthas no runnable test setup ondevelop—babel.config.jssetsmodules: falseunconditionally, so jest fails with "Cannot use import statement outside a module" on any test file. #1376 fixes that. I ran these 7 tests locally with that change applied (all pass) but deliberately did not include it here, to keep this diff to the feature.packages/markupshas no test runner at all (notype: module, bundled on publish), which is why the test sits inreactand imports the helper across packages.Also verified:
yarn lintclean on every changed file, andyarn buildinpackages/reactexits 0 (dist/cjs+dist/esmproduced).