A11y: Persist tab/highlight across docs navigation #32762
Conversation
📝 WalkthroughWalkthroughConsolidates addon state into a persisted composite object Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant A11yContext
participant Store as AddonStateStore
User->>A11yContext: Mount
A11yContext->>Store: useAddonState(UI_STATE_ID) — read persisted { ui?, results?, error?, status? }
Store-->>A11yContext: persisted state
alt a11ySelection present
A11yContext->>A11yContext: initialize ui.tab / ui.highlighted from a11ySelection
A11yContext->>Store: persist composite state (UI_STATE_ID)
Store-->>A11yContext: confirmation
else
A11yContext->>A11yContext: use persisted or default ui
end
User->>A11yContext: Toggle highlight / Switch tab / Update status
A11yContext->>A11yContext: update composite state via setState wrapper
A11yContext->>Store: persist composite state
Store-->>A11yContext: confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential focus areas:
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)code/addons/a11y/src/manager.tsx (2)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/addons/a11y/src/components/A11yContext.test.tsx (3)
16-21: Align mocks with repo test guidelinesUse spy: true and move ad‑hoc mock impls into beforeEach to reduce drift and keep tests consistent.
As per coding guidelines, update the module mock:
-vi.mock('storybook/manager-api'); +vi.mock('storybook/manager-api', { spy: true });And hoist custom mock implementations currently defined inside individual tests (e.g., useAddonState store-backed impl) into a helper or the test's beforeEach when possible. Based on guidelines.
338-380: Highlight state persistence test reads well; consider DRY helperTest is correct and representative. To avoid duplication, extract the store‑backed useAddonState mock into a small helper used by persistence tests.
426-473: Deep‑link precedence test: add assertion that persistence updatedGreat verification of precedence and query param clearing. Also assert that the persisted store reflects the override.
Apply:
expect(getByTestId('hl').textContent).toBe('true'); expect(getByTestId('tab').textContent).toBe('violations'); expect(setQueryParams).toHaveBeenCalledWith({ a11ySelection: '' }); + // Persisted UI state should also reflect the override + expect(store.get(UI_STATE_ID)).toEqual({ highlighted: true, tab: RuleType.VIOLATION });code/addons/a11y/src/components/A11yContext.tsx (1)
325-339: Avoid double writes; make deep‑link persistence atomicCurrent effect sets highlighted, calls setTab (which also persists), and then calls setUiState again. Combine into a single UI state write and set local tab only once. Behavior unchanged; fewer rerenders.
Apply:
- useEffect(() => { - if (!a11ySelection) { - return; - } - setHighlighted(true); - const [type] = a11ySelection.split('.') ?? []; - if (type && Object.values(RuleType).includes(type as RuleType)) { - setTab(type as RuleType); - } - setUiState((prev) => ({ ...prev, highlighted: true })); - // We intentionally do not include setHighlighted/setTab/setUiState in deps to avoid loops - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [a11ySelection]); + useEffect(() => { + if (!a11ySelection) return; + setHighlighted(true); + const [type] = a11ySelection.split('.') ?? []; + const validType = type && Object.values(RuleType).includes(type as RuleType) + ? (type as RuleType) + : undefined; + if (validType) setTabState(validType); + setUiState((prev) => ({ ...prev, highlighted: true, tab: validType ?? prev.tab })); + // Intentionally omit setters to avoid loops + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [a11ySelection]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/addons/a11y/src/components/A11yContext.test.tsx(2 hunks)code/addons/a11y/src/components/A11yContext.tsx(4 hunks)code/addons/a11y/src/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/a11y/src/constants.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/components/A11yContext.test.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/addons/a11y/src/constants.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/components/A11yContext.test.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/addons/a11y/src/constants.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/components/A11yContext.test.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/a11y/src/constants.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/components/A11yContext.test.tsx
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/addons/a11y/src/components/A11yContext.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/addons/a11y/src/components/A11yContext.test.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/addons/a11y/src/components/A11yContext.test.tsx
🧬 Code graph analysis (2)
code/addons/a11y/src/components/A11yContext.tsx (2)
code/core/src/manager-api/root.tsx (1)
useAddonState(455-457)code/addons/a11y/src/constants.ts (1)
UI_STATE_ID(4-4)
code/addons/a11y/src/components/A11yContext.test.tsx (2)
code/addons/a11y/src/components/A11yContext.tsx (2)
useA11yContext(488-488)A11yContextProvider(97-486)code/addons/a11y/src/constants.ts (1)
UI_STATE_ID(4-4)
🔇 Additional comments (3)
code/addons/a11y/src/constants.ts (1)
4-4: LGTM: dedicated addon state keyClear, unique namespace for UI persistence. No concerns.
code/addons/a11y/src/components/A11yContext.test.tsx (1)
382-424: Tab persistence over remounts is verified correctlySolid coverage; no issues found.
code/addons/a11y/src/components/A11yContext.tsx (1)
116-122: Good: unified, persisted UI state for tab/highlightUsing useAddonState with a typed shape and persisting in both handlers ensures state survives remounts. Nicely done.
Also applies to: 153-166
vanessayuenn
changed the title
vanessayuenn
changed the title
|
After merging the latest 'next', I noticed ci/circleci: e2e-ui and normal checks are failing, though my previous commit passed. I'm happy to debug and resolve this if these failures relate to my code—just let me know if you'd like me to proceed, or if the main branch is in flux and I should wait. If needed, I'm also happy to post more details on the errors to help clarify whether it's my changes or the branch state. |
|
View your CI Pipeline Execution ↗ for commit 83f36d8
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/a11y/src/components/A11yContext.tsx (1)
355-377: Optional chaining gaps can crash when results are undefined
results?.[ui.tab as RuleType].map/flatMap(...)accesses.map/.flatMapon possibly undefined, causing a runtime TypeError. Use optional chaining on the array, too.Apply this diff:
- menu: results?.[ui.tab as RuleType].map<HighlightMenuItem[]>((result) => { + menu: results?.[ui.tab as RuleType]?.map<HighlightMenuItem[]>((result) => { const selectors = result.nodes .flatMap((n) => n.target) .map(String) .filter((e) => selected.includes(e)); return [ { - id: `${ui.tab}.${result.id}:info`, + id: `${ui.tab}.${result.id}:info`, title: getTitleForAxeResult(result), description: getFriendlySummaryForAxeResult(result), selectors, }, { - id: `${ui.tab}.${result.id}`, + id: `${ui.tab}.${result.id}`, iconLeft: 'info', iconRight: 'shareAlt', title: 'Learn how to resolve this violation', clickEvent: EVENTS.SELECT, selectors, }, ]; }), }); } - const others = results?.[ui.tab as RuleType] - .flatMap((r) => r.nodes.flatMap((n) => n.target).map(String)) + const others = results?.[ui.tab as RuleType] + ?.flatMap((r) => r.nodes.flatMap((n) => n.target).map(String)) .filter((e) => ![...unhighlightedSelectors, ...selected].includes(e)); if (others?.length) { emit(HIGHLIGHT, { id: `${ADDON_ID}/others`, selectors: others, styles: { - outline: `1px solid color-mix(in srgb, ${colorsByType[ui.tab]}, transparent 30%)`, - backgroundColor: `color-mix(in srgb, ${colorsByType[ui.tab]}, transparent 60%)`, + outline: `1px solid color-mix(in srgb, ${colorsByType[ui.tab]}, transparent 30%)`, + backgroundColor: `color-mix(in srgb, ${colorsByType[ui.tab]}, transparent 60%)`, }, hoverStyles: { outlineWidth: '2px', }, focusStyles: { backgroundColor: 'transparent', }, - menu: results?.[ui.tab as RuleType].map<HighlightMenuItem[]>((result) => { + menu: results?.[ui.tab as RuleType]?.map<HighlightMenuItem[]>((result) => {Also applies to: 380-390, 397-419
🧹 Nitpick comments (3)
code/addons/a11y/src/components/A11yContext.tsx (3)
99-105: Avoid side‑effects inside useMemo and tighten deep‑link parsing
api.setQueryParamsinsideuseMemois a side‑effect. Move touseEffectfor safety (React strict mode double renders can otherwise bite).- The regex uses unescaped dots; it matches too broadly and may accept invalid values.
Apply this diff:
- const a11ySelection = useMemo(() => { - const value = api.getQueryParam('a11ySelection'); - if (value) { - api.setQueryParams({ a11ySelection: '' }); - } - return value; - }, [api]); + const a11ySelection = useMemo(() => api.getQueryParam('a11ySelection'), [api]); + useEffect(() => { + if (a11ySelection) { + // Clear the param after reading; if supported, removing it entirely is preferable. + api.setQueryParams({ a11ySelection: '' }); + } + }, [api, a11ySelection]);And for the validator:
- if (a11ySelection && /^[a-z]+.[a-z-]+.[0-9]+$/.test(a11ySelection)) { + if (a11ySelection && /^[a-z]+\.[a-z-]+\.[0-9]+$/.test(a11ySelection)) {Also, the comment says “violation” but the actual type is “violations”. Consider aligning the example or accepting both forms when parsing.
Also applies to: 304-321
161-168: Prefer immutable Map updates for clarityCurrent code mutates
prev(delete/set) before cloning. Use a fresh Map to avoid in‑place mutation while keeping logic the same.Apply this diff:
- setSelectedItems((prev) => new Map(prev.delete(key) ? prev : prev.set(key, `${key}.1`))); + setSelectedItems((prev) => { + const next = new Map(prev); + if (next.has(key)) next.delete(key); + else next.set(key, `${key}.1`); + return next; + });
304-321: Comment driftComment refers to
setHighlighted/setTab/setUiStatewhich no longer exist as separate setters. Update to reflectsetStateto prevent future confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/addons/a11y/src/components/A11yContext.test.tsx(1 hunks)code/addons/a11y/src/components/A11yContext.tsx(14 hunks)code/addons/a11y/src/manager.tsx(1 hunks)code/addons/a11y/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/a11y/src/components/A11yContext.test.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/a11y/src/types.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/manager.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/addons/a11y/src/types.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/manager.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/addons/a11y/src/types.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/manager.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/a11y/src/types.tscode/addons/a11y/src/components/A11yContext.tsxcode/addons/a11y/src/manager.tsx
🧬 Code graph analysis (2)
code/addons/a11y/src/components/A11yContext.tsx (2)
code/core/src/manager-api/root.tsx (1)
useAddonState(455-457)code/addons/a11y/src/types.ts (4)
RuleType(33-37)RuleType(39-39)EnhancedResults(49-53)Status(60-67)
code/addons/a11y/src/manager.tsx (2)
code/core/src/manager-api/root.tsx (2)
useStorybookApi(294-297)useAddonState(455-457)code/addons/a11y/src/types.ts (4)
RuleType(33-37)RuleType(39-39)EnhancedResults(49-53)Status(60-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: Danger JS
- GitHub Check: normal
🔇 Additional comments (1)
code/addons/a11y/src/types.ts (1)
60-67: Shared Status type looks goodCentralizing status strings here improves consistency across manager/context/tests. No issues spotted.
code/addons/a11y/src/manager.tsx
Outdated
| const [{ results }] = useAddonState<{ | ||
| ui: { highlighted: boolean; tab: RuleType }; | ||
| results: EnhancedResults | undefined; | ||
| error: unknown; | ||
| status: Status; | ||
| }>(ADDON_ID); | ||
| const violationsNb = results?.violations?.length || 0; | ||
| const incompleteNb = results?.incomplete?.length || 0; |
There was a problem hiding this comment.
🧩 Analysis chain
Unsafe destructuring of shared addon state can throw on first mount
If Title renders before any component sets a default for ADDON_ID, state is undefined and const [{ results }] = useAddonState(...) throws. Use a null‑safe pattern.
Apply this diff:
- const [{ results }] = useAddonState<{
+ const [state] = useAddonState<{
ui: { highlighted: boolean; tab: RuleType };
results: EnhancedResults | undefined;
error: unknown;
status: Status;
}>(ADDON_ID);
- const violationsNb = results?.violations?.length || 0;
- const incompleteNb = results?.incomplete?.length || 0;
+ const results = state?.results;
+ const violationsNb = results?.violations?.length ?? 0;
+ const incompleteNb = results?.incomplete?.length ?? 0;Run to find any other call sites that may assume the old shape or destructure unsafely:
🏁 Script executed:
#!/bin/bash
# Search for useAddonState calls keyed by ADDON_ID and show surrounding context
rg -nP -C3 'useAddonState<[^>]*>\(\s*ADDON_ID' --type ts --type tsx code | sed -n '1,200p'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for useAddonState calls with proper file type handling
rg -nP -C3 'useAddonState' --type ts code/addons/a11y/src/manager.tsxLength of output: 552
🏁 Script executed:
#!/bin/bash
# Broader search across code directory for all useAddonState patterns
rg -nP -C3 'useAddonState' --type ts code | head -100Length of output: 6818
🏁 Script executed:
#!/bin/bash
# Check if ADDON_ID is defined and where
rg -nP 'ADDON_ID\s*=' --type ts code/addons/a11y | head -50Length of output: 77
🏁 Script executed:
#!/bin/bash
# View the useAddonState implementation to understand when it returns undefined
cat code/core/src/manager-api/root.tsx | sed -n '455,480p'Length of output: 916
🏁 Script executed:
#!/bin/bash
# Check the current state of A11yContext.tsx around line 107 for similar issue
sed -n '100,120p' code/addons/a11y/src/components/A11yContext.tsxLength of output: 538
🏁 Script executed:
#!/bin/bash
# Check current lines 16-25 in manager.tsx to see if fix is already applied
sed -n '13,30p' code/addons/a11y/src/manager.tsxLength of output: 605
Apply the proposed diff to code/addons/a11y/src/manager.tsx (lines 16-23)
The code destructures unsafely from useAddonState(ADDON_ID) with no default state. The test suite proves this can return [undefined], which throws when destructuring. The fix correctly introduces null-safety:
- const [{ results }] = useAddonState<{
+ const [state] = useAddonState<{
ui: { highlighted: boolean; tab: RuleType };
results: EnhancedResults | undefined;
error: unknown;
status: Status;
}>(ADDON_ID);
- const violationsNb = results?.violations?.length || 0;
- const incompleteNb = results?.incomplete?.length || 0;
+ const results = state?.results;
+ const violationsNb = results?.violations?.length ?? 0;
+ const incompleteNb = results?.incomplete?.length ?? 0;📝 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.
| const [{ results }] = useAddonState<{ | |
| ui: { highlighted: boolean; tab: RuleType }; | |
| results: EnhancedResults | undefined; | |
| error: unknown; | |
| status: Status; | |
| }>(ADDON_ID); | |
| const violationsNb = results?.violations?.length || 0; | |
| const incompleteNb = results?.incomplete?.length || 0; | |
| const [state] = useAddonState<{ | |
| ui: { highlighted: boolean; tab: RuleType }; | |
| results: EnhancedResults | undefined; | |
| error: unknown; | |
| status: Status; | |
| }>(ADDON_ID); | |
| const results = state?.results; | |
| const violationsNb = results?.violations?.length ?? 0; | |
| const incompleteNb = results?.incomplete?.length ?? 0; |
🤖 Prompt for AI Agents
In code/addons/a11y/src/manager.tsx around lines 16 to 23, the call to
useAddonState(ADDON_ID) is destructured without a default and can return
[undefined], causing a crash; change the destructuring to provide a safe default
state (e.g. const [state = {} as { ui?: { highlighted: boolean; tab: RuleType };
results?: EnhancedResults; error?: unknown; status?: Status }] =
useAddonState(ADDON_ID)) and then read values off state (const violationsNb =
state.results?.violations?.length || 0; const incompleteNb =
state.results?.incomplete?.length || 0) so the code never throws when the hook
returns undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/addons/a11y/src/components/A11yContext.tsx (1)
205-213: Critical: Stale closure bug remains unfixed.The
setTimeoutcallback checksif (status === 'ran')using the captured value from line 122, not the state you just set on line 203. This condition will typically fail becausestatuswas likely'running'or'initial'when this callback was created.Apply the fix from the previous review:
setTimeout(() => { - if (status === 'ran') { - setState((prev) => ({ ...prev, status: 'ready' })); - } + setState((prev) => + prev.status === 'ran' ? { ...prev, status: 'ready' } : prev + ); if (selectedItems.size === 1) { const [key] = selectedItems.values(); document.getElementById(key)?.scrollIntoView({ behavior: 'smooth', block: 'center' }); } }, 900);Note: The
selectedItemscheck on line 209 also reads a captured value, but sinceselectedItemsis local state (not in the addon state), and the check is only for UX (scrolling), this is lower priority.
🧹 Nitpick comments (1)
code/addons/a11y/src/components/A11yContext.tsx (1)
453-462: Consider moving inline callbacks outside the return statement.Defining
setTabandsetStatusinline within the JSX is unconventional and slightly reduces readability. Consider defining them alongside other handlers (e.g., nearhandleManualon line 285).For example:
const setTab = useCallback( (type: RuleType) => setState((prev) => ({ ...prev, ui: { ...prev.ui, tab: type } })), [setState] ); const setStatus = useCallback( (status: Status) => setState((prev) => ({ ...prev, status })), [setState] ); return ( <A11yContext.Provider value={{ // ... setTab, setStatus, // ... }} {...props} /> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/a11y/src/components/A11yContext.tsx(14 hunks)code/addons/a11y/src/manager.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/a11y/src/manager.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/a11y/src/components/A11yContext.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/addons/a11y/src/components/A11yContext.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/addons/a11y/src/components/A11yContext.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/a11y/src/components/A11yContext.tsx
🧬 Code graph analysis (1)
code/addons/a11y/src/components/A11yContext.tsx (2)
code/core/src/manager-api/root.tsx (1)
useAddonState(455-457)code/addons/a11y/src/types.ts (4)
RuleType(33-37)RuleType(39-39)EnhancedResults(49-53)Status(60-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (3)
code/addons/a11y/src/components/A11yContext.tsx (3)
107-120: LGTM! State structure aligns with persistence goals.The composite state initialization correctly groups UI preferences (
highlighted,tab) with ephemeral data (results,error,status), enabling persistence of user preferences across navigation.
142-144: LGTM! Consistent state management patterns.All state updates correctly use the updater form of
setState, ensuring atomic reads and writes of the composite state. The refactoring consistently routes tab and highlight changes through theuisubstate.Also applies to: 193-198, 254-267, 285-288, 300-302
329-423: LGTM! Highlight effect correctly uses persisted UI state.The effect consistently references
ui.tabandui.highlightedthroughout, ensuring highlight behavior reflects the persisted user preferences. Color mapping, result filtering, and menu generation all correctly key offui.tab.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/a11y/src/manager.test.tsx (1)
11-11: Addspy: trueoption to the mock.As per coding guidelines, all package and file mocks should use the
spy: trueoption.Apply this diff:
-vi.mock('storybook/manager-api'); +vi.mock('storybook/manager-api', { spy: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/a11y/src/manager.test.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/addons/a11y/src/manager.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/addons/a11y/src/manager.test.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/a11y/src/manager.test.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/addons/a11y/src/manager.test.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/addons/a11y/src/manager.test.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/addons/a11y/src/manager.test.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/a11y/src/manager.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/addons/a11y/src/manager.test.tsx (2)
66-73: LGTM! State structure correctly updated.The mock correctly reflects the new nested state structure where violations and incomplete are nested under
results. The test logic properly validates the badge count (3 = 1 violation + 2 incomplete).Same note as the previous test: ensure the partial state object (omitting
ui,error,status) is safe for the manager code.
41-41: No issues found. The partial state mock is intentional and correct.The Title component at line 17 of manager.tsx only destructures
{ results }from the addon state, despite the full state type includingui,error, and a status property. The mock at line 41 correctly provides only theresultsproperty that the component actually uses. This is a valid pattern—TypeScript allows destructuring a subset of object properties, and the test correctly mocks only what's necessary for the Title component to render. No changes are needed.
|
Thanks for merging this, @ndelangen! Really appreciate the feedback and tweaks. My first opensource merged PR got me jumping out my seat hahaha :) |
4 participants
Problem:
The A11y panel UI state (selected tab and highlight toggle) is lost when navigating Stories → Docs → Stories. See #32634.
Solution:
Persist the A11y panel UI state with useAddonState so preferences survive navigation between Stories and Docs.
Implementation details:
${ADDON_ID}/uiFiles touched:
Test coverage:
Manual testing:
Risks/considerations:
Fixes:
Summary by CodeRabbit