Resolve feedbacks of new assistant#2937
Merged
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:
WalkthroughAdds assistant-awareness and mobile-responsive UI across cozy-search: assistant avatars replace icons in conversation lists; AssistantSelection accepts a new 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)
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 |
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
from
e9c6d4e to
7cdfa98
Compare
lethemanh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-search/src/components/Conversations/ConversationComposer.jsx`:
- Around line 63-65: AssistantSelection is being passed disabled from
ConversationComposer but ignores it; update AssistantSelection (the component)
to accept and destructure a disabled prop (e.g., function AssistantSelection({
className, disabled }) ), give it a sensible default (false), and apply it to
the interactive elements inside (disable the select/button elements, set
aria-disabled={disabled}, and/or set tabIndex={disabled ? -1 : 0} and a disabled
CSS class or pointer-events style) so the selector becomes non-interactive when
disabled; also update its propTypes or TypeScript props to include disabled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/cozy-search/src/components/Conversations/ConversationComposer.jsxpackages/cozy-search/src/components/Conversations/ConversationListItem.jsxpackages/cozy-search/src/components/Conversations/ConversationListItemWider.jsxpackages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsxpackages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.tspackages/cozy-search/src/components/queries.js
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
from
7cdfa98 to
ac56259
Compare
zatteo
reviewed
Mar 3, 2026
zatteo
reviewed
Mar 3, 2026
| } | ||
| } | ||
|
|
||
| export default useFetchConversations |
Member
There was a problem hiding this comment.
If you use useQuery, it exposes already an hasMore and fetchMore. Did you try with them? Normally all this pagination fetching logic should work almost out of the box with cozy-client.
Contributor
Author
There was a problem hiding this comment.
If you remember, useQuery does not return reference data. In this case, I need reference data to get the selected assistant.
Btw, I will add comment into this hook to explain more detail.
Member
There was a problem hiding this comment.
Good idea. You can link the cozy-client issue also to explain why you had to do this and you could not use useQuery.
Contributor
There was a problem hiding this comment.
So you mean this issue? linagora/cozy-client#1083
Because in this case, the .include does not work, but we do have the relationship in the response (but not the assistant content).
That being said, why do we need to have the assistant data at this point?
zatteo
reviewed
Mar 3, 2026
zatteo
reviewed
Mar 3, 2026
zatteo
reviewed
Mar 3, 2026
| if (!query) return conversations | ||
| const debouncedFetchConversations = useMemo( | ||
| () => | ||
| debounce(async value => { |
Member
There was a problem hiding this comment.
Why do we need the debounce here?
Contributor
Author
There was a problem hiding this comment.
That's because I want to make change to the query when the user pauses their typing.
zatteo
reviewed
Mar 3, 2026
| background var(--paleGrey) | ||
|
|
||
| &--dark | ||
| background var(--grey900) |
Member
There was a problem hiding this comment.
When you finish responsive view of assistant, maybe we can take a look together to Figma + current code + how it looks with current code to see if there are alternatives to these --light and --dark.
We rarely need to use them so I wonder if there are alternatives.
Contributor
Author
There was a problem hiding this comment.
In light mode uses the color #F3F6F9 but in dark mode it looks like this
That's why I have to set a different color in dark mode.
Member
There was a problem hiding this comment.
@JF-Cozy we have this Figma mockup with a "message card" that correspond to the user message. What is the recommended way to implement it? Especially since:
- there is no obvious cozy-ui components to use
- we need to support dark mode
Contributor
There was a problem hiding this comment.
How this message works? Is it like an Alert? Or it's always there is that case and we "just" have to create a div somehow?
Contributor
There was a problem hiding this comment.
Ok has discussed: no component in cozy-ui to do this, here they are all css var https://github.com/cozy/cozy-ui/blob/master/stylus/settings/themes/light-normal.styl you can use
or Box with class/style and use backgroundColor --defaultBackgroundColor, and Typography for the text.
@lethemanh please do not use non semantic color, use only var from https://github.com/cozy/cozy-ui/blob/master/stylus/settings/themes/light-normal.styl and avoid as much as possible creating custom css or style. Take a deep look to cozy-ui, but also Mui v4 documentation to find right component to use for your needs 👍 And if there is none, talk to designers to see if you can change something in the mockup.
Contributor
Author
There was a problem hiding this comment.
@JF-Cozy the color #F3F6F9 is --grey100 in cozy-ui, but when switching to dark mode, the color looks so bright
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
2 times, most recently
from
310268f to
a6597e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/cozy-search/src/components/Conversations/ConversationListItem.jsx (1)
26-26: Consider adding a fallback for the theme value.If
useCozyTheme()returns an undefinedtype, the constructed class name at line 36 would beconversation-list-item--selected--undefined, which won't match any defined style in the stylesheet (only--lightand--darkvariants exist).🛡️ Proposed defensive fallback
- const { type: theme } = useCozyTheme() + const { type: theme = 'light' } = useCozyTheme()Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx` at line 26, The theme value from useCozyTheme() can be undefined; ensure the className uses a safe fallback (e.g., 'light') by deriving a safeTheme before building class names so you never produce `--undefined`; update the code around useCozyTheme() (the const { type: theme } = useCozyTheme() usage) and the className construction that uses `conversation-list-item--selected--${theme}` to instead use a validated value (for example default theme when theme is falsy) so only `--light`/`--dark` variants are emitted.packages/cozy-search/src/components/queries.js (1)
71-71: Makedefinitionresilient to empty invocations.Consider defaulting the whole argument object to avoid a destructuring crash if
definition()is called without params.Suggested fix
- definition: ({ offset = 0, query = {} }) => + definition: ({ offset = 0, query = {} } = {}) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/queries.js` at line 71, The definition function can throw when called with no arguments; update its signature so the parameter object itself defaults to an empty object (i.e., change the parameter from ({ offset = 0, query = {} }) to ({ offset = 0, query = {} } = {})) so destructuring is safe when definition() is invoked without params; ensure any callers still work with the new defaulting behavior.packages/cozy-search/src/hooks/useFetchConversations.js (1)
31-34: Consider documenting the custom pagination approach.Per the past review discussion, this hook exists because
useQuerydoesn't return reference data needed for the assistant. Adding a brief comment explaining this design decision would help future maintainers understand why standard cozy-client pagination wasn't used.📝 Suggested documentation
+/** + * Note: We implement custom pagination here instead of using useQuery's built-in + * hasMore/fetchMore because useQuery doesn't return reference data (included). + * We need the included assistant data to augment conversations. + * See: https://github.com/cozy/cozy-client/issues/XXXX + */ const useFetchConversations = ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/hooks/useFetchConversations.js` around lines 31 - 34, Add a concise comment inside the useFetchConversations hook explaining why custom pagination is implemented: note that standard cozy-client/useQuery pagination was not used because the assistant needs reference data (not returned by useQuery) so the hook calls conversationsQuery.definition({ offset, query: fetchQuery }) directly; place this comment near the conversationsQuery.definition call and mention the specific need for reference data and the offset-based pagination approach so future maintainers understand the design decision.packages/cozy-search/src/components/Search/SearchConversation.jsx (1)
136-136: Verify NotFoundConversation display logic.
NotFoundConversationrenders whenconversations?.length === 0, which includes the initial load state before any data is fetched. Is this the intended UX, or should it only appear after a search returns no results?If it should only show after a search:
💡 Conditional rendering suggestion
- {conversations?.length === 0 && <NotFoundConversation />} + {searchStr && conversations?.length === 0 && <NotFoundConversation />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` at line 136, The NotFoundConversation component is currently shown whenever conversations?.length === 0 (including initial load); update the conditional in SearchConversation.jsx so NotFoundConversation only renders after an actual search has completed and returned no results — for example replace the condition with a check like !isLoading && searchQuery?.trim() !== '' && conversations?.length === 0, or introduce/consume a hasSearched/searchSubmitted boolean and render NotFoundConversation when hasSearched && !isLoading && conversations?.length === 0; reference the conversations state/prop, the existing isLoading (or loading) flag, and the searchQuery/hasSearched signal in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-search/src/actions/rename.jsx`:
- Around line 35-38: The rename action currently exposes a clickable-but-noop
menu item; change its displayCondition to gate the entry behind a
backend-support check (e.g. call an existing feature flag or helper like
isRenameSupported() or backendSupportsRename) so it returns false until the
backend endpoint exists, leaving the action: () stub unchanged; update the
displayCondition in the rename action (displayCondition and action in
packages/cozy-search/src/actions/rename.jsx) to only show when the support
flag/helper indicates rename is available.
In `@packages/cozy-search/src/components/Assistant/AssistantContainer.jsx`:
- Line 22: The theme value from useCozyTheme() can be undefined or non-exact
causing the styles lookup in AssistantContainer to fail; update the code around
the useCozyTheme() consumer (the theme variable) to normalize/fallback to a
known value (e.g., 'light' or 'dark') before using it for style keys—for
example, compute a safeTheme = (type === 'dark' ? 'dark' : 'light') or use a
whitelist check and then use safeTheme wherever the styles[theme] lookup happens
(references: useCozyTheme, theme variable and the styles lookup in
AssistantContainer.jsx).
In `@packages/cozy-search/src/components/Assistant/AssistantSelection.jsx`:
- Around line 20-21: AssistantSelection currently allows the dropdown to stay
open when the new disabled prop is true; add a guard in the click handler(s)
(e.g., the onClick/toggleDropdown or handleAssistantClick functions) to ignore
clicks when disabled, and add an effect that watches the disabled prop and calls
the same close function (e.g., setOpen(false) or closeDropdown) to auto-close
the menu when disabled becomes true; reference AssistantSelection, the disabled
prop, the dropdown open state variable (isOpen/setIsOpen or similar), and the
click handler(s) to locate where to add the guard and auto-close behavior.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 20-24: The query state is initialized to undefined which causes
useFetchConversations' useEffect comparison (previousQueryRef.current vs query
using isEqual) to treat mount as unchanged and skip fetchConversations; fix by
either initializing query in SearchConversation.jsx to an empty object (change
useState(undefined) to useState({})) or modify useFetchConversations' effect to
treat the first render specially (check if previousQueryRef.current is undefined
and force fetchConversations), referencing the query/setQuery state in
SearchConversation.jsx and the previousQueryRef.current, isEqual, and
fetchConversations logic inside useFetchConversations.
In `@packages/cozy-search/src/hooks/useFetchConversations.js`:
- Around line 15-18: The hook useFetchConversations currently accepts a limit
parameter (defaulting to FETCH_CONVERSATIONS_LIMIT) but does not pass it into
conversationsQuery.definition; update the call to conversationsQuery.definition
(in useFetchConversations) to include limit along with offset and query so the
constructed query uses the provided limit, or if buildChatConversationsQuery
hardcodes the limit, remove the unused limit parameter and default to the
internal behavior; reference useFetchConversations,
conversationsQuery.definition, buildChatConversationsQuery and
FETCH_CONVERSATIONS_LIMIT when making the change.
In `@packages/cozy-search/src/locales/en.json`:
- Line 35: Update the user-facing string for the "load_more" key in the en.json
locale so it uses standard CTA text: replace "Fetch more" with "Load more"
(i.e., change the value of the "load_more" key in the locales/en.json file to
"Load more").
---
Nitpick comments:
In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx`:
- Line 26: The theme value from useCozyTheme() can be undefined; ensure the
className uses a safe fallback (e.g., 'light') by deriving a safeTheme before
building class names so you never produce `--undefined`; update the code around
useCozyTheme() (the const { type: theme } = useCozyTheme() usage) and the
className construction that uses `conversation-list-item--selected--${theme}` to
instead use a validated value (for example default theme when theme is falsy) so
only `--light`/`--dark` variants are emitted.
In `@packages/cozy-search/src/components/queries.js`:
- Line 71: The definition function can throw when called with no arguments;
update its signature so the parameter object itself defaults to an empty object
(i.e., change the parameter from ({ offset = 0, query = {} }) to ({ offset = 0,
query = {} } = {})) so destructuring is safe when definition() is invoked
without params; ensure any callers still work with the new defaulting behavior.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Line 136: The NotFoundConversation component is currently shown whenever
conversations?.length === 0 (including initial load); update the conditional in
SearchConversation.jsx so NotFoundConversation only renders after an actual
search has completed and returned no results — for example replace the condition
with a check like !isLoading && searchQuery?.trim() !== '' &&
conversations?.length === 0, or introduce/consume a hasSearched/searchSubmitted
boolean and render NotFoundConversation when hasSearched && !isLoading &&
conversations?.length === 0; reference the conversations state/prop, the
existing isLoading (or loading) flag, and the searchQuery/hasSearched signal in
your change.
In `@packages/cozy-search/src/hooks/useFetchConversations.js`:
- Around line 31-34: Add a concise comment inside the useFetchConversations hook
explaining why custom pagination is implemented: note that standard
cozy-client/useQuery pagination was not used because the assistant needs
reference data (not returned by useQuery) so the hook calls
conversationsQuery.definition({ offset, query: fetchQuery }) directly; place
this comment near the conversationsQuery.definition call and mention the
specific need for reference data and the offset-based pagination approach so
future maintainers understand the design decision.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b07a3a44-7693-46a0-a6da-feded9933b15
📒 Files selected for processing (27)
packages/cozy-search/src/actions/delete.jsxpackages/cozy-search/src/actions/rename.jsxpackages/cozy-search/src/actions/share.jsxpackages/cozy-search/src/components/Assistant/AssistantContainer.jsxpackages/cozy-search/src/components/Assistant/AssistantSelection.jsxpackages/cozy-search/src/components/Assistant/styles.stylpackages/cozy-search/src/components/Conversations/ConversationComposer.jsxpackages/cozy-search/src/components/Conversations/ConversationListItem.jsxpackages/cozy-search/src/components/Conversations/ConversationListItemWider.jsxpackages/cozy-search/src/components/Conversations/styles.stylpackages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsxpackages/cozy-search/src/components/Messages/AssistantMessage.jsxpackages/cozy-search/src/components/Messages/UserMessage.jsxpackages/cozy-search/src/components/Messages/styles.stylpackages/cozy-search/src/components/Search/NotFoundConversation.jsxpackages/cozy-search/src/components/Search/SearchConversation.jsxpackages/cozy-search/src/components/Sidebar/index.jsxpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsxpackages/cozy-search/src/components/TwakeKnowledges/styles.stylpackages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.tspackages/cozy-search/src/components/constants.jspackages/cozy-search/src/components/queries.jspackages/cozy-search/src/hooks/useFetchConversations.jspackages/cozy-search/src/locales/en.jsonpackages/cozy-search/src/locales/fr.jsonpackages/cozy-search/src/locales/ru.jsonpackages/cozy-search/src/locales/vi.json
💤 Files with no reviewable changes (1)
- packages/cozy-search/src/components/TwakeKnowledges/styles.styl
✅ Files skipped from review due to trivial changes (1)
- packages/cozy-search/src/actions/delete.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx
- packages/cozy-search/src/components/Conversations/ConversationComposer.jsx
- packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts
There was a problem hiding this comment.
Actionable comments posted: 6
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/AssistantSelectionItem.jsx (1)
89-112:⚠️ Potential issue | 🟡 MinorHardcoded English aria-labels may need i18n.
Given that the PR introduces new localization keys elsewhere, the hardcoded
"Edit assistant"and"Delete assistant"strings may be inconsistent with the i18n approach. Consider using translated strings for accessibility labels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 89 - 112, The aria-label values on the IconButton elements in AssistantSelectionItem.jsx are hardcoded English strings ("Edit assistant" and "Delete assistant"); replace them with i18n translations (use your project's translation hook/function—e.g., t('assistant.edit') and t('assistant.delete') or the existing keys convention) so accessibility labels are localized, and update or add the corresponding localization keys; ensure the same change is applied where IconButton with onClick handlers handleEdit and handleDelete are used.
♻️ Duplicate comments (1)
packages/cozy-search/src/components/Assistant/AssistantSelection.jsx (1)
22-24:⚠️ Potential issue | 🟠 MajorEnsure
disabledactually prevents/ends assistant switching.
disabledis passed toChips, but Line 40 still opens the menu unconditionally, and an already-open menu stays interactive whendisabledbecomestrue.💡 Proposed fix
-import React, { useState, useRef } from 'react' +import React, { useEffect, useState, useRef } from 'react' @@ const AssistantSelection = ({ className, disabled }) => { @@ + useEffect(() => { + if (disabled) { + setOpen(false) + } + }, [disabled]) + const handleClick = () => { + if (disabled) return setOpen(true) } @@ - {open && ( + {open && !disabled && ( <ActionsMenuAlso applies to: 40-42, 75-77, 80-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelection.jsx` around lines 22 - 24, The AssistantSelection component currently still opens and allows interaction with the assistant menu even when the prop disabled is true; update the click handler on the Chips (the function that opens the menu) to return early if disabled, and guard any interactive callbacks (menu item handlers) to no-op when disabled; additionally add an effect inside AssistantSelection to call the menu close function (e.g., setIsOpen(false) or closeMenu) whenever disabled transitions to true so an already-open menu closes immediately; ensure the disabled prop is still forwarded to Chips and any menu components so they render in a disabled state.
🧹 Nitpick comments (3)
packages/cozy-search/src/components/Search/SearchConversation.jsx (1)
77-80: Normalize whitespace-only input before toggling search mode.
searchStrtruthiness treats" "as an active search and triggers a regex query. Trim the input for query/mode decisions to avoid unnecessary fetches and confusing empty-result states.🧹 Proposed refactor
const handleSearchChange = e => { - const newQuery = e.target.value - setSearchStr(newQuery) - debouncedFetchConversations(newQuery) + const newQuery = e.target.value + const normalizedQuery = newQuery.trim() + setSearchStr(newQuery) + debouncedFetchConversations(normalizedQuery) } ... - {searchStr ? ( + {searchStr.trim() ? (Also applies to: 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` around lines 77 - 80, Compute both the raw input and a trimmed version in handlers (e.g., in handleSearchChange): keep setSearchStr(rawValue) so the UI shows whitespace if user typed it, but use trimmedValue = rawValue.trim() to decide search mode and to call debouncedFetchConversations—if trimmedValue is empty, avoid toggling search mode or issuing a fetch; otherwise call debouncedFetchConversations(trimmedValue). Apply the same change to the other similar handler referenced (around the second occurrence) so all mode decisions and fetches use the trimmed input while the displayed searchStr remains the original raw value.packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx (2)
92-96: Simplify className condition for readability.The condition
!isMobile || (!isSelected && isMobile)is logically equivalent to!(isSelected && isMobile), which is clearer and mirrors the inverse condition foru-db.♻️ Suggested simplification
className={cx({ - [styles['menu-item-icon-button']]: - !isMobile || (!isSelected && isMobile), + [styles['menu-item-icon-button']]: !(isSelected && isMobile), 'u-db': isSelected && isMobile })}Apply the same simplification to the Delete IconButton at lines 104-108.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 92 - 96, Replace the verbose condition used in the cx call for the menu-item icon button so it uses the simplified logical expression !(isSelected && isMobile) instead of !isMobile || (!isSelected && isMobile); do this for the IconButton that uses styles['menu-item-icon-button'] and the matching Delete IconButton (the other cx call referencing the same classes) so both className calculations become clearer and mirror the inverse condition used for 'u-db'.
48-51: Consider removinguseMemofor this trivial comparison.The computation
selectedAssistant?.id === assistant.idis a simple primitive comparison with negligible cost. The overhead ofuseMemo(creating the dependency array, checking dependencies) may exceed the savings. A plainconst isSelected = selectedAssistant?.id === assistant.idwould be simpler and equally performant.♻️ Suggested simplification
- const isSelected = useMemo( - () => selectedAssistant?.id === assistant.id, - [selectedAssistant?.id, assistant.id] - ) + const isSelected = selectedAssistant?.id === assistant.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 48 - 51, The useMemo around the trivial primitive comparison in AssistantSelectionItem.jsx is unnecessary; replace the memoized expression "const isSelected = useMemo(() => selectedAssistant?.id === assistant.id, [selectedAssistant?.id, assistant.id])" with a direct assignment "const isSelected = selectedAssistant?.id === assistant.id", and remove the unused useMemo import if it becomes unused, keeping references to selectedAssistant and assistant unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-search/src/actions/share.jsx`:
- Around line 35-38: The Share menu item currently uses displayCondition: () =>
true but its action is a no-op; update the Share entry so it is not a clickable
dead item: either change displayCondition to gate the item behind a feature flag
(e.g. displayCondition: () => isShareEnabled()) or replace the empty action
implementation with a short feedback flow that dispatches a localized “not
available yet” toast (e.g. call the app’s toast/notification helper with a
'share.notAvailable' message) and keep displayCondition false until backend
support exists; modify the symbols in this file (the Share action object, its
displayCondition and action) accordingly.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 113-118: The IconButton used to close the mobile
SearchConversation lacks an accessible name; update the IconButton in
SearchConversation.jsx (the IconButton wrapping <Icon icon={CrossIcon} /> and
calling setIsOpenSearchConversation(false)) to include a localized aria-label
(and optionally title) such as
aria-label={t('close')}/aria-label={i18n.t('close')} or a passed prop like
closeLabel so screen readers announce it; ensure you import/use the existing
localization utility (e.g., t or i18n) or accept a localized prop and pass that
string into the IconButton.
In `@packages/cozy-search/src/components/Sidebar/index.jsx`:
- Around line 58-70: The IconButton usage wrapping the toggle Button is an
icon-only control and needs an explicit accessible name; update the IconButton
components (the one with onToggleSidebar and the other icon-only IconButton
instances referenced) to include a descriptive aria-label (e.g.,
aria-label="Toggle sidebar") and ensure any inner Button with only an Icon (the
Button with label={<Icon icon={BurgerIcon} />}) either has aria-hidden on the
decorative Icon and the label moved to the outer IconButton or is given an
accessible name as well; locate the IconButton and Button components in Sidebar
(look for IconButton, Button, onToggleSidebar, and Icon icon={BurgerIcon}) and
add appropriate aria-labels for all icon-only buttons mentioned.
- Around line 141-144: Replace the non-interactive <div> with an accessible
control so keyboard users can dismiss the overlay: change the element that uses
styles['sidebar-overlay--mobile'] to a semantic <button> (or at minimum add
role="button" tabIndex={0} and key handlers) and wire its onClick to
onToggleSidebar; ensure the control has an appropriate aria-label/aria-hidden
logic if needed and that Enter/Space keypresses call onToggleSidebar so
Sidebar's onToggleSidebar works for both mouse and keyboard.
In `@packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx`:
- Around line 24-37: The mobile Dialog branch in TwakeKnowledgePanelContainer is
missing the .source-panel wrapper and an onClose handler, and
TwakeKnowledgePanel is not forwarding onClose to the container; update
TwakeKnowledgePanelContainer to render the same Paper-equivalent wrapper (use
cx(styles['source-panel'], 'u-h-100') or similar) wrapping Dialog content so
.source-panel-content keeps its flex-direction: column behavior, add an onClose
prop to the Dialog (e.g., <Dialog fullScreen open onClose={onClose}>), and
modify TwakeKnowledgePanel to accept and pass an onClose prop down to
TwakeKnowledgePanelContainer so Escape/backdrop and parent-driven close work
correctly.
In
`@packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsx`:
- Around line 56-59: The icon-only chips in TwakeKnowledgeSelector.jsx render an
<img> using twakeKnowledge.icon and an empty label on mobile, leaving
screen-reader users without an accessible name; update the JSX in the
TwakeKnowledgeSelector component to provide a descriptive accessible name by
adding an alt attribute to the <img> (e.g., alt={twakeKnowledge.name} or a
localized description) and ensure the chip/button element exposing the control
has an aria-label or visible fallback text using twakeKnowledge.name (or a
specific accessibleLabel prop) so the chip is identifiable to assistive tech;
locate the image node that references twakeKnowledge.icon and the chip rendering
logic (where the label is currently empty) and add the alt and
aria-label/visible label changes consistently for mobile icon-only variants.
---
Outside diff comments:
In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx`:
- Around line 89-112: The aria-label values on the IconButton elements in
AssistantSelectionItem.jsx are hardcoded English strings ("Edit assistant" and
"Delete assistant"); replace them with i18n translations (use your project's
translation hook/function—e.g., t('assistant.edit') and t('assistant.delete') or
the existing keys convention) so accessibility labels are localized, and update
or add the corresponding localization keys; ensure the same change is applied
where IconButton with onClick handlers handleEdit and handleDelete are used.
---
Duplicate comments:
In `@packages/cozy-search/src/components/Assistant/AssistantSelection.jsx`:
- Around line 22-24: The AssistantSelection component currently still opens and
allows interaction with the assistant menu even when the prop disabled is true;
update the click handler on the Chips (the function that opens the menu) to
return early if disabled, and guard any interactive callbacks (menu item
handlers) to no-op when disabled; additionally add an effect inside
AssistantSelection to call the menu close function (e.g., setIsOpen(false) or
closeMenu) whenever disabled transitions to true so an already-open menu closes
immediately; ensure the disabled prop is still forwarded to Chips and any menu
components so they render in a disabled state.
---
Nitpick comments:
In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx`:
- Around line 92-96: Replace the verbose condition used in the cx call for the
menu-item icon button so it uses the simplified logical expression !(isSelected
&& isMobile) instead of !isMobile || (!isSelected && isMobile); do this for the
IconButton that uses styles['menu-item-icon-button'] and the matching Delete
IconButton (the other cx call referencing the same classes) so both className
calculations become clearer and mirror the inverse condition used for 'u-db'.
- Around line 48-51: The useMemo around the trivial primitive comparison in
AssistantSelectionItem.jsx is unnecessary; replace the memoized expression
"const isSelected = useMemo(() => selectedAssistant?.id === assistant.id,
[selectedAssistant?.id, assistant.id])" with a direct assignment "const
isSelected = selectedAssistant?.id === assistant.id", and remove the unused
useMemo import if it becomes unused, keeping references to selectedAssistant and
assistant unchanged.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 77-80: Compute both the raw input and a trimmed version in
handlers (e.g., in handleSearchChange): keep setSearchStr(rawValue) so the UI
shows whitespace if user typed it, but use trimmedValue = rawValue.trim() to
decide search mode and to call debouncedFetchConversations—if trimmedValue is
empty, avoid toggling search mode or issuing a fetch; otherwise call
debouncedFetchConversations(trimmedValue). Apply the same change to the other
similar handler referenced (around the second occurrence) so all mode decisions
and fetches use the trimmed input while the displayed searchStr remains the
original raw value.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 085c03cf-965a-4780-b3dc-7321c3c616ba
📒 Files selected for processing (35)
packages/cozy-search/src/actions/delete.jsxpackages/cozy-search/src/actions/rename.jsxpackages/cozy-search/src/actions/share.jsxpackages/cozy-search/src/components/Assistant/AssistantContainer.jsxpackages/cozy-search/src/components/Assistant/AssistantSelection.jsxpackages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsxpackages/cozy-search/src/components/Assistant/styles.stylpackages/cozy-search/src/components/Conversations/ConversationBar.jsxpackages/cozy-search/src/components/Conversations/ConversationComposer.jsxpackages/cozy-search/src/components/Conversations/ConversationListItem.jsxpackages/cozy-search/src/components/Conversations/ConversationListItemWider.jsxpackages/cozy-search/src/components/Conversations/styles.stylpackages/cozy-search/src/components/CreateAssistantSteps/ProviderSelectionStep.jsxpackages/cozy-search/src/components/CreateAssistantSteps/styles.stylpackages/cozy-search/src/components/Messages/UserMessage.jsxpackages/cozy-search/src/components/Messages/styles.stylpackages/cozy-search/src/components/Search/NotFoundConversation.jsxpackages/cozy-search/src/components/Search/SearchConversation.jsxpackages/cozy-search/src/components/Search/styles.stylpackages/cozy-search/src/components/Sidebar/index.jsxpackages/cozy-search/src/components/Sidebar/styles.stylpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsxpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsxpackages/cozy-search/src/components/TwakeKnowledges/styles.stylpackages/cozy-search/src/components/Views/AssistantView.jsxpackages/cozy-search/src/components/Views/CreateAssistantDialog.jsxpackages/cozy-search/src/components/Views/EditAssistantDialog.jsxpackages/cozy-search/src/components/constants.jspackages/cozy-search/src/components/queries.jspackages/cozy-search/src/components/styles.stylpackages/cozy-search/src/hooks/useFetchConversations.jspackages/cozy-search/src/locales/en.jsonpackages/cozy-search/src/locales/fr.jsonpackages/cozy-search/src/locales/ru.jsonpackages/cozy-search/src/locales/vi.json
💤 Files with no reviewable changes (1)
- packages/cozy-search/src/components/TwakeKnowledges/styles.styl
✅ Files skipped from review due to trivial changes (2)
- packages/cozy-search/src/actions/delete.jsx
- packages/cozy-search/src/actions/rename.jsx
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/cozy-search/src/components/Assistant/styles.styl
- packages/cozy-search/src/components/Assistant/AssistantContainer.jsx
- packages/cozy-search/src/components/constants.js
- packages/cozy-search/src/components/Search/NotFoundConversation.jsx
- packages/cozy-search/src/components/Messages/UserMessage.jsx
- packages/cozy-search/src/components/queries.js
- packages/cozy-search/src/locales/ru.json
- packages/cozy-search/src/locales/fr.json
- packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx
- packages/cozy-search/src/locales/vi.json
- packages/cozy-search/src/hooks/useFetchConversations.js
- packages/cozy-search/src/components/Conversations/ConversationListItem.jsx
- packages/cozy-search/src/components/Messages/styles.styl
- packages/cozy-search/src/locales/en.json
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
3 times, most recently
from
0335eba to
26ad997
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/Conversations/ConversationListItem.jsx (1)
26-37:⚠️ Potential issue | 🟡 MinorHarden theme-based class selection to avoid undefined CSS class keys.
If
themeis ever missing/unexpected, the computed key can be undefined and class composition becomes fragile. Please guard the lookup before passing it tocx.Suggested fix
const ConversationListItem = ({ conversation, selected, onOpenConversation }) => { const { t, lang } = useI18n() const { type: theme } = useCozyTheme() + const selectedThemeClass = + styles[`conversation-list-item--selected--${theme}`] return ( <ListItem button onClick={() => onOpenConversation(conversation._id)} className={cx( 'u-bdrs-4 u-ov-hidden u-mb-half', styles['conversation-list-item'], - { - [styles[`conversation-list-item--selected--${theme}`]]: selected - } + selected && selectedThemeClass )} selected={selected} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx` around lines 26 - 37, The theme-based class lookup can produce an undefined key if theme is missing; in ConversationListItem.jsx, guard the computed key from useCozyTheme() before passing it to cx by computing a safe variable (e.g., selectedClass) that checks theme is a non-empty string and that styles has the corresponding key (styles[`conversation-list-item--selected--${theme}`]) and only include it in the cx call when present; update the onClick/ListItem className usage to reference this safe selectedClass instead of directly indexing styles with the potentially undefined template string.
♻️ Duplicate comments (5)
packages/cozy-search/src/actions/rename.jsx (1)
35-38:⚠️ Potential issue | 🟠 MajorGate rename action visibility until backend support exists.
Line 35 still exposes Rename unconditionally, while Lines 36-38 are still a no-op. This leaves a clickable dead menu item.
Proposed fix
return { name: 'rename', icon, label, Component: makeComponent(label, icon), - displayCondition: () => true, + displayCondition: () => false, action: () => { - // TO DO: Add action to rename due to this action does not exist yet in backend, we will implement it later + // TODO: Implement rename when backend support is available. } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/actions/rename.jsx` around lines 35 - 38, The Rename menu item is currently always visible but its action is a no-op; update rename.jsx to hide the item until backend support exists by changing displayCondition (the displayCondition function) to check a real feature flag or capability (e.g., hasRenameSupport) or simply return false, and remove or null out the action handler (the action function) so there is no clickable dead entry; ensure you reference displayCondition and action in rename.jsx when making the change so the UI is gated until backend support is implemented.packages/cozy-search/src/components/Search/SearchConversation.jsx (1)
113-118:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the mobile close button.
This icon-only
IconButtonhas no label for screen readers.Suggested fix
{isMobile && ( <IconButton size="small" + aria-label={t('assistant.dialog.close')} + title={t('assistant.dialog.close')} onClick={() => setIsOpenSearchConversation(false)} > <Icon icon={CrossIcon} /> </IconButton> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` around lines 113 - 118, The IconButton in SearchConversation.jsx (the IconButton that calls setIsOpenSearchConversation(false) and renders Icon with CrossIcon) is missing an accessible name; add an accessible label to the button (e.g., aria-label="Close" or a descriptive title prop) so screen readers can announce it, or include visually hidden text inside the button tied to the Icon to provide the label; ensure the attribute is added on the IconButton component itself (not only on the Icon) to make the mobile close button accessible.packages/cozy-search/src/components/Sidebar/index.jsx (2)
141-144:⚠️ Potential issue | 🟠 MajorUse a keyboard-accessible element for overlay dismissal.
The clickable overlay
divis mouse-only. Use a semantic button (or add full keyboard semantics).Suggested fix
{isMobile && sidebarOpen && ( - <div + <button + type="button" className={styles['sidebar-overlay--mobile']} onClick={onToggleSidebar} - ></div> + aria-label={t('assistant.dialog.close')} + /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Sidebar/index.jsx` around lines 141 - 144, Replace the mouse-only overlay div with a keyboard-accessible control: change the element rendered for styles['sidebar-overlay--mobile'] to a semantic button (or keep the div but add role="button", tabIndex="0", aria-label="Close sidebar", and keyDown handler that calls onToggleSidebar on Enter/Escape), and ensure the onClick handler uses the same onToggleSidebar function; update any styles to account for button default focus/appearance if needed and keep the className and onToggleSidebar reference to locate the element in Sidebar (index.jsx).
58-88:⚠️ Potential issue | 🟠 MajorAdd accessible names to icon-only sidebar controls.
The toggle/search/close/new-chat icon buttons need explicit localized
aria-labels so assistive tech can announce them correctly.Also applies to: 103-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Sidebar/index.jsx` around lines 58 - 88, The icon-only IconButton instances (the sidebar toggle using BurgerIcon, the search toggle using SearchIcon, the close using CrossSmallIcon, and the new-chat button referenced around lines 103-109) lack accessible names; update each IconButton to include an explicit localized aria-label (e.g., via your i18n utility or translation hook) such as "Open sidebar"/"Close sidebar"/"Open search"/"New chat" and pass it as the aria-label prop on the IconButton elements that call onToggleSidebar and onToggleSearch so screen readers can announce them.packages/cozy-search/src/locales/en.json (1)
35-35:⚠️ Potential issue | 🟡 MinorUse “Load more” for the pagination CTA text.
Line 35 uses “Fetch more”, which reads technical in end-user UI copy.
Suggested fix
- "load_more": "Fetch more" + "load_more": "Load more"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/locales/en.json` at line 35, Update the end-user copy for the pagination CTA by changing the "load_more" JSON value in packages/cozy-search/src/locales/en.json from "Fetch more" to "Load more" so the label reads as a user-friendly action; locate the "load_more" key and replace its string value accordingly.
🧹 Nitpick comments (1)
packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx (1)
92-96: Deduplicate action button visibility class logic.The same
cx(...)object is repeated for both buttons. Extracting it once reduces drift risk.♻️ Proposed refactor
const isSelected = useMemo( () => selectedAssistant?.id === assistant.id, [selectedAssistant?.id, assistant.id] ) + const actionButtonClassName = cx({ + [styles['menu-item-icon-button']]: + !isMobile || (!isSelected && isMobile), + 'u-db': isSelected && isMobile + }) return ( @@ <IconButton aria-label="Edit assistant" size="small" - className={cx({ - [styles['menu-item-icon-button']]: - !isMobile || (!isSelected && isMobile), - 'u-db': isSelected && isMobile - })} + className={actionButtonClassName} onClick={handleEdit} > @@ <IconButton aria-label="Delete assistant" size="small" - className={cx({ - [styles['menu-item-icon-button']]: - !isMobile || (!isSelected && isMobile), - 'u-db': isSelected && isMobile - })} + className={actionButtonClassName} onClick={handleDelete} >Also applies to: 104-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 92 - 96, The className cx(...) object used to control action button visibility is duplicated; extract it into a single variable (e.g., actionButtonClass) and reuse it for both buttons to avoid drift. Locate the duplicated cx call that references styles['menu-item-icon-button'] and 'u-db' and depends on isMobile and isSelected (the className usages around AssistantSelectionItem.jsx where cx(...) appears at the two button render sites), compute the class once (const actionButtonClass = cx({...})) and replace both className attributes with that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsx`:
- Around line 147-149: The current useEffect clears
cancelledMessageIdsRef.current whenever conversationId changes, which allows
late events from the previous stream to bypass the cancellation check in the
stream handler and be forwarded into the new conversation; instead, stop
clearing cancelledMessageIdsRef in the useEffect tied to conversationId and only
remove entries when the previous stream is fully terminated (e.g., on stream
close/error/abort inside the stream cleanup logic or via an explicit per-stream
generation token), so update the logic around the stream handler (the code that
checks cancelledMessageIdsRef and forwards messages) to clear or rotate
cancellation state at stream teardown rather than on conversationId change
and/or introduce a generationId tied to each stream to avoid cross-conversation
contamination while locating references to cancelledMessageIdsRef, the useEffect
that currently clears it, and the stream forwarding logic that does the check
and forwarding.
- Around line 101-108: Effect in CozyAssistantRuntimeProvider that sets selected
assistant ID currently only depends on
conversation?.relationships?.assistant?.data?._id and setSelectedAssistantId, so
switching routes where conversationId changes but assistant relation value
remains identical won't retrigger it; add the conversationId (the prop/state
that identifies the current conversation) to the dependency array so the
useEffect re-runs on conversation switches and updates selectedAssistantId
accordingly, keeping references to
conversation?.relationships?.assistant?.data?._id and setSelectedAssistantId
intact.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 55-61: User input is being injected directly into the
MongoDB/$regex query (the fetchQuery object built in SearchConversation.jsx
using the value variable), which can allow regex injection or create
catastrophic backtracking; escape regex metacharacters in value before using it
in $regex. Add a small utility (e.g., escapeRegExp) and call it on value when
building fetchQuery (or alternatively create a RegExp from the escaped string)
so the $regex pattern contains a literal-escaped version of value rather than
raw user input.
In `@packages/cozy-search/src/hooks/useFetchConversations.js`:
- Around line 35-65: The fetchConversations async handler can let slower earlier
responses overwrite newer state; protect state updates by stamping each request
with an incrementing ID (useRef) or an AbortController and only call
setConversations, setHasMore and setSkip when the response's ID matches the
latest request ID (or when the fetch wasn't aborted); update the same pattern in
the other similar fetch function around lines 68-76 that uses conversationsQuery
and client so stale responses are ignored and only the latest query result
mutates state.
---
Outside diff comments:
In `@packages/cozy-search/src/components/Conversations/ConversationListItem.jsx`:
- Around line 26-37: The theme-based class lookup can produce an undefined key
if theme is missing; in ConversationListItem.jsx, guard the computed key from
useCozyTheme() before passing it to cx by computing a safe variable (e.g.,
selectedClass) that checks theme is a non-empty string and that styles has the
corresponding key (styles[`conversation-list-item--selected--${theme}`]) and
only include it in the cx call when present; update the onClick/ListItem
className usage to reference this safe selectedClass instead of directly
indexing styles with the potentially undefined template string.
---
Duplicate comments:
In `@packages/cozy-search/src/actions/rename.jsx`:
- Around line 35-38: The Rename menu item is currently always visible but its
action is a no-op; update rename.jsx to hide the item until backend support
exists by changing displayCondition (the displayCondition function) to check a
real feature flag or capability (e.g., hasRenameSupport) or simply return false,
and remove or null out the action handler (the action function) so there is no
clickable dead entry; ensure you reference displayCondition and action in
rename.jsx when making the change so the UI is gated until backend support is
implemented.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 113-118: The IconButton in SearchConversation.jsx (the IconButton
that calls setIsOpenSearchConversation(false) and renders Icon with CrossIcon)
is missing an accessible name; add an accessible label to the button (e.g.,
aria-label="Close" or a descriptive title prop) so screen readers can announce
it, or include visually hidden text inside the button tied to the Icon to
provide the label; ensure the attribute is added on the IconButton component
itself (not only on the Icon) to make the mobile close button accessible.
In `@packages/cozy-search/src/components/Sidebar/index.jsx`:
- Around line 141-144: Replace the mouse-only overlay div with a
keyboard-accessible control: change the element rendered for
styles['sidebar-overlay--mobile'] to a semantic button (or keep the div but add
role="button", tabIndex="0", aria-label="Close sidebar", and keyDown handler
that calls onToggleSidebar on Enter/Escape), and ensure the onClick handler uses
the same onToggleSidebar function; update any styles to account for button
default focus/appearance if needed and keep the className and onToggleSidebar
reference to locate the element in Sidebar (index.jsx).
- Around line 58-88: The icon-only IconButton instances (the sidebar toggle
using BurgerIcon, the search toggle using SearchIcon, the close using
CrossSmallIcon, and the new-chat button referenced around lines 103-109) lack
accessible names; update each IconButton to include an explicit localized
aria-label (e.g., via your i18n utility or translation hook) such as "Open
sidebar"/"Close sidebar"/"Open search"/"New chat" and pass it as the aria-label
prop on the IconButton elements that call onToggleSidebar and onToggleSearch so
screen readers can announce them.
In `@packages/cozy-search/src/locales/en.json`:
- Line 35: Update the end-user copy for the pagination CTA by changing the
"load_more" JSON value in packages/cozy-search/src/locales/en.json from "Fetch
more" to "Load more" so the label reads as a user-friendly action; locate the
"load_more" key and replace its string value accordingly.
---
Nitpick comments:
In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx`:
- Around line 92-96: The className cx(...) object used to control action button
visibility is duplicated; extract it into a single variable (e.g.,
actionButtonClass) and reuse it for both buttons to avoid drift. Locate the
duplicated cx call that references styles['menu-item-icon-button'] and 'u-db'
and depends on isMobile and isSelected (the className usages around
AssistantSelectionItem.jsx where cx(...) appears at the two button render
sites), compute the class once (const actionButtonClass = cx({...})) and replace
both className attributes with that variable.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 668838ff-2431-4881-8f6a-9b58603e6ca1
📒 Files selected for processing (38)
packages/cozy-search/src/actions/delete.jsxpackages/cozy-search/src/actions/rename.jsxpackages/cozy-search/src/actions/share.jsxpackages/cozy-search/src/components/Assistant/AssistantContainer.jsxpackages/cozy-search/src/components/Assistant/AssistantSelection.jsxpackages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsxpackages/cozy-search/src/components/Assistant/styles.stylpackages/cozy-search/src/components/Conversations/ConversationBar.jsxpackages/cozy-search/src/components/Conversations/ConversationComposer.jsxpackages/cozy-search/src/components/Conversations/ConversationListItem.jsxpackages/cozy-search/src/components/Conversations/ConversationListItemWider.jsxpackages/cozy-search/src/components/Conversations/styles.stylpackages/cozy-search/src/components/CozyAssistantRuntimeProvider.tsxpackages/cozy-search/src/components/CreateAssistantSteps/ProviderSelectionStep.jsxpackages/cozy-search/src/components/CreateAssistantSteps/styles.stylpackages/cozy-search/src/components/Messages/AssistantMessage.jsxpackages/cozy-search/src/components/Messages/UserMessage.jsxpackages/cozy-search/src/components/Messages/styles.stylpackages/cozy-search/src/components/Search/NotFoundConversation.jsxpackages/cozy-search/src/components/Search/SearchConversation.jsxpackages/cozy-search/src/components/Search/styles.stylpackages/cozy-search/src/components/Sidebar/index.jsxpackages/cozy-search/src/components/Sidebar/styles.stylpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsxpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsxpackages/cozy-search/src/components/TwakeKnowledges/styles.stylpackages/cozy-search/src/components/Views/AssistantView.jsxpackages/cozy-search/src/components/Views/CreateAssistantDialog.jsxpackages/cozy-search/src/components/Views/EditAssistantDialog.jsxpackages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.tspackages/cozy-search/src/components/constants.jspackages/cozy-search/src/components/queries.jspackages/cozy-search/src/components/styles.stylpackages/cozy-search/src/hooks/useFetchConversations.jspackages/cozy-search/src/locales/en.jsonpackages/cozy-search/src/locales/fr.jsonpackages/cozy-search/src/locales/ru.jsonpackages/cozy-search/src/locales/vi.json
💤 Files with no reviewable changes (1)
- packages/cozy-search/src/components/TwakeKnowledges/styles.styl
✅ Files skipped from review due to trivial changes (1)
- packages/cozy-search/src/actions/delete.jsx
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/cozy-search/src/components/Assistant/AssistantContainer.jsx
- packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx
- packages/cozy-search/src/components/adapters/CozyRealtimeChatAdapter.ts
- packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsx
- packages/cozy-search/src/components/Messages/styles.styl
- packages/cozy-search/src/locales/vi.json
- packages/cozy-search/src/components/Conversations/ConversationBar.jsx
- packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx
- packages/cozy-search/src/components/CreateAssistantSteps/ProviderSelectionStep.jsx
- packages/cozy-search/src/components/Assistant/AssistantSelection.jsx
- packages/cozy-search/src/components/Search/NotFoundConversation.jsx
- packages/cozy-search/src/components/CreateAssistantSteps/styles.styl
- packages/cozy-search/src/components/Messages/AssistantMessage.jsx
- packages/cozy-search/src/components/Views/AssistantView.jsx
- packages/cozy-search/src/components/Messages/UserMessage.jsx
- packages/cozy-search/src/actions/share.jsx
- packages/cozy-search/src/components/constants.js
- packages/cozy-search/src/components/Assistant/styles.styl
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
2 times, most recently
from
4a07021 to
3345702
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/cozy-search/src/components/Search/SearchConversation.jsx (2)
112-119:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the mobile close button.
This icon-only
IconButtonlacks an accessible label. Screen readers will announce an unlabeled control.♿ Proposed fix
{isMobile && ( <IconButton size="small" + aria-label={t('assistant.search_conversation.close')} onClick={() => setIsOpenSearchConversation(false)} > <Icon icon={CrossIcon} /> </IconButton> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` around lines 112 - 119, The mobile close IconButton is missing an accessible name; add an ARIA label or visible text to the IconButton rendered when isMobile is true so screen readers can identify it. Update the IconButton in SearchConversation.jsx (the IconButton that uses onClick={() => setIsOpenSearchConversation(false)} and Icon icon={CrossIcon}) to include a prop like aria-label="Close search conversation" or a non-visual label (e.g., aria-label or title) so the control is announced correctly.
54-66:⚠️ Potential issue | 🟠 MajorEscape user input before building the
$regexquery.Raw input is injected directly into regex pattern construction. Special regex characters (e.g.,
.*+?^${}()|[]\) in user input can break matching or potentially cause ReDoS (Regular Expression Denial of Service) with expensive patterns.🛡️ Proposed fix
import debounce from 'lodash/debounce' +import escapeRegExp from 'lodash/escapeRegExp'Then in the debounced function:
const fetchQuery = value ? { messages: { $elemMatch: { content: { - $regex: value + $regex: escapeRegExp(value) } } } } : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Search/SearchConversation.jsx` around lines 54 - 66, The debounced handler in SearchConversation.jsx injects raw user input into the MongoDB $regex pattern (setQuery) which can be exploited or break; add an escapeRegExp utility (e.g., escapeRegExp = s => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) and call it on value before building fetchQuery, then use the escaped string for the messages.$elemMatch.content.$regex field so special regex metacharacters are neutralized.packages/cozy-search/src/hooks/useFetchConversations.js (1)
32-63:⚠️ Potential issue | 🟠 MajorGuard against stale async responses overwriting newer query results.
Concurrent requests are not guarded. A slower earlier request can resolve after a newer query and replace the state with stale data. This can occur when users rapidly type in the search field.
🛡️ Proposed fix using request ID
const previousQueryRef = useRef() + const latestRequestIdRef = useRef(0) const fetchConversations = useCallback( async (offset = 0, fetchQuery) => { + const requestId = ++latestRequestIdRef.current try { const response = await client.query( conversationsQuery.definition({ offset, query: fetchQuery }) ) + // Discard stale responses + if (requestId !== latestRequestIdRef.current) return const combinedData = response.data?.map(conversation => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/hooks/useFetchConversations.js` around lines 32 - 63, The fetchConversations callback in useFetchConversations can let slower earlier requests overwrite newer results; protect state updates by tracking a monotonically increasing requestId (e.g., useRef latestRequestId) inside useFetchConversations and increment it at each call, capture the current id in the async closure, and only call setConversations, setHasMore and setSkip if the captured id matches latestRequestId.current; update the error branch similarly to ignore errors from stale requests and keep references to fetchConversations, client and conversationsQuery intact.packages/cozy-search/src/components/Assistant/AssistantSelection.jsx (1)
40-46:⚠️ Potential issue | 🟡 MinorClose the dropdown when
disabledbecomes true.The
disabledprop is passed to theChipscomponent, but if the menu is already open whendisabledbecomes true (e.g., when the thread becomes non-empty), it will remain open. Consider adding a guard and an effect to auto-close.🛡️ Proposed fix
-import React, { useState, useRef } from 'react' +import React, { useEffect, useState, useRef } from 'react'Then add the effect and guard:
const AssistantSelection = ({ className, disabled }) => { const { t } = useI18n() const { isMobile } = useBreakpoints() const buttonRef = useRef(null) const [open, setOpen] = useState(false) + + useEffect(() => { + if (disabled) { + setOpen(false) + } + }, [disabled]) + // ... existing code ... const handleClick = () => { + if (disabled) return setOpen(true) }Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelection.jsx` around lines 40 - 46, The dropdown can stay open when disabled flips true; update AssistantSelection to close it when disabled changes by adding a useEffect that watches the disabled prop and calls setOpen(false) (or handleClose) when disabled becomes true, and add a guard in handleClick to no-op if disabled is true so opening is prevented; reference the existing handleClick, handleClose, setOpen, disabled and Chips usage to locate where to add the effect and the guard.packages/cozy-search/src/components/Sidebar/index.jsx (1)
58-70:⚠️ Potential issue | 🟡 MinorAdd accessible names to icon-only buttons.
These
IconButtoncontrols are icon-only and need explicitaria-labelvalues for assistive technologies to announce their purpose.♿ Proposed fix
<IconButton size="medium" className={cx('u-bdrs-6 u-p-0')} onClick={onToggleSidebar} + aria-label={t('assistant.sidebar.toggle')} ><IconButton size="medium" className="u-bdrs-6" onClick={onToggleSearch} + aria-label={t('assistant.sidebar.search')} ><IconButton size="medium" className="u-bdrs-6" onClick={onToggleSidebar} + aria-label={t('assistant.sidebar.close')} ><IconButton size="medium" className="u-bg-primaryColor u-white u-bdrs-6" onClick={createNewConversation} + aria-label={t('assistant.sidebar.create_new')} >Also applies to: 73-79, 82-88, 103-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Sidebar/index.jsx` around lines 58 - 70, The IconButton instances (e.g., the one wrapping Button with label={<Icon icon={BurgerIcon} />} tied to onToggleSidebar and other IconButton usages) are icon-only and must include explicit aria-label attributes; add aria-label to each IconButton, using a descriptive string (for the sidebar toggle use a dynamic value like sidebarOpen ? "Close sidebar" : "Open sidebar") and for other icon buttons provide appropriate labels ("Close", "Open", "Expand", etc.), ensuring the aria-label is applied to the outer IconButton component rather than the inner Button so assistive tech can announce the control purpose.
🧹 Nitpick comments (5)
packages/cozy-search/src/actions/rename.jsx (1)
37-37: Good placeholder marker; tighten comment for consistency.Line 37 reflects the intended UI stub. Suggest using a concise tracked TODO (same style as sibling actions) to improve follow-up hygiene.
Suggested tweak
- // TO DO: Add action to rename due to this action does not exist yet in backend, we will implement it later + // TODO(cozy-libs#<issue>): Implement rename action when backend support is available.Based on learnings: "In the actions directory of cozy-search, identify UI-only placeholders where displayCondition is always true and the action is a no-op... apply the same placeholder pattern consistently... Consider adding a TODO comment with the targeted PR or issue number for backend implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/actions/rename.jsx` at line 37, Replace the freeform comment in rename.jsx with the same concise tracked TODO pattern used by sibling actions: mark the action as a UI-only placeholder by making the comment "TODO(cozy-search#<issue>): implement backend rename action; currently displayCondition is true and action is a no-op" (replace <issue> with the target issue/PR number), and ensure the action object still has displayCondition: true and a no-op handler so it remains a UI stub ready to be wired to the backend later.packages/cozy-search/src/actions/delete.jsx (1)
37-37: Placeholder intent is clear; use a tracked TODO format.Line 37 is correctly explicit about backend absence. Consider standardizing to
TODOwith an issue/PR reference so this stub is easier to track.Suggested tweak
- // TO DO: Add action to remove due to this action does not exist yet in backend, we will implement it later + // TODO(cozy-libs#<issue>): Implement delete action when backend support is available.Based on learnings: "In the actions directory of cozy-search, identify UI-only placeholders where displayCondition is always true and the action is a no-op... verify that these files are clearly marked as placeholders... Consider adding a TODO comment with the targeted PR or issue number for backend implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/actions/delete.jsx` at line 37, Replace the informal comment with a tracked TODO that references the backend issue/PR and clarifies this is a UI-only placeholder for the delete action: update the comment in the delete action (the "delete" export where displayCondition always returns true and the handler is a no-op) to use a standardized format like "TODO(cozy-search#<issue>): UI-only placeholder — backend delete action not implemented; remove when backend supports deletion" and include any existing issue/PR number or "TBD" if unknown so it can be easily found and tracked.packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx (2)
48-52: Consider removing unnecessary memoization.The
isSelectedcomputation is a simple reference equality check (===), which is cheaper than the dependency array comparison thatuseMemoperforms. A direct computation would be more efficient here.♻️ Suggested simplification
- const isSelected = useMemo( - () => selectedAssistant?.id === assistant.id, - [selectedAssistant?.id, assistant.id] - ) + const isSelected = selectedAssistant?.id === assistant.idAlso update line 2:
-import React, { useMemo } from 'react' +import React from 'react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 48 - 52, The useMemo around isSelected is unnecessary overhead; replace the memoized expression with a direct equality check by setting isSelected to selectedAssistant?.id === assistant.id (remove the useMemo call) and remove the now-unused useMemo import if present; update any related references to use the plain isSelected variable.
92-108: Simplify the conditional class expression.The condition
!isMobile || (!isSelected && isMobile)is logically equivalent to!(isMobile && isSelected), which is easier to read and understand.♻️ Suggested simplification
className={cx({ [styles['menu-item-icon-button']]: - !isMobile || (!isSelected && isMobile), + !(isMobile && isSelected), 'u-db': isSelected && isMobile })}Apply the same change to both IconButtons (Edit and Delete).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx` around lines 92 - 108, The className conditional for the IconButton instances is overly complex; replace the expression !isMobile || (!isSelected && isMobile) with the equivalent, clearer form !(isMobile && isSelected) in both IconButton usages in AssistantSelectionItem.jsx (the IconButton rendering the edit button with onClick={handleEdit} and the subsequent delete IconButton), keeping the rest of the cx call and styles['menu-item-icon-button'] intact so behavior is unchanged.packages/cozy-search/src/components/Conversations/ConversationBar.jsx (1)
23-24: Preserve caller-providedclassNamewhen forwarding props.With
...propson Line 56 and a fixedclassNameon Line 57, any upstreamclassNameis dropped. Consider composing it to keep extensibility.♻️ Suggested tweak
const ConversationBar = ({ value, isEmpty, isRunning, onKeyDown, onSend, onCancel, + className, ...props }) => { @@ <SearchBar {...props} - className={cx(styles['conversationBar'], { + className={cx(className, styles['conversationBar'], { [styles['conversationBar--mobile']]: isMobile })}Also applies to: 56-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-search/src/components/Conversations/ConversationBar.jsx` around lines 23 - 24, The component ConversationBar is clobbering any caller-provided className by spreading ...props and then passing a fixed className; update the prop handling to merge/compose the incoming className with the component's fixed class(es) instead of overwriting them — e.g., read className from props (or from the function signature), concatenate it with the fixed class string, and pass that combined value to the element where className is set (referencing ConversationBar, onCancel, and the element using className) so upstream styles are preserved while keeping the component's default classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-search/src/components/Sidebar/index.jsx`:
- Around line 140-145: The overlay div rendered when isMobile && sidebarOpen
should be hidden from assistive tech and keyboard dismissal handled at the
sidebar container: add aria-hidden="true" to the overlay element
(styles['sidebar-overlay--mobile'], onToggleSidebar) and implement an Escape-key
handler in the Sidebar component (e.g., inside a useEffect in Sidebar or the
container component that listens for 'keydown' and calls onToggleSidebar when
key === 'Escape' and sidebarOpen is true, with proper cleanup).
---
Duplicate comments:
In `@packages/cozy-search/src/components/Assistant/AssistantSelection.jsx`:
- Around line 40-46: The dropdown can stay open when disabled flips true; update
AssistantSelection to close it when disabled changes by adding a useEffect that
watches the disabled prop and calls setOpen(false) (or handleClose) when
disabled becomes true, and add a guard in handleClick to no-op if disabled is
true so opening is prevented; reference the existing handleClick, handleClose,
setOpen, disabled and Chips usage to locate where to add the effect and the
guard.
In `@packages/cozy-search/src/components/Search/SearchConversation.jsx`:
- Around line 112-119: The mobile close IconButton is missing an accessible
name; add an ARIA label or visible text to the IconButton rendered when isMobile
is true so screen readers can identify it. Update the IconButton in
SearchConversation.jsx (the IconButton that uses onClick={() =>
setIsOpenSearchConversation(false)} and Icon icon={CrossIcon}) to include a prop
like aria-label="Close search conversation" or a non-visual label (e.g.,
aria-label or title) so the control is announced correctly.
- Around line 54-66: The debounced handler in SearchConversation.jsx injects raw
user input into the MongoDB $regex pattern (setQuery) which can be exploited or
break; add an escapeRegExp utility (e.g., escapeRegExp = s =>
s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) and call it on value before building
fetchQuery, then use the escaped string for the
messages.$elemMatch.content.$regex field so special regex metacharacters are
neutralized.
In `@packages/cozy-search/src/components/Sidebar/index.jsx`:
- Around line 58-70: The IconButton instances (e.g., the one wrapping Button
with label={<Icon icon={BurgerIcon} />} tied to onToggleSidebar and other
IconButton usages) are icon-only and must include explicit aria-label
attributes; add aria-label to each IconButton, using a descriptive string (for
the sidebar toggle use a dynamic value like sidebarOpen ? "Close sidebar" :
"Open sidebar") and for other icon buttons provide appropriate labels ("Close",
"Open", "Expand", etc.), ensuring the aria-label is applied to the outer
IconButton component rather than the inner Button so assistive tech can announce
the control purpose.
In `@packages/cozy-search/src/hooks/useFetchConversations.js`:
- Around line 32-63: The fetchConversations callback in useFetchConversations
can let slower earlier requests overwrite newer results; protect state updates
by tracking a monotonically increasing requestId (e.g., useRef latestRequestId)
inside useFetchConversations and increment it at each call, capture the current
id in the async closure, and only call setConversations, setHasMore and setSkip
if the captured id matches latestRequestId.current; update the error branch
similarly to ignore errors from stale requests and keep references to
fetchConversations, client and conversationsQuery intact.
---
Nitpick comments:
In `@packages/cozy-search/src/actions/delete.jsx`:
- Line 37: Replace the informal comment with a tracked TODO that references the
backend issue/PR and clarifies this is a UI-only placeholder for the delete
action: update the comment in the delete action (the "delete" export where
displayCondition always returns true and the handler is a no-op) to use a
standardized format like "TODO(cozy-search#<issue>): UI-only placeholder —
backend delete action not implemented; remove when backend supports deletion"
and include any existing issue/PR number or "TBD" if unknown so it can be easily
found and tracked.
In `@packages/cozy-search/src/actions/rename.jsx`:
- Line 37: Replace the freeform comment in rename.jsx with the same concise
tracked TODO pattern used by sibling actions: mark the action as a UI-only
placeholder by making the comment "TODO(cozy-search#<issue>): implement backend
rename action; currently displayCondition is true and action is a no-op"
(replace <issue> with the target issue/PR number), and ensure the action object
still has displayCondition: true and a no-op handler so it remains a UI stub
ready to be wired to the backend later.
In `@packages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsx`:
- Around line 48-52: The useMemo around isSelected is unnecessary overhead;
replace the memoized expression with a direct equality check by setting
isSelected to selectedAssistant?.id === assistant.id (remove the useMemo call)
and remove the now-unused useMemo import if present; update any related
references to use the plain isSelected variable.
- Around line 92-108: The className conditional for the IconButton instances is
overly complex; replace the expression !isMobile || (!isSelected && isMobile)
with the equivalent, clearer form !(isMobile && isSelected) in both IconButton
usages in AssistantSelectionItem.jsx (the IconButton rendering the edit button
with onClick={handleEdit} and the subsequent delete IconButton), keeping the
rest of the cx call and styles['menu-item-icon-button'] intact so behavior is
unchanged.
In `@packages/cozy-search/src/components/Conversations/ConversationBar.jsx`:
- Around line 23-24: The component ConversationBar is clobbering any
caller-provided className by spreading ...props and then passing a fixed
className; update the prop handling to merge/compose the incoming className with
the component's fixed class(es) instead of overwriting them — e.g., read
className from props (or from the function signature), concatenate it with the
fixed class string, and pass that combined value to the element where className
is set (referencing ConversationBar, onCancel, and the element using className)
so upstream styles are preserved while keeping the component's default classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3d59a7d-694e-43f1-8fa3-f8a1e665158d
📒 Files selected for processing (35)
packages/cozy-search/src/actions/delete.jsxpackages/cozy-search/src/actions/rename.jsxpackages/cozy-search/src/actions/share.jsxpackages/cozy-search/src/components/Assistant/AssistantContainer.jsxpackages/cozy-search/src/components/Assistant/AssistantSelection.jsxpackages/cozy-search/src/components/Assistant/AssistantSelectionItem.jsxpackages/cozy-search/src/components/Assistant/styles.stylpackages/cozy-search/src/components/Conversations/ConversationBar.jsxpackages/cozy-search/src/components/Conversations/ConversationComposer.jsxpackages/cozy-search/src/components/Conversations/ConversationListItem.jsxpackages/cozy-search/src/components/Conversations/ConversationListItemWider.jsxpackages/cozy-search/src/components/Conversations/styles.stylpackages/cozy-search/src/components/CreateAssistantSteps/ProviderSelectionStep.jsxpackages/cozy-search/src/components/CreateAssistantSteps/styles.stylpackages/cozy-search/src/components/Messages/UserMessage.jsxpackages/cozy-search/src/components/Messages/styles.stylpackages/cozy-search/src/components/Search/NotFoundConversation.jsxpackages/cozy-search/src/components/Search/SearchConversation.jsxpackages/cozy-search/src/components/Search/styles.stylpackages/cozy-search/src/components/Sidebar/index.jsxpackages/cozy-search/src/components/Sidebar/styles.stylpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsxpackages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsxpackages/cozy-search/src/components/TwakeKnowledges/styles.stylpackages/cozy-search/src/components/Views/AssistantView.jsxpackages/cozy-search/src/components/Views/CreateAssistantDialog.jsxpackages/cozy-search/src/components/Views/EditAssistantDialog.jsxpackages/cozy-search/src/components/constants.jspackages/cozy-search/src/components/queries.jspackages/cozy-search/src/components/styles.stylpackages/cozy-search/src/hooks/useFetchConversations.jspackages/cozy-search/src/locales/en.jsonpackages/cozy-search/src/locales/fr.jsonpackages/cozy-search/src/locales/ru.jsonpackages/cozy-search/src/locales/vi.json
💤 Files with no reviewable changes (1)
- packages/cozy-search/src/components/TwakeKnowledges/styles.styl
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/cozy-search/src/components/Conversations/ConversationComposer.jsx
- packages/cozy-search/src/components/Search/styles.styl
- packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgeSelector.jsx
- packages/cozy-search/src/components/constants.js
- packages/cozy-search/src/components/Conversations/ConversationListItem.jsx
- packages/cozy-search/src/components/Assistant/styles.styl
- packages/cozy-search/src/components/styles.styl
- packages/cozy-search/src/locales/ru.json
- packages/cozy-search/src/locales/vi.json
- packages/cozy-search/src/components/CreateAssistantSteps/styles.styl
- packages/cozy-search/src/components/Views/AssistantView.jsx
- packages/cozy-search/src/locales/fr.json
- packages/cozy-search/src/components/Views/EditAssistantDialog.jsx
- packages/cozy-search/src/components/Messages/UserMessage.jsx
- packages/cozy-search/src/actions/share.jsx
- packages/cozy-search/src/components/Conversations/ConversationListItemWider.jsx
- packages/cozy-search/src/locales/en.json
- packages/cozy-search/src/components/TwakeKnowledges/TwakeKnowledgePanel.jsx
Comment on lines
+140
to
145
| {isMobile && sidebarOpen && ( | ||
| <div | ||
| className={styles['sidebar-overlay--mobile']} | ||
| onClick={onToggleSidebar} | ||
| ></div> | ||
| )} |
There was a problem hiding this comment.
Add aria-hidden="true" to the overlay and handle Escape key dismissal.
Per the established pattern in this codebase, the overlay is correctly a visual backdrop <div> rather than a button. However, it should be marked with aria-hidden="true" to hide it from assistive technologies, and keyboard dismissal (Escape key) should be handled at the sidebar container level.
♿ Proposed fix
Add aria-hidden to the overlay:
{isMobile && sidebarOpen && (
<div
className={styles['sidebar-overlay--mobile']}
onClick={onToggleSidebar}
+ aria-hidden="true"
></div>
)}Add Escape key handler at the container level (e.g., using useEffect):
+ useEffect(() => {
+ const handleKeyDown = (e) => {
+ if (e.key === 'Escape' && sidebarOpen && isMobile) {
+ onToggleSidebar()
+ }
+ }
+ document.addEventListener('keydown', handleKeyDown)
+ return () => document.removeEventListener('keydown', handleKeyDown)
+ }, [sidebarOpen, isMobile])Based on learnings: "the mobile sidebar overlay is intentionally a <div> acting as a visual backdrop, not a <button>. The correct accessibility approach is to mark it with aria-hidden="true" and handle keyboard dismissal (Escape key) at the sidebar container level."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cozy-search/src/components/Sidebar/index.jsx` around lines 140 -
145, The overlay div rendered when isMobile && sidebarOpen should be hidden from
assistive tech and keyboard dismissal handled at the sidebar container: add
aria-hidden="true" to the overlay element (styles['sidebar-overlay--mobile'],
onToggleSidebar) and implement an Escape-key handler in the Sidebar component
(e.g., inside a useEffect in Sidebar or the container component that listens for
'keydown' and calls onToggleSidebar when key === 'Escape' and sidebarOpen is
true, with proper cleanup).
3345702 to
49d13a2
Compare
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
4 times, most recently
from
0a1c596 to
8e01986
Compare
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
2 times, most recently
from
78b66cd to
44d297f
Compare
paultranvan
reviewed
Mar 9, 2026
| .limitBy(FETCH_CONVERSATIONS_LIMIT), | ||
| options: ({ bookmark, query = {} }) => ({ | ||
| as: `${CHAT_CONVERSATIONS_DOCTYPE}/recent-${ | ||
| bookmark || '' |
Contributor
There was a problem hiding this comment.
You should not have bookmark in the query name, this is still the same query, and the results on the query will be saved in cozy-client cache under this name. If you add the bookmark info, it will break the cache mechanism
| messages: { | ||
| $elemMatch: { | ||
| content: { | ||
| $regex: escapeRegExp(value) |
Contributor
There was a problem hiding this comment.
@Crash-- We can, but we don't have fulltext search in backend as far as I know. Even though that's something that could be envisioned, as a platform feature?
Or we can use CouchDB query like it''s done here with regex, but as said, it's inefficient, both for performance and quality
Otherwise, I assume we could something simple, with fuse.js like it's done with contacts
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
from
44d297f to
e9be50f
Compare
paultranvan
approved these changes
Mar 11, 2026
Contributor
paultranvan
left a comment
paultranvan
left a comment
There was a problem hiding this comment.
I saw visual improvements we could make, but I think we should first merge this, and move on
added 11 commits
March 12, 2026 17:09
lethemanh
force-pushed
the
resolve-feedbacks-of-assistant
branch
from
e9be50f to
45d92bd
Compare
rezk2ll
approved these changes
Mar 12, 2026
This was referenced Mar 12, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to:
linagora/cozy-home#2351
cozy/cozy-stack#4687
Summary by CodeRabbit
New Features
UI / Style Changes
Localization