Conversation
lethemanh
requested review from
JF-Cozy,
doubleface,
rezk2ll and
zatteo
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a complete assistant feature in packages/cozy-search: adds runtime and UI dependencies, numerous React components (AssistantContainer, AssistantAvatar, Sidebar, ConversationList and items, ConversationComposer, ConversationBar, Conversation, message components, Twake knowledge panel, AssistantView), runtime wiring (CozyAssistantRuntimeProvider, CozyRealtimeChatAdapter, StreamBridge), hooks and helpers (useConversation, buildChatConversationsQuery, formatConversationDate, grouping helper), action factories (share, rename, delete), styles, TypeScript declarations, locale additions, expands AssistantProvider context with new state/setters, and exposes new public exports from the package index. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
To formalize what we said together with @Letheman : we plan to end with a PR with only assistant-ui lib migration:
|
lethemanh
force-pushed
the
new-assistant-ui
branch
4 times, most recently
from
0494154 to
f3a5d8b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cozy-search/src/components/Assistant/AssistantSelection.jsx (1)
104-104:⚠️ Potential issue | 🟡 MinorHardcoded English string — should use i18n.
"Create Assistant"is a hardcoded English string. Given this PR adds localization support across multiple languages, this should use the translation function for consistency.
🤖 Fix all issues with AI agents
In `@packages/cozy-search/package.json`:
- Line 24: Package.json currently lists the scaffolding CLI "shadcn" as a
regular dependency; move it to devDependencies (or remove it) so it isn't
included in production bundles. Open the packages/cozy-search package.json,
locate the "shadcn" entry and remove it from the top-level "dependencies"
object, then add the same version string under "devDependencies" (or delete it
entirely if scaffolding is complete); ensure package.json remains valid JSON and
run the package manager install to update lockfiles.
- Around line 13-14: Update the package.json peerDependencies so React is
required at version 18+: change the "react" peerDependency from its current
">=16.12.0" (or similar) to ">=18.0.0" in this package's package.json, and make
the same change in any other package.json files that list the same peers (the
entries near the `@assistant-ui/react` and `@assistant-ui/react-markdown`
dependencies). Ensure the peerDependencies object reflects "react": ">=18.0.0"
to match `@assistant-ui/react` v0.12.x peer requirements.
In `@packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts`:
- Around line 70-71: Remove any direct logging of user-authored content in
CozyRealtimeChatAdapter: stop printing the full messages array and the user
query to the console. Replace those console calls in the method that references
the messages variable and the user query with either sanitized/redacted
placeholders (e.g., log message counts, types, or masked text) or remove them
entirely; use non-PII context such as "No user message found" or "Received query
(redacted)" and, if needed for debugging, log only metadata like messages.length
or message IDs. Ensure the console.error/console.log calls in
CozyRealtimeChatAdapter reference only non-PII values and remove any direct
inclusion of the messages or raw query variables.
- Around line 80-86: The two development console.log calls in
CozyRealtimeChatAdapter (the logs around sending the request and after POST)
must be removed or replaced with a gated debug/logger call to avoid leaking user
query data; update the method in CozyRealtimeChatAdapter that sends the request
(the code invoking client.stackClient.fetchJSON with
`/ai/chat/conversations/${conversationId}` and userQuery) to either remove both
console.log lines or route them through the adapter's logger/debug flag (e.g.,
this.logger.debug or a DEBUG environment check) and ensure the userQuery content
is never logged in plaintext.
- Around line 106-115: The StreamBridge map entry for this conversationId is
never removed on normal completion; call cleanup() for the StreamBridge entry
when the for-await loop finishes successfully (i.e. in the branch where
!wasAborted and before/after yielding the final complete event) so the
StreamBridge map doesn't leak. Locate the completion block in
CozyRealtimeChatAdapter where finalText is computed (uses sanitizeChatContent)
and a complete/status stop event is yielded, and invoke cleanup() (the same
function used in error/abort paths) to remove the map entry and call complete()
safely.
In `@packages/cozy-search/src/components/Assistant/AssistantAvatar.jsx`:
- Line 12: The component currently returns undefined when assistant is falsy
(line showing "if (!assistant) return"), which breaks React 16; update the
AssistantAvatar component so that when assistant is not present it returns null
instead of undefined (replace the early return with an explicit null return) to
avoid the "Nothing was returned from render" runtime error.
In `@packages/cozy-search/src/components/Conversations/ConversationBar.jsx`:
- Around line 51-55: The handleKeyDown currently ignores and swallows the
KeyboardEvent — it calls onKeyDown() with no args, causing the parent to be
unable to detect Enter and causing the handler to fire on every keypress; change
handleKeyDown to accept the event (e.g., e or event), early-return if isEmpty,
then call onKeyDown(event) so the parent can inspect event.key (or event.code)
and only act on Enter; update any place that assigns handleKeyDown (e.g., the
input's onKeyDown) to pass the event through as well.
- Around line 40-48: The handler handleSend currently mutates the DOM input
value via inputRef.current.value = '' while the input is controlled by a value
prop, causing a controlled/uncontrolled conflict; remove the direct DOM mutation
and instead let the parent clear the controlled value—either call an existing
prop (onSend) after which the parent should reset value, or introduce/ call a
dedicated callback (e.g., onClear or onValueChange('')) from handleSend so the
parent updates the state; you may keep the height reset
(inputRef.current.style.height = 'auto') but do not set inputRef.current.value
directly in ConversationBar.jsx.
In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx`:
- Around line 30-33: The toggleMenu handler currently assumes an event and calls
e.stopPropagation(), which causes a TypeError when ActionsMenu invokes onClose
without an event; update toggleMenu (and the similar handler in
ConversationListItemWider) to accept an optional event and guard calls to
e.stopPropagation()/e.preventDefault() (e.g., if (e && e.stopPropagation) ...)
or split into two handlers: one that toggles state (calls
setIsMenuOpen(!isMenuOpen)) for programmatic/onClose use and a separate onClick
handler that receives the event and calls e.stopPropagation()/e.preventDefault()
before toggling; ensure ActionsMenu's onClose uses the event-safe toggling
handler.
In
`@packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx`:
- Around line 33-36: The toggleMenu function currently calls e.preventDefault()
unconditionally which will throw if invoked without an event (e.g., from onClose
callbacks); update toggleMenu (the function that toggles isMenuOpen via
setIsMenuOpen) to accept an optional event parameter and guard the
preventDefault call (check that e exists before calling preventDefault, e.g.,
use a conditional or optional chaining) so it safely toggles the menu when
called with or without a DOM event.
In `@packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx`:
- Around line 136-194: The created and updated handlers in the useRealtime call
are identical; extract their common logic into a single shared function (e.g.,
handleConversationChange) that accepts a Conversation, checks res._id ===
conversationId and res.messages, computes newIds, finds lastAssistantMsg,
updates cancelledMessageIdsRef/currentStreamingMessageIdRef as before, and sets
messagesIdRef.current = newIds; then pass that shared function for both the
created and updated keys in the useRealtime payload so created and updated both
call the same handler (keeping references to conversationId, messagesIdRef,
currentStreamingMessageIdRef, and cancelledMessageIdsRef).
In `@packages/cozy-search/src/components/queries.js`:
- Around line 67-79: The query returned by buildChatConversationsQuery is
missing indexFields so Mango may do a full collection scan; update the
definition function for buildChatConversationsQuery (the Q(...) chain for
CHAT_CONVERSATIONS_DOCTYPE) to include
.indexFields(['cozyMetadata.doctypeVersion','cozyMetadata.updatedAt']) (place it
before .sortBy) so the filter and sort can use an index and avoid collection
scans.
In `@packages/cozy-search/src/components/Sidebar/index.jsx`:
- Around line 87-89: The JSX string "Recent chats" is hard-coded; wrap it with
the i18n translator (use t('recent_chats') or existing key style) in the Sidebar
component so it matches other strings (e.g., the nearby t(...) usage), and add
the corresponding "recent_chats" key and translations to all locale files
(en.json, fr.json, ru.json, vi.json). Ensure the key name follows project
conventions and update any imports/usage if necessary so the component uses the
translated value.
In `@packages/cozy-search/src/components/TwakeKnowledges/ChatKnowledge.jsx`:
- Around line 48-54: The ListItem's onClick and the Checkbox's onChange are both
calling onToggleItems, causing double-toggle when clicking the checkbox; remove
the Checkbox onChange handler so only the ListItem onClick triggers
onToggleItems (keep Checkbox checked prop using selectedItems.includes(chat.id)
and leave ListItem's onClick as the single toggle entrypoint referencing
chat.id).
In `@packages/cozy-search/src/components/TwakeKnowledges/DriveKnowledge.jsx`:
- Around line 111-117: The component DriveKnowledge.jsx currently contains
hard-coded placeholder arrays myDriveItems and sharedItems; replace these with
real data by either accepting item lists as props (e.g., add props like
myDriveItems and sharedItems to the component signature and use them instead of
the hard-coded arrays) or implement a data fetch inside the component (call the
backend/API and populate state before rendering), and wire those lists into the
existing props usage (selectedItems, onToggleItems, onClearItems); if this is
temporary scaffolding, add a clear TODO comment next to myDriveItems/sharedItems
indicating they are mocks and must be replaced with real data before production.
In `@packages/cozy-search/src/hooks/useConversation.jsx`:
- Around line 11-20: The goToConversation function currently pushes both
'assistant' and conversationId when the path ends with '/assistant', causing a
duplicate segment; change the branch that handles assistantIndex !== -1 so that
if parts.length > assistantIndex + 1 you replace parts[assistantIndex + 1] with
conversationId, but if parts.length === assistantIndex + 1 you only append the
conversationId (not another 'assistant') — update the logic around
assistantIndex and parts manipulation (in goToConversation) to either set the
next segment or push a single conversationId accordingly.
🟡 Minor comments (14)
packages/cozy-search/src/locales/ru.json-51-51 (1)
51-51:⚠️ Potential issue | 🟡 MinorRussian pluralization requires 3 forms, but only 2 are provided.
Russian uses three plural forms (one / few / many). The
items_selectedkey has only two forms. Compare withsourceson line 14, which correctly provides three. The current form will produce incorrect text for counts like 1 or 5+.🐛 Suggested fix
- "items_selected": "Выбрано %{smart_count} элемента |||| Выбрано %{smart_count} элементов", + "items_selected": "Выбран %{smart_count} элемент |||| Выбрано %{smart_count} элемента |||| Выбрано %{smart_count} элементов",packages/cozy-search/src/actions/rename.jsx-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorFix Prettier formatting error — CI is failing.
There's an extra space on this line that Prettier flags. This is blocking the pipeline.
- action: () => { } + action: () => {}packages/cozy-search/src/components/TwakeKnowledges/DriveKnowledge.jsx-32-37 (1)
32-37:⚠️ Potential issue | 🟡 MinorSelect-all/clear counts diverge from the rendered list.
The "select all" checkbox on Lines 47-49 considers all
items, but the collapsed list only renderscustomItems(Lines 83-102), which filters out IDs'my-drive'and'shared-with-me'. Currently none of the hard-coded items carry those IDs, making the filter dead code. If items with those IDs are ever passed in, the checkbox will toggle items the user can't see.Either remove the
customItemsfilter and render all items, or make the checkbox logic operate oncustomItemstoo.packages/cozy-search/src/components/Search/SearchConversation.jsx-41-50 (1)
41-50:⚠️ Potential issue | 🟡 MinorFallback to
Date.now()silently places conversations withoutupdatedAtinto the "today" group.Line 43 uses
Date.now()as fallback whenconv.cozyMetadata?.updatedAtis missing. This means any conversation lacking metadata will always appear under "Recent," regardless of when it was actually created. Consider usingconv.cozyMetadata?.createdAtas a secondary fallback, or placing such conversations in the "older" group by default.packages/cozy-search/src/components/Conversations/ConversationListItem.jsx-81-85 (1)
81-85:⚠️ Potential issue | 🟡 MinorMissing optional chaining on
conversation.cozyMetadata— potential runtime crash.Line 82 accesses
conversation.cozyMetadata.updatedAtwithout optional chaining. IfcozyMetadataisundefined(e.g., for a newly created or malformed conversation), this will throw aTypeError.Proposed fix
{formatConversationDate( - conversation.cozyMetadata.updatedAt, + conversation.cozyMetadata?.updatedAt, t, lang )}packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx-134-136 (1)
134-136:⚠️ Potential issue | 🟡 MinorMissing
altattribute on<img>— accessibility issue.Line 135 renders
<img src={getIcon()} className="u-mr-1" />without analtattribute. This is an accessibility gap and may also trigger lint warnings. Add a descriptive alt or usealt=""if decorative.Proposed fix
- <img src={getIcon()} className="u-mr-1" /> + <img src={getIcon()} className="u-mr-1" alt="" />packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx-179-187 (1)
179-187:⚠️ Potential issue | 🟡 Minor
count={3}is hardcoded and disconnected from the actual item count.The
countprop should derive from the data:Proposed fix
<MailSection title={t('assistant.twake_knowledges.inbox')} icon={EmailIcon} - count={3} + count={inboxItems.length} items={inboxItems}packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx-75-75 (1)
75-75:⚠️ Potential issue | 🟡 Minor
{count && <span>...}will render0to the DOM ifcountis0.This is a common React gotcha with the
&&short-circuit pattern for numbers. Use an explicit boolean check:Proposed fix
- {count && <span className={styles['badge']}>{count}</span>} + {count > 0 && <span className={styles['badge']}>{count}</span>}packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx-179-181 (1)
179-181:⚠️ Potential issue | 🟡 MinorFix formatting to pass CI.
The build pipeline reports extra whitespace on lines 180-181. Apply the formatter to fix:
Proposed fix
label={ openedKnowledgePanel === 'drive' ? t('assistant.twake_knowledges.select_folders') - : openedKnowledgePanel === 'mail' - ? t('assistant.twake_knowledges.select_emails') - : t('assistant.twake_knowledges.select_messages') + : openedKnowledgePanel === 'mail' + ? t('assistant.twake_knowledges.select_emails') + : t('assistant.twake_knowledges.select_messages') }packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx-74-74 (1)
74-74:⚠️ Potential issue | 🟡 MinorCI failure: missing return types on functions.
The build pipeline requires explicit return types on these functions. Add the appropriate return type annotations to satisfy the linter.
- Line 74:
ConversationLoader—JSX.Element | null- Line 112:
CozyAssistantRuntimeProviderInner—JSX.Element- Line 261: cleanup callback —
void- Line 279:
CozyAssistantRuntimeProvider—JSX.Element | nullAlso applies to: 112-112, 261-261, 279-279
packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsx-49-51 (1)
49-51:⚠️ Potential issue | 🟡 MinorMissing
altattribute on chip icon images.The
<img>elements lackalttext, which hinders accessibility. Add an emptyalt=""if decorative, or a descriptive label.Proposed fix
- <img src={twakeKnowledge.icon} className="u-h-1 u-ml-half" /> + <img src={twakeKnowledge.icon} alt="" className="u-h-1 u-ml-half" />packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx-89-91 (1)
89-91:⚠️ Potential issue | 🟡 MinorPotential crash if
conversation.cozyMetadatais undefined.If a conversation document lacks
cozyMetadata, accessing.updatedAtwill throw. Add optional chaining.Proposed fix
- {formatConversationDate(conversation.cozyMetadata.updatedAt, t, lang)} + {formatConversationDate(conversation.cozyMetadata?.updatedAt, t, lang)}packages/cozy-search/src/actions/share.jsx-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorFix formatting: remove extra whitespace.
The linter flags a formatting violation here. Also, the
actionis a no-op — if the share feature isn't implemented yet, consider adding aTODOcomment to clarify intent.Proposed fix
- action: () => { } + // TODO: implement share action + action: () => {}packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsx-70-72 (1)
70-72:⚠️ Potential issue | 🟡 MinorMargin applied to the wrong last item when a chip is hidden by feature flag.
indexis relative to the filtered array, buttwakeKnowledges.lengthis the unfiltered array length. When the chat chip is hidden, the last visible chip still receivesu-mr-half, causing a trailing gap.Proposed fix
Store the filtered array first and use its length:
+ const filteredKnowledges = twakeKnowledges.filter(k => k.display) + return ( <div className="u-flex u-flex-row u-flex-wrap u-flex-items-center u-flex-justify-end"> - {twakeKnowledges - .filter(twakeKnowledge => twakeKnowledge.display) - .map((twakeKnowledge, index) => { + {filteredKnowledges.map((twakeKnowledge, index) => { ... className={cx('u-mr-0', styles['knowledge-chips-item'], { - 'u-mr-half': index < twakeKnowledges.length - 1 + 'u-mr-half': index < filteredKnowledges.length - 1 })}
🧹 Nitpick comments (27)
packages/cozy-search/src/types.d.ts (2)
24-28:useEventListenerhandler signature drops the event parameter.The handler is typed as
() => void, but event listener callbacks typically receive anEventargument. Consumers passing(e) => { e.preventDefault(); ... }would see a type mismatch.Proposed fix
declare module 'cozy-ui/transpiled/react/hooks/useEventListener' { // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const useEventListener: (element: any, event: string, handler: () => void) => void + const useEventListener: (element: any, event: string, handler: (event: Event) => void) => void export default useEventListener }
50-56:useI18nreturn type may be missingpolyglot.The actual
useI18nfromtwake-i18nalso returnspolyglot(used internally byuseExtendI18n). If any consumer in this package ever needs it, the type would be incomplete. Fine for now if onlytandlangare used.packages/cozy-search/src/components/helpers.js (1)
66-75: Hardcodedhour12: trueoverrides locale preference.Passing
langtotoLocaleTimeStringis good, buthour12: trueforces 12-hour (AM/PM) format for all locales, including those that conventionally use 24-hour time (e.g.,fr,ru,vi). Consider removinghour12to let the locale decide, or usehourCycleonly when explicitly needed.♻️ Suggested fix
const timeStr = date.toLocaleTimeString(lang, { hour: 'numeric', - minute: '2-digit', - hour12: true + minute: '2-digit' })packages/cozy-search/src/components/AssistantProvider.jsx (1)
133-168: Context value grows large — consider splitting in the future.The provider now exposes 18+ values. Every state change re-creates the context object and re-renders all consumers. This isn't a regression (it follows the existing pattern), but as the surface keeps growing, consider splitting into separate contexts (e.g., a
TwakeKnowledgeContext) to limit unnecessary re-renders.packages/cozy-search/src/components/Messages/UserMessage.jsx (1)
19-23: Inline component re-created on every render.The
Textcomponent passed toMessagePrimitive.Partsis an anonymous arrow function, so it's recreated each render. This can cause unnecessary unmount/remount cycles for the rendered elements. Extract it to a stable reference.♻️ Suggested fix
+const TextPart = ({ text }) => <Typography>{text}</Typography> + const UserMessage = () => { return ( <MessagePrimitive.Root className="u-mt-1"> <Card className={cx( 'u-bg-paleGrey u-bdrs-5 u-bdw-0 u-ml-auto u-p-half', styles['cozyThread-user-messages'] )} > <MessagePrimitive.Parts components={{ - Text: ({ text }) => <Typography>{text}</Typography> + Text: TextPart }} /> </Card> </MessagePrimitive.Root> ) }packages/cozy-search/src/components/adapters/StreamBridge.ts (1)
75-95: Missingreturn()method on the async iterator.When a
for await...ofloop exits early (viabreak,throw, orreturn), the runtime callsiterator.return(). Without it, the stream won't self-clean. Currently mitigated because the adapter callscleanup()explicitly, but addingreturn()would make the iterator protocol-complete and safer for other consumers.Suggested addition
[Symbol.asyncIterator]() { return this - } + }, + return(): Promise<IteratorResult<string>> { + isDone = true + return Promise.resolve({ value: undefined as unknown as string, done: true }) + } }packages/cozy-search/package.json (1)
17-18: Bothclassnamesandclsxare listed — they serve the same purpose.These two libraries are functionally equivalent for conditional class name joining. Consider consolidating on one (preferably
clsxsince it's smaller) to avoid redundancy.packages/cozy-search/src/components/styles.styl (1)
19-21: Consider using100dvhinstead of100vhfor mobile viewport accuracy.
100vhon mobile browsers includes the URL bar area, which can cause content to be clipped or scrollbars to appear unexpectedly.100dvh(dynamic viewport height) adjusts for the actual visible area. If this view targets mobile as well, consider:.assistantWrapper - height calc(100vh - 48px) + height calc(100dvh - 48px)If only desktop is targeted,
100vhis fine.packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx (1)
12-12: Simplify the import path — this file is already in theAssistant/directory.Since
AssistantSelectionItem.jsxis located incomponents/Assistant/, the import../Assistant/AssistantAvatarunnecessarily traverses up and back into the same directory. Use a direct sibling import instead.Proposed fix
-import AssistantAvatar from '../Assistant/AssistantAvatar' +import AssistantAvatar from './AssistantAvatar'packages/cozy-search/src/components/TwakeKnowledges/styles.styl (2)
30-35: WebKit-only scrollbar styling won't apply in Firefox.The
::-webkit-scrollbarrules are ignored by Firefox and other non-Chromium browsers. If consistent scrollbar styling matters, consider addingscrollbar-width: thinandscrollbar-colorfor Firefox.Proposed addition for Firefox support
.source-panel-content flex 1 overflow-y auto padding 0 8px + scrollbar-width thin + scrollbar-color var(--dividerColor) transparent &::-webkit-scrollbar width 6px
46-51: Excessive!importantusage across utility classes.Many rules here use
!importanton nearly every property (e.g.,.section-header,.nested-item,.clear-all-button). This is likely to override cozy-ui/Material UI defaults, but it makes future maintenance harder. Consider using more specific selectors or scoping via a parent class instead, where feasible.packages/cozy-search/src/components/Assistant/AssistantContainer.jsx (1)
27-35: Hard-codedu-bg-whitemay break dark mode / theming.Both the sidebar wrapper (Line 27) and the main content area (Line 33) use
u-bg-white, while the stylesheets elsewhere (e.g.,styles.styl) usevar(--paperBackgroundColor)for theme compatibility. Consider using a theme-aware background utility or CSS variable instead.packages/cozy-search/src/index.ts (1)
7-12: Public API additions look reasonable.One note:
CozyComposer(Line 10) re-exportsConversationComposer— the naming mismatch between the public alias and the internal module name may cause confusion when navigating between consumer code and source. Consider aligning the names.packages/cozy-search/src/components/Sidebar/index.jsx (1)
28-32: No loading or error state for the conversations query.When
conversationsisundefined(loading) or the query fails, the sidebar shows nothing. Consider showing a spinner or skeleton while loading, and handling the error case for better UX.packages/cozy-search/src/components/TwakeKnowledges/ChatKnowledge.jsx (1)
17-22: Hardcoded mock data should be replaced with real data or clearly marked as placeholder.The
chatsarray is hardcoded with static entries ('Team Discussion','Project Updates', etc.). Unlike the conversation queries used elsewhere (e.g.,buildChatConversationsQueryinSearchConversation.jsx), this component doesn't fetch real data. If this is intentional scaffolding for a future integration, consider adding a// TODOcomment. If it should already be dynamic, wire it to a query or accept it as a prop.Additionally, since this array is static, move it outside the component to avoid re-creating it on every render.
Proposed minimal fix
+const CHATS = [ + { id: 'chat1', name: 'Team Discussion' }, + { id: 'chat2', name: 'Project Updates' }, + { id: 'chat3', name: 'General Chat' }, + { id: 'chat4', name: 'Support' } +] + const ChatKnowledge = ({ selectedItems, onToggleItems, onClearItems }) => { const { t } = useI18n() - const chats = [ - { id: 'chat1', name: 'Team Discussion' }, - { id: 'chat2', name: 'Project Updates' }, - { id: 'chat3', name: 'General Chat' }, - { id: 'chat4', name: 'Support' } - ] + // TODO: Replace with real data from API + const chats = CHATSpackages/cozy-search/src/components/Search/SearchConversation.jsx (2)
58-62: SearchBar is not wired to any search/filter logic.The
SearchBarrenders but has noonChange,onSearch, or value binding. It's currently non-functional. If this is intentional scaffolding, a// TODOcomment would help. Otherwise, it should be wired to filterconversationsbefore grouping.
20-24:buildChatConversationsQuery()is called on every render without memoization.Each call returns a fresh object. While
cozy-client'suseQuerylikely deduplicates by theaskey, it's a good practice to stabilize the reference (e.g., call it outside the component or wrap inuseMemo).Proposed fix
const SearchConversation = () => { const { t } = useI18n() const { createNewConversation, goToConversation } = useConversation() - const conversationsQuery = buildChatConversationsQuery() + const conversationsQuery = useMemo(() => buildChatConversationsQuery(), []) const { data: conversations } = useQuery( conversationsQuery.definition, conversationsQuery.options )packages/cozy-search/src/components/Conversations/ConversationListItem.jsx (1)
35-35:makeActionsis called on every render.
makeActions([share, rename, remove], { t })creates a new actions array each render. Consider memoizing it withuseMemo:Proposed fix
- const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx (1)
90-127: Three parallel switch statements onopenedKnowledgePanelcan be consolidated into a config map.
getTitle,getDescription, andgetIconall switch on the same key. A single lookup object would be more maintainable and eliminate the repetition:Proposed refactor
+const PANEL_CONFIG = { + drive: { titleKey: 'title_drive', descKey: 'desc_drive', icon: TDrive }, + mail: { titleKey: 'title_mail', descKey: 'desc_mail', icon: TMail }, + chat: { titleKey: 'title_chat', descKey: 'desc_chat', icon: TChat } +} // Inside the component: - const getTitle = () => { ... } - const getDescription = () => { ... } - const getIcon = () => { ... } + const config = PANEL_CONFIG[openedKnowledgePanel] || {} + const title = config.titleKey ? t(`assistant.twake_knowledges.${config.titleKey}`) : t('assistant.twake_knowledges.title_default') + const description = config.descKey ? t(`assistant.twake_knowledges.${config.descKey}`) : '' + const icon = config.icon || nullpackages/cozy-search/src/components/Views/AssistantDialog.jsx (2)
62-62: Redundant ternary — both branches are identical.
title={isMobile ? ' ' : ' '}always evaluates to' '. Simplify totitle=" ".Proposed fix
- title={isMobile ? ' ' : ' '} + title=" "
53-60: Inline style ondialogContent— consider using a CSS class.The rest of the component uses utility classes and
.stylmodules for styling. The inlinestyleobject ondialogContent(lines 54-59) is inconsistent with that pattern and will create a new object reference on every render.packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx (1)
143-165: Hardcoded mock data — same concern asChatKnowledge.jsx.
inboxItemsandstarredItemsare hardcoded with placeholder email data. If this is intentional scaffolding, add// TODOcomments to indicate these should be replaced with real data fetches. Also, these arrays are re-created on every render — move them outside the component if they remain static for now.Also applies to: 167-175
packages/cozy-search/src/components/Assistant/AssistantAvatar.jsx (1)
14-52: Consider extracting the repeatedclassNamecomputation.The same
cx(styles['assistant-icon'], { [styles['assistant-icon--small']]: isSmall }, className)expression is duplicated across all three branches. Extracting it into a variable would reduce repetition and make future changes less error-prone.Proposed refactor
const AssistantAvatar = ({ assistant, isSmall, className }) => { if (!assistant) return null + const iconClassName = cx( + styles['assistant-icon'], + { [styles['assistant-icon--small']]: isSmall }, + className + ) + if (assistant.id !== DEFAULT_ASSISTANT.id && !assistant.icon) { return ( <Icon icon={AssistantIcon} - className={cx( - styles['assistant-icon'], - { - [styles['assistant-icon--small']]: isSmall - }, - className - )} + className={iconClassName} /> ) }Apply the same replacement for the other two branches.
packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx (1)
38-38:makeActionsis called on every render.This creates new action instances and a new array reference on each render, which can trigger unnecessary re-renders of
ActionsMenu. Consider memoizing withuseMemo.Proposed fix
- const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])Add
useMemoto the React import on line 2.packages/cozy-search/src/components/Conversations/Conversation.jsx (1)
16-19: Unnecessary spread destructure.
{ ...queryMyselfResult }creates a shallow copy for no apparent benefit. This looks like a leftover from whendatawas extracted. Simplify to a direct assignment.Proposed fix
- const { ...queryMyselfResult } = useQuery( + const queryMyselfResult = useQuery( myselfQuery.definition, myselfQuery.options )packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx (2)
114-114:new StreamBridge()runs on every render.
useRef(new StreamBridge())evaluates the constructor on each render, discarding the result after the first. Use lazy initialization to avoid wasteful allocations.Proposed fix
- const streamBridgeRef = useRef(new StreamBridge()) + const streamBridgeRef = useRef<StreamBridge | null>(null) + if (!streamBridgeRef.current) { + streamBridgeRef.current = new StreamBridge() + }
122-124:useEffectwith empty deps referencesinitialMessages.The effect reads
initialMessagesbut has[]as deps, so it only runs on mount. This is intentionally "run once," but it will trigger an ESLintreact-hooks/exhaustive-depswarning. Suppress it explicitly to signal intent.Proposed fix
useEffect(() => { messagesIdRef.current = initialMessages.map(m => m.id).filter((id): id is string => !!id) + // eslint-disable-next-line react-hooks/exhaustive-deps }, [])
| ) | ||
| } | ||
|
|
||
| export default AssistantViewWithProviders |
There was a problem hiding this comment.
I don't understand, how is the top bar displayed? It is the same top bar than the app and the full screen modal has a margin top?
There was a problem hiding this comment.
The top bar is placed in cozy-home and cannot be displayed in the assistant if we use modal.
There was a problem hiding this comment.
Can we still display the assistant 100% full screen?
There was a problem hiding this comment.
Do you mean display assistant without top bar, right? If so, we still can display like that with the flag cozy.top-bar-in-assistant.enabled is false
lethemanh
force-pushed
the
new-assistant-ui
branch
from
f3a5d8b to
e10de18
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cozy-search/package.json (1)
52-53:⚠️ Potential issue | 🟠 MajorDev React version (16.12.0) is incompatible with
@assistant-ui/react.The devDependencies pin React at 16.12.0, but
@assistant-ui/reactrequires^18 || ^19. This means tests and local development using these assistant-ui components will fail or produce misleading results. Bump the dev React/ReactDOM versions to at least 18.x to match the runtime requirements.Proposed fix
- "react": "16.12.0", - "react-dom": "16.13.0", + "react": "18.2.0", + "react-dom": "18.2.0",packages/cozy-search/src/components/Assistant/AssistantSelection.jsx (1)
104-104:⚠️ Potential issue | 🟡 MinorHardcoded English string "Create Assistant" should use the translation function.
This string will not be localized for fr/ru/vi users. Use
useI18nor thetfunction to get a translated label.Proposed fix (assuming a translation key exists or will be added)
- <Typography variant="body1">Create Assistant</Typography> + <Typography variant="body1">{t('assistant.create')}</Typography>
🤖 Fix all issues with AI agents
In `@packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts`:
- Around line 120-126: The code in CozyRealtimeChatAdapter currently yields the
raw backend/network error message (errorMessage) into the chat UI; change this
so the yielded content uses a generic user-facing string like "An unexpected
error occurred. Please try again later." (do not include error.message or error
details), and separately log the real error for debugging (e.g., call
logger.error or console.error with the Error object) before yielding; update the
errorMessage handling and the yield block that returns content: [{ type: 'text',
text: `Error: ${errorMessage}` }] and status: { type: 'incomplete', reason:
'error' } to use the generic message while preserving the existing status shape.
In `@packages/cozy-search/src/components/Conversations/Conversation.jsx`:
- Around line 15-24: The code currently runs buildMyselfQuery() and
useQuery(...) (queryMyselfResult) but never uses the returned data; either
remove the unused query or document that it's intentional prefetching. To fix:
if you don't need the data, delete buildMyselfQuery() and the useQuery(...)
invocation (and the isLoading check), removing queryMyselfResult and
isQueryLoading usage; otherwise add a short clarifying comment above the
buildMyselfQuery/useQuery block stating it is intentional prefetching to warm
the cozy-client cache and keep isLoading handling as-is. Reference symbols:
buildMyselfQuery, useQuery, queryMyselfResult, isQueryLoading,
ConversationComposer, UserMessage, AssistantMessage.
In
`@packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx`:
- Around line 55-59: The code accesses conversation.messages[...] directly and
can crash if messages is undefined or empty; update the accesses in
ConversationListItemWider.jsx (the JSX that renders primary and the secondary
message preview) to safely handle missing or short message arrays by deriving a
safeMessages = conversation.messages || [] (or using optional chaining and a
length check) and then indexing only after verifying safeMessages.length >= 2
(or >=1 for the last item), falling back to an empty string or placeholder when
no message exists; apply this same guard to both places that reference
conversation.messages[...] (the primary block using the second-to-last message
and the other usage around lines 85–87).
In `@packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx`:
- Around line 30-32: The useEffect that calls setSelectedItems based on
selectedTwakeKnowledge[openedKnowledgePanel] causes local selectedItems to be
overwritten whenever selectedTwakeKnowledge reference changes (even for
unrelated panels), leading to stale-state flickers; to fix, change the
synchronization to only initialize or sync when the openedKnowledgePanel
actually changes or when the incoming value truly differs (compare by panel key
or deep equality) OR switch to a key-based remount so the component is
re-mounted per panel (e.g., use key={openedKnowledgePanel} on this panel
component), and ensure useEffect/selectors reference useEffect,
setSelectedItems, selectedTwakeKnowledge, openedKnowledgePanel, and
selectedItems so local toggles/clears are not clobbered by upstream object
identity changes.
- Around line 149-154: The SearchBar in TwakeKnowledgePanel.jsx is rendered
without state or handlers; add a controlled input and filtering: inside the
TwakeKnowledgePanel component create local state (e.g., const [query, setQuery]
= useState('')), pass value={query} and onChange={e => setQuery(e.target.value)}
to the SearchBar, and apply a filter (e.g., filter by name or content) to the
array of knowledges/entries before mapping/rendering them so the UI updates as
the user types; if this is intentionally WIP, instead add a clear TODO comment
above the SearchBar explaining the missing wiring.
- Line 135: The img rendered by TwakeKnowledgePanel (the element using
getIcon()) is missing an alt attribute; update the JSX in
TwakeKnowledgePanel.jsx to include a meaningful alt string (or alt="" if the
icon is purely decorative) on the <img> tag so screen readers get appropriate
information; if the alt text should reflect the icon type, derive it from the
same source that selects the icon (e.g., pass a label or key alongside getIcon()
or compute an alt based on the knowledge item) and set that value as the img's
alt prop.
In `@packages/cozy-search/src/locales/ru.json`:
- Line 51: The "items_selected" translation in ru.json currently has only two
plural forms; update the "items_selected" value to include three Russian plural
forms separated by "||||" (singular, paucal, plural) mirroring the pattern used
for the "sources" key: provide an appropriate singular form for smart_count=1,
the 2-4 form, and the 0/5+ form so Russian declension is correct (edit the
"items_selected" entry to include all three variants).
🧹 Nitpick comments (15)
packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx (1)
12-12: Unnecessary path traversal — file is already inAssistant/.Since
AssistantSelectionItem.jsxlives in theAssistantdirectory,../Assistant/AssistantAvatargoes up a level only to come back into the same folder. A direct relative import is cleaner.Suggested fix
-import AssistantAvatar from '../Assistant/AssistantAvatar' +import AssistantAvatar from './AssistantAvatar'packages/cozy-search/src/components/TwakeKnowledges/styles.styl (2)
43-64: Excessive!importantusage suggests specificity conflicts.Nearly every property in
.section-header,.nested-item, and.clear-all-buttonuses!important. This typically indicates fighting against a UI library's inline styles or high-specificity selectors. While it works, it makes future style overrides very difficult and is fragile to upstream changes.Consider whether these classes can be applied with higher specificity selectors (e.g.,
.source-panel .section-header) or by using the UI library's built-in styling/theming mechanisms (e.g.,sxprop,classesoverride) instead of!important.
30-35: WebKit-only scrollbar styling.The
::-webkit-scrollbarpseudo-elements only work in Chromium/WebKit browsers. Firefox users will see the default scrollbar. If cross-browser custom scrollbar styling matters, consider addingscrollbar-width: thinandscrollbar-colorfor Firefox support.Proposed addition for Firefox support
.source-panel-content flex 1 overflow-y auto padding 0 8px + scrollbar-width thin + scrollbar-color var(--dividerColor) transparent &::-webkit-scrollbar width 6pxpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx (1)
90-127: Three parallel switch statements on the same key can be consolidated into a config map.
getTitle,getDescription, andgetIconall switch onopenedKnowledgePanelwith identical cases. A single lookup object would reduce duplication and make adding new panels trivial.Suggested refactor
+const PANEL_CONFIG = { + drive: { titleKey: 'title_drive', descKey: 'desc_drive', icon: TDrive }, + mail: { titleKey: 'title_mail', descKey: 'desc_mail', icon: TMail }, + chat: { titleKey: 'title_chat', descKey: 'desc_chat', icon: TChat } +} + const TwakeKnowledgePanel = ({ onClose }) => { const { t } = useI18n() // ... + const panelConfig = PANEL_CONFIG[openedKnowledgePanel] || {} + const title = t(`assistant.twake_knowledges.${panelConfig.titleKey || 'title_default'}`) + const description = panelConfig.descKey ? t(`assistant.twake_knowledges.${panelConfig.descKey}`) : '' + const icon = panelConfig.icon || nullThen use
title,description, andicondirectly in the JSX, removing the threegetXfunctions.packages/cozy-search/package.json (1)
17-18: Removeclsxfrom direct dependencies—it's pulled in transitively byclass-variance-authority.
classnamesis actively used throughout the codebase (15+ files), whileclsxhas zero direct imports. Sinceclass-variance-authorityalready declaresclsxas a dependency, there's no reason to list it as a direct dependency.packages/cozy-search/src/components/Conversations/Conversation.jsx (1)
16-16: Unnecessary spread destructure.
const { ...queryMyselfResult }is equivalent toconst queryMyselfResult = useQuery(...). The spread-into-rest adds no value here.♻️ Simplify
- const { ...queryMyselfResult } = useQuery( + const queryMyselfResult = useQuery( myselfQuery.definition, myselfQuery.options )packages/cozy-search/src/components/Conversations/ConversationBar.jsx (1)
77-100:IconButtonwrappingButtonis redundant nesting.Both
IconButtonandButtonare interactive controls. Whilecomponent="div"on the innerButtonavoids an illegal nested<button>, theIconButtonwrapper serves no visible purpose — it adds an extra DOM node and click target that doesn't contribute styling or semantics beyond what theButtonalready provides. Consider using just one of the two.Simplify by removing the IconButton wrapper
- endAdornment: isRunning ? ( - <IconButton className="u-p-0 u-mr-half"> - <Button - size="small" - component="div" - className="u-miw-auto u-w-2 u-h-2 u-bdrs-circle" - classes={{ label: 'u-flex u-w-auto' }} - label={<Icon icon={StopIcon} size={12} />} - onClick={onCancel} - /> - </IconButton> - ) : ( - <IconButton className="u-p-0 u-mr-half"> - <Button - size="small" - component="div" - className="u-miw-auto u-w-2 u-h-2 u-bdrs-circle" - classes={{ label: 'u-flex u-w-auto' }} - label={<Icon icon={PaperplaneIcon} size={12} rotate={-45} />} - disabled={isEmpty} - onClick={handleSend} - /> - </IconButton> + endAdornment: isRunning ? ( + <Button + size="small" + className="u-miw-auto u-w-2 u-h-2 u-bdrs-circle u-mr-half" + classes={{ label: 'u-flex u-w-auto' }} + label={<Icon icon={StopIcon} size={12} />} + onClick={onCancel} + /> + ) : ( + <Button + size="small" + className="u-miw-auto u-w-2 u-h-2 u-bdrs-circle u-mr-half" + classes={{ label: 'u-flex u-w-auto' }} + label={<Icon icon={PaperplaneIcon} size={12} rotate={-45} />} + disabled={isEmpty} + onClick={handleSend} + /> ),packages/cozy-search/src/actions/delete.jsx (1)
9-24: DuplicatedmakeComponentacross delete, share, and rename action files.The
makeComponentfunction is nearly identical indelete.jsx,share.jsx, andrename.jsx. The only variations are theclassNameprop anddisplayName. Consider extracting a shared factory that accepts{ displayName, className }to reduce duplication.♻️ Shared factory sketch (e.g., in a new `makeActionComponent.jsx`)
import React, { forwardRef } from 'react' import ActionsMenuItem from 'cozy-ui/transpiled/react/ActionsMenu/ActionsMenuItem' import Icon from 'cozy-ui/transpiled/react/Icon' import ListItemIcon from 'cozy-ui/transpiled/react/ListItemIcon' import ListItemText from 'cozy-ui/transpiled/react/ListItemText' export const makeActionComponent = (label, icon, { displayName, className } = {}) => { const Component = forwardRef((props, ref) => ( <ActionsMenuItem className={className} {...props} ref={ref}> <ListItemIcon> <Icon className={className} icon={icon} /> </ListItemIcon> <ListItemText primary={label} /> </ActionsMenuItem> )) Component.displayName = displayName || 'ActionComponent' return Component }packages/cozy-search/src/components/adapters/StreamBridge.ts (2)
75-95: Consider adding areturn()method on the async iterator.The iterator lacks a
return()method, which means if a consumer usesbreakin afor awaitloop or the loop is otherwise interrupted, the iterator's internal cleanup won't run automatically per the async iterator protocol. Currently, the adapter handles this externally viastreamBridge.cleanup(), so it works in practice. However, implementingreturn()would make the iterator self-contained and safer for any future consumers.♻️ Proposed addition
iterator: { next: (): Promise<IteratorResult<string>> => new Promise((resolve, reject) => { // ... existing logic }), + return: (): Promise<IteratorResult<string>> => { + if (!isDone) { + isDone = true + if (resolveNext) { + resolveNext({ value: undefined as unknown as string, done: true }) + resolveNext = null + rejectNext = null + } + } + return Promise.resolve({ value: undefined as unknown as string, done: true }) + }, [Symbol.asyncIterator]() { return this } }
16-18:cleanupCallbackis a single global callback, not per-conversation.If
StreamBridgeis ever used to manage multiple simultaneous conversations, the singlecleanupCallbackwill fire for any conversation's cleanup. Currently the provider uses one conversation at a time (route-driven), so this is fine — just noting for future awareness.packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx (1)
122-124: Missing dependency inuseEffectwill trigger exhaustive-deps lint warning.
initialMessagesis used inside the effect but not listed in the dependency array. While the intent (run on mount only) is valid since this component is remounted per conversation, the empty[]will trigger a React lint warning. Suppress it explicitly to signal intent.♻️ Proposed fix
useEffect(() => { messagesIdRef.current = initialMessages.map(m => m.id).filter((id): id is string => !!id) - }, []) + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally run only on mount; initialMessages is stable for this component's lifetime + }, [])packages/cozy-search/src/components/styles.styl (1)
20-21: Magic number48px— consider using a CSS variable or a comment.The
48pxpresumably corresponds to a header/toolbar height. A brief comment or a shared CSS variable (e.g.,--header-height) would improve maintainability and make the coupling explicit.Also note that
100vhon mobile browsers doesn't account for the dynamic address bar. If mobile support matters,100dvh(dynamic viewport height) is more reliable, though it requires checking your browser support targets.packages/cozy-search/src/components/Sidebar/index.jsx (1)
28-32:buildChatConversationsQuery()is re-invoked on every render.Consider hoisting the query outside the component or wrapping in
useMemoto avoid rebuilding the query descriptor object each render cycle. It's functionally harmless (useQuery caches byoptions.as), but it's a cheap win for clarity.Proposed fix
+const conversationsQuery = buildChatConversationsQuery() + const Sidebar = () => { ... - const conversationsQuery = buildChatConversationsQuery() const { data: conversations } = useQuery( conversationsQuery.definition, conversationsQuery.options )packages/cozy-search/src/components/Views/AssistantDialog.jsx (1)
62-62: Redundant ternary — both branches are identical.
isMobile ? ' ' : ' 'evaluates to' 'in all cases. If the title should differ between mobile and desktop, update accordingly; otherwise simplify.Proposed fix
- title={isMobile ? ' ' : ' '} + title=" "packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx (1)
38-38:makeActionsis recomputed on every render.Consider memoizing with
useMemosince the inputs (t) only change on locale switch.Proposed fix
- const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])Add
useMemoto the React import on Line 2.
lethemanh
force-pushed
the
new-assistant-ui
branch
from
e10de18 to
794a4e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cozy-search/src/components/Assistant/AssistantSelection.jsx (1)
98-106:⚠️ Potential issue | 🟡 MinorHardcoded English string "Create Assistant" — should use
t()for i18n.All other user-facing strings in this PR go through
useI18n, but this label is a raw English string. It will not be translated.Proposed fix
You'd need to import
useI18nand add a locale key, then:- <Typography variant="body1">Create Assistant</Typography> + <Typography variant="body1">{t('assistant.sidebar.create_new')}</Typography>(or a dedicated key if
create_newdoesn't match the intended label)
🤖 Fix all issues with AI agents
In `@packages/cozy-search/src/components/Conversations/ConversationBar.jsx`:
- Around line 76-99: The JSX nests an IconButton (outer) around a Button
(inner), which produces invalid nested <button> elements and causes
inaccessible/incorrect click behavior; update ConversationBar.jsx by removing
the IconButton wrapper and apply any necessary classes/props directly to the
inner Button (the components referenced are IconButton and Button), ensuring the
active handlers (onClick: handleSend and onCancel) and props (disabled tied to
isEmpty, size, component, className, classes, label/icons, rotate) are preserved
on the single rendered interactive element when isRunning toggles; alternatively
replace IconButton with a non-interactive wrapper (span/div) if additional
layout is required so clicks on the full area trigger the inner handler.
- Around line 40-48: The current handleSend function calls both onSend() and
onAssistantExecute(), causing duplicate POSTs (the
composerRuntime/CozyRealtimeChatAdapter already posts when onSend triggers).
Remove the redundant onAssistantExecute() invocation from handleSend (keep
onSend() and the inputRef height reset) so only onSend() performs the backend
request; update any related comments if needed to note that
CozyRealtimeChatAdapter.run now handles the POST.
In
`@packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx`:
- Around line 60-69: The dots IconButton is inside the ListItem whose onClick
calls onOpenConversation(conversation._id), so clicking the IconButton triggers
both toggleMenu and the ListItem navigation; update the toggleMenu handler used
by the IconButton (reference: toggleMenu function and IconButton with ref
anchorRef) to call event.stopPropagation() (and event.preventDefault() if
appropriate) at the start so the click on IconButton does not bubble to the
ListItem onClick that calls onOpenConversation(conversation._id).
In `@packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx`:
- Around line 202-212: The useMemo that creates adapter via
createCozyRealtimeChatAdapter is missing t in its dependency array, causing
stale translations when locale changes; update the useMemo dependency list for
adapter (the call that references client, conversationId,
streamBridgeRef.current and t) to include t so the adapter is recreated on
locale/translation updates.
- Around line 125-147: handleConversationChange is recreated each render causing
realtime subscriptions to hold stale callbacks; wrap it with React.useCallback
and add conversationId (and any other used refs/values if needed) to its
dependency array so the callback identity only changes when its dependencies
change, then use the memoized handleConversationChange in the useRealtime
subscription setup to ensure subscribe/unsubscribe use matching callback
references (refer to handleConversationChange, currentStreamingMessageIdRef,
messagesIdRef, cancelledMessageIdsRef).
In `@packages/cozy-search/src/components/helpers.js`:
- Around line 66-75: The time formatting forces 12-hour output by hardcoding
hour12: true in the date.toLocaleTimeString call; remove the hour12 property so
toLocaleTimeString(date, lang, {...}) uses the locale passed in (lang) and
formats times correctly for locales like fr, ru, vi—update the block that
computes timeStr (where isToday/isYesterday are used) to call
date.toLocaleTimeString(lang, { hour: 'numeric', minute: '2-digit' }) instead of
including hour12.
In `@packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx`:
- Line 75: In MailKnowledge.jsx the badge check "{count && <span
className={styles['badge']}>{count}</span>}" will render "0" when count is 0;
change the condition to explicitly guard against null/undefined and zero (e.g.,
use "count != null && count !== 0 && <span
className={styles['badge']}>{count}</span>" or "typeof count === 'number' &&
count > 0 && <span className={styles['badge']}>{count}</span>") so the span is
only rendered for a positive numeric count.
In `@packages/cozy-search/src/components/Views/AssistantDialog.jsx`:
- Line 62: The title prop in AssistantDialog.jsx uses a dead ternary
(title={isMobile ? ' ' : ' '}) which always yields a single space; update the
JSX to either remove the ternary and pass the intended static value directly
(e.g., title=" " or title="") or make the branches meaningful (e.g.,
title={isMobile ? '' : ' '}) depending on desired mobile vs desktop behavior;
look for the title prop usage in the AssistantDialog component and adjust the
expression accordingly to eliminate the redundant ternary.
In `@packages/cozy-search/src/locales/fr.json`:
- Around line 37-45: The French locale is missing the assistant.time and
assistant.message translation blocks, causing untranslated keys in
Conversation.jsx (t('assistant.message.welcome')) and AssistantMessage.jsx
(t('assistant.message.running')); add an "assistant" object to
packages/cozy-search/src/locales/fr.json with a "time" block (today/yesterday)
and a "message" block (welcome/running) immediately after the existing
"search_conversation" block (before "twake_knowledges"), using French
translations for those keys so the components render localized text.
In `@packages/cozy-search/src/locales/vi.json`:
- Around line 43-49: Add the missing assistant.message translation block to the
Vietnamese locale by defining the keys assistant.message.welcome and
assistant.message.running with appropriate Vietnamese strings (matching the
English intent), e.g., add an "assistant": { "message": { "welcome": "...",
"running": "..." } } object at the top level of the vi.json so those keys exist
and mirror the English locale's entries.
🧹 Nitpick comments (19)
packages/cozy-search/src/components/Messages/UserMessage.jsx (1)
19-23: Extract the inlineTextcomponent to a stable reference.The anonymous arrow function passed as
Textcreates a new component identity on every render, which can cause unnecessary unmount/remount cycles. Define it outsideUserMessage.♻️ Proposed fix
+const Text = ({ text }) => <Typography>{text}</Typography> + const UserMessage = () => { return ( <MessagePrimitive.Root className="u-mt-1"> <Card className={cx( 'u-bg-paleGrey u-bdrs-5 u-bdw-0 u-ml-auto u-p-half', styles['cozyThread-user-messages'] )} > <MessagePrimitive.Parts components={{ - Text: ({ text }) => <Typography>{text}</Typography> + Text }} /> </Card> </MessagePrimitive.Root> ) }packages/cozy-search/src/types.d.ts (1)
24-28:handlertype is too restrictive — it should accept an event parameter.The handler is typed as
() => void, but event listener callbacks typically receive anEventobject. This will cause type errors for any caller that needs to read the event.Proposed fix
declare module 'cozy-ui/transpiled/react/hooks/useEventListener' { - const useEventListener: (element: any, event: string, handler: () => void) => void + const useEventListener: (element: any, event: string, handler: (event: Event) => void) => void export default useEventListener }packages/cozy-search/src/components/helpers.js (1)
49-51: No validation for invalid date strings.
new Date('garbage')produces anInvalid Dateobject, and subsequent comparisons andtoLocaleTimeString/toLocaleDateStringcalls will return"Invalid Date"strings rendered in the UI. A quick guard would prevent this.Proposed fix
export const formatConversationDate = (dateString, t, lang) => { if (!dateString) return '' const date = new Date(dateString) + if (isNaN(date.getTime())) return '' const now = new Date()packages/cozy-search/src/components/TwakeKnowledges/styles.styl (1)
1-93: Stylesheet looks reasonable for the panel layout.The Stylelint error on line 2 is a false positive — Stylus syntax doesn't use colons for property-value pairs.
The heavy
!importantusage throughout (lines 46–47, 49–51, 54–60, 84–85, 88, 91–93) suggests overriding component library defaults. This works but can become fragile if upstream styles change. Consider scoping via more specific selectors where feasible.packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx (1)
24-139:MailSectionandDriveSectionare nearly identical — consider extracting a sharedKnowledgeSectioncomponent.Both components have the same collapsible header pattern, tri-state checkbox logic, clear-all button, and nested item list. The only differences are the item rendering (mail shows subject/from/preview/date; drive shows folder name). A shared wrapper accepting a custom
renderItemprop would eliminate the duplication.Also applies to: 18-107
packages/cozy-search/src/components/TwakeKnowledges/DriveKnowledge.jsx (1)
35-37:customItemsfilter is dead code — no items currently have IDs'my-drive'or'shared-with-me'.The hardcoded items use IDs like
'doc1','proj1', etc., so this filter returns the same array asitems. If this is forward-looking logic for real data, add a comment to clarify. Otherwise, it adds confusion.packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx (1)
59-127: Fourswitchstatements onopenedKnowledgePanel— consolidate into a config map.
renderContent,getTitle,getDescription, andgetIconall switch on the same key. A single config object would eliminate the repetition and make adding new panel types a one-line change.Suggested approach
+const PANEL_CONFIG = { + drive: { + component: DriveKnowledge, + titleKey: 'assistant.twake_knowledges.title_drive', + descKey: 'assistant.twake_knowledges.desc_drive', + icon: TDrive + }, + mail: { + component: MailKnowledge, + titleKey: 'assistant.twake_knowledges.title_mail', + descKey: 'assistant.twake_knowledges.desc_mail', + icon: TMail + }, + chat: { + component: ChatKnowledge, + titleKey: 'assistant.twake_knowledges.title_chat', + descKey: 'assistant.twake_knowledges.desc_chat', + icon: TChat + } +}Then use
PANEL_CONFIG[openedKnowledgePanel]to look up all four values at once.packages/cozy-search/src/components/Conversations/ConversationComposer.jsx (2)
26-28:handleSendhas no empty-input guard — relies onConversationBarfor protection.
composerRuntime.send()is called unconditionally here. The guard (if (isEmpty) return) lives insideConversationBar.handleSend(for button clicks) andConversationBar.handleKeyDown(for Enter). This works for the current wiring, but ifhandleSendis ever called from a different path, an empty message could be dispatched. Consider adding a guard here as well, or document the assumption.💡 Optional: add a local guard
const handleSend = useCallback(() => { + if (isEmpty) return composerRuntime.send() - }, [composerRuntime]) + }, [composerRuntime, isEmpty])
61-66: Layout with single child when feature flag is off.When
cozy.create-assistant.enabledisfalse, onlyTwakeKnowledgeSelectorrenders inside theu-flex-justify-betweencontainer. A single child withjustify-betweenbehaves likejustify-start, pushing the knowledge selector to the left. If the intent is to keep it right-aligned regardless,u-flex-justify-endwould be more robust.packages/cozy-search/src/components/Assistant/styles.styl (1)
23-36: Heavy use of!importantfor menu-item overrides.Five
!importantdeclarations to override the baseActionsMenuItemstyles suggests a specificity battle. This works but is fragile — any future cozy-ui update that adds its own!importantwill break these overrides. If possible, consider increasing specificity via nesting or a more specific selector instead.packages/cozy-search/src/actions/share.jsx (2)
9-24:makeComponentis duplicated acrossshare.jsx,rename.jsx, anddelete.jsx.The
makeComponenthelper inshare.jsxandrename.jsxis identical. Evendelete.jsxonly differs by addingclassName="u-error". Consider extracting a sharedmakeActionComponent(label, icon, options?)utility to reduce duplication.
36-36: Share action is a no-op.
action: () => { }means clicking "Share" does nothing. If this is intentionally stubbed out for a future PR, consider adding a brief comment or a// TODOso it's not mistaken for a bug.packages/cozy-search/src/actions/rename.jsx (1)
9-24:makeComponentis duplicated across action files.This helper is identical in
rename.jsx,delete.jsx, andshare.jsx(per AI summary). Consider extracting it into a shared utility to reduce duplication. Low priority given the small footprint.packages/cozy-search/src/components/Views/AssistantDialog.jsx (1)
53-60: Inline style object is re-created on every render.The
dialogContent.styleobject is a new reference each render, which can defeat shallow-comparison optimizations downstream. Consider extracting it as a module-level constant.Proposed fix
+const dialogContentStyle = { + display: 'flex', + flexDirection: 'column', + flexGrow: 1, + padding: 0 +} + const AssistantDialog = () => { ... dialogContent: { - style: { - display: 'flex', - flexDirection: 'column', - flexGrow: 1, - padding: 0 - } + style: dialogContentStyle }packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx (2)
112-114:useEffectintentionally omitsinitialMessagesfrom deps — add suppression comment.This effect seeds
messagesIdRefonce on mount. The missing dependency is intentional, but a future contributor (or linter) may "fix" it and introduce a bug. Aneslint-disable-next-linecomment clarifies intent.useEffect(() => { messagesIdRef.current = initialMessages.map(m => m.id).filter((id): id is string => !!id) + // eslint-disable-next-line react-hooks/exhaustive-deps -- One-time initialization from initial load }, [])
66-95:ConversationLoaderrendersnullduring loading — consider a loading indicator.Returning
nullwhile the conversation query loads means the user sees a blank screen. A spinner or skeleton would improve perceived responsiveness, especially on slow connections.packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx (1)
38-38:actionsarray is recreated on every render.
makeActions(...)is called unconditionally on each render. Consider memoizing withuseMemokeyed ont.- const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts (1)
87-104: Abort is only checked between chunks, not proactively.If the stream is blocked waiting for the next chunk (no data arriving),
abortSignal.abortedwon't be evaluated until the nextfor awaititeration completes. For a responsive cancel, consider listening toabortSignal.addEventListener('abort', ...)to callstreamBridge.cleanup()immediately. This is a minor UX concern — the stop button may feel unresponsive if the backend stalls.packages/cozy-search/src/components/adapters/StreamBridge.ts (1)
24-26:cleanupCallbackis a singleton shared across all conversations.
setCleanupCallbackstores a single callback, not per-conversation. IfStreamBridgewere ever used for multiple concurrent conversations (e.g., prefetching), cleanup for one conversation would invoke the other's callback. Currently safe given the 1:1 provider-to-conversation relationship, but worth noting for future changes.
lethemanh
force-pushed
the
new-assistant-ui
branch
3 times, most recently
from
22a7fcc to
1d44629
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cozy-search/package.json (2)
51-52:⚠️ Potential issue | 🟠 MajordevDependencies still pin React 16 — tests will run against an unsupported version.
peerDependenciesnow correctly require>=18.0.0, but the dev versions used for testing are stillreact@16.12.0/react-dom@16.13.0. This means:
- Unit tests execute against a React version the library no longer supports.
@assistant-ui/reactinternals (e.g.,useSyncExternalStore, concurrent features) will break or silently degrade under React 16, making test results unreliable.Bump both to
18.x(e.g.,18.2.0) to match the declared peer range.- "react": "16.12.0", - "react-dom": "16.13.0", + "react": "18.2.0", + "react-dom": "18.2.0",
32-32:⚠️ Potential issue | 🟠 MajorUpgrade
@testing-library/reactto v13+ for React 18 compatibility.
@testing-library/reactv10.4.9 uses the legacyReactDOM.renderAPI, which is incompatible with React 18. Version 13 and newer require and support React 18 withcreateRoot-based rendering.Suggested change
- "@testing-library/react": "10.4.9", + "@testing-library/react": "14.2.1",
🤖 Fix all issues with AI agents
In `@packages/cozy-search/package.json`:
- Around line 13-14: Update the package.json dependency entry for
`@assistant-ui/react-markdown` to a 0.12.x range to match `@assistant-ui/react`
v0.12.x (change the version spec for "@assistant-ui/react-markdown" from
"^0.11.0" to "^0.12.0" or a compatible 0.12.x); ensure the updated version
aligns with the peer dependency requirement for `@assistant-ui/react` (e.g.,
^0.12.9) so both "@assistant-ui/react" and "@assistant-ui/react-markdown" are on
compatible 0.12.x releases.
In `@packages/cozy-search/src/components/adapters/StreamBridge.ts`:
- Around line 74-94: The iterator.next implementation allows multiple concurrent
pending calls because resolveNext/rejectNext are overwritten; update
iterator.next in StreamBridge to guard against concurrent calls by detecting
when resolveNext or rejectNext is already set and immediately reject the new
Promise (or return the existing pending promise) instead of overwriting;
reference the iterator.next method and the resolveNext/rejectNext, queue,
isDone, push/complete/error flow so you add the guard at the start of next() to
prevent orphaning earlier promises and ensure only one outstanding consumer
promise is tracked.
In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx`:
- Around line 79-86: The code accesses conversation.cozyMetadata.updatedAt
directly in ConversationListItem.jsx which can throw if cozyMetadata is
undefined; update the call to formatConversationDate to use optional chaining
(e.g., pass conversation.cozyMetadata?.updatedAt) and ensure
formatConversationDate can accept undefined/nullable timestamps (or provide a
sensible fallback) so the Typography render is safe when cozyMetadata is
missing.
In `@packages/cozy-search/src/components/Conversations/styles.styl`:
- Around line 79-85: The action button (.conversation-list-item-action) is
currently hidden with display: none !important and only shown on :hover, making
it inaccessible to keyboard users; add a :focus-within rule on
.conversation-list-item to reveal .conversation-list-item-action when the list
item or its children receive keyboard focus (mirror the hover behavior) and
ensure the rule has at least the same specificity/importance as the existing
display: none !important so keyboard-only users can tab to and operate the dots
menu.
In `@packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx`:
- Around line 18-21: The project imports AssistantRuntimeProvider and
useLocalRuntime from `@assistant-ui/react` which requires React ^18/^19 but the
package currently uses React 16.12.0; fix by upgrading the cozy-search package
React peer (and any react-dom/test-utils) to ^18 (or ^19) in package.json and
run a reinstall and test, or if upgrading is not possible remove/replace
`@assistant-ui/react` usage (e.g., eliminate
AssistantRuntimeProvider/useLocalRuntime and their consumers) with a compatible
library; update any React APIs used in CozyAssistantRuntimeProvider.tsx to React
18-compatible patterns (keep references to AssistantRuntimeProvider and
useLocalRuntime when deciding whether to upgrade or remove).
In `@packages/cozy-search/src/components/helpers.js`:
- Around line 49-82: The function formatConversationDate does not guard against
unparseable dateString values; after const date = new Date(dateString) add a
validity check (e.g., if (isNaN(date.getTime())) return '' or a localized
fallback) to short-circuit before computing isToday/isYesterday or calling
toLocaleDateString; this prevents returning "Invalid Date" and keeps behavior
consistent with the existing empty-string guard for falsy inputs.
In `@packages/cozy-search/src/components/Messages/MarkdownText.jsx`:
- Around line 1-14: The component MarkdownText uses the React 18+ hook
useMessagePartText from `@assistant-ui/react` which is incompatible with the
project's React 16.12.0; fix by either upgrading React to ^18 (update project
peer dependency and rebuild) or remove the offending hook and implement a
React-16-compatible replacement: modify MarkdownText to obtain the message text
from an alternative source (e.g., a prop passed into MarkdownText or a local
context/provider) instead of calling useMessagePartText; ensure references to
useMessagePartText are removed and that MarkdownText still renders Markdown with
the text via the existing Markdown component.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 42-44: The current logic in SearchConversation.jsx sets date = new
Date(conv.cozyMetadata?.updatedAt || Date.now()).getTime(), which forces items
missing updatedAt into the "today" bucket; change the fallback to prefer
conv.cozyMetadata?.createdAt before Date.now() (e.g. use
conv.cozyMetadata?.updatedAt ?? conv.cozyMetadata?.createdAt) or alternatively
skip/flag conversations missing both timestamps so they aren't grouped as
"today"; update the code that computes the date value (the date variable) to
apply this new fallback/filtering.
- Around line 58-62: The SearchBar is rendered but not wired to state or
filtering logic; update SearchConversation.jsx to manage a search query state
(e.g., useState for query) and pass it into the SearchBar via value and onChange
(or onInput) so user input updates the query, then use that query to filter the
conversations list (or invoke a provided callback prop like onSearchChange)
before rendering results; if this is intentionally a WIP, add a clear TODO
comment next to the SearchBar noting that value/onChange and filtering must be
implemented.
- Around line 20-24: buildChatConversationsQuery() currently gets called on
every render which returns a new object and causes useQuery (consuming
conversationsQuery.definition/options) to re-run; to fix, move the call out of
the render or memoize it: replace the inline call to
buildChatConversationsQuery() with a stable value (e.g., wrap the call in
useMemo inside SearchConversation component or export a module-level constant)
so conversationsQuery (and its .definition/.options) keep stable references
across renders and prevent unnecessary re-fetches.
In `@packages/cozy-search/src/types.d.ts`:
- Around line 24-28: The handler type for useEventListener is too
narrow—currently declared as () => void which rejects handlers that accept an
Event parameter; update the declaration of useEventListener so the handler
accepts an event (e.g., (event: Event) => void or (event?: Event) => void to
allow both signatures) and, while here, consider tightening the element type
from any to EventTarget | null to better reflect usage; edit the
useEventListener declaration accordingly.
🧹 Nitpick comments (19)
packages/cozy-search/src/types.d.ts (1)
50-56:useI18nreturn type is incomplete — missingpolyglot.The actual
useI18nhook (seepackages/twake-i18n/src/useExtendI18n.jsx) also returnspolyglotin its return value, which is used byuseExtendI18n. While current consumers in this PR only destructuretandlang, omittingpolyglotfrom the type makes the declaration inaccurate and could cause type errors for future callers.Proposed fix
declare module 'twake-i18n' { export function useI18n(): { t: (key: string, options?: Record<string, unknown>) => string lang: string + polyglot: any } export function useExtendI18n(locales: Record<string, unknown>): void }packages/cozy-search/src/components/styles.styl (1)
20-21: Consider documenting or extracting the magic48pxvalue.The
48pxoffset presumably corresponds to a header/toolbar height. A comment clarifying what it accounts for would help future maintainers.packages/cozy-search/src/components/Messages/UserMessage.jsx (1)
19-23: Extract the inlineTextcomponent to avoid re-creation on every render.The inline
({ text }) => <Typography>{text}</Typography>creates a new component reference each render, which can causeMessagePrimitive.Partsto unmount/remount its children unnecessarily.♻️ Proposed fix
Define the component outside
UserMessage:+const TextPart = ({ text }) => <Typography>{text}</Typography> + const UserMessage = () => { return ( <MessagePrimitive.Root className="u-mt-1"> <Card className={cx( 'u-bg-paleGrey u-bdrs-5 u-bdw-0 u-ml-auto u-p-half', styles['cozyThread-user-messages'] )} > <MessagePrimitive.Parts components={{ - Text: ({ text }) => <Typography>{text}</Typography> + Text: TextPart }} /> </Card> </MessagePrimitive.Root> ) }packages/cozy-search/src/components/Messages/AssistantMessage.jsx (1)
15-15: Stabilize theuseMessageselector to avoid potential extra re-renders.If
useMessageuses reference equality on the selector (common pattern), a new arrow function each render could cause unnecessary re-subscriptions.♻️ Proposed fix
+const selectIsRunning = s => s.status?.type === 'running' + const AssistantMessage = () => { const { t } = useI18n() - const isRunning = useMessage(s => s.status?.type === 'running') + const isRunning = useMessage(selectIsRunning)packages/cozy-search/src/components/TwakeKnowledges/styles.styl (2)
43-93: Excessive!importantusage and generic class names risk style collisions.There are ~15
!importantdeclarations, suggesting these styles are fighting the component library's specificity. Additionally, generic names like.badge,.section-header,.nested-itemcould easily clash with other styles in the app.Consider:
- Namespacing classes (e.g.,
.twakeKnowledge-badge,.twakeKnowledge-sectionHeader) to avoid collisions.- Investigating whether higher specificity selectors or component-level style overrides could replace
!important.
30-35: WebKit-only scrollbar styling — Firefox users get default scrollbar.The custom scrollbar is only styled with
-webkit-scrollbarpseudo-elements. Consider addingscrollbar-width: thinandscrollbar-colorfor Firefox support.♻️ Proposed fix
.source-panel-content flex 1 overflow-y auto padding 0 8px + scrollbar-width thin + scrollbar-color var(--dividerColor) transparent &::-webkit-scrollbar width 6pxpackages/cozy-search/src/components/Assistant/styles.styl (1)
23-36: Consider reducing!importantusage.There are six
!importantannotations across.menu-itemand.menu-item-icon-button. While sometimes necessary to override library (cozy-ui) defaults, heavy reliance on!importantmakes future style overrides difficult. Consider increasing specificity or using cozy-ui's own class overrides where possible.packages/cozy-search/src/components/AssistantProvider.jsx (1)
133-168: Context is growing large — consider splitting concerns.The
AssistantContextnow exposes 15+ values. Every state change (e.g., togglingisOpenSearchConversation) triggers re-renders for all consumers, even those that only useselectedAssistantId. Consider splitting into separate contexts (e.g.,AssistantUIContextfor UI state,AssistantKnowledgeContextfor knowledge panel state) if performance becomes an issue.packages/cozy-search/src/components/TwakeKnowledges/MailKnowledge.jsx (1)
143-175: Hard-coded data: add a TODO comment for replacement with real data.Similar to
DriveKnowledge.jsx,inboxItemsandstarredItemsare temporary scaffolding. Add a// TODO: replace with real data sourcecomment to track this, and plan for data fetching, error handling, and loading states.Based on learnings: "In packages/cozy-search/src/components/TwakeKnowledges/DriveKnowledge.jsx, the hard-coded myDriveItems and sharedItems arrays are temporary scaffolding and will be replaced with real data later."
packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx (1)
112-114: Suppress the exhaustive-deps lint warning for this intentional mount-only effect.
initialMessagesis intentionally captured only at mount time, but ESLint'sreact-hooks/exhaustive-depsrule will flag it. Add a suppression comment to document the intent.Proposed fix
useEffect(() => { messagesIdRef.current = initialMessages.map(m => m.id).filter((id): id is string => !!id) + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally capture initial value only }, [])packages/cozy-search/src/components/Views/AssistantDialog.jsx (1)
53-60: Consider extracting inline styles to a CSS class.The inline
styleobject ondialogContentwill create a new object reference on every render. Since you already importstyles.styl, moving these declarations into a stylesheet class would be more consistent with the rest of the component.packages/cozy-search/src/components/adapters/StreamBridge.ts (2)
74-94: Missingreturn()method on the async iterator.The
AsyncIterableIteratorprotocol optionally supportsreturn()for early termination (e.g.,breakfromfor-await-of). Without it, breaking out of a consuming loop won't signal the bridge to clean up the stream.Proposed addition
[Symbol.asyncIterator]() { return this - } + }, + return(): Promise<IteratorResult<string>> { + isDone = true + return Promise.resolve({ value: undefined as unknown as string, done: true }) + }
63-72: Queued chunks are discarded on error.When
error()is called,isDoneis set totrueand the error is stored. Sincenext()checkserrorbefore draining the queue (line 77), any buffered chunks are lost. If partial delivery matters, consider draining the queue before rejecting. If discarding is intentional, a brief comment would clarify the design choice.packages/cozy-search/src/actions/share.jsx (2)
9-24:makeComponentis duplicated acrossshare.jsx,rename.jsx, anddelete.jsx.All three action files define nearly identical
makeComponentfactories (differing only indisplayNameand optional class names likeu-errorfor delete). Consider extracting a shared helper, e.g.makeActionComponent(displayName, className), to reduce duplication.
36-36:actionis a no-op — add a TODO comment.The share action handler does nothing. If this is intentional WIP, a
// TODO: implement share actioncomment would signal intent to future maintainers.packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx (1)
90-127: Consolidate repeatedswitchstatements into a config map.
getTitle,getDescription, andgetIconall switch on the sameopenedKnowledgePanelvalue. A single lookup object would reduce repetition and make adding new panel types easier.Example refactor
+const PANEL_CONFIG = { + drive: { icon: TDrive, titleKey: 'title_drive', descKey: 'desc_drive' }, + mail: { icon: TMail, titleKey: 'title_mail', descKey: 'desc_mail' }, + chat: { icon: TChat, titleKey: 'title_chat', descKey: 'desc_chat' } +} + const TwakeKnowledgePanel = ({ onClose }) => { // ... + const config = PANEL_CONFIG[openedKnowledgePanel] + const title = config ? t(`assistant.twake_knowledges.${config.titleKey}`) : t('assistant.twake_knowledges.title_default') + const description = config ? t(`assistant.twake_knowledges.${config.descKey}`) : '' + const icon = config?.icon ?? nullpackages/cozy-search/src/components/Conversations/ConversationListItem.jsx (1)
35-35:makeActionsis called on every render.This creates new action objects each render cycle, which could cause unnecessary re-renders of
ActionsMenu. Consider memoizing withuseMemo.Proposed fix
- const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])Add
useMemoto the React import:-import React, { useState, useRef } from 'react' +import React, { useState, useRef, useMemo } from 'react'packages/cozy-search/src/components/Conversations/Conversation.jsx (1)
38-43: Nit: object shorthand for component props.
UserMessage: UserMessageandAssistantMessage: AssistantMessagecan use ES6 shorthand.Proposed fix
<ThreadPrimitive.Messages components={{ - UserMessage: UserMessage, - AssistantMessage: AssistantMessage + UserMessage, + AssistantMessage }} />packages/cozy-search/package.json (1)
17-18:classnamesandclsxserve the same purpose — consider consolidating.Both libraries do conditional class-name joining.
clsxwas likely pulled in alongside theshadcn/assistant-uipatterns whileclassnameswas already in use. Keeping both adds unnecessary bundle weight and creates inconsistency in which utility developers reach for.Pick one (typically
clsxsince it's smaller and already required byclass-variance-authority) and migrate theclassnamescall sites.
| <SearchBar | ||
| className="u-mb-2" | ||
| placeholder={t('assistant.search_conversation.placeholder')} | ||
| size="medium" | ||
| /> |
There was a problem hiding this comment.
SearchBar is rendered but not wired to any filtering logic.
The search bar has no value, onChange, or connected filter. Users will type into it with no effect. If this is intentional WIP, add a TODO comment; otherwise wire it up.
🤖 Prompt for AI Agents
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` around
lines 58 - 62, The SearchBar is rendered but not wired to state or filtering
logic; update SearchConversation.jsx to manage a search query state (e.g.,
useState for query) and pass it into the SearchBar via value and onChange (or
onInput) so user input updates the query, then use that query to filter the
conversations list (or invoke a provided callback prop like onSearchChange)
before rendering results; if this is intentionally a WIP, add a clear TODO
comment next to the SearchBar noting that value/onChange and filtering must be
implemented.
There was a problem hiding this comment.
Temporary skip the functions for search bar
There was a problem hiding this comment.
@lethemanh, understood! I've noted that the SearchBar functionality is temporarily deferred for now.
✏️ Learnings added
Learnt from: lethemanh
Repo: linagora/cozy-libs PR: 2925
File: packages/cozy-search/src/components/Search/SearchConversation.jsx:49-55
Timestamp: 2026-02-12T03:37:51.940Z
Learning: In `packages/cozy-search/src/components/Search/SearchConversation.jsx`, the SearchBar component (around lines 58-62) is intentionally rendered without value/onChange/filtering logic as a temporary placeholder. The search functionality is planned to be implemented later.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lethemanh
Repo: linagora/cozy-libs PR: 2911
File: packages/cozy-search/src/components/Views/CreateAssistantDialog.jsx:52-67
Timestamp: 2026-01-24T17:24:53.584Z
Learning: In `packages/cozy-search/src/components/Views/CreateAssistantDialog.jsx` and `EditAssistantDialog.jsx`, the dialog closing after submission is handled by the `useAssistantDialog` hook's `handleNext` function, which calls `onClose()` after awaiting the `onSubmit` callback passed to it. The local `onSubmit` functions in these dialogs do not need to call `onClose()` directly.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cozy-search/package.json (1)
51-52:⚠️ Potential issue | 🟠 MajorDev React versions (16.x) conflict with peer dependency requirement (≥18.0.0).
Lines 84–85 require
react >=18.0.0andreact-dom >=18.0.0, but the dev dependencies pin React 16.12.0 and React-DOM 16.13.0. This means tests and local development run against a React version that the library explicitly declares as unsupported. Several new dependencies (@assistant-ui/react,zustandv5) also require React 18+, so tests will likely fail or behave incorrectly.Upgrade both to at least 18.0.0:
Proposed fix
- "react": "16.12.0", - "react-dom": "16.13.0", + "react": "18.0.0", + "react-dom": "18.0.0",You may also need to upgrade
@testing-library/react(v10 → v13+) and drop@testing-library/react-hooks(merged into@testing-library/react≥13.1).
🤖 Fix all issues with AI agents
In `@packages/cozy-search/package.json`:
- Around line 84-85: The peerDependency change in package.json raises "react"
and "react-dom" to ">=18.0.0" which is a breaking change for consumers on React
16/17; update the package version from "0.16.2" to a new semver-major (or
appropriate 0.x increment per your release policy) and add a migration note in
the release changelog/README explaining the React 18 requirement and migration
steps; ensure the version field in package.json (currently "0.16.2") is bumped
and that the changelog/release notes mention the peer dependency change and any
required consumer migration steps.
In `@packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts`:
- Around line 118-121: The rendered message currently prepends a hardcoded
English "Error: " before the translated string in the yielded content; update
the yield in CozyRealtimeChatAdapter (the block that returns content: [{ type:
'text', text: ... }]) to remove the hardcoded prefix or move it into your i18n
keys so the whole message is translated (e.g., replace the template string with
a single t(...) call for a fully localized key like 'assistant.error' or use a
new key that includes the prefix). Ensure the change targets the yield that
builds the error content object and only uses t(...) for the full message.
In `@packages/cozy-search/src/components/Search/helpers.js`:
- Line 8: The early return `if (!conversations) return {}` returns a different
shape than the normal path and causes callers expecting `{ today: [], older: []
}` to crash; change the early return in the helper to return `{ today: [],
older: [] }` instead so functions like `groups.today.map(...)` are safe, and
ensure the function that processes `conversations` (the same helper)
consistently returns that shape for all code paths.
🧹 Nitpick comments (18)
packages/cozy-search/src/types.d.ts (1)
50-56:useI18ndeclaration omits additional properties returned by the hook.From
packages/twake-i18n/src/useExtendI18n.jsx, the real hook also returnspolyglot(and potentially other fields). The current declaration is fine if onlytandlangare used in this package, but consider adding an index signature or a comment noting the partial typing so future consumers aren't surprised.packages/cozy-search/src/components/Search/helpers.js (1)
23-23:Date.now()fallback silently bins conversations with missingupdatedAtinto "today".If a conversation has no
cozyMetadata.updatedAt, the fallback toDate.now()will always place it in the "today" group regardless of when it was actually created. If this is intentional, a brief comment would help future readers; otherwise, consider falling back toconv.cozyMetadata?.createdAtor placing such items in "older" as a safer default.packages/cozy-search/package.json (1)
17-18: Redundant class-name utilities: bothclassnamesandclsxare listed.These libraries serve the same purpose (conditional class string joining).
clsxis smaller and is the one expected by theshadcn/tailwind-mergepattern (cnhelper typically wrapsclsx+twMerge). Consider migrating existingclassnamesusage toclsxand dropping theclassnamesdependency to avoid shipping two libraries that do the same thing.packages/cozy-search/src/components/TwakeKnowledges/styles.styl (2)
30-35: WebKit-only scrollbar styling — no effect on Firefox.
::-webkit-scrollbarrules are not supported by Firefox. If consistent scrollbar appearance matters, consider addingscrollbar-width: thinandscrollbar-colorfor Firefox support.Proposed addition
.source-panel-content flex 1 overflow-y auto padding 0 8px + scrollbar-width thin + scrollbar-color var(--dividerColor) transparent &::-webkit-scrollbar width 6px
46-51: Heavy!importantusage throughout the stylesheet.Many rules use
!important(lines 46-47, 50-51, 54-60, 63-64, 84-85, 88, 91-93). This is common when overriding cozy-ui/MUI defaults, but it makes future style adjustments harder. Consider whether some of these can be replaced with more specific selectors to reduce!importantreliance over time.packages/cozy-search/src/actions/rename.jsx (2)
9-24:makeComponentis duplicated across rename, share, and delete action files.The
makeComponenthelper is nearly identical inrename.jsx,share.jsx, anddelete.jsx(the only variation beingdelete.jsxaddingclassName="u-error"). Consider extracting it to a shared utility to reduce duplication.
36-36: No-opactionhandler — consider adding a TODO.The
actioncallback is an empty function. If this is intentional WIP, a// TODO: implement rename actioncomment would help other developers understand the incomplete state.packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx (1)
59-127: Four parallelswitchstatements on the same key could be a config map.
renderContent,getTitle,getDescription, andgetIconall switch onopenedKnowledgePanel. A config object would reduce repetition and make adding new panel types a one-line change.Example consolidation
+const PANEL_CONFIG = { + drive: { + titleKey: 'assistant.twake_knowledges.title_drive', + descKey: 'assistant.twake_knowledges.desc_drive', + icon: TDrive, + Component: DriveKnowledge + }, + mail: { + titleKey: 'assistant.twake_knowledges.title_mail', + descKey: 'assistant.twake_knowledges.desc_mail', + icon: TMail, + Component: MailKnowledge + }, + chat: { + titleKey: 'assistant.twake_knowledges.title_chat', + descKey: 'assistant.twake_knowledges.desc_chat', + icon: TChat, + Component: ChatKnowledge + } +}Then inside the component:
const config = PANEL_CONFIG[openedKnowledgePanel] // use config.titleKey, config.icon, config.Component, etc.packages/cozy-search/src/components/Views/AssistantDialog.jsx (1)
53-60: Consider extracting the inline style to a constant.A new object is created on every render. Since the values are static, hoist them outside the component or use
useMemo.♻️ Suggested refactor
+const dialogContentStyle = { + display: 'flex', + flexDirection: 'column', + flexGrow: 1, + padding: 0 +} + const AssistantDialog = () => { ... dialogContent: { - style: { - display: 'flex', - flexDirection: 'column', - flexGrow: 1, - padding: 0 - } + style: dialogContentStyle }packages/cozy-search/src/components/adapters/StreamBridge.ts (2)
74-94: Missingreturn()method on the async iterator.When a
for-await-ofloop is broken (e.g., viabreak,throw, orreturn), the engine callsiterator.return()to signal cleanup. Without it, the stream remains open until the adapter explicitly callscleanup(). Addingreturn()makes the iterator self-cleaning and more robust if consumed outside the adapter.♻️ Suggested addition
iterator: { next: (): Promise<IteratorResult<string>> => new Promise((resolve, reject) => { // ... existing logic }), + return: (): Promise<IteratorResult<string>> => { + if (!isDone) { + isDone = true + if (resolveNext) { + resolveNext({ value: undefined as unknown as string, done: true }) + resolveNext = null + rejectNext = null + } + } + return Promise.resolve({ value: undefined as unknown as string, done: true }) + }, [Symbol.asyncIterator]() { return this } }
16-18: Single globalcleanupCallbackshared across all conversations.
setCleanupCallbackstores one callback for the entire bridge. If multiple conversations stream concurrently,cleanup(convA)will invoke the same callback that was perhaps intended forconvB. If multi-conversation streaming is a future possibility, consider making the callback per-conversation (e.g., stored in theStreamController).packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts (1)
90-95: Abort is only checked between chunks — not while waiting.If the stream is blocked waiting for the next chunk in
next(), theabortSignal.abortedcheck at line 91 won't run until a chunk arrives. For long pauses this means the UI appears unresponsive to cancellation. Registering anabortevent listener that callsstreamBridge.cleanup(conversationId)would make cancellation immediate.♻️ Sketch
+ const onAbort = () => { + streamBridge.cleanup(conversationId) + } + abortSignal?.addEventListener('abort', onAbort, { once: true }) + try { await client.stackClient.fetchJSON(...) // ... for-await loop (remove the inner abortSignal check) ... } catch (error) { // ... + } finally { + abortSignal?.removeEventListener('abort', onAbort) }packages/cozy-search/src/components/Views/AssistantView.jsx (1)
14-57: Extract shared dialog rendering logic.The dialogs section (lines 33–54) is identically duplicated in
AssistantDialog.jsx(lines 74–95). Consider extracting into a shared component likeAssistantDialogsOverlayto reduce duplication, allowing both views to render the three conditional dialogs without repeating the same markup and logic.packages/cozy-search/src/components/Conversations/ConversationListItem.jsx (2)
79-83: Inconsistent optional chaining onmessages.length.Line 52 uses
conversation.messages?.length(with optional chaining) while line 81 usesconversation.messages.length(without). Although the outer?.[]short-circuits safely whenmessagesis nullish, the inconsistency is confusing and fragile if refactored later.Proposed fix
<Typography className="u-db u-ellipsis u-mb-half u-fz-xsmall"> { - conversation.messages?.[conversation.messages.length - 1] + conversation.messages?.[conversation.messages?.length - 1] ?.content } </Typography>
35-35:makeActionsis recomputed on every render.
makeActions([share, rename, remove], { t })creates a new actions array each render. Consider memoizing withuseMemokeyed ontto avoid unnecessary work and stable references forActionsMenu.Proposed fix
+import React, { useState, useRef, useMemo } from 'react' ... - const actions = makeActions([share, rename, remove], { t }) + const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t])packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx (1)
22-38: Significant code duplication withConversationListItem.jsx.Both components share identical state management,
toggleMenulogic, action creation, imports, and i18n setup. Consider extracting a shared hook (e.g.,useConversationItemMenu) to DRY up the common logic.// e.g., useConversationItemMenu.js export const useConversationItemMenu = () => { const { t, lang } = useI18n() const [isMenuOpen, setIsMenuOpen] = useState(false) const anchorRef = useRef(null) const toggleMenu = e => { e?.stopPropagation(); setIsMenuOpen(!isMenuOpen) } const actions = useMemo(() => makeActions([share, rename, remove], { t }), [t]) return { t, lang, isMenuOpen, anchorRef, toggleMenu, actions } }packages/cozy-search/src/components/Conversations/ConversationBar.jsx (2)
71-94:IconButtonhas noonClick— clicks on its padding area are inert.The
onClickhandlers (onCancelon line 79,handleSendon line 91) are bound to the innerButton(rendered as<div>). The outerIconButtonhas noonClick, so clicking its padding (outside the inner div) does nothing. Consider movingonClickto theIconButtonor ensuring the inner div fills the entire clickable area.
29-33: Pass the ref object rather thanref.current, or attach the listener inuseEffectfor clarity.
useEventListener(inputRef.current, 'input', ...)capturesundefinedduring the initial render since the ref hasn't been assigned yet. The listener only gets attached on a subsequent re-render wheninputRef.currentis populated. While this typically works because the component re-renders on value changes, it's fragile and relies on external re-renders for correctness.Better patterns:
- Pass the ref object itself and let
useEventListenerhandle dereferencing internally, or- Move the listener setup into a
useEffectthat depends on the input element being available
lethemanh
force-pushed
the
new-assistant-ui
branch
3 times, most recently
from
cf09f50 to
b26247b
Compare
lethemanh
force-pushed
the
new-assistant-ui
branch
from
64ad3c8 to
83148b6
Compare
|
Last translation issue :'( 
|
I've added all the missing keys |
lethemanh
force-pushed
the
new-assistant-ui
branch
from
83148b6 to
4f9d65c
Compare
| </div> | ||
|
|
||
| <div className="u-ov-auto u-flex-auto"> | ||
| {groupedConversations.today?.length > 0 && ( |
There was a problem hiding this comment.
I don't get why we group conversations by today vs older ?
| msg.content?.toLowerCase().includes(lowerQuery) | ||
| ) | ||
| ) | ||
| }, [conversations, query]) |
There was a problem hiding this comment.
If I understand correctly, the actual search relies on the .includes filter. But this is quite sub-optimal, because:
- it searches in all conversations, with no index. If we have plenty of content, this will be quite slow
- I don't think we have all conversations loaded in memory, as this is currently paginated at 50 conversations
- it will only work if the query is a subset of a conversation, but this will miss plenty of case, like if I have a conversation with "history of football" and I search "history football", it will fails.
@Crash-- We have fuse search for contacts, and I think we wanted to have similar search for admin panel ? Can we reuse it?
| const { data: conversations } = useQuery( | ||
| conversationsQuery.definition, | ||
| conversationsQuery.options | ||
| ) |
There was a problem hiding this comment.
Here we load the 50 last conversations, which is ok, but what if I want more? We need to have some pagination to load more conversations
cc @zatteo
lethemanh
force-pushed
the
new-assistant-ui
branch
from
4f9d65c to
4addbb8
Compare
|
@zatteo @paultranvan we will merge this PR and all the issues from the feedbacks will be resolved in another PR |
BREAKING CHANGE: You need to upgrade react >= 18.0.0 and react-dom >= 18.0.0
lethemanh
force-pushed
the
new-assistant-ui
branch
from
4addbb8 to
72c13a1
Compare
|
solid pr, can be improved a bit stuff we can fix
some concerns
deps
Missing
Nits
|
|
Thanks for the review @rezk2ll. We wanted to merge this large PR and fix some already identified issues in future PR to avoid a never ending PR which has already 100+ comments. Part of this PR is only decorative UI as you saw. It is intentional. @lethemanh you can add a word about it in PR description for future reader. That said, my two cent:
What do you think guys? |
|
@rezk2ll thank for your comments, I have some feedback as below:
These will be fixed in the next PR.
These 3 actions are not ready in the backend, so I put them under the flag
Currently, we don't have a specific spec for conversation name so it's temporary for now.
Currently, the backend does not support chat with the selected assistant, but in the next PR, each conversation will be stored with the selected assistant.
For
Currently, the search action is not available, it's UI only.
As @zatteo 's comment. |
4 participants
Result:
Screen.Recording.2026-02-12.at.17.49.01.mov
Related to:
linagora/cozy-home#2351
Feature flags:
cozy.source-knowledge.enabled: Enable source knowledge.cozy.search-conversation.enabled: Enable search conversation.cozy.top-bar-in-assistant.enabled: Enable top bar in assistant view.Summary by CodeRabbit
New Features
Chores
Documentation