Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a server-injected window.ELIZA_CONFIG.apiBase and client getApiBase(), rewires client knowledge API calls to use that base, updates server HTML/asset injection to derive pluginBasePath from request, and improves asset path parsing and logging. Also bumps package version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Browser
participant SRV as Server (routes.ts)
participant UI as KnowledgeTab (TSX)
participant API as Backend APIs
User->>SRV: GET /.../display
SRV->>SRV: Derive pluginBasePath (strip trailing /display)
SRV-->>User: HTML + window.ELIZA_CONFIG = { apiBase: pluginBasePath } (assets rewritten)
User->>UI: Load bundle
UI->>UI: getApiBase() → ELIZA_CONFIG.apiBase or /api
UI->>API: GET {apiBase}/documents?...
API-->>UI: Documents JSON
UI->>API: GET {apiBase}/knowledges?...
API-->>UI: Chunks JSON
UI->>API: POST {apiBase}/documents (upload)
API-->>UI: Response
UI->>API: GET {apiBase}/search?...
API-->>UI: Search results
sequenceDiagram
autonumber
actor Browser
participant SRV as Server (frontendAssetHandler)
Browser->>SRV: GET /.../assets/<name>?v=hash
SRV->>SRV: Compute fullPath, find last "/assets/", strip query → assetName
SRV-->>Browser: Asset file (or 404) — errors/logs include fullPath
sequenceDiagram
autonumber
participant UI as KnowledgeTab
participant Viewer as PDF Viewer (MemoryDetails)
UI->>Viewer: Open document
alt content-type == PDF && base64 present
Viewer->>Viewer: Build data: URL
Viewer-->>UI: Render iframe + zoom controls (±, Reset)
else content-type == PDF && no content
Viewer-->>UI: "PDF Not Available"
else
Viewer-->>UI: Render text content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 summary |
✅ Actions performedSummary regeneration triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/routes.ts (3)
516-523: Normalize and harden pluginBasePath derivationSafer to strip query/hash and trailing slashes and prefer baseUrl when present to avoid accidental matches.
- const requestPath = req.originalUrl || req.url || req.path; - const pluginBasePath = requestPath.replace(/\/display.*$/, ''); + const rawPath = (req.originalUrl || req.url || req.path) as string; + // Prefer baseUrl when available (Express), then normalize + const base = (req.baseUrl as string | undefined) || rawPath; + const pluginBasePath = base + .replace(/\/display(?:[?#].*)?$/, '') // drop "/display" and anything after it + .replace(/\/+$/, ''); // drop trailing slashes
546-549: Broaden asset path rewrites to cover both quote types and absolute/relative formsCovers src|href with ' or " and paths like ./assets and /assets.
- injectedHtml = injectedHtml.replace(/src="\.\/assets\//g, `src="${pluginBasePath}/assets/`); - injectedHtml = injectedHtml.replace(/href="\.\/assets\//g, `href="${pluginBasePath}/assets/`); + injectedHtml = injectedHtml + .replace(/(src|href)=["']\.?\/assets\//g, `$1="${pluginBasePath}/assets/`);
593-597: Avoid double “assets/” when using manifest-derived filenamesIf manifest already includes “assets/…”, prefix just the base path.
- <link rel="stylesheet" href="${pluginBasePath}/assets/${cssFile}"> + <link rel="stylesheet" href="${pluginBasePath}/${cssFile.startsWith('assets/') ? cssFile : `assets/${cssFile}`}"> @@ - <script type="module" src="${pluginBasePath}/assets/${jsFile}"></script> + <script type="module" src="${pluginBasePath}/${jsFile.startsWith('assets/') ? jsFile : `assets/${jsFile}`}"></script>Also applies to: 609-609
src/frontend/ui/knowledge-tab.tsx (2)
29-37: Type ELIZA_CONFIG accurately and normalize apiBase (guards SSR and double slashes)Prevents // in URLs and avoids window access errors under SSR.
declare global { interface Window { ELIZA_CONFIG?: { - agentId: string; + agentId: UUID; apiBase: string; }; } } @@ -const getApiBase = () => { - // Check if we have the ELIZA_CONFIG from the server - if (window.ELIZA_CONFIG?.apiBase) { - return window.ELIZA_CONFIG.apiBase; - } - return '/api'; -}; +const getApiBase = () => { + const base = + (typeof window !== 'undefined' && window.ELIZA_CONFIG?.apiBase) ? window.ELIZA_CONFIG.apiBase : '/api'; + // trim trailing slashes + return base.replace(/\/+$/, ''); +};Also applies to: 251-259
296-298: Dynamic apiBase usage: LGTM; consider hoisting to avoid repeated callsMinor: cache getApiBase() at module scope to avoid recomputing per call.
Example (outside this hunk):
const API_BASE = getApiBase(); // then use `${API_BASE}/…` throughoutAlso applies to: 309-311, 332-334, 356-358, 701-702, 757-759
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/frontend/ui/knowledge-tab.tsx(9 hunks)src/routes.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
src/frontend/ui/knowledge-tab.tsx (2)
1153-1155: UI padding tweak: LGTMNo functional impact; reads fine.
266-273: Agent scoping enforced in service layer
KnowledgeService.getMemories automatically addsagentId: this.runtime.agentIdto all document queries, ensuring each agent only sees its own documents; no further filtering needed.
| // Inject config into existing HTML with the correct API base path | ||
| // Also fix relative asset paths to use absolute paths | ||
| let injectedHtml = html.replace( | ||
| '<head>', | ||
| `<head> | ||
| <script> | ||
| window.ELIZA_CONFIG = { | ||
| agentId: '${agentId}', | ||
| apiBase: '/api' | ||
| apiBase: '${pluginBasePath}' | ||
| }; | ||
| </script>` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid XSS in injected config by serializing via JSON
Injecting request-derived strings into a JS literal is fragile. Serialize instead.
- let injectedHtml = html.replace(
- '<head>',
- `<head>
- <script>
- window.ELIZA_CONFIG = {
- agentId: '${agentId}',
- apiBase: '${pluginBasePath}'
- };
- </script>`
- );
+ const elizaConfig = JSON.stringify({ agentId, apiBase: pluginBasePath });
+ let injectedHtml = html.replace(
+ '<head>',
+ `<head><script>window.ELIZA_CONFIG=${elizaConfig};</script>`
+ );📝 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.
| // Inject config into existing HTML with the correct API base path | |
| // Also fix relative asset paths to use absolute paths | |
| let injectedHtml = html.replace( | |
| '<head>', | |
| `<head> | |
| <script> | |
| window.ELIZA_CONFIG = { | |
| agentId: '${agentId}', | |
| apiBase: '/api' | |
| apiBase: '${pluginBasePath}' | |
| }; | |
| </script>` | |
| ); | |
| // Inject config into existing HTML with the correct API base path | |
| // Also fix relative asset paths to use absolute paths | |
| const elizaConfig = JSON.stringify({ agentId, apiBase: pluginBasePath }); | |
| let injectedHtml = html.replace( | |
| '<head>', | |
| `<head><script>window.ELIZA_CONFIG=${elizaConfig};</script>` | |
| ); |
🤖 Prompt for AI Agents
In src/routes.ts around lines 533 to 544, the code injects request-derived
strings directly into a JS literal which risks XSS; instead serialize the config
object with JSON.stringify and inject that serialized string. Replace the
current template-literal interpolation with code that builds the config =
JSON.stringify({ agentId, apiBase: pluginBasePath }) and then writes
window.ELIZA_CONFIG = <that JSON string>; into the injected script so values are
safely escaped (ensure the serialized string is inserted as raw JSON, not as a
nested template string, and avoid unescaped "</script>" by using JSON.stringify
output).
|
bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0xbbjoker
force-pushed
the
fix/knowledge-panel-loading
branch
from
59c3af5 to
4e40fa3
Compare
| ) => { | ||
| const params = new URLSearchParams(); | ||
| params.append('agentId', agentId); | ||
| // Don't append agentId to params if it's already in the URL path |
There was a problem hiding this comment.
Bug: API Parameter Mismatch Causes Filtering Issues
The getKnowledgeDocuments API call no longer appends the agentId parameter to its query, but its server-side handler still expects it. This creates an inconsistency with other API calls in the file and may lead to incorrect document filtering or API failures.
1 participant
Summary by CodeRabbit
Improvements
Bug Fixes
Chores