Improve Experimental UI Architecture (Non-Breaking) - #1937
Open
devbyaryanvala wants to merge 5 commits into
Open
devbyaryanvala wants to merge 5 commits into
devbyaryanvala wants to merge 5 commits into
Conversation
--Folder structure improved for ease of future development --Minor UI bugs fixed --No backend Changes has been done Signed-off-by: devbyaryanvala <aryanvala.edu@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughAdds design tokens and modularized CSS, switches to self-hosted fonts and HTML metadata, introduces collapsible sidebar and chat panel state/controls, restructures query/results/header UI, and restyles chat, graph, and logs UIs toward a dark-theme token system. Changes
Sequence Diagram(s)sequenceDiagram
participant User as rgba(18,99,255,0.5)
participant App as rgba(0,168,132,0.5)
participant Chat as rgba(153,102,255,0.5)
User->>App: Click "Collapse Chat" button
App->>Chat: invoke onCollapse() prop
Chat-->>App: onCollapse callback invoked
App->>App: set chatCollapsed = true
App->>User: hide chat panel, show floating chat-open button
User->>App: Click floating "Open chat" button
App->>App: set chatCollapsed = false
App->>Chat: render expanded ChatAssistant
Chat->>User: visible chat panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
experimental/ag-ui/front-end/src/components/ChatAssistant.tsx (2)
119-483:⚠️ Potential issue | 🟠 MajorStale-closure bugs from empty
useCallbackdependency list ininitializeAgent.
initializeAgentis wrapped inuseCallback(..., [])but its body captures several values that will go stale:
connectionStatus(line 124) — captured as'disconnected'forever; theconnectionStatus === 'connected'guard will never become true via this closure, so the "already initialized but not connected" branch for retryingsendInitialMessagecannot fire.sendInitialMessage,fetchModel,scheduleReconnect— all defined withuseCallbacklater in the component. They are only referenced at call time (after mount), so they work, but ESLint'sreact-hooks/exhaustive-depswill (correctly) flag them and a future refactor could trip over this.Because
scheduleReconnectdepends oninitializeAgent, andinitializeAgentshould depend onscheduleReconnect/fetchModel/sendInitialMessage, you have a cyclic-deps shape that's easy to get wrong. Two common fixes:
- Move the mutable values/callbacks behind refs (
connectionStatusRef,sendInitialMessageRef) and read from.currentinsideinitializeAgent.- Add the real deps and break the cycle by making one of the callbacks (
scheduleReconnectorinitializeAgent) a ref-based wrapper.This also affects the
sendInitialMessagecallback at line 543, which depends onpageContext(used insiderunAgent) but omits it from the dependency array (line 587) — a stalepageContextcan be sent on the initial run if props change before the effect fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx` around lines 119 - 483, The initializeAgent useCallback has stale-closure bugs because it closes over mutable values and other callbacks; replace those captured values with refs and read .current inside initializeAgent (e.g., create connectionStatusRef, sendInitialMessageRef, fetchModelRef and scheduleReconnectRef and assign the respective functions/.current where they are defined), update initializeAgent to use connectionStatusRef.current and call sendInitialMessageRef.current()/fetchModelRef.current()/scheduleReconnectRef.current() instead of the direct identifiers, and break the dependency cycle by keeping initializeAgent's dependency array empty while wiring the real callbacks through refs; likewise, fix sendInitialMessage to read pageContext from a pageContextRef (set when props change) so it doesn't capture a stale pageContext.
491-494:⚠️ Potential issue | 🟠 MajorUse
AbortSignal.timeout()instead of non-existenttimeoutoption infetch().The
fetch()API does not support atimeoutoption in its init object. Lines 491-494 (checkConnection) and 507-510 (fetchModel) both pass{ method: 'GET', timeout: 5000 } as anytofetch(), but the timeout is silently ignored. Theas anycast suppresses TypeScript errors that would catch this mistake.Without a working timeout, if the backend is slow or hung,
checkConnection(called fromscheduleReconnectat line 607) will not resolve within 5 seconds. This causesscheduleReconnectto be called again, creating an indefinite loop where exponential backoff never succeeds.Use
AbortSignal.timeout(5000)instead:Proposed fix
const response = await fetch(modelUrl, { method: 'GET', - timeout: 5000 - } as any); + signal: AbortSignal.timeout(5000), + });Apply in both
checkConnectionandfetchModel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx` around lines 491 - 494, The fetch calls in checkConnection and fetchModel incorrectly pass a non-existent timeout option and cast to any; replace that pattern by creating an AbortSignal via AbortSignal.timeout(5000) and pass it as the signal property in the fetch init (e.g., fetch(modelUrl, { method: 'GET', signal })) instead of { timeout: 5000 } and remove the as any cast; ensure any existing catch paths handle AbortError/DOMException appropriately (treat as timeout) so scheduleReconnect's backoff works as intended.
🧹 Nitpick comments (8)
experimental/ag-ui/front-end/src/components/MainContent.tsx (2)
1289-1289: Drop the structural closing comment.This restates the JSX structure rather than explaining why the wrapper exists; indentation should carry this.
🧹 Proposed cleanup
- </div>{/* end page-content */} + </div>As per coding guidelines, Write clear, concise comments that explain 'why' rather than 'what'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` at line 1289, Remove the structural closing comment "/* end page-content */" that follows the closing </div> in the MainContent component's JSX; locate the closing div in MainContent (where the trailing comment appears) and delete the comment so only the JSX remains, keeping comments that explain why code exists but removing this redundant "what"-style marker.
1177-1185: Extract the repeated example-chip handler.The five inline handlers duplicate the same DOM flash logic and use
el, which makes the new UI harder to maintain.♻️ Proposed helper extraction
Add this before
return:+ const applyExampleQuery = React.useCallback((exampleQuery: string) => { + setQuery(exampleQuery); + + const queryInputElement = document.getElementById('query-input'); + if (queryInputElement) { + queryInputElement.classList.add('flash'); + setTimeout(() => queryInputElement.classList.remove('flash'), 500); + } + }, []);Then simplify the chip handlers:
- <button className="example-chip" onClick={() => { setQuery('up'); const el = document.getElementById('query-input'); if(el) el.classList.add('flash'); setTimeout(() => el?.classList.remove('flash'), 500); }}>up<span className="chip-arrow">→</span></button> + <button className="example-chip" onClick={() => applyExampleQuery('up')}>up<span className="chip-arrow">→</span></button>Apply the same pattern to the other example chips.
As per coding guidelines, Use semantic, descriptive names for variables, functions, and components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines 1177 - 1185, The onClick handlers for the example-chip buttons duplicate DOM-flash logic and inline setQuery calls; extract a descriptive helper like handleExampleChip(query: string) (placed in MainContent component before return) that calls setQuery(query), finds the input via document.getElementById('query-input'), adds the 'flash' class and schedules its removal with setTimeout, then replace each inline onClick with onClick={() => handleExampleChip('...')}; apply the same change to all example-chip buttons (e.g., the ones setting 'up', 'rate(http_requests_total[5m])', 'process_cpu_seconds_total', 'source=logs-* | head 10', and 'source=logs-* | where severity="ERROR"') so the DOM-flash behavior is centralized and the code is more maintainable.experimental/ag-ui/front-end/src/styles/layout.css (3)
211-214: Duplicate@keyframes statusPulseacross stylesheets.The same keyframe is defined identically here and in
experimental/ag-ui/front-end/src/styles/sidebar.css(lines 187–190). SinceApp.cssimports both, the later import silently overrides the earlier. Move shared animations (statusPulse,fadeIn,pulse,thinking-pulse,editorFlash,spin,pageFadeIn) into a single base/animations stylesheet to follow DRY and avoid drift.Also note: the stylelint config enforces kebab-case; consider renaming to
status-pulseand updating references (same applies forpageFadeIn→page-fade-in).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/layout.css` around lines 211 - 214, Duplicate keyframe definitions (statusPulse) exist across stylesheets; extract shared animations (statusPulse, fadeIn, pulse, thinking-pulse, editorFlash, spin, pageFadeIn) into a single animations/base stylesheet and import it from App.css instead of defining them in multiple files (e.g., remove from layout.css and sidebar.css). Rename keyframe names to kebab-case (status-pulse, page-fade-in, etc.) and update all references across CSS/JS files to the new names to satisfy stylelint kebab-case rules, ensuring only the new central animations file contains the canonical definitions.
4-37: Consider100dvhfor mobile viewport stability.Both
.appand.observability-platformuseheight: 100vh, which on iOS/Android can cause layout jumps or content clipped under mobile browser chrome (address bar). Since both haveoverflow-y: autoand this is the primary app shell, switching to100dvh(dynamic viewport) avoids the well-known mobile-browser gap. Keep100vhas a fallback if broader support is needed.Proposed change
.app { display: flex; - height: 100vh; + height: 100vh; + height: 100dvh; width: 100vw; ... .observability-platform { display: flex; flex-direction: column; - height: 100vh; + height: 100vh; + height: 100dvh;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/layout.css` around lines 4 - 37, Update the shell height rules for .app and .observability-platform to use the dynamic viewport unit to avoid mobile browser chrome layout issues: replace or supplement height: 100vh with height: 100dvh (and keep 100vh as a fallback if desired). Modify the CSS declarations inside .app and .observability-platform so they prefer height: 100dvh while preserving existing properties like overflow-y: auto, position, and background to maintain current behavior.
217-229: Collapsed chat wrapper still occupies flex child space.
.chat-panel-wrapperonly animateswidthandopacity, and the collapsed state setswidth: 0+pointer-events: none. Because the wrapper is a flex child of.appwithoutflex-shrink: 1on its contents, the internal.chat-assistant(fixedwidth: 420px) can briefly cause horizontal overflow during the 0.3s transition. Consider also transitioning/clamping viamax-width, or settingflex: 0 0 autowith an explicitwidthon the wrapper so the collapse is smoother and doesn't rely on childoverflow: hiddenalone to clip the chat panel during animation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/layout.css` around lines 217 - 229, The collapsed chat wrapper still allows the fixed-width child (.chat-assistant) to force overflow during the width transition; update .chat-panel-wrapper (and its .collapsed state) to animate/max-clamp the container rather than relying on the child to shrink — for example change the animation to use max-width (set an initial max-width matching the assistant, e.g. 420px) and keep overflow:hidden, or make the wrapper a fixed flex item (flex: 0 0 auto) with an explicit width and animate that width to 0 in the .chat-panel-wrapper.collapsed rule; ensure .chat-assistant’s fixed width is matched by the wrapper’s max-width/width so the container clips the child during the 0.3s transition.experimental/ag-ui/front-end/src/components/ChatAssistant.tsx (1)
925-936: Dead avatar markup —.message-avataris hidden via CSS but still renders and loads images per message.
ChatAssistant.csssets.message-avatar { display: none }(line 214 of that file), yet the TSX unconditionally renders a<div className="message-avatar">with an<img src="/holmesgpt-logo.png">for every assistant message and an SVG for every user message. The image itself is cacheable, but you still create hidden DOM nodes per message (and trigger image decode on first render) for no visual benefit. Either remove the avatar block from the TSX, or removedisplay: nonefrom the CSS if avatars are intended to come back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx` around lines 925 - 936, The markup renders a hidden avatar block for every message (the <div className="message-avatar"> inside the messages.map in ChatAssistant.tsx) which still creates DOM nodes and loads the /holmesgpt-logo.png; either remove the avatar markup from the messages.map render or conditionally render it only when CSS shows avatars (e.g., check a feature flag or a prop like showAvatars before rendering <div className="message-avatar"> and the <img>), or alternatively remove/adjust the CSS rule `.message-avatar { display: none }` so avatars are actually used; update ChatAssistant.tsx to stop unconditionally creating the img/SVG when avatars are disabled.experimental/ag-ui/front-end/src/styles/query.css (1)
205-210: Global "legacy compat" hide rules have broad blast radius.
.query-hint,.execute-button,.query-label-row,.query-hint-inlineare all hidden globally viadisplay: none. These are generic names that future components (inside or outsideexperimental/ag-ui) may reuse and be silently hidden without an obvious cause. If this compat layer is truly temporary, please scope the selectors to an ancestor (e.g.,.query-section .execute-button) and/or add a TODO with a removal timeline tied to the old markup being fully deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/query.css` around lines 205 - 210, The global "legacy compat" CSS rules (.query-label-row, .query-hint-inline, .query-hint, .execute-button) are too generic and may hide unrelated elements; scope these selectors under a specific ancestor (e.g., prefix with a container like .query-section or .legacy-query) so only the old query UI is affected, and replace the global selectors in the rule for .query-input-container and .execute-button accordingly; also add a TODO comment near the selectors noting this is temporary, include the reason (old markup compatibility) and a removal timeline or issue/PR reference tied to when the old markup is removed, and keep .run-button untouched as the replacement.experimental/ag-ui/front-end/src/styles/sidebar.css (1)
99-124: Hardcoded hex colors bypass the design-token system.The PR introduces a design-token/CSS custom-properties system, but
.nav-item(and several other rules in this file) hardcode#555555,#1A1A1A,#6366F1,#E5E5E5. This makes theming brittle: future token updates won't propagate here, and the nav will drift from the rest of the UI. Prefer the existing tokens (e.g.,var(--text-muted),var(--bg-inset),var(--accent-indigo)/var(--accent-purple),var(--text-primary)) — or add new tokens if none fit.Same pattern in
query.css(#1A1A2E,#252547,#2A2A2A,#6366F1,#E5E5E5,#555555) andChatAssistant.css(#22C55E,#555555,#2A2A2A,#FCA5A5).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/sidebar.css` around lines 99 - 124, Replace hardcoded hex color values used in the sidebar rules (.nav-item, .nav-item:hover:not(.disabled), .nav-item:active:not(.disabled), .nav-item.active) with the design tokens (e.g., use var(--text-muted) instead of `#555555`, var(--bg-inset) instead of `#1A1A1A`, var(--accent-indigo) or var(--accent-purple) instead of `#6366F1`, and var(--text-primary) instead of `#E5E5E5`); apply the same replacements in query.css and ChatAssistant.css for each listed hex (`#1A1A2E`, `#252547`, `#2A2A2A`, `#6366F1`, `#E5E5E5`, `#555555`, `#22C55E`, `#FCA5A5`) and if no existing token matches the semantic intent, add a new CSS custom property (token) with a clear name and use that token in the corresponding selectors (e.g., .nav-item, .nav-item.active, ChatAssistant styles) so theming updates propagate consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/ag-ui/front-end/public/index.html`:
- Around line 8-10: The HTML currently loads fonts from Google via link tags
(<link rel="preconnect" href="https://fonts.googleapis.com">, <link
rel="preconnect" href="https://fonts.gstatic.com" crossorigin>, and the <link
href="https://fonts.googleapis.com/css2?..."> stylesheet) which leaks client
metadata; replace those external CDN calls by self-hosting the font files and
updating index.html to reference local font assets (or your first-party CDN) and
add appropriate preload/link tags for the local font files and corresponding
`@font-face` declarations in your CSS; ensure you remove the external hrefs, add
local <link rel="preload" as="font"> entries for the font files, and update CSS
to use the self-hosted font family names so the UI no longer calls
fonts.googleapis.com or fonts.gstatic.com.
In `@experimental/ag-ui/front-end/src/App.css`:
- Around line 1-4: The header comment in App.css overstates that "All component
styles are now in src/styles/"; update that comment to accurately reflect that
most component styles were moved to src/styles/ while a few components (e.g.,
GraphVisualization.css and ChatAssistant.css) remain as component-scoped
imports, so future readers aren’t misled — edit the text in the App.css top
comment to say "Most component styles moved to src/styles/; some
component-scoped styles remain (e.g., GraphVisualization.css,
ChatAssistant.css)."
In `@experimental/ag-ui/front-end/src/App.tsx`:
- Around line 140-149: The h2 currently renders an empty string when
sidebarCollapsed is true and the collapse button only relies on title — remove
the empty heading by either rendering the h2 only when there is visible text or
provide an accessible name (e.g., a visually-hidden span) so the
landmark/headline is not empty; add explicit ARIA on the button: set
aria-expanded={sidebarCollapsed} and add an aria-label (e.g.,
aria-label={sidebarCollapsed ? "Expand sidebar" : "Collapse sidebar"}) and mark
decorative SVG parts aria-hidden="true"; update the same pattern where the
controls repeat (lines referenced by the comment) so setSidebarCollapsed,
sidebarCollapsed, the collapse button and the h2 are adjusted accordingly.
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css`:
- Line 221: Replace the deprecated CSS property "word-wrap: break-word;" with
the modern equivalent "overflow-wrap: break-word;" in the relevant rule (locate
the occurrence of the "word-wrap" declaration in ChatAssistant.css) so
Stylelint's property-no-deprecated rule is satisfied; if legacy IE support is
intentionally required, keep "overflow-wrap: break-word;" and place "word-wrap:
break-word;" after it as a fallback.
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx`:
- Around line 1013-1018: The UI hint “⌘↵” is incorrect because handleKeyDown
currently sends on plain Enter; update the hint in ChatAssistant component so
the displayed shortcut matches the actual behavior — replace the macOS-specific
“⌘↵” shown when inputValue.length === 0 with a neutral “↵” or “Enter” string to
avoid platform bias, or alternatively change handleKeyDown to require a modifier
(e.metaKey || e.ctrlKey) if you prefer Cmd/Ctrl+Enter; ensure the change aligns
between the render logic (the span using inputValue) and the send logic in
handleKeyDown.
In `@experimental/ag-ui/front-end/src/components/LogsVisualization.css`:
- Around line 143-151: Replace the deprecated CSS property in the .log-cell
rule: remove word-wrap and add overflow-wrap: break-word so the .log-cell
selector uses the modern standard (overflow-wrap) instead of the legacy
word-wrap to satisfy Stylelint's property-no-deprecated rule.
In `@experimental/ag-ui/front-end/src/styles/explorer.css`:
- Around line 13-25: The CSS shows .explorer-header styled as clickable but
MainContent.tsx toggles on .explorer-title, so make the visual affordance match
the real click target: either move the click handler from .explorer-title to the
header element in MainContent.tsx so .explorer-header remains the interactive
target, or keep the handler on .explorer-title and update explorer.css to apply
cursor: pointer and the :hover background transition to .explorer-title (and
remove them from .explorer-header) so only the actual clickable element appears
interactive.
In `@experimental/ag-ui/front-end/src/styles/query.css`:
- Around line 274-280: The mobile breakpoint override for .run-button currently
sets border-left: var(--accent-purple) causing a single purple edge; update the
.run-button rule in the mobile styles to either remove the border-left
declaration (so it inherits the desktop border color) or change it to
border-left: var(--border-default) so all borders remain consistent with the
editor; locate the .run-button selector in the mobile CSS block and apply this
change.
In `@experimental/ag-ui/front-end/src/styles/results.css`:
- Around line 32-36: Replace the hard-coded color in the .empty-state p rule
with the design-system text token instead of `#555555`; locate the .empty-state p
selector in results.css and change its color to the appropriate CSS variable
token (for example var(--text-secondary) or the project’s text-muted token) so
the helper text uses the existing theme token and meets contrast standards.
- Around line 105-111: The global CSS rule ".graph-container" in results.css is
too generic and overrides GraphVisualization's internal styles; rename or remove
this rule so the graph component owns its own ".graph-container". Either change
the selector in results.css to a result-scoped name (e.g.,
".results-graph-container" or scope under a parent like ".results
.graph-container") and update usages, or delete the rule and move the
background/border/shadow styles into the GraphVisualization component stylesheet
so only the intended component gets those styles.
---
Outside diff comments:
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx`:
- Around line 119-483: The initializeAgent useCallback has stale-closure bugs
because it closes over mutable values and other callbacks; replace those
captured values with refs and read .current inside initializeAgent (e.g., create
connectionStatusRef, sendInitialMessageRef, fetchModelRef and
scheduleReconnectRef and assign the respective functions/.current where they are
defined), update initializeAgent to use connectionStatusRef.current and call
sendInitialMessageRef.current()/fetchModelRef.current()/scheduleReconnectRef.current()
instead of the direct identifiers, and break the dependency cycle by keeping
initializeAgent's dependency array empty while wiring the real callbacks through
refs; likewise, fix sendInitialMessage to read pageContext from a pageContextRef
(set when props change) so it doesn't capture a stale pageContext.
- Around line 491-494: The fetch calls in checkConnection and fetchModel
incorrectly pass a non-existent timeout option and cast to any; replace that
pattern by creating an AbortSignal via AbortSignal.timeout(5000) and pass it as
the signal property in the fetch init (e.g., fetch(modelUrl, { method: 'GET',
signal })) instead of { timeout: 5000 } and remove the as any cast; ensure any
existing catch paths handle AbortError/DOMException appropriately (treat as
timeout) so scheduleReconnect's backoff works as intended.
---
Nitpick comments:
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.tsx`:
- Around line 925-936: The markup renders a hidden avatar block for every
message (the <div className="message-avatar"> inside the messages.map in
ChatAssistant.tsx) which still creates DOM nodes and loads the
/holmesgpt-logo.png; either remove the avatar markup from the messages.map
render or conditionally render it only when CSS shows avatars (e.g., check a
feature flag or a prop like showAvatars before rendering <div
className="message-avatar"> and the <img>), or alternatively remove/adjust the
CSS rule `.message-avatar { display: none }` so avatars are actually used;
update ChatAssistant.tsx to stop unconditionally creating the img/SVG when
avatars are disabled.
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx`:
- Line 1289: Remove the structural closing comment "/* end page-content */" that
follows the closing </div> in the MainContent component's JSX; locate the
closing div in MainContent (where the trailing comment appears) and delete the
comment so only the JSX remains, keeping comments that explain why code exists
but removing this redundant "what"-style marker.
- Around line 1177-1185: The onClick handlers for the example-chip buttons
duplicate DOM-flash logic and inline setQuery calls; extract a descriptive
helper like handleExampleChip(query: string) (placed in MainContent component
before return) that calls setQuery(query), finds the input via
document.getElementById('query-input'), adds the 'flash' class and schedules its
removal with setTimeout, then replace each inline onClick with onClick={() =>
handleExampleChip('...')}; apply the same change to all example-chip buttons
(e.g., the ones setting 'up', 'rate(http_requests_total[5m])',
'process_cpu_seconds_total', 'source=logs-* | head 10', and 'source=logs-* |
where severity="ERROR"') so the DOM-flash behavior is centralized and the code
is more maintainable.
In `@experimental/ag-ui/front-end/src/styles/layout.css`:
- Around line 211-214: Duplicate keyframe definitions (statusPulse) exist across
stylesheets; extract shared animations (statusPulse, fadeIn, pulse,
thinking-pulse, editorFlash, spin, pageFadeIn) into a single animations/base
stylesheet and import it from App.css instead of defining them in multiple files
(e.g., remove from layout.css and sidebar.css). Rename keyframe names to
kebab-case (status-pulse, page-fade-in, etc.) and update all references across
CSS/JS files to the new names to satisfy stylelint kebab-case rules, ensuring
only the new central animations file contains the canonical definitions.
- Around line 4-37: Update the shell height rules for .app and
.observability-platform to use the dynamic viewport unit to avoid mobile browser
chrome layout issues: replace or supplement height: 100vh with height: 100dvh
(and keep 100vh as a fallback if desired). Modify the CSS declarations inside
.app and .observability-platform so they prefer height: 100dvh while preserving
existing properties like overflow-y: auto, position, and background to maintain
current behavior.
- Around line 217-229: The collapsed chat wrapper still allows the fixed-width
child (.chat-assistant) to force overflow during the width transition; update
.chat-panel-wrapper (and its .collapsed state) to animate/max-clamp the
container rather than relying on the child to shrink — for example change the
animation to use max-width (set an initial max-width matching the assistant,
e.g. 420px) and keep overflow:hidden, or make the wrapper a fixed flex item
(flex: 0 0 auto) with an explicit width and animate that width to 0 in the
.chat-panel-wrapper.collapsed rule; ensure .chat-assistant’s fixed width is
matched by the wrapper’s max-width/width so the container clips the child during
the 0.3s transition.
In `@experimental/ag-ui/front-end/src/styles/query.css`:
- Around line 205-210: The global "legacy compat" CSS rules (.query-label-row,
.query-hint-inline, .query-hint, .execute-button) are too generic and may hide
unrelated elements; scope these selectors under a specific ancestor (e.g.,
prefix with a container like .query-section or .legacy-query) so only the old
query UI is affected, and replace the global selectors in the rule for
.query-input-container and .execute-button accordingly; also add a TODO comment
near the selectors noting this is temporary, include the reason (old markup
compatibility) and a removal timeline or issue/PR reference tied to when the old
markup is removed, and keep .run-button untouched as the replacement.
In `@experimental/ag-ui/front-end/src/styles/sidebar.css`:
- Around line 99-124: Replace hardcoded hex color values used in the sidebar
rules (.nav-item, .nav-item:hover:not(.disabled),
.nav-item:active:not(.disabled), .nav-item.active) with the design tokens (e.g.,
use var(--text-muted) instead of `#555555`, var(--bg-inset) instead of `#1A1A1A`,
var(--accent-indigo) or var(--accent-purple) instead of `#6366F1`, and
var(--text-primary) instead of `#E5E5E5`); apply the same replacements in
query.css and ChatAssistant.css for each listed hex (`#1A1A2E`, `#252547`, `#2A2A2A`,
`#6366F1`, `#E5E5E5`, `#555555`, `#22C55E`, `#FCA5A5`) and if no existing token matches
the semantic intent, add a new CSS custom property (token) with a clear name and
use that token in the corresponding selectors (e.g., .nav-item,
.nav-item.active, ChatAssistant styles) so theming updates propagate
consistently.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 5a459a47-4758-4563-9604-5bc23df3cda5
⛔ Files ignored due to path filters (1)
experimental/ag-ui/front-end/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
experimental/ag-ui/front-end/public/index.htmlexperimental/ag-ui/front-end/src/App.cssexperimental/ag-ui/front-end/src/App.tsxexperimental/ag-ui/front-end/src/components/ChatAssistant.cssexperimental/ag-ui/front-end/src/components/ChatAssistant.tsxexperimental/ag-ui/front-end/src/components/GraphVisualization.cssexperimental/ag-ui/front-end/src/components/LogsVisualization.cssexperimental/ag-ui/front-end/src/components/MainContent.tsxexperimental/ag-ui/front-end/src/index.cssexperimental/ag-ui/front-end/src/styles/explorer.cssexperimental/ag-ui/front-end/src/styles/layout.cssexperimental/ag-ui/front-end/src/styles/modal.cssexperimental/ag-ui/front-end/src/styles/query.cssexperimental/ag-ui/front-end/src/styles/results.cssexperimental/ag-ui/front-end/src/styles/sidebar.css
…upporting styles Signed-off-by: devbyaryanvala <aryanvala.edu@gmail.com>
devbyaryanvala
force-pushed
the
feature/ui-improvement
branch
from
April 22, 2026 16:10
759068f to
cb84b14
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
experimental/ag-ui/front-end/src/App.tsx (1)
239-248:⚠️ Potential issue | 🟡 MinorAdd an explicit accessible name to the chat opener.
When collapsed, this is an icon-only button and currently relies on
title; addaria-labeland hide the decorative SVG from assistive tech.Proposed fix
{chatCollapsed && ( <button + type="button" className="chat-open-btn" onClick={() => setChatCollapsed(false)} title="Open HolmesGPT Chat" + aria-label="Open HolmesGPT Chat" > - <svg viewBox="0 0 24 24" width="18" height="18" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"> + <svg aria-hidden="true" focusable="false" viewBox="0 0 24 24" width="18" height="18" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"> <path d="M21 15a2 2 0 0 1-2 2H7l-4 4V5a2 2 0 0 1 2-2h14a2 2 0 0 1 2 2z" /> </svg> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/App.tsx` around lines 239 - 248, The chat opener button rendered when chatCollapsed (the button with className "chat-open-btn" that calls setChatCollapsed(false)) is icon-only and only uses title; add an explicit accessible name by adding aria-label (e.g., aria-label="Open HolmesGPT Chat") to the button and mark the decorative SVG as hidden from assistive technology (aria-hidden="true" on the <svg>) so screen readers get the button label instead of reading the SVG.
🧹 Nitpick comments (1)
experimental/ag-ui/front-end/public/fonts/fonts.css (1)
1-56: Add italic font variants for Inter and Fira Code.Italic styling is used in 7 places across the frontend (explorer.css, LogsVisualization.css, GraphVisualization.css, ChatAssistant.css), but no italic font files are currently loaded. Browsers will synthesize italics from the regular fonts, which typically produces inferior rendering compared to true italic typefaces. Add italic variants (font-style: italic) for the font weights already declared to improve visual quality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/public/fonts/fonts.css` around lines 1 - 56, The CSS currently only declares normal styles for the Inter and Fira Code `@font-face` blocks; add matching italic `@font-face` entries for each declared weight so browsers load real italics instead of synthesizing them. For font-family 'Inter' add font-style: italic blocks for weights 400, 500, 600, 700, 800 with font-display: swap and src pointing to the corresponding local italic files (e.g., inter-400-italic.woff2, inter-500-italic.woff2, etc.), and for 'Fira Code' add italic `@font-face` entries for weights 400 and 500 with src pointing to fira-code-400-italic.woff2 and fira-code-500-italic.woff2; keep font-family names and weights identical to the existing normal declarations so CSS font-weight/font-style matching works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/ag-ui/front-end/src/index.css`:
- Around line 149-159: The .visually-hidden helper uses the deprecated clip
property; replace the clip declaration inside the .visually-hidden rule with a
modern equivalent by removing clip: rect(0, 0, 0, 0); and adding clip-path:
inset(50%); (optionally include -webkit-clip-path: inset(50%) for broader
support) to preserve the same screen-reader-only hiding behavior.
- Around line 162-190: Rename the `@keyframes` identifier fadeIn to fade-in in
index.css and update all animation references that use animation: fadeIn to
animation: fade-in; specifically update the five callers in results.css (two
references), modal.css (one reference) and ChatAssistant.css (two references).
While editing, verify whether unused keyframes fadeInScale and glowPulse are
actually referenced elsewhere and either keep them if used or remove them if
not. Ensure spelling and hyphenation match exactly (fade-in) across all CSS
files to avoid broken animations.
- Around line 47-48: The font-family CSS variables (--font-primary and
--font-mono) use unquoted capitalized font names that trigger Stylelint's
value-keyword-case rule; update the declarations for --font-primary and
--font-mono in index.css to wrap the capitalized family names (e.g.,
BlinkMacSystemFont, Roboto, Oxygen, Ubuntu and any other unquoted multi-word or
capitalized names) in quotes so they are treated as string literals and exempt
from the case rule while preserving the same font stack.
In `@experimental/ag-ui/front-end/src/styles/explorer.css`:
- Around line 20-28: Replace the interactive div with a semantic button in
MainContent.tsx (the element rendering with className "explorer-title" and
onClick) and update the CSS .explorer-title to reset native button styles (e.g.,
background: none; border: none; padding: 0; width: 100%; text-align: left;)
while preserving hover styling and border-radius; also ensure the JSX adds
proper accessibility attributes such as aria-expanded (if it toggles visibility)
and removes any manual key handlers so keyboard users can focus/activate the
control by default.
In `@experimental/ag-ui/front-end/src/styles/query.css`:
- Around line 83-90: In .editor-action-btn svg change stroke: currentColor to
stroke: currentcolor (lowercase) to satisfy Stylelint, and rename the keyframes
identifier from the camelCase name (e.g., Flash or flash) to a kebab-case name
(e.g., flash-animation) and update every animation/animation-name reference that
targets that keyframe (including the occurrences around lines 126-130) to use
the new kebab-case identifier so selectors like .editor-action-btn svg and any
.some-class { animation: ... } point to the renamed keyframe.
---
Duplicate comments:
In `@experimental/ag-ui/front-end/src/App.tsx`:
- Around line 239-248: The chat opener button rendered when chatCollapsed (the
button with className "chat-open-btn" that calls setChatCollapsed(false)) is
icon-only and only uses title; add an explicit accessible name by adding
aria-label (e.g., aria-label="Open HolmesGPT Chat") to the button and mark the
decorative SVG as hidden from assistive technology (aria-hidden="true" on the
<svg>) so screen readers get the button label instead of reading the SVG.
---
Nitpick comments:
In `@experimental/ag-ui/front-end/public/fonts/fonts.css`:
- Around line 1-56: The CSS currently only declares normal styles for the Inter
and Fira Code `@font-face` blocks; add matching italic `@font-face` entries for each
declared weight so browsers load real italics instead of synthesizing them. For
font-family 'Inter' add font-style: italic blocks for weights 400, 500, 600,
700, 800 with font-display: swap and src pointing to the corresponding local
italic files (e.g., inter-400-italic.woff2, inter-500-italic.woff2, etc.), and
for 'Fira Code' add italic `@font-face` entries for weights 400 and 500 with src
pointing to fira-code-400-italic.woff2 and fira-code-500-italic.woff2; keep
font-family names and weights identical to the existing normal declarations so
CSS font-weight/font-style matching works.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: e6188c97-27ff-42fa-82b9-d1ab54f0dff5
⛔ Files ignored due to path filters (7)
experimental/ag-ui/front-end/public/fonts/fira-code/fira-code-400.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/fira-code/fira-code-500.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/inter/inter-400.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/inter/inter-500.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/inter/inter-600.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/inter/inter-700.woff2is excluded by!**/*.woff2experimental/ag-ui/front-end/public/fonts/inter/inter-800.woff2is excluded by!**/*.woff2
📒 Files selected for processing (11)
experimental/ag-ui/front-end/public/fonts/fonts.cssexperimental/ag-ui/front-end/public/index.htmlexperimental/ag-ui/front-end/src/App.cssexperimental/ag-ui/front-end/src/App.tsxexperimental/ag-ui/front-end/src/components/ChatAssistant.cssexperimental/ag-ui/front-end/src/components/ChatAssistant.tsxexperimental/ag-ui/front-end/src/components/LogsVisualization.cssexperimental/ag-ui/front-end/src/index.cssexperimental/ag-ui/front-end/src/styles/explorer.cssexperimental/ag-ui/front-end/src/styles/query.cssexperimental/ag-ui/front-end/src/styles/results.css
✅ Files skipped from review due to trivial changes (2)
- experimental/ag-ui/front-end/public/index.html
- experimental/ag-ui/front-end/src/styles/results.css
🚧 Files skipped from review as they are similar to previous changes (1)
- experimental/ag-ui/front-end/src/components/ChatAssistant.tsx
…yling components Signed-off-by: devbyaryanvala <aryanvala.edu@gmail.com>
Contributor
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 (2)
experimental/ag-ui/front-end/src/components/MainContent.tsx (2)
950-962:⚠️ Potential issue | 🟠 MajorInvalid HTML:
<button>wraps block-level<h4>/<p>elements.Per the HTML spec, the content model of
<button>is phrasing content only — nesting<h4>and<p>inside it is invalid and triggers React's DOM-nesting warning in the console. It also produces an awkward AT experience (a heading announced inside a button role). The same pattern is repeated for the OpenSearch explorer at lines 1058-1070.Prefer a phrasing-only button (e.g.,
<span>s) and keep the heading semantics outside, or render the toggle as a button adjacent to the heading.♻️ Suggested restructuring
- <div className="explorer-header"> - <button type="button" className="explorer-title" onClick={() => setShowExplorer(!showExplorer)} aria-expanded={showExplorer}> - <h4> - Prometheus Series Explorer - {availableMetrics.length > 0 && ( - <span className="series-count-pill">({availableMetrics.length})</span> - )} - <span className="toggle-icon">{showExplorer ? '▼' : '▶'}</span> - </h4> - <p>Browse series, labels, and values to build your query</p> - </button> - </div> + <div className="explorer-header"> + <button + type="button" + className="explorer-title" + onClick={() => setShowExplorer(!showExplorer)} + aria-expanded={showExplorer} + aria-controls="prometheus-explorer-boxes" + > + <span className="explorer-title-text"> + Prometheus Series Explorer + {availableMetrics.length > 0 && ( + <span className="series-count-pill">({availableMetrics.length})</span> + )} + <span className="toggle-icon" aria-hidden="true">{showExplorer ? '▼' : '▶'}</span> + </span> + <span className="explorer-title-subtitle">Browse series, labels, and values to build your query</span> + </button> + </div>Apply the same fix to the OpenSearch explorer header at 1058-1070.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines 950 - 962, The button in the Prometheus explorer header is wrapping block-level elements (h4 and p) which is invalid; update MainContent.tsx to keep the heading semantics outside the <button> by moving the <h4> and <p> out of the button and rendering a phrase-only toggle button (e.g., a <button> that contains only inline elements or text like the toggle-icon and series-count) that calls setShowExplorer(!showExplorer); apply the same change to the OpenSearch explorer header (the analogous header that uses showExplorer/setShowExplorer and availableMetrics) so headings remain semantic and the toggle is a phrasing-only button.
909-917:⚠️ Potential issue | 🟡 MinorIcon-only retry button needs an accessible name.
↺is rendered as visible text content, but screen readers will announce it as a Unicode symbol ("clockwise gapped circle arrow" or nothing meaningful depending on AT).titleis not a reliable accessible name — it's inconsistently surfaced and unavailable on touch. Addaria-label="Retry connection"(and apply the same fix at line 936-942 for the OpenSearch retry button).♿ Proposed fix
<button + type="button" className="retry-connection-btn" onClick={() => checkPrometheusConnection(true)} + aria-label="Retry connection" title="Retry connection" > ↺ </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines 909 - 917, The retry icon-only button lacks an accessible name—add aria-label="Retry connection" to the Prometheus retry button that calls checkPrometheusConnection(true) in the MainContent component so screen readers get a meaningful name; apply the same change to the OpenSearch retry button (the button invoking checkOpenSearchConnection or similar near the other retry block) to ensure both icon-only retry buttons have aria-labels.
🧹 Nitpick comments (3)
experimental/ag-ui/front-end/src/styles/sidebar.css (2)
50-68: Add focus states for keyboard navigation accessibility.The collapse button and navigation items lack visible focus indicators for keyboard users. Adding
:focus-visiblestyles would improve accessibility without affecting mouse interaction.♿ Proposed accessibility enhancement
.sidebar-collapse-btn:hover { background: rgba(255, 255, 255, 0.08); color: var(--text-primary); } + +.sidebar-collapse-btn:focus-visible { + outline: 2px solid var(--accent-blue); + outline-offset: 2px; +}.nav-item:active:not(.disabled) { transform: scale(0.98); } + +.nav-item:focus-visible { + outline: 2px solid var(--accent-blue); + outline-offset: -2px; +}Also applies to: 89-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/sidebar.css` around lines 50 - 68, Add keyboard focus-visible styles for the collapse button and sidebar navigation items: update .sidebar-collapse-btn to include a :focus-visible rule (e.g., outline or subtle box-shadow and outline-offset) that provides a clear visible ring without changing hover styles, and add matching :focus-visible rules for the sidebar navigation item selectors used in this file (e.g., .sidebar-nav-item and .nav-link or the actual nav item classes present between lines 89-117) so keyboard users get a distinct focus indicator while mouse interactions remain unchanged.
99-99: Replace hard-coded colors with CSS custom properties for consistency.The navigation items use hard-coded color values (
#555555,#1A1A1A,#6366F1,#E5E5E5) instead of CSS custom properties, which is inconsistent with the tokenized dark theme approach used throughout the rest of the file. This reduces maintainability and makes theme adjustments more difficult.♻️ Proposed refactor to use design tokens
Define the missing tokens (likely in your main CSS variables file):
--nav-item-color: `#555555`; --nav-item-active-bg: `#1A1A1A`; --nav-item-active-border: `#6366F1`; --nav-item-active-color: `#E5E5E5`; --nav-item-disabled-color: `#555555`;Then update this file:
.nav-item { display: flex; align-items: center; gap: 0.85rem; width: 100%; padding: 0.75rem 1rem; background: transparent; border: none; border-left: 2px solid transparent; border-radius: 0; - color: `#555555`; + color: var(--nav-item-color); font-size: 0.9rem; font-weight: 500;.nav-item.active { - background-color: `#1A1A1A`; - border-left: 2px solid `#6366F1`; - color: `#E5E5E5`; + background-color: var(--nav-item-active-bg); + border-left: 2px solid var(--nav-item-active-border); + color: var(--nav-item-active-color); font-weight: 500; }.nav-item.disabled { - color: `#555555`; + color: var(--nav-item-disabled-color); cursor: default; pointer-events: none; } .nav-item.disabled:hover { background: transparent; - color: `#555555`; + color: var(--nav-item-disabled-color); transform: none; }Also applies to: 120-122, 128-128, 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/styles/sidebar.css` at line 99, Replace the hard-coded color literals used for navigation items with CSS custom properties: define the tokens (--nav-item-color, --nav-item-active-bg, --nav-item-active-border, --nav-item-active-color, --nav-item-disabled-color) in your global variables file, then update the navigation-related rules in this stylesheet that currently use `#555555`, `#1A1A1A`, `#6366F1`, and `#E5E5E5` to use those variables (e.g., swap color: `#555555` to color: var(--nav-item-color), active background to var(--nav-item-active-bg), active border to var(--nav-item-active-border), active color to var(--nav-item-active-color), and disabled color to var(--nav-item-disabled-color)); ensure you update all occurrences noted in the review (the navigation item selectors and the lines referenced around 99, 120–122, 128, and 135) so theme tokens are used consistently.experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
1170-1189: Extract the chip handler and use a ref instead ofgetElementById.The five example chips each repeat the same inline logic (
setQuery(...),getElementById('query-input'), toggleflashclass,setTimeoutcleanup). Two concerns:
- DRY: the same expression is duplicated 5×, which makes it error-prone to change (e.g., the flash duration or class name).
- React antipattern: reaching into the DOM via
document.getElementByIdbypasses React's declarative model. Use auseRefon the textarea (the id is only referenced here and for the<label>-less textarea) so the behaviour still works if the id is ever renamed or the component is rendered multiple times on the same page.♻️ Proposed refactor
const [query, setQuery] = useState(initialQuery); const [isExecuting, setIsExecuting] = useState(false); + const queryInputRef = React.useRef<HTMLTextAreaElement | null>(null); + + // Set the query and briefly flash the textarea to draw attention to it + const selectExampleQuery = React.useCallback((exampleQuery: string) => { + setQuery(exampleQuery); + const el = queryInputRef.current; + if (!el) return; + el.classList.add('flash'); + setTimeout(() => el.classList.remove('flash'), 500); + }, []);<textarea id="query-input" + ref={queryInputRef} className="query-input"- <button className="example-chip" onClick={() => { setQuery('up'); const el = document.getElementById('query-input'); if(el) el.classList.add('flash'); setTimeout(() => el?.classList.remove('flash'), 500); }}>up<span className="chip-arrow">→</span></button> - <button className="example-chip" onClick={() => { setQuery('rate(http_requests_total[5m])'); ... }}>rate(http_requests_total[5m])<span className="chip-arrow">→</span></button> - ... + {['up', 'rate(http_requests_total[5m])', 'process_cpu_seconds_total'].map((ex) => ( + <button key={ex} type="button" className="example-chip" onClick={() => selectExampleQuery(ex)}> + {ex}<span className="chip-arrow">→</span> + </button> + ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines 1170 - 1189, Extract the repeated inline logic into a single handler (e.g., handleExampleChipClick) and replace document.getElementById('query-input') with a React ref attached to the query textarea (e.g., const queryInputRef = useRef<HTMLTextAreaElement | null>(null)); have handleExampleChipClick call setQuery(value), add the 'flash' class to queryInputRef.current (if present), and remove it after a timeout (store and clear the timer to avoid leaks). Update all example-chip onClick handlers to call handleExampleChipClick with the chip string; remove direct uses of 'query-input' id in the buttons and leave the id only if still needed for other code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css`:
- Around line 217-223: The .message bubble can grow past the flex container due
to intrinsic width of long unbroken content—add min-width: 0 to the .message
rule and constrain inline code/pre inside that bubble by adding rules like
.message pre, .message code { max-width: 100%; overflow: auto; white-space: pre;
word-break: break-word; } so long code blocks are capped to the bubble width and
scroll instead of expanding the panel; update both occurrences of the .message
block/styles.
- Around line 405-445: The input's right-side inline elements (.input-hint and
.char-count) overlap typed text because .chat-input input only has 0.85rem right
padding; update the styling for .chat-input input (the input selector) to
increase padding-right to accommodate those elements (e.g. set padding-right to
a value large enough for .input-hint and .char-count combined) and/or adjust
.input-hint/.char-count widths/positions so the input text never flows under
them.
- Around line 519-528: The mobile media query for .chat-assistant needs to reset
the base min-width to avoid horizontal overflow on narrow viewports; update the
`@media` (max-width: 768px) block that targets .chat-assistant to set min-width: 0
(or min-width: unset) alongside width: 100vw and max-width: 100vw so the
component can shrink below the base 300px; ensure you modify the .chat-assistant
rule within ChatAssistant.css to include this min-width reset.
In `@experimental/ag-ui/front-end/src/styles/sidebar.css`:
- Line 150: Replace the CSS keyword casing for `currentColor` to `currentcolor`
in sidebar.css where the `stroke` property uses it (specifically the occurrences
around the `stroke: currentColor;` instances at the locations noted — also check
any similar uses such as `fill: currentColor;`) so that the keyword follows
standard lowercase CSS conventions; update both occurrences (the one at the
stroke declaration on line ~150 and the other at ~237) to `currentcolor`.
- Line 175: Rename the CSS keyframe from statusPulse to kebab-case status-pulse
and update all references: change the animation property values (e.g.,
animation: statusPulse 1.5s infinite) to use status-pulse, rename the `@keyframes`
block (currently `@keyframes` statusPulse) to `@keyframes` status-pulse, and update
any other selectors or JS/CSS references that use statusPulse so they all match
the new status-pulse identifier.
---
Outside diff comments:
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx`:
- Around line 950-962: The button in the Prometheus explorer header is wrapping
block-level elements (h4 and p) which is invalid; update MainContent.tsx to keep
the heading semantics outside the <button> by moving the <h4> and <p> out of the
button and rendering a phrase-only toggle button (e.g., a <button> that contains
only inline elements or text like the toggle-icon and series-count) that calls
setShowExplorer(!showExplorer); apply the same change to the OpenSearch explorer
header (the analogous header that uses showExplorer/setShowExplorer and
availableMetrics) so headings remain semantic and the toggle is a phrasing-only
button.
- Around line 909-917: The retry icon-only button lacks an accessible name—add
aria-label="Retry connection" to the Prometheus retry button that calls
checkPrometheusConnection(true) in the MainContent component so screen readers
get a meaningful name; apply the same change to the OpenSearch retry button (the
button invoking checkOpenSearchConnection or similar near the other retry block)
to ensure both icon-only retry buttons have aria-labels.
---
Nitpick comments:
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx`:
- Around line 1170-1189: Extract the repeated inline logic into a single handler
(e.g., handleExampleChipClick) and replace
document.getElementById('query-input') with a React ref attached to the query
textarea (e.g., const queryInputRef = useRef<HTMLTextAreaElement | null>(null));
have handleExampleChipClick call setQuery(value), add the 'flash' class to
queryInputRef.current (if present), and remove it after a timeout (store and
clear the timer to avoid leaks). Update all example-chip onClick handlers to
call handleExampleChipClick with the chip string; remove direct uses of
'query-input' id in the buttons and leave the id only if still needed for other
code.
In `@experimental/ag-ui/front-end/src/styles/sidebar.css`:
- Around line 50-68: Add keyboard focus-visible styles for the collapse button
and sidebar navigation items: update .sidebar-collapse-btn to include a
:focus-visible rule (e.g., outline or subtle box-shadow and outline-offset) that
provides a clear visible ring without changing hover styles, and add matching
:focus-visible rules for the sidebar navigation item selectors used in this file
(e.g., .sidebar-nav-item and .nav-link or the actual nav item classes present
between lines 89-117) so keyboard users get a distinct focus indicator while
mouse interactions remain unchanged.
- Line 99: Replace the hard-coded color literals used for navigation items with
CSS custom properties: define the tokens (--nav-item-color,
--nav-item-active-bg, --nav-item-active-border, --nav-item-active-color,
--nav-item-disabled-color) in your global variables file, then update the
navigation-related rules in this stylesheet that currently use `#555555`, `#1A1A1A`,
`#6366F1`, and `#E5E5E5` to use those variables (e.g., swap color: `#555555` to color:
var(--nav-item-color), active background to var(--nav-item-active-bg), active
border to var(--nav-item-active-border), active color to
var(--nav-item-active-color), and disabled color to
var(--nav-item-disabled-color)); ensure you update all occurrences noted in the
review (the navigation item selectors and the lines referenced around 99,
120–122, 128, and 135) so theme tokens are used consistently.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 650eff25-4b71-427e-8a40-b519477e3d0f
📒 Files selected for processing (8)
experimental/ag-ui/front-end/src/components/ChatAssistant.cssexperimental/ag-ui/front-end/src/components/MainContent.tsxexperimental/ag-ui/front-end/src/index.cssexperimental/ag-ui/front-end/src/styles/explorer.cssexperimental/ag-ui/front-end/src/styles/modal.cssexperimental/ag-ui/front-end/src/styles/query.cssexperimental/ag-ui/front-end/src/styles/results.cssexperimental/ag-ui/front-end/src/styles/sidebar.css
✅ Files skipped from review due to trivial changes (4)
- experimental/ag-ui/front-end/src/styles/modal.css
- experimental/ag-ui/front-end/src/styles/results.css
- experimental/ag-ui/front-end/src/styles/query.css
- experimental/ag-ui/front-end/src/styles/explorer.css
🚧 Files skipped from review as they are similar to previous changes (1)
- experimental/ag-ui/front-end/src/index.css
…ent chat and tool execution Signed-off-by: devbyaryanvala <aryanvala.edu@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
experimental/ag-ui/front-end/src/components/ChatAssistant.css (1)
107-203: Hardcoded colors bypass the design-token system.The PR explicitly moves styling onto design tokens, but several new rules hardcode hex values instead of using variables:
#555555(lines 107, 198),#22C55E(line 116),#2A2A2A(line 193), and#FCA5A5(line 313). The neighboring.disconnected/.connecting/.errorvariants correctly usevar(--status-error),var(--status-checking), so theconnecteddefault and divider/text colors are the odd ones out. This breaks theme consistency and will not respond if tokens are retuned.Consider mapping these to the existing token set (e.g.
var(--status-ok)for the green indicator,var(--text-faint)/var(--text-muted)for the subdued text, andvar(--border-muted)for the divider lines), or introduce tokens if none exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css` around lines 107 - 203, Several rules hardcode colors instead of using design tokens: replace the hex colors used in .chat-header .connection-indicator (currently `#22C55E`) with var(--status-ok), replace the text color usages of `#555555` (in the header and .date-divider span) with var(--text-faint) or var(--text-muted), and replace the divider color `#2A2A2A` (date-divider::before/::after) with var(--border-muted); also swap the referenced `#FCA5A5` occurrence to an appropriate token (e.g. var(--status-error) or a new token) if present elsewhere. Update the CSS selectors .chat-header .connection-indicator, .chat-header .connection-indicator.disconnected/.connecting/.error, .date-divider::before, .date-divider::after, and .date-divider span to use these tokens, and if a needed token doesn’t exist, add a new design token (e.g. --status-ok) instead of hardcoding hex values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css`:
- Around line 226-232: Update the CSS rule that targets `.message pre, .message
code` so it no longer uses the deprecated `word-break: break-word` and so it
only applies block-level behavior to preformatted blocks: change the selector to
`.message pre, .message pre code` and replace `word-break: break-word` with
`overflow-wrap: anywhere` (or `word-break: break-all` if you prefer). This
ensures inline `<code>` (handled by the existing `.message-text code` rule)
keeps normal wrapping and avoids inline scrollbars while preserving correct
overflow behavior for pre blocks.
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx`:
- Around line 78-80: The flash timer stored in flashTimerRef can outlive the
component and attempt to mutate a stale queryInputRef; update the cleanup logic
for the effect(s) that set the timeout (and the component unmount cleanup) to
clearTimeout on flashTimerRef.current and set flashTimerRef.current = null, and
ensure any existing timeout-clear paths also null out the ref to avoid
double-calls; also replace the redundant handler comment near
queryInputRef/flashTimerRef with a concise explanation of why the timer is
centralized (to coordinate example-chip flash effects) rather than restating the
function name.
- Around line 1139-1182: The editor label span (class "editor-lang-label") isn't
associated with the textarea (id "query-input") and icon/spinner-only buttons
(class "editor-action-btn" and "run-button") lack accessible names; update the
DOM so the span becomes an accessible label for the textarea (use
aria-labelledby on the textarea pointing to the span or replace span with a
<label> associated with id "query-input"), and add explicit accessible names to
the action buttons (e.g., aria-labels like "Copy query", "Clear query", and for
the run-button show text when not executing and provide aria-label or
visually-hidden text when showing the spinner) so screen readers and keyboard
users can identify controls; touch up related handlers/refs (queryInputRef,
handleExecuteQuery) only if needed to preserve functionality.
---
Nitpick comments:
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css`:
- Around line 107-203: Several rules hardcode colors instead of using design
tokens: replace the hex colors used in .chat-header .connection-indicator
(currently `#22C55E`) with var(--status-ok), replace the text color usages of
`#555555` (in the header and .date-divider span) with var(--text-faint) or
var(--text-muted), and replace the divider color `#2A2A2A`
(date-divider::before/::after) with var(--border-muted); also swap the
referenced `#FCA5A5` occurrence to an appropriate token (e.g. var(--status-error)
or a new token) if present elsewhere. Update the CSS selectors .chat-header
.connection-indicator, .chat-header
.connection-indicator.disconnected/.connecting/.error, .date-divider::before,
.date-divider::after, and .date-divider span to use these tokens, and if a
needed token doesn’t exist, add a new design token (e.g. --status-ok) instead of
hardcoding hex values.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 34979b35-18c9-423a-af80-80ed687c4d78
📒 Files selected for processing (4)
experimental/ag-ui/front-end/src/components/ChatAssistant.cssexperimental/ag-ui/front-end/src/components/MainContent.tsxexperimental/ag-ui/front-end/src/styles/explorer.cssexperimental/ag-ui/front-end/src/styles/sidebar.css
✅ Files skipped from review due to trivial changes (2)
- experimental/ag-ui/front-end/src/styles/sidebar.css
- experimental/ag-ui/front-end/src/styles/explorer.css
Comment on lines
+226
to
232
| .message pre, | ||
| .message code { | ||
| max-width: 100%; | ||
| overflow: auto; | ||
| white-space: pre; | ||
| word-break: break-word; | ||
| } |
Contributor
There was a problem hiding this comment.
Deprecated word-break: break-word and inline/block selector conflict.
Two problems in this rule:
- Stylelint flags
word-break: break-wordas deprecated (line 231). Preferoverflow-wrap: anywhereorword-break: break-all. .message codealso matches inline<code>(rendered by ReactMarkdown for inline backticks). Applyingwhite-space: pre,overflow: auto, andmax-width: 100%to inline code disables normal wrapping inside paragraphs and can produce awkward inline scrollbars. This block-level treatment should be scoped topre(andpre code) only — the inline.message-text coderule at line 268 already handles the inline case.
🛠️ Proposed fix
-.message pre,
-.message code {
- max-width: 100%;
- overflow: auto;
- white-space: pre;
- word-break: break-word;
-}
+.message pre {
+ max-width: 100%;
+ overflow: auto;
+ white-space: pre;
+ overflow-wrap: anywhere;
+}🧰 Tools
🪛 Stylelint (17.7.0)
[error] 231-231: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/ag-ui/front-end/src/components/ChatAssistant.css` around lines
226 - 232, Update the CSS rule that targets `.message pre, .message code` so it
no longer uses the deprecated `word-break: break-word` and so it only applies
block-level behavior to preformatted blocks: change the selector to `.message
pre, .message pre code` and replace `word-break: break-word` with
`overflow-wrap: anywhere` (or `word-break: break-all` if you prefer). This
ensures inline `<code>` (handled by the existing `.message-text code` rule)
keeps normal wrapping and avoids inline scrollbars while preserving correct
overflow behavior for pre blocks.
Comment on lines
+78
to
+80
| // Ref for query textarea (used by example chip flash effect) | ||
| const queryInputRef = useRef<HTMLTextAreaElement | null>(null); | ||
| const flashTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); |
Contributor
There was a problem hiding this comment.
Clean up the flash timer on unmount.
flashTimerRef is newly introduced but not cleared in the existing timeout cleanup, so a pending flash timeout can outlive the component/page remount and mutate a stale textarea ref. The handler comment also mostly restates the function name; either remove it or explain why the timer is centralized. As per coding guidelines, “Write clear, concise comments that explain 'why' rather than 'what'”.
Proposed fix
- // Shared handler for example-chip clicks
+ // Centralize flash timing so repeated chip clicks clear any pending animation cleanup.
const handleExampleChipClick = useCallback((value: string) => {
setQuery(value);
if (flashTimerRef.current) {
clearTimeout(flashTimerRef.current);
}
@@
React.useEffect(() => {
return () => {
+ if (flashTimerRef.current) {
+ clearTimeout(flashTimerRef.current);
+ flashTimerRef.current = null;
+ }
if (prometheusRetryTimeoutRef.current) {
clearTimeout(prometheusRetryTimeoutRef.current);
}
if (opensearchRetryTimeoutRef.current) {
clearTimeout(opensearchRetryTimeoutRef.current);Also applies to: 794-807, 889-899
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines 78
- 80, The flash timer stored in flashTimerRef can outlive the component and
attempt to mutate a stale queryInputRef; update the cleanup logic for the
effect(s) that set the timeout (and the component unmount cleanup) to
clearTimeout on flashTimerRef.current and set flashTimerRef.current = null, and
ensure any existing timeout-clear paths also null out the ref to avoid
double-calls; also replace the redundant handler comment near
queryInputRef/flashTimerRef with a concise explanation of why the timer is
centralized (to coordinate example-chip flash effects) rather than restating the
function name.
Comment on lines
+1139
to
+1182
| <span className="editor-lang-label"> | ||
| {selectedPage === 'metrics' ? 'PromQL' : selectedPage === 'logs' ? 'PPL' : 'Query'} | ||
| </span> | ||
| <div className="editor-actions"> | ||
| <button | ||
| className="editor-action-btn" | ||
| title="Copy query" | ||
| onClick={() => { if (query.trim()) navigator.clipboard.writeText(query); }} | ||
| > | ||
| <svg viewBox="0 0 24 24"><rect x="9" y="9" width="13" height="13" rx="2" ry="2" /><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1" /></svg> | ||
| </button> | ||
| <button | ||
| className="editor-action-btn" | ||
| title="Clear query" | ||
| onClick={() => setQuery('')} | ||
| > | ||
| <svg viewBox="0 0 24 24"><line x1="18" y1="6" x2="6" y2="18" /><line x1="6" y1="6" x2="18" y2="18" /></svg> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <textarea | ||
| ref={queryInputRef} | ||
| id="query-input" | ||
| className="query-input" | ||
| value={query} | ||
| onChange={(e) => setQuery(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder={ | ||
| selectedPage === 'metrics' | ||
| ? 'Enter PromQL query (e.g., up, rate(http_requests_total[5m]))...' | ||
| : selectedPage === 'logs' | ||
| ? 'Enter PPL query (e.g., source=logs-* | head 10)...' | ||
| : 'Enter trace query...' | ||
| } | ||
| rows={3} | ||
| /> | ||
| </div> | ||
| <button | ||
| className="run-button" | ||
| onClick={handleExecuteQuery} | ||
| disabled={!query.trim() || isExecuting} | ||
| > | ||
| {isExecuting ? <span className="btn-spinner"></span> : 'Run'} | ||
| </button> |
Contributor
There was a problem hiding this comment.
Preserve accessible names for the query editor controls.
The visible editor label is not associated with the textarea, and the icon/spinner-only buttons rely on visual affordances. Add explicit labels so keyboard and screen-reader users can identify the editor actions consistently.
Proposed fix
- <span className="editor-lang-label">
+ <label className="editor-lang-label" htmlFor="query-input">
{selectedPage === 'metrics' ? 'PromQL' : selectedPage === 'logs' ? 'PPL' : 'Query'}
- </span>
+ </label>
<div className="editor-actions">
<button
+ type="button"
className="editor-action-btn"
title="Copy query"
+ aria-label="Copy query"
onClick={() => { if (query.trim()) navigator.clipboard.writeText(query); }}
>
- <svg viewBox="0 0 24 24"><rect x="9" y="9" width="13" height="13" rx="2" ry="2" /><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1" /></svg>
+ <svg viewBox="0 0 24 24" aria-hidden="true" focusable="false"><rect x="9" y="9" width="13" height="13" rx="2" ry="2" /><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1" /></svg>
</button>
<button
+ type="button"
className="editor-action-btn"
title="Clear query"
+ aria-label="Clear query"
onClick={() => setQuery('')}
>
- <svg viewBox="0 0 24 24"><line x1="18" y1="6" x2="6" y2="18" /><line x1="6" y1="6" x2="18" y2="18" /></svg>
+ <svg viewBox="0 0 24 24" aria-hidden="true" focusable="false"><line x1="18" y1="6" x2="6" y2="18" /><line x1="6" y1="6" x2="18" y2="18" /></svg>
</button>
</div>
@@
<button
+ type="button"
className="run-button"
onClick={handleExecuteQuery}
disabled={!query.trim() || isExecuting}
+ aria-label={isExecuting ? 'Running query' : 'Run query'}
+ aria-busy={isExecuting}
>
- {isExecuting ? <span className="btn-spinner"></span> : 'Run'}
+ {isExecuting ? <span className="btn-spinner" aria-hidden="true"></span> : 'Run'}
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span className="editor-lang-label"> | |
| {selectedPage === 'metrics' ? 'PromQL' : selectedPage === 'logs' ? 'PPL' : 'Query'} | |
| </span> | |
| <div className="editor-actions"> | |
| <button | |
| className="editor-action-btn" | |
| title="Copy query" | |
| onClick={() => { if (query.trim()) navigator.clipboard.writeText(query); }} | |
| > | |
| <svg viewBox="0 0 24 24"><rect x="9" y="9" width="13" height="13" rx="2" ry="2" /><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1" /></svg> | |
| </button> | |
| <button | |
| className="editor-action-btn" | |
| title="Clear query" | |
| onClick={() => setQuery('')} | |
| > | |
| <svg viewBox="0 0 24 24"><line x1="18" y1="6" x2="6" y2="18" /><line x1="6" y1="6" x2="18" y2="18" /></svg> | |
| </button> | |
| </div> | |
| </div> | |
| <textarea | |
| ref={queryInputRef} | |
| id="query-input" | |
| className="query-input" | |
| value={query} | |
| onChange={(e) => setQuery(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder={ | |
| selectedPage === 'metrics' | |
| ? 'Enter PromQL query (e.g., up, rate(http_requests_total[5m]))...' | |
| : selectedPage === 'logs' | |
| ? 'Enter PPL query (e.g., source=logs-* | head 10)...' | |
| : 'Enter trace query...' | |
| } | |
| rows={3} | |
| /> | |
| </div> | |
| <button | |
| className="run-button" | |
| onClick={handleExecuteQuery} | |
| disabled={!query.trim() || isExecuting} | |
| > | |
| {isExecuting ? <span className="btn-spinner"></span> : 'Run'} | |
| </button> | |
| <label className="editor-lang-label" htmlFor="query-input"> | |
| {selectedPage === 'metrics' ? 'PromQL' : selectedPage === 'logs' ? 'PPL' : 'Query'} | |
| </label> | |
| <div className="editor-actions"> | |
| <button | |
| type="button" | |
| className="editor-action-btn" | |
| title="Copy query" | |
| aria-label="Copy query" | |
| onClick={() => { if (query.trim()) navigator.clipboard.writeText(query); }} | |
| > | |
| <svg viewBox="0 0 24 24" aria-hidden="true" focusable="false"><rect x="9" y="9" width="13" height="13" rx="2" ry="2" /><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1" /></svg> | |
| </button> | |
| <button | |
| type="button" | |
| className="editor-action-btn" | |
| title="Clear query" | |
| aria-label="Clear query" | |
| onClick={() => setQuery('')} | |
| > | |
| <svg viewBox="0 0 24 24" aria-hidden="true" focusable="false"><line x1="18" y1="6" x2="6" y2="18" /><line x1="6" y1="6" x2="18" y2="18" /></svg> | |
| </button> | |
| </div> | |
| </div> | |
| <textarea | |
| ref={queryInputRef} | |
| id="query-input" | |
| className="query-input" | |
| value={query} | |
| onChange={(e) => setQuery(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder={ | |
| selectedPage === 'metrics' | |
| ? 'Enter PromQL query (e.g., up, rate(http_requests_total[5m]))...' | |
| : selectedPage === 'logs' | |
| ? 'Enter PPL query (e.g., source=logs-* | head 10)...' | |
| : 'Enter trace query...' | |
| } | |
| rows={3} | |
| /> | |
| </div> | |
| <button | |
| type="button" | |
| className="run-button" | |
| onClick={handleExecuteQuery} | |
| disabled={!query.trim() || isExecuting} | |
| aria-label={isExecuting ? 'Running query' : 'Run query'} | |
| aria-busy={isExecuting} | |
| > | |
| {isExecuting ? <span className="btn-spinner" aria-hidden="true"></span> : 'Run'} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/ag-ui/front-end/src/components/MainContent.tsx` around lines
1139 - 1182, The editor label span (class "editor-lang-label") isn't associated
with the textarea (id "query-input") and icon/spinner-only buttons (class
"editor-action-btn" and "run-button") lack accessible names; update the DOM so
the span becomes an accessible label for the textarea (use aria-labelledby on
the textarea pointing to the span or replace span with a <label> associated with
id "query-input"), and add explicit accessible names to the action buttons
(e.g., aria-labels like "Copy query", "Clear query", and for the run-button show
text when not executing and provide aria-label or visually-hidden text when
showing the spinner) so screen readers and keyboard users can identify controls;
touch up related handlers/refs (queryInputRef, handleExecuteQuery) only if
needed to preserve functionality.
|
+1 |
This branch has not been deployed
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.
♻️ Refactor: Improve Experimental UI Architecture (Non-Breaking)
Summary
This PR refactors the experimental UI to improve structure, maintainability, and scalability while preserving all existing behavior.
Motivation
The previous UI implementation worked functionally but had structural limitations:
As the experimental UI grows, these issues would slow down development and increase complexity.
What’s Changed
Architecture Improvements
Backward Compatibility
experimental/ag-ui/server-agui.py)Tests
make test-without-llm)Scope
Trade-offs
Why this should be merged
This refactor improves the foundation of the experimental UI without introducing breaking changes.
It enables cleaner future development while respecting existing system contracts and tests.
Related Issue
Issue Number: #1935
Screenshots can be found there.
Summary by CodeRabbit
New Features
Style