fix(a11y): resolve IBM Equal Access violations on sidebar, canvas, and note contrast - #14212
fix(a11y): resolve IBM Equal Access violations on sidebar, canvas, and note contrast#14212olayinkaadelakun wants to merge 2 commits into
Conversation
WalkthroughChangesNote contrast
Flow accessibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PageComponent
participant I18n
participant ReactFlow
PageComponent->>I18n: Translate flow.nodeAriaLabel with node name or type
I18n-->>PageComponent: Return localized aria label
PageComponent->>ReactFlow: Render transformed nodes
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx (1)
574-585: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse a realistic integration boundary for the accessibility tests.
Both tests mock core sidebar primitives, so they can pass without verifying the production accessibility tree.
src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx#L574-L585: query the real accessible role/name and reduce primitive mocks or add an integration test.src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarSegmentedNav.test.tsx#L360-L391: render real menu primitives when asserting list semantics and separator order.As per coding guidelines, frontend tests should avoid excessive mocks and verify meaningful behavior rather than acting as smoke tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx` around lines 574 - 585, Update sidebarDraggableComponent.test.tsx lines 574-585 to query the rendered accessible role and name through the real sidebar primitives, reducing or removing primitive mocks or adding an integration-level test. Update sidebarSegmentedNav.test.tsx lines 360-391 to render real menu primitives when asserting list semantics and separator order; preserve the intended accessibility assertions while avoiding smoke tests based solely on mocked structure.Source: Coding guidelines
src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx (1)
933-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for every new accessibility contract.
src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx#L933-L934: test the localized canvas label and per-node label transformation, including empty display names.src/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx#L733-L734: test the navigation landmark with its localized accessible name.As per coding guidelines, new frontend implementations should include corresponding tests that verify meaningful behavior rather than placeholders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx` around lines 933 - 934, Add regression tests for the accessibility contracts in PageComponent and flowSidebarComponent: verify PageComponent applies the localized canvas label and transforms each node’s label correctly, including nodes with empty display names; verify flowSidebarComponent renders its navigation landmark with the localized accessible name. Update the relevant component test suites at src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx lines 933-934 and src/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx lines 733-734, with no direct implementation change required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/CustomNodes/NoteNode/__tests__/note-node-contrast.test.tsx`:
- Around line 69-99: Extend the NoteNode contrast tests around the existing
lastNodeDescriptionProps assertions to verify placeholderClassName: require
text-foreground/70 and dark:!text-background for opaque presets, and require
that the transparent/no-background preset does not include the forced
placeholder color classes.
In
`@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx`:
- Around line 134-138: The draggable wrapper around the Add/Select controls
should not be exposed as a composite button. Update the wrapper and its keyboard
handling in sidebarDraggableComponent so the drag target is separate from
independently focusable child actions, or make the wrapper a guarded activation
target with aria-disabled; ensure Enter/Space cannot activate disabled controls
and do not trap focus with tabIndex={-1}.
In `@src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx`:
- Around line 124-126: Update the ariaLabel construction in PageComponent to
treat an empty display_name as missing by using a truthy fallback to
node.data?.type instead of nullish-only fallback. Preserve the existing
translation call and node name precedence for non-empty display names.
---
Nitpick comments:
In
`@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx`:
- Around line 574-585: Update sidebarDraggableComponent.test.tsx lines 574-585
to query the rendered accessible role and name through the real sidebar
primitives, reducing or removing primitive mocks or adding an integration-level
test. Update sidebarSegmentedNav.test.tsx lines 360-391 to render real menu
primitives when asserting list semantics and separator order; preserve the
intended accessibility assertions while avoiding smoke tests based solely on
mocked structure.
In `@src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx`:
- Around line 933-934: Add regression tests for the accessibility contracts in
PageComponent and flowSidebarComponent: verify PageComponent applies the
localized canvas label and transforms each node’s label correctly, including
nodes with empty display names; verify flowSidebarComponent renders its
navigation landmark with the localized accessible name. Update the relevant
component test suites at
src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx lines 933-934
and src/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx
lines 733-734, with no direct implementation change required.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ef1c808-5029-4d6f-b450-f1b4f78bb275
📒 Files selected for processing (15)
src/frontend/src/CustomNodes/NoteNode/__tests__/note-node-contrast.test.tsxsrc/frontend/src/CustomNodes/NoteNode/index.tsxsrc/frontend/src/locales/de.jsonsrc/frontend/src/locales/en.jsonsrc/frontend/src/locales/es.jsonsrc/frontend/src/locales/fr.jsonsrc/frontend/src/locales/ja.jsonsrc/frontend/src/locales/pt.jsonsrc/frontend/src/locales/zh-Hans.jsonsrc/frontend/src/pages/FlowPage/components/PageComponent/index.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarSegmentedNav.test.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarSegmentedNav.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx
💤 Files with no reviewable changes (1)
- src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarSegmentedNav.tsx
| expect(lastNodeDescriptionProps.inputClassName).toContain( | ||
| "text-foreground", | ||
| ); | ||
| expect(lastNodeDescriptionProps.mdClassName).toContain( | ||
| "text-foreground", | ||
| ); | ||
| // Dark-mode override must be preserved alongside the new light-mode fix. | ||
| expect(lastNodeDescriptionProps.inputClassName).toContain( | ||
| "dark:text-background", | ||
| ); | ||
| expect(lastNodeDescriptionProps.mdClassName).toContain( | ||
| "dark:!text-background", | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| it("leaves the transparent/no-background preset without a forced text color", () => { | ||
| const data = { | ||
| id: "note-1", | ||
| node: { | ||
| description: "Some note text", | ||
| template: { backgroundColor: "transparent" }, | ||
| }, | ||
| } as unknown as NoteDataType; | ||
|
|
||
| render(<NoteNode data={data} />); | ||
|
|
||
| expect(lastNodeDescriptionProps.inputClassName).not.toContain( | ||
| "text-foreground", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the changed placeholder contrast contract.
The test never asserts placeholderClassName, so a regression in the new text-foreground/70 dark:!text-background behavior at NoteNode/index.tsx Line 239 would pass unnoticed. Assert it for both opaque presets and the transparent preset.
Proposed test additions
expect(lastNodeDescriptionProps.mdClassName).toContain(
"dark:!text-background",
);
+ expect(lastNodeDescriptionProps.placeholderClassName).toContain(
+ "text-foreground/70",
+ );
+ expect(lastNodeDescriptionProps.placeholderClassName).toContain(
+ "dark:!text-background",
+ );
...
expect(lastNodeDescriptionProps.inputClassName).not.toContain(
"text-foreground",
);
+ expect(lastNodeDescriptionProps.placeholderClassName).not.toContain(
+ "text-foreground",
+ );As per coding guidelines, frontend tests must “verify the tests actually cover the new or changed behavior rather than acting as placeholders.”
📝 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.
| expect(lastNodeDescriptionProps.inputClassName).toContain( | |
| "text-foreground", | |
| ); | |
| expect(lastNodeDescriptionProps.mdClassName).toContain( | |
| "text-foreground", | |
| ); | |
| // Dark-mode override must be preserved alongside the new light-mode fix. | |
| expect(lastNodeDescriptionProps.inputClassName).toContain( | |
| "dark:text-background", | |
| ); | |
| expect(lastNodeDescriptionProps.mdClassName).toContain( | |
| "dark:!text-background", | |
| ); | |
| }); | |
| }); | |
| it("leaves the transparent/no-background preset without a forced text color", () => { | |
| const data = { | |
| id: "note-1", | |
| node: { | |
| description: "Some note text", | |
| template: { backgroundColor: "transparent" }, | |
| }, | |
| } as unknown as NoteDataType; | |
| render(<NoteNode data={data} />); | |
| expect(lastNodeDescriptionProps.inputClassName).not.toContain( | |
| "text-foreground", | |
| ); | |
| }); | |
| expect(lastNodeDescriptionProps.inputClassName).toContain( | |
| "text-foreground", | |
| ); | |
| expect(lastNodeDescriptionProps.mdClassName).toContain( | |
| "text-foreground", | |
| ); | |
| // Dark-mode override must be preserved alongside the new light-mode fix. | |
| expect(lastNodeDescriptionProps.inputClassName).toContain( | |
| "dark:text-background", | |
| ); | |
| expect(lastNodeDescriptionProps.mdClassName).toContain( | |
| "dark:!text-background", | |
| ); | |
| expect(lastNodeDescriptionProps.placeholderClassName).toContain( | |
| "text-foreground/70", | |
| ); | |
| expect(lastNodeDescriptionProps.placeholderClassName).toContain( | |
| "dark:!text-background", | |
| ); | |
| }); | |
| }); | |
| it("leaves the transparent/no-background preset without a forced text color", () => { | |
| const data = { | |
| id: "note-1", | |
| node: { | |
| description: "Some note text", | |
| template: { backgroundColor: "transparent" }, | |
| }, | |
| } as unknown as NoteDataType; | |
| render(<NoteNode data={data} />); | |
| expect(lastNodeDescriptionProps.inputClassName).not.toContain( | |
| "text-foreground", | |
| ); | |
| expect(lastNodeDescriptionProps.placeholderClassName).not.toContain( | |
| "text-foreground", | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/CustomNodes/NoteNode/__tests__/note-node-contrast.test.tsx`
around lines 69 - 99, Extend the NoteNode contrast tests around the existing
lastNodeDescriptionProps assertions to verify placeholderClassName: require
text-foreground/70 and dark:!text-background for opaque presets, and require
that the transparent/no-background preset does not include the forced
placeholder color classes.
Source: Coding guidelines
| role="button" | ||
| aria-label={t("sidebar.addComponentToCanvas", { | ||
| name: display_name, | ||
| })} | ||
| tabIndex={0} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file"
fd -a 'sidebarDraggableComponent\.tsx$' . || true
echo "Inspect outline"
path="$(fd 'sidebarDraggableComponent\.tsx$' . | head -n1)"
if [ -n "${path:-}" ]; then
ast-grep outline "$path" --match 'SidebarDraggableComponent' --view expanded || true
echo "--- relevant lines ---"
sed -n '1,230p' "$path" | nl -ba
fi
echo "Search related attributes"
if [ -n "${path:-}" ]; then
rg -n "role=\"button\"|tabIndex|onKeyDown|aria-disabled|isDisabled|disabled|SelectTrigger" "$path" || true
fiRepository: langflow-ai/langflow
Length of output: 538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "File line count"
wc -l "$path"
echo "Relevant lines 1-260"
awk '{printf "%5d\t%s\n", NR, $0}' "$path" | sed -n '1,260p'
echo "Search within file"
rg -n "role=\"button\"|ariaLabel|tabIndex|onKeyDown|aria-disabled|isDisabled|disabled|SelectTrigger|Button" "$path" || trueRepository: langflow-ai/langflow
Length of output: 11329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Use add component hook implementation"
rg -n "export function useAddComponent|function useAddComponent|const useAddComponent|m:\\/\\/.*useAddComponent|useAddComponent" src/frontend/src/hooks src/frontend/src -g '*.ts' -g '*.tsx' || true
hook_file="$(rg -l "useAddComponent" src/frontend/src/hooks src/frontend/src/components src/frontend/src/stores 2>/dev/null | head -n1 || true)"
if [ -n "${hook_file:-}" ]; then
echo "--- hook file: $hook_file ---"
sed -n '1,220p' "$hook_file" | awk '{printf "%5d\t%s\n", NR, $0}'
fi
echo "Interactive controls in parent/sidebar context"
parent="$(dirname "$(fd 'sidebarDraggableComponent\.tsx$' . | head -n1)")"
rg -n "SidebarDraggableComponent|disabled|component\\.disabled|isDisabled|disabledTooltip" "$parent"/.. src/frontend/src/pages src/frontend/src/components src/frontend/src/stores -g '*.ts' -g '*.tsx' || trueRepository: langflow-ai/langflow
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "Remaining relevant lines 260-276"
sed -n '260,276p' "$path" | awk '{printf "%5d\t%s\n", NR+259, $0}'
echo "Check for aria-disabled or activation guard in file"
rg -n "aria-disabled|disabled\\?|!disabled|handleKeyDown|role=\"button\"" "$path" || true
echo "Static check of activation functions referenced around role button"
python3 - <<'PY'
from pathlib import Path
import re
p = Path("./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx")
text = p.read_text()
checks = {
"role_button": r'<div[^>]*role\s*=\s*"\bbutton\b"',
"tabindex_0_wrapper": r'<div[^>]*tabIndex\s*=\s*\{0\}[^>]*onKeyDown\s*=\s*\{handleKeyDown\}[^>]*>',
"handle_keydown_add_component": r'function handleKeyDown.*?[^}]?\n[^\n]*addComponent\(\s*apiClass\s*,\s*itemName\s*\)',
"disabled_reference_in_keydown": re.compile(r'handleKeyDown\([^)]+\)\s*\{(?P<body>.*?)[\}]', re.S),
"child_tabindex_minus_1": r'tabIndex\s*=\s*\{-1\}',
"conditional_add_button": r'!\s*disabled\s*\{\s*<Button',
}
for name, pattern in checks.items():
m = re.search(pattern, text, flags=re.S if isinstance(pattern, str) else re.S)
body = m.group("body") if isinstance(pattern, str) and name == "disabled_reference_in_keydown" else None
print(f"{name}: {bool(m)}" + (f" | body={repr(body.strip())}" if body else ""))
PYRepository: langflow-ai/langflow
Length of output: 1429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "Relevant lines around keyboard handler and wrapper"
sed -n '95,145p' "$path" | awk '{printf "%5d\t%s\n", NR+94, $0}'
echo "Search for disabled gating around handler or wrapper"
rg -n "handleKeyDown|disabled|aria-disabled|role=\"button\"" "$path" || true
echo "Read-only structural validation"
python3 - <<'PY'
from pathlib import Path
p = Path("./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx")
text = p.read_text()
print("has_handleKeyDown_addComponent_no_disabled_guard=", "handleKeyDown" in text and "addComponent(apiClass, itemName)" in text and "disabled" not in text.split("const handleKeyDown",1)[1].split("};",1)[0])
print("has_wrapper_role_button=", 'role="button"' in text and 'tabIndex={0}' in text and 'onKeyDown={handleKeyDown}' in text)
print("has_child_tabindex_minus_1=", 'tabIndex={-1}' in text)
print("has_conditional_add_button=", '{"} {<Button' in text or '{!disabled && (' in text)
PYRepository: langflow-ai/langflow
Length of output: 2640
Do not expose this composite as a single button.
The wrapper is focusable and traps keyboard activation on the Add/Select actions via tabIndex={-1}, while its Enter/Space handler bypasses disabled. Split the drag target from the interactive controls, or make the wrapper a real activation target with guard/aria-disabled and keep the child actions independently focusable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx`
around lines 134 - 138, The draggable wrapper around the Add/Select controls
should not be exposed as a composite button. Update the wrapper and its keyboard
handling in sidebarDraggableComponent so the drag target is separate from
independently focusable child actions, or make the wrapper a guarded activation
target with aria-disabled; ensure Enter/Space cannot activate disabled controls
and do not trap focus with tabIndex={-1}.
| ariaLabel: t("flow.nodeAriaLabel", { | ||
| name: node.data?.node?.display_name ?? node.data?.type, | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fall back when display_name is empty.
The Note creation path sets display_name to an empty string at Line [835], so ?? produces " node" instead of falling back to the node type. Use a truthy fallback:
Proposed fix
- name: node.data?.node?.display_name ?? node.data?.type,
+ name:
+ node.data?.node?.display_name || node.data?.type || node.type,📝 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.
| ariaLabel: t("flow.nodeAriaLabel", { | |
| name: node.data?.node?.display_name ?? node.data?.type, | |
| }), | |
| ariaLabel: t("flow.nodeAriaLabel", { | |
| name: | |
| node.data?.node?.display_name || node.data?.type || node.type, | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/pages/FlowPage/components/PageComponent/index.tsx` around
lines 124 - 126, Update the ariaLabel construction in PageComponent to treat an
empty display_name as missing by using a truthy fallback to node.data?.type
instead of nullish-only fallback. Preserve the existing translation call and
node name precedence for non-empty display names.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14212 +/- ##
==================================================
+ Coverage 61.30% 61.31% +0.01%
==================================================
Files 2342 2342
Lines 237114 237130 +16
Branches 33340 33340
==================================================
+ Hits 145354 145406 +52
+ Misses 89963 89927 -36
Partials 1797 1797
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary
role="separator"from the sidebar nav divider<li>(element is alreadyaria-hidden, so the role added nothing).role="button"+aria-labelto draggable sidebar component items.<nav>landmark via theSidebarcomponent's existingrole="navigation"support.aria-labelto the ReactFlow canvas wrapper and to each node passed to<ReactFlow>.text-foregroundoverride in light mode, alongside the existing dark-mode overrides.flow.canvasLabel,flow.nodeAriaLabel,sidebar.componentsPanel) uploaded to GP; non-English locales intentionally left untouched pending the next translation sync.QA — how to test
<li>hasaria-hidden="true"and noroleattribute.data-testid="rf__wrapper") — confirm it hasaria-label="Flow canvas".aria-labellike "Chat Input node".Summary by CodeRabbit
Accessibility
Bug Fixes
Localization
Tests