Conversation
WalkthroughAdds AI Chat Assistant foundations: test environment shims for Web Streams and a minimal 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 docstrings
🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
fe008cf to
356cfef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
356cfef to
f36c564
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BundleMonFiles updated (12)
Unchanged files (9)
Total files change +275.17KB +4.98% Groups updated (2)
Unchanged groups (1)
Final result: ✅ View report in BundleMon website ➡️ |
paultranvan
force-pushed
the
update-for-assistant
branch
from
f36c564 to
ff5734b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
ff5734b to
eb38118
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
eb38118 to
8aea7ad
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
8aea7ad to
e27d0f8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
e27d0f8 to
3df6b2e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
force-pushed
the
update-for-assistant
branch
from
3df6b2e to
b9802b4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
paultranvan
requested review from
JF-Cozy,
doubleface,
lethemanh,
rezk2ll and
zatteo
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/modules/navigation/AppRoute.jsx (1)
58-60: Consider using a more robust route filtering approach.The current filter relies on exact string matching of
props.path. IfBarRouteselements don't consistently have aprops.pathproperty or if the path format changes, this could silently fail.Consider extracting the path string to a constant for consistency:
♻️ Suggested improvement
+const ASSISTANT_ROUTE_PATH = 'assistant/:conversationId' + const filteredBarRoutes = BarRoutes.filter( - r => r.props?.path !== 'assistant/:conversationId' + r => r.props?.path !== ASSISTANT_ROUTE_PATH )Then reuse the constant in line 84:
- <Route path="assistant/:conversationId" element={<AssistantLayout />} /> + <Route path={ASSISTANT_ROUTE_PATH} element={<AssistantLayout />} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/AppRoute.jsx` around lines 58 - 60, The filter on BarRoutes using r.props?.path !== 'assistant/:conversationId' is brittle; extract the path string into a shared constant (e.g., ASSISTANT_CONVERSATION_PATH) and use that constant when filtering and elsewhere (referenced in filteredBarRoutes and any other uses around line ~84) and defensively handle missing path values by coercing to a string or using optional chaining (e.g., const path = r.props?.path ?? '') before comparison so the filter won't silently fail if props or path are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jestHelpers/setup.js`:
- Line 7: Replace the minimal stubbed Response class (global.Response = class
Response {}) with a full fetch/Response polyfill; either import and invoke the
existing whatwg-fetch or undici fetch polyfill in jestHelpers/setup.js so
global.Response (and associated properties/methods like ok, status, json, text,
body) are provided instead of the empty class, ensuring any code that uses
Response (e.g., assistant-stream) gets the expected behavior.
In `@manifest.webapp`:
- Around line 217-226: Replace the broad verbs ["ALL"] on the permissions
objects chatConversations and assistants with least-privilege verb lists that
match actual usage (e.g., only read/create/update verbs such as GET, POST,
PUT/PATCH) so DELETE is not implicitly granted; update the "verbs" arrays on the
chatConversations and assistants entries to the minimal set required by the
feature, and only retain "DELETE" if the code explicitly requires conversation
deletion.
In `@src/lib/doctypes.js`:
- Around line 38-47: Replace the hardcoded doctype string in the
conversations.relationships.assistant block with the DOCTYPE_AI_CHAT_ASSISTANTS
constant: locate the conversations object (symbol DOCTYPE_AI_CHAT_CONVERSATIONS)
and update the assistant.relationships.type value that currently reads
'io.cozy.ai.chat.assistants' to reference DOCTYPE_AI_CHAT_ASSISTANTS so the
module uses the defined constant rather than a literal string.
- Around line 14-15: Add a schema entry for the assistants doctype so
cozy-client can normalize/cache assistant docs: create a schema object keyed by
DOCTYPE_AI_CHAT_ASSISTANTS (the same constant exported alongside
DOCTYPE_AI_CHAT_CONVERSATIONS) mirroring the pattern used for the conversations
schema (include id, relationships, and any necessary attributes) so
RealTimeQueries/AssistantLayout.jsx subscriptions and the conversations schema
relationship target are properly recognized and normalized by cozy-client.
In `@src/modules/views/Assistant/AssistantLayout.jsx`:
- Around line 16-36: AssistantLayout is not reading the route parameter
conversationId defined on the route; update AssistantLayout to import and call
useParams() to extract conversationId and pass it into <AssistantView
conversationId={conversationId} /> (or, if the route param is unnecessary,
remove conversationId from the route instead); specifically modify the
AssistantLayout function to const { conversationId } = useParams() and forward
that value to the AssistantView component (or coordinate route change if you
choose to remove the param).
---
Nitpick comments:
In `@src/modules/navigation/AppRoute.jsx`:
- Around line 58-60: The filter on BarRoutes using r.props?.path !==
'assistant/:conversationId' is brittle; extract the path string into a shared
constant (e.g., ASSISTANT_CONVERSATION_PATH) and use that constant when
filtering and elsewhere (referenced in filteredBarRoutes and any other uses
around line ~84) and defensively handle missing path values by coercing to a
string or using optional chaining (e.g., const path = r.props?.path ?? '')
before comparison so the filter won't silently fail if props or path are absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7127f15-5512-4d82-b30e-363abd05b8f5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
jestHelpers/setup.jsmanifest.webapppackage.jsonsrc/lib/doctypes.jssrc/modules/navigation/AppRoute.jsxsrc/modules/views/Assistant/AssistantLayout.jsxsrc/modules/views/Assistant/assistant.styl
| // jsdom doesn't expose Web Streams/Fetch APIs needed by assistant-stream | ||
| if (!global.TransformStream) global.TransformStream = TransformStream | ||
| if (!global.ReadableStream) global.ReadableStream = ReadableStream | ||
| if (!global.Response) global.Response = class Response {} |
There was a problem hiding this comment.
The minimal Response stub may be insufficient.
The empty Response class lacks essential properties and methods (ok, status, json(), text(), body, etc.) that code may rely on. If assistant-stream or related code accesses these members, tests will fail silently or throw.
Consider using a more complete polyfill:
♻️ Suggested improvement
-if (!global.Response) global.Response = class Response {}
+if (!global.Response) {
+ // Minimal Response polyfill for assistant-stream
+ global.Response = class Response {
+ constructor(body, init = {}) {
+ this.body = body
+ this.status = init.status ?? 200
+ this.ok = this.status >= 200 && this.status < 300
+ this.headers = new Map(Object.entries(init.headers ?? {}))
+ }
+ async json() { return JSON.parse(this.body) }
+ async text() { return String(this.body) }
+ }
+}Alternatively, use whatwg-fetch (already in dependencies) or undici's fetch polyfill which provides a complete implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!global.Response) global.Response = class Response {} | |
| if (!global.Response) { | |
| // Minimal Response polyfill for assistant-stream | |
| global.Response = class Response { | |
| constructor(body, init = {}) { | |
| this.body = body | |
| this.status = init.status ?? 200 | |
| this.ok = this.status >= 200 && this.status < 300 | |
| this.headers = new Map(Object.entries(init.headers ?? {})) | |
| } | |
| async json() { return JSON.parse(this.body) } | |
| async text() { return String(this.body) } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jestHelpers/setup.js` at line 7, Replace the minimal stubbed Response class
(global.Response = class Response {}) with a full fetch/Response polyfill;
either import and invoke the existing whatwg-fetch or undici fetch polyfill in
jestHelpers/setup.js so global.Response (and associated properties/methods like
ok, status, json, text, body) are provided instead of the empty class, ensuring
any code that uses Response (e.g., assistant-stream) gets the expected behavior.
| "chatConversations": { | ||
| "description": "Required by the cozy Assistant", | ||
| "description": "Required by the AI Assistant to show conversations", | ||
| "type": "io.cozy.ai.chat.conversations", | ||
| "verbs": ["GET", "POST"] | ||
| "verbs": ["ALL"] | ||
| }, | ||
| "assistants": { | ||
| "type": "io.cozy.ai.chat.assistants", | ||
| "verbs": ["ALL"], | ||
| "description": "Required to fetch, create and update AI assistants" | ||
| }, |
There was a problem hiding this comment.
Consider using least-privilege permissions instead of ["ALL"].
Both chatConversations and assistants permissions now use ["ALL"] verbs, which includes DELETE. Following the principle of least privilege, specify only the verbs actually needed by the feature.
If the assistant only needs to read, create, and update:
🛡️ Suggested restriction
"chatConversations": {
"description": "Required by the AI Assistant to show conversations",
"type": "io.cozy.ai.chat.conversations",
- "verbs": ["ALL"]
+ "verbs": ["GET", "POST", "PUT", "PATCH"]
},
"assistants": {
"type": "io.cozy.ai.chat.assistants",
- "verbs": ["ALL"],
+ "verbs": ["GET", "POST", "PUT", "PATCH"],
"description": "Required to fetch, create and update AI assistants"
},If DELETE is genuinely required (e.g., for clearing conversation history), the current configuration is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifest.webapp` around lines 217 - 226, Replace the broad verbs ["ALL"] on
the permissions objects chatConversations and assistants with least-privilege
verb lists that match actual usage (e.g., only read/create/update verbs such as
GET, POST, PUT/PATCH) so DELETE is not implicitly granted; update the "verbs"
arrays on the chatConversations and assistants entries to the minimal set
required by the feature, and only retain "DELETE" if the code explicitly
requires conversation deletion.
| export const DOCTYPE_AI_CHAT_CONVERSATIONS = 'io.cozy.ai.chat.conversations' | ||
| export const DOCTYPE_AI_CHAT_ASSISTANTS = 'io.cozy.ai.chat.assistants' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other doctypes without schema entries are handled
rg -n 'RealTimeQueries' --type=js -A 2 -B 2
# Check if there are other doctypes used without schema entries
rg -n 'DOCTYPE_' src/lib/doctypes.jsRepository: linagora/twake-drive
Length of output: 7153
🏁 Script executed:
cat -n src/lib/doctypes.jsRepository: linagora/twake-drive
Length of output: 1979
Add a schema entry for the assistants doctype.
DOCTYPE_AI_CHAT_ASSISTANTS is used in RealTimeQueries (in AssistantLayout.jsx) and referenced as a relationship target in the conversations schema, but lacks its own schema definition. While this works for realtime subscriptions, adding a schema entry ensures cozy-client properly normalizes and caches these documents, matching the pattern used for the conversations doctype.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/doctypes.js` around lines 14 - 15, Add a schema entry for the
assistants doctype so cozy-client can normalize/cache assistant docs: create a
schema object keyed by DOCTYPE_AI_CHAT_ASSISTANTS (the same constant exported
alongside DOCTYPE_AI_CHAT_CONVERSATIONS) mirroring the pattern used for the
conversations schema (include id, relationships, and any necessary attributes)
so RealTimeQueries/AssistantLayout.jsx subscriptions and the conversations
schema relationship target are properly recognized and normalized by
cozy-client.
| conversations: { | ||
| doctype: DOCTYPE_AI_CHAT_CONVERSATIONS, | ||
| attributes: {}, | ||
| relationships: { | ||
| assistant: { | ||
| type: 'has-one', | ||
| doctype: 'io.cozy.ai.chat.assistants' | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the constant instead of hardcoded doctype string.
Line 44 uses the literal string 'io.cozy.ai.chat.assistants' instead of the DOCTYPE_AI_CHAT_ASSISTANTS constant defined on line 15. This creates a maintenance risk if the doctype name changes.
♻️ Suggested fix
conversations: {
doctype: DOCTYPE_AI_CHAT_CONVERSATIONS,
attributes: {},
relationships: {
assistant: {
type: 'has-one',
- doctype: 'io.cozy.ai.chat.assistants'
+ doctype: DOCTYPE_AI_CHAT_ASSISTANTS
}
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conversations: { | |
| doctype: DOCTYPE_AI_CHAT_CONVERSATIONS, | |
| attributes: {}, | |
| relationships: { | |
| assistant: { | |
| type: 'has-one', | |
| doctype: 'io.cozy.ai.chat.assistants' | |
| } | |
| } | |
| }, | |
| conversations: { | |
| doctype: DOCTYPE_AI_CHAT_CONVERSATIONS, | |
| attributes: {}, | |
| relationships: { | |
| assistant: { | |
| type: 'has-one', | |
| doctype: DOCTYPE_AI_CHAT_ASSISTANTS | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/doctypes.js` around lines 38 - 47, Replace the hardcoded doctype
string in the conversations.relationships.assistant block with the
DOCTYPE_AI_CHAT_ASSISTANTS constant: locate the conversations object (symbol
DOCTYPE_AI_CHAT_CONVERSATIONS) and update the assistant.relationships.type value
that currently reads 'io.cozy.ai.chat.assistants' to reference
DOCTYPE_AI_CHAT_ASSISTANTS so the module uses the defined constant rather than a
literal string.
| const AssistantLayout = () => { | ||
| return ( | ||
| <LayoutUI monoColumn> | ||
| <RealTimeQueries doctype={DOCTYPE_AI_CHAT_CONVERSATIONS} /> | ||
| <RealTimeQueries doctype={DOCTYPE_AI_CHAT_ASSISTANTS} /> | ||
| <BarComponent | ||
| searchOptions={{ enabled: true }} | ||
| appSlug="assistant" | ||
| disableInternalStore | ||
| componentsProps={{ | ||
| Wrapper: { | ||
| className: cx('u-elevation-0', styles['assistant-topbar-border']) | ||
| } | ||
| }} | ||
| /> | ||
| <main className={styles['assistant-view']}> | ||
| <AssistantView /> | ||
| </main> | ||
| </LayoutUI> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if AssistantView expects a conversationId prop or uses useParams internally
rg -n --type=js 'AssistantView' -A 10 -B 2
# Check cozy-search for AssistantView props
ast-grep --pattern 'export $_ AssistantView'Repository: linagora/twake-drive
Length of output: 1702
🏁 Script executed:
# Find router/route configuration files
fd -type f -name "*route*" -o -name "*router*" | head -20Repository: linagora/twake-drive
Length of output: 234
🏁 Script executed:
# Search for conversationId usage in the codebase
rg -n "conversationId" --type=js | head -20Repository: linagora/twake-drive
Length of output: 271
🏁 Script executed:
# Search for useParams usage in Assistant-related files
rg -n "useParams" --type=js | grep -i assistantRepository: linagora/twake-drive
Length of output: 46
🏁 Script executed:
# Check the file structure around AssistantLayout for route configuration
rg -n "assistant/:conversationId\|:/conversationId" --type=jsRepository: linagora/twake-drive
Length of output: 46
🏁 Script executed:
# Search for conversationId usage more carefully
rg "conversationId" -nRepository: linagora/twake-drive
Length of output: 271
🏁 Script executed:
# Check the full AssistantLayout.jsx file
cat src/modules/views/Assistant/AssistantLayout.jsxRepository: linagora/twake-drive
Length of output: 1072
🏁 Script executed:
# Look for any context providers or hooks that might handle conversationId
rg "useParams|useContext|useState.*conversation" -n --type jsxRepository: linagora/twake-drive
Length of output: 91
🏁 Script executed:
# Check package.json for cozy-search version/documentation
rg "cozy-search" -nRepository: linagora/twake-drive
Length of output: 374
🏁 Script executed:
# Look for other route patterns with parameters to see how they're handled
rg "useParams" -nRepository: linagora/twake-drive
Length of output: 6074
🏁 Script executed:
# Check if there's any documentation or comments about AssistantView
rg -B3 -A3 "AssistantView" -nRepository: linagora/twake-drive
Length of output: 1142
Extract conversationId from route parameters or remove the unused route parameter.
The route is defined as assistant/:conversationId, but AssistantLayout does not extract this parameter. All comparable route components in the codebase (FilesViewer, DriveFolderView, SharedDriveFolderView, etc.) consistently use useParams() to extract their route parameters. Either extract the parameter and pass it to AssistantView if needed, or remove it from the route definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/views/Assistant/AssistantLayout.jsx` around lines 16 - 36,
AssistantLayout is not reading the route parameter conversationId defined on the
route; update AssistantLayout to import and call useParams() to extract
conversationId and pass it into <AssistantView conversationId={conversationId}
/> (or, if the route param is unnecessary, remove conversationId from the route
instead); specifically modify the AssistantLayout function to const {
conversationId } = useParams() and forward that value to the AssistantView
component (or coordinate route change if you choose to remove the param).
|
|
||
| const filteredBarRoutes = BarRoutes.filter( | ||
| r => r.props?.path !== 'assistant/:conversationId' | ||
| ) |
There was a problem hiding this comment.
Why do we need to filter bar routes here?
There was a problem hiding this comment.
And why we did not have to do it in home? cc @lethemanh
Polyfill jsdom with Node built-in Web Streams and Response APIs needed by assistant-stream.
zatteo
force-pushed
the
update-for-assistant
branch
from
b9802b4 to
76acb67
Compare
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| AppRoute.jsx | 1 rule in this hotspot | 9.12 → 9.12 | Suppress |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/navigation/AppRoute.jsx (1)
58-60: Consider adding a clarifying comment for the filtering logic.The filtering prevents duplicate route definitions when
BarRoutesfromcozy-baralready includes the assistant route that's explicitly added at line 84. This addresses the question raised in previous reviews about why filtering is needed. A brief inline comment would help future maintainers understand the intent.💡 Optional: Add clarifying comment
+// Filter out the assistant route from BarRoutes since it's explicitly defined above +// to render outside the main Layout wrapper const filteredBarRoutes = BarRoutes.filter( r => r.props?.path !== 'assistant/:conversationId' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/AppRoute.jsx` around lines 58 - 60, Add a brief inline comment above the filteredBarRoutes declaration explaining that BarRoutes may include an 'assistant/:conversationId' route from cozy-bar and we filter it out to avoid duplicate route definitions because the app adds the assistant route explicitly elsewhere; reference the filteredBarRoutes constant and the path 'assistant/:conversationId' so future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/navigation/AppRoute.jsx`:
- Around line 58-60: Add a brief inline comment above the filteredBarRoutes
declaration explaining that BarRoutes may include an 'assistant/:conversationId'
route from cozy-bar and we filter it out to avoid duplicate route definitions
because the app adds the assistant route explicitly elsewhere; reference the
filteredBarRoutes constant and the path 'assistant/:conversationId' so future
maintainers understand the intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10e370cb-dd76-4ee6-8d06-18238c9a0bc8
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
jestHelpers/setup.jsmanifest.webapppackage.jsonsrc/lib/doctypes.jssrc/modules/navigation/AppRoute.jsxsrc/modules/views/Assistant/AssistantLayout.jsxsrc/modules/views/Assistant/assistant.styl
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/lib/doctypes.js
🚧 Files skipped from review as they are similar to previous changes (3)
- jestHelpers/setup.js
- src/modules/views/Assistant/AssistantLayout.jsx
- manifest.webapp
|
It display a white screen when we try to close it from the AI button in search bar. |
3 participants
This is the equivalent of linagora/cozy-home#2351
Summary by CodeRabbit
New Features
Tests
Chores