Actions: Retain newest items when limit is reached#34081
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnforces and bounds action history in the ActionLogger by introducing a configurable Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
Actionable comments posted: 2
🤖 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 40-56: action.options.limit is used directly causing
NaN/fractional values to bypass the limit guard and produce incorrect slice
behavior; normalize it first (e.g., coerce to a finite non-negative integer
using Number.isFinite + Math.trunc/Math.max) and then use that normalized value
in the existing guard (limit <= 0) and both slice calls
(updatedActions.slice(-limit) and [...prevActions, nextAction].slice(-limit));
update references to the local variable named limit in this block so previous,
safeDeepEqual, prevActions, updatedActions, and nextAction logic remains
unchanged but uses the sanitized limit.
In `@code/core/template/stories/parameters-actions.stories.ts`:
- Line 35: The play function currently destructures canvasElement from an
untyped parameter causing an implicit any; import PlayFunctionContext and
annotate the play parameter with that type (e.g., change the play signature to
accept a PlayFunctionContext and then destructure canvasElement from it) and add
the corresponding import for PlayFunctionContext so the parameter is strongly
typed like the other stories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f344ddba-000f-4cf2-9c31-b3c294f733b7
📒 Files selected for processing (3)
code/core/src/actions/containers/ActionLogger/index.tsxcode/core/template/stories/parameters-actions.stories.tsdocs/essentials/actions.mdx
|
Addressed the CodeRabbit review items in the latest commit:
|
|
Closing in favour of #34053 |
What I changed
Addresses #34052 by fixing action log retention behavior and removing object mutation in
ActionLogger.✅ Fixes in
ActionLoggerprevious.count++)action.count = 1)actions.limitviaslice(-limit)limit <= 0safely by returning an empty listFile:
code/core/src/actions/containers/ActionLogger/index.tsx✅ Story to showcase behavior when limit is reached
Added a story that sets
actions.limit = 3and emits 5 clicks to demonstrate that only the latest 3 actions are retained.File:
code/core/template/stories/parameters-actions.stories.ts✅ Documentation updates
Documented actions parameters:
limit(default50) and that it keeps most recent actions when fullclearOnStoryChange(defaulttrue)File:
docs/essentials/actions.mdxWhy
The previous implementation made a shallow array copy, then mutated nested action objects, which breaks immutability expectations and can cause subtle state issues. It also trimmed with
slice(0, limit), which retains oldest entries and drops newest at capacity.Notes
yarn install.Fixes #34052
Summary by CodeRabbit
New Features
limiton retained actions (default: 50) and clears history whenlimitis non-positive.Documentation
limitandclearOnStoryChangeActions addon parameters.