fix(actions): prevent state mutation and retain newest actions when limit reached

[shanliuling]

What is the problem?

The ActionLogger component has two issues:

1. State mutation: The addAction function mutates objects directly in React state, which violates React's immutability principle and can cause subtle bugs:

   • previous.count++ mutates the existing action object
   • action.count = 1 mutates the incoming action object

2. Wrong slice direction: slice(0, limit) keeps the oldest actions and discards the newest when at capacity. Users typically expect to see the most recent action logs.

How did I fix it?

1. Immutability fix: Create new objects instead of mutating:

   // Before: mutation
   previous.count++;
   
   // After: new object
   updated[updated.length - 1] = { ...previous, count: previous.count + 1 };

2. Slice direction fix: Use slice(-limit) to keep the newest actions:

   // Before: keeps oldest
   return newActions.slice(0, action.options.limit);
   
   // After: keeps newest
   return [...prevActions, newAction].slice(-action.options.limit);

How was this tested?

• Added comprehensive unit tests covering:
  • Adding new actions
  • Incrementing count for consecutive same actions
  • Immutability (no mutation of original objects)
  • Limit behavior (retaining newest actions)
  • Edge cases (limit = 1, count increment with limit)

Closes #34052

Summary by CodeRabbit

• Tests

  • Added comprehensive unit tests for ActionLogger's action handling, covering action creation, addition, count increment, and limit behavior.

• Bug Fixes

  • Enhanced ActionLogger's data handling to prevent unintended side effects.

[coderabbitai]

📝 Walkthrough

Walkthrough

A comprehensive test suite for ActionLogger's addAction logic has been added, and the implementation has been refactored to use immutable update patterns, replacing direct mutations of action counts and arrays with new object creation and trimming operations.

Changes

Cohort / File(s)	Summary
ActionLogger Test Suite code/core/src/actions/containers/ActionLogger/index.test.ts	New comprehensive unit tests covering addAction behavior: creation, duplicate handling, count incrementing, immutability verification, and limit enforcement across bounded action history.
ActionLogger Implementation code/core/src/actions/containers/ActionLogger/index.tsx	Refactored addAction to use immutable patterns: replaced in-place mutation of action counts with new object creation and changed array concatenation to use slice-based trimming per action limit.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• storybookjs/storybook#33977: Modifies the same ActionLogger container's addAction logic to enforce immutable update patterns by replacing mutations with new object creation and array trimming.

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can generate a title for your PR based on the changes with custom instructions.

Set the reviews.auto_title_instructions setting to generate a title for your PR based on the changes in the PR with custom instructions.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

code/core/src/actions/containers/ActionLogger/index.test.ts (1)

15-26: Testing duplicated logic instead of the actual implementation.

The test duplicates the addAction logic locally rather than testing the actual implementation in ActionLogger. This creates a risk that:

• Tests pass even if the real implementation has bugs
• The test's addAction and production addAction could diverge over time

Consider extracting the pure addAction logic from the component into a separate, exportable utility function that can be imported and tested directly. This aligns better with testing real behavior.

♻️ Suggested approach

// In a separate file, e.g., actionUtils.ts
export function addAction(prevActions: ActionDisplay[], action: ActionDisplay): ActionDisplay[] {
  const previous = prevActions.length ? prevActions[prevActions.length - 1] : null;
  // ... rest of implementation
}

// In the test file
import { addAction } from './actionUtils';

Then in the component:

import { addAction } from './actionUtils';
// Use it in useCallback

Based on learnings: "Export functions that need direct tests and test real behavior, not just syntax patterns"

🤖 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.test.ts` around lines 15
- 26, The test currently reimplements the addAction logic locally instead of
testing the real implementation in ActionLogger; extract the pure addAction
logic from the ActionLogger component into a new exported utility (e.g.,
actionUtils.ts) that exports function addAction(prevActions: ActionDisplay[],
action: ActionDisplay): ActionDisplay[], update the ActionLogger to import and
use this exported addAction (for example inside useCallback), and change the
test to import addAction from the new utility and assert its behavior so the
test covers the actual implementation rather than a duplicated copy.

🤖 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.test.ts`:
- Around line 158-173: The test's assertions contradict the actual behavior of
addAction/createAction: with limit = 2 the final result should contain two
distinct actions (args ['1','2']) with counts [1,2]. Fix by either (A) keeping
limit = 2 and update expectations to expect length 2, args order ['1','2'],
result[0].count === 1 and result[1].count === 2, or (B) change the limit to 1 if
the intent was to push out the older action and then assert length 1, args
['2'], and count 2; adjust the assertions around createAction and addAction
accordingly.

---

Nitpick comments:
In `@code/core/src/actions/containers/ActionLogger/index.test.ts`:
- Around line 15-26: The test currently reimplements the addAction logic locally
instead of testing the real implementation in ActionLogger; extract the pure
addAction logic from the ActionLogger component into a new exported utility
(e.g., actionUtils.ts) that exports function addAction(prevActions:
ActionDisplay[], action: ActionDisplay): ActionDisplay[], update the
ActionLogger to import and use this exported addAction (for example inside
useCallback), and change the test to import addAction from the new utility and
assert its behavior so the test covers the actual implementation rather than a
duplicated copy.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b7b60efc-96d4-4e00-ab56-ab1fcd19a157

📥 Commits

Reviewing files that changed from the base of the PR and between 546aece and a901f49.

📒 Files selected for processing (2)

• code/core/src/actions/containers/ActionLogger/index.test.ts
• code/core/src/actions/containers/ActionLogger/index.tsx

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test expectations appear incorrect for the limit scenario.

Tracing through the test logic:

1. addAction([], action1) → [{ args: ['1'], count: 1 }]
2. addAction(result, action2) → [{ args: ['1'], count: 1 }, { args: ['2'], count: 1 }]
3. addAction(result, action3) (same data as action2) → updates last element's count to 2, slices to last 2 elements → [{ args: ['1'], count: 1 }, { args: ['2'], count: 2 }]

With limit: 2, no action gets pushed out since there are only 2 distinct actions. The expectations at lines 171-172 appear inconsistent:

• Line 169 expects length 2
• Line 171 expects map(...args[0]) to equal ['2'] (length 1)
• Line 172 expects result[0].count to be 2, but result[0] would be the action with args: ['1'] having count: 1

🐛 Suggested fix for test expectations

      expect(result).toHaveLength(2);
-      // Should keep ['click 2' (count: 2)] since action1 was pushed out
-      expect(result.map((a) => a.data.args[0])).toEqual(['2']);
-      expect(result[0].count).toBe(2);
+      // Both actions remain since limit is 2
+      expect(result.map((a) => a.data.args[0])).toEqual(['1', '2']);
+      expect(result[0].count).toBe(1);
+      expect(result[1].count).toBe(2);

Or if the intent was to test that older actions get pushed out when incrementing, use limit: 1:

-      const limit = 2;
+      const limit = 1;
       const action1 = createAction('click', ['1'], { limit });
       const action2 = createAction('click', ['2'], { limit });
       const action3 = createAction('click', ['2'], { limit }); // Same as action2

       let result: ActionDisplay[] = [];
       result = addAction(result, action1);
       result = addAction(result, action2);
       result = addAction(result, action3);

-      expect(result).toHaveLength(2);
-      // Should keep ['click 2' (count: 2)] since action1 was pushed out
-      expect(result.map((a) => a.data.args[0])).toEqual(['2']);
-      expect(result[0].count).toBe(2);
+      expect(result).toHaveLength(1);
+      expect(result[0].data.args[0]).toBe('2');
+      expect(result[0].count).toBe(2);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	it('should still apply limit when incrementing count', () => {
	const limit = 2;
	const action1 = createAction('click', ['1'], { limit });
	const action2 = createAction('click', ['2'], { limit });
	const action3 = createAction('click', ['2'], { limit }); // Same as action2
	let result: ActionDisplay[] = [];
	result = addAction(result, action1);
	result = addAction(result, action2);
	result = addAction(result, action3);
	expect(result).toHaveLength(2);
	// Should keep ['click 2' (count: 2)] since action1 was pushed out
	expect(result.map((a) => a.data.args[0])).toEqual(['2']);
	expect(result[0].count).toBe(2);
	});
	it('should still apply limit when incrementing count', () => {
	const limit = 2;
	const action1 = createAction('click', ['1'], { limit });
	const action2 = createAction('click', ['2'], { limit });
	const action3 = createAction('click', ['2'], { limit }); // Same as action2
	let result: ActionDisplay[] = [];
	result = addAction(result, action1);
	result = addAction(result, action2);
	result = addAction(result, action3);
	expect(result).toHaveLength(2);
	// Both actions remain since limit is 2
	expect(result.map((a) => a.data.args[0])).toEqual(['1', '2']);
	expect(result[0].count).toBe(1);
	expect(result[1].count).toBe(2);
	});

🤖 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.test.ts` around lines 158
- 173, The test's assertions contradict the actual behavior of
addAction/createAction: with limit = 2 the final result should contain two
distinct actions (args ['1','2']) with counts [1,2]. Fix by either (A) keeping
limit = 2 and update expectations to expect length 2, args order ['1','2'],
result[0].count === 1 and result[1].count === 2, or (B) change the limit to 1 if
the intent was to push out the older action and then assert length 1, args
['2'], and count 2; adjust the assertions around createAction and addAction
accordingly.