-
Notifications
You must be signed in to change notification settings - Fork 348
Issue #511: refactor(rust): streamline backend to minimal API and update docs structure #515
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
Issue #511: refactor(rust): streamline backend to minimal API and update docs structure #515
Conversation
WalkthroughThe PR removes most Tauri frontend modules, services, repositories, utils, and tests; prunes the Tauri CI test job; preserves only Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Frontend (UI)
participant Tauri as Tauri Bridge
participant Svc as services::get_server_path
participant FS as App Resources
UI->>Tauri: invoke("get_server_path")
Tauri->>Svc: call get_server_path(handle)
Svc->>FS: AppHandle.path().resolve(..., BaseDirectory::Resource)
FS-->>Svc: path string / error
Svc-->>Tauri: Result<String, String>
Tauri-->>UI: Path or error
note over UI,Svc: All other previous Tauri commands removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (3)
docs/frontend/screenshots.md (1)
1-26: Screenshots doc reads well; add brief accessibility alt text context.Alt texts exist; consider adding a short context note (e.g., “people shown are stock photos”) if applicable, and verify image paths resolve.
docs/backend/backend_rust/api.md (1)
17-20: Prefer typed invoke example and minimal error handling.Use a typed invoke to match the
Result<String, String>and show a quick try/catch.-const serverPath = await invoke("get_server_path"); -console.log("Server path:", serverPath); +try { + const serverPath = await invoke<string>("get_server_path"); + console.log("Server path:", serverPath); +} catch (err) { + console.error("Failed to get server path:", err); +}docs/frontend/state-management.md (1)
224-235: Persist example references rootReducer without showing its definition.Either define
rootReduceror show how to wrap the combined reducers you already list in the store setup.-const persistedReducer = persistReducer(persistConfig, rootReducer); +import { combineReducers } from '@reduxjs/toolkit'; +const rootReducer = combineReducers({ + media: mediaReducer, + ui: uiReducer, + albums: albumsReducer, + settings: settingsReducer, +}); +const persistedReducer = persistReducer(persistConfig, rootReducer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/assets/screenshots/ai-tagging.pngis excluded by!**/*.pngdocs/assets/screenshots/main-gallery.pngis excluded by!**/*.pngdocs/assets/screenshots/settings.pngis excluded by!**/*.png
📒 Files selected for processing (19)
.github/workflows/pr-check-tests.yml(0 hunks)docs/backend/backend_rust/api.md(1 hunks)docs/frontend/gallery-view.md(0 hunks)docs/frontend/screenshots.md(1 hunks)docs/frontend/state-management.md(1 hunks)docs/frontend/ui-components.md(0 hunks)frontend/src-tauri/src/lib.rs(0 hunks)frontend/src-tauri/src/main.rs(1 hunks)frontend/src-tauri/src/models.rs(0 hunks)frontend/src-tauri/src/repositories/cache_repository.rs(0 hunks)frontend/src-tauri/src/repositories/file_repository.rs(0 hunks)frontend/src-tauri/src/repositories/mod.rs(0 hunks)frontend/src-tauri/src/services/cache_service.rs(0 hunks)frontend/src-tauri/src/services/file_service.rs(0 hunks)frontend/src-tauri/src/services/mod.rs(0 hunks)frontend/src-tauri/src/utils/file_utils.rs(0 hunks)frontend/src-tauri/src/utils/mod.rs(0 hunks)frontend/src-tauri/tests/mod_test.rs(0 hunks)mkdocs.yml(1 hunks)
💤 Files with no reviewable changes (14)
- frontend/src-tauri/src/repositories/mod.rs
- docs/frontend/gallery-view.md
- frontend/src-tauri/src/models.rs
- frontend/src-tauri/src/utils/file_utils.rs
- frontend/src-tauri/src/lib.rs
- frontend/src-tauri/src/repositories/file_repository.rs
- docs/frontend/ui-components.md
- frontend/src-tauri/src/utils/mod.rs
- frontend/src-tauri/src/services/cache_service.rs
- frontend/src-tauri/src/services/file_service.rs
- frontend/src-tauri/tests/mod_test.rs
- frontend/src-tauri/src/repositories/cache_repository.rs
- frontend/src-tauri/src/services/mod.rs
- .github/workflows/pr-check-tests.yml
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
🔇 Additional comments (3)
mkdocs.yml (1)
74-74: Nav entry for Screenshots looks good; please verify assets exist.Confirm that
docs/assets/screenshots/{main-gallery.png,ai-tagging.png,settings.png}are present to avoid broken links at build time.frontend/src-tauri/src/main.rs (2)
24-24: Only exposing get_server_path via invoke_handler: LGTM.Nice pruning of the public surface.
24-24: Ensure the command is annotated and compiles without missing imports.Verify
services::get_server_pathis marked with#[tauri::command]and either imports or fully-qualifiesBaseDirectory. Otherwisegenerate_handler!will fail orBaseDirectorywon’t resolve.Apply this in
frontend/src-tauri/src/services/mod.rsif needed:use tauri::path::BaseDirectory; use tauri::AppHandle; #[tauri::command] pub fn get_server_path(handle: AppHandle) -> Result<String, String> { let resource_path = handle .path() .resolve("resources/backend", BaseDirectory::Resource) .map_err(|e| e.to_string())?; Ok(resource_path.to_string_lossy().to_string()) }
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 (2)
docs/frontend/state-management.md (2)
162-181: Tighten the createAlbum stub: types and stable IDs.Type the thunk and use crypto.randomUUID() for IDs.
Apply this diff:
-export const createAlbum = createAsyncThunk( +export const createAlbum = createAsyncThunk<Album, NewAlbum>( 'albums/create', async (albumData: NewAlbum, { rejectWithValue }) => { try { // Mock implementation - replace with actual backend call when available - const album = await new Promise(resolve => + const album = await new Promise<Album>(resolve => setTimeout(() => resolve({ - id: Date.now().toString(), + id: crypto.randomUUID(), name: albumData.name, description: albumData.description, createdAt: new Date().toISOString(), photos: [] }), 1000) ); return album;
149-160: Import completeness for examples.Examples should include their imports to be copypasta‑ready.
Apply this diff if you keep a separate “Async Thunks” snippet (in addition to Store Setup):
-```typescript -export const loadServerPath = createAsyncThunk<string, void>( +```typescript +import { createAsyncThunk } from '@reduxjs/toolkit'; +import { invoke } from '@tauri-apps/api/core'; + +export const loadServerPath = createAsyncThunk<string, void>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/backend/backend_rust/api.md(1 hunks)docs/frontend/state-management.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/backend/backend_rust/api.md
🔇 Additional comments (1)
docs/frontend/state-management.md (1)
97-107: Fix slice mismatch: use Settings slice for serverPath, align thunk + selector, and import invoke from @tauri-apps/api/tauri.Docs reference state.server/serverSlice; change to state.settings/settingsSlice, add serverPath/loading/error to SettingsState, and change the thunk to 'settings/loadServerPath' — import invoke from '@tauri-apps/api/tauri' (Tauri v2).
Apply this diff to the shown snippets:
@@ interface SettingsState { directories: string[]; autoBackup: boolean; compressionLevel: number; thumbnailSize: 'small' | 'medium' | 'large'; secureFolder: { enabled: boolean; path: string; }; + serverPath: string | null; + loading: boolean; + error: string | null; } @@ -```typescript -export const loadServerPath = createAsyncThunk( - 'server/loadPath', +```typescript +import { createAsyncThunk } from '@reduxjs/toolkit'; +import { invoke } from '@tauri-apps/api/tauri'; + +export const loadServerPath = createAsyncThunk<string, void>( + 'settings/loadServerPath', + async (_, { rejectWithValue }) => { + try { + const serverPath = await invoke<string>('get_server_path'); + return serverPath; + } catch (error) { + return rejectWithValue((error as Error).message); + } + } +);@@
-typescript -import { useSelector, useDispatch } from 'react-redux'; -import { RootState, AppDispatch } from '../store'; -import { loadServerPath } from '../store/slices/serverSlice'; +typescript
+import { useSelector, useDispatch } from 'react-redux';
+import { RootState, AppDispatch } from '../store';
+import { loadServerPath } from '../store/slices/settingsSlice';
@@
const ServerPathComponent = () => {
const dispatch = useDispatch();
- const { serverPath, loading } = useSelector((state: RootState) => state.server);
- const { serverPath, loading } = useSelector((state: RootState) => state.settings);
const { viewMode } = useSelector((state: RootState) => state.ui);Also applies to: 149-160, 190-206 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
rahulharpal1603
left a 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.
@codiphile I have requested two changes, please resolve those and update the PR
|
@codiphile While taking the screenshots, add all the 40 photos(10 each for 4 people) into a single folder only. There is an issue going on with multiple folders right now. Add this single folder and enable AI tagging on it. |
f589c41 to
00d7a55
Compare
7d4a8cc to
3100427
Compare
rahulharpal1603
left a 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.
LGTM! I have made some minor changes to the state management docs.
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 (2)
docs/frontend/state-management.md (1)
180-199: Use typed hooks in examples for consistency.Switch to useAppDispatch/useAppSelector per your Typed Hooks section to keep examples aligned.
-import { useSelector, useDispatch } from "react-redux"; -import { RootState, AppDispatch } from "../app/store"; +import { useAppDispatch, useAppSelector } from "../app/hooks"; import { setImages, nextImage } from "../features/imageSlice"; import { showLoader, hideLoader } from "../features/loaderSlice"; const ImageViewer = () => { - const dispatch = useDispatch<AppDispatch>(); - const { images, currentViewIndex } = useSelector( - (state: RootState) => state.images - ); - const { loading, message } = useSelector((state: RootState) => state.loader); + const dispatch = useAppDispatch(); + const { images, currentViewIndex } = useAppSelector((state) => state.images); + const { loading, message } = useAppSelector((state) => state.loader);docs/backend/backend_rust/api.md (1)
3-6: Clarify return type semantics for frontend consumers.“Returns: Result<String, String>” is Rust-side. Consider noting the frontend receives a Promise and briefly listing error cases returned in Err(String).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
docs/assets/screenshots/ai-tagging.pngis excluded by!**/*.pngdocs/assets/screenshots/ai-tagging2.pngis excluded by!**/*.pngdocs/assets/screenshots/home.pngis excluded by!**/*.pngdocs/assets/screenshots/settings.pngis excluded by!**/*.pngfrontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.github/workflows/pr-check-tests.yml(0 hunks)docs/backend/backend_rust/api.md(1 hunks)docs/frontend/gallery-view.md(0 hunks)docs/frontend/screenshots.md(1 hunks)docs/frontend/state-management.md(1 hunks)docs/frontend/ui-components.md(0 hunks)frontend/src-tauri/src/lib.rs(0 hunks)frontend/src-tauri/src/main.rs(1 hunks)frontend/src-tauri/src/models.rs(0 hunks)frontend/src-tauri/src/services/cache_service.rs(0 hunks)frontend/src-tauri/src/services/file_service.rs(0 hunks)frontend/src-tauri/src/services/mod.rs(0 hunks)frontend/src-tauri/src/utils/file_utils.rs(0 hunks)frontend/src-tauri/src/utils/mod.rs(0 hunks)frontend/src-tauri/tests/mod_test.rs(0 hunks)mkdocs.yml(1 hunks)
💤 Files with no reviewable changes (11)
- frontend/src-tauri/src/lib.rs
- docs/frontend/gallery-view.md
- frontend/src-tauri/src/utils/mod.rs
- .github/workflows/pr-check-tests.yml
- frontend/src-tauri/src/services/cache_service.rs
- frontend/src-tauri/src/services/file_service.rs
- frontend/src-tauri/src/services/mod.rs
- frontend/src-tauri/tests/mod_test.rs
- frontend/src-tauri/src/models.rs
- docs/frontend/ui-components.md
- frontend/src-tauri/src/utils/file_utils.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/frontend/screenshots.md
- frontend/src-tauri/src/main.rs
- mkdocs.yml
🔇 Additional comments (5)
docs/frontend/state-management.md (4)
150-171: Consider documenting redux-persist if the app persists state.If persistence is in use, wire it in the Store Setup and ignore all persist actions to silence warnings. Otherwise, add a note that persistence is intentionally omitted.
Apply if using redux-persist:
-import { configureStore } from "@reduxjs/toolkit"; +import { configureStore, combineReducers } from "@reduxjs/toolkit"; +import { + persistReducer, + persistStore, + FLUSH, + REHYDRATE, + PAUSE, + PERSIST, + PURGE, + REGISTER, +} from "redux-persist"; +import storage from "redux-persist/lib/storage"; import loaderReducer from "@/features/loaderSlice"; import onboardingReducer from "@/features/onboardingSlice"; import imageReducer from "@/features/imageSlice"; import faceClustersReducer from "@/features/faceClustersSlice"; import infoDialogReducer from "@/features/infoDialogSlice"; import folderReducer from "@/features/folderSlice"; -export const store = configureStore({ - reducer: { - loader: loaderReducer, - onboarding: onboardingReducer, - images: imageReducer, - faceClusters: faceClustersReducer, - infoDialog: infoDialogReducer, - folders: folderReducer, - }, -}); +const rootReducer = combineReducers({ + loader: loaderReducer, + onboarding: onboardingReducer, + images: imageReducer, + faceClusters: faceClustersReducer, + infoDialog: infoDialogReducer, + folders: folderReducer, +}); + +const persistConfig = { key: "root", storage, whitelist: ["onboarding", "folders"] }; +const persistedReducer = persistReducer(persistConfig, rootReducer); + +export const store = configureStore({ + reducer: persistedReducer, + middleware: (getDefaultMiddleware) => + getDefaultMiddleware({ + serializableCheck: { ignoredActions: [FLUSH, REHYDRATE, PAUSE, PERSIST, PURGE, REGISTER] }, + }), +}); +export const persistor = persistStore(store); export type RootState = ReturnType<typeof store.getState>; export type AppDispatch = typeof store.dispatch;Also verify "@/features" alias exists:
#!/bin/bash set -euo pipefail echo "== Check TS path aliases for @/* ==" fd -a tsconfig*.json | while read -r f; do echo "--- $f ---" cat "$f" done | rg -n '"paths"\s*:\s*\{\s*"@/\*"\s*:\s*\[\s*".+/\*"\s*\]' echo echo "== Confirm usage of redux-persist in app ==" rg -n --type=ts --type=tsx -C2 'redux-persist|persistReducer|persistStore'
205-211: Typed hooks section looks good.
1-14: Past backend-integration issues are resolved in this doc.No examples reference removed Tauri commands; the guide sticks to Redux-only patterns.
Also applies to: 146-149, 174-178
19-46: Ensure slice APIs in docs/frontend/state-management.md match frontend/features exports.Action names and state shapes (e.g., closeImageView, removeFolders, updateClusterName, markCompleted, setImages, addImages, setCurrentViewIndex, nextImage, previousImage, updateImage, removeImage, setError, clearImages, setFolders, addFolder, updateFolder, clearFolders, setClusters) must exactly match the exported actions/state in the real slices; automated sandbox search couldn't confirm these exports (rg returned no matches/unrecognized file types). Manually verify the slice exports in frontend/features and update the doc to avoid doc–code drift.
docs/backend/backend_rust/api.md (1)
24-26: LGTM: cross‑platform note is now accurate and consistent.The unified AppHandle.path().resolve(..., BaseDirectory::Resource) wording correctly reflects actual behavior.
refactor: Streamline Rust backend to minimal API and update documentation structure
Overview
This PR significantly streamlines the Rust backend by removing unused functionality and simplifies the documentation structure to focus on the current Redux-based architecture.
Rust Backend Cleanup
Documentation Restructure
Summary by CodeRabbit