ISSUE - 1369: BUG FIX: Settings page does not update active YOLO model after changing it in Model Manager#1371
Open
Takitxt wants to merge 2 commits into
Open
ISSUE - 1369: BUG FIX: Settings page does not update active YOLO model after changing it in Model Manager#1371Takitxt wants to merge 2 commits into
Takitxt wants to merge 2 commits into
Conversation
Contributor
WalkthroughThis PR expands Tauri permissions and adds cross-window synchronization between the Model Manager window and the Settings page. Closing Model Manager now emits a ChangesModel Manager to Settings sync
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ModelManagerWindow
participant TauriEventBus
participant SettingsPage
participant Backend
User->>ModelManagerWindow: Close window
ModelManagerWindow->>TauriEventBus: emit("models-updated")
TauriEventBus->>SettingsPage: deliver models-updated
SettingsPage->>Backend: fetch /models/status
Backend-->>SettingsPage: installed tiers data
SettingsPage->>SettingsPage: refetch() preferences
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (1)
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared fetch logic to eliminate duplication.
The new event-listener effect duplicates the model-status fetch and
setInstalledTierslogic already in the mount effect (lines 44–80). The existing version has more robust handling (AbortController, error states, loading states); the new one omits those, creating divergent behavior for the same operation. Extract a sharedfetchAndUpdateInstalledTiershelper to keep them consistent.♻️ Proposed refactor: shared fetch helper
+ const fetchAndUpdateInstalledTiers = async (signal?: AbortSignal) => { + const res = await fetch(`${BACKEND_URL}/models/status`, signal ? { signal } : undefined); + if (!res.ok) throw new Error(`Failed to load models (${res.status})`); + const data: ModelStatusResponse = await res.json(); + if (data.success && data.data) { + setInstalledTiers(getInstalledModelTiers(data.data)); + } + }; + useEffect(() => { const controller = new AbortController(); - const fetchModelStatus = async () => { - try { - const res = await fetch(`${BACKEND_URL}/models/status`, { - signal: controller.signal, - }); - if (!res.ok) { - throw new Error(`Failed to load models (${res.status})`); - } - const data: ModelStatusResponse = await res.json(); - if (controller.signal.aborted) return; - if (data.success && data.data) { - setInstalledTiers(getInstalledModelTiers(data.data)); - setTierFetchError(null); - } else { - setTierFetchError('Failed to load models'); - } - } catch (err) { + const fetchModelStatus = async () => { + try { + await fetchAndUpdateInstalledTiers(controller.signal); setTierFetchError(null); + } catch (err) { // ... existing error handling } finally { ... } }; // ... }, []); useEffect(() => { const unlistenPromise = listen('models-updated', async () => { try { - const res = await fetch(`${BACKEND_URL}/models/status`); - if (res.ok) { - const data: ModelStatusResponse = await res.json(); - if (data.success && data.data) { - setInstalledTiers(getInstalledModelTiers(data.data)); - } - } + await fetchAndUpdateInstalledTiers(); } catch (err) { console.error('Failed to refresh model status', err); } refetch(); }); // ... }, [refetch]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx` around lines 94 - 106, The models-updated listener in UserPreferencesCard duplicates the same model-status fetch and setInstalledTiers flow already handled in the mount effect. Extract that logic into a shared fetchAndUpdateInstalledTiers helper used by both the initial load effect and the listen callback, and keep the existing AbortController/loading/error handling from the mount path consistent in the shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/ModelManager/ModelManager.tsx`:
- Around line 74-87: The on-close flow in ModelManager’s useEffect is racing
window teardown because the onCloseRequested callback awaits
emit('models-updated') without preventing the default close. Update the handler
to call event.preventDefault() first, then await the emit, and only after that
explicitly destroy the current window using the existing getCurrentWindow() flow
so Settings reliably receives the event; keep the cleanup/unlisten logic
unchanged.
In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx`:
- Line 107: The `refetch()` call in `UserPreferencesCard` is a floating promise
and may reject without being handled. Update the callback that triggers
`refetch()` to either await the promise or explicitly handle its rejection with
a catch path, using the `refetch` function in `UserPreferencesCard` so failures
from `getUserPreferences` do not become unhandled promise rejections.
---
Nitpick comments:
In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx`:
- Around line 94-106: The models-updated listener in UserPreferencesCard
duplicates the same model-status fetch and setInstalledTiers flow already
handled in the mount effect. Extract that logic into a shared
fetchAndUpdateInstalledTiers helper used by both the initial load effect and the
listen callback, and keep the existing AbortController/loading/error handling
from the mount path consistent in the shared implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98191bcf-9041-49bf-9e05-a6613004cc51
📒 Files selected for processing (4)
frontend/src-tauri/capabilities/model_manager.jsonfrontend/src/hooks/useUserPreferences.tsxfrontend/src/pages/ModelManager/ModelManager.tsxfrontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx
Takitxt
changed the title
Author
|
@rohan-pandeyy Can you go through my PR for issue number 1369. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #(#1369)
Screenshots/Recordings:
Pictopy Before:
Pictopy.Before.mov
Pictopy After:
Pictopy.After.mov
Additional Notes:
TOTAL NUMBER OF FILES CHANGED : 4
TOTAL NUMBER OF LINES OF CODE WRITTEN : 38
NUMBER OF CHANGES AND DELETION MADE : 0
Problem Statement:
PictoPy lets users configure which YOLO model is used for object detection, from a dropdown in Settings.
To manage those models, Settings has a "Configure..." option that opens a separate Model Manager window.
Bug:
When a user downloaded a new model tier or changed the active tier inside Model Manager,
then closed that window and returned to Settings, the Settings dropdown did not reflect the change.
The user is in the need to refresh to see changes.
Expected behavior:
Closing Model Manager should instantly refresh Settings.
Reason for updating files:
Settings and Model Manager are not the same window layouts. They are different windows altogether with different rust commands.
In order to make a connection between the two so that they can communicate with each other, I have used emit() function.
FILE 1 CHANGES:
Name of File: frontend/src/pages/modelmanager/ModelManager.tsx
Line 74 to 89.
FILE 2 CHANGES:
Name of File: frontend/src/hooks/useUserPrefrences.tsx
FILE 3 CHANGES:
Name of File: frontend/src/settingsPage/components/UserPreferencesCard.tsx
LINE 94 - 113.
FILE 4 CHANGES:
Name of File: frontend/src-tauri/capabilities/ModelManager.tsx
LINE 6 - 8.
**It was done for the closing of the ModelManager window **
Conclusion:
My code does not throw any error or stop any api calls but i still want the moderator or mentor to go through my code and PR.
My solution fixes the bug issue - 1369
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes