actions: keep newest entries when limit is reached#34207
Conversation
|
Implemented and pushed. This PR fixes Actions panel retention at Please take a look when convenient. |
📝 WalkthroughWalkthroughExtracted action-list update logic into an exported helper 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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
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 31-41: The trimming logic uses slice(-limit) which treats limit <=
0 as "no trim", so update the reducer in ActionLogger to special-case when limit
<= 0 (e.g., if (limit <= 0) return [] for zero or return prevActions / updated
without slice if negative semantics desired) before both return paths that call
slice(-limit) — adjust the branch that updates the last repeated action (uses
previous, updated, previous.count) and the branch that appends newAction to
handle limit <= 0 explicitly; also add a regression test asserting that
options.limit = 0 results in an empty retained list (or the intended zero
retention) to prevent future regressions.
In `@docs/essentials/actions.mdx`:
- Around line 107-119: Update the documented signature for action() to reflect
the full options shape instead of only { limit?: number } — use options?:
ActionOptions (or describe options as supporting limit, depth,
clearOnStoryChange, etc.) so calls using depth or clearOnStoryChange are shown
as valid; keep the separate subsection documenting options.limit (Type: number,
Default: 50) as an additional supported field while referencing ActionOptions
for the complete schema and examples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c0bb039-b3a5-4a4f-a221-e8437c36e7c3
📒 Files selected for processing (4)
code/core/src/actions/containers/ActionLogger/index.test.tscode/core/src/actions/containers/ActionLogger/index.tsxcode/core/template/stories/basics.stories.tsdocs/essentials/actions.mdx
|
Thanks for the review! Addressed both points in the latest commit:
Please take another look. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/actions/containers/ActionLogger/index.tsx (1)
61-65: Minor:clearOnStoryChangecheck may reference stale config after retention trim.With the new
slice(-limit)retention keeping the newest entries,actions[0]becomes the oldest retained action rather than the first action ever added. If actions with differentclearOnStoryChangesettings are mixed, this check could behave unexpectedly.This is pre-existing logic and not introduced by this PR, so it's low priority. Consider storing the
clearOnStoryChangepreference separately (e.g., from the first action ever received, or from configuration) if consistent behavior across mixed settings is desired.🤖 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 61 - 65, handleStoryChange currently checks actions[0].options.clearOnStoryChange which can be stale after retention trimming (slice(-limit)) because actions[0] is the oldest retained action, not the first-ever; change the logic to track the desired clearOnStoryChange flag separately (e.g., store a module/state variable or a ref like firstActionClearOnStoryChange when the very first action is received or read from config) and have handleStoryChange consult that stored flag instead of actions[0]; ensure the flag is set when actions are initialized/first pushed (update the code paths that populate actions and apply retention) and preserved across trims so clearActions is invoked consistently based on the original preference (references: handleStoryChange, clearOnStoryChange, clearActions, actions).
🤖 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 61-65: handleStoryChange currently checks
actions[0].options.clearOnStoryChange which can be stale after retention
trimming (slice(-limit)) because actions[0] is the oldest retained action, not
the first-ever; change the logic to track the desired clearOnStoryChange flag
separately (e.g., store a module/state variable or a ref like
firstActionClearOnStoryChange when the very first action is received or read
from config) and have handleStoryChange consult that stored flag instead of
actions[0]; ensure the flag is set when actions are initialized/first pushed
(update the code paths that populate actions and apply retention) and preserved
across trims so clearActions is invoked consistently based on the original
preference (references: handleStoryChange, clearOnStoryChange, clearActions,
actions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b372372-1520-4559-952b-c694e6289503
📒 Files selected for processing (3)
code/core/src/actions/containers/ActionLogger/index.test.tscode/core/src/actions/containers/ActionLogger/index.tsxdocs/essentials/actions.mdx
✅ Files skipped from review due to trivial changes (1)
- code/core/src/actions/containers/ActionLogger/index.test.ts
|
Is already handled by #34053 |
Summary
Fixes Actions panel retention logic when
options.limitis reached so Storybook keeps the newest entries instead of the oldest.Changes
code/core/src/actions/containers/ActionLogger/index.tsx:slice(0, limit)to latest-window behavior (slice(-limit))setActionsupdater (immutable updates forcountand new entries)code/core/src/actions/containers/ActionLogger/index.test.ts:code/core/template/stories/basics.stories.ts:OptionLimitto showcase panel behavior with a small limitdocs/essentials/actions.mdx:action(..., { limit })Notes
I couldn’t run local vitest in this environment because dependency installation timed out; CI should validate the full suite.
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor