feat: message search within conversation - #622
Conversation
8eb4a41 to
3838f07
Compare
There was a problem hiding this comment.
Thanks for this — the HTML-splitting highlighter is careful work (entity and code-region guards are more than most attempts do), and the store/component split is clean.
I ran the branch on an iOS simulator against a seeded session to check the behaviour rather than argue from the code alone. Four issues are visually reproducible; the screenshots are attached inline where the code causes them.
The seeded conversation used for every capture, oldest first:
| # | author | content |
|---|---|---|
| 1 | user | How do leaves make energy? |
| 2 | assistant (assistant_turn) |
Leaves use chlorophyll inside the chloroplasts to turn sunlight into chemical energy. |
| 3 | user | I don't remember the gradient rule for water uptake |
| 4 | assistant (assistant_turn) |
The concentration gradient drives water uptake through the roots. |
Blocking
- Search never matches assistant replies — they are
assistant_turnrows, nottext. - Filtering the
messagesprop makes a zero-result search render the new-chat empty state.
Should fix before merge
- The highlight background is invisible in the light theme (it works in dark).
- Queries containing
',&,<,>are counted but never highlighted. createNewSessiondoesn't reset search state.- A self-closing
<code/>silently disables highlighting for the rest of a message.
Non-visual items (performance, tests, a11y, minor polish) are in a follow-up comment so this review stays readable.
Verification on the branch: tsc --noEmit clean, eslint 0 errors, jest 263 suites / 3975 tests green. No native changes. Note the branch is 37 commits behind main and currently conflicts in ChatScreen.tsx.
Generated by PocketPal Dev Team
|
|
||
| const query = this.searchQuery.toLowerCase().trim(); | ||
| return messages.filter( | ||
| msg => msg.type === 'text' && msg.text.toLowerCase().includes(query), |
There was a problem hiding this comment.
Blocking — search can never match an assistant reply.
Every assistant reply the current pipeline produces is an assistant_turn row, created at src/hooks/useChatSession.ts:198-203. MessageType.AssistantTurn has no text field — the content lives in steps[].content. So msg.type === 'text' && msg.text only ever matches user messages.
ChatSessionRepository.ts:472-478 states the contract:
For
assistant_turnrows thetextcolumn stays empty; every consumer routes throughderivedText(message).
Reproduced on a simulator. The word "energy" appears in both a user message and the assistant reply — the count reads 1 and the reply is gone:
The helper is already imported in this file (line 18) and used at line 482:
return messages.filter(msg =>
derivedText(msg).toLowerCase().includes(query),
);Worth fixing the test alongside it: ChatSessionStore.test.ts:2590 ("excludes non-text messages") uses an image row, so it passes both before and after this change and doesn't pin the behaviour. An assistant_turn fixture would.
One heads-up: fixing this puts large assistant bodies into the highlight path for the first time, which is what makes the re-render cost in my follow-up comment real. Worth doing both together.
| // Compute messages and search match count | ||
| const messages = | ||
| chatSessionStore.isSearchMode && chatSessionStore.searchQuery.trim() | ||
| ? chatSessionStore.filteredSessionMessages |
There was a problem hiding this comment.
Blocking — swapping the messages prop breaks derivations that treat it as conversation truth.
ChatView.messages isn't just the render list. Filtering it also changes:
ChatView.tsx:810—newestMessageId = messages[0].id, the active-vs-persisted predicateChatView.tsx:1104-1119— the html-preview soft cap, which countsassistant_turnrowsBannerRow.tsx:39—messages.find(m => m.type === 'assistant_turn'), so the context-limit / heavy-talent banners go dead during searchChatView.tsx:998+912-932—messages.length === 0rendersListEmptyComponentChatView.tsx:1184— the same condition floats the pal's suggested-prompts row, whoseonSelectsends a message
The last two are the visible one. Searching a word that only exists in an assistant reply gives "No results" and replaces the conversation with the new-chat empty state — it reads as though the chat was deleted:
(This sim had no model downloaded, hence the "No Models Available" variant; with a model you'd get the greeting bubble and suggested prompts instead.)
Suggestion: keep messages unfiltered and apply the search at the row-render layer, or give search its own results mode with its own empty state and the composer suppressed. The composer staying live is its own trap — you can send a message during search and it'll never appear, because the reply is an assistant_turn and gets filtered out.
| () => ({ | ||
| ...createTagsStyles(theme), | ||
| mark: { | ||
| backgroundColor: theme.colors.tertiaryContainer, |
There was a problem hiding this comment.
The highlight background is invisible in the light theme.
Measured from the screenshots below (sampled pixels, not estimates):
| theme | mark background | vs bubble background | contrast |
|---|---|---|---|
| light | #F1F3FF |
#F2F2F2 |
1.01:1 |
| dark | #016665 |
#212121 |
2.37:1 |
Same query, same message, light then dark:
Dark is a proper highlight. In light the only cue is the text colour shifting to #013332 against normal #111111 — 1.37:1, a hue shift you can find if you already know where to look, but not something the eye can scan to. Worth being precise: it isn't literally nothing in light mode, but it doesn't function as a highlight.
tertiaryContainer is a container token, not a highlight token, and it happens to be near-white in light. Dedicated searchHighlight / onSearchHighlight tokens clearing ~3:1 in both themes would fix it.
Minor: borderRadius isn't honoured on nested RN Text, so it's inert here.
| if (entity) { | ||
| return entity; | ||
| } | ||
| return plain.replace(matchRegex, '<mark>$1</mark>'); |
There was a problem hiding this comment.
Queries containing ', &, < or > are counted but never highlighted.
The filter in the store matches the raw markdown, while this runs on marked() output, where those characters are already escaped. The entity guard above then skips them by design, so the two can't agree.
don't is the everyday case — marked turns it into don't. The message is listed and the count says 1, but nothing is highlighted:
Confirmed by pixel-sampling both captures: the mark background #F1F3FF is present in the gradient screenshot and absent everywhere in this one.
Same class of mismatch applies to markdown syntax and to matches that only exist inside a code block.
Fix is either decoding entities before matching and re-escaping on emit, or making the store filter operate on the same text this function sees. Either way the count and the highlight should come from one definition of "match".
Note the test never highlights inside HTML entities currently locks the behaviour in, so it'd need to move with the fix.
| (_full, tag: string, text: string) => { | ||
| if (tag) { | ||
| const lower = tag.toLowerCase(); | ||
| if (/^<(pre|code)[\s/>]/.test(lower)) { |
There was a problem hiding this comment.
A self-closing <code/> leaks codeDepth and disables highlighting for the rest of the message.
/^<(pre|code)[\s/>]/ matches <code/>, which increments the depth with no closing tag to unwind it. marked.use({}) has no sanitiser, so raw HTML from a model reaches this string:
highlightSearchMatches('<p><code/>hello world and more hello</p>', 'hello')
// → unchanged, no marks at all
highlightSearchMatches('<p><span class="code">hello</span></p>', 'hello')
// → '…<mark>hello</mark>…' (control: the guard is otherwise fine)Anchoring both branches fixes it, e.g. /^<(pre|code)(\s[^>]*)?>$/.
Credit where due: I tried to break this function on attribute values, <br/>, nested <pre><code class=…>, uppercase <CODE>, and an unbalanced </code>, and it handled all of them correctly. This is the only case that got through.
| // Instead, preserve global settings as they are | ||
| this.exitEditMode(); | ||
| // Search is a per-session view; don't carry it into a new chat. | ||
| this.isSearchMode = false; |
There was a problem hiding this comment.
createNewSession is the one session-switch path without this reset.
This reset and the matching one in setActiveSession cover their paths, but createNewSession (line ~621) writes activeSessionId without clearing search state. It's reachable one tap from the new search icon: header ⋮ → Duplicate → duplicateSession → createNewSession. The new session opens still filtered by the previous query.
Calling the existing exitSearchMode() from all three writers would cover it and remove the duplicated inline reset.
Remaining review notes (non-visual)Follow-up to the inline review. Nothing here is reproducible in a screenshot, so it's grouped rather than anchored to lines. Performance — worth handling together with the
|
Search finds text in the active chat session. Matches are highlighted in place (find-in-page): the full conversation stays visible and a match count is shown, rather than filtering the message list. - Toolbar icon toggles a search bar with an auto-focus input. - Matches are highlighted in message content via <mark> tags, using dedicated searchHighlight / onSearchHighlight theme tokens that clear ~3:1 against the message background in both light and dark themes. - Match count (or "No results") is shown as the user types. - SearchQueryContext passes the query to MarkdownView without prop drilling; the `mark` element model + style are registered once on MarkdownProvider. Why find-in-page rather than filtering the message list: ChatView's `messages` prop is conversation truth — it drives the empty state, context banners, and the suggested-prompts row. Swapping it for a filtered list made a zero-result search render the new-chat state, as if the chat were deleted. Highlighting in place never touches that prop. Correctness: - Match count and highlight both derive from `derivedText`, so assistant replies (`assistant_turn` rows, whose `text` column is empty) are searched via their step content instead of being skipped. - The highlighter decodes basic HTML entities before matching and re-encodes after, so a query containing ' & < or > highlights and agrees with the count. - A self-closing <code/> no longer leaks the code-region guard and suppresses highlighting for the rest of a message. - Search state resets on session create, switch, and reset; opening search leaves edit mode so the whole conversation is searched. Closes a-ghorbani#603
3838f07 to
35428d5
Compare
|
Thanks — the two blocking bugs and the highlight issues were all reproducible as described. Rebased onto current The one design change worth calling out: I switched from filtering the message list to find-in-page — the full conversation stays visible, matches highlight in place, and a count is shown. This is the root fix for blocking #2: Blocking
Should-fix Also: rewrote the store's Search-mode tests (the old ones used an Verification: Deferred (non-blocking, tracked for a follow-up): the perf debounce / query-as-prop change (find-in-page highlights all visible rows, so it's still worth doing), the a11y set (BackHandler, live region, hitSlop, match-count wording), and the minor polish (l10n dedup, |
Visual evidence — search projection + match navigation (iPhone 17 Pro sim, iOS 26, seeded DB, no inference)Generated by PocketPal Dev Team |
|
Re-reviewed the rebased branch on an iPhone 17 Pro simulator (iOS 26) with the conversation seeded straight into SQLite — no model, no inference. Verification below is from those runs, not from reading the diff. The six fixes holdI re-ran the mutations that originally caught each one, against the new code. Every guard still bites: reverting the Blocking — the entity fix introduced a rendering regression
Same session, same scroll, search bar open in both — only the query differs: It fires on every text run, not just matching ones — a message containing The cause is the decode-all/encode-all round trip. Matching needs the decoded string; emitting needs the original. An implementation you can take, if usefulRather than describe it, I built it: It builds a small projection per message — the visible text plus, per visible character, the byte range it occupies in the rendered HTML. Matching runs on the visible text; emitting splices That deletes the tag walking, the Inline code is highlightable (it renders through the default renderer); fenced code isn't, because Behaviour changes worth an explicit decision: the count reports occurrences rather than messages, and a match may span inline markup ( Match navigation and menu placementSince match positions fall out of the projection, prev/next was cheap to add — chevrons plus an Search also moves from the chat header into the overflow menu. That unloads a header carrying up to four controls, and it resolves two accessibility problems for free: the icon reused the placeholder string as its label and reported Still open
Verification
Two notes on my own branch, so you can weigh it fairly: an independent review of it found four issues I'd missed (a memory regression, a Generated by PocketPal Dev Team |











Summary
<mark>tags viaSearchQueryContext(avoids prop drilling through ChatView → Message → TextMessage → MarkdownView).Why find-in-page rather than filtering the list
ChatView'smessagesprop is conversation truth — it drives the empty state, the context banners, and the suggested-prompts row (whoseonSelectsends a message). Swapping it for a filtered list made a zero-result search render the new-chat state, as if the chat had been deleted. Highlighting in place never touches that prop, so all of those derivations stay anchored to the real conversation.Implementation details
isSearchMode,searchQuery,enterSearchMode()(also exits edit mode),exitSearchMode(),setSearchQuery(), and asearchMatchCountgetter. Count derives fromderivedText, so assistant replies (assistant_turnrows, whosetextcolumn is empty) are searched via their step content. Search state resets on session create, switch, and reset.customContent; wraps ChatView inSearchQueryContext.Provider.SearchQueryContextand wraps matches in<mark>after markdown→HTML. The highlighter decodes the basic HTML entities before matching and re-encodes after (so a query with',&,<,>highlights and agrees with the count), skips code/pre regions, and no longer leaks on a self-closing<code/>.markelement model and styles it with dedicatedsearchHighlight/onSearchHighlighttheme tokens that clear ~3:1 against the message background in both themes (the oldtertiaryContainerwas invisible in light mode).searchHighlight/onSearchHighlightadded to the token palette andSemanticColors.chat.searchMessages,chat.noResults,chat.closeSearchin en.json.Test plan
searchMatchCount(user + assistant_turn fixtures, whitespace/not-in-mode gating), edit-mode exit on open, and search-state reset on create + session switch. Mutation-verified: reverting to themsg.type === 'text'filter turns the assistant-reply test red.<code/>, code-block skip, and a render test asserting thesearchHighlighttoken is applied (mutation-verified againsttertiaryContainer).tsc --noEmitclean ·eslintclean ·l10n:validatevalid.Acceptance criteria (#603)
ChatView'smessagesprop corrupts its conversation-truth derivations (empty state, banners, suggested-prompts), which was blocking bug [feat] Add n_gpu_layer param to the settings #2. Flagging for explicit sign-off; happy to add prev/next jump or revisit if you'd prefer literal list-filtering.<mark>+ dedicated theme tokens.exitSearchMode.Deferred (non-blocking, follow-up)
Perf debounce / query-as-prop, the a11y set (BackHandler, live region, hitSlop, match-count wording), and minor polish (l10n dedup,
autoFocusprop,spellCheck, StyleSheet→createStyles).Closes #603