-
Notifications
You must be signed in to change notification settings - Fork 33
fix: fix ESlint warnings #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe PR updates React hook dependency arrays across 11 frontend components, primarily adding missing dependencies like translation functions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/features/tasks/components/ChatArea.tsx (2)
81-121: Restoration effect dependency update is safe; minor optional cleanup onlyIncluding
selectedTeamin the dependency array matches the values used in the effect and avoids stale logs, and thehasRestoredPreferencesguard prevents any re-run loop whensetSelectedTeamis called. The added dependency looks correct and low-cost.If you ever want to cut a tiny bit of noise, you could remove
selectedTeamfrom the log (or move that log elsewhere) so the effect doesn’t re-run on every team change just for logging, but this is purely optional.
273-279: Scroll-on-open effect dependency expansion looks correctAdding
hasMessagesto the dependency array aligns with the values used inside the effect and keeps the scroll behavior consistent when the presence of messages changes. SincehasMessagesis derived fromselectedTaskDetail?.id, this is effectively redundant but harmless and resolves ESLint warnings without changing behavior.frontend/src/features/settings/components/TabParamSync.tsx (1)
18-39: Consider moving the constant mapping outside the effect.The
tabNameToIndexmapping is a constant object that doesn't depend on any props, state, or effect dependencies. Moving it inside theuseEffectcauses it to be recreated on every execution (wheneversearchParamsorsetTabIndexchange). While the performance impact is negligible for this small object, it's more idiomatic to define such constants at the module or component level.Apply this diff to move the mapping outside the effect:
+const tabNameToIndex: Record<string, number> = { + integrations: 0, + bots: 1, + bot: 1, + team: 2, +}; + export default function TabParamSync({ tabIndex: _tabIndex, setTabIndex }: TabParamSyncProps) { const searchParams = useSearchParams(); useEffect(() => { - // Tab name to index mapping parameterization - const tabNameToIndex: Record<string, number> = { - integrations: 0, - bots: 1, - bot: 1, - team: 2, - }; - const tab = searchParams?.get('tab');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/src/features/login/components/OidcTokenHandler.tsx(1 hunks)frontend/src/features/onboarding/useOnboarding.ts(4 hunks)frontend/src/features/settings/components/BotList.tsx(1 hunks)frontend/src/features/settings/components/GitHubIntegration.tsx(1 hunks)frontend/src/features/settings/components/McpConfigImportModal.tsx(1 hunks)frontend/src/features/settings/components/TabParamSync.tsx(1 hunks)frontend/src/features/settings/components/TeamList.tsx(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(2 hunks)frontend/src/features/tasks/components/ResizableSidebar.tsx(3 hunks)frontend/src/features/tasks/components/TaskParamSync.tsx(1 hunks)frontend/src/features/tasks/components/TeamSelector.tsx(1 hunks)
🔇 Additional comments (11)
frontend/src/features/settings/components/McpConfigImportModal.tsx (1)
91-91: LGTM! Correct ESLint fix.Adding
agentTypeto the dependency array is correct since it's used within the callback (line 75). This ensures the callback references the currentagentTypevalue and prevents stale closure bugs.frontend/src/features/tasks/components/TaskParamSync.tsx (1)
54-54: Includingmessagein the effect dependencies is correct and keeps ESLint happyThis aligns the
useEffectwithreact-hooks/exhaustive-depsand ensures the effect always sees the currentmessageinstance; given the early-return guards, the extra reruns whenmessagechanges should be negligible. Please just confirm thatApp.useApp().messageis stable enough in your usage so you don’t accidentally trigger unnecessary re-runs.frontend/src/features/tasks/components/TeamSelector.tsx (1)
91-113: LGTM! Correct dependency addition.Adding
tto theuseMemodependency array is correct since the translation function is used within the memoized computation (line 105). This ensures the team options update properly when the language changes.frontend/src/features/login/components/OidcTokenHandler.tsx (1)
24-60: LGTM! Dependency array correctly updated.The addition of
tto the dependency array is appropriate since the translation function is used for displaying error and success messages. The effect will now properly reflect language changes if they occur during the OIDC flow.frontend/src/features/settings/components/GitHubIntegration.tsx (1)
30-49: Verify stability to avoid unnecessary data refetches.Similar to BotList.tsx, this data-loading effect now depends on
messageandt. If these references are not stable across renders, the effect will re-run and refetch git information unnecessarily. Please confirm that bothmessage(fromApp.useApp()) andt(fromuseTranslation()) maintain stable identities.frontend/src/features/onboarding/useOnboarding.ts (2)
67-132: LGTM! Proper use of useCallback for function stabilization.Wrapping
startTourinuseCallbackis the correct approach to stabilize the function reference, especially since it's used as a dependency in the useEffect below (line 166). All values used within the callback are properly included in the dependency array.
145-166: LGTM! Dependency array correctly updated.Including
startTourin the dependency array is correct since the function is called within the effect. The memoization above ensures this won't cause unnecessary re-runs.frontend/src/features/settings/components/TeamList.tsx (1)
77-91: Consistent pattern with other data-loading effects.This follows the same pattern as BotList.tsx, with
messageandtadded to the dependency array. The memoized setters (setBotsSorted,setTeamsSorted) are correctly included. The same stability verification concern applies here as noted in BotList.tsx.frontend/src/features/tasks/components/ResizableSidebar.tsx (2)
46-51: LGTM! Proper memoization of the save callback.Wrapping
saveWidthinuseCallbackwithstorageKeyas a dependency is correct. This stabilizes the function reference and prevents unnecessary re-creation, which is important since it's used in the effect dependency array below.
60-92: LGTM! Dependency array correctly includes memoized callback.Adding
saveWidthto the dependency array is correct since it's called within the effect (line 77). The memoization ensures this won't cause unnecessary effect re-runs. The formatting improvements (semicolons, trailing commas) also enhance code consistency.frontend/src/features/settings/components/BotList.tsx (1)
50-63: Dependency array is correct — no changes needed.The
tfunction from react-i18next has caused stability issues in concurrent mode and should be included in dependency arrays, and Ant Design's message API does not guarantee unchanging function object identity across renders. Your code correctly includes bothmessageandtas dependencies alongsidesetBotsSorted, which prevents stale closures and ensures the effect re-runs when these references change.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.