feat(web-app-chat-with-file): add AI chat sidebar extension#454
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
6526b93 to
9072311
Compare
dj4oC
left a comment
There was a problem hiding this comment.
Really nice work overall — the architecture is sound (operator-controlled proxy, API key kept server-side), the UI is polished, and the auth/XSS fundamentals are right. My main concern is one serious data-loss bug in the edit/rewrite feature that I'd treat as blocking, plus the new package's unit tests hanging CI and a test suite that happens to mask the data-integrity bugs. Details below.
🔴 Blocking
1. Editing a file larger than ~12k chars silently destroys everything past the cutoff.
useChat.fetchFileText() truncates content to MAX_CONTENT_CHARS = 12_000 (useChat.ts:151). In edit mode that truncated text is what's sent to the model (<file_content>…, ~line 213), and applyEdit() then overwrites the entire file with the model's output (putFileContents({ content: proposal }), ~line 301). So for any .txt/.md file longer than 12k chars, "fix this typo" rewrites the first 12k and deletes the rest. Worse, the diff preview is computed against the truncated original (originalContent: fileText, ~line 261), so the user sees a clean little diff and no hint the tail is about to vanish. Compounding it: max_tokens: 4096 (~line 234) with no finish_reason check means even a sub-12k file can be written back truncated if the completion is cut off. Suggested fix: disable/warn for files over the limit, diff against the full original, and refuse to apply when finish_reason === 'length'.
2. test:unit will hang CI. web-app-chat-with-file/package.json uses bare vitest (watch mode); it never exits. ai-doc-summary correctly uses vitest run. Please change it.
3. The test suite masks the bugs that matter. It's broad and well-organized (~1,300 lines), but the data-integrity paths are laundered by loose mocks: no fixture is bigger than ~20 chars (truncation never hit), the webdav mock returns headers: { ETag } with the exact capital-E casing the buggy reader expects, the putFileContents assertion (useChat.spec.ts:438) checks only content and ignores previousEntityTag, and the e2e test never sends a message or applies an edit. All three bugs below pass green. (Also useChat.spec.ts:589 "restores cached messages" has no expect.)
🟠 Should-fix
4. ETag conflict protection may be a no-op. cachedFileEtag = headers?.ETag (useChat.ts:152) is case-sensitive; oCIS/axios commonly deliver etag lowercase → null → previousEntityTag: '' → unconditional overwrite, no 412. Read the header case-insensitively and assert a non-empty precondition is actually sent.
5. The oCIS access token is sent to whatever llm.endpoint is configured. buildHeaders() attaches the user's bearer token, and the README shows a raw http://…:11434/v1 as an endpoint example. A misconfigured endpoint leaks the user's oCIS token to a third party. Derive the proxy URL from the oCIS base, or hard-validate that endpoint is the same-origin proxy.
6. The proxy forwards the request body unvalidated. ai-llm-proxy passes the client's full JSON (model, max_tokens, …) to the LLM with no model allowlist, max_tokens clamp, request-size cap, or per-user rate limit (readBody buffers unbounded). Any authenticated user can run up arbitrary cost / OOM the proxy. The endpoint is operator-controlled, but the request isn't.
7. "Streaming" isn't. The description mentions a "streaming chat UI", but the client uses res.json() (no stream: true) and the proxy buffers the whole response via llmRes.text() (index.ts:154). Either implement real SSE passthrough or drop the streaming wording.
8. Config / infra inconsistencies: support/actions/ocis.apps.yaml keys the config as web-app-chat-with-file (and localhost), but the extension's APP_ID is chat-with-file (index.ts:16) — so it stays unconfigured in that stack (the e2e passes only because it never calls the LLM); dev/docker/ocis.apps.yaml uses the correct key. The new /ollama Traefik route (dev/docker/traefik/configs/ollama.yml) is dead and bypasses the auth proxy — remove it. The proxy Dockerfile runs as root (add USER node). ai-doc-summary's check:types calls vue-tsc, which isn't in its devDeps.
9. ai-doc-summary README is out of sync with the refactor — it still documents capability "probing", a Connecting state, and "auto-triggers a summary on mount", none of which the refactored useLlm.ts/useSummary.ts do (a test even asserts mount does not auto-trigger).
🟡 Nits
applyEdithas no extension guard (the UI gates edit to txt/md viaisEditable, but the composable would write to any path) — worth a defensive check.- Fence/
<file_content>stripping (useChat.ts:247) can mangle a markdown file that legitimately opens with a ``` fence. - On error,
submit()has already cleared the textarea, so the user's typed prompt is lost. - Proxy: the comment says discovery happens "at startup" but it's lazy; CORS reflects an arbitrary
Origin; l10n scaffolding is incomplete (emptytranslations.json, no.pot).
🟢 What's good
- XSS-safe: all LLM output and file content render through
<pre>{{ }}text interpolation, neverv-html— correct and important here. - Edit mode is correctly gated to txt/md in the UI (
isEditable+ disabled pill + the watch that forceschat), so PDFs can't be edit-corrupted in normal use. - Auth model is solid: the proxy validates the oCIS token via
userinfo, keeps the API key server-side, and only ever calls fixed endpoints (no client-driven SSRF). - Thoughtful UX: friendly error-code mapping, optimistic-message rollback, 412 messaging, LRU message cache, 60s timeouts.
diff.tsLCS is correct with a large-file guard and good unit tests.
Bottom line: I'd hold the edit feature until the >12k truncation/overwrite is fixed (it's a real data-loss path on the user's own files); the rest is comparatively routine.
The commit is not in sync with the branch actually implementing the doc summary extension. Once that PR is merged, this one will be rebased and it will be dropped from the current diff. I keep it here now only because we need the ai proxy which is integrated in that PR. |
This is not about the app ID but about how the extension is mounted. In case of GHA, it is really mounted as web-app-chat-with-file so the key in the yaml file is correct. dev/docker/ocis.apps.yaml mounts the extension in different way. That's why it has different key. |
b011d11 to
e18384c
Compare
|
Review addressed. |
Adds a new oCIS Web extension that opens a Chat sidebar panel for PDF, TXT, and MD files, allowing users to have a back-and-forth conversation about the file's content or ask the LLM to rewrite it in place. The LLM endpoint is fully operator-controlled via the ai-llm-proxy sidecar — no API keys reach the browser. Includes unit and E2E tests, Docker dev environment wiring, and CI integration. Signed-off-by: Lukas Hirt <info@hirt.cz>
e18384c to
42d09f0
Compare
Adds a new web-app-chat-with-file oCIS Web extension that opens a Chat sidebar panel when a user opens a PDF, TXT, or MD file. Users can have a back-and-forth conversation about the file's content or ask the LLM to rewrite/edit the file in place.
What's new
Video
Zaznam.obrazovky.2026-06-16.v.23.mp4
Security / ops
The LLM endpoint is fully operator-controlled via the ai-llm-proxy sidecar — no API keys are exposed to the browser.
Infrastructure
Tests