Hi Mr. Ralf, here is the updated frontend design!#15
Merged
rdmueller merged 1 commit intoLLM-Coding:mainfrom Mar 16, 2026
Merged
Hi Mr. Ralf, here is the updated frontend design!#15rdmueller merged 1 commit intoLLM-Coding:mainfrom
rdmueller merged 1 commit intoLLM-Coding:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request significantly enhances the mitigation and risk visualization system with smooth animations and improved interactivity. The MitigationCard component receives visual refinement and animated measures rendering. RadarChart undergoes a major rewrite to support animated value transitions, interactive tooltips, and registration-based updates. RiskRadar integrates RAF-based value interpolation to animate slider changes in sync with the chart. CSS styling is updated across multiple components with refined transitions, hover states, and new animation keyframes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Slider as RiskRadar<br/>(Slider)
participant AnimLoop as RAF<br/>Animation Loop
participant ChartRef as RadarChart<br/>(Updater Ref)
participant SVG as SVG Elements
User->>Slider: Adjust slider value
Slider->>AnimLoop: onChange triggers animateTo(targetValue)
AnimLoop->>AnimLoop: Interpolate value via RAF<br/>(floatValuesRef)
AnimLoop->>Slider: Update slider DOM value
AnimLoop->>ChartRef: Call chartValuesRef updater<br/>with animated value
ChartRef->>SVG: Animate polygon/dots/labels<br/>via animateColor/animateAttr
SVG->>User: Smooth visual transition
sequenceDiagram
actor User
participant Chart as RadarChart
participant SVG as SVG Dots/Labels
participant Tooltip as Tooltip Group
User->>Chart: Hover over dot marker
Chart->>SVG: Detect mouseenter on dot
SVG->>Tooltip: Position tooltip near axis
Tooltip->>Tooltip: Trigger tooltipIn animation
Tooltip->>User: Display labeled value<br/>with shadow effect
User->>Chart: Mouse leave
Chart->>Tooltip: Remove tooltip or fade out
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/MitigationCard.jsx (1)
16-20:⚠️ Potential issue | 🟠 MajorUse a real button for the collapsible header.
Line 17 turns a
<div>into the control that reveals measures, but it is neither focusable nor keyboard-operable. That blocks keyboard users from opening the section. A<button type="button">witharia-expanded/aria-controlswould fix the interaction cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MitigationCard.jsx` around lines 16 - 20, Replace the non-interactive div header with a real semantic control: use a <button type="button"> (keep className={styles.header} and style prop) and move the onClick handler there (retain onClick={() => active && setOpen(!open)}), add aria-expanded={open} and aria-controls pointing at the collapsible content id (e.g. aria-controls={`measures-${id}`}), and when inactive set disabled or aria-disabled to true so keyboard users cannot operate it; also ensure the collapsible panel element has the matching id (e.g. id={`measures-${id}`}) so the aria-controls reference resolves (references: MitigationCard.jsx, setOpen, open, active, styles.header).
🧹 Nitpick comments (1)
src/components/MitigationCard.module.css (1)
71-79: Avoid a fixed height cap for expandable measures.This wrapper still clips anything taller than
600px, so longer descriptions or future translation growth can disappear behindoverflow: hidden. If this is meant to support arbitrary content height, the CSS should use the grid approach described in the comment or a measured height.CSS-only version that does not depend on a magic number
.measuresWrapper { - max-height: 0; - overflow: hidden; - transition: max-height 0.6s cubic-bezier(0.4, 0, 0.2, 1); + display: grid; + grid-template-rows: 0fr; + transition: grid-template-rows 0.6s cubic-bezier(0.4, 0, 0.2, 1); } .measuresWrapperOpen { - max-height: 600px; - transition: max-height 0.8s cubic-bezier(0.4, 0, 0.2, 1); + grid-template-rows: 1fr; } .measures { + min-height: 0; + overflow: hidden; margin-top: 10px; display: flex; flex-direction: column;Also applies to: 82-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MitigationCard.module.css` around lines 71 - 79, The current .measuresWrapper/.measuresWrapperOpen use a fixed max-height:600px which clips taller content; replace the fixed cap with a CSS-grid based expandable technique: change .measuresWrapper to use display: grid and grid-template-rows: 0fr with transition on grid-template-rows (or grid-auto-rows) and set .measuresWrapperOpen to grid-template-rows: 1fr so the container expands to its content height without a magic number; alternatively use a measured CSS variable for height if a JS measurement approach is preferred. Ensure the transition targets the grid rows (grid-template-rows or height via a CSS var) and remove the hard-coded max-height values from both .measuresWrapper and .measuresWrapperOpen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/RadarChart.jsx`:
- Around line 355-364: The circle points are only mouse-accessible; make them
keyboard-accessible by adding tabIndex={0}, onFocus={(e) => handleDotEnter(e,
i)} and onBlur={handleDotLeave} to the same element using the existing
dotRefs.current[i] setup, and provide an accessible label (e.g., aria-label or
aria-describedby) that describes the point value/category so screen readers can
announce it; consider adding role="button" to styles.circle for clear semantics.
Ensure you update the element where dotRefs, handleDotEnter, handleDotLeave, and
styles.circle are used so keyboard focus triggers the exact same tooltip
behavior as mouse hover.
In `@src/components/RadarChart.module.css`:
- Around line 33-40: The CSS uses camelCase keyframe names and the CSS color
keyword currentColor which violate stylelint; rename keyframes to kebab-case
(e.g., change pulseHover → pulse-hover, tooltipIn → tooltip-in) and update all
animation properties that reference them (for example in the .circle class's
animation and :hover animation). Also replace currentColor with lowercase
currentcolor everywhere keywords are used. Apply these same renames/keyword
fixes to the other keyframe usages mentioned (the blocks covering the other
occurrences).
In `@src/components/RiskRadar.jsx`:
- Around line 95-102: The set() handler cancels rafRef.current but only updates
floatValuesRef, chartValuesRef, and React state, leaving uncontrolled slider DOM
thumbs out of sync; update the DOM slider inputs to the same rounded snapshot
before calling setValues (e.g., iterate your slider refs collection and assign
each ref.value = rounded for the affected key or for the full snapshot) or
convert the inputs to controlled components so their value comes from state;
apply the same synchronization fix to the analogous block referenced at lines
205-213 (the other setter that cancels rafRef).
- Around line 259-267: The locked summary currently hard-codes "locked" and
"Tier", causing mixed-language output; add translation keys (e.g., t.lockedLabel
and t.tierLabel) to the locale tables and replace the hard-coded strings in the
RiskRadar.jsx JSX (within the lockedSummary/lockedText block that uses
lockedCount, locked, and TIER_BG) to use t.lockedLabel and t.tierLabel instead;
ensure the sentence still uses the existing plural logic for measures
(t.measures/t.measure) and build the tier span text from the new t.tierLabel
(e.g., `${t.tierLabel} ${g.tier}`) so all fragments come from translations.
In `@src/components/RiskRadar.module.css`:
- Around line 273-277: The CSS uses animation: tierPop which violates the
keyframes-name-pattern; rename the keyframes and its usage to kebab-case (e.g.,
tier-pop) so the `@keyframes` block (currently defining tierPop) and the
.tierNumber rule (animation: tierPop 0.5s ...) both reference the same
kebab-case identifier; update both the animation property in the .tierNumber
selector and the corresponding `@keyframes` declaration (and any other occurrences
in the file, e.g., the block around lines 279-292) to the new kebab-case name.
---
Outside diff comments:
In `@src/components/MitigationCard.jsx`:
- Around line 16-20: Replace the non-interactive div header with a real semantic
control: use a <button type="button"> (keep className={styles.header} and style
prop) and move the onClick handler there (retain onClick={() => active &&
setOpen(!open)}), add aria-expanded={open} and aria-controls pointing at the
collapsible content id (e.g. aria-controls={`measures-${id}`}), and when
inactive set disabled or aria-disabled to true so keyboard users cannot operate
it; also ensure the collapsible panel element has the matching id (e.g.
id={`measures-${id}`}) so the aria-controls reference resolves (references:
MitigationCard.jsx, setOpen, open, active, styles.header).
---
Nitpick comments:
In `@src/components/MitigationCard.module.css`:
- Around line 71-79: The current .measuresWrapper/.measuresWrapperOpen use a
fixed max-height:600px which clips taller content; replace the fixed cap with a
CSS-grid based expandable technique: change .measuresWrapper to use display:
grid and grid-template-rows: 0fr with transition on grid-template-rows (or
grid-auto-rows) and set .measuresWrapperOpen to grid-template-rows: 1fr so the
container expands to its content height without a magic number; alternatively
use a measured CSS variable for height if a JS measurement approach is
preferred. Ensure the transition targets the grid rows (grid-template-rows or
height via a CSS var) and remove the hard-coded max-height values from both
.measuresWrapper and .measuresWrapperOpen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c03295-8208-4908-8086-87516ac9e322
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/MitigationCard.jsxsrc/components/MitigationCard.module.csssrc/components/RadarChart.jsxsrc/components/RadarChart.module.csssrc/components/RiskRadar.jsxsrc/components/RiskRadar.module.csssrc/constants.jssrc/utils.js
Comment on lines
+33
to
+40
| .circle { | ||
| transition: r 0.3s ease; | ||
| animation: pulse 2.4s ease-in-out infinite; | ||
| } | ||
|
|
||
| .circle:hover { | ||
| r: 9px; | ||
| animation: pulseHover 0.8s ease-in-out infinite; |
There was a problem hiding this comment.
Stylelint will fail on these new animation rules.
pulseHover / tooltipIn violate the repo's kebab-case keyframe rule, and currentColor trips value-keyword-case. Rename the keyframes and use currentcolor so this stylesheet passes lint.
Also applies to: 45-70, 89-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RadarChart.module.css` around lines 33 - 40, The CSS uses
camelCase keyframe names and the CSS color keyword currentColor which violate
stylelint; rename keyframes to kebab-case (e.g., change pulseHover →
pulse-hover, tooltipIn → tooltip-in) and update all animation properties that
reference them (for example in the .circle class's animation and :hover
animation). Also replace currentColor with lowercase currentcolor everywhere
keywords are used. Apply these same renames/keyword fixes to the other keyframe
usages mentioned (the blocks covering the other occurrences).
Comment on lines
+95
to
+102
| const set = (k, v) => { | ||
| if (rafRef.current) cancelAnimationFrame(rafRef.current); | ||
| const rounded = Math.round(v); | ||
| const next = { ...floatValuesRef.current, [k]: rounded }; | ||
| floatValuesRef.current = next; | ||
| if (chartValuesRef.current) chartValuesRef.current(next); | ||
| setValues((p) => ({ ...p, [k]: rounded })); | ||
| }; |
There was a problem hiding this comment.
Keep the uncontrolled sliders, chart, and React state in sync when an animation is interrupted.
set() cancels the running rAF, but it only commits the changed key to state and never normalizes the DOM slider values. If a user grabs a slider mid-preset animation—or drags to a fractional value with step="any"—the thumb can stay at the intermediate float while the chart/state snap elsewhere. Sync all slider refs to the same rounded snapshot before setValues, or make the inputs controlled.
One way to keep the three sources of truth aligned
+ const syncSliderValues = (next) => {
+ keys.forEach((key) => {
+ const el = sliderRefs.current[key];
+ if (el) el.value = next[key];
+ });
+ };
+
const set = (k, v) => {
- if (rafRef.current) cancelAnimationFrame(rafRef.current);
- const rounded = Math.round(v);
- const next = { ...floatValuesRef.current, [k]: rounded };
+ if (rafRef.current) {
+ cancelAnimationFrame(rafRef.current);
+ rafRef.current = null;
+ }
+ const base = Object.fromEntries(
+ keys.map((key) => [key, Math.round(floatValuesRef.current[key])]),
+ );
+ const next = { ...base, [k]: Math.round(v) };
floatValuesRef.current = next;
+ syncSliderValues(next);
if (chartValuesRef.current) chartValuesRef.current(next);
- setValues((p) => ({ ...p, [k]: rounded }));
+ setValues(next);
};Also applies to: 205-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RiskRadar.jsx` around lines 95 - 102, The set() handler
cancels rafRef.current but only updates floatValuesRef, chartValuesRef, and
React state, leaving uncontrolled slider DOM thumbs out of sync; update the DOM
slider inputs to the same rounded snapshot before calling setValues (e.g.,
iterate your slider refs collection and assign each ref.value = rounded for the
affected key or for the full snapshot) or convert the inputs to controlled
components so their value comes from state; apply the same synchronization fix
to the analogous block referenced at lines 205-213 (the other setter that
cancels rafRef).
Comment on lines
+259
to
+267
| <div className={styles.lockedSummary}> | ||
| <span className={styles.lockedIcon}>🔒</span> | ||
| <span className={styles.lockedText}> | ||
| {lockedCount} locked {lockedCount !== 1 ? t.measures : t.measure} | ||
| {" — "} | ||
| {locked.map((g, i) => ( | ||
| <span key={g.tier}> | ||
| {i > 0 && ", "} | ||
| <span style={{ color: TIER_BG[g.tier - 1] }}>Tier {g.tier}</span> |
There was a problem hiding this comment.
The locked summary bypasses localization.
"locked" and "Tier" are hard-coded here even though the rest of the component uses t.*. German users will get a mixed-language sentence as soon as any mitigations are locked. Move those fragments into the locale tables and build the summary from translations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RiskRadar.jsx` around lines 259 - 267, The locked summary
currently hard-codes "locked" and "Tier", causing mixed-language output; add
translation keys (e.g., t.lockedLabel and t.tierLabel) to the locale tables and
replace the hard-coded strings in the RiskRadar.jsx JSX (within the
lockedSummary/lockedText block that uses lockedCount, locked, and TIER_BG) to
use t.lockedLabel and t.tierLabel instead; ensure the sentence still uses the
existing plural logic for measures (t.measures/t.measure) and build the tier
span text from the new t.tierLabel (e.g., `${t.tierLabel} ${g.tier}`) so all
fragments come from translations.
Comment on lines
+273
to
+277
| .tierNumber { | ||
| display: inline-block; | ||
| animation: tierPop 0.5s cubic-bezier(0.34, 1.56, 0.64, 1) forwards; | ||
| transition: color 0.6s ease; | ||
| } |
There was a problem hiding this comment.
tierPop currently violates the stylesheet naming rule.
The repo's keyframes-name-pattern expects kebab-case, so both the animation: reference and the @keyframes block need the same kebab-case name.
Also applies to: 279-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RiskRadar.module.css` around lines 273 - 277, The CSS uses
animation: tierPop which violates the keyframes-name-pattern; rename the
keyframes and its usage to kebab-case (e.g., tier-pop) so the `@keyframes` block
(currently defining tierPop) and the .tierNumber rule (animation: tierPop 0.5s
...) both reference the same kebab-case identifier; update both the animation
property in the .tierNumber selector and the corresponding `@keyframes`
declaration (and any other occurrences in the file, e.g., the block around lines
279-292) to the new kebab-case name.
Member
|
thanx! A very valuable addition :-) |
10 tasks
raifdmueller
added a commit
to raifdmueller/vibe-coding-risk-radar
that referenced
this pull request
Mar 16, 2026
…used function) Closes part of LLM-Coding#16 (Muss: Code-Hygiene) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
raifdmueller
added a commit
to raifdmueller/vibe-coding-risk-radar
that referenced
this pull request
Mar 16, 2026
Add i18n keys for "locked", "unlocksAtTier" in both DE and EN. Closes part of LLM-Coding#16 (Muss: i18n) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rdmueller
added a commit
that referenced
this pull request
Mar 16, 2026
Fix tech debt from PR #15 (Muss + Sollte)
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
2 participants
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.
Hi Ralf,
I've completed the frontend improvements on the Vibe-Coding Risk Radar project and have opened a pull request with all the changes.
Here's a quick summary of what's included:
I was careful not to touch any of the core logic, data files or i18n — purely visual and interaction improvements.
Please have a look when you get a chance and let me know if you'd like anything adjusted!
Here is a video of the changes in action: https://drive.google.com/file/d/16dJYgQ-XPv8VyprOXQFqcD4YiVVsJcH8/view?usp=sharing
Best regards
Maria Virk
Summary by CodeRabbit
Release Notes
New Features
Style