Improve dev workflow by separating backend and Tauri frontend commands issue(fixes #837) #841
Conversation
📝 WalkthroughWalkthroughThis change set addresses React hooks best practices in onboarding components by correcting useEffect dependency arrays, adds conditional database connection handling in the backend, includes React hooks documentation guidelines, and introduces npm scripts for running backend and frontend services during development. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (1)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
36-42: LGTM! Clearer logic with proper dependencies.The changes improve readability by extracting localStorage checks into variables and correctly include
dispatchandstepIndexin the dependency array.Minor: Add space in dependency array for consistency
For consistency with other onboarding step files in this PR:
- }, [dispatch,stepIndex]); + }, [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 (8)
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.tsxpackage.json
🧰 Additional context used
🧬 Code graph analysis (3)
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)
frontend/src/components/OnboardingSteps/ServerCheck.tsx (2)
frontend/src/features/loaderSlice.ts (1)
hideLoader(21-24)frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
🔇 Additional comments (8)
backend/app/database/face_clusters.py (1)
229-235: LGTM! Consistent connection management pattern.The conditional commit and close operations now match the pattern used throughout this file (e.g.,
db_delete_all_clusters,db_insert_clusters_batch), ensuring proper resource management when using external vs. owned connections.frontend/src/components/OnboardingSteps/ServerCheck.tsx (3)
38-42: LGTM! Proper early return during loading state.The early return prevents dispatching
hideLoaderor other actions while still loading, which is the correct behavior.
43-57: LGTM! Proper error handling with early exit.The early return after initiating the exit prevents any subsequent logic from executing, which is appropriate given the error state.
63-72: LGTM! Comprehensive dependency array.The dependency array now includes all values used within the effect (
dispatch,stepIndex, and all query states), following React's Rules of Hooks and preventing stale closures.docs/frontend/state-management.md (1)
239-246: LGTM! Clear hooks guidelines.The documentation appropriately describes React hooks best practices and aligns with the dependency array updates made across the onboarding components in this PR.
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
33-37: LGTM! Proper dependency array for useEffect.The dependency array now includes
dispatchandstepIndex, ensuring the effect re-runs if the step index changes and following exhaustive-deps rules.frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
33-37: LGTM! Consistent dependency array pattern.Matches the pattern applied across other onboarding steps, properly including
dispatchandstepIndexin the dependency array.frontend/src/components/OnboardingSteps/UpdateStep.tsx (1)
25-45: LGTM! Proper async effect with unmount guard.The implementation correctly:
- Uses a mounted flag (
isMount) to prevent state updates after unmount- Includes cleanup function to set the flag to false
- Guards both the check initiation and completion dispatch with the mount flag
- Includes
checkForUpdatesfrom the custom hook in dependenciesThis prevents race conditions and "Can't perform a React state update on an unmounted component" warnings.
| "scripts": { | ||
| "backend": "cd backend && uvicorn main:app --reload", | ||
| "frontend": "cd frontend\\src-tauri && cargo tauri dev", | ||
| "linux-dev": "bash ./scripts/linux-dev.sh", | ||
| "win-dev": "cd scripts && win-dev.bat", | ||
| "prepare": "husky", | ||
| "lint:check": "cd frontend && eslint --max-warnings 0 --config .eslintrc.json .", | ||
| "lint:fix": "cd frontend && eslint --max-warnings 0 --config .eslintrc.json . --fix", | ||
| "format:fix": "cd frontend && prettier --write \"**/*.{ts,tsx,json}\"", | ||
| "format:check": "cd frontend && prettier --check \"**/*.{ts,tsx,json}\"", | ||
| "cargo:check": "cd backend && cargo check", | ||
| "setup": "cd scripts && node setup.js" | ||
| }, |
There was a problem hiding this comment.
Major: PR objectives not fully met - missing unified "dev" command and "concurrently" package.
The PR objectives explicitly state:
- "Provide root-level scripts (e.g., dev:backend, dev:frontend, and a combined dev)"
- "Enable a single entrypoint (e.g., npm run dev) using a cross-platform process runner (suggested: concurrently)"
Current implementation:
- ✅ Separate
backendandfrontendscripts provided - ❌ No unified
devscript to run both services together - ❌
concurrentlypackage not installed
🔎 Proposed fix to meet PR objectives
First, install concurrently:
npm install --save-dev concurrentlyThen update package.json:
"scripts": {
- "backend": "cd backend && uvicorn main:app --reload",
- "frontend": "cd frontend\\src-tauri && cargo tauri dev",
+ "dev": "concurrently \"npm run dev:backend\" \"npm run dev:frontend\"",
+ "dev:backend": "cd backend && uvicorn main:app --reload",
+ "dev:frontend": "cd frontend/src-tauri && cargo tauri dev",
"linux-dev": "bash ./scripts/linux-dev.sh",This provides:
- Individual commands:
npm run dev:backend,npm run dev:frontend - Unified command:
npm run dev(runs both with concurrently) - Cross-platform compatibility via concurrently
Note: The PR title mentions this "fixes #837" which specifically requested a unified workflow, so this is a blocker for the stated objectives.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
package.json lines 6-18: the PR missed adding a unified dev script and the
concurrently devDependency; install concurrently as a devDependency (npm install
--save-dev concurrently) and update the scripts section to expose dev:backend
and dev:frontend (reusing the existing backend and frontend commands) and add a
new dev script that runs both with concurrently (e.g., "dev": "concurrently
\"npm:dev:backend\" \"npm:dev:frontend\"") so a single npm run dev starts both
services cross-platform.
| }, | ||
| "scripts": { | ||
| "backend": "cd backend && uvicorn main:app --reload", | ||
| "frontend": "cd frontend\\src-tauri && cargo tauri dev", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for tauri.conf.json to understand the directory structure
fd -t f "tauri.conf.json"Repository: AOSSIE-Org/PictoPy
Length of output: 96
🏁 Script executed:
# Check the directory structure under frontend
fd -type d "frontend" -o -type d "src-tauri" | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 232
🏁 Script executed:
# List the actual directory structure
ls -la frontend 2>/dev/null | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 1279
🏁 Script executed:
# Check the full package.json to see all scripts and how paths are used
cat package.jsonRepository: AOSSIE-Org/PictoPy
Length of output: 920
🏁 Script executed:
# Check the content of tauri.conf.json to understand the project structure
cat frontend/src-tauri/tauri.conf.jsonRepository: AOSSIE-Org/PictoPy
Length of output: 1927
🏁 Script executed:
# Check if there's a Cargo.toml in the src-tauri directory
ls -la frontend/src-tauri/ | grep -E "Cargo|tauri"Repository: AOSSIE-Org/PictoPy
Length of output: 252
Cross-platform compatibility broken by Windows-specific path separator.
The frontend script uses backslashes (\\), which only work on Windows. This breaks compatibility on macOS and Linux.
- "frontend": "cd frontend\\src-tauri && cargo tauri dev",
+ "frontend": "cd frontend/src-tauri && cargo tauri dev",Forward slashes work on all platforms. The directory path frontend/src-tauri is correct (Cargo.toml and tauri.conf.json are located there).
📝 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.
| "frontend": "cd frontend\\src-tauri && cargo tauri dev", | |
| "frontend": "cd frontend/src-tauri && cargo tauri dev", |
🤖 Prompt for AI Agents
In package.json around line 8, the "frontend" npm script uses a Windows-only
backslash path ("cd frontend\\src-tauri && cargo tauri dev"); replace the
backslash with a forward slash so the script is cross-platform (use "cd
frontend/src-tauri && cargo tauri dev") or otherwise use a platform-agnostic
approach (e.g., run the command from the project root with the forward-slash
path). Ensure the script points to frontend/src-tauri exactly and works on
macOS/Linux and Windows.
|
@V4Vikaskumar why did you change the package manager from npm to pnpm |
|
@nugget-cloud you are right. now i fix this and i am working now. |
2 participants
Summary
This PR improves the local development experience by clearly defining separate commands for running the backend and the Tauri frontend.
Why this change
Earlier, the development workflow was not very clear for first-time contributors.
By using explicit and independent commands, the setup and execution process becomes easier to understand and follow.
How this helps new contributors
Overall benefit
These changes make project setup more straightforward and consistent, helping both new and existing contributors run the project smoothly with less effort.
After this Command
After.this.command.mp4
I experimented with creating a single command to run both the frontend and backend, but I failed every time, as repeated attempts caused issues such as infinite loops and improper shutdown handling.
Related Issue
Fixes #837
Summary by CodeRabbit
Documentation
Chores
npm run backend) and frontend (npm run frontend) environments.Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.