[4.x][8221][8222] Accesibility - snack duration, animations enabling/disabling#4898
[4.x][8221][8222] Accesibility - snack duration, animations enabling/disabling#4898jvega190 wants to merge 11 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds two user accessibility preferences—snackbar notification duration and animation toggle—backed by localStorage. New state utilities, two ChangesAccessibility Preferences: Storage, Hooks, UI, and Provider Wiring
Sequence Diagram(s)sequenceDiagram
participant User
participant AccountManagement
participant state.ts
participant localStorage
participant useSnackbarDuration
participant useEnableAnimations
participant CrafterCMSNextBridge
participant CrafterThemeProvider
User->>AccountManagement: Update notification duration / animations checkbox
User->>AccountManagement: Click Save
AccountManagement->>state.ts: setStoredSnackbarDuration(user, value)
AccountManagement->>state.ts: setStoredEnableAnimations(user, value)
state.ts->>localStorage: persist values
state.ts->>window: dispatch custom events
window-->>useSnackbarDuration: storage/custom event triggers re-read
useSnackbarDuration-->>CrafterCMSNextBridge: updated autoHideDuration
window-->>useEnableAnimations: storage/custom event triggers re-read
useEnableAnimations-->>CrafterThemeProvider: updated enableAnimations → theme rebuilt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…into enhancement/4.x/8221 # Conflicts: # ui/app/src/utils/state.ts # yarn.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ui/app/src/hooks/useSnackbarDuration.ts (1)
17-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDecouple hook defaults from component modules.
Line 17 creates an unnecessary hook→component dependency. Move
DEFAULT_SNACKBAR_DURATIONto a shared constants/state module and import it from both places.🤖 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 `@ui/app/src/hooks/useSnackbarDuration.ts` at line 17, The import statement in useSnackbarDuration.ts currently imports DEFAULT_SNACKBAR_DURATION from the AccountManagement component module, creating an unnecessary coupling. Move the DEFAULT_SNACKBAR_DURATION constant to a shared constants module (create one if it doesn't exist, such as in a constants or config directory), then update the import statement in useSnackbarDuration.ts to import from the new shared constants module instead of from AccountManagement. Also update AccountManagement to import from the same shared constants module to keep the constant definition centralized.
🤖 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 `@ui/app/src/components/AccountManagement/AccountManagement.tsx`:
- Around line 393-395: The NumberField component in AccountManagement.tsx is
performing arithmetic operations directly on snackDuration without explicitly
handling the null case. Add null checks before dividing or multiplying the
values to ensure that null states are preserved and not coerced into unexpected
values. Specifically, in both the value prop calculation (snackDuration / 1000)
and the onValueChange handler (value * 1000), add conditional logic to return
null or handle the null case explicitly rather than allowing JavaScript coercion
to bypass the intended null state gate.
In `@ui/app/src/utils/state.ts`:
- Around line 468-470: The removeStoredSnackbarDuration function removes a
storage item but does not emit a change event to notify same-tab subscribers
using useSyncExternalStore, causing their data to become stale. After calling
window.localStorage.removeItem in removeStoredSnackbarDuration, emit a change
event to notify subscribers of the removal (use the same notification mechanism
used elsewhere in this file for storage updates). Apply the same fix to the
other function mentioned around lines 498-500 that also removes storage keys
without notifying subscribers.
- Around line 463-466: The getStoredSnackbarDuration function can return NaN
when the stored value is malformed or non-numeric, since parseInt(value, 10)
returns NaN for invalid inputs. This NaN value will bypass the null fallback in
the consuming hook and leak invalid values into snackbar configuration. After
parsing the value with parseInt in the return statement, validate that the
result is not NaN by using Number.isNaN() or checking if the parsed value is a
valid number before returning it; return null instead when the parsed value is
NaN.
---
Nitpick comments:
In `@ui/app/src/hooks/useSnackbarDuration.ts`:
- Line 17: The import statement in useSnackbarDuration.ts currently imports
DEFAULT_SNACKBAR_DURATION from the AccountManagement component module, creating
an unnecessary coupling. Move the DEFAULT_SNACKBAR_DURATION constant to a shared
constants module (create one if it doesn't exist, such as in a constants or
config directory), then update the import statement in useSnackbarDuration.ts to
import from the new shared constants module instead of from AccountManagement.
Also update AccountManagement to import from the same shared constants module to
keep the constant definition centralized.
🪄 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: 390e4b7f-5565-442e-be79-10de3fdc8d02
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
ui/app/src/components/AccountManagement/AccountManagement.tsxui/app/src/components/CrafterCMSNextBridge/CrafterCMSNextBridge.tsxui/app/src/components/CrafterThemeProvider/CrafterThemeProvider.tsxui/app/src/hooks/useEnableAnimations.tsui/app/src/hooks/useSnackbarDuration.tsui/app/src/utils/state.ts
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== AccountManagement.tsx ==="
cat ui/app/src/components/AccountManagement/AccountManagement.tsx
echo ""
echo "=== CrafterCMSNextBridge.tsx ==="
cat ui/app/src/components/CrafterCMSNextBridge/CrafterCMSNextBridge.tsx
echo ""
echo "=== CrafterThemeProvider.tsx ==="
cat ui/app/src/components/CrafterThemeProvider/CrafterThemeProvider.tsx
echo ""
echo "=== useEnableAnimations.ts ==="
cat ui/app/src/hooks/useEnableAnimations.ts
echo ""
echo "=== useSnackbarDuration.ts ==="
cat ui/app/src/hooks/useSnackbarDuration.ts
echo ""
echo "=== state.ts ==="
cat ui/app/src/utils/state.tsLength of output: 52783
🐛 Issues1. Typo in helper text —
Should be "on the screen". 2. Unreachable
// Current — null path is never reached
onValueChange={(value) =>
setSnackDuration(Number.isFinite(value) ? Number(value) * 1000 : DEFAULT_SNACKBAR_DURATION)
}The Save button's // Option A: allow null when cleared
onValueChange={(value) =>
setSnackDuration(value === null ? null : Math.round(value * 1000))
}
// Option B: always keep a valid value, simplify disabled check
disabled={initialSnackDuration === snackDuration && initialEnableAnimations === enableAnimations}3.
|
…o state utility, update imports, and improve helper text clarity in AccountManagement component.
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== AccountManagement.tsx (relevant sections) ==="
grep -n "snackDuration\|onValueChange\|How long\|at on\|disabled=\|DEFAULT_SNACKBAR_DURATION\|null" ui/app/src/components/AccountManagement/AccountManagement.tsx | head -80
echo ""
echo "=== state.ts (subscribe functions key checks) ==="
grep -n "includes\|endsWith\|snackbarDuration\|enableAnimations" ui/app/src/utils/state.ts
echo ""
echo "=== useSnackbarDuration.ts import source ==="
grep -n "import" ui/app/src/hooks/useSnackbarDuration.ts
echo ""
echo "=== DEFAULT_SNACKBAR_DURATION definition location ==="
grep -rn "DEFAULT_SNACKBAR_DURATION" ui/app/src/Length of output: 4642
✅ Fixed — Typo ( ✅ Fixed — Cross-layer import (
// state.ts line 495 — change to:
if (e.key?.endsWith('.enableAnimations')) {
// Either allow null on clear:
onValueChange={(value) => setSnackDuration(value === null ? null : Math.round(value * 1000))}
// Or drop the null type/guard entirely since it can never be null:
const [snackDuration, setSnackDuration] = useState<number>(
getStoredSnackbarDuration(user.username) ?? DEFAULT_SNACKBAR_DURATION
);
// and remove: if (snackDuration === null) return;Two quick one-liners to sort out before handoff, then it should be good to go! 🐇 ✏️ Learnings added
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/package.json (1)
57-57: 🧹 Nitpick | 🔵 TrivialPin the RC dependency to a specific version for stability.
At Line 57,
^1.0.0-rc.0can float to newer release-candidate versions (e.g.,1.0.0-rc.1) and will also match the stable1.0.0release. For pre-release dependencies in production, pin to the exact version (e.g.,"1.0.0-rc.0"instead of"^1.0.0-rc.0") to prevent unexpected upgrades during installs.🤖 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 `@ui/app/package.json` at line 57, The `@base-ui-components/react` dependency is specified with a caret prefix (^1.0.0-rc.0), which allows version floating to newer release candidates and stable releases. Remove the caret prefix from the `@base-ui-components/react` version specification in package.json and pin it to the exact version (1.0.0-rc.0) to ensure stability and prevent unexpected version upgrades during package installations.
🤖 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.
Nitpick comments:
In `@ui/app/package.json`:
- Line 57: The `@base-ui-components/react` dependency is specified with a caret
prefix (^1.0.0-rc.0), which allows version floating to newer release candidates
and stable releases. Remove the caret prefix from the `@base-ui-components/react`
version specification in package.json and pin it to the exact version
(1.0.0-rc.0) to ensure stability and prevent unexpected version upgrades during
package installations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64573075-a319-464c-9022-8ad0aecb26fe
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ui/app/package.jsonui/app/src/components/AccountManagement/AccountManagement.tsxui/app/src/hooks/useSnackbarDuration.tsui/app/src/utils/state.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/src/hooks/useSnackbarDuration.ts
- ui/app/src/utils/state.ts
…ent: remove null handling and ensure proper type definition. Update localStorage event listener condition in state utility for better clarity.
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== state.ts subscribe functions key checks ==="
grep -n "includes\|endsWith\|snackbarDuration\|enableAnimations" ui/app/src/utils/state.ts
echo ""
echo "=== AccountManagement.tsx null-related lines ==="
grep -n "snackDuration\|null\|onValueChange\|useState\|disabled=" ui/app/src/components/AccountManagement/AccountManagement.tsx | head -60Length of output: 3071
✅ Fixed — ✅ Fixed — Dead One last tiny nit (optional): Line 391 still has a dead-code null conditional: value={snackDuration === null ? null : snackDuration / 1000}Since value={snackDuration / 1000}This is cosmetic — TypeScript might even warn about it. Up to you whether to clean it before handoff, but it doesn't block review. Everything else looks good. The implementation is solid and ready for human review! 🐇 |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== AccountManagement.tsx — snackDuration-related lines ==="
grep -n "snackDuration\|null\|onValueChange\|useState\|disabled=\|value=" ui/app/src/components/AccountManagement/AccountManagement.tsx | head -80
echo ""
echo "=== state.ts — subscribe key checks ==="
grep -n "endsWith\|includes\|snackbarDuration\|enableAnimations" ui/app/src/utils/state.ts
echo ""
echo "=== useSnackbarDuration.ts ==="
cat ui/app/src/hooks/useSnackbarDuration.ts
echo ""
echo "=== useEnableAnimations.ts ==="
cat ui/app/src/hooks/useEnableAnimations.ts
echo ""
echo "=== CrafterThemeProvider.tsx — new lines ==="
grep -n "enableAnimations\|transitions\|ripple\|animation\|MuiCssBaseline" ui/app/src/components/CrafterThemeProvider/CrafterThemeProvider.tsx
echo ""
echo "=== CrafterCMSNextBridge.tsx — snackbar lines ==="
grep -n "snackbar\|autoHide\|useSnackbar" ui/app/src/components/CrafterCMSNextBridge/CrafterCMSNextBridge.tsxLength of output: 7439
✅ Typo — "at on the screen" → "on the screen" The implementation is solid and ready for human review. 🐇 |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== AST outlines ==="
ast-grep outline ui/app/src/components/AccountManagement/AccountManagement.tsx --view expanded || true
ast-grep outline ui/app/src/utils/state.ts --view expanded || true
ast-grep outline ui/app/src/hooks/useSnackbarDuration.ts --view expanded || true
ast-grep outline ui/app/src/hooks/useEnableAnimations.ts --view expanded || true
ast-grep outline ui/app/src/components/CrafterThemeProvider/CrafterThemeProvider.tsx --view expanded || true
ast-grep outline ui/app/src/components/CrafterCMSNextBridge/CrafterCMSNextBridge.tsx --view expanded || true
echo
echo "=== Targeted searches ==="
echo "-- AccountManagement: snackbar/accessibility state and UI --"
rg -n -C 3 "DEFAULT_SNACKBAR_DURATION|snackDuration|enableAnimations|onValueChange|onSaveAccessibility|value=|disabled=|on the screen|at on" ui/app/src/components/AccountManagement/AccountManagement.tsx || true
echo
echo "-- state.ts: storage subscriptions and constants --"
rg -n -C 3 "DEFAULT_SNACKBAR_DURATION|subscribeSnackbarDuration|subscribeEnableAnimations|endsWith|includes|snackbarDuration|enableAnimations" ui/app/src/utils/state.ts || true
echo
echo "-- hook imports and snapshots --"
sed -n '1,220p' ui/app/src/hooks/useSnackbarDuration.ts
echo
sed -n '1,220p' ui/app/src/hooks/useEnableAnimations.ts
echo
echo "-- CrafterThemeProvider animation handling --"
rg -n -C 3 "useEnableAnimations|transitions|MuiTouchRipple|disableRipple|MuiCssBaseline|animation|transition" ui/app/src/components/CrafterThemeProvider/CrafterThemeProvider.tsx || true
echo
echo "-- CrafterCMSNextBridge snackbar provider duration --"
rg -n -C 3 "useSnackbarDuration|autoHideDuration|SnackbarProvider" ui/app/src/components/CrafterCMSNextBridge/CrafterCMSNextBridge.tsx || true
echo
echo "=== Focused file slices ==="
sed -n '140,470p' ui/app/src/components/AccountManagement/AccountManagement.tsx
echo
sed -n '430,540p' ui/app/src/utils/state.tsLength of output: 44189
What I verified:
I don’t see any remaining blockers in these changes. This looks ready to hand over for human review. |
2 participants
craftercms/craftercms#8221
craftercms/craftercms#8222