feat(learn): configurable source citation visibility (FEAT-014)#69
Merged
Conversation
Introduces the show_sources flag end-to-end in the backend so instructors
can hide the citations block per course without touching the widget.
- vektra-shared: VEKTRA_LEARN_SHOW_SOURCES env (default true) + resolve_show_sources() matching the grounding_mode pattern (namespace JSONB config overrides env).
- vektra-admin: PATCH /admin/namespaces/{id}/config now accepts show_sources alongside grounding_mode. New ALLOWED_CONFIG_TYPES map supports bool validation (ERR-ADMIN-007 rejects non-bool, including int/string coercion attempts). Partial-merge semantics unchanged.
- vektra-app: expose learn_show_sources_default on app.state for the API layer to consume.
- vektra-learn: resolve show_sources per request (namespace config > env > default), return the value in CourseQueryResponse, and tag the SSE "sources" event so the streaming widget sees the same flag as the JSON path.
The API always returns the full sources list regardless of the flag;
visibility is a presentation concern for the widget.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (FEAT-014) Resolution chain inside the widget: 1. data-show-sources attribute on the script tag (client-side force) 2. show_sources flag from the learn query response (server-resolved) 3. default true (legacy behaviour when both are absent) api-client.js forwards the server value as a second argument to the onSources callback for both the SSE and the JSON-fallback paths. index.js applies the resolution and only calls ui.addSources() when the effective value is true — keeping ChatUI agnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- api.md: document show_sources on PATCH /admin/namespaces/{id}/config, covering the new bool type and the fallback to VEKTRA_LEARN_SHOW_SOURCES on null.
- CHANGELOG: extend the v0.5.0 entry with the resolution chain and the rationale for keeping sources in the API payload.
- BACKLOG: mark FEAT-014 completed (Moodle-side integration deferred to the sibling vektra-moodle plan).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fvadicamo
added
documentation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a namespace-level Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant AdminAPI as Admin API
participant DB as Database
participant App as App
participant LearnAPI as Learn API
participant Widget as Widget
Admin->>AdminAPI: PATCH /api/v1/admin/namespaces/{id}/config\n{"show_sources": false}
AdminAPI->>AdminAPI: validate key, enum, and runtime type
AdminAPI->>DB: persist namespace.config
DB-->>AdminAPI: OK
AdminAPI-->>Admin: 200 {"config": {"show_sources": false}}
App->>App: startup reads VEKTRA_LEARN_SHOW_SOURCES\nset app.state.learn_show_sources_default
Widget->>LearnAPI: POST /api/v1/query (with namespace)
LearnAPI->>DB: resolve_show_sources(namespace)
DB-->>LearnAPI: namespace value or null
LearnAPI->>LearnAPI: effective = namespace ?? app.state.learn_show_sources_default
alt streaming (SSE)
LearnAPI->>Widget: emit "sources" event with {sources: [...], show_sources: effective}
else non-stream
LearnAPI-->>Widget: JSON response includes "show_sources": effective
end
Widget->>Widget: compute effective using data-show-sources > server hint > default(true)
Widget->>Widget: render or skip citations based on resolved flag
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements configurable source citation visibility (FEAT-014) across the Vektra platform. It introduces a show_sources configuration option for namespaces, a corresponding global environment variable, and support for a client-side override in the chat widget. The resolution logic ensures that the widget respects the most specific setting available while the API continues to provide full source data for analytics. A review comment suggests programmatically deriving the allowed configuration keys whitelist in the admin API to prevent validation bypasses during future updates.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vektra-admin/src/vektra_admin/api.py (1)
53-64: Consider asserting the "exactly one map" invariant.The docstring at lines 54-57 states that a key must appear in exactly one of
ALLOWED_CONFIG_VALUES/ALLOWED_CONFIG_TYPES, but nothing enforces it. If a future key is accidentally added to both maps, both checks would run silently; if added to neither (but present inALLOWED_CONFIG_KEYS), values bypass validation entirely. A module-level assertion would surface the mistake at import time.💡 Optional guard
ALLOWED_CONFIG_TYPES: dict[str, type] = { "show_sources": bool, } +assert ALLOWED_CONFIG_KEYS == ( + set(ALLOWED_CONFIG_VALUES) ^ set(ALLOWED_CONFIG_TYPES) +), "Each config key must be validated by exactly one of VALUES/TYPES maps"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-admin/src/vektra_admin/api.py` around lines 53 - 64, Add a module-level assertion that enforces the invariant described: for every key in ALLOWED_CONFIG_KEYS the key must appear in exactly one of ALLOWED_CONFIG_VALUES or ALLOWED_CONFIG_TYPES, and additionally assert that ALLOWED_CONFIG_VALUES.keys() and ALLOWED_CONFIG_TYPES.keys() are subsets of ALLOWED_CONFIG_KEYS; implement this check using ALLOWED_CONFIG_KEYS, ALLOWED_CONFIG_VALUES, and ALLOWED_CONFIG_TYPES so import-time failures surface if a key is missing from both maps or accidentally present in both.vektra-admin/tests/test_integration.py (1)
778-859: LGTM — solid end-to-end coverage.The four new tests mirror the existing WI-3 suite and correctly exercise:
- boolean persistence (both
trueandfalse),- strict type rejection across representative non-bool JSON types (
"true",1,"yes") — importantly,1is rejected thanks toisinstance(1, bool) is False,- partial-merge semantics across
show_sourcesandgrounding_mode.One minor follow-up worth considering: a test that seeds a namespace with
show_sources: Falseand verifiesresolve_show_sources()reads it back (analogous totest_namespace_config_patch_resolves_via_shared_helperfor grounding mode) would close the loop end-to-end for the resolver path too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-admin/tests/test_integration.py` around lines 778 - 859, Add a new async test (e.g., test_namespace_config_resolves_show_sources_from_storage) that seeds a namespace with an explicit stored config containing {"show_sources": False} (use the existing helpers like _seed_namespace and _write_namespace_config/_read_namespace_config if available), then call the same resolver used elsewhere (resolve_show_sources or the admin GET that invokes it) and assert it returns False; mirror the pattern from test_namespace_config_patch_resolves_via_shared_helper for grounding_mode so you verify the resolver path reads persisted boolean overrides end-to-end.
🤖 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-learn/widget/src/index.js`:
- Around line 125-127: The code reading data-show-sources doesn't trim
whitespace so values like " false " are misinterpreted; update the logic where
scriptTag.getAttribute("data-show-sources") is processed (the showSourcesAttr
variable) and construct clientShowSourcesOverride by trimming before lowercasing
(i.e., use showSourcesAttr.trim().toLowerCase()) and preserve the null check so
that null stays null, ensuring correct boolean evaluation in
clientShowSourcesOverride.
---
Nitpick comments:
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 53-64: Add a module-level assertion that enforces the invariant
described: for every key in ALLOWED_CONFIG_KEYS the key must appear in exactly
one of ALLOWED_CONFIG_VALUES or ALLOWED_CONFIG_TYPES, and additionally assert
that ALLOWED_CONFIG_VALUES.keys() and ALLOWED_CONFIG_TYPES.keys() are subsets of
ALLOWED_CONFIG_KEYS; implement this check using ALLOWED_CONFIG_KEYS,
ALLOWED_CONFIG_VALUES, and ALLOWED_CONFIG_TYPES so import-time failures surface
if a key is missing from both maps or accidentally present in both.
In `@vektra-admin/tests/test_integration.py`:
- Around line 778-859: Add a new async test (e.g.,
test_namespace_config_resolves_show_sources_from_storage) that seeds a namespace
with an explicit stored config containing {"show_sources": False} (use the
existing helpers like _seed_namespace and
_write_namespace_config/_read_namespace_config if available), then call the same
resolver used elsewhere (resolve_show_sources or the admin GET that invokes it)
and assert it returns False; mirror the pattern from
test_namespace_config_patch_resolves_via_shared_helper for grounding_mode so you
verify the resolver path reads persisted boolean overrides end-to-end.
🪄 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: 9802e6dd-9e55-4fd8-be3f-396b56539920
⛔ Files ignored due to path filters (1)
.s2s/BACKLOG.mdis excluded by!.s2s/**
📒 Files selected for processing (13)
CHANGELOG.mddocs/reference/api.mdvektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_integration.pyvektra-app/src/vektra_app/main.pyvektra-learn/src/vektra_learn/api.pyvektra-learn/src/vektra_learn/query.pyvektra-learn/tests/test_api.pyvektra-learn/tests/test_query.pyvektra-learn/widget/src/api-client.jsvektra-learn/widget/src/index.jsvektra-shared/src/vektra_shared/config.pyvektra-shared/src/vektra_shared/namespace.py
A hand-maintained whitelist alongside the validation maps was a single forgotten edit away from letting a new key bypass validation entirely. Derive it from the union of ALLOWED_CONFIG_VALUES and ALLOWED_CONFIG_TYPES so the relationship cannot drift, and add a module-level disjoint check that surfaces a duplicated registration at import time. Addresses Gemini inline 3127135275 and CodeRabbit nitpick on api.py:53-64. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Whitespace from server-side templating (e.g. " false ") was silently treated as truthy because the comparison ran on the unmodified attribute value. Trim before lowercasing so the explicit override behaves as documented. Addresses CodeRabbit inline 3127144765. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror test_namespace_config_patch_resolves_via_shared_helper for the FEAT-014 resolver: PATCH show_sources=False, then call the shared helper with default_value=True and assert the namespace override wins. Closes the loop between the write path (admin PATCH) and the read path (vektra_shared.namespace.resolve_show_sources) with no cache between. Addresses CodeRabbit nitpick on test_integration.py:778-859. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressing CodeRabbit nitpicks from the review body:
|
fvadicamo
commented
Apr 24, 2026
fvadicamo
left a comment
fvadicamo
left a comment
Contributor
Author
There was a problem hiding this comment.
All review comments addressed in 3 commits (f9b4ccb, 7951412, c806afc):
- Gemini medium (api.py:64): derive ALLOWED_CONFIG_KEYS from validation maps + import-time disjoint check
- CodeRabbit potential issue (index.js:127): trim before lowercase on data-show-sources
- CodeRabbit nitpick (api.py:53-64): same root cause as above, addressed by the derivation approach
- CodeRabbit nitpick (test_integration.py): added end-to-end resolver test for show_sources
Lint clean, all 14 admin PATCH tests pass.
Adds the read side of the namespace-config contract so upstream plugins (starting with the Moodle block edit form) can render "Use default" vs "Override" without re-implementing the resolver chain client-side. The response separates two views: - config: the raw JSONB stored in namespaces.config (empty when nothing has been PATCHed) - resolved: the value queries actually observe, computed via the same fallback chain (namespace > env > hardcoded default) used at runtime The resolved values are computed inline from the loaded ORM row to keep the endpoint to a single DB session — the standalone resolvers in vektra_shared.namespace remain for callers that hold only a namespace id and need to open their own session. The validation logic (enum membership for grounding_mode, isinstance for show_sources) is the same in both paths so behaviour cannot drift. Auth: admin scope. Errors mirror PATCH (404 ERR-ADMIN-005 on missing). 4 integration tests cover empty/stored/missing/scope. Documented in docs/reference/api.md and CHANGELOG. Motivated by the contract checklist in vektra-internal/moodle/feat14-moodle-contract-checklist.md (G1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Scope expansion: added the symmetric GET endpoint (843eaaa). The Moodle block edit form needs to render the current The new endpoint returns:
The Implementation notes:
Documented in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vektra-admin/src/vektra_admin/api.py (1)
570-595: Future-proof_resolved_from_storedagainst whitelist drift.Two small coupling issues in this helper that can silently bite when
ALLOWED_CONFIG_KEYSis extended:
- Line 586 hardcodes
{"strict", "hybrid"}instead of reusingALLOWED_CONFIG_VALUES["grounding_mode"]. Add a new allowed mode to the whitelist and this validator will still accept only the old two.- The function shape (explicit
grounding_default/show_sources_defaultkwargs, hardcoded return dict) means adding a third key toALLOWED_CONFIG_KEYSrequires touching both this helper and its only caller. Meanwhile the GET handler's docstring promises "Always includes every key inALLOWED_CONFIG_KEYS" — that invariant is only maintained by hand.A mechanical refactor to key the logic off
ALLOWED_CONFIG_VALUES/ALLOWED_CONFIG_TYPESplus a defaults map keeps the invariant by construction.♻️ Proposed refactor sketch
-def _resolved_from_stored( - stored: dict[str, Any], - *, - grounding_default: str, - show_sources_default: bool, -) -> dict[str, Any]: - """...""" - raw_grounding = stored.get("grounding_mode") - grounding = ( - raw_grounding if raw_grounding in {"strict", "hybrid"} else grounding_default - ) - raw_show_sources = stored.get("show_sources") - show_sources = ( - raw_show_sources if isinstance(raw_show_sources, bool) else show_sources_default - ) - return { - "grounding_mode": grounding, - "show_sources": show_sources, - } +def _resolved_from_stored( + stored: dict[str, Any], + defaults: dict[str, Any], +) -> dict[str, Any]: + """Apply the namespace > env > hardcoded-default chain to a loaded JSONB. + + *defaults* must provide an entry for every key in ALLOWED_CONFIG_KEYS. + """ + resolved: dict[str, Any] = {} + for key in ALLOWED_CONFIG_KEYS: + raw = stored.get(key) + allowed_values = ALLOWED_CONFIG_VALUES.get(key) + allowed_type = ALLOWED_CONFIG_TYPES.get(key) + if allowed_values is not None and raw in allowed_values: + resolved[key] = raw + elif allowed_type is not None and isinstance(raw, allowed_type): + resolved[key] = raw + else: + resolved[key] = defaults[key] + return resolvedAnd at the call site:
- resolved = _resolved_from_stored( - stored, - grounding_default=getattr( - request.app.state, "grounding_mode_default", "strict" - ), - show_sources_default=getattr( - request.app.state, "learn_show_sources_default", True - ), - ) + resolved = _resolved_from_stored( + stored, + defaults={ + "grounding_mode": getattr( + request.app.state, "grounding_mode_default", "strict" + ), + "show_sources": getattr( + request.app.state, "learn_show_sources_default", True + ), + }, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-admin/src/vektra_admin/api.py` around lines 570 - 595, The helper _resolved_from_stored currently hardcodes the grounding-mode whitelist and the return shape; change it to derive validation and keys from the module-level ALLOWED_CONFIG_VALUES / ALLOWED_CONFIG_TYPES / ALLOWED_CONFIG_KEYS and a small defaults map so it automatically covers any future keys. Specifically, replace the literal {"strict","hybrid"} with ALLOWED_CONFIG_VALUES["grounding_mode"], consult ALLOWED_CONFIG_TYPES for expected types (e.g. bool) when deciding whether to accept stored values, and build the returned dict by iterating ALLOWED_CONFIG_KEYS and using either the validated stored value or the provided defaults map (instead of explicit grounding_default/show_sources_default args and a hardcoded return dict) so the GET handler will always include every key in ALLOWED_CONFIG_KEYS without further edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 570-595: The helper _resolved_from_stored currently hardcodes the
grounding-mode whitelist and the return shape; change it to derive validation
and keys from the module-level ALLOWED_CONFIG_VALUES / ALLOWED_CONFIG_TYPES /
ALLOWED_CONFIG_KEYS and a small defaults map so it automatically covers any
future keys. Specifically, replace the literal {"strict","hybrid"} with
ALLOWED_CONFIG_VALUES["grounding_mode"], consult ALLOWED_CONFIG_TYPES for
expected types (e.g. bool) when deciding whether to accept stored values, and
build the returned dict by iterating ALLOWED_CONFIG_KEYS and using either the
validated stored value or the provided defaults map (instead of explicit
grounding_default/show_sources_default args and a hardcoded return dict) so the
GET handler will always include every key in ALLOWED_CONFIG_KEYS without further
edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49ad5597-7b41-4941-a547-50679efb39cf
📒 Files selected for processing (4)
CHANGELOG.mddocs/reference/api.mdvektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- docs/reference/api.md
The previous helper hardcoded the grounding_mode whitelist and built the return dict by hand, so adding a new key to ALLOWED_CONFIG_KEYS would have silently bypassed the GET endpoint's "Always includes every key" invariant — the same drift risk the round 1 review flagged for the whitelist itself. Iterate ALLOWED_CONFIG_KEYS, look up each key's validator in ALLOWED_CONFIG_VALUES / ALLOWED_CONFIG_TYPES, and accept a single defaults dict (asserted to cover every key). Adding a new whitelist key now flows through automatically; the assertion fails fast at runtime if the call site forgot to provide its default. Existing GET tests still pass — response shape is unchanged. Addresses CodeRabbit nitpick on api.py:570-595. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Fixed in 4377360. Refactored |
fvadicamo
commented
Apr 24, 2026
fvadicamo
left a comment
fvadicamo
left a comment
Contributor
Author
There was a problem hiding this comment.
Round 2 addressed in 4377360. Single CR nitpick on _resolved_from_stored applied: helper now derives validation+keys from ALLOWED_CONFIG_VALUES/TYPES with assertion-guarded defaults dict. 4/4 GET tests still pass, lint clean.
This was referenced Apr 25, 2026
fvadicamo
added this to the
v0.5.0 - Widget production-ready & instructor configuration milestone
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes FEAT-014: per-course toggle for the source citations section in the widget, with a three-level resolution chain (client override > namespace config > env default) so instructors can turn it off without touching the admin UI.
Why
Raised during Moodle integration testing (BACKLOG FEAT-014): for pedagogical or compliance reasons, some courses should not expose chunk IDs, scores, and snippets to students. Before this change the widget always rendered the citations block, with no way to opt out short of a widget rebuild.
How
Resolution chain (first match wins):
data-show-sourcesattribute on the widget<script>tag — client-side forcenamespaces.config.show_sources— per-course override writable viaPATCH /api/v1/admin/namespaces/{id}/configVEKTRA_LEARN_SHOW_SOURCESenv var — platform defaulttrue— legacy fallback if the server omits the flagServer side (
vektra-learnquery endpoint):show_sourcestop-level fieldshow_sourceson thesourcesevent payloadsourceslist is always returned regardless — visibility is a widget concern, analytics/QueryTrace keep complete dataPATCH whitelist now accepts
show_sourcesalongsidegrounding_mode. A newALLOWED_CONFIG_TYPESmap handles bool validation so integer or string values are rejected withERR-ADMIN-007.Files touched
vektra-sharedVEKTRA_LEARN_SHOW_SOURCESenv,resolve_show_sources()helpervektra-adminvektra-applearn_show_sources_defaultonapp.statevektra-learnvektra-learn/widgetdata-show-sourcesparsing,onSources(sources, show_sources)callback wiringdocs/reference/api.mdCHANGELOG.md.s2s/BACKLOG.mdTests
All 200 admin + learn + shared namespace tests pass. 14 new tests cover the whitelist, response shape, SSE payload, and resolution defaults.
make lintclean.Test plan
{"show_sources": false}then query → widget does not render citationsdata-show-sources="true"overrides a namespace-disabled settingdata-show-sources="false"overrides a namespace-enabled settingtrue(current behaviour)Refs
.s2s/BACKLOG.mdSummary by CodeRabbit
New Features
Documentation
Tests