fix(actions): prevent state mutation and retain newest actions in ActionLogger#34053
fix(actions): prevent state mutation and retain newest actions in ActionLogger#34053tysoncung wants to merge 1 commit intostorybookjs:nextfrom
Conversation
…ionLogger Fix three bugs in the ActionLogger addAction callback: 1. previous.count++ mutates the previous state object directly, violating React's immutability principle. Now creates a new object with the incremented count. 2. action.count = 1 mutates the incoming action object. Now creates a shallow copy with count set to 1. 3. slice(0, limit) retains the oldest actions and discards newest when the limit is reached. Changed to slice(-limit) to keep the most recent actions, which is the expected behavior for an action log. Fixes storybookjs#34052
📝 WalkthroughWalkthroughThis change refactors the ActionLogger component to adopt an immutable update pattern instead of mutating action objects in-place. The logic for determining previous action data and conditional updates remains functionally equivalent, but now creates new objects when updating the last entry or appending new actions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 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 or undefined limits before
calling slice(-action.options.limit): validate and normalize
action.options.limit to a non-negative integer (e.g., parse to Number, floor,
and clamp to >=0) and then branch on that value so you only call slice(-limit)
when limit > 0; when limit === 0 return an empty array (or otherwise enforce the
intended zero-behavior) instead of using slice(-0) or slice(-undefined). Apply
this for both uses in the block that references
updated.slice(-action.options.limit) and [...prevActions,
newAction].slice(-action.options.limit), using the normalized limit variable and
the newAction/prevActions/updated symbols to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f95661c9-0757-48c3-87e9-5c5e203bc437
📒 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
# Find ActionsParameters type definition and limit property
rg -n "ActionsParameters|limit" code/core/src/actions --type=ts --type=tsx -A 5 -B 2 | head -100
# Also search more broadly for the type definition
rg -n "interface.*ActionsParameters|type.*ActionsParameters" . --type=ts --type=tsxRepository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for explicit zero-limit examples/configuration
rg -n "limit\s*:\s*0|limit.*=.*0" code/core/src --type=ts --type=tsx
# Look for limit validation or guards
rg -n "limit.*[<>].*0|if.*limit" code/core/src/actions --type=ts --type=tsx -A 2 -B 1Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the actual file in question for context
cat -n code/core/src/actions/containers/ActionLogger/index.tsx | head -60Repository: storybookjs/storybook
Length of output: 2466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the types file - look for it in the actions directory
find code/core/src/actions -name "types*" -o -name "index.ts" | head -20Repository: storybookjs/storybook
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for ActionsParameters definition across the codebase
rg -n "ActionsParameters" code/core/src --max-count=20 -A 3 -B 1Repository: storybookjs/storybook
Length of output: 1427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for limit property within types or interfaces
rg -n "limit.*:" code/core/src/actions -A 1 -B 1 | head -80Repository: storybookjs/storybook
Length of output: 480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the types.ts file to see the full ActionsParameters definition
cat -n code/core/src/actions/types.tsRepository: storybookjs/storybook
Length of output: 2315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the ActionOptions model for limit type and any constraints
cat -n code/core/src/actions/models/ActionOptions.tsRepository: storybookjs/storybook
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check configureActions for more context on limit and validation
cat -n code/core/src/actions/runtime/configureActions.tsRepository: storybookjs/storybook
Length of output: 397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any validation or guards on limit in the codebase
rg -n "limit.*[>0<]|if.*limit|limit.*\|\|" code/core/src/actions -B 2 -A 2Repository: storybookjs/storybook
Length of output: 830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if limit can be undefined or if action.options.limit is always defined
# Look at ActionDisplay type definition
rg -n "ActionDisplay" code/core/src/actions -A 5 -B 1 | head -80Repository: storybookjs/storybook
Length of output: 6227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any tests that set limit to 0 or negative values
rg -n "limit.*0|limit.*-" code/core --max-count=30Repository: storybookjs/storybook
Length of output: 1646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check ActionDisplay interface to see how options are structured
cat -n code/core/src/actions/models/ActionDisplay.tsRepository: storybookjs/storybook
Length of output: 338
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Check if there's any validation or type narrowing that ensures limit is positive
rg -n "ActionDisplay.*count|options.*limit" code/core/src/actions -B 3 -A 3 | head -100Repository: storybookjs/storybook
Length of output: 1198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the runtime/action.ts to see how actionOptions is constructed and if limit is always set
cat -n code/core/src/actions/runtime/action.ts | sed -n '80,105p'Repository: storybookjs/storybook
Length of output: 1124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if config.limit is always applied or if there's fallback handling
rg -n "config\." code/core/src/actions/runtime/action.ts -B 2 -A 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let me trace how actionOptions are passed to the action function
rg -n "export.*action\s*=" code/core/src/actions/runtime/action.ts -A 3 -B 3Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if limit can be undefined - look at how ActionOptions is actually used
rg -n "actionOptions\|ActionOptions" code/core/src/actions/runtime/action.ts -B 2 -A 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for where the limit is actually resolved or if there's a fallback
rg -n "\.limit|limit\s*\?" code/core/src/actions -B 1 -A 1 | head -60Repository: storybookjs/storybook
Length of output: 700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let me check more precisely how action.options.limit is set
# by looking at the full action function signature
cat -n code/core/src/actions/runtime/action.ts | sed -n '40,100p'Repository: storybookjs/storybook
Length of output: 2418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if limit value is ever validated or constrained elsewhere
rg -n "if.*limit\|limit.*?.*:|limit.*>|limit.*<" code/core/src/actionsRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Double-check by looking at the complete ActionOptions type definition
cat -n code/core/src/actions/models/ActionOptions.tsRepository: storybookjs/storybook
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what happens when slice is called with -0
node <<'EOF'
const arr = [1, 2, 3, 4, 5];
console.log("slice(-50):", arr.slice(-50)); // Should give [1,2,3,4,5]
console.log("slice(-0):", arr.slice(-0)); // Equivalent to slice(0)
console.log("slice(0):", arr.slice(0)); // Gives full array
console.log("[] with slice(-0):", [].slice(-0)); // Empty array
EOFRepository: storybookjs/storybook
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any integration tests or stories that verify the limit=0 behavior
rg -n "limit.*0|limit:.*0" code/core/src/actions --type=js -B 2 -A 2Repository: storybookjs/storybook
Length of output: 509
Guard non-positive or undefined limit before using slice(-limit).
Lines 44 and 47 use slice(-action.options.limit) without validation. Since ActionOptions is Partial<Options>, the limit property can be undefined. Additionally, limit has no type constraint enforcing positive integers. When limit is 0 or undefined, slice(-0) and slice(-undefined) both return the full array, silently disabling log truncation instead of keeping zero or all entries as expected. Add a guard to handle these edge cases explicitly.
🤖 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 or undefined limits before calling
slice(-action.options.limit): validate and normalize action.options.limit to a
non-negative integer (e.g., parse to Number, floor, and clamp to >=0) and then
branch on that value so you only call slice(-limit) when limit > 0; when limit
=== 0 return an empty array (or otherwise enforce the intended zero-behavior)
instead of using slice(-0) or slice(-undefined). Apply this for both uses in the
block that references updated.slice(-action.options.limit) and [...prevActions,
newAction].slice(-action.options.limit), using the normalized limit variable and
the newAction/prevActions/updated symbols to locate the code.
1 participant
What
Fix three bugs in the
ActionLoggeraddActioncallback:State mutation via
previous.count++— directly mutates the previous state object, violating React's immutability principle. This can cause subtle reconciliation bugs where React doesn't detect the state change.Incoming object mutation via
action.count = 1— mutates the incoming action object rather than creating a copy. This could affect other references to the same object.slice(0, limit)keeps oldest, discards newest — when the action log reaches its limit, the oldest entries are retained and new ones are dropped. Changed toslice(-limit)to keep the most recent actions, which is the expected behavior for an action log.How
{ ...previous, count: previous.count + 1 }instead of mutatingprevious.count.{ ...action, count: 1 }instead of mutatingaction.count.slice(-limit)instead ofslice(0, limit)to retain the newest actions.Testing
Verified the fix addresses all three checklist items from #34052. The changes are minimal and focused on the state updater function.
Closes #34052
Summary by CodeRabbit