fix(actions): prevent state mutation and keep newest actions on limit#34218
fix(actions): prevent state mutation and keep newest actions on limit#34218Sigmabrogz wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/actions/containers/ActionLogger/index.tsx (1)
49-50: Minor inconsistency: optional chaining onoptionsis unnecessary.Per the
ActionDisplayinterface incode/core/src/actions/models/ActionDisplay.ts,optionsis a required field (not optional), whilelimitwithinActionOptionscan be undefined due toPartial<Options>. The optional chaining onoptionsis inconsistent with line 55 which accessesactions[0].options.clearOnStoryChangedirectly.The slice fix using
slice(-limit)correctly keeps the most recent N actions, aligning with the rolling window behavior intended by the defaultlimit: 50.♻️ Suggested consistency fix
- const limit = action.options?.limit; + const limit = action.options.limit; return limit ? newActions.slice(-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 49 - 50, The optional chaining on action.options is unnecessary because ActionDisplay.options is required; change "const limit = action.options?.limit" to "const limit = action.options.limit" in the ActionLogger logic so it consistently accesses the required options field (keep the existing conditional return using limit and the slice behavior: newActions.slice(-limit) when limit is truthy, otherwise return newActions).
🤖 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 49-50: The optional chaining on action.options is unnecessary
because ActionDisplay.options is required; change "const limit =
action.options?.limit" to "const limit = action.options.limit" in the
ActionLogger logic so it consistently accesses the required options field (keep
the existing conditional return using limit and the slice behavior:
newActions.slice(-limit) when limit is truthy, otherwise return newActions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 888fd826-4ab8-448c-b73f-45b1669503ef
📒 Files selected for processing (1)
code/core/src/actions/containers/ActionLogger/index.tsx
|
Closing in favour of #34053 |
Fixes #34052. This PR prevents state mutations when updating
ActionLogger's actions in React and fixes the slice logic so that the newest actions are kept when reaching the limit (usingslice(-limit)instead ofslice(0, limit)).I have tested these changes to make sure they follow the immutability rules and ensure that
action.countdoes not mutate the original objects.(Auto-generated AI PR by Sigma)
Summary by CodeRabbit
Bug Fixes
Refactor