Conversation
Closes the variant-scoping incoherence flagged by PR #215's coverage audit and pinned by tests/playwright/rendering-invariants.spec.ts test #8: 8 dashboard handlers silently dropped `?variant=` while the layout banner showed scoped, leaving an asymmetry between what the URL claimed and what the handler actually rendered. Per-handler design choices: - graph_view (A: accept-and-scope) — adds `variant` to GraphParams, builds a scoped store via try_build_scope, renders against the filtered graph. Variant scoping reduces node count, which materially helps the O(n^2)-ish layout pass on large dogfood projects, so this is the path that actually improves performance rather than regressing it. - verification_view (A) — switches to ViewParams + try_build_scope so verification status reflects the active variant's artifact slice. - eu_ai_act_view (A) — Annex IV compliance is naturally per-variant (each variant ships a different artifact mix); now scoped. - traceability_view (A) — adds `variant` to TraceParams; the traceability tree was the canonical scope-sensitive view that should never have been silent-dropping. - doc_linkage_view (A) — link counts shown for each doc derive from the scoped store now. - documents_list (A) — list itself is doc-driven, but the per-doc link counters come from the scoped store; banner now consistent. - results_view (A) — verification results are tied to artifacts; scoping to the variant's bound IDs is meaningful. - search_view (A) — adds `variant` to SearchParams; search now ranks only against in-scope artifacts. - externals_list (C: explicit ignore + comment) — externals are loaded from sibling repos and don't participate in this project's feature-model bindings, so variant filtering has no semantic meaning there. Param is accepted gracefully (so bookmarked URLs degrade) and the rationale is documented inline. Updates rendering-invariants.spec.ts test #8 to assert the new scoped behaviour (was pinning the silent-drop bug); adds 2 new Playwright tests verifying /traceability and /documents end-to-end. Adds 3 new Rust integration tests (`newly_scoped_handlers_render_variant_banner`, `search_handler_honors_variant_scope`, `externals_view_accepts_variant_param_without_400`) covering the 200 + banner contract for the seven banner-routes, the fragment-only contract for /search, and the gracefully-ignored contract for /externals. Refs: FEAT-001
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: af1c7a7 | Previous: 3cdb942 | Ratio |
|---|---|---|---|
query/10000 |
154546 ns/iter (± 2066) |
110920 ns/iter (± 985) |
1.39 |
This comment was automatically generated by workflow using github-action-benchmark.
📐 Rivet artifact deltaNo artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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 the variant-scoping incoherence flagged by PR #215's coverage audit and pinned by
tests/playwright/rendering-invariants.spec.tstest #8: 8 dashboard handlers silently dropped?variant=while the layout banner showed scoped, leaving a surprising asymmetry between what the URL claimed and what the handler actually rendered.This is exactly the kind of "user signal is consistency" fix the audit flagged: every other ViewParams-bearing route (
/artifacts,/coverage,/matrix,/stats,/stpa,/validate) was already scoped — these eight just lagged behind.Per-handler design choices
Bias toward A: accept-and-scope since variant scoping is semantically meaningful nearly everywhere, and the renderer-side machinery (
VariantScope::render_context) already exists and is identical to what the seven scoped routes use.graph_viewverification_viewminimal-cishould not show verification rows for artifacts not in scope.eu_ai_act_viewtraceability_viewdoc_linkage_viewdocuments_listresults_viewsearch_view/searchis fragment-only so no banner; the param is honored upstream of layout.)externals_listWhat changed
rivet-cli/src/serve/views.rs— the eight handlers above. Each one now:ViewParams(or extends its existing param struct withvariant: Option<String>).try_build_scope(&state, ¶ms.variant)— the same helper used byartifacts_list,coverage_view,matrix_view, etc.Response(notHtml<String>) so the 400-on-unknown-variant error path matches the rest of the dashboard.externals_list, acceptsViewParamsbut ignores the variant field — documented in the doc-comment.tests/playwright/rendering-invariants.spec.ts— flips test #8 from pinning the silent-drop bug to asserting the new scoped behaviour, then adds two more end-to-end tests for/traceabilityand/documents.rivet-cli/tests/serve_integration.rs— three new Rust integration tests:newly_scoped_handlers_render_variant_banner— sweeps the seven banner-routes, asserts 200+banner on valid variant and 400 on unknown variant.search_handler_honors_variant_scope— same contract for/searchminus the banner (fragment-only endpoint).externals_view_accepts_variant_param_without_400— pins the Choice C contract for/externals.Test plan
cargo build -p rivet-cli— cleancargo test -p rivet-cli— 99 lib + 42 serve_integration + the rest passcargo clippy -p rivet-cli --all-targets -- -D warnings— clean (only pre-existing MSRV note)cargo fmt -p rivet-cli -- --check— cleantests/playwright/rendering-invariants.spec.tstest feat: cross-repository artifact linking #8 plus two new testsTrailers
Refs: FEAT-001(dashboard/serve) per CLAUDE.md quick reference.