test(viewer): tokens SSOT proptest surface (WBS-6.2 #450) - #450
test(viewer): tokens SSOT proptest surface (WBS-6.2 #450)#450KooshaPari wants to merge 1 commit into
Conversation
Adds crates/sl-viewer/tests/properties_viewer_tokens.rs with 10
proptest properties pinning the design-token SSOT invariants:
* lab_coat::*:
* Every hex is a well-formed #RRGGBB (7-char lowercase ASCII hex).
* Every hex is non-empty.
* All 16 documented hex constants are pairwise distinct.
* Every hex appears in TOKENS_CSS so the Rust mirror and the
CSS SSOT stay in sync.
* REQUIRED_CSS_VARS:
* Every entry starts with --.
* Every entry is non-empty.
* The set is duplicate-free.
* Every entry appears in TOKENS_CSS.
* VIEWER_COLOR_SCHEME:
* Declares both :root and :root[data-theme dark] selectors.
* Uses color-scheme exactly twice.
Updates WBS-6.2 evidence list, TRACEABILITY.json, and CHANGELOG.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
📝 WalkthroughSummaryThe PR adds property-based tests for The change is small and does not add public API changes. Must FixNone identified. Should Fix
ConsiderThe invariant-only properties use fixed ranges such as Run the repository checks before merge:
Approve / Request ChangesRequest changes for the two test-quality issues above, unless the hard-coded token list and selector checks are intentionally accepted. WalkthroughThe PR adds property-based tests for sl-viewer token invariants. It validates color constants, required CSS variables, CSS token presence, and theme declarations. The PR also records the tests in the changelog and WBS-6.2 traceability evidence. ChangesViewer token SSOT
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| /// The full list of `lab_coat::*` hex constants in stable declaration | ||
| /// order. We compute this once via a small reflection-on-source approach: | ||
| /// every `pub const` in `lab_coat::*` whose value is a `&'static str` | ||
| /// starting with `#`. Since we can't introspect Rust modules at runtime, | ||
| /// we hard-code the list (mirroring `tokens.rs`). The constants are | ||
| /// public — any new addition requires also extending this list, which |
There was a problem hiding this comment.
Suggestion: The list is manually maintained, and none of the properties compares it with the actual declarations in lab_coat. Adding a new public hex constant without adding it here will leave every test passing, so the claimed exhaustiveness check cannot detect Rust/CSS drift. Tie the test list to a compile-time declaration manifest or add an independently checked expected declaration count. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ New Lab-Coat constants can bypass SSOT checks.
- ⚠️ Rust/CSS token drift may reach viewer styling.
- ⚠️ Claimed exhaustiveness depends on manual maintenance.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_tokens.rs
**Line:** 44:49
**Comment:**
*Incomplete Implementation: The list is manually maintained, and none of the properties compares it with the actual declarations in `lab_coat`. Adding a new public hex constant without adding it here will leave every test passing, so the claimed exhaustiveness check cannot detect Rust/CSS drift. Tie the test list to a compile-time declaration manifest or add an independently checked expected declaration count.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Property: every `lab_coat::*` hex appears as a substring of | ||
| /// `TOKENS_CSS` so the Rust mirror and the CSS SSOT stay in sync. | ||
| /// If a constant is added without updating the CSS, this fails. | ||
| #[test] | ||
| fn lab_coat_hex_in_tokens_css(i in lab_coat_hex_indices_strategy()) { | ||
| let hex = lab_coat_hex_list()[i]; | ||
| prop_assert!( | ||
| TOKENS_CSS.contains(hex), | ||
| "TOKENS_CSS missing lab_coat hex {:?}", | ||
| hex, | ||
| ); |
There was a problem hiding this comment.
Suggestion: This only checks that each hex occurs somewhere in the stylesheet, not that it is assigned to the corresponding Lab-Coat variable. If an assignment is changed or removed while the same value remains in another declaration or comment, the test still passes and falsely reports the Rust mirror as synchronized. Validate the variable/value assignment together, as the existing token unit test does. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Incorrect CSS assignments can evade added properties.
- ❌ Viewer colors may fall back after token removal.
- ⚠️ Full Lab-Coat mirror coverage remains incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_tokens.rs
**Line:** 109:119
**Comment:**
*Incorrect Condition Logic: This only checks that each hex occurs somewhere in the stylesheet, not that it is assigned to the corresponding Lab-Coat variable. If an assignment is changed or removed while the same value remains in another declaration or comment, the test still passes and falsely reports the Rust mirror as synchronized. Validate the variable/value assignment together, as the existing token unit test does.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Property: every `REQUIRED_CSS_VARS` entry appears as a | ||
| /// substring of `TOKENS_CSS`. Catches drift where a var name is | ||
| /// added to the list without updating the CSS file. | ||
| #[test] | ||
| fn required_css_var_in_tokens_css(i in required_var_index_strategy()) { | ||
| let var = REQUIRED_CSS_VARS[i]; | ||
| prop_assert!( | ||
| TOKENS_CSS.contains(var), | ||
| "TOKENS_CSS missing required CSS var {:?}", | ||
| var, | ||
| ); |
There was a problem hiding this comment.
Suggestion: Substring matching does not establish that a required variable is declared as a CSS custom property. In particular, --sl-text is satisfied by the longer --sl-text-muted name, and --sl-accent is satisfied by --sl-accent-secondary; removing the shorter declaration would therefore leave this test green. Check declaration boundaries, such as a parsed declaration name or a line beginning with the exact variable followed by :. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Missing semantic variables can break viewer colors.
- ⚠️ Prefix collisions produce false-positive tests.
- ⚠️ Body and component styles consume affected variables.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_tokens.rs
**Line:** 150:160
**Comment:**
*Incorrect Condition Logic: Substring matching does not establish that a required variable is declared as a CSS custom property. In particular, `--sl-text` is satisfied by the longer `--sl-text-muted` name, and `--sl-accent` is satisfied by `--sl-accent-secondary`; removing the shorter declaration would therefore leave this test green. Check declaration boundaries, such as a parsed declaration name or a line beginning with the exact variable followed by `:`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| fn viewer_color_scheme_declares_both_selectors(_i in 0u8..4) { | ||
| prop_assert!(VIEWER_COLOR_SCHEME.contains(":root")); | ||
| prop_assert!(VIEWER_COLOR_SCHEME.contains("[data-theme=\"dark\"]")); |
There was a problem hiding this comment.
Suggestion: The two independent substring assertions do not require the dark selector to be :root[data-theme="dark"]. A stylesheet containing :root plus an unrelated selector containing [data-theme="dark"] would pass even though the root dark-mode rule is missing. Assert the combined selector as one exact required substring or parse the selector. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Root dark-mode wiring can evade regression tests.
- ❌ Browser color-scheme switching may stop working.
- ⚠️ Viewer theme behavior depends on this selector.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_tokens.rs
**Line:** 171:173
**Comment:**
*Incorrect Condition Logic: The two independent substring assertions do not require the dark selector to be `:root[data-theme="dark"]`. A stylesheet containing `:root` plus an unrelated selector containing `[data-theme="dark"]` would pass even though the root dark-mode rule is missing. Assert the combined selector as one exact required substring or parse the selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sl-viewer/tests/properties_viewer_tokens.rs`:
- Around line 129-147: Update required_css_var_starts_with_double_dash and
required_css_var_nonempty to reject the bare "--" value by requiring each entry
to contain at least one character after the prefix. If supported by the existing
test scope, also validate that the suffix matches the documented CSS
custom-property identifier format.
- Around line 171-184: Strengthen viewer_color_scheme_declares_both_selectors
and viewer_color_scheme_declares_color_scheme_property to validate complete CSS
rules rather than substring counts. Assert a standalone light :root rule and the
exact :root[data-theme="dark"] rule, then verify each rule contains exactly one
color-scheme declaration, excluding matches from other rules or comments.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 960c5c9d-f775-46f0-a9a0-1ff64ae30a02
📒 Files selected for processing (4)
CHANGELOG.mdcrates/sl-viewer/tests/properties_viewer_tokens.rsdocs/ops/TRACEABILITY.jsondocs/ops/WBS.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Kilo Code Review
- GitHub Check: Summary
- GitHub Check: prepare
- GitHub Check: browser e2e · axe · responsive · visual
⚠️ CI failures not shown inline (2)
GitHub Check: Summary: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
GitHub Check: Mergify Merge Queue: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
🧰 Additional context used
📓 Path-based instructions (5)
*
📄 CodeRabbit inference engine (AGENTS.md)
*: Perform feature work in a git worktree under.claude/worktrees/, created fromorigin/mainon a branch named<type>/<topic>, rather than working directly onmain.
Do not make direct commits to protectedmain; use a pull request.
Do not usegit reset --hard,git stash, orgit cleanin worktrees.
Do not use--no-verifyor bypass hooks without operator approval.
Do not work on a branch or worktree another actor is using.
Files:
CHANGELOG.md
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Use the Rust toolchain pinned inrust-toolchain.toml; the workspace MSRV is Rust 1.85.
Validate Rust workspace changes with the prescribed locked build, all-features test suite, Clippy, and rustfmt checks where applicable.
Files:
crates/sl-viewer/tests/properties_viewer_tokens.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Fix Clippy warnings; do not add
#[allow]unless it includes a tracking-issue comment.
Files:
crates/sl-viewer/tests/properties_viewer_tokens.rs
crates/sl-viewer/**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
crates/sl-viewer/**/*.{rs,toml}: Thesl-viewercrate uses Dioxus 0.6; use the Dioxus CLI/toolchain for desktop development and bundling.
Usecargo check -p sl-vieweras the fast inner-loop check for viewer changes.
Files:
crates/sl-viewer/tests/properties_viewer_tokens.rs
crates/sl-viewer/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When packaging the macOS viewer, account for the documented Electrobun/Dioxus code-signing requirements.
Files:
crates/sl-viewer/tests/properties_viewer_tokens.rs
🪛 LanguageTool
docs/ops/WBS.md
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...; fuzz/fuzz_targets/jsonl_ingest.rs; .github/workflows/ci.yml; .github/workflows/b...
(GITHUB)
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...ingest.rs; .github/workflows/ci.yml; .github/workflows/bench-gate.yml; docs/ops/pe...
(GITHUB)
| fn required_css_var_starts_with_double_dash(i in required_var_index_strategy()) { | ||
| let var = REQUIRED_CSS_VARS[i]; | ||
| prop_assert!(var.starts_with("--"), "var {:?} must start with '--'", var); | ||
| } | ||
|
|
||
| /// Property: `REQUIRED_CSS_VARS` has no duplicates. | ||
| #[test] | ||
| fn required_css_vars_unique(_i in 0u8..4) { | ||
| let list = REQUIRED_CSS_VARS; | ||
| let set: HashSet<_> = list.iter().collect(); | ||
| prop_assert_eq!(set.len(), list.len()); | ||
| } | ||
|
|
||
| /// Property: every `REQUIRED_CSS_VARS` entry is non-empty (no | ||
| /// empty `--` strings accidentally added). | ||
| #[test] | ||
| fn required_css_var_nonempty(i in required_var_index_strategy()) { | ||
| let var = REQUIRED_CSS_VARS[i]; | ||
| prop_assert!(!var.is_empty(), "REQUIRED_CSS_VARS[{}] is empty", i); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject the bare "--" custom-property name.
required_css_var_starts_with_double_dash accepts "--". required_css_var_nonempty also accepts it because the string is not empty. This does not enforce the documented no-empty-name invariant.
Require at least one character after the prefix. Consider validating the supported CSS identifier format too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_viewer_tokens.rs` around lines 129 - 147,
Update required_css_var_starts_with_double_dash and required_css_var_nonempty to
reject the bare "--" value by requiring each entry to contain at least one
character after the prefix. If supported by the existing test scope, also
validate that the suffix matches the documented CSS custom-property identifier
format.
| fn viewer_color_scheme_declares_both_selectors(_i in 0u8..4) { | ||
| prop_assert!(VIEWER_COLOR_SCHEME.contains(":root")); | ||
| prop_assert!(VIEWER_COLOR_SCHEME.contains("[data-theme=\"dark\"]")); | ||
| } | ||
|
|
||
| /// Property: `VIEWER_COLOR_SCHEME` declares `color-scheme` for | ||
| /// both modes (the W3C CSS prop that triggers browser scrollbar | ||
| /// and form-control color flips). | ||
| #[test] | ||
| fn viewer_color_scheme_declares_color_scheme_property(_i in 0u8..4) { | ||
| prop_assert!(VIEWER_COLOR_SCHEME.contains("color-scheme")); | ||
| // Both modes must set the property. | ||
| let occurrences = VIEWER_COLOR_SCHEME.matches("color-scheme").count(); | ||
| prop_assert_eq!(occurrences, 2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate complete selector rules instead of independent substrings.
Line 172 succeeds when only :root[data-theme="dark"] exists because that selector already contains :root. Line 173 accepts the dark attribute selector anywhere in the CSS. The color-scheme count can also pass when both declarations occur in one rule or in comments.
Assert a standalone light :root rule, the exact :root[data-theme="dark"] rule, and one color-scheme declaration inside each rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_viewer_tokens.rs` around lines 171 - 184,
Strengthen viewer_color_scheme_declares_both_selectors and
viewer_color_scheme_declares_color_scheme_property to validate complete CSS
rules rather than substring counts. Assert a standalone light :root rule and the
exact :root[data-theme="dark"] rule, then verify each rule contains exactly one
color-scheme declaration, excluding matches from other rules or comments.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// starting with `#`. Since we can't introspect Rust modules at runtime, | ||
| /// we hard-code the list (mirroring `tokens.rs`). The constants are | ||
| /// public — any new addition requires also extending this list, which | ||
| /// the `proptest` exhaustiveness check below will catch. |
There was a problem hiding this comment.
WARNING: Exhaustiveness claim is inaccurate
The comment claims "the proptest exhaustiveness check below will catch" missing lab_coat constants, but lab_coat_hexes_distinct only verifies that the hardcoded list has no duplicates — it does not check that the list is complete. If a new lab_coat constant is added without updating lab_coat_hex_list(), the test silently won't cover it. Consider adding a compile-time assertion or a test that iterates the actual lab_coat module to verify completeness.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// Property: every `lab_coat::*` hex constant is non-empty (sanity | ||
| /// check — the well-formedness check above is the stricter version). | ||
| #[test] | ||
| fn lab_coat_hex_nonempty(i in lab_coat_hex_indices_strategy()) { |
There was a problem hiding this comment.
SUGGESTION: Redundant non-empty check
lab_coat_hex_nonempty verifies that each hex is non-empty, but this is already enforced by lab_coat_hex_well_formed (a 7-character string cannot be empty). The check provides no additional coverage beyond what the well-formedness property already guarantees.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 58.5K · Output: 19.7K · Cached: 629.4K |
|
Closing due to merge conflicts. |
User description
Summary
Adds
crates/sl-viewer/tests/properties_viewer_tokens.rswith 10 proptest properties pinning thetokensmodule SSOT invariants (WBS-6.2 #450).lab_coat::*(4 properties)#RRGGBB(7-char lowercase ASCII hex).TOKENS_CSS.REQUIRED_CSS_VARS(4 properties)--.TOKENS_CSS.VIEWER_COLOR_SCHEME(2 properties):rootand:root[data-theme="dark"]selectors.color-schemeexactly twice.Validation
cargo test -p sl-viewer --test properties_viewer_tokens --features "desktop parquet" --locked— 10 passedcargo fmt --all --check— cleanWBS / TRACEABILITY
WBS-6.2 evidence list and
TRACEABILITY.jsongaincrates/sl-viewer/tests/properties_viewer_tokens.rs. Status stayspartial(fuzzing cadence, full loom/shuttle, perf-budget gates remain). CHANGELOG Unreleased documents the new surface.CodeAnt-AI Description
Add automated checks that keep viewer color tokens, CSS variables, and theme rules consistent
What Changed
Impact
✅ Fewer color-token and CSS drift regressions✅ Consistent light and dark theme wiring✅ Clearer automated coverage for viewer styling💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.