fix(actions): correct options.limit application in ActionLogger#34054
fix(actions): correct options.limit application in ActionLogger#34054mibragimov wants to merge 1 commit into
Conversation
The actions log was not correctly applying the options.limit when filtering log entries. The bug was in the slice direction: - Actions are appended to the END of the array via newActions.push() - slice(0, limit) keeps the FIRST 'limit' items (oldest actions) - Once the limit was reached, NEW actions were silently dropped instead of the oldest being evicted Fix: use slice(-limit) to keep the most recent 'limit' actions. Also guard against limit being undefined/falsy to avoid slice(-0) which would return an empty array. Fixes storybookjs#34052
📝 WalkthroughWalkthroughModified the ActionLogger's action truncation logic to retain the most recent N actions instead of the oldest N actions when a limit is applied, changing from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/actions/containers/ActionLogger/index.tsx (1)
39-46: Immutability violation: mutating objects in state.The linked issue
#34052also identifies thatprevious.count++(line 42) andaction.count = 1(line 44) mutate objects directly. The shallow copy[...prevActions]doesn't protect nested object references, sopreviousstill points to an object in the previous state. This violates React's immutability principles and could cause subtle bugs with re-renders or stale closures.If addressing this is planned for a follow-up, consider leaving a TODO comment. Otherwise:
♻️ Proposed fix for immutability
setActions((prevActions) => { - const newActions = [...prevActions]; - const previous = newActions.length && newActions[newActions.length - 1]; - if (previous && safeDeepEqual(previous.data, action.data)) { - previous.count++; - } else { - action.count = 1; - newActions.push(action); - } + const previous = prevActions.length && prevActions[prevActions.length - 1]; + let newActions: ActionDisplay[]; + if (previous && safeDeepEqual(previous.data, action.data)) { + newActions = [ + ...prevActions.slice(0, -1), + { ...previous, count: previous.count + 1 }, + ]; + } else { + newActions = [...prevActions, { ...action, count: 1 }]; + } return action.options.limit ? newActions.slice(-action.options.limit) : newActions; });🤖 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 39 - 46, The code mutates existing objects in state via previous.count++ and action.count = 1; instead, create new object copies before changing counts: when merging with the previous item (identified by previous and safeDeepEqual) clone previous (e.g., {...previous}) and increment the clone's count, replacing the last element in newActions with that clone; when adding the incoming action (symbol action) clone it with count: 1 (e.g., {...action, count: 1}) and push the clone into newActions. Ensure no direct writes to previous or action occur; if you intend to defer this change, add a TODO noting the immutability fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/actions/containers/ActionLogger/index.tsx`:
- Around line 39-46: The code mutates existing objects in state via
previous.count++ and action.count = 1; instead, create new object copies before
changing counts: when merging with the previous item (identified by previous and
safeDeepEqual) clone previous (e.g., {...previous}) and increment the clone's
count, replacing the last element in newActions with that clone; when adding the
incoming action (symbol action) clone it with count: 1 (e.g., {...action, count:
1}) and push the clone into newActions. Ensure no direct writes to previous or
action occur; if you intend to defer this change, add a TODO noting the
immutability fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83207b93-d27e-4c45-b5f2-9543615f3eee
📒 Files selected for processing (1)
code/core/src/actions/containers/ActionLogger/index.tsx
Summary
Fixes a logic bug in the
ActionLoggercontainer whereoptions.limitwas not correctly applied when trimming the actions log.The Bug
Actions are appended to the end of the array via
newActions.push(action). The original code used:slice(0, limit)keeps the firstlimititems (the oldest actions). Once the limit was reached, new actions were silently dropped rather than the oldest ones being evicted. This meant the actions panel would stop updating once it hit the limit.The Fix
slice(-limit)keeps the lastlimititems (the most recent actions), so the panel always shows the latest actions and correctly evicts the oldest ones. The falsy guard preventsslice(-0)(which returns an empty array) whenlimitis0orundefined.Closes #34052
Summary by CodeRabbit