fix(actions): fix state mutation and slice direction in ActionLogger#34129
fix(actions): fix state mutation and slice direction in ActionLogger#34129saurav61091 wants to merge 2 commits into
Conversation
The addAction callback was mutating objects in place (previous.count++ and action.count = 1), violating React's immutability principle. Additionally, slice(0, limit) was retaining the oldest actions and discarding newest ones. Changed to slice(-limit) to keep the most recent entries, which is the expected behavior for an action log. Fixes storybookjs#34052
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors ActionLogger's action accumulation to use immutable updates: computes previous actions from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/actions/containers/ActionLogger/index.tsx`:
- Around line 44-47: Guard against non-positive action.options.limit before
using slice: compute a sanitized limit (e.g., const limit = Math.max(0,
Number(action.options?.limit) || 0)) and if limit === 0 return an empty array
immediately; otherwise use .slice(-limit) in both branches (replace
slice(-action.options.limit) with slice(-limit)). Update both the updated branch
and the newAction branch to use this sanitized limit so zero or negative limits
don't cause unexpected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eff4a98b-05e7-4490-997e-5fd82e6e21ab
📒 Files selected for processing (1)
code/core/src/actions/containers/ActionLogger/index.tsx
| return updated.slice(-action.options.limit); | ||
| } else { | ||
| action.count = 1; | ||
| newActions.push(action); | ||
| const newAction = { ...action, count: 1 }; | ||
| return [...prevActions, newAction].slice(-action.options.limit); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether 'limit' is guaranteed to be > 0 by type/defaults.
rg -n -C3 '\blimit\b' code/core/src/actions --glob '*.{ts,tsx}'
# Verify all slice usages in ActionLogger-related files.
rg -n -C2 'slice\(' code/core/src/actions/containers/ActionLogger code/core/src/actions/components/ActionLogger --glob '*.{ts,tsx}'Repository: storybookjs/storybook
Length of output: 2907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full ActionOptions interface definition
echo "=== ActionOptions Interface ==="
cat code/core/src/actions/models/ActionOptions.ts
echo -e "\n=== Check for limit validation in ActionLogger ==="
cat code/core/src/actions/containers/ActionLogger/index.tsx
echo -e "\n=== Check for limit validation in runtime config ==="
cat code/core/src/actions/runtime/configureActions.ts
echo -e "\n=== Search for any validation/guards on limit ==="
rg -n -B2 -A2 'limit.*[<>=]|validate.*limit|if.*limit' code/core/src/actions --glob '*.{ts,tsx}'Repository: storybookjs/storybook
Length of output: 3167
Handle non-positive limits before slicing.
Using slice(-action.options.limit) breaks for limit === 0 or negative limits. When limit === 0, slice(-0) behaves like slice(0), which keeps all actions instead of none. The limit property is typed as number with no TypeScript constraint enforcing positive values, and no runtime guard prevents zero or negative values from reaching this code.
Suggested patch
const addAction = useCallback((action: ActionDisplay) => {
setActions((prevActions) => {
+ const limit = action.options.limit;
+ if (limit <= 0) {
+ return [];
+ }
const previous = prevActions.length ? prevActions[prevActions.length - 1] : null;
if (previous && safeDeepEqual(previous.data, action.data)) {
const updated = [...prevActions];
updated[updated.length - 1] = { ...previous, count: previous.count + 1 };
- return updated.slice(-action.options.limit);
+ return updated.slice(-limit);
} else {
const newAction = { ...action, count: 1 };
- return [...prevActions, newAction].slice(-action.options.limit);
+ return [...prevActions, newAction].slice(-limit);
}
});
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/actions/containers/ActionLogger/index.tsx` around lines 44 -
47, Guard against non-positive action.options.limit before using slice: compute
a sanitized limit (e.g., const limit = Math.max(0, Number(action.options?.limit)
|| 0)) and if limit === 0 return an empty array immediately; otherwise use
.slice(-limit) in both branches (replace slice(-action.options.limit) with
slice(-limit)). Update both the updated branch and the newAction branch to use
this sanitized limit so zero or negative limits don't cause unexpected behavior.
Sanitize action.options.limit to handle zero, negative, or undefined values. When limit is non-positive, return all actions without slicing. Addresses CodeRabbit review feedback.
|
There is already a PR in place fixing the issue: #34053. Closing |
2 participants
Summary
addActioncallback —previous.count++andaction.count = 1were mutating objects in place, violating React's immutability principle and potentially causing reconciliation bugsslice(0, limit)toslice(-limit)so the most recent actions are retained instead of the oldest onesWhat Changed
File:
code/core/src/actions/containers/ActionLogger/index.tsxprevious.count++count: 1instead of mutatingaction.count = 1slice(-limit)to keep newest entries when at capacityTest plan
Fixes #34052
Summary by CodeRabbit