fix(actions): fix state mutation and retain newest actions when limit is reached#34107
fix(actions): fix state mutation and retain newest actions when limit is reached#34107sukets16 wants to merge 1 commit into
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughExtracts action-list management into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/actions/containers/ActionLogger/addAction.test.ts (1)
62-86: Consider adding a test for undefinedlimit.Given that
ActionOptionsis aPartial<Options>type andlimitcould be undefined at runtime, consider adding a test that verifies the behavior whenoptions.limitis undefined to document the expected behavior.📝 Example test case
it('handles undefined limit gracefully', () => { const action = createAction({ id: '1', options: { clearOnStoryChange: true } as ActionDisplay['options'], // no limit }); const result = computeAddAction([], action); expect(result).toHaveLength(1); // Document expected behavior - either uses default or allows unlimited });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/containers/ActionLogger/addAction.test.ts` around lines 62 - 86, Add a test in addAction.test.ts that covers when an action's options.limit is undefined: use createAction to make an action with options that omit limit (e.g., { clearOnStoryChange: true } as ActionDisplay['options']), call computeAddAction([], action), and assert the resulting state behavior (e.g., result has length 1 or matches the code's documented default/unlimited behavior); reference computeAddAction, createAction, and ActionDisplay/ActionOptions in the test to clearly document expected handling of undefined limit.
🤖 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/addAction.ts`:
- Around line 22-26: The code uses action.options.limit directly in
slice(-action.options.limit) which fails when limit is undefined
(slice(-undefined) === slice(0)), so defensively resolve a numeric limit first
and use it in both return paths; e.g., compute const limit = typeof
action.options?.limit === "number" ? action.options.limit :
<DEFAULT_LIMIT_FROM_ActionOptions_OR_FALLBACK> (or import the default from
ActionOptions), then replace slice(-action.options.limit) with slice(-limit) for
updated and [...prevActions, newAction].slice(-limit); reference
action.options.limit, prevActions, updated, and newAction when making the
change.
---
Nitpick comments:
In `@code/core/src/actions/containers/ActionLogger/addAction.test.ts`:
- Around line 62-86: Add a test in addAction.test.ts that covers when an
action's options.limit is undefined: use createAction to make an action with
options that omit limit (e.g., { clearOnStoryChange: true } as
ActionDisplay['options']), call computeAddAction([], action), and assert the
resulting state behavior (e.g., result has length 1 or matches the code's
documented default/unlimited behavior); reference computeAddAction,
createAction, and ActionDisplay/ActionOptions in the test to clearly document
expected handling of undefined limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4cad208-9e27-486d-ba97-8a1a8821db67
📒 Files selected for processing (4)
code/core/src/actions/containers/ActionLogger/addAction.test.tscode/core/src/actions/containers/ActionLogger/addAction.tscode/core/src/actions/containers/ActionLogger/index.tsxdocs/essentials/actions.mdx
… is reached The addAction callback in the Actions addon had two bugs: 1. State mutation: previous.count++ and action.count = 1 mutated objects directly, violating React's immutability principle. 2. Wrong slice direction: slice(0, limit) kept the oldest actions and discarded the newest. Changed to slice(-limit) to retain recent entries. Extracted the state update logic into a pure computeAddAction function for testability, and added unit tests covering immutability, limit behavior, and count incrementing. Also documented the limit parameter in the actions addon docs. Closes storybookjs#34052
68194e8 to
f3f46ad
Compare
|
Closing in favour of #34053 |
Summary
Fixes #34052
The
addActioncallback in the Actions addon had two bugs:1. State mutation
previous.count++andaction.count = 1mutated objects directly inside a React state updater, violating immutability and potentially causing reconciliation issues.2. Wrong slice direction
slice(0, limit)kept the oldest actions and discarded the newest when the limit was reached. Changed toslice(-limit)so the most recent actions are retained.Changes
code/core/src/actions/containers/ActionLogger/addAction.ts-- Extracted the state update logic into a purecomputeAddActionfunction that creates new objects instead of mutating, and usesslice(-limit)to keep newest entries.code/core/src/actions/containers/ActionLogger/index.tsx-- Simplified the component to use the extracted function.code/core/src/actions/containers/ActionLogger/addAction.test.ts-- 7 unit tests covering: adding actions, count incrementing, immutability of inputs, immutability of previous state, limit retaining newest, limit with count increment, and distinct action entries.docs/essentials/actions.mdx-- Documented thelimitparameter (type, default value, behavior when reached).Checklist from #34052
Summary by CodeRabbit
New Features
limitparameter (default 50) to control the maximum number of displayed actions; oldest actions are discarded when the limit is reached.Tests
Documentation
limitparameter.