Fix: prevent closing externally managed database connections in db_update_cluster #836
Conversation
📝 WalkthroughWalkthroughThis PR addresses database connection lifecycle management in the backend and fixes React Hooks dependency issues across multiple onboarding components. The db_update_cluster function now conditionally commits and closes connections only when it owns the connection, preventing premature closure of externally-managed connections. Frontend onboarding steps update useEffect dependency arrays and add lifecycle guards to prevent stale effects and memory leaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/OnboardingSteps/ServerCheck.tsx (1)
63-72: Fix: Add spaces after commas in dependency array.For consistency, format as:
[dispatch, stepIndex, ...]with spaces after each comma.🔎 Proposed fix
}, [ - dispatch, - stepIndex, + dispatch, + stepIndex, mainBackendSuccess, mainBackendLoading, mainBackendError, syncMicroserviceSuccess, syncMicroserviceLoading, syncMicroserviceError, ]);
🧹 Nitpick comments (3)
backend/app/database/face_clusters.py (2)
186-208: Consider API consistency for parameter type.The function accepts an optional
conn(Connection) parameter, while other functions in this file likedb_delete_all_clustersanddb_insert_clusters_batchaccept an optionalcursor(Cursor) parameter. Both approaches work correctly, but having a consistent API across similar functions would improve developer experience and reduce confusion.Consider aligning the parameter type in a future refactor, though this is not urgent and the current implementation functions correctly.
209-236: Add explicit error handling for consistency.The function lacks an explicit
exceptblock with rollback, unlike similar functions in this file (db_delete_all_clustersanddb_insert_clusters_batch). While SQLite automatically rolls back on connection close, explicit error handling improves code clarity and consistency.🔎 Proposed refactor to add explicit error handling
cursor = conn.cursor() try: # Build the update query dynamically based on provided parameters update_fields = [] update_values = [] if cluster_name is not None: update_fields.append("cluster_name = ?") update_values.append(cluster_name) if not update_fields: return False update_values.append(cluster_id) cursor.execute( f"UPDATE face_clusters SET {', '.join(update_fields)} WHERE cluster_id = ?", update_values, ) updated = cursor.rowcount > 0 if own_connection: conn.commit() return updated + except Exception: + if own_connection: + conn.rollback() + raise finally: if own_connection: conn.close()docs/frontend/state-management.md (1)
239-246: Consider adding formatting guidelines for dependency arrays.Since the code changes revealed consistent formatting issues (missing spaces after commas), consider adding a note about proper dependency array formatting:
- Format dependency arrays with spaces after commas: `[dispatch, stepIndex]`
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
backend/app/database/face_clusters.pydocs/frontend/state-management.mdfrontend/src/components/OnboardingSteps/AvatarSelectionStep.tsxfrontend/src/components/OnboardingSteps/FolderSetupStep.tsxfrontend/src/components/OnboardingSteps/ServerCheck.tsxfrontend/src/components/OnboardingSteps/ThemeSelectionStep.tsxfrontend/src/components/OnboardingSteps/UpdateStep.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
frontend/src/components/OnboardingSteps/UpdateStep.tsx (2)
frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
🔇 Additional comments (7)
backend/app/database/face_clusters.py (1)
229-230: LGTM! Core fix correctly addresses the connection lifecycle issue.The conditional commit and close operations properly respect connection ownership:
- When the function creates its own connection (
own_connection=True), it commits and closes appropriately- When an external connection is provided (
own_connection=False), the function preserves it for the caller to manageThis implementation successfully prevents premature closure of externally managed connections and maintains transaction integrity, directly addressing issue #824.
Also applies to: 234-235
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
33-37: Verify behavior when stepIndex changes while localStorage state persists.Similar to FolderSetupStep, if
stepIndexchanges while the component remains mounted and localStorage retains the'themeChosen'flag, the effect will incorrectly mark the new step as completed.Verify whether onboarding steps are unmounted between transitions or if the same component instance receives different stepIndex values.
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
36-42: Verify behavior when stepIndex changes while localStorage state persists.The effect marks
stepIndexas completed when both localStorage values exist. If the component receives a newstepIndexwhile these localStorage items persist, it could skip the wrong step.Verify the component lifecycle during onboarding navigation to ensure this scenario doesn't occur.
frontend/src/components/OnboardingSteps/UpdateStep.tsx (1)
26-44: LGTM: Mount guard pattern prevents memory leaks.The
isMountflag properly prevents state updates after component unmount, avoiding potential memory leaks and React warnings during async update checks. The cleanup function correctly resets the flag.frontend/src/components/OnboardingSteps/ServerCheck.tsx (1)
38-58: LGTM: Early returns prevent redundant dispatches.The early returns when loading (line 41) or on error (line 57) properly prevent the effect from continuing execution and dispatching conflicting actions. This is a clean pattern for managing the async health check states.
docs/frontend/state-management.md (1)
239-246: LGTM: Documentation aligns with implemented changes.The React Hooks Guidelines clearly explain the reasoning behind the useEffect dependency updates made throughout the onboarding components. This will help maintain consistency in future development.
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
33-37: This concern is not applicable to the current implementation.The component architecture prevents the scenario described. When
markCompleted(stepIndex)is dispatched (line 57), the Redux reducer updates bothcurrentStepIndexANDcurrentStepNameatomically (onboardingSlice.ts lines 40-41). TheOnboardingStepcomponent then switches which step component to render based onstepName(OnboardingStep.tsx lines 31-46). This causesFolderSetupStepto unmount entirely when transitioning to the next step, rather than remaining mounted with a newstepIndexprop. Additionally, once localStorage is set to 'true', the component returns null (line 65), preventing any further effect execution.
| dispatch(markCompleted(stepIndex)); | ||
| } | ||
| }, []); | ||
| }, [dispatch,stepIndex]); |
There was a problem hiding this comment.
Fix: Add space after comma in dependency array.
For consistency, format as: [dispatch, stepIndex] instead of [dispatch,stepIndex].
🔎 Proposed fix
- }, [dispatch,stepIndex]);
+ }, [dispatch, stepIndex]);📝 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.
| }, [dispatch,stepIndex]); | |
| }, [dispatch, stepIndex]); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx around line
42, the dependency array in the useEffect is missing a space after the comma:
change `[dispatch,stepIndex]` to `[dispatch, stepIndex]` to match project
formatting and consistency; update the array spacing and run/format to ensure
linting passes.
| dispatch(markCompleted(stepIndex)); | ||
| } | ||
| }, []); | ||
| }, [dispatch,stepIndex]); |
There was a problem hiding this comment.
Fix: Add space after comma in dependency array.
The dependency array should have proper spacing for consistency with coding standards: [dispatch, stepIndex] instead of [dispatch,stepIndex].
🔎 Proposed fix
- }, [dispatch,stepIndex]);
+ }, [dispatch, stepIndex]);📝 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.
| }, [dispatch,stepIndex]); | |
| }, [dispatch, stepIndex]); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/FolderSetupStep.tsx around line 37,
the dependency array in the useEffect is missing a space after the comma; update
the array to have a space for consistency and style (change
`[dispatch,stepIndex]` to `[dispatch, stepIndex]`) ensuring formatting matches
project linting and style rules.
| dispatch(markCompleted(stepIndex)); | ||
| } | ||
| }, []); | ||
| }, [dispatch,stepIndex]); |
There was a problem hiding this comment.
Fix: Add space after comma in dependency array.
Consistent with formatting standards, the dependency array should have proper spacing: [dispatch, stepIndex] instead of [dispatch,stepIndex].
🔎 Proposed fix
- }, [dispatch,stepIndex]);
+ }, [dispatch, stepIndex]);📝 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.
| }, [dispatch,stepIndex]); | |
| }, [dispatch, stepIndex]); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx around line
37, the dependency array currently reads "[dispatch,stepIndex]" missing a space
after the comma; update it to "[dispatch, stepIndex]" to match formatting
standards (add a single space after the comma in the dependency array).
| return () => { | ||
| isMount = false; | ||
| } | ||
| }, [dispatch,stepIndex, checkForUpdates]); |
There was a problem hiding this comment.
Fix: Add spaces after commas in dependency array.
Format as: [dispatch, stepIndex, checkForUpdates] instead of [dispatch,stepIndex, checkForUpdates].
🔎 Proposed fix
- }, [dispatch,stepIndex, checkForUpdates]);
+ }, [dispatch, stepIndex, checkForUpdates]);📝 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.
| }, [dispatch,stepIndex, checkForUpdates]); | |
| }, [dispatch, stepIndex, checkForUpdates]); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/UpdateStep.tsx around line 45, the
dependency array in the useEffect hook is missing a space after the first comma;
change the array from [dispatch,stepIndex, checkForUpdates] to [dispatch,
stepIndex, checkForUpdates] to match project formatting and maintain consistent
spacing after commas.
1 participant
Summary
This PR fixes an issue where
db_update_cluster()was unconditionally closing the database connection, even when the connection was provided by the caller.This caused unexpected connection closures and crashes in transaction-based workflows.
What was changed
commit()andclose()are only performed when the connection is created inside the function.Why this is needed
db_update_cluster()supports being called within an external transaction context. Closing externally supplied connections breaks transactional behavior and results in runtime errors such as:Summary
This PR fixes an issue where
db_update_cluster()was unconditionally closing the database connection, even when the connection was provided by the caller.This caused unexpected connection closures and crashes in transaction-based workflows.
What was changed
commit()andclose()are only performed when the connection is created inside the function.Why this is needed
db_update_cluster()supports being called within an external transaction context. Closing externally supplied connections breaks transactional behavior and results in runtime errors such as:How it was tested
db_update_cluster()calls using a shared connection.Checking
I run this command in python terminal and output is
Command : --
Before Check
After Check
Related Issue
Fixes #824
Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.