Skip to content

fix: align delivered-pr-types chart colors with bucket cards - #11

Merged
ascerra merged 2 commits into
mainfrom
fix/delivered-pr-types-chart-colors
Aug 25, 2026
Merged

fix: align delivered-pr-types chart colors with bucket cards#11
ascerra merged 2 commits into
mainfrom
fix/delivered-pr-types-chart-colors

Conversation

@ascerra

@ascerra ascerra commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix-filer summary cards used theme tokens (--chart-1 / --chart-5 / --accent), but the line chart hardcoded different hex values — so Bot filed looked lime in the cards and orange in the graph.
  • Resolve those CSS theme tokens at runtime for the chart (and related type colors) so cards, legend, and lines stay in sync (including light/dark).

Test plan

  • Open docs/delivered-pr-types.html locally or on the Pages preview
image
  • Confirm Core / External / Bot card colors match the corresponding chart lines and legend swatches
  • Hard-refresh if a cached delivered-pr-types.js is still serving old colors

Resolve theme CSS tokens for the fix-filer line chart so Bot filed uses accent lime (not orange) and matches the summary cards.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Align delivered PR chart colors with themed bucket cards

🐞 Bug fix ✨ Enhancement 🕐 Less than 10 minutes

Grey Divider

AI Description

• Resolve chart colors from active CSS theme tokens with safe fallbacks.
• Align fix-source lines and legends with Core, External, and Bot summary cards.
• Apply themed semantic colors to related PR-type chart series.
Diagram

graph TD
  T["CSS Theme Tokens"] --> R["Theme Resolver"] --> M["Color Maps"] --> D["D3 Renderer"] --> V["Lines and Legend"]
  T --> B["Bucket Cards"]
Loading
High-Level Assessment

The runtime token resolver is the most appropriate approach because it preserves concrete SVG color values for browser compatibility while making the chart follow the existing CSS theme palette. Direct CSS-variable SVG attributes were dismissed due to inconsistent browser behavior, while duplicated hardcoded colors caused the original mismatch.

Files changed (1) +19 / -4

Bug fix (1) +19 / -4
delivered-pr-types.jsResolve chart series colors from CSS theme tokens +19/-4

Resolve chart series colors from CSS theme tokens

• Adds a helper that converts CSS custom properties into concrete colors with safe fallbacks. Fix-source chart lines and legend swatches now match their summary cards, while related PR-type series also adopt existing semantic theme colors.

docs/delivered-pr-types.js

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:49 PM UTC · Completed 12:03 AM UTC

Commit: d3b5df2 · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Theme changes leave stale charts ✓ Resolved 🐞 Bug ≡ Correctness
Description
themeColor() is evaluated only when the two color maps are initialized, whereas the bucket cards
continue to use live CSS variables. Changing the system color scheme while the page is open updates
the cards but leaves chart lines and legend swatches in the previous scheme until reload.
Code

docs/delivered-pr-types.js[R6-8]

+  function themeColor(name, fallback) {
+    const v = getComputedStyle(document.documentElement).getPropertyValue(name).trim();
+    return v || fallback;
Relevance

●●● Strong

Directly undermines the stated theme-sync intent; reviewers accepted related runtime correctness
fixes in this dashboard PR.

PR-#7

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The color maps call themeColor() once at module initialization, and all later renders reuse those
stored values. The stylesheet changes the underlying tokens through prefers-color-scheme, while
the script registers resize and control listeners but no color-scheme listener or color refresh.

docs/delivered-pr-types.js[5-26]
docs/delivered-pr-types.js[253-297]
docs/style.css[24-41]
docs/style.css[160-173]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Theme colors are captured once during script initialization. Re-resolve the color maps and rerender when the `prefers-color-scheme` media query changes, or resolve the tokens during each render.

## Issue Context
The cards directly reference CSS variables and therefore react immediately to media-query changes. Chart paths and legend swatches receive concrete values from constants initialized only once.

## Fix Focus Areas
- docs/delivered-pr-types.js[5-26]
- docs/delivered-pr-types.js[253-297]
- docs/style.css[24-41]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Hero type colors diverge ✓ Resolved 🐞 Bug ≡ Correctness
Description
TYPE_COLORS now uses theme tokens, but renderHero() still hardcodes the previous fix and feature
hex values. The hero metrics therefore no longer match the corresponding chart colors, and remain
dark-palette colors in light mode.
Code

docs/delivered-pr-types.js[R11-12]

+    feat: themeColor("--chart-1", "#5cb3ff"),
+    fix: themeColor("--negative", "#f87171"),
Relevance

●●● Strong

Clear same-file color inconsistency contradicts the PR’s stated goal of synchronizing cards, charts,
and legends.

PR-#7

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed TYPE_COLORS maps feature and fix to theme-derived colors, while renderHero() at
lines 236-237 still emits the old #ff453a and #6b93f7 values. The light palette also defines
different token values in style.css, proving those hardcoded hero values cannot follow the
selected scheme.

docs/delivered-pr-types.js[10-19]
docs/delivered-pr-types.js[225-238]
docs/style.css[24-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The delivery chart colors now come from `TYPE_COLORS`, while the hero's Fix PR and top-type values still use obsolete hardcoded hex colors. Render hero colors from the same resolved color map so related metrics remain synchronized in both themes.

## Issue Context
The previous hero hex values matched the previous `feat` and `fix` chart values. This PR replaces those chart values without updating the duplicated hero styling.

## Fix Focus Areas
- docs/delivered-pr-types.js[10-19]
- docs/delivered-pr-types.js[225-238]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread docs/delivered-pr-types.js Outdated
Comment thread docs/delivered-pr-types.js
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [pattern-inconsistency] docs/delivered-pr-types.js:6 — The sibling file dashboard.js passes CSS variable references ("var(--chart-1)") directly to SVG fill and stroke attributes, while this file now uses getComputedStyle() to resolve CSS variables to concrete hex strings. This inconsistency pre-dates the PR — the old code already used hardcoded hex values rather than var() references — so this is not a regression. The PR improves correctness by tying colors to live CSS tokens. Consider unifying the color pattern across both files in a follow-up.

  • [missing-authorization] — This PR has no linked issue. The diff is non-trivial (80+ changed lines with structural refactoring of color constants into runtime-resolved functions, new CSS custom properties, and a new matchMedia event listener). While the scope is well-contained and the intent is clear from the PR description, non-trivial changes should trace to an authorized issue for traceability.

Previous run

Review

Findings

Medium

  • [logic-error] docs/delivered-pr-types.js:221renderHero still hardcodes #ff453a for the Fix count and #6b93f7 for the top-type percentage. After this PR, TYPE_COLORS.fix resolves to themeColor('--negative', '#f87171') and TYPE_COLORS.feat resolves to themeColor('--chart-1', '#5cb3ff'). The hero stats will display colors that no longer match the chart lines or legend swatches, undermining the PR’s goal of keeping visual elements in sync.
    Remediation: Replace the hardcoded inline colors with the resolved values from TYPE_COLORS, e.g.: style="color:${TYPE_COLORS.fix}" and style="color:${TYPE_COLORS[topType] || TYPE_COLORS.feat}".

Low

  • [consistency] docs/delivered-pr-types.js:11TYPE_COLORS mixes two color-assignment idioms: feat, fix, ci, test, and perf use themeColor() to resolve CSS custom properties, while docs, chore, and other remain bare hex literals. Within a single constant map, some colors will respond to the active theme and others will not.
    Remediation: Either wrap the remaining bare-hex entries in themeColor() with themselves as the fallback, or add a comment explaining why those entries intentionally skip theme resolution.

Labels: PR fixes a visual bug in the docs dashboard (color mismatch between chart and cards)


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the bug Something isn't working label Aug 19, 2026
Re-resolve type/fix colors on each render and prefers-color-scheme
change, drive hero stats from the same map, and theme docs/chore/other
via CSS tokens so light/dark flips stay consistent without reload.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@ascerra

ascerra commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review findings in a0564f0:

  • Medium / renderHero hardcoded colors — hero Fix and top-type values now use the same resolved typeColors map as the charts.
  • Low / mixed TYPE_COLORS idiomsdocs / chore / other now resolve via --type-docs / --type-chore / --type-other in style.css (with dark + light values), same themeColor() path as the other series.
  • Theme flip staleness (Qodo) — colors re-resolve each render; prefers-color-scheme changes trigger a redraw.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 20, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:39 AM UTC · Completed 11:53 AM UTC

Commit: a0564f0 · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review August 20, 2026 11:53

Superseded by updated review

Comment thread docs/delivered-pr-types.js
@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 20, 2026
@ascerra
ascerra merged commit a9f594a into main Aug 25, 2026
61 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 25, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ❌ Failure · Started 11:10 AM UTC · Completed 11:11 AM UTC

Commit: a0564f0 · View workflow run →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant