Add search functionality to Independent Panel chat history #878
Conversation
Copilot
AI
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PeterDaveHello
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to ce93b66
Previous suggestions✅ Suggestions up to commit 8b4b361
✅ Suggestions up to commit 8f27ec4
✅ Suggestions up to commit 8b7a3f9
✅ Suggestions up to commit c2bdd00
✅ Suggestions up to commit fd448a9
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/IndependentPanel/App.jsx (1)
156-165: Refactor all string-basedstyleprops and replace array index keys with stable IDsWe’ve confirmed that using
style="…"andkey={index}occurs in multiple components and can lead to runtime warnings and unstable UI updates. These should be updated to React’s object-style props and stable keys:• src/pages/IndependentPanel/App.jsx
– Replacekey={index}withkey={session.sessionId}
– Changestyle="display: flex; align-items: center; justify-content: space-between;"to an object literal• Other files using string-based
styleprops
- src/pages/IndependentPanel/App.jsx (line 201):
<div … style="height:100%;">- src/popup/sections/GeneralPart.jsx (lines 178, 217, 234, 250, 267, 280)
- src/popup/sections/FeaturePages.jsx (line 22)
- src/popup/Popup.jsx (line 97)
- src/services/clients/bing/BingImageCreator.js (lines 396–398, 424–427)
- src/components/ConversationItem/index.jsx (line 12)
- src/components/ConversationCard/index.jsx (lines 344–346, 376–378, 438–439)
Apply this diff in App.jsx as an example:
--- a/src/pages/IndependentPanel/App.jsx +++ b/src/pages/IndependentPanel/App.jsx @@ -160,9 +160,10 @@ {filteredSessions.map( (session, index) => ( - <button - key={index} + <button key={session.sessionId} className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`} - style="display: flex; align-items: center; justify-content: space-between;" + style={{ + display: 'flex', + alignItems: 'center', + justifyContent: 'space-between', + }} onClick={() => setSessionIdSafe(session.sessionId)} > {session.sessionName}Please update all occurrences to ensure consistency and prevent potential reconciliation bugs.
♻️ Duplicate comments (1)
src/pages/IndependentPanel/App.jsx (1)
119-125: Array.isArray check likely redundant per data shapeA previous review noted this already. If
conversationRecordsis always an array, the presence check is enough; otherwise, keep the guard.Minimal tweak if you keep the current (non-memoized) approach:
- if (session.conversationRecords && Array.isArray(session.conversationRecords)) { + if (session.conversationRecords) {
🧹 Nitpick comments (6)
src/_locales/fr/main.json (1)
100-101: Minor wording suggestion for more idiomatic French.“Rechercher des conversations...” is fine. If you prefer a slightly more natural phrasing for a placeholder, consider “Rechercher dans les conversations...”.
Apply if desired:
- "Search conversations...": "Rechercher des conversations...", + "Search conversations...": "Rechercher dans les conversations...",src/_locales/zh-hant/main.json (1)
100-101: Unify terminology: use 對話記錄 vs. 聊天記錄 consistently.This locale earlier uses “對話記錄” (e.g., Line 99: “清空對話記錄”), but the new placeholder says “聊天記錄...”. Recommend aligning to “對話記錄” for consistency within zh-Hant.
Apply this diff:
- "Search conversations...": "搜尋聊天記錄...", + "Search conversations...": "搜尋對話記錄...",src/pages/IndependentPanel/styles.scss (1)
93-103: Add box-sizing and line-height for robust sizingWithout border-box, padding + border can cause subtle overflow in narrow widths; line-height also improves vertical rhythm.
Apply this diff:
.search-input { width: 100%; min-height: 40px; padding: 8px 12px; border: 1px solid var(--theme-border-color); border-radius: 5px; background-color: var(--theme-color); color: var(--font-color); font-size: 14px; font-family: 'Cairo', sans-serif; + box-sizing: border-box; + line-height: 1.2; }src/pages/IndependentPanel/App.jsx (3)
107-129: Memoize filtering and simplify normalizationThe filter runs on every render; useMemo avoids unnecessary work. Also normalize via toLocaleLowerCase and guard question/answer with String() to be resilient.
Apply this diff:
- // Filter sessions based on search query - const filteredSessions = sessions.filter((session) => { - if (!searchQuery.trim()) return true - - const query = searchQuery.toLowerCase() - - // Search in session name - if (session.sessionName && session.sessionName.toLowerCase().includes(query)) { - return true - } - - // Search in conversation records - if (session.conversationRecords && Array.isArray(session.conversationRecords)) { - return session.conversationRecords.some((record) => { - const questionMatch = record.question && record.question.toLowerCase().includes(query) - const answerMatch = record.answer && record.answer.toLowerCase().includes(query) - return questionMatch || answerMatch - }) - } - - return false - }) + // Filter sessions based on search query + const filteredSessions = useMemo(() => { + const q = searchQuery.trim().toLocaleLowerCase() + if (!q) return sessions + return sessions.filter((session) => { + // Search in session name + if (session.sessionName && String(session.sessionName).toLocaleLowerCase().includes(q)) { + return true + } + // Search in conversation records + if (session.conversationRecords) { + return session.conversationRecords.some((record) => { + const question = record?.question ? String(record.question).toLocaleLowerCase() : '' + const answer = record?.answer ? String(record.answer).toLocaleLowerCase() : '' + return question.includes(q) || answer.includes(q) + }) + } + return false + }) + }, [sessions, searchQuery])And add the missing import:
- import { useEffect, useRef, useState } from 'react' + import { useEffect, useRef, useState, useMemo } from 'react'
146-154: Prefer input type=search and add basic a11y hintsType=search gives platform affordances. Add aria-label and enterKeyHint for better UX. Consider disabling autocomplete for privacy.
Apply this diff:
- <div className="search-container"> - <input - type="text" - placeholder={t('Search conversations...')} - value={searchQuery} - onChange={(e) => setSearchQuery(e.target.value)} - className="search-input" - /> - </div> + <div className="search-container"> + <input + type="search" + aria-label={t('Search')} + enterKeyHint="search" + autoComplete="off" + placeholder={t('Search conversations...')} + value={searchQuery} + onChange={(e) => setSearchQuery(e.target.value)} + className="search-input" + /> + </div>Optional: add Escape-to-clear behavior if desired.
156-185: Optional: show a no-results hintThe PR description mentions a graceful no-results state, but none is rendered. Consider a small placeholder when
filteredSessions.length === 0.Example (strings would need i18n keys, e.g., “No results”):
<div className="chat-list"> - {filteredSessions.map( + {filteredSessions.length === 0 ? ( + <div className="empty-state">{t('No results')}</div> + ) : filteredSessions.map( ( session, index, // TODO editable session name ) => ( <buttonIf you want, I can add the “No results” key across locales in a follow-up patch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/_locales/en/main.json(1 hunks)src/_locales/es/main.json(1 hunks)src/_locales/fr/main.json(1 hunks)src/_locales/ja/main.json(1 hunks)src/_locales/zh-hans/main.json(1 hunks)src/_locales/zh-hant/main.json(1 hunks)src/pages/IndependentPanel/App.jsx(3 hunks)src/pages/IndependentPanel/styles.scss(1 hunks)
🔇 Additional comments (6)
src/_locales/ja/main.json (1)
100-101: LGTM: Japanese search strings read naturally and align with file-wide style.Both keys are accurate and consistent with existing placeholder punctuation (“...”) used elsewhere in this locale.
src/_locales/en/main.json (1)
100-101: All locale files include the new keys as expectedThe verification script ran across every
src/_locales/**/main.jsonand exited cleanly (exit code 0), confirming that:
- Both
"Search"and"Search conversations..."keys are present in every locale.- Both keys map to non-empty string values in all files.
No further action is required—approving these changes.
src/_locales/es/main.json (1)
100-101: LGTM: Spanish translations look correct and consistent.“Buscar” and the placeholder read naturally and align with existing style.
src/pages/IndependentPanel/styles.scss (1)
88-92: LGTM on container placementThe search container slots cleanly into the sidebar stack and inherits theming tokens correctly.
src/_locales/zh-hans/main.json (1)
100-101: LGTM: keys and Simplified translations look correct“搜索”“搜索聊天记录...” fit existing tone and punctuation conventions in this file.
src/pages/IndependentPanel/App.jsx (1)
28-28: State addition looks good
searchQueryis scoped and initialized properly.
|
@copilot please check the reviews and see if we need some updates. |
I've reviewed the feedback from both Copilot and CodeRabbit. There are several actionable improvements to address: Critical Issues:
Performance & Code Quality: Minor Improvements: I'll implement these changes now. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds search functionality to the Independent Panel's chat history sidebar to help users find conversations more easily, particularly when Bing conversations expire frequently. The implementation includes real-time filtering, multi-language support, and seamless theme integration.
- Real-time search filtering that searches across session names, questions, and answers
- Multi-language translations added for 6 languages with proper localization keys
- CSS styling that matches existing theme system with proper focus states and accessibility
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| App.jsx | Implements search state management, filtering logic, and search input component |
| styles.scss | Adds themed CSS styling for search input with proper accessibility features |
| main.json (6 files) | Adds localized search translations for English, Spanish, French, Japanese, and Chinese variants |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Persistent suggestions updated to latest commit 67db72f |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/IndependentPanel/App.jsx (1)
291-365: Critical: button nested inside button breaks HTML/a11y; render DeleteButton as a siblingInteractive controls must not be nested. This causes keyboard activation and focus issues. Render the session button and the delete control as siblings within the list item.
- {filteredSessions.map((session) => ( - <div role="listitem" key={session.sessionId}> - <button - type="button" - aria-current={sessionId === session.sessionId ? 'page' : undefined} - className={`normal-button chat-list-item ${ - sessionId === session.sessionId ? 'active' : '' - }`} - onClick={() => { - setSessionIdSafe(session.sessionId) - }} - > - <span className="chat-list-title">{session.sessionName}</span> - <span - className="gpt-util-group" - onClick={(e) => { - e.stopPropagation() - }} - > - <DeleteButton - size={14} - text={t('Delete Conversation')} - onConfirm={async () => { - const deletedId = session.sessionId - const updatedSessions = await deleteSession(deletedId) - if (!isMountedRef.current) return - setSessions(updatedSessions) - if (!updatedSessions || updatedSessions.length === 0) { - stopCurrentPort() - setSessionId(null) - setCurrentSession(null) - return - } - // Only change active session if the deleted one was active - if (sessionId === deletedId) { - // When searching, prefer the next visible item in the filtered result - const q = normalizeForSearch(debouncedQuery).trim() - if (q) { - const SEP = '\n—\n' - const nextFiltered = updatedSessions.find((s) => { - if (!s || !s.sessionId) return false - const nameNorm = normalizeForSearch(s?.sessionName) - let bodyNorm = '' - if (Array.isArray(s?.conversationRecords)) { - bodyNorm = s.conversationRecords - .map( - (r) => - `${normalizeForSearch(r?.question)} ${normalizeForSearch( - r?.answer, - )}`, - ) - .join(SEP) - } - return nameNorm.includes(q) || bodyNorm.includes(q) - }) - if (nextFiltered) { - await setSessionIdSafe(nextFiltered.sessionId) - return - } - } - // Fallback to first valid item in full list - const next = updatedSessions.find((s) => s && s.sessionId) - if (next) { - await setSessionIdSafe(next.sessionId) - } else { - setSessionId(null) - setCurrentSession(null) - } - } - }} - /> - </span> - </button> - </div> - ))} + {filteredSessions.map((session) => ( + <div role="listitem" key={session.sessionId} className="chat-list-row"> + <button + type="button" + aria-current={sessionId === session.sessionId ? 'page' : undefined} + className={`normal-button chat-list-item ${ + sessionId === session.sessionId ? 'active' : '' + }`} + onClick={() => setSessionIdSafe(session.sessionId)} + > + <span className="chat-list-title">{session.sessionName}</span> + </button> + <span className="gpt-util-group" onClick={(e) => e.stopPropagation()}> + <DeleteButton + size={14} + text={t('Delete Conversation')} + onConfirm={async () => { + const deletedId = session.sessionId + const updatedSessions = await deleteSession(deletedId) + if (!isMountedRef.current) return + setSessions(updatedSessions) + if (!updatedSessions || updatedSessions.length === 0) { + stopCurrentPort() + setSessionId(null) + setCurrentSession(null) + return + } + if (sessionId === deletedId) { + const q = normalizeForSearch(debouncedQuery).trim() + if (q) { + const nextFiltered = updatedSessions.find((s) => { + if (!s || !s.sessionId) return false + const nameNorm = normalizeForSearch(s?.sessionName) + let bodyNorm = '' + if (Array.isArray(s?.conversationRecords)) { + bodyNorm = s.conversationRecords + .map((r) => `${normalizeForSearch(r?.question)} ${normalizeForSearch(r?.answer)}`) + .join('\n—\n') + } + return nameNorm.includes(q) || bodyNorm.includes(q) + }) + if (nextFiltered) { + await setSessionIdSafe(nextFiltered.sessionId) + return + } + } + const next = updatedSessions.find((s) => s && s.sessionId) + if (next) await setSessionIdSafe(next.sessionId) + else { + setSessionId(null) + setCurrentSession(null) + } + } + }} + /> + </span> + </div> + ))}
🧹 Nitpick comments (9)
src/components/DeleteButton/index.jsx (2)
63-74: Make the icon non-focusable when hidden and DRY the open-confirm logicAvoid leaving a hidden, focusable control in the tab order; also reuse one handler for click/keyboard.
- tabIndex={0} + tabIndex={waitConfirm ? -1 : 0} ... - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - e.stopPropagation() - setWaitConfirm(true) - } - }} - onClick={(e) => { - e.stopPropagation() - setWaitConfirm(true) - }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + e.stopPropagation() + setWaitConfirm(true) + } + }} + onClick={(e) => { + e.stopPropagation() + setWaitConfirm(true) + }}Optionally, extract a small
openConfirm(e)function and call it from both handlers to avoid duplication.
24-31: Nit: avoid inline font sizingPrefer a CSS class for the tiny confirm button to keep styling consistent.
src/pages/IndependentPanel/App.jsx (7)
85-91: Tighten effect dependency for collapsed sync
[config && config.independentPanelCollapsed]can be brittle. Use optional chaining directly for clarity.- }, [config && config.independentPanelCollapsed]) + }, [config?.independentPanelCollapsed])
139-156: Simplify and speed up input sanitizationThe code point loop is overkill for a search box. A single regex is simpler and faster.
- const raw = e?.target?.value ?? '' - // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points - const CP_TAB = 9 - const CP_LF = 10 - const CP_CR = 13 - const CP_PRINTABLE_MIN = 32 - const CP_DEL = 127 - const isAllowedCodePoint = (cp) => - cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) - const sanitizedArr = Array.from(raw).filter((ch) => { - const cp = ch.codePointAt(0) - return cp != null && isAllowedCodePoint(cp) - }) - const limited = sanitizedArr.slice(0, 500).join('') - setSearchQuery(limited) + const raw = e?.target?.value ?? '' + const cleaned = raw.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '').slice(0, 500) + setSearchQuery(cleaned)
92-99: Guard against out-of-order session loadsIf
sessionIdchanges rapidly, late resolves can overwrite newer state. Ignore stale results.- useEffect(() => { - // eslint-disable-next-line - ;(async () => { - if (sessions.length > 0) { - setCurrentSession((await getSession(sessionId)).session) - } - })() - }, [sessionId]) + useEffect(() => { + let alive = true + ;(async () => { + if (sessions.length > 0) { + const { session } = await getSession(sessionId) + if (alive) setCurrentSession(session || null) + } + })() + return () => { + alive = false + } + }, [sessionId, sessions])
172-203: Don’t always hijack Ctrl/Cmd+FCurrently prevents the browser find even when typing in fields or when users want to search the conversation body. Only intercept when not typing; otherwise let default behavior proceed.
- const onKeyDown = (e) => { + const onKeyDown = (e) => { const target = e.target const isTypingField = target && (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) - // Always route find shortcut to panel search (and auto-expand temporarily) - if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { - e.preventDefault() - focusSearch() - return - } + // Route find to sidebar search only when not typing in another field + if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { + if (!isTypingField) { + e.preventDefault() + focusSearch() + } + return + }Would you prefer additionally limiting this to when the sidebar is visible?
205-217: Move pure helpers outside the component
toSafeString/normalizeForSearchare pure; define them at module scope to avoid re-creation each render.
218-235: Indexing approach is sound; minor nit on constantsPrecomputing a normalized index is a solid perf win. Consider hoisting
SEPto module scope and reusing it in the delete handler to avoid duplication.
266-283: ClearforceExpandon blur unconditionallyIf the user focuses search while expanded,
forceExpandcan remain true after blur. Clearing it always keeps the collapse logic predictable.- onBlur={() => collapsed && setForceExpand(false)} + onBlur={() => setForceExpand(false)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/DeleteButton/index.jsx(2 hunks)src/pages/IndependentPanel/App.jsx(7 hunks)
🔇 Additional comments (6)
src/pages/IndependentPanel/App.jsx (6)
101-111: Good fix: clears temporary expand and persists pin stateClearing
forceExpandhere prevents the “stuck expanded” edge case; try/catch aroundsetUserConfigis appropriate.
125-137: Good: stop port and fully reset when clearing all sessionsThis closes the streaming port, clears selection, and resets search state—prevents leaks and stale UI.
157-162: Debounce looks good200ms debounce via effect is clean and avoids global timers.
236-244: LGTM: memoized filtering over normalized indexClear, efficient, and resilient to partial matches and diacritics.
248-256: Sidebar toggle and ARIA look correct
aria-expandedon the pin button and thecollapsed && !forceExpandclass logic read well.
381-383: Keying ConversationCard by sessionId is fineRemounting per session change is intentional here to reset per-session UI state.
|
@coderabbitai review |
|
/gemini review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@PeterDaveHello I'll conduct a comprehensive review of PR #878 that adds search functionality to the Independent Panel chat history. This review will examine: Core Implementation:
UI/UX & Styling:
Localization & Accessibility:
Component Enhancements:
Code Quality:
The review will analyze the search implementation quality, debouncing logic, theme integration, translation accuracy, accessibility compliance, and overall code architecture to ensure this feature meets the project's quality and performance standards. ✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a search functionality to the chat history sidebar, along with extensive translations and accessibility improvements. The implementation is well-done, especially the use of useMemo for performance and the thoughtful accessibility attributes. My feedback focuses on improving code maintainability and structure in App.jsx by refactoring duplicated logic and moving constants to the appropriate scope. Overall, these are great enhancements.
| const CP_TAB = 9 | ||
| const CP_LF = 10 | ||
| const CP_CR = 13 | ||
| const CP_PRINTABLE_MIN = 32 | ||
| const CP_DEL = 127 | ||
| const isAllowedCodePoint = (cp) => | ||
| cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) |
There was a problem hiding this comment.
These constants and the helper function isAllowedCodePoint are redefined on every render. For better performance and code clarity, they should be defined once outside the App component, at the module level. You can remove them from this function and define them above the App component.
| onConfirm={async () => { | ||
| const deletedId = session.sessionId | ||
| const updatedSessions = await deleteSession(deletedId) | ||
| if (!isMountedRef.current) return | ||
| setSessions(updatedSessions) | ||
| if (!updatedSessions || updatedSessions.length === 0) { | ||
| stopCurrentPort() | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| return | ||
| } | ||
| // Only change active session if the deleted one was active | ||
| if (sessionId === deletedId) { | ||
| // When searching, prefer the next visible item in the filtered result | ||
| const q = normalizeForSearch(debouncedQuery).trim() | ||
| if (q) { | ||
| const SEP = '\n—\n' | ||
| const nextFiltered = updatedSessions.find((s) => { | ||
| if (!s || !s.sessionId) return false | ||
| const nameNorm = normalizeForSearch(s?.sessionName) | ||
| let bodyNorm = '' | ||
| if (Array.isArray(s?.conversationRecords)) { | ||
| bodyNorm = s.conversationRecords | ||
| .map( | ||
| (r) => | ||
| `${normalizeForSearch(r?.question)} ${normalizeForSearch( | ||
| r?.answer, | ||
| )}`, | ||
| ) | ||
| .join(SEP) | ||
| } | ||
| return nameNorm.includes(q) || bodyNorm.includes(q) | ||
| }) | ||
| if (nextFiltered) { | ||
| await setSessionIdSafe(nextFiltered.sessionId) | ||
| return | ||
| } | ||
| } | ||
| // Fallback to first valid item in full list | ||
| const next = updatedSessions.find((s) => s && s.sessionId) | ||
| if (next) { | ||
| await setSessionIdSafe(next.sessionId) | ||
| } else { | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
This onConfirm handler is quite large and contains duplicated logic, which impacts readability and maintainability. I have two suggestions:
- Extract to a named function: The entire logic could be extracted into a separate function, e.g.,
handleDeleteSession(deletedId), to make the JSX cleaner. - Remove duplicated search logic: The logic to find the next filtered session duplicates the main filtering logic. You can extract the logic that creates
normalizedIndexinto a helper functioncreateNormalizedIndex(sessions)and reuse it here. This avoids having to update search logic in two places.
Here is a suggested implementation that addresses the duplicated logic:
onConfirm={async () => {
const deletedId = session.sessionId
const updatedSessions = await deleteSession(deletedId)
if (!isMountedRef.current) return
setSessions(updatedSessions)
if (!updatedSessions || updatedSessions.length === 0) {
stopCurrentPort()
setSessionId(null)
setCurrentSession(null)
return
}
// Only change active session if the deleted one was active
if (sessionId === deletedId) {
const q = normalizeForSearch(debouncedQuery).trim()
if (q) {
const updatedNormalizedIndex = createNormalizedIndex(updatedSessions)
const nextFilteredItem = updatedNormalizedIndex.find(
(i) => i.nameNorm.includes(q) || i.bodyNorm.includes(q),
)
if (nextFilteredItem) {
await setSessionIdSafe(nextFilteredItem.session.sessionId)
return
}
}
// Fallback to first valid item in full list
const next = updatedSessions.find((s) => s && s.sessionId)
if (next) {
await setSessionIdSafe(next.sessionId)
} else {
setSessionId(null)
setCurrentSession(null)
}
}
}}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| const clearConversations = async () => { | ||
| const sessions = await resetSessions() | ||
| setSessions(sessions) | ||
| await setSessionIdSafe(sessions[0].sessionId) | ||
| const next = await resetSessions() | ||
| setSessions(next) | ||
| if (next && next.length > 0) { | ||
| await setSessionIdSafe(next[0].sessionId) | ||
| } else { | ||
| stopCurrentPort() | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| setSearchQuery('') | ||
| setDebouncedQuery('') |
There was a problem hiding this comment.
[P2] Search filter never resets after clearing history
When the user clears conversations while a search filter is active, the search input and its debounced value remain unchanged, so the sidebar continues to render an empty result with “No conversations found”. The intended reset logic in clearConversations only runs when resetSessions() returns an empty array, but that service always creates one default session, so the branch is never taken. As a result the new default session is hidden from the list until the user manually clears the search field. The cleanup should also clear searchQuery/debouncedQuery when the conversations are reset so the list reflects the newly created session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DeleteButton/index.jsx (1)
15-20: Guard against setState on unmounted component during async confirmIf the parent unmounts this component after a successful delete, the pending
setWaitConfirm(false)/setConfirming(false)will fire on an unmounted component and trigger React warnings. Guard with a mounted ref.function DeleteButton({ onConfirm, size, text }) { const { t } = useTranslation() const [waitConfirm, setWaitConfirm] = useState(false) const confirmRef = useRef(null) const [confirming, setConfirming] = useState(false) + const mountedRef = useRef(true) + useEffect(() => { + mountedRef.current = true + return () => { + mountedRef.current = false + } + }, []) ... onClick={async (e) => { if (confirming) return e.preventDefault() e.stopPropagation() setConfirming(true) try { await onConfirm() - setWaitConfirm(false) + if (mountedRef.current) setWaitConfirm(false) } catch (err) { // Keep confirmation visible to allow retry; optionally log // eslint-disable-next-line no-console console.error(err) } finally { - setConfirming(false) + if (mountedRef.current) setConfirming(false) } }}Also applies to: 41-55
♻️ Duplicate comments (2)
src/pages/IndependentPanel/App.jsx (2)
188-200: Don’t hijack Ctrl/Cmd+F when typing or when panel shouldn’t capture searchOnly override the shortcut when not typing in another field; otherwise let the browser find dialog work. This avoids surprising users.
- if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { - e.preventDefault() - focusSearch() - return - } + if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { + if (isTypingField) return + e.preventDefault() + focusSearch() + return + }
291-364: Fix invalid nested interactive controls (button inside button)
DeleteButtonrenders an interactive button; nesting it inside the session<button>is invalid HTML and harms a11y/keyboard behavior. Render them as siblings.- {filteredSessions.map((session) => ( - <div role="listitem" key={session.sessionId}> - <button - type="button" - aria-current={sessionId === session.sessionId ? 'page' : undefined} - className={`normal-button chat-list-item ${sessionId === session.sessionId ? 'active' : ''}`} - onClick={() => { - setSessionIdSafe(session.sessionId) - }} - > - <span className="chat-list-title">{session.sessionName}</span> - <span - className="gpt-util-group" - onClick={(e) => { - e.stopPropagation() - }} - > - <DeleteButton - size={14} - text={t('Delete Conversation')} - onConfirm={async () => { - const deletedId = session.sessionId - const updatedSessions = await deleteSession(deletedId) - if (!isMountedRef.current) return - setSessions(updatedSessions) - if (!updatedSessions || updatedSessions.length === 0) { - stopCurrentPort() - setSessionId(null) - setCurrentSession(null) - return - } - // Only change active session if the deleted one was active - if (sessionId === deletedId) { - // When searching, prefer the next visible item in the filtered result - const q = normalizeForSearch(debouncedQuery).trim() - if (q) { - const SEP = '\n—\n' - const nextFiltered = updatedSessions.find((s) => { - if (!s || !s.sessionId) return false - const nameNorm = normalizeForSearch(s?.sessionName) - let bodyNorm = '' - if (Array.isArray(s?.conversationRecords)) { - bodyNorm = s.conversationRecords - .map((r) => `${normalizeForSearch(r?.question)} ${normalizeForSearch(r?.answer)}`) - .join(SEP) - } - return nameNorm.includes(q) || bodyNorm.includes(q) - }) - if (nextFiltered) { - await setSessionIdSafe(nextFiltered.sessionId) - return - } - } - // Fallback to first valid item in full list - const next = updatedSessions.find((s) => s && s.sessionId) - if (next) { - await setSessionIdSafe(next.sessionId) - } else { - setSessionId(null) - setCurrentSession(null) - } - } - }} - /> - </span> - </button> - </div> - ))} + {filteredSessions.map((session) => ( + <div role="listitem" key={session.sessionId} className="chat-list-row"> + <button + type="button" + aria-current={sessionId === session.sessionId ? 'page' : undefined} + className={`normal-button chat-list-item ${sessionId === session.sessionId ? 'active' : ''}`} + onClick={() => setSessionIdSafe(session.sessionId)} + > + <span className="chat-list-title">{session.sessionName}</span> + </button> + <span className="gpt-util-group" onClick={(e) => e.stopPropagation()}> + <DeleteButton + size={14} + text={t('Delete Conversation')} + onConfirm={async () => { + const deletedId = session.sessionId + const updatedSessions = await deleteSession(deletedId) + if (!isMountedRef.current) return + setSessions(updatedSessions) + if (!updatedSessions || updatedSessions.length === 0) { + stopCurrentPort() + setSessionId(null) + setCurrentSession(null) + return + } + if (sessionId === deletedId) { + const q = normalizeForSearch(debouncedQuery).trim() + if (q) { + const SEP = '\n—\n' + const nextFiltered = updatedSessions.find((s) => { + if (!s || !s.sessionId) return false + const nameNorm = normalizeForSearch(s?.sessionName) + let bodyNorm = '' + if (Array.isArray(s?.conversationRecords)) { + bodyNorm = s.conversationRecords + .map((r) => `${normalizeForSearch(r?.question)} ${normalizeForSearch(r?.answer)}`) + .join(SEP) + } + return nameNorm.includes(q) || bodyNorm.includes(q) + }) + if (nextFiltered) { + await setSessionIdSafe(nextFiltered.sessionId) + return + } + } + const next = updatedSessions.find((s) => s && s.sessionId) + if (next) await setSessionIdSafe(next.sessionId) + else { + setSessionId(null) + setCurrentSession(null) + } + } + }} + /> + </span> + </div> + ))}
🧹 Nitpick comments (3)
src/pages/IndependentPanel/App.jsx (3)
139-155: Simplify and speed up search input sanitizationRegex replace is clearer and faster; wrap handler with
useCallback.- const handleSearchChange = (e) => { - const raw = e?.target?.value ?? '' - // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points - const CP_TAB = 9 - const CP_LF = 10 - const CP_CR = 13 - const CP_PRINTABLE_MIN = 32 - const CP_DEL = 127 - const isAllowedCodePoint = (cp) => - cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) - const sanitizedArr = Array.from(raw).filter((ch) => { - const cp = ch.codePointAt(0) - return cp != null && isAllowedCodePoint(cp) - }) - const limited = sanitizedArr.slice(0, 500).join('') - setSearchQuery(limited) - } + const handleSearchChange = useCallback((e) => { + const raw = e?.target?.value ?? '' + const cleaned = raw.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '').slice(0, 500) + setSearchQuery(cleaned) + }, [])Also add
useCallbackto your imports.
280-282: Always clear temporary expansion on blur (belt-and-suspenders)
toggleSidebaralready clearsforceExpand, but clearing on blur regardless of state avoids edge cases.- onBlur={() => collapsed && setForceExpand(false)} + onBlur={() => setForceExpand(false)}Also applies to: 101-111
205-217: Hoist pure helpers outside the componentAvoid re-creating
toSafeString/normalizeForSearchon every render.- // Utility function to safely convert any value to a string - const toSafeString = (value) => - typeof value === 'string' ? value : value == null ? '' : String(value) - - // Normalization utility for search - const normalizeForSearch = (value) => - toSafeString(value) - .toLowerCase() - .normalize('NFD') - .replace(/[\u0300-\u036f]/g, '') - .replace(/\s+/g, ' ') - .trim() + // moved above component (see added block below)Add near the imports (top of file):
+// Utility: safe string conversion +const toSafeString = (value) => + typeof value === 'string' ? value : value == null ? '' : String(value) + +// Utility: normalize for diacritic- and case-insensitive search +const normalizeForSearch = (value) => + toSafeString(value) + .toLowerCase() + .normalize('NFD') + .replace(/[\u0300-\u036f]/g, '') + .replace(/\s+/g, ' ') + .trim()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/DeleteButton/index.jsx(2 hunks)src/pages/IndependentPanel/App.jsx(7 hunks)
🔇 Additional comments (5)
src/components/DeleteButton/index.jsx (1)
32-34: Confirm button disabled/busy wiring looks goodAlso applies to: 39-40
src/pages/IndependentPanel/App.jsx (4)
34-57: Port stop + safe session selection looks solid
85-90: Persist/sync collapsed state LGTM
125-137: Clear resets also wipe search state — good
218-244: Precomputed normalized index + debounced filter are well done
| role="button" | ||
| tabIndex={0} | ||
| aria-label={text} | ||
| aria-hidden={waitConfirm ? 'true' : undefined} | ||
| style={waitConfirm ? { visibility: 'hidden' } : {}} | ||
| onKeyDown={(e) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid aria-hidden on a focusable control
aria-hidden must not be applied to focusable/interactive elements. When hidden, remove it from the tab order.
- role="button"
- tabIndex={0}
+ role="button"
+ tabIndex={waitConfirm ? -1 : 0}
aria-label={text}
- aria-hidden={waitConfirm ? 'true' : undefined}
- style={waitConfirm ? { visibility: 'hidden' } : {}}
+ aria-hidden={waitConfirm ? 'true' : undefined}
+ style={waitConfirm ? { visibility: 'hidden', pointerEvents: 'none' } : {}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| role="button" | |
| tabIndex={0} | |
| aria-label={text} | |
| aria-hidden={waitConfirm ? 'true' : undefined} | |
| style={waitConfirm ? { visibility: 'hidden' } : {}} | |
| onKeyDown={(e) => { | |
| role="button" | |
| tabIndex={waitConfirm ? -1 : 0} | |
| aria-label={text} | |
| aria-hidden={waitConfirm ? 'true' : undefined} | |
| style={waitConfirm ? { visibility: 'hidden', pointerEvents: 'none' } : {}} | |
| onKeyDown={(e) => { |
🤖 Prompt for AI Agents
In src/components/DeleteButton/index.jsx around lines 63 to 68, the button
currently sets aria-hidden on a focusable control and uses visibility:hidden
while leaving it in the tab order; remove aria-hidden from the element and when
waitConfirm is true make it unfocusable by setting tabIndex to -1 (or
conditionally omit role/interactive attributes and render a non-interactive
element), and keep the visual hiding (visibility:hidden or display:none) so it
is not reachable by screen readers or keyboard navigation.
656f286 to
ce93b66
Compare
Add a sidebar search with debounced, diacritic-insensitive filtering across conversation titles and content. Include i18n strings for the search UI. Keep the panel responsive and accessible: precompute a normalized per-session index to keep typing smooth on long lists, improve keyboard navigation and shortcuts to focus the search and temporarily expand the sidebar, and move sidebar styling into SCSS. Persist and reliably sync the sidebar's collapsed state across reloads, handling storage failures without blocking the UI. Harden deletion and clear flows, sanitize inputs, and guard async updates to keep state consistent. Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
PeterDaveHello
force-pushed
the
copilot/fix-550
branch
from
ce93b66 to
43f9ab4
Compare
|
@coderabbitai review |
|
/gemini review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@PeterDaveHello I'll conduct a comprehensive review of PR #878 that adds search functionality to the Independent Panel chat history. This review will examine: Core Implementation:
UI/UX & Styling:
Localization & Accessibility:
Component Enhancements:
Code Quality:
The review will analyze the search implementation quality, debouncing logic, theme integration, translation accuracy, accessibility compliance, and overall code architecture to ensure this feature meets the project's quality and performance standards. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request adds a search functionality to the chat history sidebar, including real-time filtering, multi-language support, and UI improvements. The implementation is well-done, with good use of React hooks for performance optimization (useMemo, debouncing) and accessibility enhancements. My review includes two main points: one is a suggestion to refactor some duplicated logic in App.jsx to improve maintainability, and the other is an accessibility improvement in the DeleteButton component to use a native <button> element instead of a <span>.
| onConfirm={async () => { | ||
| const deletedId = session.sessionId | ||
| const updatedSessions = await deleteSession(deletedId) | ||
| if (!isMountedRef.current) return | ||
| setSessions(updatedSessions) | ||
| if (!updatedSessions || updatedSessions.length === 0) { | ||
| stopCurrentPort() | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| return | ||
| } | ||
| // Only change active session if the deleted one was active | ||
| if (sessionId === deletedId) { | ||
| // When searching, prefer the next visible item in the filtered result | ||
| const q = normalizeForSearch(debouncedQuery).trim() | ||
| if (q) { | ||
| const SEP = '\n—\n' | ||
| const nextFiltered = updatedSessions.find((s) => { | ||
| if (!s || !s.sessionId) return false | ||
| const nameNorm = normalizeForSearch(s?.sessionName) | ||
| let bodyNorm = '' | ||
| if (Array.isArray(s?.conversationRecords)) { | ||
| bodyNorm = s.conversationRecords | ||
| .map( | ||
| (r) => | ||
| `${normalizeForSearch(r?.question)} ${normalizeForSearch( | ||
| r?.answer, | ||
| )}`, | ||
| ) | ||
| .join(SEP) | ||
| } | ||
| return nameNorm.includes(q) || bodyNorm.includes(q) | ||
| }) | ||
| if (nextFiltered) { | ||
| await setSessionIdSafe(nextFiltered.sessionId) | ||
| return | ||
| } | ||
| } | ||
| // Fallback to first valid item in full list | ||
| const next = updatedSessions.find((s) => s && s.sessionId) | ||
| if (next) { | ||
| await setSessionIdSafe(next.sessionId) | ||
| } else { | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| } | ||
| } | ||
| }} | ||
| /> |
There was a problem hiding this comment.
This onConfirm handler contains logic to find the next session that duplicates the filtering logic used for the session list. This makes the code harder to maintain. To improve this, you can extract the session normalization logic into a helper function and reuse it.
First, move toSafeString and normalizeForSearch outside the App component, as they are pure functions. Then, create a new helper function getNormalizedSessionParts (also outside the component) and update normalizedIndex to use it:
const getNormalizedSessionParts = (session, normalizeFn) => {
const nameNorm = normalizeFn(session.sessionName)
let bodyNorm = ''
if (Array.isArray(session.conversationRecords)) {
const SEP = '\n—\n'
bodyNorm = session.conversationRecords
.map((r) => `${normalizeFn(r?.question)} ${normalizeFn(r?.answer)}`)
.join(SEP)
}
return { nameNorm, bodyNorm }
}
// in App component:
const normalizedIndex = useMemo(() => {
if (!Array.isArray(sessions)) return []
return sessions
.filter((s) => Boolean(s?.sessionId))
.map((s) => ({ session: s, ...getNormalizedSessionParts(s, normalizeForSearch) }))
}, [sessions])Then you can simplify this onConfirm handler as suggested below.
onConfirm={async () => {
const deletedId = session.sessionId
const updatedSessions = await deleteSession(deletedId)
if (!isMountedRef.current) return
setSessions(updatedSessions)
if (!updatedSessions || updatedSessions.length === 0) {
stopCurrentPort()
setSessionId(null)
setCurrentSession(null)
return
}
// Only change active session if the deleted one was active
if (sessionId === deletedId) {
// When searching, prefer the next visible item in the filtered result
const q = normalizeForSearch(debouncedQuery).trim()
if (q) {
const nextFiltered = updatedSessions.find((s) => {
if (!s || !s.sessionId) return false
const { nameNorm, bodyNorm } = getNormalizedSessionParts(s, normalizeForSearch)
return nameNorm.includes(q) || bodyNorm.includes(q)
})
if (nextFiltered) {
await setSessionIdSafe(nextFiltered.sessionId)
return
}
}
// Fallback to first valid item in full list
const next = updatedSessions.find((s) => s && s.sessionId)
if (next) {
await setSessionIdSafe(next.sessionId)
} else {
setSessionId(null)
setCurrentSession(null)
}
}
}}
|
Codex Review: Didn't find any major issues. Keep them coming! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
|
@PeterDaveHello I'll conduct a comprehensive review of PR #878 that adds search functionality to the Independent Panel chat history. This review will examine: Core Implementation:
UI/UX & Styling:
Localization & Accessibility:
Component Enhancements:
Code Quality:
The review will analyze the search implementation quality, debouncing logic, theme integration, translation accuracy, accessibility compliance, and overall code architecture to ensure this feature meets the project's quality and performance standards. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/IndependentPanel/App.jsx (1)
63-79: Initial load assumes sessions[0]; add defensive checks.Avoid indexing empty arrays and handle missing conversationRecords.
- const sessions = await getSessions() - if ( - urlFrom !== 'store' && - sessions[0].conversationRecords && - sessions[0].conversationRecords.length > 0 - ) { + const sessions = await getSessions() + const first = Array.isArray(sessions) ? sessions[0] : undefined + if ( + urlFrom !== 'store' && + first?.conversationRecords && + first.conversationRecords.length > 0 + ) { await createNewChat() } else { setSessions(sessions) - await setSessionIdSafe(sessions[0].sessionId) + if (first?.sessionId) await setSessionIdSafe(first.sessionId) }
♻️ Duplicate comments (7)
src/pages/IndependentPanel/styles.scss (2)
82-85: Reconsider showing search while collapsed (60px width is unusable).Either hide it when collapsed or surface it via a popover/expand-on-focus pattern only when hover/expanded.
Apply this if you opt to hide it when collapsed:
- .chat-sidebar.collapsed .search-container { - display: block; - } + .chat-sidebar.collapsed .search-container { + display: none; + }
121-133: Accessible focus styles: good fix.Retaining outline via :focus-visible preserves keyboard a11y.
src/pages/IndependentPanel/App.jsx (5)
279-282: Unconditionally clear temporary expansion on blur.Prevents “stuck expanded” after interacting with pin while focused.
- onBlur={() => collapsed && setForceExpand(false)} + onBlur={() => setForceExpand(false)}
92-99: Fix race/missing deps in currentSession loader.Out-of-order resolutions can set stale session; also add sessions as a dep.
-useEffect(() => { - // eslint-disable-next-line - ;(async () => { - if (sessions.length > 0) { - setCurrentSession((await getSession(sessionId)).session) - } - })() -}, [sessionId]) +useEffect(() => { + let cancelled = false + ;(async () => { + if (!sessionId) { + if (!cancelled) setCurrentSession(null) + return + } + if (!sessions.find((s) => s && s.sessionId === sessionId)) { + if (!cancelled) setCurrentSession(null) + return + } + const { session } = await getSession(sessionId) + if (!cancelled && isMountedRef.current) setCurrentSession(session) + })() + return () => { + cancelled = true + } +}, [sessionId, sessions])
125-137: Clear search after reset to avoid “No results” ghost state.Resetting history should also reset filters.
-const clearConversations = async () => { +const clearConversations = async () => { const next = await resetSessions() setSessions(next) + setSearchQuery('') + setDebouncedQuery('') if (next && next.length > 0) { await setSessionIdSafe(next[0].sessionId) } else { stopCurrentPort() setSessionId(null) setCurrentSession(null) - setSearchQuery('') - setDebouncedQuery('') } }
172-203: Don’t hijack Ctrl/Cmd+F when the sidebar is collapsed.Let the browser Find dialog work unless the sidebar/search is visible.
-useEffect(() => { +useEffect(() => { const focusSearch = () => { if (searchInputRef.current) { setForceExpand(true) searchInputRef.current.focus() searchInputRef.current.select() } } const onKeyDown = (e) => { const target = e.target const isTypingField = target && (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) + const sidebarVisible = !collapsed || forceExpand // Always route find shortcut to panel search (and auto-expand temporarily) - if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { - e.preventDefault() - focusSearch() - return - } + if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { + if (sidebarVisible) { + e.preventDefault() + focusSearch() + } + return + } // Quick open search with '/' when not typing in a field - if (!isTypingField && !e.ctrlKey && !e.metaKey && !e.altKey && e.key === '/') { + if (sidebarVisible && !isTypingField && !e.ctrlKey && !e.metaKey && !e.altKey && e.key === '/') { e.preventDefault() focusSearch() } } window.addEventListener('keydown', onKeyDown) return () => window.removeEventListener('keydown', onKeyDown) -}, []) +}, [collapsed, forceExpand])
291-364: Fix invalid nested interactive controls (button inside button).DeleteButton renders a button; nesting it inside the session button breaks semantics and keyboard a11y. Render as siblings.
- {filteredSessions.map((session) => ( - <div role="listitem" key={session.sessionId}> - <button - type="button" - aria-current={sessionId === session.sessionId ? 'page' : undefined} - className={`normal-button chat-list-item ${ - sessionId === session.sessionId ? 'active' : '' - }`} - onClick={() => { - setSessionIdSafe(session.sessionId) - }} - > - <span className="chat-list-title">{session.sessionName}</span> - <span - className="gpt-util-group" - onClick={(e) => { - e.stopPropagation() - }} - > - <DeleteButton - size={14} - text={t('Delete Conversation')} - onConfirm={async () => { - const deletedId = session.sessionId - const updatedSessions = await deleteSession(deletedId) - if (!isMountedRef.current) return - setSessions(updatedSessions) - if (!updatedSessions || updatedSessions.length === 0) { - stopCurrentPort() - setSessionId(null) - setCurrentSession(null) - return - } - // Only change active session if the deleted one was active - if (sessionId === deletedId) { - // When searching, prefer the next visible item in the filtered result - const q = normalizeForSearch(debouncedQuery).trim() - if (q) { - const SEP = '\n—\n' - const nextFiltered = updatedSessions.find((s) => { - if (!s || !s.sessionId) return false - const nameNorm = normalizeForSearch(s?.sessionName) - let bodyNorm = '' - if (Array.isArray(s?.conversationRecords)) { - bodyNorm = s.conversationRecords - .map( - (r) => - `${normalizeForSearch(r?.question)} ${normalizeForSearch( - r?.answer, - )}`, - ) - .join(SEP) - } - return nameNorm.includes(q) || bodyNorm.includes(q) - }) - if (nextFiltered) { - await setSessionIdSafe(nextFiltered.sessionId) - return - } - } - // Fallback to first valid item in full list - const next = updatedSessions.find((s) => s && s.sessionId) - if (next) { - await setSessionIdSafe(next.sessionId) - } else { - setSessionId(null) - setCurrentSession(null) - } - } - }} - /> - </span> - </button> - </div> - ))} + {filteredSessions.map((session) => ( + <div role="listitem" key={session.sessionId} className="chat-list-item"> + <button + type="button" + aria-current={sessionId === session.sessionId ? 'page' : undefined} + className={`normal-button chat-list-item ${sessionId === session.sessionId ? 'active' : ''}`} + onClick={() => setSessionIdSafe(session.sessionId)} + > + <span className="chat-list-title">{session.sessionName}</span> + </button> + <span className="gpt-util-group"> + <DeleteButton + size={14} + text={t('Delete Conversation')} + onConfirm={async () => { + const deletedId = session.sessionId + const updatedSessions = await deleteSession(deletedId) + if (!isMountedRef.current) return + setSessions(updatedSessions) + if (!updatedSessions || updatedSessions.length === 0) { + stopCurrentPort() + setSessionId(null) + setCurrentSession(null) + return + } + if (sessionId === deletedId) { + const q = normalizeForSearch(debouncedQuery).trim() + if (q) { + const SEP = '\n—\n' + const nextFiltered = updatedSessions.find((s) => { + if (!s || !s.sessionId) return false + const nameNorm = normalizeForSearch(s?.sessionName) + let bodyNorm = '' + if (Array.isArray(s?.conversationRecords)) { + bodyNorm = s.conversationRecords + .map((r) => `${normalizeForSearch(r?.question)} ${normalizeForSearch(r?.answer)}`) + .join(SEP) + } + return nameNorm.includes(q) || bodyNorm.includes(q) + }) + if (nextFiltered) { + await setSessionIdSafe(nextFiltered.sessionId) + return + } + } + const next = updatedSessions.find((s) => s && s.sessionId) + if (next) await setSessionIdSafe(next.sessionId) + else { + setSessionId(null) + setCurrentSession(null) + } + } + }} + /> + </span> + </div> + ))}
🧹 Nitpick comments (4)
src/pages/IndependentPanel/styles.scss (1)
101-105: Add inner padding to avoid edge-to-edge input.Small spacing improves usability and matches the rest of the sidebar spacing.
.search-container { - padding: 0; + padding: 10px; background-color: inherit; - margin-top: 15px; + margin-top: 10px; }src/pages/IndependentPanel/App.jsx (3)
139-155: Over-sanitizing search harms UX; simplify.Control chars aren’t necessary to strip; trim and length-cap is enough and faster.
-const handleSearchChange = (e) => { - const raw = e?.target?.value ?? '' - // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points - const CP_TAB = 9 - const CP_LF = 10 - const CP_CR = 13 - const CP_PRINTABLE_MIN = 32 - const CP_DEL = 127 - const isAllowedCodePoint = (cp) => - cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) - const sanitizedArr = Array.from(raw).filter((ch) => { - const cp = ch.codePointAt(0) - return cp != null && isAllowedCodePoint(cp) - }) - const limited = sanitizedArr.slice(0, 500).join('') - setSearchQuery(limited) -} +const handleSearchChange = (e) => { + const raw = e?.target?.value ?? '' + const cleaned = raw.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '').slice(0, 500) + setSearchQuery(cleaned) +}
205-217: Move pure helpers outside the component.toSafeString/normalizeForSearch don’t need re-creation per render.
381-396: Keying the container forces ConversationCard remounts.If you need to preserve internal state/scroll, remove the key here and let props drive updates.
- <div key={currentSession.sessionId} className="chatgptbox-container"> + <div className="chatgptbox-container">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/_locales/de/main.json(1 hunks)src/_locales/en/main.json(1 hunks)src/_locales/es/main.json(1 hunks)src/_locales/fr/main.json(1 hunks)src/_locales/in/main.json(1 hunks)src/_locales/it/main.json(1 hunks)src/_locales/ja/main.json(1 hunks)src/_locales/ko/main.json(1 hunks)src/_locales/pt/main.json(1 hunks)src/_locales/ru/main.json(1 hunks)src/_locales/tr/main.json(1 hunks)src/_locales/zh-hans/main.json(1 hunks)src/_locales/zh-hant/main.json(1 hunks)src/components/DeleteButton/index.jsx(2 hunks)src/config/index.mjs(1 hunks)src/pages/IndependentPanel/App.jsx(7 hunks)src/pages/IndependentPanel/styles.scss(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/_locales/it/main.json
🚧 Files skipped from review as they are similar to previous changes (14)
- src/_locales/pt/main.json
- src/_locales/ru/main.json
- src/_locales/zh-hans/main.json
- src/_locales/fr/main.json
- src/_locales/tr/main.json
- src/_locales/en/main.json
- src/_locales/ko/main.json
- src/config/index.mjs
- src/_locales/de/main.json
- src/_locales/ja/main.json
- src/_locales/in/main.json
- src/_locales/zh-hant/main.json
- src/_locales/es/main.json
- src/components/DeleteButton/index.jsx
🔇 Additional comments (4)
src/pages/IndependentPanel/styles.scss (3)
4-19: Theme tokens look good and consistent across modes.Adding --font-muted-color and --theme-secondary-color per theme is correct and fixes prior contrast concerns.
Also applies to: 24-40
107-119: Search input theming and sizing LGTM.Use of var(--theme-secondary-color, var(--theme-color)) prevents light-theme contrast issues.
186-189: Utility group hide-on-collapse behavior is sensible.Keeps destructive controls out of the cramped state.
src/pages/IndependentPanel/App.jsx (1)
34-44: Port shutdown helper LGTM.Centralizing stop/disconnect avoids leaks.
| const setSessionIdSafe = async (sessionId) => { | ||
| stopCurrentPort() | ||
| const { session, currentSessions } = await getSession(sessionId) | ||
| if (session) setSessionId(sessionId) | ||
| else if (currentSessions.length > 0) setSessionId(currentSessions[0].sessionId) | ||
| if (session && session.sessionId) { | ||
| setSessionId(session.sessionId) | ||
| } else if (Array.isArray(currentSessions) && currentSessions.length > 0) { | ||
| setSessionId(currentSessions[0].sessionId) | ||
| } else { | ||
| setSessionId(null) | ||
| setCurrentSession(null) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard getSession against null/invalid IDs.
Calling getSession(sessionId) with null/undefined can throw or fetch junk. Early-return and set state safely.
-const setSessionIdSafe = async (sessionId) => {
- stopCurrentPort()
- const { session, currentSessions } = await getSession(sessionId)
+const setSessionIdSafe = async (sessionId) => {
+ stopCurrentPort()
+ if (!sessionId) {
+ setSessionId(null)
+ setCurrentSession(null)
+ return
+ }
+ const { session, currentSessions } = await getSession(sessionId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setSessionIdSafe = async (sessionId) => { | |
| stopCurrentPort() | |
| const { session, currentSessions } = await getSession(sessionId) | |
| if (session) setSessionId(sessionId) | |
| else if (currentSessions.length > 0) setSessionId(currentSessions[0].sessionId) | |
| if (session && session.sessionId) { | |
| setSessionId(session.sessionId) | |
| } else if (Array.isArray(currentSessions) && currentSessions.length > 0) { | |
| setSessionId(currentSessions[0].sessionId) | |
| } else { | |
| setSessionId(null) | |
| setCurrentSession(null) | |
| } | |
| } | |
| const setSessionIdSafe = async (sessionId) => { | |
| stopCurrentPort() | |
| if (!sessionId) { | |
| setSessionId(null) | |
| setCurrentSession(null) | |
| return | |
| } | |
| const { session, currentSessions } = await getSession(sessionId) | |
| if (session && session.sessionId) { | |
| setSessionId(session.sessionId) | |
| } else if (Array.isArray(currentSessions) && currentSessions.length > 0) { | |
| setSessionId(currentSessions[0].sessionId) | |
| } else { | |
| setSessionId(null) | |
| setCurrentSession(null) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/pages/IndependentPanel/App.jsx around lines 46 to 57, guard getSession
against null/undefined/invalid IDs by early-returning: if sessionId is falsy or
not a valid string/number, call stopCurrentPort(), setSessionId(null),
setCurrentSession(null) and return before calling getSession; additionally wrap
the await getSession(sessionId) call in a try/catch to handle and log runtime
errors and in the catch setSessionId(null) and setCurrentSession(null) so state
is safe on failure.
Ensure the sidebar’s temporary expansion (forceExpand) is cleared when the search input loses focus. This prevents an edge case where the panel could remain stuck expanded if the pin state is toggled while the input is focused. - Unconditionally clear forceExpand on input blur - Kept behavior of clearing forceExpand on pin toggle Why: avoids a UX inconsistency reported in review while keeping scope minimal and isolated to the Independent Panel UI.
|
@coderabbitai review |
|
/gemini review |
|
@PeterDaveHello I'll conduct a comprehensive review of PR #878 that adds search functionality to the Independent Panel chat history. This review will examine: Core Implementation:
UI/UX & Styling:
Localization & Accessibility:
Component Enhancements:
Code Quality:
The review will analyze the search implementation quality, debouncing logic, theme integration, translation accuracy, accessibility compliance, and overall code architecture to ensure this feature meets the project's quality and performance standards. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive search functionality to the Independent Panel chat history sidebar, allowing users to search across session names, questions, and answers in real-time. The implementation includes keyboard shortcuts, multi-language support, and proper accessibility features.
Key Changes
- Implemented real-time search filtering with debouncing for optimal performance
- Added comprehensive translations for search UI across 13 languages
- Enhanced sidebar UI with persistent search visibility and improved chat list layout
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/IndependentPanel/App.jsx | Core search functionality with state management, filtering logic, keyboard shortcuts, and improved session handling |
| src/pages/IndependentPanel/styles.scss | Search input styling, theme integration, and layout improvements for sidebar and chat list items |
| src/components/DeleteButton/index.jsx | Enhanced async delete handling with proper error handling and accessibility improvements |
| src/config/index.mjs | Added default configuration for sidebar collapsed state persistence |
| src/_locales/*/main.json | Added search-related translations across 13 language files |
| // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points | ||
| const CP_TAB = 9 | ||
| const CP_LF = 10 | ||
| const CP_CR = 13 | ||
| const CP_PRINTABLE_MIN = 32 | ||
| const CP_DEL = 127 | ||
| const isAllowedCodePoint = (cp) => | ||
| cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) | ||
| const sanitizedArr = Array.from(raw).filter((ch) => { | ||
| const cp = ch.codePointAt(0) | ||
| return cp != null && isAllowedCodePoint(cp) | ||
| }) | ||
| const limited = sanitizedArr.slice(0, 500).join('') | ||
| setSearchQuery(limited) |
There was a problem hiding this comment.
The input sanitization logic is overly complex for a search input field. Standard search inputs don't typically need control character filtering beyond what the browser provides. Consider simplifying to just setSearchQuery(raw.slice(0, 500)) unless there's a specific security requirement that necessitates this complexity.
| // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points | |
| const CP_TAB = 9 | |
| const CP_LF = 10 | |
| const CP_CR = 13 | |
| const CP_PRINTABLE_MIN = 32 | |
| const CP_DEL = 127 | |
| const isAllowedCodePoint = (cp) => | |
| cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) | |
| const sanitizedArr = Array.from(raw).filter((ch) => { | |
| const cp = ch.codePointAt(0) | |
| return cp != null && isAllowedCodePoint(cp) | |
| }) | |
| const limited = sanitizedArr.slice(0, 500).join('') | |
| setSearchQuery(limited) | |
| // Truncate input to 500 characters for search query | |
| setSearchQuery(raw.slice(0, 500)) |
Copilot uses AI. Check for mistakes.
| if (sessionId === deletedId) { | ||
| // When searching, prefer the next visible item in the filtered result | ||
| const q = normalizeForSearch(debouncedQuery).trim() | ||
| if (q) { | ||
| const SEP = '\n—\n' | ||
| const nextFiltered = updatedSessions.find((s) => { | ||
| if (!s || !s.sessionId) return false | ||
| const nameNorm = normalizeForSearch(s?.sessionName) | ||
| let bodyNorm = '' | ||
| if (Array.isArray(s?.conversationRecords)) { | ||
| bodyNorm = s.conversationRecords | ||
| .map( | ||
| (r) => | ||
| `${normalizeForSearch(r?.question)} ${normalizeForSearch( | ||
| r?.answer, | ||
| )}`, | ||
| ) | ||
| .join(SEP) | ||
| } | ||
| return nameNorm.includes(q) || bodyNorm.includes(q) | ||
| }) |
There was a problem hiding this comment.
The search filtering logic is duplicated here and in the normalizedIndex useMemo. Extract this filtering logic into a reusable function to avoid code duplication and ensure consistency.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Code Review
This pull request introduces a well-implemented search functionality to the Independent Panel chat history. The changes demonstrate a strong focus on performance, accessibility, and user experience, incorporating features like debounced search, memoized filtering, keyboard shortcuts, and robust handling of asynchronous operations. The code is clean and follows modern React practices. I have identified a couple of opportunities to improve maintainability by reducing code duplication. Overall, this is an excellent contribution.
| const CP_TAB = 9 | ||
| const CP_LF = 10 | ||
| const CP_CR = 13 | ||
| const CP_PRINTABLE_MIN = 32 | ||
| const CP_DEL = 127 | ||
| const isAllowedCodePoint = (cp) => | ||
| cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) |
There was a problem hiding this comment.
These constants and the isAllowedCodePoint helper function are redefined on every render because they are inside handleSearchChange. For better performance and code organization, they should be defined once, outside the App component.
| const q = normalizeForSearch(debouncedQuery).trim() | ||
| if (q) { | ||
| const SEP = '\n—\n' | ||
| const nextFiltered = updatedSessions.find((s) => { | ||
| if (!s || !s.sessionId) return false | ||
| const nameNorm = normalizeForSearch(s?.sessionName) | ||
| let bodyNorm = '' | ||
| if (Array.isArray(s?.conversationRecords)) { | ||
| bodyNorm = s.conversationRecords | ||
| .map( | ||
| (r) => | ||
| `${normalizeForSearch(r?.question)} ${normalizeForSearch( | ||
| r?.answer, | ||
| )}`, | ||
| ) | ||
| .join(SEP) | ||
| } | ||
| return nameNorm.includes(q) || bodyNorm.includes(q) | ||
| }) |
There was a problem hiding this comment.
This logic to find the next filtered session after deletion is a duplicate of the logic used to create the normalizedIndex (lines 219-234). This duplication can lead to maintenance issues. Consider extracting this logic into a shared helper function to keep it DRY (Don't Repeat Yourself). The SEP constant is also duplicated.
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| onClick={async (e) => { | ||
| if (confirming) return | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| setConfirming(true) | ||
| try { | ||
| await onConfirm() | ||
| setWaitConfirm(false) | ||
| } catch (err) { | ||
| // Keep confirmation visible to allow retry; optionally log | ||
| // eslint-disable-next-line no-console | ||
| console.error(err) | ||
| } finally { | ||
| setConfirming(false) | ||
| } |
There was a problem hiding this comment.
[P2] Guard DeleteButton state updates after async removal
The confirm handler awaits onConfirm() and then calls setWaitConfirm(false) and setConfirming(false). When a conversation is deleted, the DeleteButton unmounts immediately, so these state setters execute after unmount and React logs "Can't perform a React state update on an unmounted component" warnings. This is a memory‑leak pattern and will happen whenever the delete action removes the row (the common case). Consider tracking a mounted flag or moving the async work to the parent instead of updating local state after unmount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/IndependentPanel/App.jsx (1)
67-77: Guard against empty sessions before indexing sessions[0]Directly accessing sessions[0] can throw when the list is empty. Add a defensive check before reading conversationRecords or passing sessionId.
- const sessions = await getSessions() - if ( - urlFrom !== 'store' && - sessions[0].conversationRecords && - sessions[0].conversationRecords.length > 0 - ) { + const sessions = await getSessions() + if (!Array.isArray(sessions) || sessions.length === 0) { + setSessions([]) + setSessionId(null) + setCurrentSession(null) + return + } + if ( + urlFrom !== 'store' && + Array.isArray(sessions[0].conversationRecords) && + sessions[0].conversationRecords.length > 0 + ) { await createNewChat() } else { setSessions(sessions) - await setSessionIdSafe(sessions[0].sessionId) + await setSessionIdSafe(sessions[0].sessionId) }
♻️ Duplicate comments (8)
src/_locales/en/main.json (1)
100-102: LGTM: baseline keys addedStrings look correct and consistent with other locales.
Quick parity check across all locales and code usage:
#!/bin/bash set -euo pipefail echo "== Locale parity ==" fd -a main.json src/_locales | while read -r f; do printf "---- %s\n" "$f" for k in 'Search' 'Search conversations...' 'No conversations found'; do grep -nE "^\s*\"$k\"\s*:" "$f" || echo "MISSING: $k in $f" done done echo echo "== Code usage ==" rg -nP --type js --type jsx --type ts --type tsx -C1 \ "(?:t|i18n\.t|browser\.i18n\.getMessage)\(\s*['\"](Search|Search conversations\.\.\.|No conversations found)['\"]\s*\)" src || truesrc/_locales/fr/main.json (1)
100-102: FR placeholder resolved as previously suggested.“Rechercher des conversations...” matches prior feedback and context.
src/pages/IndependentPanel/styles.scss (1)
126-133: Focus styles restored—accessibility preserved.Using :focus-visible with an outline is the right approach.
src/components/DeleteButton/index.jsx (1)
66-68: Don’t use aria-hidden on a focusable control; remove from tab order when hidden.This is an a11y violation. Make it unfocusable instead of aria-hidden while visible.
- role="button" - tabIndex={0} + role="button" + tabIndex={waitConfirm ? -1 : 0} aria-label={text} - aria-hidden={waitConfirm ? 'true' : undefined} - style={waitConfirm ? { visibility: 'hidden' } : {}} + style={waitConfirm ? { visibility: 'hidden', pointerEvents: 'none' } : {}}src/pages/IndependentPanel/App.jsx (4)
188-200: Don’t hijack Ctrl/Cmd+F while user is typing; fall back to native findThe global handler always prevents default for Ctrl/Cmd+F, even in inputs/textareas. Limit override to when not typing in a field; allow native find otherwise.
- // Always route find shortcut to panel search (and auto-expand temporarily) - if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { - e.preventDefault() - focusSearch() - return - } + // Only override find shortcut when not typing in a field + if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && e.key.toLowerCase() === 'f') { + if (!isTypingField) { + e.preventDefault() + focusSearch() + } + return + }
92-100: Fix potential race and stale updates in current session loaderIf sessionId changes rapidly, responses can resolve out-of-order; also calling getSession(null) is risky. Add cancellation and guard invalid IDs; include sessions in deps to refresh on list changes.
- useEffect(() => { - // eslint-disable-next-line - ;(async () => { - if (sessions.length > 0) { - setCurrentSession((await getSession(sessionId)).session) - } - })() - }, [sessionId]) + useEffect(() => { + let cancelled = false + ;(async () => { + if (!sessionId || sessions.length === 0) { + if (!cancelled) setCurrentSession(null) + return + } + try { + const { session } = await getSession(sessionId) + if (!cancelled) setCurrentSession(session ?? null) + } catch { + if (!cancelled) setCurrentSession(null) + } + })() + return () => { + cancelled = true + } + }, [sessionId, sessions])
125-136: Clear search filter when history is resetWith an active filter, pressing “Clear conversations” leaves the list empty even though a default session exists. Reset the query unconditionally after resetSessions().
const clearConversations = async () => { const next = await resetSessions() setSessions(next) + setSearchQuery('') + setDebouncedQuery('') if (next && next.length > 0) { await setSessionIdSafe(next[0].sessionId) } else { - stopCurrentPort() - setSessionId(null) - setCurrentSession(null) - setSearchQuery('') - setDebouncedQuery('') + stopCurrentPort() + setSessionId(null) + setCurrentSession(null) } }
291-364: Fix invalid nested interactive controls (button inside button)DeleteButton renders a button; nesting it inside the session button is invalid HTML and harms a11y/keyboard behavior. Render them as siblings in the row.
- {filteredSessions.map((session) => ( - <div role="listitem" key={session.sessionId}> - <button + {filteredSessions.map((session) => ( + <div role="listitem" key={session.sessionId} className="chat-list-row"> + <button type="button" aria-current={sessionId === session.sessionId ? 'page' : undefined} className={`normal-button chat-list-item ${ sessionId === session.sessionId ? 'active' : '' }`} onClick={() => { setSessionIdSafe(session.sessionId) }} > - <span className="chat-list-title">{session.sessionName}</span> - <span - className="gpt-util-group" - onClick={(e) => { - e.stopPropagation() - }} - > - <DeleteButton - size={14} - text={t('Delete Conversation')} - onConfirm={async () => { - const deletedId = session.sessionId - const updatedSessions = await deleteSession(deletedId) - if (!isMountedRef.current) return - setSessions(updatedSessions) - if (!updatedSessions || updatedSessions.length === 0) { - stopCurrentPort() - setSessionId(null) - setCurrentSession(null) - return - } - // Only change active session if the deleted one was active - if (sessionId === deletedId) { - // When searching, prefer the next visible item in the filtered result - const q = normalizeForSearch(debouncedQuery).trim() - if (q) { - const SEP = '\n—\n' - const nextFiltered = updatedSessions.find((s) => { - if (!s || !s.sessionId) return false - const nameNorm = normalizeForSearch(s?.sessionName) - let bodyNorm = '' - if (Array.isArray(s?.conversationRecords)) { - bodyNorm = s.conversationRecords - .map( - (r) => - `${normalizeForSearch(r?.question)} ${normalizeForSearch( - r?.answer, - )}`, - ) - .join(SEP) - } - return nameNorm.includes(q) || bodyNorm.includes(q) - }) - if (nextFiltered) { - await setSessionIdSafe(nextFiltered.sessionId) - return - } - } - // Fallback to first valid item in full list - const next = updatedSessions.find((s) => s && s.sessionId) - if (next) { - await setSessionIdSafe(next.sessionId) - } else { - setSessionId(null) - setCurrentSession(null) - } - } - }} - /> - </span> - </button> - </div> - ))} + <span className="chat-list-title">{session.sessionName}</span> + </button> + <span className="gpt-util-group"> + <DeleteButton + size={14} + text={t('Delete Conversation')} + onConfirm={async () => { + const deletedId = session.sessionId + const updatedSessions = await deleteSession(deletedId) + if (!isMountedRef.current) return + setSessions(updatedSessions) + if (!updatedSessions || updatedSessions.length === 0) { + stopCurrentPort() + setSessionId(null) + setCurrentSession(null) + return + } + // Only change active session if the deleted one was active + if (sessionId === deletedId) { + const q = normalizeForSearch(debouncedQuery).trim() + if (q) { + const SEP = '\n—\n' + const nextFiltered = updatedSessions.find((s) => { + if (!s || !s.sessionId) return false + const nameNorm = normalizeForSearch(s?.sessionName) + let bodyNorm = '' + if (Array.isArray(s?.conversationRecords)) { + bodyNorm = s.conversationRecords + .map( + (r) => + `${normalizeForSearch(r?.question)} ${normalizeForSearch( + r?.answer, + )}`, + ) + .join(SEP) + } + return nameNorm.includes(q) || bodyNorm.includes(q) + }) + if (nextFiltered) { + await setSessionIdSafe(nextFiltered.sessionId) + return + } + } + const next = updatedSessions.find((s) => s && s.sessionId) + if (next) await setSessionIdSafe(next.sessionId) + else { + setSessionId(null) + setCurrentSession(null) + } + } + }} + /> + </span> + </div> + ))}
🧹 Nitpick comments (16)
src/_locales/it/main.json (1)
100-102: Tweak empty-state phrasing for natural Italian“Nessuna conversazione trovata” reads more idiomatic than “corrispondente”.
- "No conversations found": "Nessuna conversazione corrispondente", + "No conversations found": "Nessuna conversazione trovata",src/_locales/de/main.json (1)
100-102: Unify terminology: Gespräche vs. KonversationenThe placeholder uses “Gesprächen”, but the empty-state says “Konversationen”. Prefer “Gespräche” consistently.
- "No conversations found": "Keine passenden Konversationen gefunden", + "No conversations found": "Keine passenden Gespräche gefunden",src/config/index.mjs (1)
478-478: Confirm product intent for default collapsed panel
IndependentPanel now defaults to collapsed (independentPanelCollapsed: true), and App.jsx correctly reads this flag viagetUserConfigand persists it viasetUserConfigwith boolean coercion (!!config.independentPanelCollapsed), preventing tri-state issues. If this matches product intent, add a short doc comment aboveindependentPanelCollapsedinsrc/config/index.mjsto explain its default behavior.src/_locales/zh-hant/main.json (1)
100-102: Traditional Chinese consistency: 記錄 vs 紀錄Elsewhere the file uses “記錄”; align these two lines to match.
- "Search conversations...": "搜尋對話紀錄...", - "No conversations found": "沒有符合的對話紀錄", + "Search conversations...": "搜尋對話記錄...", + "No conversations found": "沒有符合的對話記錄",src/_locales/pt/main.json (1)
100-102: More idiomatic PT-BR empty-state“Nenhuma conversa encontrada” is the common phrasing in Portuguese UIs.
- "No conversations found": "Nenhuma conversa correspondente", + "No conversations found": "Nenhuma conversa encontrada",src/_locales/zh-hans/main.json (1)
100-102: Unify term: 对话 vs 聊天记录Use “对话” consistently with surrounding strings.
- "Search conversations...": "搜索对话内容...", - "No conversations found": "未找到匹配的聊天记录", + "Search conversations...": "搜索对话...", + "No conversations found": "未找到匹配的对话",src/_locales/es/main.json (1)
100-102: Minor phrasing tweak for placeholder (optional).“Buscar conversaciones...” reads a bit cleaner than “Buscar en las conversaciones...”. Consider:
- "Search conversations...": "Buscar en las conversaciones...", + "Search conversations...": "Buscar conversaciones...",src/pages/IndependentPanel/styles.scss (3)
82-85: Collapsed sidebar UX: consider hiding search or using an icon trigger.At 60px width, a visible input is largely unusable. Options:
- Hide when collapsed and show on hover/expand, or
- Show a search icon that expands on focus/hover.
Example (hide when collapsed):
-.chat-sidebar.collapsed .search-container { - display: block; -} +.chat-sidebar.collapsed .search-container { display: none; }
101-105: Apply padding to search container (regression vs earlier suggestion).Padding helps spacing and hit area.
- .search-container { - padding: 0; + .search-container { + padding: 10px; background-color: inherit; margin-top: 15px; }
206-211: Prevent hover styles on disabled buttons (optional).Disabled controls can still match :hover. Add a disabled style and guard hover:
.normal-button:disabled { opacity: 0.6; cursor: not-allowed; } .normal-button:not(:disabled):hover, .normal-button:not(:disabled).active { background-color: var(--active-color); }Also applies to: 218-221
src/components/DeleteButton/index.jsx (2)
16-16: Guard against rapid double-activation with a ref.State alone may allow two quick clicks before re-render. Add a ref for reentrancy.
- const [confirming, setConfirming] = useState(false) + const [confirming, setConfirming] = useState(false) + const confirmingRef = useRef(false)And use confirmingRef in the handler (see next comment).
68-74: Optional: add Escape to cancel.Improve keyboard UX:
onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault() e.stopPropagation() setWaitConfirm(true) + } else if (e.key === 'Escape') { + e.preventDefault() + e.stopPropagation() + if (!confirmingRef.current) setWaitConfirm(false) } }}src/pages/IndependentPanel/App.jsx (4)
139-155: Overly complex input sanitization for a search fieldThis level of codepoint filtering is hard to maintain and may block valid characters (e.g., emojis). Prefer a simpler, fast regex or just length limiting.
- const handleSearchChange = (e) => { - const raw = e?.target?.value ?? '' - // Keep Tab/LF/CR, remove other control chars (incl. DEL), then truncate by code points - const CP_TAB = 9 - const CP_LF = 10 - const CP_CR = 13 - const CP_PRINTABLE_MIN = 32 - const CP_DEL = 127 - const isAllowedCodePoint = (cp) => - cp === CP_TAB || cp === CP_LF || cp === CP_CR || (cp >= CP_PRINTABLE_MIN && cp !== CP_DEL) - const sanitizedArr = Array.from(raw).filter((ch) => { - const cp = ch.codePointAt(0) - return cp != null && isAllowedCodePoint(cp) - }) - const limited = sanitizedArr.slice(0, 500).join('') - setSearchQuery(limited) - } + const handleSearchChange = (e) => { + const raw = e?.target?.value ?? '' + // Strip control chars; keep everything else (incl. emoji), then limit length + const cleaned = raw.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '').slice(0, 500) + setSearchQuery(cleaned) + }
205-217: Hoist pure helpers out of the component to avoid re-creation every rendertoSafeString and normalizeForSearch don’t depend on component state. Move them to module scope.
- // Utility function to safely convert any value to a string - const toSafeString = (value) => - typeof value === 'string' ? value : value == null ? '' : String(value) - - // Normalization utility for search - const normalizeForSearch = (value) => - toSafeString(value) - .toLowerCase() - .normalize('NFD') - .replace(/[\u0300-\u036f]/g, '') - .replace(/\s+/g, ' ') - .trim() +// move above function App() +// const toSafeString = (value) => (typeof value === 'string' ? value : value == null ? '' : String(value)) +// const normalizeForSearch = (value) => +// toSafeString(value).toLowerCase().normalize('NFD').replace(/[\u0300-\u036f]/g, '').replace(/\s+/g, ' ').trim()If you’d like, I can provide a full-file diff with the helpers hoisted.
326-345: Avoid reimplementing normalization logic in delete handlerThis duplicates the normalized search built for normalizedIndex. Reuse normalizedIndex or extract a helper to get name/body normalized once.
- if (q) { - const SEP = '\n—\n' - const nextFiltered = updatedSessions.find((s) => { - if (!s || !s.sessionId) return false - const nameNorm = normalizeForSearch(s?.sessionName) - let bodyNorm = '' - if (Array.isArray(s?.conversationRecords)) { - bodyNorm = s.conversationRecords - .map( - (r) => - `${normalizeForSearch(r?.question)} ${normalizeForSearch( - r?.answer, - )}`, - ) - .join(SEP) - } - return nameNorm.includes(q) || bodyNorm.includes(q) - }) + if (q) { + const nextFiltered = updatedSessions.find((s) => { + if (!s || !s.sessionId) return false + const { nameNorm, bodyNorm } = (() => { + const idx = normalizedIndex.find((i) => i.session.sessionId === s.sessionId) + return idx ?? { nameNorm: normalizeForSearch(s?.sessionName), bodyNorm: '' } + })() + return nameNorm.includes(q) || bodyNorm.includes(q) + })
157-161: Alternative: useDeferredValue instead of manual debounceReact 18’s useDeferredValue can replace debouncedQuery state/effect for simpler code. Optional, but reduces timers and state churn.
-import { useEffect, useRef, useState, useMemo } from 'react' +import { useEffect, useRef, useState, useMemo, useDeferredValue } from 'react' @@ - const [debouncedQuery, setDebouncedQuery] = useState('') + const deferredQuery = useDeferredValue(searchQuery) @@ - // Debounce search input for performance - useEffect(() => { - const id = setTimeout(() => setDebouncedQuery(searchQuery), 200) - return () => clearTimeout(id) - }, [searchQuery]) + // (no effect needed) @@ - const q = normalizeForSearch(debouncedQuery).trim() + const q = normalizeForSearch(deferredQuery).trim() - }, [normalizedIndex, debouncedQuery]) + }, [normalizedIndex, deferredQuery])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/_locales/de/main.json(1 hunks)src/_locales/en/main.json(1 hunks)src/_locales/es/main.json(1 hunks)src/_locales/fr/main.json(1 hunks)src/_locales/in/main.json(1 hunks)src/_locales/it/main.json(1 hunks)src/_locales/ja/main.json(1 hunks)src/_locales/ko/main.json(1 hunks)src/_locales/pt/main.json(1 hunks)src/_locales/ru/main.json(1 hunks)src/_locales/tr/main.json(1 hunks)src/_locales/zh-hans/main.json(1 hunks)src/_locales/zh-hant/main.json(1 hunks)src/components/DeleteButton/index.jsx(2 hunks)src/config/index.mjs(1 hunks)src/pages/IndependentPanel/App.jsx(7 hunks)src/pages/IndependentPanel/styles.scss(5 hunks)
🔇 Additional comments (10)
src/_locales/ko/main.json (1)
100-102: LGTM: clear and natural KR strings“대화에서 검색...” and “일치하는 대화가 없습니다” read well.
src/_locales/ja/main.json (1)
100-102: JP translations look correct and consistent.Wording and ellipsis usage are natural for the search UI.
src/_locales/in/main.json (1)
100-102: ID translations LGTM.Natural phrasing; matches the feature semantics.
src/_locales/tr/main.json (1)
100-102: TR translations LGTM.Terminology is consistent with existing strings.
src/_locales/ru/main.json (2)
100-102: RU translations LGTM.Clear and idiomatic; no casing/grammar issues.
100-102: Key coverage verified across all locales
All_locales/*/main.jsonfiles include the three new keys—Search,Search conversations…, andNo conversations found—with correct strings and no typos or missing entries.src/pages/IndependentPanel/styles.scss (4)
4-19: Theme tokens added: good coverage.Defining --font-muted-color and --theme-secondary-color for auto/dark/light improves contrast and theming.
Also applies to: 24-30, 34-40
107-119: Search input styling looks solid.Full-width, box-sizing, and theme-aware background are correct.
121-124: Placeholder color fallback is sensible.Works across themes; no action needed.
146-150: No-results style OK.Muted text and padding are adequate.
| onClick={async (e) => { | ||
| if (confirming) return | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| setConfirming(true) | ||
| try { | ||
| await onConfirm() | ||
| setWaitConfirm(false) | ||
| } catch (err) { | ||
| // Keep confirmation visible to allow retry; optionally log | ||
| // eslint-disable-next-line no-console | ||
| console.error(err) | ||
| } finally { | ||
| setConfirming(false) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid setState on unmounted after async confirm.
Wrap with a mounted ref and use confirmingRef to block reentry.
- onClick={async (e) => {
- if (confirming) return
+ onClick={async (e) => {
+ if (confirmingRef.current) return
e.preventDefault()
e.stopPropagation()
- setConfirming(true)
+ confirmingRef.current = true
+ setConfirming(true)
try {
await onConfirm()
- setWaitConfirm(false)
+ if (mountedRef.current) setWaitConfirm(false)
} catch (err) {
console.error(err)
} finally {
- setConfirming(false)
+ confirmingRef.current = false
+ if (mountedRef.current) setConfirming(false)
}
}}Add at top of component:
const mountedRef = useRef(false)
useEffect(() => { mountedRef.current = true; return () => { mountedRef.current = false } }, [])🤖 Prompt for AI Agents
In src/components/DeleteButton/index.jsx around lines 41 to 55, the async
onClick handler calls setState after an awaited onConfirm which can update state
on an unmounted component and allows reentry; add a mountedRef via useRef and a
useEffect that sets mountedRef.current = true and clears it on unmount, and also
add a confirmingRef useRef to mirror/guard the confirming boolean to prevent
reentry; in the onClick handler check confirmingRef.current at start, set
confirmingRef.current = true when starting, await onConfirm, then before calling
setWaitConfirm or setConfirming check mountedRef.current and only call the state
setters if still mounted, and finally set confirmingRef.current = false in the
finally block (only updating state if mounted).
| <div className={`chat-sidebar ${collapsed && !forceExpand ? 'collapsed' : ''}`}> | ||
| <div className="chat-sidebar-button-group"> | ||
| <button className="normal-button" onClick={toggleSidebar}> | ||
| <button | ||
| type="button" | ||
| className="normal-button" | ||
| aria-expanded={!collapsed} | ||
| onClick={toggleSidebar} | ||
| > | ||
| {collapsed ? t('Pin') : t('Unpin')} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
aria-expanded must reflect temporary expansion (forceExpand)
When forceExpand is true and collapsed is true, the sidebar is visually expanded but aria-expanded reports false. Compute accessibility state from both flags.
- <div className={`chat-sidebar ${collapsed && !forceExpand ? 'collapsed' : ''}`}>
+ <div className={`chat-sidebar ${collapsed && !forceExpand ? 'collapsed' : ''}`}>
<div className="chat-sidebar-button-group">
<button
type="button"
className="normal-button"
- aria-expanded={!collapsed}
+ aria-expanded={!collapsed || forceExpand}
onClick={toggleSidebar}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={`chat-sidebar ${collapsed && !forceExpand ? 'collapsed' : ''}`}> | |
| <div className="chat-sidebar-button-group"> | |
| <button className="normal-button" onClick={toggleSidebar}> | |
| <button | |
| type="button" | |
| className="normal-button" | |
| aria-expanded={!collapsed} | |
| onClick={toggleSidebar} | |
| > | |
| {collapsed ? t('Pin') : t('Unpin')} | |
| <div className={`chat-sidebar ${collapsed && !forceExpand ? 'collapsed' : ''}`}> | |
| <div className="chat-sidebar-button-group"> | |
| <button | |
| type="button" | |
| className="normal-button" | |
| aria-expanded={!collapsed || forceExpand} | |
| onClick={toggleSidebar} | |
| > | |
| {collapsed ? t('Pin') : t('Unpin')} |
🤖 Prompt for AI Agents
In src/pages/IndependentPanel/App.jsx around lines 248 to 256, the button's
aria-expanded currently uses only the collapsed flag so when forceExpand is true
but collapsed is true the visual state is expanded while aria-expanded remains
false; update the aria-expanded calculation to reflect either a non-collapsed
state or a forced expansion (e.g., aria-expanded should be true when !collapsed
|| forceExpand) so the accessibility state matches the visual state.
Implementation Status
✅ Functionality: Search works correctly across all conversation data
✅ Code Quality: All React best practices implemented, passes ESLint
✅ Performance: Memoized search filtering for optimal performance
✅ Accessibility: Complete ARIA labels and focus indicators
✅ Build Status: Successfully builds without errors
✅ Translations: Complete multi-language support for 13 languages
✅ CI Status: All linting and build checks pass
✅ Review Feedback: All Qodo Merge and Copilot AI suggestions implemented
✅ Event Handling: Proper click propagation prevention in delete buttons
The search functionality is fully functional and production-ready. All review feedback has been addressed and implemented.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
PR Type
Enhancement
Description
Add real-time search functionality to Independent Panel chat history
Implement comprehensive search across session names, questions, and answers
Add multi-language support for search interface (6 languages)
Style search input to match existing theme system
Diagram Walkthrough
File Walkthrough
App.jsx
Implement search state and filtering logicsrc/pages/IndependentPanel/App.jsx
searchQuerystate for managing search inputfilteredSessionsfunction with comprehensive search logicstyles.scss
Add search input styling and themingsrc/pages/IndependentPanel/styles.scss
.search-containerstyling for search input wrapper.search-inputstyles with theme variable integrationmain.json
Add English search translationssrc/_locales/en/main.json
main.json
Add Spanish search translationssrc/_locales/es/main.json
main.json
Add French search translationssrc/_locales/fr/main.json
main.json
Add Japanese search translationssrc/_locales/ja/main.json
main.json
Add Simplified Chinese search translationssrc/_locales/zh-hans/main.json
main.json
Add Traditional Chinese search translationssrc/_locales/zh-hant/main.json
Summary by CodeRabbit
New Features
Enhancements
Accessibility
Localization
UI/Style