feat(learn): widget production-ready + prof config#66
Conversation
- mark FEAT-006, FEAT-007, FEAT-009, FEAT-010 completed (PR #50, 2026-03-23) - note FEAT-005 partial overlap with FEAT-020 hybrid grounding mode - add FEAT-022 suggested questions (quick-start chips) - add FEAT-023 socratic interaction mode (per-course learning style)
Covers widget production-ready items (FEAT-016, FEAT-012, FEAT-004) plus
the instructor configuration backend endpoint. Splits course customization
into two categories: visual (Moodle block config, data-attrs) and behavioral
(namespaces.config JSONB, admin API via Moodle server-side).
Implementation order: WI-3 (PATCH /namespaces/config) -> WI-2 (document name
in citations) -> WI-1 (GET /conversations/{id}/turns JWT-scoped) -> WI-4
(widget data-attrs) -> WI-5 (widget conversation persistence).
…config (WI-3) Partial-update endpoint for the namespaces.config JSONB column, admin-scoped. The FEAT-020 read path (resolve_grounding_mode in vektra-shared) already consumes this column; this adds the missing write path. - Whitelist keys (grounding_mode only for v0.5.0). Unknown keys rejected 400 so upstream clients see typos immediately. - Null value removes the key, falling back to the env default. - Partial merge preserves unrelated existing keys. - New error codes ERR-ADMIN-005/006/007 documented in error-codes.md. - Documented in api.md. - Integration tests cover set, partial merge, null removal, unknown key, invalid value, 404, 403, and end-to-end via resolve_grounding_mode. Part of v0.5.0 widget-and-prof-config plan (see .s2s/plans/20260418-v050-widget-and-prof-config.md). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Students see chunk UUIDs as citations today. This adds the filename of the source document to each SourceRef so the widget can render "[1] lecture-07.pdf (0.82)" instead of "[1] 4a2f3b... (0.82)". - Extend SourceRef and SourceRefBody with document_name: str | None. - Helper _fetch_document_names(doc_ids) batches a lookup against source_documents.filename and returns an empty map on any failure (DB not initialised in tests, transient error). Pipelines continue returning responses even when enrichment fails; the widget already falls back to chunk_id when document_name is absent. - Both SimpleQueryPipeline and AdvancedQueryPipeline populate the field in execute() and execute_stream(). - vektra-learn CourseQueryResponse forwards the field so the learn widget gets it over SSE and JSON paths. Widget side is already prepared (vektra-learn/widget/src/chat-ui.js:227 uses src.document_name || src.chunk_id), so no JS change needed. Part of v0.5.0 widget-and-prof-config plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-004) Student-facing mirror of the admin turns endpoint. Needed so the widget can restore a conversation after a page refresh (WI-5). - Auth: dashboard JWT (same dependency as /query). Authorization is namespace-scoped: the conversation's namespace must match the namespace resolved from the token (namespace claim > course_id fallback). - 403 on namespace mismatch so the widget can distinguish "wrong course" from "deleted conversation" (404) and reset its local state. - Returns decrypted question/answer/created_at plus an empty sources array for v0.5.0. Source enrichment from query_traces is out of scope here. - Admin-only metadata (model, prompt_tokens, response_id) is intentionally not exposed in the student response. - New error codes ERR-LEARN-005 (404) and ERR-LEARN-006 (403) registered in vektra_shared.errors and docs/reference/error-codes.md. Part of v0.5.0 widget-and-prof-config plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…FEAT-016) Universities need their own title, color, welcome message without forking the widget. Five new optional attributes on the <script> tag: - data-title header text and button aria-label - data-primary-color accent color for buttons, user bubble, links - data-icon emoji or image URL for the floating button - data-welcome-message assistant message shown on first open - data-powered-by "false" hides the Vektra attribution footer Safety: - Title and welcome-message are rendered via textContent, never innerHTML. - Primary color is matched against a conservative whitelist (hex, rgb(), hsl(), named) before being injected as a CSS custom property; anything with quotes/semicolons/whitespace is silently dropped. - Icon URLs use <img src> (browser-sanitized); emoji/text uses textContent. - Hover states use filter: brightness() so a custom primary color still feels interactive without needing a second data-attr. No backend or API contract changes. Missing attributes keep the current v0.4.0 defaults, so existing deploys upgrade without visible changes other than the new "Powered by Vektra" footer (which can be turned off). Part of v0.5.0 widget-and-prof-config plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…004)
A page refresh currently loses the whole conversation. Students asking
follow-up questions about course materials can't afford that. This adds
tab-scoped persistence and a "New chat" button.
- sessionStorage key "vektra-conv:<course_id>" stores { conversation_id,
stored_at }. Tab-scoped avoids leaking conversations across students on
shared university lab computers; localStorage would have been wrong.
- Stale cutoff at 24h so tabs left open overnight don't silently resurface
yesterday's chat at the top of the screen.
- On widget init: read storage, call the new WI-1 endpoint, replay turns
into the chat panel before enabling input. 404/403/empty turns silently
abandon the stored id (widget recovers; no error shown to the user).
- Each successful query persists the server-assigned conversation_id so
that the next reload picks up where we left off.
- "New chat" button in the panel header clears storage and resets the
client conversation id so the next question opens a fresh thread. Works
as a recovery path if the server reassigns the id.
- ApiClient gains setConversationId(id) and getConversationTurns(id).
Part of v0.5.0 widget-and-prof-config plan. All five work items done;
widget bundle rebuilt and lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Mark FEAT-012 completed, FEAT-016 partial (data-attrs only), FEAT-004 partial (persistence + new chat, sources still empty). - Record all 5 WIs under an Unreleased v0.5.0 section so the release notes are ready when this branch merges. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ream (WI-2) Missed in 192b62c. The advanced pipeline is the default in production (Combo D), and students use streaming (stream: true hardcoded in the widget client), so citations served over SSE never got the filename — the widget fell back to chunk UUIDs exactly in the code path that matters most. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review findings addressed in one commit — all three changes are
cross-cutting and touching any one of them alone would force a stale
code/doc split.
1. WI-2 archived marker (REQ-057, plan gate): _fetch_document_names now
also reads deleted_at and appends "(archived)" to the filename when
the source document is soft-deleted. Citations stay traceable for
students without hiding the link to a removed asset.
2. WI-1 audit log (NFR-007): successful turns reads write a
learn_conversation_turns_read audit row via vektra_shared.audit so
decrypted conversation access is traceable. Uses the learn JWT
sentinel key_id (same one used for ensure_conversation) and captures
namespace, conversation_id, turn count, student_id, course_id.
3. WI-3 route naming: PATCH namespace config moves from
/api/v1/namespaces/{id}/config to /api/v1/admin/namespaces/{id}/config
to match the admin-endpoint convention established by DEBT-011's
/api/v1/admin/conversations/{id}/turns. Tests, api.md, and CHANGELOG
updated accordingly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ype test
Second round of review fixes.
WI-4: two new optional attrs, data-powered-by-text and data-powered-by-url,
so universities can replace the footer text/link without touching the
widget source. Default behaviour unchanged ("Powered by Vektra" → vektralabs).
Custom URLs are whitelisted to http(s) and same-origin paths; javascript:
and data: URLs are rejected.
WI-5: restoreConversation is now awaited in init(), preventing a fast-typing
user from racing the fetch and seeing their first user message wiped out
when replayTurns arrives. Paired with a bounded AbortSignal.timeout(8s) on
getConversationTurns so a stalled backend cannot block widget init
indefinitely.
WI-3: integration test asserts a non-string value (42) is still rejected
with ERR-ADMIN-007. Guards the whitelist contract against future
permissive/type-coercing refactors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds admin-scoped namespace config PATCH, JWT-scoped conversation turns GET with audit logging, enriches source citations with Changes
Sequence Diagram(s)sequenceDiagram
participant Widget as Widget (browser)
participant ApiClient as ApiClient
participant LearnAPI as Learn API (server)
participant ConvStore as ConversationStore
participant Audit as Audit Logger
Widget->>ApiClient: request getConversationTurns(conversationId)
ApiClient->>LearnAPI: GET /api/v1/learn/conversations/{id}/turns (JWT)
LearnAPI->>ConvStore: retrieve metadata & decrypted turns
ConvStore-->>LearnAPI: turns / not found / access denied
alt success
LearnAPI->>Audit: schedule audit task (turns_read, request_id, meta)
LearnAPI-->>ApiClient: 200 JSON {conversation_id, namespace, turns}
else not found
LearnAPI-->>ApiClient: 404 ERR-LEARN-005
else namespace mismatch
LearnAPI-->>ApiClient: 403 ERR-LEARN-006
end
ApiClient-->>Widget: returns parsed body or null/throws
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Code Review
This pull request implements the v0.5.0 milestone, focusing on making the learn chatbot widget production-ready and adding instructor-level configuration. Key changes include a new PATCH endpoint for namespace behavioral settings, a GET endpoint for conversation history retrieval, and white-label customization for the widget via data-* attributes. Additionally, source citations now include document filenames instead of UUIDs. The review feedback correctly identifies issues with hardcoded hover styles that would break custom branding and suggests a more deterministic approach for testing background tasks to avoid flakiness.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/api.md (1)
283-297:⚠️ Potential issue | 🟡 MinorUpdate the query response example with
document_name.The API now returns per-source
document_name, but the documented/api/v1/queryresponse still omits it.📝 Proposed docs update
"snippet": "...", "citation_id": "d4e5f6-...", - "document_version": 1 + "document_version": 1, + "document_name": "lecture-07.pdf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/api.md` around lines 283 - 297, Update the /api/v1/query response example in docs/reference/api.md to include the new per-source field document_name inside each object in the "sources" array (alongside existing fields like doc_id, chunk_id, score, snippet, citation_id, document_version) so the example matches the current API contract; locate the JSON block under the "Response:" section for the query endpoint and add "document_name": "..." to each source entry.
🧹 Nitpick comments (7)
vektra-core/tests/test_pipeline.py (1)
423-497: Add a streaming assertion fordocument_name.These tests cover
execute()and the helper, but the PR also changed streamedsourcespayloads. A small_collect_stream()case assertingsources.data[*]["document_name"]would protect the streaming regression this PR fixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/tests/test_pipeline.py` around lines 423 - 497, Add a streaming assertion that mirrors test_execute_populates_document_name: use the existing helper _collect_stream() to consume pipeline.stream_execute (or pipeline.execute streaming variant used in this test suite) and assert that each streamed item’s sources.data[*]["document_name"] contains the expected names when monkeypatching pipeline_mod._fetch_document_names to return the mapping (doc_a -> "lecture-07.pdf", doc_b -> "slides.pptx") and that they are all None when the helper returns an empty dict; place this check alongside the existing non-stream assertions in test_execute_populates_document_name and reuse the same doc ids, monkeypatch functions, and _make_pipeline/vector_store setup so the streaming regression is covered.vektra-learn/widget/src/index.js (1)
122-197: Restore-before-bind flow resolves the replay race cleanly.Awaiting
restoreConversation()beforecheckConnection()(and before any user input is possible in practice, since the UI is already mounted but the user won't have time to type before the 8s-bounded fetch resolves) prevents the "replayTurns wipes an in-flight user message" race called out in the comment. The 8s timeout inapi-client.jsprovides a hard upper bound so a stalled backend cannot block init indefinitely.Minor: when the
elsebranch at line 236 callsinit()without awaiting, any unhandled rejection would become an unhandled promise rejection in the console._readStored,_writeStored,_clearStored, andrestoreConversationall havetry/catch, andChatUI/ApiClientconstructors don't throw async, so this is effectively safe — but wrappinginit()in.catch(err => console.error(...))would be a belt-and-braces improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/widget/src/index.js` around lines 122 - 197, The PR comment warns that calling init() without awaiting can surface unhandled promise rejections; wrap the call-site where init() is invoked (the init() function itself or the top-level invocation that triggers it) with a rejection handler so any thrown async errors are logged/handled (e.g., call init().catch(err => console.error("init failed", err))). Reference symbols: init(), restoreConversation(), _readStored(), _writeStored(), _clearStored(), ChatUI, ApiClient.vektra-learn/src/vektra_learn/api.py (3)
600-609: Strict dict access can 500 the endpoint on schema drift.
t["turn_number"],t["question"], andt["created_at"]will raiseKeyError(→ 500 Internal Server Error) ifget_turns_detailever returns a row missing one of these keys — e.g., a legacy/partial row, or a future schema change. Sincet.get("answer")already hedges for answer, consider being consistent and defensive across the board:♻️ Proposed fix
items = [ ConversationTurnItem( - turn_number=t["turn_number"], - question=t["question"], + turn_number=t.get("turn_number", 0), + question=t.get("question", ""), answer=t.get("answer"), - created_at=t["created_at"], + created_at=t["created_at"], # required — fail loud if missing sources=[], ) for t in turns ]Or validate the contract more tightly via a typed interface on
ConversationStore.get_turns_detail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/src/vektra_learn/api.py` around lines 600 - 609, The list comprehension that builds ConversationTurnItem uses strict dict indexing (t["turn_number"], t["question"], t["created_at"]) which can KeyError on schema drift; change to defensive access by using t.get("turn_number"), t.get("question"), t.get("created_at") and handle missing values (e.g., provide sensible defaults, skip incomplete rows, or raise a clear validation error) so the endpoint doesn't 500; update the comprehension that iterates over turns returned by get_turns_detail and ensure ConversationTurnItem construction tolerates missing keys (or validate/normalize each t before creating ConversationTurnItem).
580-587: Direct key access on metadata dict — use.get()for robustness.
meta["namespace_id"]will raiseKeyError→ 500 if a conversation row is persisted without that field for any reason. Given line 571 already validatedmeta is not None, use.get()plus a comparison so a missing namespace cannot be mistaken for a match:♻️ Proposed fix
- if meta["namespace_id"] != namespace: + if meta.get("namespace_id") != namespace: err = ErrorResponse( category=ErrorCategory.PERMANENT, code=ERR_LEARN_006,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/src/vektra_learn/api.py` around lines 580 - 587, Replace direct dict access meta["namespace_id"] with meta.get("namespace_id") so a missing key doesn't raise KeyError and can't be mistaken for a match; update the condition in the block that raises the ErrorResponse (referencing meta, namespace, ErrorResponse, ERR_LEARN_006, http_status_for, HTTPException, err.to_envelope()) to use meta.get("namespace_id") and treat None as a mismatch (e.g., if meta.get("namespace_id") is None or meta.get("namespace_id") != namespace) before raising the existing error.
498-512: Duplicate namespace-resolution logic withcourse_query.
_resolve_namespace_from_tokenre-implements the exact fallback chain thatcourse_querydoes inline at lines 663-673 and 694-695 (sans the enrollment lookup). Consider consolidating: havecourse_queryuse this helper (or a variant that returns(course_id, namespace)) so both endpoints stay in lockstep if the JWT schema evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/src/vektra_learn/api.py` around lines 498 - 512, Duplicate namespace-resolution logic exists: _resolve_namespace_from_token re-implements the same fallback chain used inline in course_query. Refactor by extending _resolve_namespace_from_token to return both (course_id, namespace) or add a new helper (e.g., resolve_course_and_namespace_from_token) and have course_query call that helper instead of duplicating logic; ensure the helper preserves the current behavior (raise the same HTTPException when course_id missing and compute namespace = token_payload.get("namespace") or course_id) so both endpoints remain consistent if the JWT schema changes.vektra-learn/widget/src/chat-ui.js (2)
138-144: Primary-color override is leaked and re-applied across instances.Each
ChatUIinstantiation appends a new<style>element todocument.headthat sets:root { --vektra-primary: ... }. There is no cleanup path, and repeated instantiation (e.g., hot-reloading during dev, or a host page that re-initializes the widget) will accumulate style nodes. Also, because the override is on:root, it affects the entire document, not just the widget — if the host page already defines--vektra-primary, the last-injected instance wins globally.Consider (a) scoping the custom property to the widget root elements (
.vektra-chat-btn, .vektra-chat-panel { --vektra-primary: ... }) and (b) caching/replacing a single<style id="vektra-primary-override">node instead of appending on every construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/widget/src/chat-ui.js` around lines 138 - 144, The ChatUI constructor currently appends an unscoped <style> with `:root { --vektra-primary: ... }` (see this._primaryColor, override.textContent, document.head.appendChild) which leaks and accumulates across instances; change it to reuse/replace a single style node (e.g., select or create a style with a stable id like "vektra-primary-override" instead of always appending) and scope the CSS to the widget roots rather than :root (e.g., target .vektra-chat-btn, .vektra-chat-panel or the widget root class) so the custom property only applies to the widget and multiple ChatUI instantiations update the same style node instead of adding new ones.
195-216: Misleading inline comment — please tidy.The comment on lines 198-201 walks itself back ("actually the default wins in that case too") and leaves the reader uncertain about the intended behavior. The actual semantics are straightforward: if
poweredByTextis set, the whole footer is a single link; otherwise the footer renders"Powered by <Vektra>". A blank/invalidpoweredByUrlalways falls back to the Vektra URL. Trim the comment down to match what the code does so future maintainers don't second-guess it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/widget/src/chat-ui.js` around lines 195 - 216, The inline comment around the footer link creation is misleading and should be replaced with a concise description of the actual behavior: if this._poweredByText is set the footer renders as a single link using that text; otherwise the footer renders "Powered by <Vektra>" with a linked "Vektra" label; a blank/invalid poweredByUrl falls back to the Vektra URL (url variable) — update the comment near the code that creates the link elements (references: this._poweredByText, url, footer) to state only this behavior and remove the self-contradictory sentences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 101-117: The _fetch_document_names() implementation creates keys
from DB rows using raw row[0] which may be UUID objects while inputs were
stringified, causing silent lookup failures; update _fetch_document_names() to
return dict[str, str] and store keys as strings by converting row[0] with
str(row[0]) when assigning to out, and update all lookup sites to use str(...)
when accessing the map (replace name_map.get(r.document_id) with
name_map.get(str(r.document_id)) at the three call sites referenced: pipeline.py
usages around the current lines ~581 and ~976 and advanced_pipeline.py around
~557), ensuring key normalization and type annotation consistency.
In `@vektra-learn/src/vektra_learn/api.py`:
- Around line 614-631: The audit task is currently only scheduled when
request_id (from request.state.request_id) exists, causing missed audit records;
remove the conditional guard and always call
background_tasks.add_task(_audit_log_event, ...) for the turns-read path
(keeping _LEARN_SENTINEL_KEY_ID,
endpoint="/api/v1/learn/conversations/{conversation_id}/turns", method="GET",
status_code=200, action="learn_conversation_turns_read" and the same
log_metadata), but ensure request_id in the payload is set to the actual
request_id if present or a synthesized value (e.g., None or a generated UUID) so
audits are always emitted even when middleware is not wired.
In `@vektra-learn/tests/test_api.py`:
- Around line 634-651: Rename the misleading test function
test_501ish_when_store_lacks_decryption to
test_500_err_learn_001_when_store_lacks_decryption and update its docstring to
reflect that get_conversation_turns should raise a 500 (ERR-LEARN-001) when the
provided conv_store lacks required decryption/metadata methods; ensure
references to ERR-LEARN-001 and get_conversation_turns in the test remain
correct so the assertion exc_info.value.status_code == 500 and error code check
still match.
In `@vektra-learn/widget/src/chat-ui.js`:
- Around line 208-216: The footer currently hardcodes "Powered by " instead of
using the widget i18n; add a poweredBy key to each I18N entry (e.g., poweredBy:
"Powered by" / "Offerto da") and replace the hardcoded string in the
default-footer block with this._lang.poweredBy so that the code that creates
footer, link, and link.textContent ("Vektra") uses localized text; update any
I18N imports/initializations where I18N is defined to include poweredBy so
this._lang.poweredBy is available at runtime.
In `@vektra-learn/widget/src/styles.js`:
- Around line 68-72: The hover rules for .vektra-chat-btn:hover and
.vektra-chat-send:hover override custom --vektra-primary by using background:
${t.primaryHover} unconditionally; change those hover backgrounds to use the
same CSS variable fallback pattern as the resting state (i.e., background:
var(--vektra-primary, <theme-hover-fallback>)) so custom data-primary-color
values remain darkened by filter: brightness(0.92) while theme-default still
falls back to t.primaryHover; update both occurrences referenced (hover blocks
for .vektra-chat-btn and .vektra-chat-send) accordingly.
In `@vektra-shared/src/vektra_shared/types.py`:
- Around line 286-289: The comment above the FEAT-012 field document_name is
inaccurate: soft-deleted (archived) source rows are represented by appending "
(archived)" to the filename rather than returning None. Update the docstring for
document_name to state that soft-deleted/archived documents will have "
(archived)" appended to the filename and only DB lookup failures produce None,
so the comment matches the actual archived-source behavior for the document_name
field.
---
Outside diff comments:
In `@docs/reference/api.md`:
- Around line 283-297: Update the /api/v1/query response example in
docs/reference/api.md to include the new per-source field document_name inside
each object in the "sources" array (alongside existing fields like doc_id,
chunk_id, score, snippet, citation_id, document_version) so the example matches
the current API contract; locate the JSON block under the "Response:" section
for the query endpoint and add "document_name": "..." to each source entry.
---
Nitpick comments:
In `@vektra-core/tests/test_pipeline.py`:
- Around line 423-497: Add a streaming assertion that mirrors
test_execute_populates_document_name: use the existing helper _collect_stream()
to consume pipeline.stream_execute (or pipeline.execute streaming variant used
in this test suite) and assert that each streamed item’s
sources.data[*]["document_name"] contains the expected names when monkeypatching
pipeline_mod._fetch_document_names to return the mapping (doc_a ->
"lecture-07.pdf", doc_b -> "slides.pptx") and that they are all None when the
helper returns an empty dict; place this check alongside the existing non-stream
assertions in test_execute_populates_document_name and reuse the same doc ids,
monkeypatch functions, and _make_pipeline/vector_store setup so the streaming
regression is covered.
In `@vektra-learn/src/vektra_learn/api.py`:
- Around line 600-609: The list comprehension that builds ConversationTurnItem
uses strict dict indexing (t["turn_number"], t["question"], t["created_at"])
which can KeyError on schema drift; change to defensive access by using
t.get("turn_number"), t.get("question"), t.get("created_at") and handle missing
values (e.g., provide sensible defaults, skip incomplete rows, or raise a clear
validation error) so the endpoint doesn't 500; update the comprehension that
iterates over turns returned by get_turns_detail and ensure ConversationTurnItem
construction tolerates missing keys (or validate/normalize each t before
creating ConversationTurnItem).
- Around line 580-587: Replace direct dict access meta["namespace_id"] with
meta.get("namespace_id") so a missing key doesn't raise KeyError and can't be
mistaken for a match; update the condition in the block that raises the
ErrorResponse (referencing meta, namespace, ErrorResponse, ERR_LEARN_006,
http_status_for, HTTPException, err.to_envelope()) to use
meta.get("namespace_id") and treat None as a mismatch (e.g., if
meta.get("namespace_id") is None or meta.get("namespace_id") != namespace)
before raising the existing error.
- Around line 498-512: Duplicate namespace-resolution logic exists:
_resolve_namespace_from_token re-implements the same fallback chain used inline
in course_query. Refactor by extending _resolve_namespace_from_token to return
both (course_id, namespace) or add a new helper (e.g.,
resolve_course_and_namespace_from_token) and have course_query call that helper
instead of duplicating logic; ensure the helper preserves the current behavior
(raise the same HTTPException when course_id missing and compute namespace =
token_payload.get("namespace") or course_id) so both endpoints remain consistent
if the JWT schema changes.
In `@vektra-learn/widget/src/chat-ui.js`:
- Around line 138-144: The ChatUI constructor currently appends an unscoped
<style> with `:root { --vektra-primary: ... }` (see this._primaryColor,
override.textContent, document.head.appendChild) which leaks and accumulates
across instances; change it to reuse/replace a single style node (e.g., select
or create a style with a stable id like "vektra-primary-override" instead of
always appending) and scope the CSS to the widget roots rather than :root (e.g.,
target .vektra-chat-btn, .vektra-chat-panel or the widget root class) so the
custom property only applies to the widget and multiple ChatUI instantiations
update the same style node instead of adding new ones.
- Around line 195-216: The inline comment around the footer link creation is
misleading and should be replaced with a concise description of the actual
behavior: if this._poweredByText is set the footer renders as a single link
using that text; otherwise the footer renders "Powered by <Vektra>" with a
linked "Vektra" label; a blank/invalid poweredByUrl falls back to the Vektra URL
(url variable) — update the comment near the code that creates the link elements
(references: this._poweredByText, url, footer) to state only this behavior and
remove the self-contradictory sentences.
In `@vektra-learn/widget/src/index.js`:
- Around line 122-197: The PR comment warns that calling init() without awaiting
can surface unhandled promise rejections; wrap the call-site where init() is
invoked (the init() function itself or the top-level invocation that triggers
it) with a rejection handler so any thrown async errors are logged/handled
(e.g., call init().catch(err => console.error("init failed", err))). Reference
symbols: init(), restoreConversation(), _readStored(), _writeStored(),
_clearStored(), ChatUI, ApiClient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84af0efb-f900-4fac-addd-a01b7729adf4
⛔ Files ignored due to path filters (2)
.s2s/BACKLOG.mdis excluded by!.s2s/**.s2s/plans/20260418-v050-widget-and-prof-config.mdis excluded by!.s2s/**
📒 Files selected for processing (18)
CHANGELOG.mddocs/reference/api.mddocs/reference/error-codes.mdvektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_integration.pyvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/api.pyvektra-core/src/vektra_core/pipeline.pyvektra-core/tests/test_pipeline.pyvektra-learn/src/vektra_learn/api.pyvektra-learn/src/vektra_learn/query.pyvektra-learn/tests/test_api.pyvektra-learn/widget/src/api-client.jsvektra-learn/widget/src/chat-ui.jsvektra-learn/widget/src/index.jsvektra-learn/widget/src/styles.jsvektra-shared/src/vektra_shared/errors.pyvektra-shared/src/vektra_shared/types.py
…3120130889) The existing `if request_id:` guard silently dropped the NFR-007 audit row whenever the RequestIdMiddleware wasn't wired (tests, early boot, misconfiguration). A successful 200 with no audit row is the worst possible failure mode for a compliance log. Synthesize a UUID fallback so every successful turns read leaves an audit trail unconditionally, and add a regression test covering the empty-state case. Renames the adjacent 501ish test to the accurate ERR-LEARN-001 name while we're in the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…i dup)
Both .vektra-chat-btn:hover and .vektra-chat-send:hover hardcoded
${t.primaryHover}, overriding the CSS variable so a prof's
data-primary-color snapped back to theme-default blue on hover.
Switch both hover backgrounds to var(--vektra-primary, ${t.primaryHover})
matching the resting-state pattern: custom-color deployments get their
brand color darkened by the existing filter: brightness(0.92), theme
default still gets the handcrafted primaryHover shade.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
asyncpg returns uuid.UUID objects for UUID columns and callers pass SearchResult.document_id (UUID) — both sides are UUID, so the lookup happened to work in practice. But the helper's signature left this coincidence implicit, and any path that passed a stringified id (or a future SearchResult field using str) would silently miss. Stringify on both ends: helper returns dict[str, str] with str(row[0]) keys, call sites do name_map.get(str(r.document_id)). Test fake updated to match the documented contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch of cosmetic / docs fixes from the PR #66 review. - widget: localize "Powered by" via new I18N.poweredBy key (en/it) (CR-3120130900) - widget: tidy the self-contradicting comment around the powered-by footer branch; behaviour unchanged (CR nitpick chat-ui.js:195-216) - widget: wrap init() with .catch(console.error) so any unexpected async rejection surfaces instead of becoming an unhandled promise rejection (CR nitpick index.js:122-197) - shared/types.py: correct SourceRef.document_name docstring — archived documents carry " (archived)" suffix; None is only for DB failures (CR-3120130909) - docs/reference/api.md: add document_name to the /api/v1/query response example so the contract matches the current payload (CR outside-diff api.md:283-297) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review follow-up — non-inline CodeRabbit nitsCommit summary for the comments posted in the review body (no inline thread). Fixed in this push
Acknowledged — deferred to follow-up work (not blocking v0.5.0)
By design — won't change
|
fvadicamo
left a comment
There was a problem hiding this comment.
All review feedback addressed. Summary:
4 functional fixes (separate commits)
dfe6be5— audit unconditional withuuid4()fallback (CR-3120130889, Major)c8e72f8— hover preserves--vektra-primary(CR-3120130903 + Gemini dup, Major)18af4f3—str()normalization in_fetch_document_names(CR-3120130886, Minor)86e5330(batched cosmetic) — "Powered by" i18n, docstring archived semantics, api.md response example,init().catch(), footer comment tidy
Deferred / acknowledged — rationale in the follow-up comment: namespace-resolution refactor, :root scoping of custom properties, extra streaming unit assertion.
By design — detailed in the same comment: strict dict access on NOT NULL contract fields, asyncio.sleep pattern consistent with existing test_audit_log_completeness.
make lint clean, 439 unit tests pass (+2 new: audit-without-request-id + rename), 9/9 integration WI-3, widget bundle rebuilt.
Three low-priority items deferred during the PR #66 self-review, now tracked in the backlog so they don't get lost once the PR merges. - DEBT-017: consolidate namespace-resolution logic between `_resolve_namespace_from_token` and `course_query` in vektra-learn - DEBT-018: scope the widget `--vektra-primary` override to widget roots and dedupe the style node across instantiations - DEBT-019: add a unit assertion for `document_name` on the streaming sources payload to complement `test_execute_populates_document_name` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Deferred items tracked in backlog (c57e5f5):
See |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vektra-learn/widget/src/index.js (1)
125-197:⚠️ Potential issue | 🟠 MajorGate user actions while restore is still in flight.
restoreConversation()is awaited, but only afterChatUIis constructed and itsonSend/onNewChatcallbacks are installed. A fast user can still send or reset during the 8s restore fetch; when restore resolves,client.setConversationId(...)andui.replayTurns(...)can overwrite that newer state.🐛 Proposed race guard
+ let restoreCancelled = false; + let restorePromise = Promise.resolve(); + const ui = new ChatUI({ theme, language, @@ - onSend(question) { + async onSend(question) { + await restorePromise; const stream = ui.createStreamMessage(); client.query(question, { @@ }, onNewChat() { + restoreCancelled = true; // Reset both storage and client-side state so the next query // creates a fresh conversation. _clearStored(courseId); @@ const payload = await client.getConversationTurns(stored.conversation_id); + if (restoreCancelled) return; if (payload && Array.isArray(payload.turns) && payload.turns.length > 0) { client.setConversationId(stored.conversation_id); ui.replayTurns(payload.turns); @@ - await restoreConversation(); + restorePromise = restoreConversation(); + await restorePromise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-learn/widget/src/index.js` around lines 125 - 197, The race happens because ChatUI's onSend/onNewChat handlers are active while restoreConversation() is pending; to fix, add a restore guard (e.g. a boolean restoreInProgress) and check it in the onSend and onNewChat callbacks to disable/ignore user actions (or show a temporary "restoring" state) until restore completes; set restoreInProgress = true before calling await restoreConversation(), call client.setConversationId(...) and ui.replayTurns(...) as you do, then clear restoreInProgress and re-enable the UI (e.g. via ui.setConnectionStatus or a new ui.enableInteractions/disableInteractions API) in both success and catch paths so client.setConversationId and ui.replayTurns cannot overwrite newer user actions. Ensure you reference the existing restoreConversation, ChatUI construction (onSend/onNewChat), client.setConversationId, and ui.replayTurns when making the changes.
🧹 Nitpick comments (1)
vektra-core/src/vektra_core/pipeline.py (1)
577-586: Trace the document-name lookup as a pipeline step.The new async DB lookup can add latency or silently degrade citations, but it is not reflected in
QueryTrace.steps. Add a smallStepTracearound both non-streaming and streaming lookup sites so traces explain where citation enrichment time went.♻️ Proposed trace addition
- name_map = await _fetch_document_names([r.document_id for r in selected_chunks]) + t0 = time.monotonic() + name_map = await _fetch_document_names([r.document_id for r in selected_chunks]) + steps.append( + StepTrace( + name="document_names", + duration_ms=_elapsed_ms(t0), + metadata={ + "requested": len(selected_chunks), + "resolved": len(name_map), + }, + ) + )Apply the same pattern before building
sources_datain_stream().As per coding guidelines,
vektra-core/**: RAG engine component. Check for: QueryTrace completeness.Also applies to: 972-981
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/src/vektra_core/pipeline.py` around lines 577 - 586, Wrap the async document-name lookup (_fetch_document_names([...]) call) with a StepTrace and append it to the current QueryTrace.steps so the lookup latency is recorded; do this in both the non-streaming path where sources_data is built (the location using name_map = await _fetch_document_names([r.document_id for r in selected_chunks]) and constructing SourceRef) and the streaming path inside _stream() before building sources_data, following the existing StepTrace pattern used in the file (create a StepTrace with a descriptive name like "document-name-lookup", start time, end time, status/details, then append to QueryTrace.steps) so citation enrichment time appears in QueryTrace for both flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@vektra-learn/widget/src/index.js`:
- Around line 125-197: The race happens because ChatUI's onSend/onNewChat
handlers are active while restoreConversation() is pending; to fix, add a
restore guard (e.g. a boolean restoreInProgress) and check it in the onSend and
onNewChat callbacks to disable/ignore user actions (or show a temporary
"restoring" state) until restore completes; set restoreInProgress = true before
calling await restoreConversation(), call client.setConversationId(...) and
ui.replayTurns(...) as you do, then clear restoreInProgress and re-enable the UI
(e.g. via ui.setConnectionStatus or a new
ui.enableInteractions/disableInteractions API) in both success and catch paths
so client.setConversationId and ui.replayTurns cannot overwrite newer user
actions. Ensure you reference the existing restoreConversation, ChatUI
construction (onSend/onNewChat), client.setConversationId, and ui.replayTurns
when making the changes.
---
Nitpick comments:
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 577-586: Wrap the async document-name lookup
(_fetch_document_names([...]) call) with a StepTrace and append it to the
current QueryTrace.steps so the lookup latency is recorded; do this in both the
non-streaming path where sources_data is built (the location using name_map =
await _fetch_document_names([r.document_id for r in selected_chunks]) and
constructing SourceRef) and the streaming path inside _stream() before building
sources_data, following the existing StepTrace pattern used in the file (create
a StepTrace with a descriptive name like "document-name-lookup", start time, end
time, status/details, then append to QueryTrace.steps) so citation enrichment
time appears in QueryTrace for both flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6606e619-ceea-4570-8c3d-c4999355919e
⛔ Files ignored due to path filters (1)
.s2s/BACKLOG.mdis excluded by!.s2s/**
📒 Files selected for processing (10)
docs/reference/api.mdvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/pipeline.pyvektra-core/tests/test_pipeline.pyvektra-learn/src/vektra_learn/api.pyvektra-learn/tests/test_api.pyvektra-learn/widget/src/chat-ui.jsvektra-learn/widget/src/index.jsvektra-learn/widget/src/styles.jsvektra-shared/src/vektra_shared/types.py
✅ Files skipped from review due to trivial changes (1)
- vektra-learn/widget/src/styles.js
🚧 Files skipped from review as they are similar to previous changes (6)
- vektra-shared/src/vektra_shared/types.py
- docs/reference/api.md
- vektra-core/tests/test_pipeline.py
- vektra-core/src/vektra_core/advanced_pipeline.py
- vektra-learn/tests/test_api.py
- vektra-learn/widget/src/chat-ui.js
…round) Previous fix awaited restoreConversation() in init() but left the input field live for the 8s-bounded fetch. A fast user could type+send during the await: _handleSend would add the user bubble to the DOM, then when restore resolved, replayTurns would wipe messagesEl and the bubble would disappear with the message half-sent. Lock input with a ChatUI._restoring flag mirroring the existing _sending flag: setRestoring(true) before the fetch, setRestoring(false) in the finally branch. Both _handleSend and _handleNewChat guard on the flag, so user actions can't preempt the replay. Lock is a no-op when there's nothing stored to restore (_readStored returns null). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The FEAT-012 helper _fetch_document_names is the only step between retrieval and LLM that touches Postgres, so it was the one place where an unexpected latency spike (slow replica, cold cache) could blame the LLM call instead. Emit a StepTrace with requested/resolved counts around all four call sites — SimpleQueryPipeline.execute and execute_stream, AdvancedQueryPipeline.execute and execute_stream — so citation enrichment time is explicit in QueryTrace. step_names expectation in test_execute_returns_response_and_trace updated to include "document_names" (now 9 steps). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review round 2 — CodeRabbitFixed
|
Follows the project convention where the per-submodule pyproject.toml carries `X.Y.Z-dev` throughout the cycle; even the `v0.4.0` release tag pins `0.4.0-dev`. uv.lock regenerated to pick up the new workspace versions.
API reference (docs/reference/api.md):
- New "Learn" section documenting POST /api/v1/learn/query (JSON + SSE,
show_sources flag, document_name) and GET /api/v1/learn/conversations/{id}/turns
(JWT scope, ERR-LEARN-005/006, learn_conversation_turns_read audit row)
- Spell out the show_sources resolution chain in the namespaces PATCH section
- Document the (archived) suffix on document_name for soft-deleted documents
Configuration (docs/reference/configuration.md):
- Add VEKTRA_PROMPT_GROUNDING_MODE (Query pipeline)
- Add VEKTRA_LEARN_SHOW_SOURCES (E-learning)
Error codes (docs/reference/error-codes.md):
- Fix ERR-LEARN-004: 422 -> 409, message corrected to "Duplicate enrollment"
(the documented text never matched the actual handler in vektra-learn/api.py)
Getting started (docs/getting-started/first-query.md):
- Sample response includes document_name; field added to the description table
Component READMEs:
- vektra-learn: full widget integration guide (data-* attrs, sessionStorage
persistence, endpoint table)
- vektra-admin: list namespace config endpoints
- vektra-core: document_name join logic and (archived) fallback
CHANGELOG.md:
- Widget data-* attribute list in v0.5.0 entry was missing
data-powered-by-text and data-powered-by-url (both shipped in PR #66)
Scope: HIGH + MEDIUM drift from the v0.5.0 doc audit. LOW items
(component README polish, dedicated widget guide, helper docs in
vektra-shared README, configuration variable count reconciliation)
deferred to a follow-up.
- v0.5.0: widget production-ready (FEAT-016/012/004 + WI-1/2/3) + instructor namespace config - Branding: rebrand to "Vektra RAG" + widget footer "VektraLabs" + vektralabs.github.io org landing - Security: 2 critical CVEs litellm closed (auth bypass), pytest 9, esbuild 0.25 - DEBT-018 widget primary-color scope, DEBT-020 audit log fallback (admin/learn/ingest) - Docs: consolidated widget integration guide, CHANGELOG/BACKLOG aligned Reviews: 14/14 addressed across PR #66/69/70/71/72/73 (Gemini + CodeRabbit + code-quality bot) Tests: 632 passed; CI all green; integration + NFR gates pass Refs: closes DEBT-018, DEBT-020; spawns DEBT-022, DEBT-023; coordinated with vektra-moodle#14
What
Closes the gap between "widget works" (v0.4.0) and "widget is production-ready
for real courses". Five work items plus two rounds of self-review fixes.
GET /api/v1/learn/conversations/{id}/turns(JWT-scoped, audited)document_nameon every source citation, with(archived)markerfor soft-deleted documents
PATCH /api/v1/admin/namespaces/{id}/config(whitelisted,partial merge, audited)
welcome message, configurable powered-by)
Why
Universities deploying Vektra need per-course customization without forking,
professors need to toggle strict/hybrid grounding without admin access, and
students lose their conversation on every page refresh. Three pain points,
one milestone.
Details
WI-3 —
PATCH /api/v1/admin/namespaces/{id}/configALLOWED_CONFIG_KEYS = {"grounding_mode"},values
{"strict", "hybrid"}. Unknown keys → 400 to surface Moodleplugin typos early.
nullremoves a key (falls back to env default).namespace_config_updatedviavektra_shared.audit.ERR-ADMIN-005/006/007.WI-2 — document_name in source citations
SourceRef.document_namejoined fromsource_documents.filename.(archived)suffixso answers stay traceable (REQ-057).
SimpleQueryPipeline.execute/execute_streamandAdvancedQueryPipeline.execute/execute_stream(the default pipelinein production — a miss here would have meant no filename in the widget).
WI-1 —
GET /api/v1/learn/conversations/{id}/turns403 on mismatch (
ERR-LEARN-006), 404 on missing (ERR-LEARN-005).(model, prompt_tokens, response_id) intentionally excluded.
learn_conversation_turns_read) per NFR-007.[]for v0.5.0; enrichment fromquery_traces.chunks_retrievedis deferred.WI-4 — widget white-label
data-title,data-primary-color,data-icon(emoji or URL),data-welcome-message,data-powered-by,data-powered-by-text,data-powered-by-url.textContent, color through aconservative regex whitelist before injection as CSS custom property,
icon URL via
<img src>(browser-sanitized), footer URL restrictedto http(s) and same-origin paths.
var(--vektra-primary, ...)so changes apply withoutrebuilding the bundle.
WI-5 — conversation persistence
sessionStorage["vektra-conv:" + course_id]keyed per course,tab-scoped (avoids leaking across students on shared computers),
24h stale cutoff.
restoreConversation()awaited at init with 8sAbortSignal.timeouton the WI-1 fetch: a stalled backend cannot block widget startup and
a fast-typing user cannot race
replayTurns._conversationId.Self-review fixes (two rounds)
AdvancedQueryPipeline.execute_stream()misseddocument_name—critical since advanced is the default pipeline (Combo D) and the
widget streams by default (
45437c8)._fetch_document_namesdid not honour soft-delete — plan requiredthe
(archived)marker (e787cc4).content access (
e787cc4).PATCH /namespaces/{id}/configroute moved to/api/v1/admin/...to match DEBT-011's convention (
e787cc4).data-powered-by-text+data-powered-by-urlso operators cancustomise the footer without rebuild (
bda675b).bda675b).Testing
make lintclean (ruff + mypy + import-linter).vektra-shared/tests/test_config.py::TestLLMConfigare pre-existingenv-pollution and were verified against the develop baseline; this PR
does not touch that area.
vektra-admin -k namespace_config9/9 (testcontainers,real Postgres per
feedback_no_mock_database).Manual smoke test on Kalypso (image rebuilt from this branch):
PATCH .../confighybrid{"config":{"grounding_mode":"hybrid"}}200ERR-ADMIN-006400ERR-ADMIN-007400{"config":{}}200ERR-ADMIN-005404esc-100document_name="Escapologia Fiscale - Introduzione.pdf"conv_id=eda7…200GET .../turnsmatching namespacesources=[]200GET .../turnsnon-existentERR-LEARN-005404GET .../turnswrong courseERR-LEARN-006403namespace_config_updated+learn_conversation_turns_readinaudit_logwith full metadataBrowser verification (not automated, before merge):
Deliberate tradeoffs
courses; the plan asked for it so the widget can distinguish "wrong
course" from "deleted conversation". Conversation IDs are UUIDs.
data-powered-bydefaulttrueadds "Powered by Vektra" to upgradeddeploys; operators can hide with
falseor replace with-text/-url.[]; new turns are unaffected.(archived)marker is English only for v0.5.0.Deferred follow-ups
pyproject.toml— done at merge timevektra-moodlerepo — separate PR cycleChecklist
make lintand unit tests passdocs/reference/api.mdanddocs/reference/error-codes.mdupdatedTraceability
.s2s/plans/20260418-v050-widget-and-prof-config.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Documentation