feat(web,hub): add opt-in search for session message content - #1598
feat(web,hub): add opt-in search for session message content#1598techotaku39 wants to merge 31 commits into
Conversation
There was a problem hiding this comment.
Findings
- [Major] Deduplicate by session before applying the global result limit — a high-frequency recent session can consume the entire candidate row set and hide other matching sessions. Evidence hub/src/store/messageContentSearch.ts:209.
- [Major] Release target locks when clearing a message window — the existing invalidation path clears the window and immediately syncs, but the new lock makes that sync a no-op. Evidence web/src/lib/message-window-store.ts:788.
- [Minor] Clear stale content results before the debounce — old rows remain clickable under the new query for 180 ms and can open a mismatched message/query pair. Evidence web/src/components/SessionList.tsx:1295.
- [Minor] Remove fallback card highlights when changing or dismissing a target — class-only highlights are not selected by the current cleanup. Evidence web/src/components/AssistantChat/HappyThread.tsx:1530.
Summary
Review mode: initial
- Four findings: 2 Major, 2 Minor.
- Coverage gaps: no test for per-session result starvation, invalidation while a search target lock is held, query changes during debounce, or cleanup of the class-only highlight fallback.
Testing
- Not run (automation); static review only because PR code is untrusted and was not executed.
HAPI Bot
| const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50 | ||
| // Keep enough rows to deduplicate to one hit per session without making a | ||
| // broad content search scan an unbounded number of snippets. | ||
| const candidateLimit = Math.min(10_000, Math.max(200, safeLimit * 50)) |
There was a problem hiding this comment.
[MAJOR] Deduplicate by session before applying the result limit
candidateLimit is applied to matching message rows before seenSessions deduplicates them. Because rows are ordered by session recency first, one recent session with at least 2,500 matches (for the default limit) can occupy the entire candidate set; every other matching session is then omitted, so the endpoint may return only one session even though many match.
Suggested fix:
const rows = db.prepare(`
WITH ranked AS (
SELECT f.message_id, f.session_id, f.role, f.seq, f.created_at,
s.updated_at,
snippet(message_content_search, 0, '', '', '…', 24) AS snippet,
ROW_NUMBER() OVER (
PARTITION BY f.session_id
ORDER BY CAST(f.seq AS INTEGER) DESC
) AS session_rank
FROM message_content_search AS f
INNER JOIN sessions AS s
ON s.id = f.session_id AND s.namespace = ?
WHERE message_content_search MATCH ?
)
SELECT message_id, session_id, role, seq, created_at, snippet
FROM ranked
WHERE session_rank = 1
ORDER BY updated_at DESC, CAST(seq AS INTEGER) DESC
LIMIT ?
`).all(namespace, escapeFtsPhrase(normalizedQuery), safeLimit)Apply the same per-session ranking shape to the short-query LIKE branch, and add a test where one session has more than candidateLimit hits while another session also matches.
| sessionId: string, | ||
| options: { ensureAfterCurrent?: boolean } = {} | ||
| ): Promise<void> { | ||
| if (messageWindowTargetLocks.has(sessionId)) { |
There was a problem hiding this comment.
[MAJOR] Release target locks when clearing the message window
The new early return means syncTailMessages() cannot repopulate a locked session. Existing invalidation handling does clearMessageWindow(event.sessionId) followed by syncTailMessages(...) (web/src/App.tsx:294), but clearMessageWindow does not remove messageWindowTargetLocks. While the retained search target is active, an invalidation therefore empties the chat and the recovery sync is silently skipped.
Suggested fix:
export function clearMessageWindow(sessionId: string): void {
messageWindowTargetLocks.delete(sessionId)
tailSyncControllers.delete(sessionId)
clearPersistedState(sessionId)
// existing state reset
}Add a regression test that locks a session, clears it, then verifies a subsequent tail sync calls getMessages and repopulates the window.
| } | ||
|
|
||
| const controller = new AbortController() | ||
| const timer = window.setTimeout(() => { |
There was a problem hiding this comment.
[MINOR] Clear stale content results before starting the debounce
When normalizedQuery changes, the previous contentSearchResponse remains rendered until this timer fires 180 ms later. During that interval the rows belong to the old query, but clicks pass the new normalizedQuery as targetMessageQuery, producing a mismatched message/query jump.
Suggested fix:
setContentSearchResponse(null)
setContentSearchLoading(true)
setContentSearchError(false)
const timer = window.setTimeout(() => {
void api.searchSessionContent(normalizedQuery, 50, controller.signal)
// existing handlers
}, 180)Add a component test that changes the query before the debounce expires and asserts that the old result is no longer clickable.
| scrollSearchTargetIntoView(matchMarker ?? target, viewportRef.current) | ||
| scheduleSearchTargetScroll(targetMessageId, targetSearchQuery, target) | ||
| if (!matchMarker) { | ||
| target.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS) |
There was a problem hiding this comment.
[MINOR] Clean up the class-only fallback highlight
When no text range can be created, this adds hapi-message-search-target directly to the message card. clearSearchTargetMarker() only selects elements with data-hapi-source-search-match="true", so this fallback highlight survives navigation to another match and dismissal of the search bar.
Suggested fix:
const highlighted = document.querySelectorAll<HTMLElement>(
`.${SEARCH_TARGET_HIGHLIGHT_CLASS}`
)
for (const element of highlighted) {
if (element.getAttribute(SEARCH_TARGET_MATCH_ATTRIBUTE) === 'true' && element.parentElement) {
removeSearchMatchMarker(element, element.parentElement)
} else {
element.classList.remove(SEARCH_TARGET_HIGHLIGHT_CLASS)
}
}Cover the fallback path with a test that dismisses the target and asserts the card class is removed.
There was a problem hiding this comment.
Findings
- [Major] Batch the FTS rebuild instead of materializing the full message scope — the v24 migration and session-merge rebuild load every selected content blob into JS before indexing, so peak memory scales with the entire history. Evidence hub/src/store/messageContentSearch.ts:121.
Suggested fix:let afterRowId = 0 while (true) { const batch = selectBatch.all(afterRowId, 500) as Array<DbMessageRow & { row_id: number }> if (batch.length === 0) break for (const row of batch) indexRow(row) afterRowId = batch[batch.length - 1]!.row_id }
- [Major] Resolve the rendered text part that actually contains the query — one stored assistant message can render several elements with the same source id; returning the first one makes later-block matches wait through retries and then highlight/scroll to the wrong block. Evidence web/src/chat/outline.ts:102.
Suggested fix:export function findConversationMessageAnchor(messageId: string, query?: string): HTMLElement | null { const candidates = getSourceMessageCandidates(messageId) if (query) { const matching = candidates.find((element) => findConversationMessageTextRange(element, query) !== null ) if (matching) return matching } return candidates[0] ?? null }
- [Minor] Exclude hidden AGENT_NOTIFY_SUMMARY footer text from the index — the default renderer strips this footer, but content search currently returns it in snippets and cannot highlight it in chat. Evidence shared/src/messages.ts:188.
Suggested fix:const rawText = directText ?? extractAssistantPlainText(record.content) ?? '' const text = normalizeSearchablePlainText(stripNotifySummaryFooter(rawText))
Summary
Review mode: follow-up after new commits
- The four prior findings no longer reproduce in the current diff. Three current findings remain: two Major, one Minor.
Testing
- Not run (automation); static review only because PR code is untrusted and was not executed. Current GitHub
testandintegrationchecks pass.
HAPI Bot
| SELECT id, session_id, content, created_at, seq, invoked_at | ||
| FROM messages | ||
| WHERE invoked_at IS NOT NULL | ||
| `).all() as DbMessageRow[] |
There was a problem hiding this comment.
[MAJOR] Batch the FTS rebuild instead of materializing the whole message scope
Both SELECT branches call .all(), so the v24 startup migration loads every invoked message content blob into JS at once; the session-specific branch does the same for merges. HAPI histories are not size-bounded, and compressed BLOB rows remain retained in rows while each item is decoded and inserted. A large existing database can therefore exhaust memory and prevent the hub from completing the upgrade.
Suggested fix:
let afterRowId = 0
while (true) {
const batch = db.prepare(`
SELECT rowid AS row_id, id, session_id, content, created_at, seq, invoked_at
FROM messages
WHERE invoked_at IS NOT NULL AND rowid > ?
ORDER BY rowid
LIMIT ?
`).all(afterRowId, 500) as Array<DbMessageRow & { row_id: number }>
if (batch.length === 0) break
for (const row of batch) indexRow(row)
afterRowId = batch[batch.length - 1]!.row_id
}Apply the same rowid pagination with the session predicate when sessionIds is present. Add a rebuild test that spans multiple batches and verifies every eligible row is indexed.
| */ | ||
| export function findConversationMessageAnchor(messageId: string): HTMLElement | null { | ||
| for (const element of document.querySelectorAll<HTMLElement>('[data-hapi-source-message-id]')) { | ||
| if (element.getAttribute('data-hapi-source-message-id') === messageId) return element |
There was a problem hiding this comment.
[MAJOR] Choose the source element that actually contains the query
A single stored Claude assistant message may contain multiple visible text blocks. extractAssistantPlainText() concatenates all of them into one indexed document, while NotifySummaryText renders each part with the same data-hapi-source-message-id. This loop always returns the first part. If the hit is in a later part, HappyThread retries the range lookup on the wrong element for about six seconds and finally applies the fallback highlight/scroll to that wrong part.
Suggested fix:
export function findConversationMessageAnchor(
messageId: string,
query?: string
): HTMLElement | null {
const exact = Array.from(document.querySelectorAll<HTMLElement>(
`[data-hapi-source-message-id="${CSS.escape(messageId)}"]`
))
const grouped = Array.from(document.querySelectorAll<HTMLElement>(
'[data-hapi-source-message-ids]'
)).filter((element) =>
element.getAttribute('data-hapi-source-message-ids')?.split(/\s+/).includes(messageId)
)
const candidates = [...exact, ...grouped]
if (query) {
const matching = candidates.find((element) =>
findConversationMessageTextRange(element, query) !== null
)
if (matching) return matching
}
return candidates[0] ?? document.getElementById(getConversationMessageAnchorId(messageId))
}Pass targetSearchQuery from HappyThread. Add a regression test with one assistant message containing two text blocks and a query present only in the second block.
| && typeof record.content.text === 'string' | ||
| ? record.content.text | ||
| : null | ||
| const text = normalizeSearchablePlainText(directText ?? extractAssistantPlainText(record.content) ?? '') |
There was a problem hiding this comment.
[MINOR] Do not index the hidden AGENT_NOTIFY_SUMMARY footer
The default chat renderer strips this machine footer when sessionSummaryInChat is off (NotifySummaryText.tsx:123-127), but the index stores the full assistant text here. Searching for footer-only summary/action text therefore exposes it in the session-list snippet, then opens a target whose rendered DOM does not contain the query and falls back to a card-only highlight. That contradicts the visible-text scope of this index.
Suggested fix:
const rawText = directText ?? extractAssistantPlainText(record.content) ?? ''
const text = normalizeSearchablePlainText(stripNotifySummaryFooter(rawText))
return text ? { role: 'assistant', text } : nullAdd an extraction/search test where ordinary prose is followed by a valid AGENT_NOTIFY_SUMMARY footer and assert that prose is searchable while footer-only terms are not.
There was a problem hiding this comment.
Findings
- [Major] Message ingestion performs an unindexed FTS delete for every row —
message_idisUNINDEXED, so the delete inindexMessageContent()scans the growing virtual table even for newly generated IDs. - [Major] Search indexes assistant records that the chat renderer intentionally hides —
isMetaandisCompactSummaryoutput can appear in results, but the target is removed when its context window is normalized. - [Major] Reloading a search context loses the required latest-tail reset — the persisted window keeps the real head cursor but omits
requiresLatestReset, so hydration can treat old context as a current tail.
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commits resolve the two findings from the prior HAPI Bot review. The full latest diff still has the three major issues above.
Testing
- Not run (automation); PR code was not executed during review.
- Add coverage for write-path query behavior, hidden meta/compact-summary extraction, and persistence/reload of a historical message window.
HAPI Bot
|
|
||
| export function indexMessageContent(db: Database, message: IndexableMessage): void { | ||
| ensureMessageContentSearchTable(db) | ||
| removeMessageContentSearchIndex(db, message.id) |
There was a problem hiding this comment.
[Major] Avoid a full FTS scan on every message insert. message_id is declared UNINDEXED at line 58, but every call here first executes DELETE ... WHERE message_id = ?. FTS5 cannot look up that column through its inverted index, and all normal create/import paths call this function with a newly generated ID, so ingestion cost grows with the entire search corpus (cumulative O(N²) work). Remove the delete from the insert-only path and index only rows that actually transition from queued to invoked; reserve removal/replacement for real mutations.
Suggested fix:
export function indexMessageContent(db: Database, message: IndexableMessage): void {
ensureMessageContentSearchTable(db)
if (message.invokedAt === null) return
const searchable = extractSearchableMessageText(message.content)
if (!searchable) return
insertMessageContentSearchIndex(db, {
id: message.id,
sessionId: message.sessionId,
text: searchable.text,
role: searchable.role,
seq: message.seq,
createdAt: message.createdAt
})
}For markMessagesInvoked, use UPDATE ... WHERE invoked_at IS NULL RETURNING ... and index only the returned rows, preventing duplicate inserts without this scan.
| ? record.content.text | ||
| : null | ||
| const text = normalizeSearchablePlainText( | ||
| stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '') |
There was a problem hiding this comment.
[Major] Exclude assistant records that are not chat-visible. This extraction accepts output payloads with data.isMeta or data.isCompactSummary, while web/src/chat/normalizeAgent.ts:632-644 deliberately drops both. Their text can therefore be exposed by search, and selecting the result can fail because loadMessageContext() later filters the target out during normalization. Apply the same visibility rule before extracting assistant prose.
Suggested fix:
if (isObject(record.content) && record.content.type === 'output') {
const data = isObject(record.content.data) ? record.content.data : null
if (data && (Boolean(data.isMeta) || Boolean(data.isCompactSummary))) {
return null
}
}
const text = normalizeSearchablePlainText(
stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '')
)Add extraction/search tests for both flags.
| oldestPositionSeq: response.page.nextBeforeSeq, | ||
| newestPositionAt: response.page.snapshotHeadAt, | ||
| newestPositionSeq: response.page.snapshotHeadSeq, | ||
| requiresLatestReset: true, |
There was a problem hiding this comment.
[Major] Persist this reset marker with the historical window. The context response stores the real session head in newestPosition* and sets requiresLatestReset, but PersistedMessageWindowState/persistState() omit that flag. After a reload, hydration sees valid head/epoch values and reconstructs requiresLatestReset: false; runTailSync() then requests only rows after the real head, receives none, and leaves the bounded old context displayed as though it were the current tail.
Suggested fix:
type PersistedMessageWindowState = {
// existing fields...
requiresLatestReset: boolean
}
const persisted: PersistedMessageWindowState = {
// existing fields...
requiresLatestReset: state.requiresLatestReset
}
requiresLatestReset:
parsed.requiresLatestReset === true
|| (parsed.messages.length > 0 && (newest === null || epoch === null))Add a regression test that loads a search context, flushes storage, hydrates a fresh store, and verifies the next sync fetches/replaces with the latest window.
There was a problem hiding this comment.
Findings
- [Major] Global FTS results rejoin the virtual table through the
UNINDEXEDmessage_idcolumn, so snippet retrieval can repeatedly scan the search corpus. - [Minor] Content-search results bypass the existing active-only/sidebar preparation path, allowing inactive sessions to reappear when the active-only preference is enabled.
- [Minor] The new lookup table is required by every indexed write but is absent from the schema-required table list.
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commits address the three findings from the prior HAPI Bot pass. The full latest diff still has one major performance issue and two minor consistency issues, detailed inline.
Testing
- Not run (automation); PR code was not executed during review.
git diff --checkpassed.- Add coverage for the global FTS query plan, content search with active-only enabled, and current-schema validation when the lookup table is absent.
HAPI Bot
| snippet(${MESSAGE_CONTENT_SEARCH_TABLE}, 0, '', '', '…', 24) AS snippet | ||
| FROM ranked_matches AS ranked | ||
| INNER JOIN ${MESSAGE_CONTENT_SEARCH_TABLE} AS f | ||
| ON f.message_id = ranked.message_id |
There was a problem hiding this comment.
[Major] Join snippets by FTS rowid, not the unindexed message id. message_id is declared UNINDEXED at line 94, but this second join probes the virtual table through that column after ranked_matches has already identified the matching rows. FTS5 cannot use its inverted index for this predicate, so returning up to 50 deduplicated sessions can devolve into repeated scans of the full virtual table and make the opt-in search slow on large histories.
Suggested fix:
WITH ranked_matches AS (
SELECT
f.rowid AS search_rowid,
f.message_id,
-- existing columns...
FROM message_content_search AS f
-- existing MATCH/ranking...
)
SELECT ranked.message_id, /* ... */,
snippet(message_content_search, 0, '', '', '…', 24) AS snippet
FROM ranked_matches AS ranked
INNER JOIN message_content_search AS f
ON f.rowid = ranked.search_rowidAdd a large-corpus/query-plan regression that verifies the outer lookup uses the FTS rowid.
| [allSessions, isFiltering, normalizedQuery, timeRange?.start, timeRange?.end, machineLabelsById] // eslint-disable-line react-hooks/exhaustive-deps | ||
| () => { | ||
| if (contentSearchActive) { | ||
| const results = contentSearchResponse?.results.map((result) => result.session) ?? [] |
There was a problem hiding this comment.
[Minor] Apply the existing sidebar scope before rendering content results. The metadata path starts from allSessions, which already applies prepareSidebarSessions() and the active-only preference. This branch instead renders the API results directly, so switching to Content can make inactive sessions reappear even when “active sessions only” is enabled (and also skips the normal sidebar dedup/stub filtering).
Suggested fix:
const results = contentSearchResponse?.results.map((result) => result.session) ?? []
const prepared = prepareSidebarSessions(results, selectedSessionId)
const scoped = showActiveSessionsOnly
? filterActiveSessionsOnly(prepared, selectedSessionId)
: prepared
return scoped.filter((session) => sessionMatchesTimeRange(session, timeRange))Include selectedSessionId and showActiveSessionsOnly in this memo's dependencies, and add a content-search test with active-only enabled.
| 'sessions', | ||
| 'machines', | ||
| 'messages', | ||
| 'message_content_search', |
There was a problem hiding this comment.
[Minor] Treat the auxiliary lookup table as required schema. A database already marked schema v24 does not call createMessageContentSearchTable(); startup only runs assertRequiredTablesPresent(). Because message_content_search_lookup is omitted here, a partial/corrupt v24 database can pass startup and then fail every indexed message write with “no such table”.
Suggested fix:
const REQUIRED_TABLES = [
'sessions',
'machines',
'messages',
'message_content_search',
'message_content_search_lookup',
// ...
] as constAdd a current-version reopen test that drops only the lookup table and expects the schema mismatch to be detected at startup.
There was a problem hiding this comment.
Findings
- [Major] Current-schema databases rebuild the entire message-id lookup on first search/write after every hub restart.
- [Major] The bulk session-cleanup script leaves deleted message text in the new FTS tables.
- [Major] The new-message/send jump exits the visual search lock without reconciling the bounded history window to the real tail.
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commits address the three findings from the previous HAPI Bot review. The full latest diff still has three major correctness/performance/data-retention issues, detailed inline.
Testing
- Not run (automation); PR code was not executed during review.
git diff --checkpassed.- Add regression coverage for a cold reopen of a populated v25 database, bulk cleanup with indexed messages, and leaving a search context through the new-message/send tail jump.
HAPI Bot
| // FTS5 UNINDEXED columns are intentionally not searchable, but SQLite | ||
| // still has to scan the virtual table when deleting by one of them. Keep | ||
| // an ordinary indexed message-id lookup so the write path stays bounded. | ||
| backfillMessageContentSearchLookup(db) |
There was a problem hiding this comment.
[MAJOR] Avoid replaying the full lookup backfill on every cold start. initializedDatabases is process-local, while the current-schema path only calls assertRequiredTablesPresent() (hub/src/store/index.ts:352). Therefore the first content search or indexed message write after each hub restart enters ensureMessageContentSearchTable(), reaches this line, scans every FTS row, and performs an INSERT OR IGNORE for each already-populated lookup row. On a large history that makes the first search/message O(total indexed messages) and holds the synchronous SQLite path while it runs.
Suggested fix:
export function markMessageContentSearchReady(db: Database): void {
initializedDatabases.add(db)
}
// Store.initSchema(), after assertRequiredTablesPresent()
markMessageContentSearchReady(this.db)Keep backfillMessageContentSearchLookup() confined to the v24→v25 migration, and add a populated-v25 reopen regression that verifies the first search/write does not replay the backfill.
| 'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?' | ||
| ).get(id, namespace) | ||
| if (!existing) return false | ||
| removeMessageContentSearchForSession(db, id) |
There was a problem hiding this comment.
[MAJOR] Purge the derived index from the bulk cleanup path too. This helper protects application deletes, but hub/scripts/cleanup-sessions.ts:310 still deletes sessions directly. Foreign-key cascading removes messages; it cannot remove rows from these standalone FTS/lookup tables. Running the documented cleanup script will therefore retain the deleted user/assistant text on disk and make future searches scan orphaned corpus rows, even though the API join hides them.
Suggested fix:
const messageIds = `SELECT id FROM messages WHERE session_id IN (${placeholders})`
const remove = db.transaction(() => {
db.run(`DELETE FROM message_content_search
WHERE rowid IN (SELECT search_rowid FROM message_content_search_lookup
WHERE message_id IN (${messageIds}))`, ids)
db.run(`DELETE FROM message_content_search_lookup
WHERE message_id IN (${messageIds})`, ids)
db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids)
})
remove()Add a cleanup-script regression that indexes a distinctive phrase, deletes its session, and asserts both derived tables no longer contain its message row.
|
|
||
| // Scroll to bottom handler for the indicator button | ||
| const scrollToBottom = useCallback(() => { | ||
| releaseSearchTargetHistoryLock() |
There was a problem hiding this comment.
[MAJOR] Reconcile the real tail when the search-history jump is exited. A targeted load replaces the server window with only the bounded context and sets requiresLatestReset: true (web/src/lib/message-window-store.ts:1033-1042). The retained target keeps skipInitialTailSync enabled (web/src/router.tsx:388-390). This handler—used both by the “new messages” button and by a successful send—only releases the visual lock and switches the existing 41-row context to tail; it never clears the retained target or starts syncTailMessages(). The UI can therefore show the ancient context followed by new replies while silently omitting every message between that context and the actual tail.
Suggested fix:
const scrollToBottom = useCallback(() => {
if (searchTargetHistoryLockRef.current) {
dismissSearchTarget() // clears the retained route target; useMessages performs the latest reset
} else {
releaseSearchTargetHistoryLock()
}
// existing scroll/tail-mode logic...
}, [dismissSearchTarget, releaseSearchTargetHistoryLock])Add an integration test that applies a search context, triggers forceScrollToken or the new-message indicator, and verifies a latest-page request replaces the bounded history window before tail rendering continues.
There was a problem hiding this comment.
Findings
- [Major] Tail reconciliation still retains the search target, so the next render re-locks the viewport and can leave a sent/new reply off-screen. Evidence:
web/src/components/AssistantChat/HappyThread.tsx:1317.
Suggested fix:if (activeSearchTargetId) dismissSearchTarget() else releaseSearchTargetHistoryLock() props.onJumpToTail?.()
- [Major] One- and two-character content queries use an unbounded
%LIKE%scan over the FTS corpus on the synchronous hub SQLite path. Evidence:hub/src/store/messageContentSearch.ts:335.
Suggested fix:const MIN_INDEXED_QUERY_LENGTH = 3 if ([...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH) return [] // Use MATCH for indexed searches.
Questions
- None.
Summary
Review mode: follow-up after new commits
The startup lookup backfill and bulk-cleanup retention findings are resolved. The tail-exit finding remains partially unresolved, and the full latest diff also contains an unbounded short-query scan.
Testing
- Not run (automation):
bunis unavailable in this runner environment. git diff --checkpassed.- SQLite diagnostic: the trigram
%LIKE%path for one/two-character patterns uses virtual-table scan planINDEX 0:L0; a simple 100,000-row synthetic no-match scan took about 22 ms before this PR's namespace join, window function, and ranking work.
HAPI Bot
| // bottom after a clamped/programmatic scroll. Always use the | ||
| // explicit tail transition so the bounded window is reconciled | ||
| // with the server's latest messages before the next turn renders. | ||
| props.onJumpToTail() |
There was a problem hiding this comment.
[MAJOR] Clear the retained search target before starting the tail transition. The follow-up now invokes onJumpToTail(), but releaseSearchTargetHistoryLock() only clears the local marker. SessionPage still supplies the retained initialTargetMessageId (web/src/router.tsx:352-362), so the search-target layout effect runs again when isSyncingTail/messagesVersion changes, sets searchTargetHistoryLockRef.current = true (web/src/components/AssistantChat/HappyThread.tsx:1429), and returns because the jump phase is already complete. After the latest page replaces the bounded context, the auto-scroll layout effect is therefore blocked; a successful send or “new messages” jump can leave the new reply off-screen while the search navigation remains active.
Suggested fix:
if (activeSearchTargetId) {
dismissSearchTarget() // clears the retained route target as well as the marker
} else {
releaseSearchTargetHistoryLock()
}
props.onJumpToTail?.()Extend the new forced-scroll regression to start with a completed retained search target, rerender through the latest-page replacement, and assert the target/navigation is dismissed and the viewport follows the replacement tail.
| if (!normalizedQuery) return [] | ||
|
|
||
| const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50 | ||
| const useLike = [...normalizedQuery].length < 3 |
There was a problem hiding this comment.
[MAJOR] Keep short queries off the unbounded synchronous scan path. For every query shorter than three Unicode code points this switches from trigram MATCH to searchable_text LIKE '%…%'. FTS5 cannot use the trigram index without a three-character literal, so the hub scans the entire content corpus; the LIMIT is applied only after the ROW_NUMBER() partition/ranking. The web activates content search for any non-empty input (web/src/components/SessionList.tsx:1284) after 180 ms, making a one- or two-character pause capable of blocking the hub's synchronous SQLite/event-loop path on a large history.
Suggested fix:
const MIN_INDEXED_QUERY_LENGTH = 3
const queryLength = [...normalizedQuery].length
if (queryLength < MIN_INDEXED_QUERY_LENGTH) return []
const rows = db.prepare(`
-- existing MATCH query only
`).all(namespace, escapeFtsPhrase(normalizedQuery), safeLimit)Apply the same guard to the session-scoped endpoint and surface a localized minimum-length hint in the web UI. If two-character CJK substring search is required, back it with a separately indexed bigram structure instead of %LIKE%.
There was a problem hiding this comment.
Findings
-
[Major] Unigram searches still rank a corpus-scale candidate set — the short-query index removes the virtual-table scan, but a common one-character gram such as
estill selects nearly every containing message, computesROW_NUMBER()across all of them, and only then appliesLIMIT. The web sends any non-empty content query after 180 ms, so this remains an event-loop blocking path on large histories. Evidence:hub/src/store/messageContentSearch.ts:409.
Suggested fix:const queryLength = [...normalizedQuery].length if (queryLength < 2) return [] const useShortIndex = queryLength === 2
Also stop emitting unigram rows and gate the web request at the same two-character minimum.
-
[Minor] Cross-node Markdown matches receive an invisible fallback highlight —
findConversationMessageTextRangeintentionally returns ranges spanning rendered nodes, butRange.surroundContents()throws when the range partially contains an element such as<strong>. The catch inserts only a zero-width<mark>, so the navigation scrolls correctly while no matched text is visibly highlighted. Evidence:web/src/components/AssistantChat/HappyThread.tsx:268.
Suggested fix:} catch { const fallback = range.commonAncestorContainer instanceof HTMLElement ? range.commonAncestorContainer : range.commonAncestorContainer.parentElement fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS) return fallback }
Prefer wrapping each intersecting text-node segment for exact highlighting; at minimum, make the fallback visible and add a regression using the existing
KV <strong>Cache</strong>fixture.
Questions
- None.
Summary
Review mode: follow-up after new commits
Two issues remain in the latest full diff: one-character content searches can still perform work proportional to most of the message corpus, and formatted matches spanning Markdown nodes are not visibly highlighted. The prior tail-transition issue is not present at this head.
Testing
- Not run locally (automation review; Bun is unavailable in this runner).
- GitHub checks:
testpassed;integrationpassed. - Add regressions for a one-character query over many matching rows and a visible cross-node Markdown highlight.
HAPI Bot
| if (!normalizedQuery) return [] | ||
|
|
||
| const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50 | ||
| const useShortIndex = [...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH |
There was a problem hiding this comment.
[MAJOR] Reject unigram queries before ranking the short-index candidates. The new table avoids the FTS virtual-table scan, but a common one-character gram such as e still returns nearly every message containing that character. The ROW_NUMBER() CTE processes that full candidate set before the outer LIMIT, and the web issues any non-empty content query after 180 ms, so large histories can still block the synchronous SQLite/event-loop path.
Suggested fix:
const queryLength = [...normalizedQuery].length
if (queryLength < 2) return []
const useShortIndex = queryLength === 2Apply the same guard to searchMessageContentInSession, stop generating unigram rows in getShortSearchGrams, and gate the web request at two characters with a localized hint. Add a regression for a common one-character query across many messages.
| try { | ||
| range.surroundContents(marker) | ||
| } catch { | ||
| const collapsed = range.cloneRange() |
There was a problem hiding this comment.
[MINOR] Make the cross-node fallback visibly highlight the match. findConversationMessageTextRange deliberately supports phrases spanning rendered Markdown nodes. For a range such as KV <strong>Cache</strong>, surroundContents() throws because the range partially contains <strong>; this catch then inserts only a zero-width <mark>. The jump works, but the matched phrase has no visible persistent highlight.
Suggested fix:
} catch {
const fallback = range.commonAncestorContainer instanceof HTMLElement
? range.commonAncestorContainer
: range.commonAncestorContainer.parentElement
fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
return fallback
}Prefer wrapping each intersecting text-node segment for exact highlighting. At minimum, use a visible fallback and extend the existing cross-node range test to assert a non-zero highlighted area.
There was a problem hiding this comment.
Findings
-
[Major] One-character searches still rank a corpus-scale candidate set — the unigram lookup avoids an FTS scan, but a common gram such as
ecan still select nearly every containing message;ROW_NUMBER()ranks that full set before the outerLIMIT, on the synchronous hub SQLite path. Evidencehub/src/store/messageContentSearch.ts:415.
Suggested fix:const queryLength = [...normalizedQuery].length if (queryLength < 2) return [] const useShortIndex = queryLength === 2
Apply the equivalent
{ matches: [], total: 0 }guard to the session-scoped search, stop emitting unigram rows, gate the web request at two characters, and add a common-unigram corpus regression. -
[Minor] Cross-node Markdown matches still receive an invisible fallback highlight — a range spanning elements such as
KV <strong>Cache</strong>makessurroundContents()throw; the catch inserts only a zero-width marker, so navigation scrolls but the matched phrase is not visibly highlighted. Evidenceweb/src/components/AssistantChat/HappyThread.tsx:268.
Suggested fix:} catch { const fallback = range.commonAncestorContainer instanceof HTMLElement ? range.commonAncestorContainer : range.commonAncestorContainer.parentElement fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS) return fallback }
Prefer wrapping each intersecting text-node segment for exact highlighting; add a rendered cross-node regression that asserts a visible highlighted area.
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commit changes short-index backfill mechanics, but both current-head issues remain: one-character content searches can still do corpus-scale ranking work, and formatted cross-node matches are not visibly highlighted.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regressions: common one-character query volume; visible cross-node Markdown highlighting.
HAPI Bot
| if (!normalizedQuery) return [] | ||
|
|
||
| const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50 | ||
| const useShortIndex = [...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH |
There was a problem hiding this comment.
[MAJOR] Reject one-character queries before ranking short-index candidates. A common unigram such as e can match nearly every containing message. The ROW_NUMBER() CTE processes that entire candidate set before the outer LIMIT, and the web sends any non-empty content query after 180 ms, so this can still block the synchronous hub SQLite/event-loop path on large histories.
Suggested fix:
const queryLength = [...normalizedQuery].length
if (queryLength < 2) return []
const useShortIndex = queryLength === 2Apply the equivalent { matches: [], total: 0 } guard to searchMessageContentInSession, stop generating unigram rows in getShortSearchGrams, gate the web request at two characters with a localized hint, and add a regression using a common one-character query across many messages.
| try { | ||
| range.surroundContents(marker) | ||
| } catch { | ||
| const collapsed = range.cloneRange() |
There was a problem hiding this comment.
[MINOR] Make the cross-node fallback visibly highlight the match. findConversationMessageTextRange() deliberately returns ranges that can span Markdown nodes. For a range such as KV <strong>Cache</strong>, surroundContents() throws because the range partially contains an element; this catch then inserts only a zero-width <mark>. The jump works, but no matched text receives a visible persistent highlight.
Suggested fix:
} catch {
const fallback = range.commonAncestorContainer instanceof HTMLElement
? range.commonAncestorContainer
: range.commonAncestorContainer.parentElement
fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
return fallback
}Prefer wrapping each intersecting text-node segment for exact highlighting. At minimum, add a rendered cross-node regression that asserts a non-zero visible highlighted area.
There was a problem hiding this comment.
Findings
-
[Major] Apply sidebar filters before the global result cap - the content request is capped at 50 sessions before active-only/date/machine/unread filters run, so an eligible match beyond the first 50 is silently omitted and the UI can report no results. Evidence
web/src/components/SessionList.tsx:1305.
Suggested fix:const eligibleSessionIds = contentSearchCandidateSessions.map((session) => session.id) void api.searchSessionContent({ query: normalizedQuery, limit: 50, sessionIds: eligibleSessionIds, signal: controller.signal, })
Constrain those session ids in the hub query before ranking and limiting.
-
[Minor] Release the target lock after terminal context-load failure - opening a search result skips and locks the initial tail sync, but both exhausted-retry branches only clear the component-local lock. The route target remains, so a deleted or stale hit can leave a fresh page with an empty thread until the user manually closes search. Evidence
web/src/components/AssistantChat/HappyThread.tsx:1639.
Suggested fix:} else { current.phase = 'failed' dismissSearchTarget() props.onJumpToTail?.() }
Apply the same fallback to the rejection branch.
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous bot findings are no longer present. The current head still has one result-scoping correctness issue and one terminal search-jump recovery issue.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regressions: more than 50 content matches with a later match passing sidebar filters; terminal context failure returning to the latest transcript.
HAPI Bot
| const controller = new AbortController() | ||
| const timer = window.setTimeout(() => { | ||
| setContentSearchLoading(true) | ||
| void api.searchSessionContent(normalizedQuery, 50, controller.signal) |
There was a problem hiding this comment.
[MAJOR] Apply sidebar filters before the global result cap. This request hard-caps content matches at 50 sessions, but active-only/date filtering runs later in visibleSessions, and machine/unread filtering runs after that. If the newest 50 matching sessions are outside the selected scope, the UI reports no results even though an older eligible session matches.
Suggested fix:
const eligibleSessionIds = contentSearchCandidateSessions.map((session) => session.id)
void api.searchSessionContent({
query: normalizedQuery,
limit: 50,
sessionIds: eligibleSessionIds,
signal: controller.signal,
})Constrain sessions.id in the hub query before ROW_NUMBER() and LIMIT. Add a regression with more than 50 matches where only a later session passes the active machine/date/unread filters.
| current.phase = 'idle' | ||
| scheduleSearchTargetRetry(SEARCH_TARGET_QUERY_RETRY_DELAY_MS) | ||
| } else { | ||
| searchTargetHistoryLockRef.current = false |
There was a problem hiding this comment.
[MINOR] Release the target lock when context loading is exhausted. skipInitialTailSync locks this session's message window before any latest-page request. Both terminal failure branches only clear the component-local history lock; the route target remains, so useMessages keeps the global target lock and never syncs the tail. A deleted or stale search hit on a fresh page can therefore leave an empty thread until the user manually closes search.
Suggested fix:
} else {
current.phase = 'failed'
dismissSearchTarget()
props.onJumpToTail?.()
}Use the same fallback in the rejection branch, and add a regression where getMessageContext() returns null.
There was a problem hiding this comment.
Findings
- [Major] Move the session scope out of the GET query string — the follow-up now appends every eligible session ID to the URL. Session IDs are UUID-sized, so roughly 350 sessions already produce a request target around 16 KB; no larger request-target limit is configured in the repo/docs. Users with a large history will get a transport-level failure before the scoped SQL runs. Evidence
web/src/api/client.ts:266.
Suggested fix:Parse and validate that body in the hub route, then keep applyingreturn await this.request<SessionContentSearchResponse>( '/api/sessions/content-search', { method: 'POST', signal, body: JSON.stringify({ query: query.trim(), limit, sessionIds: [...new Set(sessionIds ?? [])] }) } )
sessionIdsbefore ranking and limiting.
Questions
- None.
Summary
Review mode: follow-up after new commits
The two prior bot findings are resolved. One new scalability regression remains in the session-scoping transport.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regression: several hundred eligible session IDs should not be serialized into the request URL.
HAPI Bot
| const params = new URLSearchParams({ query: query.trim(), limit: String(limit) }) | ||
| for (const sessionId of new Set(sessionIds ?? [])) { | ||
| const trimmed = sessionId.trim() | ||
| if (trimmed) params.append('sessionId', trimmed) |
There was a problem hiding this comment.
[MAJOR] Move the session scope out of the GET query string. This loop appends every eligible session ID to the URL. With UUID-sized IDs, roughly 350 sessions already create a request target around 16 KB, and no larger request-target limit is configured in the repo/docs. Large HAPI histories will therefore fail at the browser/proxy/server transport layer before the scoped SQL can run.
Suggested fix:
return await this.request<SessionContentSearchResponse>(
'/api/sessions/content-search',
{
method: 'POST',
signal,
body: JSON.stringify({
query: query.trim(),
limit,
sessionIds: [...new Set(sessionIds ?? [])]
})
}
)Parse and validate the JSON body in the hub route, then pass sessionIds to the existing pre-limit query scope. Add a regression with several hundred IDs and assert the URL stays bounded.
There was a problem hiding this comment.
Findings
- [Minor] Keep canonical message writes and derived search-index writes atomic —
copyMessageToSessioncommits themessagesinsert before the new index writes, and the invoked-message helpers use the same split pattern. If an FTS/lookup/short-index statement fails after the canonical write, retries see an existing or already-invoked message and do not repair the missing index row, so that content remains permanently absent from search. Evidencehub/src/store/messages.ts:241.
Suggested fix:Apply the same transaction boundary toreturn db.transaction(() => { db.prepare(/* INSERT or UPDATE messages */).run(/* ... */) indexMessageContent(db, indexableMessage) return result })()
markMessagesInvokedandmarkUninvokedImmediateMessages.
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous URL-length finding is no longer present. One derived-index consistency issue remains.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regression: inject an index-write failure and verify the canonical message transition rolls back, then succeeds and becomes searchable on retry.
HAPI Bot
| invoked_at: invokedAt ?? null, | ||
| scheduled_at: message.scheduledAt ?? null | ||
| }) | ||
| indexMessageContent(db, { |
There was a problem hiding this comment.
[MINOR] Keep canonical and derived search-index writes atomic.
At this line the messages insert has already autocommitted, while indexMessageContent performs several subsequent writes to the lookup, FTS, and short-query tables. If one of those statements fails, the request throws but the copied message remains; a retry sees that canonical message and can skip copying it, leaving it permanently absent from search. The same split transaction occurs in markMessagesInvoked and markUninvokedImmediateMessages, where a retry sees invoked_at IS NOT NULL and skips indexing.
Suggested fix:
return db.transaction(() => {
db.prepare(/* INSERT or UPDATE messages */).run(/* ... */)
indexMessageContent(db, indexableMessage)
return result
})()Use this transaction boundary for each canonical-message transition plus its derived-index maintenance, and add a fault-injection regression that verifies rollback and successful retry.
There was a problem hiding this comment.
Findings
-
[Major] Bound scoped content-search IDs before constructing SQL placeholders — the POST endpoint accepts an unbounded
sessionIdsarray, andsearchMessageContentexpands every entry into anIN (?, ...)parameter. A sufficiently large authenticated request therefore exceeds SQLite's variable limit and returns 500; before that point it can force large synchronous JSON parsing and statement preparation on the Hub event loop. Evidencehub/src/web/routes/sessions.ts:225,hub/src/store/messageContentSearch.ts:425.
Suggested fix:const scopedSessionIds = sessionIds?.filter((id) => sessionsById.has(id)) if (scopedSessionIds && scopedSessionIds.length > MAX_CONTENT_SEARCH_SESSION_IDS) { return c.json({ error: 'Too many sessionIds' }, 400) } // Or bind one JSON array and scope through json_each(?) instead of one variable per id.
-
[Minor] Make session deletion and derived-index cleanup atomic —
removeMessageContentSearchForSessioncommits three index deletes before the canonicalDELETE FROM sessions. If that delete fails, the session and messages remain but their content is permanently absent from search; retrying deletion does not repair the index. Evidencehub/src/store/sessions.ts:701.
Suggested fix:return db.transaction(() => { const existing = db.prepare( 'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?' ).get(id, namespace) if (!existing) return false removeMessageContentSearchForSession(db, id) return db.prepare( 'DELETE FROM sessions WHERE id = ? AND namespace = ?' ).run(id, namespace).changes > 0 })()
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous message-transition transaction finding is no longer present. Two issues remain: scoped-search input is unbounded before dynamic SQL construction, and session deletion can desynchronize canonical data from the derived search index on failure.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regressions: oversized/scoped
sessionIdshandling without SQLite variable overflow; injected session-delete failure preserving searchable content.
HAPI Bot
| const sessionIds = body.sessionIds === undefined | ||
| ? undefined | ||
| : [...new Set( | ||
| (body.sessionIds as string[]) |
There was a problem hiding this comment.
[MAJOR] Bound scoped content-search IDs before dynamic SQL construction.
This validation accepts any-length sessionIds array, while messageContentSearch.ts:425 creates one SQLite placeholder per distinct ID. A sufficiently large authenticated request exceeds SQLite's bind-variable limit and returns 500; smaller oversized requests still amplify synchronous JSON parsing and SQL preparation on the Hub event loop. Unknown IDs are currently forwarded too, so callers can trigger this without owning a large session corpus.
Suggested fix:
const scopedSessionIds = sessionIds?.filter((id) => sessionsById.has(id))
if (scopedSessionIds && scopedSessionIds.length > MAX_CONTENT_SEARCH_SESSION_IDS) {
return c.json({ error: 'Too many sessionIds' }, 400)
}Alternatively, bind one JSON array and use json_each(?) in the SQL so the scope consumes one variable, plus add an explicit request-body bound. Add a regression that sends a scope beyond the SQLite variable threshold and expects a controlled 4xx or successful bounded query.
| 'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?' | ||
| ).get(id, namespace) | ||
| if (!existing) return false | ||
| removeMessageContentSearchForSession(db, id) |
There was a problem hiding this comment.
[MINOR] Keep session deletion and derived-index cleanup atomic.
The three search-index deletes commit before the canonical DELETE FROM sessions. If the session delete fails (for example a trigger, I/O error, or later constraint), the session and messages remain but their content is permanently absent from search; a retry does not rebuild those rows.
Suggested fix:
return db.transaction(() => {
const existing = db.prepare(
'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?'
).get(id, namespace)
if (!existing) return false
removeMessageContentSearchForSession(db, id)
return db.prepare(
'DELETE FROM sessions WHERE id = ? AND namespace = ?'
).run(id, namespace).changes > 0
})()Add a fault-injection test that aborts the session delete and verifies the message remains searchable after rollback.
There was a problem hiding this comment.
Findings
-
[Major] Preserve every raw message ID when stream updates are coalesced — the new DOM mapping derives one source ID from the surviving block ID, but the existing reducer mutates the first block for later records with the same
streamIdwithout retaining those latermsg.idvalues. The search index returns those later message IDs (and global search prefers the newest matchingseq), so the loaded text can render while the requested target ID never exists in the DOM; the jump then retries and dismisses, and match navigation can contain unlocatable entries. Evidenceweb/src/lib/assistant-runtime.ts:376, related contextweb/src/chat/reducerTimeline.ts:772,hub/src/store/messageContentSearch.ts:445.
Suggested fix:if (existing) { existing.sourceMessageIds ??= [sourceMessageIdFromBlockId(existing.id)] if (!existing.sourceMessageIds.includes(msg.id)) { existing.sourceMessageIds.push(msg.id) } existing.text = c.text // existing metadata updates... } function getSourceMessageIdsForBlock(block: VisibleChatBlock): string[] { return block.sourceMessageIds?.length ? block.sourceMessageIds : [sourceMessageIdFromBlockId(block.id)] }
-
[Major] Keep scoped content search working above 500 sessions — the client submits every sidebar candidate ID, while the route rejects scopes larger than 500. There is no repository limit keeping accumulated sessions below that count, so a user with 501 eligible sessions gets
400 Too many sessionIdsfor every content query instead of filtered results. Evidenceweb/src/components/SessionList.tsx:1434,hub/src/web/routes/sessions.ts:148.
Suggested fix:const scopeArgs = scopedSessionIds === undefined ? [] : [JSON.stringify(scopedSessionIds)] const sessionScope = scopedSessionIds === undefined ? '' : ' AND f.session_id IN (SELECT value FROM json_each(?))' // Bind the scope as one SQLite variable, and apply a request-byte / ID-length // limit instead of rejecting legitimate candidate counts. statement.all(namespace, query, ...scopeArgs, safeLimit)
-
[Minor] Make bulk cleanup index maintenance atomic — the maintenance script commits the three derived-index deletes before its canonical session delete. If the latter statement fails, the sessions and messages remain but their content is no longer searchable. Evidence
hub/scripts/cleanup-sessions.ts:315.
Suggested fix:return db.transaction(() => { removeMessageContentSearchForSessions(db, ids) db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids) return ids.length })()
Questions
- None.
Summary
Review mode: follow-up after new commits
The prior scoped-SQL overflow and normal session-deletion transaction findings were addressed, but the current head still has two major correctness regressions for streamed/large-session content search and one bulk-cleanup consistency gap.
Testing
- Not run (automation review; PR content was not executed per security policy).
- GitHub checks:
testpassed;integrationpassed. - Missing regressions: a matching later record in a coalesced
streamId; a legitimate 501-session sidebar scope; injected bulk session-delete failure preserving searchable content.
HAPI Bot
| || block.kind === 'codex-review' | ||
| || block.kind === 'generated-image' | ||
| ) { | ||
| const separator = block.id.lastIndexOf(':') |
There was a problem hiding this comment.
[MAJOR] Preserve every raw message ID across stream coalescing.
This derives only one source ID from the surviving block ID. However, reducerTimeline.ts:772-779 updates the first block when later stored messages share its streamId and discards each later msg.id. Content search indexes those stored messages separately and prefers the newest matching seq, so a result commonly points at a later streaming update whose ID is absent from the rendered card. The context loads, but target lookup keeps failing and eventually dismisses; previous/next navigation can also include these unlocatable IDs.
Suggested fix:
if (existing) {
existing.sourceMessageIds ??= [sourceMessageIdFromBlockId(existing.id)]
if (!existing.sourceMessageIds.includes(msg.id)) {
existing.sourceMessageIds.push(msg.id)
}
existing.text = c.text
// existing metadata updates...
}
function getSourceMessageIdsForBlock(block: VisibleChatBlock): string[] {
return block.sourceMessageIds?.length
? block.sourceMessageIds
: [sourceMessageIdFromBlockId(block.id)]
}Add a regression with two stored text records sharing one streamId, then jump using the second record's message ID.
| normalizedQuery, | ||
| limit, | ||
| controller.signal, | ||
| contentSearchSessionIds |
There was a problem hiding this comment.
[MAJOR] Do not make content search fail for sidebars above 500 sessions.
This call always passes every eligible sidebar session ID. The route now returns 400 Too many sessionIds when that array has 501 entries, and there is no repository cap preventing users from accumulating that many sessions. Consequently every content query fails for those users; truncating the array would silently omit valid matches and violate the filter-before-limit contract.
Suggested fix:
const scopeArgs = scopedSessionIds === undefined
? []
: [JSON.stringify(scopedSessionIds)]
const sessionScope = scopedSessionIds === undefined
? ''
: ' AND f.session_id IN (SELECT value FROM json_each(?))'
// One SQLite binding for the complete scope; separately cap request bytes and ID length.
statement.all(namespace, query, ...scopeArgs, safeLimit)Add a UI/API regression with 501 authorized candidate sessions and verify the request returns results rather than 400.
| // Store wrapper. Remove derived message-content rows first, while the | ||
| // messages still exist for the session-id lookup, so deleted text cannot | ||
| // remain searchable in the FTS index. | ||
| removeMessageContentSearchForSessions(db, ids) |
There was a problem hiding this comment.
[MINOR] Keep bulk deletion and derived-index cleanup in one transaction.
The new index cleanup runs as separate autocommit statements before the canonical DELETE FROM sessions. If the session delete fails, the canonical rows remain while their searchable rows are permanently gone. The regular deleteSession path now avoids this, but this maintenance path still has the same consistency gap.
Suggested fix:
return db.transaction(() => {
removeMessageContentSearchForSessions(db, ids)
db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids)
return ids.length
})()Add fault injection around the bulk session delete and assert the retained messages remain searchable after rollback.
There was a problem hiding this comment.
Findings
- [Major] Collapse streamed snapshots before counting message matches — the index stores and counts each raw row, while the web reducer replaces rows sharing a stream ID with one visible message. A repeated phrase therefore produces duplicate navigation targets for the same card; an older snapshot can also match text absent from the final rendered snapshot. Evidence:
hub/src/store/messageContentSearch.ts:540, with render behavior atweb/src/chat/reducerTimeline.ts:771.
Suggested fix:Extend extraction/index storage with the same stream identity used by the reducer, replacing the prior snapshot for that identity.const renderKey = searchable.renderKey ? `${message.sessionId}:${searchable.renderKey}` : message.id upsertMessageContentSearchIndex(db, { ...message, id: renderKey, text: searchable.text })
Summary
Review mode: follow-up after new commits
The three findings from the previous HAPI Bot review are addressed. One correctness issue remains in match counting/navigation for coalesced streaming snapshots.
Testing
Not run (automation review; PR content was not executed per security policy). Existing checks are passing, but coverage does not combine backend search with two raw rows sharing one rendered stream ID.
HAPI Bot
| const useShortIndex = queryLength === MIN_INDEXED_QUERY_LENGTH | ||
| const countRow = useShortIndex | ||
| ? db.prepare(` | ||
| SELECT COUNT(*) AS count |
There was a problem hiding this comment.
[MAJOR] Count the rendered message, not each stream snapshot.
COUNT(*) and the result query operate on raw indexed rows, but the chat reducer replaces text rows sharing a streamId with one visible block (web/src/chat/reducerTimeline.ts:771-783) and maps all source IDs to that one card. If a phrase appears in two snapshots, the UI reports two matches and previous/next repeatedly lands on the same card. Worse, a phrase removed by the final replacement can still match an older snapshot although it is no longer visible.
Suggested fix:
const renderKey = searchable.renderKey
? `${message.sessionId}:${searchable.renderKey}`
: message.id
upsertMessageContentSearchIndex(db, {
...message,
id: renderKey,
text: searchable.text
})Have extractSearchableMessageText expose the same stream identity used by the reducer, persist a unique render key, and replace its indexed row when a newer snapshot arrives. Add a regression with two matching rows sharing one stream ID; assert total === 1, one navigable match, and only the final snapshot text remains searchable.
There was a problem hiding this comment.
Findings
- [Major] Normalize Antigravity planner text before indexing —
extractAssistantPlainText()returns rawagy_messagecontent, and the new search extractor indexes it directly atshared/src/messages.ts:223. The chat renderer first removes the duplicated[Message] timestamp=... content=Task ... Output: ...block and converts anInside the task-NNN log...marker into a tool chip atweb/src/chat/normalizeAgent.ts:772andweb/src/chat/normalizeAgent.ts:778. As a result, sidebar search can return and snippet raw background-task/tool payload text that is absent from the rendered assistant message, violating the visible-prose-only indexing boundary.
Suggested fix:// Move/reuse stripAgyEchoedTaskResult in shared code. const outputData = isObject(record.content) && record.content.type === 'output' ? (isObject(record.content.data) ? record.content.data : null) : null const assistantText = directText ?? extractAssistantPlainText(record.content) ?? '' const visibleText = outputData?.type === 'agy_message' ? stripAgyEchoedTaskResult(assistantText) : assistantText if (outputData?.type === 'agy_message' && /^Inside the task-\d+ log\b/.test(visibleText)) { return null } const text = normalizeSearchablePlainText(stripNotifySummaryFooter(visibleText))
Summary
Review mode: follow-up after new commits
The previous streamed-snapshot indexing finding is addressed by the new render-key replacement logic. One Major issue remains in the visible-text extraction boundary for Antigravity messages.
Testing
Not run (automation review; PR content was not executed per security policy). Add a shared extractor/index test proving echoed AGY task output is removed and a task-log-only marker is not searchable.
HAPI Bot
| ? record.content.text | ||
| : null | ||
| const text = normalizeSearchablePlainText( | ||
| stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '') |
There was a problem hiding this comment.
[MAJOR] Normalize AGY planner output before adding it to the search index
For output.data.type === 'agy_message', extractAssistantPlainText() returns raw data.content, so this line indexes the duplicated [Message] ... Task ... Output: ... payload. The renderer removes that block with stripAgyEchoedTaskResult() and renders an Inside the task-NNN log... marker as a tool chip instead of prose (web/src/chat/normalizeAgent.ts:772-790). This can expose raw background-task/tool content in search snippets and create hits that cannot be found in the rendered message.
Move/reuse the renderer normalization in shared code before indexing:
const outputData = isObject(record.content) && record.content.type === 'output'
? (isObject(record.content.data) ? record.content.data : null)
: null
const assistantText = directText ?? extractAssistantPlainText(record.content) ?? ''
const visibleText = outputData?.type === 'agy_message'
? stripAgyEchoedTaskResult(assistantText)
: assistantText
if (outputData?.type === 'agy_message' && /^Inside the task-\d+ log\b/.test(visibleText)) {
return null
}
const text = normalizeSearchablePlainText(stripNotifySummaryFooter(visibleText))There was a problem hiding this comment.
Findings
- [Major] Claude sidechain prose is indexed even though it has no standalone root-timeline message -
extractSearchableMessageTextexcludesisMetaandisCompactSummary, but notdata.isSidechain; the renderer moves those rows into a subagent group under the parent tool call. A search jump loads only 20 rows before the hit, so a long sidechain can omit that parent and leave the returned message ID with no rendered anchor. Evidence:shared/src/messages.ts:212,web/src/chat/normalizeAgent.ts:231,web/src/chat/reducer.ts:123.
Suggested fix:function isHiddenAssistantOutput(content: unknown): boolean { if (!isObject(content) || content.type !== 'output') return false const data = isObject(content.data) ? content.data : null return Boolean(data?.isMeta) || Boolean(data?.isCompactSummary) || Boolean(data?.isSidechain) }
- [Major] The POST scope limit is enforced only after parsing and processing an unbounded body -
c.req.json()materializes the full payload, then every array entry is type-checked, trimmed, and deduplicated; the 256 KiB check runs later and only after IDs outside the namespace are discarded. An authenticated client can therefore send a very large duplicate/unknown-ID array, consume memory and CPU, and still pass the intended guard. Evidence:hub/src/web/routes/sessions.ts:207,hub/src/web/routes/sessions.ts:230,hub/src/web/routes/sessions.ts:145.
Suggested fix:import { bodyLimit } from 'hono/body-limit' import { MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES, serializeContentSearchSessionIds } from '../../store/messageContentSearch' const contentSearchBodyLimit = bodyLimit({ maxSize: MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES, onError: (c) => c.json({ error: 'Request body too large' }, 413) }) app.post('/sessions/content-search', contentSearchBodyLimit, async (c) => { // existing handler })
Summary
Review mode: follow-up after new commits
The prior AGY planner-text finding is addressed. Two Major issues remain: Claude sidechain hits can be returned without a navigable chat anchor, and the new POST endpoint parses an unbounded request before applying its scope-size guard.
Testing
Not run (automation review; PR content was not executed per security policy). Add focused coverage that rejects isSidechain output from the search index and returns 413 for an oversized content-search POST before parsing.
HAPI Bot
| function isHiddenAssistantOutput(content: unknown): boolean { | ||
| if (!isObject(content) || content.type !== 'output') return false | ||
| const data = isObject(content.data) ? content.data : null | ||
| return Boolean(data?.isMeta) || Boolean(data?.isCompactSummary) |
There was a problem hiding this comment.
[MAJOR] Sidechain text can produce a search hit with no rendered anchor
This predicate excludes meta and compact-summary output, but Claude marks subagent-only output with data.isSidechain (web/src/chat/normalizeAgent.ts:231). The reducer removes those messages from the root timeline and emits them only through the parent subagent tool group (web/src/chat/reducer.ts:123). Because search navigation loads only 20 rows before the hit, a long sidechain can exclude the parent tool call, so this indexed message ID cannot be located in the rendered chat.
Suggested fix:
function isHiddenAssistantOutput(content: unknown): boolean {
if (!isObject(content) || content.type !== 'output') return false
const data = isObject(content.data) ? content.data : null
return Boolean(data?.isMeta)
|| Boolean(data?.isCompactSummary)
|| Boolean(data?.isSidechain)
}| return engine | ||
| } | ||
|
|
||
| const body = await c.req.json().catch(() => null) as { |
There was a problem hiding this comment.
[MAJOR] Enforce the scope limit before parsing the JSON body
c.req.json() materializes an unbounded payload before the route validates, trims, and deduplicates every sessionIds entry. The later 256 KiB check also runs after IDs outside the namespace are discarded, so a huge duplicate/unknown-ID array can consume memory and CPU while still passing the guard.
Suggested fix:
import { bodyLimit } from 'hono/body-limit'
import {
MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
serializeContentSearchSessionIds
} from '../../store/messageContentSearch'
const contentSearchBodyLimit = bodyLimit({
maxSize: MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
onError: (c) => c.json({ error: 'Request body too large' }, 413)
})
app.post('/sessions/content-search', contentSearchBodyLimit, async (c) => {
// existing handler
})There was a problem hiding this comment.
Findings
- None.
Summary
Review mode: follow-up after new commits
No new Blocker, Major, Minor, or Nit findings at the current head. The prior HAPI Bot findings on sidechain indexing and pre-parse request-size enforcement are addressed. Residual risk remains in the large SQLite FTS migration/backfill and timing-sensitive historical-scroll behavior; the PR includes focused coverage, but it was not independently executed in this automation review.
Testing
Not run (automation review; PR content was not executed per security policy).
HAPI Bot
There was a problem hiding this comment.
Findings
-
[Major] Bound short-query indexing work per message - each unique adjacent code-point pair causes a synchronous SQLite insert. User prompts are not truncated and the send schema has no text-size bound, so one large/high-entropy prompt or imported row can trigger O(n) database writes on the Hub event loop and during migration. Evidence
hub/src/store/messageContentSearch.ts:279.Suggested fix:
const MAX_SHORT_INDEX_CHARACTERS = 16_384 function getBoundedShortSearchGrams(text: string): string[] { return getShortSearchGrams( Array.from(text).slice(0, MAX_SHORT_INDEX_CHARACTERS).join('') ) } for (const gram of getBoundedShortSearchGrams(message.text)) { insertShortGram.run(gram, lookup.search_rowid) }
Preferably store encoded grams as one FTS row per message rather than one ordinary-table row/write per gram; either approach must place a hard bound on per-message work.
-
[Minor] Normalize Markdown to rendered text before indexing - the index currently only collapses whitespace, so visible phrases split by Markdown syntax are absent from the trigram stream. For example,
KV **Cache**renders asKV Cache, but searching forKV Cachewill not match because the stored text contains the**delimiters. Evidenceshared/src/messages.ts:156.Suggested fix:
import { fromMarkdown } from 'mdast-util-from-markdown' import { toString } from 'mdast-util-to-string' function normalizeSearchablePlainText(value: string): string | null { const text = toString(fromMarkdown(value)).trim().replace(/\s+/g, ' ') return text.length > 0 ? text : null }
Questions
- None.
Summary
Review mode: follow-up after new commits
Two issues found at the current head: one Hub availability/storage risk in the two-character index write path, and one correctness gap between indexed Markdown source and visible rendered text. The previous HAPI Bot review and the compare from f9b670b... were used as context; the full latest PR diff was reviewed.
Testing
Not run (automation review; PR content was not executed). Add a bounded-work test with a large high-entropy user prompt, plus a backend search test proving KV **Cache** matches the visible query KV Cache.
HAPI Bot
| INSERT INTO ${MESSAGE_CONTENT_SEARCH_SHORT_TABLE} (gram, search_rowid) | ||
| VALUES (?, ?) | ||
| `) | ||
| for (const gram of getShortSearchGrams(message.text)) { |
There was a problem hiding this comment.
[MAJOR] Bound short-query indexing work per message
This loop performs one synchronous SQLite insert for every unique adjacent code-point pair. Because user prompts are intentionally not truncated and the send schema accepts an unbounded string, one large/high-entropy prompt (or migration/import row) can trigger O(n) writes on the Hub event loop and inflate the derived table.
Suggested fix:
const MAX_SHORT_INDEX_CHARACTERS = 16_384
const boundedText = Array.from(message.text)
.slice(0, MAX_SHORT_INDEX_CHARACTERS)
.join('')
for (const gram of getShortSearchGrams(boundedText)) {
insertShortGram.run(gram, lookup.search_rowid)
}Preferably encode all grams into one FTS row per message; whichever representation is used, enforce a hard per-message work bound and cover it with a large high-entropy prompt test.
| } | ||
|
|
||
| function normalizeSearchablePlainText(value: string): string | null { | ||
| const text = value.trim().replace(/\s+/g, ' ') |
There was a problem hiding this comment.
[MINOR] Normalize Markdown to rendered text before indexing
Whitespace normalization alone leaves Markdown delimiters in the trigram stream. A message such as KV **Cache** is visibly rendered as KV Cache, but the query KV Cache cannot match the indexed source because ** interrupts the phrase. This also undermines the new cross-Markdown-node highlighting path.
Suggested fix:
import { fromMarkdown } from 'mdast-util-from-markdown'
import { toString } from 'mdast-util-to-string'
function normalizeSearchablePlainText(value: string): string | null {
const text = toString(fromMarkdown(value)).trim().replace(/\s+/g, ' ')
return text.length > 0 ? text : null
}Add a backend search test for KV **Cache** queried as KV Cache.
There was a problem hiding this comment.
Findings
-
[Minor] Index Claude user records that the renderer exposes as user messages —
extractSearchableMessageTextroutes every outerrole: agent/assistantenvelope through assistant extraction, butextractAssistantPlainTextrejectsoutput/user. The web normalizer intentionally renders non-sidechain, all-textoutput/userarrays as ordinary user bubbles, so those visible prompts are omitted from both rebuilt and live content search. Evidenceshared/src/messages.ts:237,web/src/chat/normalizeAgent.ts:371.Suggested fix:
const data = isObject(record.content) && record.content.type === "output" && isObject(record.content.data) ? record.content.data : null const message = isObject(data?.message) ? data.message : null const blocks = Array.isArray(message?.content) ? message.content : null if (data?.type === "user" && !Boolean(data.isSidechain) && blocks?.length && blocks.every((block) => isObject(block) && block.type === "text" && typeof block.text === "string" )) { const text = extractUserPlainText(blocks) return text ? { role: "user", text } : null }
Summary
Review mode: follow-up after new commits
One search-correctness issue remains at the current head. The previous HAPI Bot findings are no longer present; the full latest PR diff was reviewed, using the prior review and compare diff only as follow-up context.
Testing
Not run (automation review; PR content was not executed). Add shared extractor and Hub search regression tests for a non-sidechain output/user record whose content is an all-text array.
HAPI Bot
| return text ? { role: 'user', text } : null | ||
| } | ||
|
|
||
| if (record.role === 'agent' || record.role === 'assistant') { |
There was a problem hiding this comment.
[MINOR] Index Claude user records rendered in the user lane
This branch treats every outer role: agent/assistant record as assistant prose. extractAssistantPlainText() returns null for content.type: output with data.type: user, while web/src/chat/normalizeAgent.ts:371-389 deliberately renders a non-sidechain, all-text array of that shape as a real user message. Consequently those visible prompts are absent from both index backfills and new-message search.
Suggested fix:
const data = isObject(record.content) && record.content.type === "output"
&& isObject(record.content.data) ? record.content.data : null
const message = isObject(data?.message) ? data.message : null
const blocks = Array.isArray(message?.content) ? message.content : null
if (data?.type === "user" && !Boolean(data.isSidechain)
&& blocks?.length && blocks.every((block) =>
isObject(block) && block.type === "text" && typeof block.text === "string"
)) {
const text = extractUserPlainText(blocks)
return text ? { role: "user", text } : null
}Place this after the hidden-output check, before assistant extraction, and cover it with an extractor plus Hub search test.
There was a problem hiding this comment.
Findings
-
[Major] Bound derived search text before FTS insertion - canonical user prompts are unbounded (
shared/src/apiTypes.ts:556) and intentionally bypass agent-content truncation, but this added path inserts the full text into trigram FTS synchronously inside message writes and the startup backfill. A large pasted prompt can therefore stall the Hub and grow the derived DB index without a bound;MAX_SHORT_INDEX_CHARACTERSonly limits the separate two-character table. Evidencehub/src/store/messageContentSearch.ts:280.Suggested fix:
const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384 function boundSearchableText(text: string): string { if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2 return `${text.slice(0, half)} ${text.slice(-half)}` } const searchableText = boundSearchableText(message.text) // Insert searchableText into FTS and derive short grams from the same value.
-
[Minor] Clear the retained message target when the route session changes -
SessionPageis reused for different$sessionIdparams, so this state survives A -> B navigation. Returning to A makesretainedMessageTargetvalid again, re-enablesskipInitialTailSync, and unexpectedly reopens the old historical match. Evidenceweb/src/router.tsx:367.Suggested fix:
useEffect(() => { setConsumedMessageTarget(null) }, [sessionId])
Summary
Review mode: follow-up after new commits
Two issues remain at the current head. The previous HAPI Bot finding about visible Claude user records is resolved by the latest commit; the full latest PR diff was reviewed, with the prior review and compare diff used only as follow-up context.
Testing
Not run (automation review; PR content was not executed). Add a regression test with a multi-megabyte user prompt that asserts bounded FTS/short-index work, and a router test for content result A -> session B -> session A without restoring the consumed target.
HAPI Bot
| ) VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| `).run( | ||
| lookup.search_rowid, | ||
| message.text, |
There was a problem hiding this comment.
[MAJOR] Bound the text stored in the trigram index
message.text can contain an arbitrarily large user prompt: SendMessageRequestSchema.text has no maximum, and the content codec deliberately leaves user messages untruncated. This line therefore tokenizes and stores the entire payload synchronously inside canonical message transactions and the migration backfill. The 16,384-character cap below only protects the two-character side table, not FTS.
Suggested fix:
const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384
function boundSearchableText(text: string): string {
if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text
const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2
return `${text.slice(0, half)} ${text.slice(-half)}`
}
const searchableText = boundSearchableText(message.text)
// Use searchableText for both the FTS row and short grams.| messageId: string | ||
| messageQuery?: string | ||
| } | null>(null) | ||
| const retainedMessageTarget = consumedMessageTarget?.sessionId === sessionId |
There was a problem hiding this comment.
[MINOR] Drop the consumed target when leaving the session
TanStack reuses SessionPage while only $sessionId changes. This retained state is merely ignored on session B, then becomes valid again when navigating back to session A, which reactivates skipInitialTailSync and reopens the old historical search hit.
Suggested fix:
useEffect(() => {
setConsumedMessageTarget(null)
}, [sessionId])There was a problem hiding this comment.
Findings
-
[Major] Bound derived search text before FTS insertion — user prompts are unbounded and this path synchronously tokenizes/stores the full text during message writes and migration backfill. Evidence
hub/src/store/messageContentSearch.ts:280.
Suggested fix:const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384 const searchableText = boundSearchableText(message.text) // Use searchableText for FTS and short grams.
-
[Minor] Clear the retained target when the route session changes —
SessionPagesurvives parameter-only navigation, so returning to session A can reactivate its consumed historical target and skip the normal tail sync. Evidenceweb/src/router.tsx:367.
Suggested fix:useEffect(() => { setConsumedMessageTarget(null) }, [sessionId])
Summary
Review mode: follow-up after new commits
Two unresolved issues remain at the latest head. Commits since the previous HAPI Bot review only changed version/build metadata; the full latest PR diff was reviewed, with the previous review and compare diff used only as follow-up context.
Testing
Not run (automation). Add a regression test with a multi-megabyte user prompt that asserts bounded FTS work, and a router test for content result A → session B → session A without restoring the consumed target.
HAPI Bot
| ) VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| `).run( | ||
| lookup.search_rowid, | ||
| message.text, |
There was a problem hiding this comment.
[MAJOR] Bound the text stored in the trigram index
message.text can contain an arbitrarily large user prompt: the send schema has no maximum and user content is intentionally not truncated. This line therefore tokenizes and stores the entire payload synchronously inside canonical message transactions and the startup backfill. MAX_SHORT_INDEX_CHARACTERS only protects the separate two-character table.
Suggested fix:
const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384
function boundSearchableText(text: string): string {
if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text
const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2
return `${text.slice(0, half)} ${text.slice(-half)}`
}
const searchableText = boundSearchableText(message.text)
// Use searchableText for both the FTS row and short grams.| messageId: string | ||
| messageQuery?: string | ||
| } | null>(null) | ||
| const retainedMessageTarget = consumedMessageTarget?.sessionId === sessionId |
There was a problem hiding this comment.
[MINOR] Drop the consumed target when leaving the session
TanStack reuses SessionPage while only $sessionId changes. This state is ignored on session B but remains stored; navigating back to session A makes it valid again, re-enables skipInitialTailSync, and reopens the old historical search hit.
Suggested fix:
useEffect(() => {
setConsumedMessageTarget(null)
}, [sessionId])There was a problem hiding this comment.
Findings
-
[Major] Avoid rebuilding the FTS and short-query indexes for every cumulative live stream snapshot — Pi emits cumulative snapshots every 250 ms, while the new write path deletes and reinserts the full growing text synchronously for each one. See the inline comment at
hub/src/store/messageContentSearch.ts:321.
Suggested fix:if (isLiveStreamSnapshot(message.content)) return // Index the explicit terminal snapshot once.
-
[Minor] Preserve both ends when bounding indexed text — the current prefix-only slice makes visible text after character 16,384 permanently unsearchable, including the tail intentionally retained by the message codec. See the inline comment at
hub/src/store/messageContentSearch.ts:120.
Suggested fix:return `${safeHead(text, half)} ${safeTail(text, half)}`
Summary
Review mode: follow-up after new commits
The two findings from the prior HAPI Bot pass no longer reproduce at this head. The full latest diff still has one Major streaming-write amplification issue and one Minor long-message search gap.
Testing
Not run (automation; PR code was not executed). Add a regression test that feeds many cumulative snapshots for one render key and asserts only the terminal snapshot rebuilds the derived indexes, plus a long-message test with the search needle in the retained tail.
HAPI Bot
| const renderKey = extractMessageRenderKey(message.content) | ||
| const searchKey = renderKey ? `${message.sessionId}:${renderKey}` : message.id | ||
| removeMessageContentSearchIndex(db, message.id) | ||
| if (searchKey !== message.id) removeMessageContentSearchIndexByKey(db, searchKey) |
There was a problem hiding this comment.
[MAJOR] Avoid rebuilding both indexes for every live stream snapshot
indexMessageContent runs inside every canonical message insert. Pi emits cumulative text snapshots every 250 ms (cli/src/pi/piMessageAccumulator.ts:6,136-166). For one render key, this path deletes the previous FTS/gram rows and parses/reinserts the entire growing text on every snapshot. Coalescing prevents duplicate search results, but it does not prevent write amplification: a long or high-entropy answer can cause thousands of synchronous short-index writes every 250 ms and block other Hub SQLite work.
Suggested fix:
const stream = getMessageStreamState(message.content)
if (stream?.snapshot && !stream.final) {
return
}
// Replace the derived row once for the terminal snapshot.Carry an explicit terminal flag through the wire format, and make the accumulator emit a terminal snapshot even when its text equals the last live snapshot.
| // allocate an Array for its entire contents just to index its prefix. The | ||
| // cap is intentionally expressed in UTF-16 code units to keep this work | ||
| // bounded even for messages containing only astral characters. | ||
| let boundedText = text.slice(0, MAX_INDEXED_MESSAGE_CHARACTERS) |
There was a problem hiding this comment.
[MINOR] Keep the retained tail searchable when applying the size cap
This prefix-only slice makes every phrase after character 16,384 invisible to search. User prompts are intentionally unbounded, and agent strings are stored with a head+tail truncation (hub/src/store/contentCodec.ts:8-20), so text that is still visible at the end of a message cannot be found.
Suggested fix:
const half = Math.floor((MAX_INDEXED_MESSAGE_CHARACTERS - 1) / 2)
let head = text.slice(0, half)
let tail = text.slice(-half)
if (/^[\uDC00-\uDFFF]/.test(tail)) tail = tail.slice(1)
if (/[\uD800-\uDBFF]$/.test(head)) head = head.slice(0, -1)
return `${head} ${tail}`There was a problem hiding this comment.
Findings
- [Major] Pi answers can remain permanently absent from content search after normal completion — the new index guard skips all live snapshots, but
PiMessageAccumulatorstill suppresses the terminal snapshot when its text equals the last live snapshot. See the inline comment athub/src/store/messageContentSearch.ts:344; related contextcli/src/pi/piMessageAccumulator.ts:159.
Suggested fix:const unchanged = segment.text === segment.lastSnapshot if (!segment.text.trim() || (unchanged && (live || kind !== 'text'))) continue
Summary
Review mode: follow-up after new commits
The prior long-message tail gap no longer reproduces. The live-snapshot write-amplification change remains incomplete because the producer does not guarantee the non-live terminal row that the index now relies on. No other findings met the 80% confidence threshold in the full current diff.
Testing
Not run (automation; PR code was not executed). Add a regression where a live snapshot contains the complete final text, message_end arrives without another delta, and the accumulator still emits a non-live terminal snapshot; then assert that row becomes searchable.
HAPI Bot
| // Pi emits cumulative live snapshots every 250 ms. They are deliberately | ||
| // not indexed; the explicit terminal snapshot carries the same render key | ||
| // and is indexed once when the stream finishes. | ||
| if (isLiveStreamSnapshot(message.content)) return |
There was a problem hiding this comment.
[MAJOR] Ensure Pi emits the terminal row that this guard relies on
This now drops every live: true snapshot, but Pi does not always emit a later non-live copy. PiMessageAccumulator.addSnapshots skips unchanged text (cli/src/pi/piMessageAccumulator.ts:159), so when the last throttled live snapshot already contains the complete answer, message_end flushes nothing. The completed answer then has no indexed row and remains unsearchable indefinitely.
Suggested fix:
const unchanged = segment.text === segment.lastSnapshot
if (!segment.text.trim() || (unchanged && (live || kind !== 'text'))) continueThis preserves live deduplication and reasoning behavior while forcing one terminal text snapshot. Add a test where message_end follows an unchanged live snapshot.
There was a problem hiding this comment.
Findings
- [Minor] Preserve sidebar scroll when clearing the content-search target — both new same-route
replacenavigations omit the repository navigation flag that disables router-wide scroll restoration, so consuming or dismissing a hit can reset the session sidebar and lose the result position. Evidenceweb/src/router.tsx:787; related contextweb/src/lib/sessionNavigation.ts:6.
Suggested fix:Apply the same option innavigate({ to: "/sessions/$sessionId", params: { sessionId }, replace: true, ...PRESERVE_SESSION_SIDEBAR_SCROLL, })
handleSearchTargetDismissed.
Summary
Review mode: follow-up after new commits
One Minor navigation regression found. The previous Pi terminal-snapshot indexing issue is resolved by the new non-live terminal emission and regression test. No other findings met the 80% confidence threshold in the full current diff.
Testing
Not run (automation; PR code was not executed). GitHub test and integration checks passed. Add a router/navigation test asserting resetScroll: false for both search-target consumption and dismissal.
HAPI Bot
| navigate({ | ||
| to: '/sessions/$sessionId', | ||
| params: { sessionId }, | ||
| replace: true, |
There was a problem hiding this comment.
[MINOR] Preserve the sidebar position during URL cleanup
The result-selection navigation already spreads PRESERVE_SESSION_SIDEBAR_SCROLL, and the helper documents why internal /sessions navigation must set resetScroll: false. These two new replace navigations omit it, so after the target is located (or dismissed), router scroll restoration can reset the still-mounted sidebar and lose the user position in the content-search results.
Suggested fix:
navigate({
to: "/sessions/$sessionId",
params: { sessionId },
replace: true,
...PRESERVE_SESSION_SIDEBAR_SCROLL,
})Apply the same addition in handleSearchTargetDismissed.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found at the current head. The prior Minor sidebar-scroll regression is fixed in web/src/router.tsx:789 and web/src/router.tsx:799. Residual risk is concentrated in production-scale index migration/storage and browser-specific historical-scroll behavior; no additional testing gap was identified beyond the PR’s existing coverage.
Testing
- Not run (automation review; PR code was not executed).
- GitHub
testandintegrationchecks passed.
HAPI Bot
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found at the current head. Residual risk remains around the schema-v27 FTS rebuild cost on unusually large production databases and browser-specific historical search jumps/scroll anchoring; the PR includes targeted migration, store, UI, and end-to-end coverage for these areas.
Testing
- Not run (automation review; PR code was not executed).
- GitHub checks observed: build, drift-gate, integration, and test passed; pr-review was pending during review.
HAPI Bot
…search # Conflicts: # cli/src/agent/messageConverter.ts # hub/src/store/index.ts # hub/src/store/messages.test.ts # hub/src/store/messages.ts # hub/src/store/migration-v13.test.ts # hub/src/store/migration-v15.test.ts # hub/src/store/migration-v18.test.ts # hub/src/store/migration-v19.test.ts # hub/src/store/migration-v20.test.ts # hub/src/store/migration-v23.test.ts # hub/src/store/migration-v24.test.ts # shared/src/messages.test.ts # shared/src/messages.ts # web/src/components/SessionList.tsx
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found at the current head. Residual risk remains around the synchronous schema-v28 FTS/short-index backfill on unusually large databases and browser-specific historical jump/scroll behavior; the PR includes targeted migration, store, component, and end-to-end coverage.
Testing
- Not run locally (automation review; Bun is unavailable in this runner).
- GitHub checks observed: build, drift-gate, integration, and test passed.
HAPI Bot
…search # Conflicts: # web/src/lib/assistant-runtime.ts
There was a problem hiding this comment.
Findings
-
[Minor] Pi error turns can remain unindexed — when the last live snapshot already contains the final partial response, the error path suppresses the terminal snapshot. The live text remains visible in chat, but the content index deliberately ignores live snapshots, so that visible response cannot be found. Evidence:
cli/src/pi/piMessageAccumulator.ts:92; related index guard:hub/src/store/messageContentSearch.ts:340.Suggested fix:
const messages = this.flush()
Questions
- None.
Summary
Review mode: follow-up after new commits
One minor correctness issue found: Pi responses that fail immediately after a final live snapshot are visible but absent from message-content search. Residual risk remains around synchronous large-database index migration and browser-specific historical scroll behavior.
Testing
- Not run (automation review; PR code was not executed).
- GitHub checks observed: build, drift-gate, integration, and test passed.
- Missing regression coverage: Pi live snapshot → error boundary → terminal searchable content.
HAPI Bot
| const wasActive = this.active; | ||
| const messages = this.flush(); | ||
| const errorMessage = 'message' in event ? extractPiTurnError(event.message) : null; | ||
| const messages = this.flush(errorMessage === null); |
There was a problem hiding this comment.
[MINOR] Error-finalized Pi text is skipped by content search
When errorMessage is non-null, flush(false) suppresses an unchanged terminal text snapshot. That is the common case when the last throttled live snapshot already contains the partial answer. The live row is still rendered, but indexMessageContent explicitly skips live snapshots, so the visible failed response never becomes searchable.
Suggested fix:
const messages = this.flush()Please update the error-boundary test to expect the terminal text followed by the error, and add coverage that this terminal row reaches content search.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found at the latest head. The prior Pi error-finalization indexing issue is addressed by the new terminal snapshot behavior and regression coverage. Residual risk remains around synchronous v25→v28 migration and common-query latency on very large databases.
Testing
- Not run (automation review; PR code was not executed).
- GitHub checks observed: build, drift-gate, integration, and test passed.
- Large-database migration/search load test: Not found in repo/docs.
HAPI Bot
Summary
Default/Contentscope switch to session search.Problem / Motivation
Session search previously searched session metadata only. Users could not find a session from a phrase remembered from the conversation, and opening a content-search result did not reliably navigate to the matching historical message.
Content search is intentionally opt-in so the existing metadata search remains the default, predictable, and lightweight behavior.
Implementation
Search scope
DefaultandContentoptions.Content.413for oversized requests.Content indexing and APIs
LIKEscans.Historical message navigation
Multiple matches and UI
<strong>.Migration / Performance
Validation
bun typecheckbun run --cwd shared test— 269 tests passedbun run --cwd hub test -- src/store/messageContentSearch.test.ts src/store/migration-v23.test.ts src/web/routes/sessions.test.ts scripts/cleanup-sessions.test.ts— 88 tests passedbun run test:web— 260 test files and 2508 tests passedpwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name session-content-search -Suite Root -TestArgs terminal-wrap-fidelity.spec.ts— 2 passedpwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name session-content-search -Suite Live -TestArgs session-content-search-jump.spec.ts— 5 passedbun run buildgit diff --checkRelated Issues
Fixes #1554
AI Assistance
Implemented with AI-assisted pair programming using OpenAI Codex and GPT-5.6.