Actions: Fix state mutation and keep newest actions when limit reached#34286
Conversation
|
View your CI Pipeline Execution ↗ for commit e022dd9
☁️ Nx Cloud last updated this comment at |
|
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 (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Storybook discovery for action container stories, introduces ActionLogger stories with play tests and a mock Manager API, refactors ActionLogger to immutably aggregate counts and enforce a configurable per-action Changes
Sequence DiagramsequenceDiagram
participant Story as Storybook Story
participant Manager as Manager API
participant Logger as ActionLogger
participant State as React State
participant UI as Rendered UI
Story->>Manager: emit(action)
Manager->>Logger: onAction(action)
Note over Logger: compare incoming action to last stored action
alt duplicate action (deep equal)
Logger->>State: replace last action with new object (count + 1)
else new action
Logger->>State: append { ...action, count: 1 }
end
State->>Logger: actions = actions.slice(-limit)
Logger->>UI: re-render actions list
Story->>UI: play() assertions verify rendered output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/actions/containers/ActionLogger/ActionLogger.stories.tsx (2)
68-74: Consider usingwaitForfor more robust async handling.The fixed 50ms
setTimeoutworks but is somewhat fragile—it may be too slow in CI or too fast if React batching changes. Consider usingwaitForfrom the testing library for more deterministic waiting:+import { waitFor } from 'storybook/test'; + async function emitActions(actions: ActionDisplay[]) { for (const action of actions) { currentApi.emit(EVENT_ID, action); - // Let React process the state update - await new Promise((resolve) => setTimeout(resolve, 50)); + // Wait for React to process the state update + await waitFor(() => {}); } }Alternatively, if the current approach works reliably in your test environment, this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/containers/ActionLogger/ActionLogger.stories.tsx` around lines 68 - 74, The helper emitActions uses a fixed 50ms setTimeout which is fragile; replace the manual sleep with a deterministic wait using testing-library's waitFor to await the React updates after each currentApi.emit(EVENT_ID, action). Update the emitActions function to import and use waitFor (or waitForNextUpdate) so you call currentApi.emit(EVENT_ID, action) and then await waitFor(() => /* assertion or state check that indicates the update applied */) instead of the fixed timeout, referencing the emitActions function and currentApi.emit to locate where to change.
52-53: Module-level state for test isolation—acceptable pattern but document the constraint.The module-level
currentApicreates implicit coupling between the decorator and play functions. This works because Storybook runs decorators synchronously before render and play. The inline comment at line 52 helps, but consider adding a note that stories should not run in parallel if this ever becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/containers/ActionLogger/ActionLogger.stories.tsx` around lines 52 - 53, Update the existing module-level comment for the currentApi variable to explicitly document the coupling constraint: explain that currentApi (type ReturnType<typeof createMockApi>) is intentionally module-scoped so the decorator and play functions share the same mock, note that Storybook executes decorators synchronously before render/play, and add a caution that stories relying on this pattern should not run in parallel to avoid test isolation issues; reference currentApi, createMockApi, and the decorator/play functions in the comment so future contributors understand the limitation and intent.
🤖 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/ActionLogger.stories.tsx`:
- Around line 68-74: The helper emitActions uses a fixed 50ms setTimeout which
is fragile; replace the manual sleep with a deterministic wait using
testing-library's waitFor to await the React updates after each
currentApi.emit(EVENT_ID, action). Update the emitActions function to import and
use waitFor (or waitForNextUpdate) so you call currentApi.emit(EVENT_ID, action)
and then await waitFor(() => /* assertion or state check that indicates the
update applied */) instead of the fixed timeout, referencing the emitActions
function and currentApi.emit to locate where to change.
- Around line 52-53: Update the existing module-level comment for the currentApi
variable to explicitly document the coupling constraint: explain that currentApi
(type ReturnType<typeof createMockApi>) is intentionally module-scoped so the
decorator and play functions share the same mock, note that Storybook executes
decorators synchronously before render/play, and add a caution that stories
relying on this pattern should not run in parallel to avoid test isolation
issues; reference currentApi, createMockApi, and the decorator/play functions in
the comment so future contributors understand the limitation and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bdc6609-b3b3-459b-855a-0f85f4144859
📒 Files selected for processing (4)
code/.storybook/main.tscode/core/src/actions/containers/ActionLogger/ActionLogger.stories.tsxcode/core/src/actions/containers/ActionLogger/index.tsxdocs/essentials/actions.mdx
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Pull request overview
Fixes Actions panel action-log limiting behavior by preventing React state mutation and ensuring the newest actions are retained when the configured limit is exceeded.
Changes:
- Avoid mutating previous state objects in the
setActionsupdater and defaultlimitto 50 when missing. - Keep the newest action entries when at capacity by switching to
slice(-limit). - Add container-level stories with play functions to cover limit/clear/dedup behavior and document
limit/clearOnStoryChange.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/essentials/actions.mdx | Documents limit, clearOnStoryChange, and configureActions for the Actions addon. |
| code/core/src/actions/containers/ActionLogger/index.tsx | Fixes state immutability and retains newest actions when enforcing the limit. |
| code/core/src/actions/containers/ActionLogger/ActionLogger.stories.tsx | Adds interaction stories validating limit behavior, deduping, and clearing. |
| code/.storybook/main.ts | Registers actions container stories in the repo Storybook config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #### `limit` | ||
|
|
||
| Type: `number` | ||
|
|
||
| Default: `50` | ||
|
|
||
| Maximum number of action entries to display in the actions panel. When the limit is reached, the oldest actions are discarded and the newest are retained. | ||
|
|
||
| This parameter is configured globally using the `configureActions` function (see [Exports](#configureactions) below) rather than as a story parameter. | ||
|
|
||
| #### `clearOnStoryChange` | ||
|
|
||
| Type: `boolean` | ||
|
|
||
| Default: `true` | ||
|
|
||
| When set to `true`, the actions panel is automatically cleared when navigating to a different story. | ||
|
|
||
| This parameter is configured globally using the `configureActions` function (see [Exports](#configureactions) below) rather than as a story parameter. |
There was a problem hiding this comment.
The docs list limit and clearOnStoryChange under "Parameters" (parameters.actions.*), but ActionsParameters in code only includes argTypesRegex, disable, handles, and expandLevel (code/core/src/actions/types.ts). Since these options are applied via configureActions/action options (not story parameters read by the addon), consider moving them out of the Parameters section or explicitly labeling them as configureActions options to avoid misleading readers.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Removed limit and clearOnStoryChange from the Parameters section entirely — they were already fully documented under the configureActions export. Commit 7933cb4.
| * Waits briefly after each emission for React state to settle. | ||
| */ | ||
| async function emitActions(actions: ActionDisplay[]) { | ||
| for (const action of actions) { | ||
| currentApi.emit(EVENT_ID, action); | ||
| // Let React process the state update | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
emitActions uses a fixed setTimeout(50) after each emit. This makes the story tests slower and can still be flaky on slower environments. Prefer waiting on UI state (e.g., waitFor / findBy* assertions) after emitting, so the play function synchronizes on actual rendering rather than a hard-coded delay.
| * Waits briefly after each emission for React state to settle. | |
| */ | |
| async function emitActions(actions: ActionDisplay[]) { | |
| for (const action of actions) { | |
| currentApi.emit(EVENT_ID, action); | |
| // Let React process the state update | |
| await new Promise((resolve) => setTimeout(resolve, 50)); | |
| * Yields to the event loop after each emission so React can process state updates. | |
| * Callers should still synchronize on actual UI state (e.g., via `waitFor` / `findBy*`). | |
| */ | |
| async function emitActions(actions: ActionDisplay[]) { | |
| for (const action of actions) { | |
| currentApi.emit(EVENT_ID, action); | |
| // Yield to allow React to process the state update without relying on a fixed timeout | |
| await Promise.resolve(); |
Sidnioulz
force-pushed
the
sidnioulz/actions-limit-opencode-test
branch
from
47b62ae to
4416405
Compare
…eached - Stop mutating previous.count and action.count in React state updater - Use slice(-limit) instead of slice(0, limit) to retain newest actions - Add fallback default of 50 for undefined limit
Add 7 story variants covering empty state, single/repeated/multiple actions, limit behavior, scrollable list, and nested data inspection.
Add documentation for the limit parameter (default 50, keeps newest), clearOnStoryChange parameter (default true), and the configureActions export function with usage example.
The limit logic lives in the container (addAction callback with slice(-limit)), not the presentational component. Stories now exercise the actual container by mocking the manager API as an event emitter and using play functions to emit actions via EVENT_ID. New stories test: single action, repeated/deduplicated actions, multiple actions, limit discarding oldest, limit with deduplication, and clearing.
…ction Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/dad07e0d-5acd-4291-ac76-ba8509abef2c Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
… fix api comment Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/8da2eff4-99ce-48f3-9f8d-a73f45474cfb Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Sidnioulz
force-pushed
the
sidnioulz/actions-limit-opencode-test
branch
from
7a4c87e to
e022dd9
Compare
limitandclearOnStoryChangefrom Parameters section (they areconfigureActionsoptions, already documented in the Exports section)setTimeout(50)withawait Promise.resolve()inemitActionsfor faster, more reliable story testsargs.apito accurately describe actual data flow (prop passed viarender, not context)Summary by CodeRabbit
New Features
clearOnStoryChangeoption to control whether the Actions panel clears during story navigation.configureActionsAPI for global configuration of Actions settings.Documentation