Skip to content

Conversation

@Sam-61s
Copy link
Contributor

@Sam-61s Sam-61s commented Jan 2, 2026

Description

  • Removes React state updates from useMemo in ToolsDashboard.tsx
  • Moves the state update logic to useEffect to avoid side effects
  • Keeps the existing behavior unchanged while improving predictability and stability

Related issue(s)

Fixes #4847

Summary by CodeRabbit

  • Refactor
    • Optimized tools dashboard performance and state management efficiency.

✏️ Tip: You can customize this high-level summary in your review settings.

@netlify
Copy link

netlify bot commented Jan 2, 2026

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b1e05ed
🔍 Latest deploy log https://app.netlify.com/projects/asyncapi-website/deploys/695aa8b646c1ad00081cff4c
😎 Deploy Preview https://deploy-preview-4857--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 2, 2026

📝 Walkthrough

Walkthrough

The PR fixes an anti-pattern where React state was being updated inside a useMemo hook in the ToolsDashboard component. The checkToolsList state has been converted to a derived, memoized value computed directly from the toolsList data, eliminating problematic state mutations within the memoization logic.

Changes

Cohort / File(s) Summary
State Derivation Refactor
components/tools/ToolsDashboard.tsx
Removed checkToolsList state variable; replaced with useMemo-based derivation from toolsList. Removed state mutation logic in filtering loop. Updated scrolling useEffect dependency array to include toolsList instead of empty array, ensuring proper reactivity to tools list changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A dashboard once fought with its state,
Updating in memos—a React mistake!
Now derived from the data so clean,
No side effects where they shouldn't be seen,
One file refactored, the bug takes its break! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: state update inside useMemo' directly and accurately describes the main change in the PR—removing state updates from useMemo and moving them to useEffect.
Linked Issues check ✅ Passed The PR successfully addresses all requirements from issue #4847: eliminates state updates from useMemo, moves logic to useEffect, and preserves component behavior as evidenced by the derived memoized checkToolsList approach.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the useMemo state update issue; the scrolling useEffect dependency update is a necessary adjustment to support the derived state pattern.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8cd3cd7 and 088cd24.

📒 Files selected for processing (1)
  • components/tools/ToolsDashboard.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:43.887Z
Learning: In the Filter component (components/navigation/Filter.tsx), the useEffect hooks are intentionally designed to react only to route/URL changes, not to prop changes (data, checks, onFilter). The omitted dependencies are by design.
📚 Learning: 2025-12-29T14:21:43.887Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:43.887Z
Learning: In the Filter component (components/navigation/Filter.tsx), the useEffect hooks are intentionally designed to react only to route/URL changes, not to prop changes (data, checks, onFilter). The omitted dependencies are by design.

Applied to files:

  • components/tools/ToolsDashboard.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.

Applied to files:

  • components/tools/ToolsDashboard.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Redirect rules - asyncapi-website
  • GitHub Check: Header rules - asyncapi-website
  • GitHub Check: Pages changed - asyncapi-website
  • GitHub Check: Test NodeJS PR - windows-latest
  • GitHub Check: Lighthouse CI
🔇 Additional comments (2)
components/tools/ToolsDashboard.tsx (2)

143-146: LGTM! Anti-pattern successfully eliminated.

The conversion of checkToolsList from state to a derived, memoized value correctly eliminates the problematic state update inside useMemo. The logic using some() is efficient and the dependencies are correct.


162-162: The dependency array [toolsList] is necessary and correct.

The effect reads toolsList[elementID]?.elementRef (line 154), so toolsList must be included in the dependency array. Additionally, since elementRef is recreated each time toolsList changes (line 134), the effect must re-run to use the fresh refs. Without this dependency, the effect would reference stale refs after filters are applied. The re-scrolling behavior on filter changes is intentional and required for the component to function properly.

Likely an incorrect or invalid review comment.


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

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

@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (2747eba) to head (b1e05ed).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #4857   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          798       798           
  Branches       146       146           
=========================================
  Hits           798       798           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Jan 2, 2026

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 41
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-4857--asyncapi-website.netlify.app/

Sourya07

This comment was marked as resolved.

@Sam-61s
Copy link
Contributor Author

Sam-61s commented Jan 5, 2026

Hi @Sourya07, thanks for the review.
I've checked the code carefully, and the issues you mentioned are not present in the current implementation:

  1. Duplicate Declaration:
    checkToolsList is declared only once, using useMemo on line 144
    There is no useState declaration for checkToolsList in the file

  2. Side Effects in useMemo:
    There is no setCheckToolsList anywhere in the codebase
    The toolsList useMemo does not call setCheckToolsList

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: State update inside useMemo

3 participants