feat(web-app-version-changelog): add version changelog sidebar panel#459
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. |
8762ceb to
67f3883
Compare
67f3883 to
aea0833
Compare
dj4oC
left a comment
There was a problem hiding this comment.
Clean, well-scoped, read-only extension (no file writes, so none of #454's data-loss risk), and the rendering is XSS-safe. The one issue I'd treat as blocking is the same token-handling pattern flagged on #454, sharper here because there's no proxy in this PR. Reviewed across six dimensions (security, rendering, diff correctness, flow, tests, infra) with each finding independently refuted; a few candidate findings were dropped as false positives (e.g. the row-0/HEAD pairing is correct by design, and bare vitest is a nit not a CI hang since it auto-detects CI).
🔴 Blocking
1. The oCIS session token is sent to the configured LLM endpoint with no validation. useChangelog.buildHeaders() attaches Authorization: Bearer <oCIS accessToken> (lines 35-42) and callLlm POSTs it to ${config.endpoint}/chat/completions (lines 74, 86). index.ts:15-17 takes endpoint verbatim from app config with only a truthiness check, with no new URL, protocol, same-origin, or allowlist guard anywhere. This PR adds no proxy (docker-compose is +1 line, the dist mount), and the README's own example shows a BYO https://.../v1 endpoint, so following the docs sends a live, replayable oCIS bearer token (plus the file's diff content) to a third party that can reuse it against the oCIS API for the token's lifetime. The dev/support configs happen to point same-origin at the existing ai-llm-proxy, but nothing in the code enforces that. Fix: constrain the endpoint to same-origin/relative (or an allowlist), or route through a token-terminating proxy and do not forward the token cross-origin.
🟠 Should-fix
- 8000-char diff cap truncates mid-line silently (
useChangelog.ts:122): a large-but-under-2000-line diff is cut at exactly 8000 chars with no marker, so the LLM summarizes a partial diff as if it were complete. Only the >2000-line case surfaces an error. diff.tsis completely untested — there is nodiff.spec.ts, anduseChangelog.spec.tsmockscomputeDiff/diffToTextaway. The LCS algorithm and the 2000-line/8000-char caps (the only real logic in the extension) are unverified. The algorithm itself looks correct on inspection, but nothing guards regressions.- Tests miss the risky paths: no unit or e2e assertion that the auth token/headers are actually sent (the e2e Generate round-trip fulfils a canned 200 without inspecting
route.request().headers()); the timeout/network error branches and the version-pairing/cache-key logic are untested. Same loose-mock pattern as #454. support/actions/ocis.apps.yamlkeys the configweb-app-ai-version-changelogbutAPP_IDisversion-changelog(index.ts:9), so it never loads in that stack (the dev apps.yaml is correct). #454 recurrence.- README documents the wrong config shape (
options.llm, global) vs the per-appversion-changelog.config.llmblock the code actually reads. - Blob fetches have no error handling (
ChangelogPanel.vue:149-177): a failed/empty fetch returns''and is diffed silently, so a transient error shows as "No text changes" or a whole-file-added diff rather than an error. - i18n not wired:
defineWebApplicationomits thetranslationsfield, so all$gettextstrings stay English-only for non-English users (the panel is otherwise fully marked up for translation). - Performance: both full version blobs are pulled into memory (
responseType: 'text') before the size guards run; the 2000-line/8000-char limits only apply after download. - Licensing:
package.jsonand the README claim Apache-2.0 with a LICENSE badge, but noLICENSEfile is added.
🟡 Nits
test:unit uses bare vitest (works in CI via auto-detection, but vitest run is the explicit/safe form). No finish_reason check, so a max_tokens: 256-truncated summary is cached as complete. Cache key falls back to the array index when a version lacks an etag (collision risk), and row-0's entry can go stale after a HEAD edit. The oldest-version row has no baseline (it diffs against the second-oldest) but is presented as a normal diff. The BCP-47 language tag is interpolated raw into the prompt. No client-side rate limiting beyond the per-key cache. Binary detection treats unknown and some text-ish MIME types (js/yaml) as binary. "Generating…" is not announced to assistive tech (no aria-live/role="status"). Dates use toLocaleString() with the runtime locale rather than the oCIS UI language. ts-node is an unused devDependency.
🟢 Done well
- XSS-safe throughout: the LLM summary, the diff, and all error/metadata strings render via
{{ }}text interpolation; nov-html/innerHTML/markdown/sanitizer anywhere, and the (attacker-influenceable) diff is never put in the DOM. - Correct concurrency: double-click is de-duped by a synchronous guard before the first
await; switching files mid-request cannot corrupt another file's entry (keyed byfileId+etag, captured at click time); Retry clears the error cleanly. - LCS diff is algorithmically correct, and the line guard bounds memory before allocation.
- No hardcoded secrets (token read live, never persisted);
max_tokenshardcoded andmodelfrom trusted config;isVisiblecorrectly hides folders and multi-select;vue-tscpresent; package added to the CI matrix.
Bottom line: the only thing I'd block on is not forwarding the oCIS token to an unvalidated/cross-origin endpoint. Everything else is routine.
Added a new sidebar panel extension that generates human-readable changelog entries for each file version. When a user selects a file with version history, the "Changelog" panel appears alongside the standard file history. Each version row has a **Generate** button that diffs the two version blobs on-demand and sends the diff to a configured BYO-LLM endpoint to produce a concise description of what changed. Signed-off-by: Lukas Hirt <info@hirt.cz>
aea0833 to
e1aa4d4
Compare
…prevent token exfiltration Validates the configured endpoint against window.location.origin at startup. A cross-origin URL disables changelog generation and logs an error rather than forwarding the oCIS session token to a third-party server. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… the input is partial Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ithm and caps Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…error, and cache key paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…with APP_ID Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…per-app applicationConfig Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e instead of silently returning empty string Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…are localised for non-English users Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ing LCS to avoid O(m*n) allocation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…k finish_reason, announce generating state to AT Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…yaml key to web-app-version-changelog The GHA workflow mounts the dist dir as /apps/$matrix.app where matrix.app is the package directory name (web-app-version-changelog). The ocis.apps.yaml key must match that path, not APP_ID. Previous change was incorrect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review. Here's what was addressed: 🔴 Blocking — fixed
🟠 Should-fix — fixed
🟡 Nits — fixed
|
Signed-off-by: Lukas Hirt <info@hirt.cz>
Summary
Adds a new web-app-version-changelog extension that registers a Changelog sidebar panel
in the file details sidebar. When a single file is selected, the panel lists its
version history and lets users generate a plain-English description of what changed in
each version — powered by a BYO LLM endpoint.
on-demand, diffs them, and sends the diff to a configured OpenAI-compatible endpoint
prompt)
Screenshots
Configuration
Add the following to the app's entry in apps.yaml:
Without configuration the panel still appears, but all Generate buttons are disabled
with a notice to configure the AI endpoint in admin settings.
Limitations
Test plan