🎨 Palette: Semantic interactive elements for visualizer - #610
🎨 Palette: Semantic interactive elements for visualizer#610sheepdestroyer wants to merge 4 commits into
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe visualizer now uses native buttons for prompt selection and annotation clearing, removing custom keyboard interaction code while preserving the existing visual design through targeted CSS resets and styles. Sequence diagram for accessible prompt selectionsequenceDiagram
participant User
participant PromptButton as PromptButton
participant Visualizer as Visualizer
User->>PromptButton: Native keyboard or pointer activation
PromptButton->>Visualizer: onclick selectPrompt(index)
Visualizer-->>User: Render selected prompt
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments### Comment 1
<locationpath="router/static/visualizer.html"line_range="255" />
<code_context>
const displayPred = pred ? escapeHtml(pred.replace('agent-','')) : '';
- return `<divclass="list-item${i === selectedIdx ? ' selected' : ''}"onclick="selectPrompt(${i})"tabindex="0"role="button"aria-label="Prompt ${i}"onkeydown="if(event.key==='Enter'||event.key===''){event.preventDefault();selectPrompt(${i})}">
+ return `<buttontype="button"class="list-item${i === selectedIdx ? ' selected' : ''}"onclick="selectPrompt(${i})"aria-label="Prompt ${i}">
<divclass="id">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</div>
<divclass="snippet">${escapeHtml(p.prompt.substring(0, 120))}</div>
</code_context>
<issue_to_address>
**issue (bug_risk):** The `aria-label="Prompt ${i}"` overrides the button's descendant text in the accessible name calculation, so screen readers announce only a generic prompt number and omit the prompt snippet, LLM label, classifier label, and review tags.
**Triggers:** When a screen-reader user navigates the prompt list.
**Suggested fix:** Remove the `aria-label` so the button's content supplies its name, or construct an accessible label that includes the prompt and relevant labels.
```suggestion return `<button type="button" class="list-item${i === selectedIdx ? ' selected' : ''}" onclick="selectPrompt(${i})">```
</issue_to_address>
### Comment 2
<locationpath="router/static/visualizer.html"line_range="256-258" />
<code_context>
- return `<divclass="list-item${i === selectedIdx ? ' selected' : ''}"onclick="selectPrompt(${i})"tabindex="0"role="button"aria-label="Prompt ${i}"onkeydown="if(event.key==='Enter'||event.key===''){event.preventDefault();selectPrompt(${i})}">
+ return `<buttontype="button"class="list-item${i === selectedIdx ? ' selected' : ''}"onclick="selectPrompt(${i})"aria-label="Prompt ${i}">
<divclass="id">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</div>
<divclass="snippet">${escapeHtml(p.prompt.substring(0, 120))}</div>
<divclass="tags">${tags}</div>
- </div>`;
+ </button>`;
</code_context>
<issue_to_address>
**nitpick (bug_risk):** Each native `<button>` contains three `<div>` descendants, but a button's HTML content model permits only phrasing content. The generated list markup is therefore invalid and does not provide a valid native-button structure for HTML validators and accessibility tooling.
**Triggers:** Whenever the prompt list is rendered.
**Suggested fix:** Use phrasing elements such as `<span>` for the button's children, applying `display: block` to the layout spans as needed.
```suggestion <span class="id" style="display:block">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</span> <span class="snippet" style="display:block">${escapeHtml(p.prompt.substring(0, 120))}</span> <span class="tags">${tags}</span>```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: router/static/visualizer.html:255
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const displayPred = pred ? escapeHtml(pred.replace('agent-','')) : ''; | ||
| return `<div class="list-item${i === selectedIdx ? ' selected' : ''}" onclick="selectPrompt(${i})" tabindex="0" role="button" aria-label="Prompt ${i}" onkeydown="if(event.key==='Enter'||event.key===' '){event.preventDefault();selectPrompt(${i})}"> | ||
| return `<button type="button" class="list-item${i === selectedIdx ? ' selected' : ''}" onclick="selectPrompt(${i})" aria-label="Prompt ${i}"> |
There was a problem hiding this comment.
issue (bug_risk): The aria-label="Prompt ${i}" overrides the button's descendant text in the accessible name calculation, so screen readers announce only a generic prompt number and omit the prompt snippet, LLM label, classifier label, and review tags.
Triggers: When a screen-reader user navigates the prompt list.
Suggested fix: Remove the aria-label so the button's content supplies its name, or construct an accessible label that includes the prompt and relevant labels.
| return `<buttontype="button" class="list-item${i === selectedIdx ? ' selected' : ''}" onclick="selectPrompt(${i})"aria-label="Prompt ${i}"> | |
| return `<buttontype="button" class="list-item${i === selectedIdx ? ' selected' : ''}" onclick="selectPrompt(${i})"> |
| <div class="id">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</div> | ||
| <div class="snippet">${escapeHtml(p.prompt.substring(0, 120))}</div> | ||
| <div class="tags">${tags}</div> |
There was a problem hiding this comment.
nitpick (bug_risk): Each native <button> contains three <div> descendants, but a button's HTML content model permits only phrasing content. The generated list markup is therefore invalid and does not provide a valid native-button structure for HTML validators and accessibility tooling.
Triggers: Whenever the prompt list is rendered.
Suggested fix: Use phrasing elements such as <span> for the button's children, applying display: block to the layout spans as needed.
| <divclass="id">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</div> | |
| <divclass="snippet">${escapeHtml(p.prompt.substring(0, 120))}</div> | |
| <divclass="tags">${tags}</div> | |
| <spanclass="id"style="display:block">#${i} · LLM: ${displayLlm}${pred ? ' · CLS: ' + displayPred : ''}</span> | |
| <spanclass="snippet"style="display:block">${escapeHtml(p.prompt.substring(0, 120))}</span> | |
| <spanclass="tags">${tags}</span> |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
💡 What: Replaced interactive
<div>and<a>elements with native<button type="button">elements in the dataset visualizer. Added CSS resets to maintain existing styling.🎯 Why: Using custom divs for interactive items creates non-semantic markup and requires manual tabindex/keydown handlers. Native buttons provide out-of-the-box keyboard support and correct screen-reader semantics.
📸 Before/After: Visual presentation remains identical, but keyboard navigation and screen-reader experience are improved.
♿ Accessibility: Ensures list items and the "clear annotation" action announce correctly as interactive buttons to screen readers, and provide native keyboard focus and activation without custom JavaScript handlers.
PR created automatically by Jules for task 2846542459358061979 started by @sheepdestroyer
Summary by Sourcery
Improve visualizer accessibility with semantic controls and align the agy fallback configuration with the supported Claude model.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: