feat: Update to use new assistant ✨#2351
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 an Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containers/App.jsx (1)
160-175:⚠️ Potential issue | 🔴 CriticalCritical routing bug:
AssistantViewwill not render due to missing<Outlet />in the parent route's element.In React Router v6, child routes render via the
<Outlet />component in the parent route's element. WhenisAssistantRouteistrue, the/connectedroute's element evaluates tofalse(since!isAssistantRoute && <Home />becomesfalse && <Home />=false), so<Home>is never mounted. Since<Home>is the only component rendering the<Outlet />for this route subtree, the nestedassistant/:conversationIdroute's<AssistantView />element has nowhere to render and will not display.Restructure the route so the parent always renders an
<Outlet />:Suggested fix: render Outlet on assistant routes
<Route path="/connected" element={ - !isAssistantRoute && ( - <Home - wrapper={contentWrapper} - shortcutsDirectories={shortcutsDirectories} - /> - ) + isAssistantRoute ? ( + <Outlet /> + ) : ( + <Home + wrapper={contentWrapper} + shortcutsDirectories={shortcutsDirectories} + /> + ) } >Import
Outletfromreact-router-dom.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 34-35: The package.json currently lists dependencies
"@assistant-ui/react", "@radix-ui/react-slot", and "lucide-react" that have no
direct imports; either remove these entries or document why they remain (e.g.,
required peer/transitive deps for "cozy-search") by adding an inline comment in
package.json or README. Locate the dependency entries for "@assistant-ui/react",
"@radix-ui/react-slot", and "lucide-react" in package.json, confirm via a
repo-wide search that there are no direct imports, and if they are unnecessary
delete them from package.json and run npm/yarn install; if they are required as
peers/transitive dependencies, add a brief comment next to each dependency (or
in project docs) stating the owner package (e.g., "kept for cozy-search peer
deps") and why it must remain. Ensure package-lock.json / yarn.lock is updated
after the change.
🧹 Nitpick comments (2)
src/containers/App.jsx (2)
110-110: Consider a more robust route check.
pathname.startsWith('/connected/assistant/')requires a trailing slash, which means/connected/assistant(without trailing slash or conversation ID) won't match. If that's intentional, it's fine, but verify that this won't cause layout inconsistencies if users navigate to/connected/assistantwithout a conversation ID.
114-119: Template literal with ternary could usecxfor consistency.You're already importing and using
cxelsewhere in this component. Consider using it here too for consistency.Suggested change
- <Layout - monoColumn - className={`${ - isAssistantRoute ? 'u-bg-primaryBackgroundLight' : 'u-bg-white' - }`} - > + <Layout + monoColumn + className={cx({ + 'u-bg-primaryBackgroundLight': isAssistantRoute, + 'u-bg-white': !isAssistantRoute + })} + >
lethemanh
force-pushed
the
update-for-assistant
branch
from
368a7e9 to
619d870
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containers/App.jsx (1)
162-183:⚠️ Potential issue | 🔴 CriticalCritical:
/connectedroute prevents child route rendering whenisNewAssistantViewistrue.When
isNewAssistantViewistrue, the/connectedroute's element evaluates tofalse(line 166). In React Router v6, a falsyelementrenders nothing and produces no<Outlet />. Since nested routes likeassistant/:conversationIddepend on the parent's<Outlet />to render, they cannot mount in this state.The
Homecomponent already imports and renders<Outlet />, but it's only rendered whenisNewAssistantViewisfalse. Whentrue, child routes fail to render.Render an
<Outlet />when the assistant view is active:Proposed fix
<Route path="/connected" element={ - !isNewAssistantView && ( + isNewAssistantView ? ( + <Outlet /> + ) : ( <Home wrapper={contentWrapper} shortcutsDirectories={shortcutsDirectories} /> ) } >This requires importing
Outletfromreact-router-dom:-import { Navigate, Route, useLocation } from 'react-router-dom' +import { Navigate, Outlet, Route, useLocation } from 'react-router-dom'
🤖 Fix all issues with AI agents
In `@src/containers/App.spec.jsx`:
- Line 40: Prettier is failing because the mock functions defined for
useNavigate contain an extra space inside their empty function bodies (e.g., the
arrow mock useNavigate: () => () => { } ); update those to remove the inner
space so they are empty blocks without whitespace (e.g., change { } to {} ) for
the useNavigate mock instances found in App.spec.jsx (also apply the same change
to the other occurrences).
🧹 Nitpick comments (2)
src/containers/App.spec.jsx (1)
36-43: Note: mockingRoutesbut notRoutemay cause issues if the realRouteinteracts with the mockedRoutes.
Routesis mocked asjest.fn()(returnsundefined), so the entire route tree renders nothing. This is presumably intentional for these tests, but worth noting that these tests won't exercise any routing logic introduced in App.jsx. Consider adding dedicated tests for the assistant view route behavior if not covered elsewhere.src/containers/App.jsx (1)
116-121: Template literal can be simplified.The template literal wrapping a single ternary can be replaced with a plain ternary for readability.
Suggested simplification
- className={`${ - isNewAssistantView ? 'u-bg-primaryBackgroundLight' : 'u-bg-white' - }`} + className={isNewAssistantView ? 'u-bg-primaryBackgroundLight' : 'u-bg-white'}
lethemanh
force-pushed
the
update-for-assistant
branch
from
619d870 to
2250971
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/containers/App.jsx`:
- Around line 163-183: The parent Route for path "/connected" currently returns
false when isNewAssistantView is true, preventing its child routes (e.g., the
"assistant/:conversationId" route rendering AssistantView/AssistantDialog) from
mounting; update the element prop so that when isNewAssistantView is true you
still render a wrapper that includes an Outlet (instead of false) so child
routes can render — modify the element expression around Home (component name
Home) to conditionally render Home when !isNewAssistantView and otherwise render
a minimal wrapper component that imports and returns <Outlet /> (ensure Outlet
is imported from react-router-dom) so AssistantView and AssistantDialog can
mount.
🧹 Nitpick comments (1)
src/containers/App.jsx (1)
116-121: Nit: Consider usingcxfor consistency with the rest of the file.You're already importing and using
cxelsewhere. Using a template literal here is fine functionally, butcxwould be more consistent.Suggested change
<Layout monoColumn - className={`${ - isNewAssistantView ? 'u-bg-primaryBackgroundLight' : 'u-bg-white' - }`} + className={cx({ + 'u-bg-primaryBackgroundLight': isNewAssistantView, + 'u-bg-white': !isNewAssistantView + })} >
| > | ||
| <BarComponent | ||
| searchOptions={{ enabled: false }} | ||
| searchOptions={{ enabled: isNewAssistantView }} |
There was a problem hiding this comment.
Keep false here. I think we will never want the search bar in home top bar.
There was a problem hiding this comment.
But I saw the search bar display in the figma, should we need a confirmation?
| wrapper={contentWrapper} | ||
| shortcutsDirectories={shortcutsDirectories} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
If the Home did not hide, the assistant view will look like this
lethemanh
force-pushed
the
update-for-assistant
branch
2 times, most recently
from
eaf1fc1 to
200ff35
Compare
lethemanh
force-pushed
the
update-for-assistant
branch
from
200ff35 to
8912f3b
Compare
lethemanh
force-pushed
the
update-for-assistant
branch
from
8912f3b to
44df368
Compare
lethemanh
force-pushed
the
update-for-assistant
branch
from
44df368 to
dd4cec9
Compare
lethemanh
force-pushed
the
update-for-assistant
branch
from
dd4cec9 to
dd379e5
Compare
BundleMonFiles updated (12)
Unchanged files (4)
Total files change +103.17KB +2.31% Groups updated (3)
Final result: ✅ View report in BundleMon website ➡️ |
lethemanh
force-pushed
the
update-for-assistant
branch
from
dd379e5 to
290a3ae
Compare
paultranvan
force-pushed
the
update-for-assistant
branch
from
38231fc to
acfb047
Compare
4 participants
Needs:
linagora/cozy-libs#2925
linagora/cozy-libs#2937
cozy/cozy-ui#2902
Result:
Screen.Recording.2026-02-12.at.17.49.01.mov
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
UI/UX Improvements
Tests
Chores