Repository navigation
fix(lit): ATX closing sequences, localized task glyphs (#79), sparkline label and fill (#81) - #82
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used🪛 LanguageTool.changeset/lit-markdown-a11y-sparkline.md[typographical] ~7-~7: Consider using an em dash in dialogues and enumerations. (DASH_RULE) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe pull request updates Markdown heading parsing and task-list accessibility labels. It adds translations for task states across 20 locales. It also changes sparkline value-label placement and line fill styling, with tests and a patch changeset. ChangesLit rendering updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The inspected changes implement the stated heading, task-label, and sparkline behavior, with assertions covering the main cases. No concrete merge-blocking issue is established; merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 25 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #82 - fix(lit): ATX closing sequences, localized task glyphs (#79), sparkline label and fill (#81)
Head: 506b156db45e495acdb8d3e2b23b205bcf8f9c80 into main. Review event: COMMENT.
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
.changeset/lit-markdown-a11y-sparkline.md |
added | +10/-0 | Documentation | Low |
packages/lit/src/locales.test.ts |
modified | +33/-0 | Test coverage | Low |
packages/lit/src/locales/ar.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/de.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/es.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/fa.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/fr.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/hi.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/id.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/it.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ja-JP.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ko.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/nl.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/pl.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/pt-BR.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ru.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/th.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/tr.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/uk.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/vi.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/zh-CN.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/zh-TW.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/markdown.ts |
modified | +13/-2 | Changed file | Low |
packages/lit/src/renderers/chart.ts |
modified | +10/-1 | Changed file | Low |
packages/lit/src/renderers/renderers.test.ts |
modified | +92/-0 | Test coverage | Low |
2 additional changed files omitted from this walkthrough.
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- ATX heading closing hash sequences are removed only when whitespace-separated and terminal.
- Markdown task-list glyph aria-labels now use localized, HTML-escaped strings across all 20 shipped locales.
- Sparkline value labels move into a trailing flex column and receive top/middle/bottom placement from the last point.
- Up/down sparkline CSS no longer applies a fill to the line path.
Affected invariants:
- Markdown output remains sanitized, including embedder-supplied accessibility labels.
- Task glyphs retain an accessible checked/unchecked name in every shipped locale.
- Sparkline polylines remain unfilled while endpoint dots retain trend colors.
- The changeset declares the observable @boardstate/lit behavior changes.
Evidence:
- packages/lit/src/markdown.ts and packages/lit/src/renderers/renderers.test.ts cover closing-sequence edge cases and escaped localized labels.
- packages/lit/src/locales.test.ts enumerates locale modules and requires both task-label keys.
- packages/lit/src/renderers/chart.ts and packages/lit/src/styles/boardstate.css implement endpoint-band label placement and separate line stroke from dot fill.
- packages/lit/src/renderers/renderers.test.ts checks sparkline DOM order, placement classes, line fill attributes, and relevant theme CSS rules.
Limitations:
- Review was limited to the supplied diff; no shell commands, project tests, builds, package scripts, app commands, or arbitrary PR code were executed.
- Visual layout and assistive-technology behavior were not exercised in a browser.
No-finding rationale: The supplied changes and focused regression tests are internally consistent, and no validated correctness, security, data-loss, CI, release, or supported-path regression was identified from the available evidence.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Related Context
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
The regex /(?:^|[ \t]+)#+[ \t]*$/ backtracked quadratically on a long run of spaces inside a heading (50k spaces ~1.2 s). A hand-written scan keeps the same CommonMark semantics (space-preceded # run, then only spaces) in linear time. Regression test: 100k spaces render in well under 500 ms.
There was a problem hiding this comment.
Walkthrough
PR: #82 - fix(lit): ATX closing sequences, localized task glyphs (#79), sparkline label and fill (#81)
Head: d8ecd33cb3ac7e4f9275ba75f1e3eef66fcaae99 into main. Review event: COMMENT.
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
.changeset/lit-markdown-a11y-sparkline.md |
added | +10/-0 | Documentation | Low |
packages/lit/src/locales.test.ts |
modified | +33/-0 | Test coverage | Low |
packages/lit/src/locales/ar.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/de.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/es.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/fa.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/fr.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/hi.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/id.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/it.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ja-JP.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ko.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/nl.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/pl.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/pt-BR.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/ru.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/th.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/tr.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/uk.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/vi.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/zh-CN.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/locales/zh-TW.ts |
modified | +2/-0 | Changed file | Low |
packages/lit/src/markdown.ts |
modified | +27/-2 | Changed file | Low |
packages/lit/src/renderers/chart.ts |
modified | +10/-1 | Changed file | Moderate: validated P2 finding |
packages/lit/src/renderers/renderers.test.ts |
modified | +102/-0 | Test coverage | Low |
2 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- ATX closing hash sequences are stripped when whitespace-preceded and followed only by whitespace.
- Markdown task-list glyph labels now use localized string-table entries across 20 locale files.
- Sparkline labels move from absolute overlay positioning into a trailing flex item.
- Up/down sparkline CSS no longer applies a fill to the line element.
Affected invariants:
- Markdown output must remain sanitized and follow CommonMark heading semantics.
- Assistive labels must be localized and safely attribute-escaped.
- Sparkline lines must remain unfilled, and value labels must not overlap or clip while tracking the reported endpoint.
Evidence:
- packages/lit/src/markdown.ts:48
- packages/lit/src/markdown.ts:85
- packages/lit/src/renderers/chart.ts:65
- packages/lit/src/styles/boardstate.css:2224
- packages/lit/src/renderers/renderers.test.ts:396
Limitations:
- Review was limited to the supplied checkout diff; no shell commands, tests, builds, package scripts, or rendered-browser checks were run.
No-finding rationale: No additional correctness, security-boundary, data-loss, API-compatibility, or CI-breaking defect was validated from the supplied diff.
Risk Taxonomy
- Runtime correctness: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Related Context
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
Four small fixes in
@boardstate/litfor the Boardstate plugin's v1.5.1 (onepatchchangeset):## Roadmap ##renders as a heading without the trailing hashes (only a space-preceded#run followed by spaces counts, per CommonMark;## Roadmap##is not a closing sequence).t()with two new keys in every locale (English stays the fallback) and are HTML-escaped; a test asserts all 20 locales define both keys.fill:on the line path, overridingfill="none"; it now sets onlystroke(a dot-only rule keeps the dot fill). Test: the up/down polyline keepsfill="none"and no stylesheet rule fills the line.Gates: lit 165/165, root 913/913, lint, typecheck, build,
git diff --check. Red-first evidence for every item. Rendered before/after captures reviewed by the maintainer.Summary by CodeRabbit