-
Notifications
You must be signed in to change notification settings - Fork 572
Change server port numbers #934
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
Change server port numbers #934
Conversation
|
|
|
Warning Rate limit exceeded@rahulharpal1603 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates hardcoded localhost ports from 8000/8001 to 52123/52124 across backend, sync-microservice, frontend, and documentation. Port 8000 becomes 52123 (backend service), and port 8001 becomes 52124 (sync-microservice). Additionally, a success toast notification is removed from the AI tagging mutation feedback in the frontend. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
|
1 similar 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: 1
🧹 Nitpick comments (3)
docs/Script_Setup_Guide.md (1)
96-100: Add language specifier to the code block.The fenced code block should specify
bashas the language for proper syntax highlighting and consistency with other code blocks in the document.🔎 Proposed fix
- ``` + ```bash pip install ruff black mypy pre-commit ```Based on static analysis hints.
backend/main.py (2)
93-93: Note: Formatting change unrelated to port migration.While this formatting change (single-line vs. multi-line path construction) is harmless, it's unrelated to the port migration objective. Consider keeping PRs focused on a single concern for easier review and rollback if needed.
123-124: Note: Router consolidation unrelated to port migration.These formatting changes (consolidating router includes to single lines) don't affect functionality but are unrelated to the port migration scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/app/config/settings.pybackend/main.pydocs/Manual_Setup_Guide.mddocs/Script_Setup_Guide.mddocs/backend/backend_python/openapi.jsonfrontend/src-tauri/tauri.conf.jsonfrontend/src/config/Backend.tsfrontend/src/hooks/useFolderOperations.tsxsync-microservice/README.mdsync-microservice/app/config/settings.pysync-microservice/main.py
🧰 Additional context used
🧬 Code graph analysis (2)
sync-microservice/app/config/settings.py (1)
frontend/src/config/Backend.ts (1)
SYNC_MICROSERVICE_URL(2-2)
backend/app/config/settings.py (1)
frontend/src/config/Backend.ts (1)
SYNC_MICROSERVICE_URL(2-2)
🪛 LanguageTool
docs/Script_Setup_Guide.md
[style] ~8-~8: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ...d in the video, please follow that step also. ### Prerequisites: - [NodeJS](https:...
(ALSO_AS_WELL)
🪛 markdownlint-cli2 (0.18.1)
docs/Script_Setup_Guide.md
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
🔇 Additional comments (17)
frontend/src-tauri/tauri.conf.json (1)
71-71: CSP port updates are correctly implemented and complete.The Content Security Policy has been properly updated to allow connections to the new backend ports (52123 for backend service, 52124 for sync-microservice) via both HTTP and WebSocket protocols. All old port references (8000, 8001) have been updated throughout the codebase—no remaining references found.
sync-microservice/main.py (1)
51-51: LGTM!The port change to 52124 is consistent with the
SYNC_MICROSERVICE_URLconfigurations across the codebase.docs/Manual_Setup_Guide.md (2)
101-104: LGTM!The backend documentation correctly reflects the new port configuration (52123) with explicit port specification in the startup command.
163-166: LGTM!The sync-microservice documentation correctly reflects the new port configuration (52124) with explicit port specification in the startup command.
sync-microservice/README.md (3)
44-44: LGTM!Startup command correctly updated to use port 52124.
150-150: LGTM!Health check URL correctly updated to the new port.
188-189: LGTM!The integration documentation clearly outlines the port assignments for both services, which will help developers understand the system architecture.
docs/Script_Setup_Guide.md (4)
54-54: LGTM!Backend startup command correctly updated for Windows to use port 52123.
62-62: LGTM!Backend startup command correctly updated for Linux to use port 52123.
74-74: LGTM!Sync-microservice startup command correctly updated for Windows to use port 52124.
82-82: LGTM!Sync-microservice startup command correctly updated for Linux to use port 52124.
backend/app/config/settings.py (1)
5-5: LGTM! All port references are updated consistently.The sync microservice URL update to port 52124 aligns with the corresponding port configuration in
sync-microservice/main.py(line 51). Verification confirms no old port references (8000, 8001) remain in the codebase, and all instances of ports 52123 (backend) and 52124 (sync microservice) are correctly configured across the backend, sync-microservice, frontend, and documentation files.sync-microservice/app/config/settings.py (1)
3-4: LGTM! Port configuration updated correctly.The port updates align with the coordinated migration across the project. The new ports (52123 for primary backend, 52124 for sync-microservice) are consistently applied.
backend/main.py (2)
72-72: LGTM! Server URL updated to new port.The FastAPI server configuration correctly reflects the new port (52123) for local development.
137-137: LGTM! Uvicorn port configuration updated correctly.The runtime port configuration matches the OpenAPI server URL definition on line 72.
docs/backend/backend_python/openapi.json (1)
14-14: LGTM! OpenAPI spec correctly reflects the new port.This auto-generated file correctly documents the updated server URL. Ensure this file is regenerated whenever the backend configuration changes.
frontend/src/config/Backend.ts (1)
1-2: LGTM! Port migration is correct.The port updates are complete. Verification confirms no remaining references to the old ports (8000/8001) exist throughout the codebase.
| useMutationFeedback(enableAITaggingMutation, { | ||
| showLoading: true, | ||
| loadingMessage: 'Enabling AI tagging', | ||
| successTitle: 'AI Tagging Enabled', | ||
| successMessage: 'AI tagging has been enabled for the selected folder.', | ||
| showSuccess: false, | ||
| errorTitle: 'AI Tagging Error', | ||
| errorMessage: 'Failed to enable AI tagging. Please try again.', | ||
| }); |
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.
🧩 Analysis chain
🏁 Script executed:
find frontend -name "useFolderOperations.tsx" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 104
🏁 Script executed:
cat -n frontend/src/hooks/useFolderOperations.tsx | head -150Repository: AOSSIE-Org/PictoPy
Length of output: 5953
Inconsistent success feedback: Enable AI tagging suppresses notifications while disable shows them.
The enable and disable AI tagging mutations have asymmetric feedback configurations:
- Enable AI tagging (lines 101-107):
showSuccess: false– no success notification - Disable AI tagging (lines 117-124):
successTitleandsuccessMessage– success notification shown
This inconsistency should be resolved: both operations should either display success feedback or both should suppress it for UX consistency.
🤖 Prompt for AI Agents
In frontend/src/hooks/useFolderOperations.tsx around lines 101 to 107, the
enable AI tagging mutation suppresses success feedback while the disable
mutation shows it; make them consistent by enabling success feedback for the
enable mutation: set showSuccess: true and add successTitle and successMessage
fields (matching the tone/content used for the disable mutation) so both
operations display equivalent success notifications.
* Update server port numbers to 52123 and 52124 across the application
Summary by CodeRabbit
Configuration
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.