Fixed the Onboarding Page Setup#485
Conversation
WalkthroughAdds three new onboarding steps (avatar, folder, theme), new UI primitives (Label, RadioGroup) using Radix, extends onboarding Redux slice with avatar/name/theme/folders, updates step constants, centralizes step rendering, and adds Radix deps; Update-step behavior/code largely unchanged but is no longer ordered first. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant OnboardingStep
participant StepComponent
participant Redux as "Redux Store"
rect rgba(230,245,255,0.6)
note over OnboardingStep,StepComponent: New ordered flow (Avatar -> Folder -> Theme)
User->>OnboardingStep: Open onboarding
OnboardingStep->>StepComponent: Render current step (by name)
StepComponent->>Redux: Read state (name, avatar, folders, theme)
User->>StepComponent: Interact (select/input)
StepComponent->>Redux: Dispatch setAvatar/setName/addFolder/removeFolder/setTheme
User->>StepComponent: Click Next
StepComponent->>Redux: Dispatch markCompleted(stepIndex)
Redux-->>OnboardingStep: State update
OnboardingStep->>StepComponent: Render next step
end
sequenceDiagram
autonumber
actor User
participant OnboardingStep
participant UpdateStep
participant Redux as "Redux Store"
rect rgba(255,240,230,0.6)
note over OnboardingStep,UpdateStep: Expected: update check before other steps (per issue)
User->>OnboardingStep: Open onboarding
OnboardingStep-->>UpdateStep: (not included in VISIBLE_STEPS by default)
Note right of OnboardingStep: Current implementation renders Avatar first
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Could you remove the comments written in all 60 files, and only keep those comments that explain why something is there, not what that part of the code is doing? Sixty file changes are not needed to solve this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (24)
frontend/src/components/ui/Slider.tsx (1)
13-22: Bug: renders two thumbs by default for single-value slidersWhen neither value nor defaultValue is provided, _values becomes [min, max], causing two thumbs to render (range behavior). This diverges from Radix/ShadCN defaults and likely breaks uncontrolled single-value sliders.
Apply this fix to derive the thumb count instead of synthesizing values, and default to a single thumb:
- // Determine slider values (support both single and range sliders) - const _values = React.useMemo( - () => - Array.isArray(value) - ? value - : Array.isArray(defaultValue) - ? defaultValue - : [min, max], - [value, defaultValue, min, max], - ); + // Determine number of thumbs (support both single and range sliders) + const thumbCount = React.useMemo( + () => + Array.isArray(value) + ? value.length + : Array.isArray(defaultValue) + ? defaultValue.length + : 1, + [value, defaultValue], + ); @@ - {/* Thumb(s): draggable handle(s) */} - {Array.from({ length: _values.length }, (_, index) => ( + {/* Thumb(s): draggable handle(s) */} + {Array.from({ length: thumbCount }, (_, index) => ( <SliderPrimitive.Thumb data-slot="slider-thumb" key={index} - className="border-primary bg-background ring-ring/50 block size-4 shrink-0 rounded-full border shadow-sm transition-[color,box-shadow] hover:ring-4 focus-visible:ring-4 focus-visible:outline-hidden disabled:pointer-events-none disabled:opacity-50" + className="border-primary bg-background ring-ring/50 block size-4 shrink-0 rounded-full border shadow-sm transition-[color,box-shadow] hover:ring-4 focus-visible:ring-4 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50" /> ))}Note: focus-visible:outline-hidden is not a valid utility; replaced with outline-none.
Also applies to: 53-60
backend/app/database/faces.py (2)
12-23: faces table lacks uniqueness on image_id; current upsert intent will not workINSERT OR REPLACE only triggers on PRIMARY KEY/UNIQUE conflicts. Since image_id is not unique, you’ll accumulate duplicate rows per image. Add a unique index to enforce one row per image_id.
cursor.execute( """ CREATE TABLE IF NOT EXISTS faces ( id INTEGER PRIMARY KEY AUTOINCREMENT, image_id INTEGER, embeddings TEXT, FOREIGN KEY (image_id) REFERENCES image_id_mapping(id) ON DELETE CASCADE ) """ ) + # Ensure one row per image_id to support upserts + cursor.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS idx_faces_image_id ON faces(image_id)" + )Optional: enable foreign keys explicitly to ensure ON DELETE CASCADE works on SQLite connections.
- conn = sqlite3.connect(DATABASE_PATH) + conn = sqlite3.connect(DATABASE_PATH) + conn.execute("PRAGMA foreign_keys = ON")
43-51: Fix upsert: use ON CONFLICT(image_id) DO UPDATE to truly update existing rowsWith the unique index in place, switch to a proper upsert so embeddings are updated instead of creating new rows.
- cursor.execute( - """ - INSERT OR REPLACE INTO faces (image_id, embeddings) - VALUES (?, ?) - """, - (image_id, embeddings_json), - ) + cursor.execute( + """ + INSERT INTO faces (image_id, embeddings) + VALUES (?, ?) + ON CONFLICT(image_id) DO UPDATE SET + embeddings=excluded.embeddings + """, + (image_id, embeddings_json), + )frontend/src/components/Updater/UpdateDialog.tsx (1)
155-157: Retry button is disabled when error is present — users cannot retry.disabled={isDownloading || !!error} prevents clicking “Retry” exactly when error is shown. Allow retry while not downloading.
Apply:
- disabled={isDownloading || !!error} + disabled={isDownloading}frontend/src/contexts/ThemeContext.tsx (1)
30-58: Unify theme persistence across ThemeContext and onboarding sliceThe ThemeContext uses
storageKey = 'vite-ui-theme'while the onboarding slice and ThemeSelectionStep read/write'theme'directly. This leads to two separate sources of truth and duplicate persistence logic. Please consolidate:• Export a single
THEME_STORAGE_KEYconstant and use it everywhere instead of hard-coding.
• Updatefrontend/src/features/onboardingSlice.ts(currently at line 22) to use that key.
• InThemeSelectionStep.tsx, replacelocalStorage.getItem('theme')/.setItem('theme', …)with the shared key or invokeThemeContext.setTheme.
• Ensure onboarding dispatches to the same ThemeContext (or vice versa) so setting the theme updates both the Redux slice and the DOM-class effect in one flow.This will guarantee a single source of truth for theme state and avoid drift between contexts.
backend/app/routes/test.py (3)
27-31: Use get_running_loop() instead of get_event_loop() inside async context.asyncio.get_event_loop() is discouraged in asyncio coroutines and may behave unexpectedly in newer Python versions. Use asyncio.get_running_loop() to reliably access the current loop.
-async def run_get_classes(img_path): - loop = asyncio.get_event_loop() - await loop.run_in_executor(None, get_classes, img_path) +async def run_get_classes(img_path): + loop = asyncio.get_running_loop() + await loop.run_in_executor(None, get_classes, img_path)
46-56: 400 errors are being converted to 500 due to broad exception handling.Raising HTTPException(status_code=400, ...) inside the try block is immediately caught by the broad except Exception and returned as 500, masking client errors. Re-raise HTTPException separately.
if img is None: # Image path is invalid or unreadable raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, message=f"Failed to load image: {img_path}", error="Failed to load image", ).model_dump(), ) @@ - except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, error="Internal server error", message=str(e) - ).model_dump(), - ) + except HTTPException: + # Preserve intended HTTP errors (e.g., 400) + raise + except Exception as e: + # Unexpected server error + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, error="Internal server error", message=str(e) + ).model_dump(), + )Also applies to: 69-75
121-131: Do not swallow 400 errors; ensure destination dir exists and handle collisions.
- Same HTTPException masking issue as above.
- Ensure IMAGES_PATH exists.
- Consider guarding against overwriting existing files.
def add_single_image(payload: AddSingleImageRequest): try: image_path = payload.path # Check if path exists and is a file if not os.path.isfile(image_path): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, error="Invalid file path", message="The provided path is not a valid file", ).model_dump(), ) image_extensions = [".jpg", ".jpeg", ".png", ".bmp", ".gif"] file_extension = os.path.splitext(image_path)[1].lower() if file_extension not in image_extensions: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, error="Invalid file type", message="The file is not a supported image type", ), ) # Copy image to gallery folder - destination_path = os.path.join(IMAGES_PATH, os.path.basename(image_path)) - shutil.copy(image_path, destination_path) + os.makedirs(IMAGES_PATH, exist_ok=True) + filename = os.path.basename(image_path) + destination_path = os.path.join(IMAGES_PATH, filename) + if os.path.exists(destination_path): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="File already exists", + message=f"'{filename}' already exists in the gallery", + ).model_dump(), + ) + shutil.copy(image_path, destination_path) return AddSingleImageResponse( success=True, message="Image copied to the gallery successfully", data={"destination_path": destination_path}, ) - - except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, message="Internal server error", error=str(e) - ).model_dump(), - ) + except HTTPException: + raise + except Exception as e: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, message="Internal server error", error=str(e) + ).model_dump(), + )Also applies to: 135-143, 145-153, 155-161
frontend/src/components/ui/scroll-area.tsx (1)
30-57: Incorrect Radix component identifiers (will not render).Use ScrollAreaPrimitive.Scrollbar and ScrollAreaPrimitive.Thumb at runtime. The current ScrollAreaPrimitive.ScrollAreaScrollbar and ScrollAreaPrimitive.ScrollAreaThumb names are type-only in some examples and are not valid component identifiers.
Apply this fix:
-function ScrollBar({ +function ScrollBar({ className, orientation = 'vertical', ...props -}: React.ComponentProps<typeof ScrollAreaPrimitive.ScrollAreaScrollbar>) { +}: React.ComponentProps<typeof ScrollAreaPrimitive.Scrollbar>) { return ( - <ScrollAreaPrimitive.ScrollAreaScrollbar + <ScrollAreaPrimitive.Scrollbar data-slot="scroll-area-scrollbar" orientation={orientation} className={cn( 'flex touch-none p-px transition-colors select-none', orientation === 'vertical' && 'h-full w-2.5 border-l border-l-transparent', orientation === 'horizontal' && 'h-2.5 flex-col border-t border-t-transparent', className, )} {...props} > {/* The draggable thumb inside scrollbar */} - <ScrollAreaPrimitive.ScrollAreaThumb + <ScrollAreaPrimitive.Thumb data-slot="scroll-area-thumb" className="bg-border relative flex-1 rounded-full" /> - </ScrollAreaPrimitive.ScrollAreaScrollbar> + </ScrollAreaPrimitive.Scrollbar> ); }backend/app/database/folders.py (1)
99-106: Unclosed connection in get_all_folder_idsget_all_folder_ids opens a connection but never closes it. Use a context manager or close explicitly.
Apply this diff:
-def get_all_folder_ids(): - # Return list of all folder IDs - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - cursor.execute("SELECT folder_id from folders") - rows = cursor.fetchall() - return [row[0] for row in rows] if rows else [] +def get_all_folder_ids(): + # Return list of all folder IDs + with sqlite3.connect(DATABASE_PATH) as conn: + rows = conn.execute("SELECT folder_id FROM folders").fetchall() + return [row[0] for row in rows] if rows else []frontend/src/components/ui/dropdown-menu.tsx (3)
64-86: Add ref forwarding and real styles; implement variant/inset supportCurrent wrapper doesn’t forward refs and uses placeholder classes. Also, the destructive variant is surfaced via data-variant but not styled.
Apply this diff:
-function DropdownMenuItem({ - className, - inset, - variant = 'default', - ...props -}: React.ComponentProps<typeof DropdownMenuPrimitive.Item> & { - inset?: boolean; - variant?: 'default' | 'destructive'; -}) { - return ( - <DropdownMenuPrimitive.Item - data-slot="dropdown-menu-item" - data-inset={inset} - data-variant={variant} - className={cn( - 'focus:bg-accent ...', // active, disabled, inset, icon styles - className, - )} - {...props} - /> - ); -} +const DropdownMenuItem = React.forwardRef< + React.ElementRef<typeof DropdownMenuPrimitive.Item>, + React.ComponentPropsWithoutRef<typeof DropdownMenuPrimitive.Item> & { + inset?: boolean; + variant?: 'default' | 'destructive'; + } +>(({ className, inset, variant = 'default', ...props }, ref) => ( + <DropdownMenuPrimitive.Item + ref={ref} + data-slot="dropdown-menu-item" + data-inset={inset} + data-variant={variant} + className={cn( + 'relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none ' + + 'focus:bg-accent focus:text-accent-foreground ' + + 'data-[disabled]:pointer-events-none data-[disabled]:opacity-50 ' + + 'data-[inset]:pl-8 ' + + 'data-[variant=destructive]:text-destructive', + className, + )} + {...props} + /> +)); +DropdownMenuItem.displayName = 'DropdownMenuItem';
131-145: Fix radio item styling ("...") and padding for left indicatorSame issue as checkbox: ensure adequate padding and remove placeholder class.
- <DropdownMenuPrimitive.RadioItem + <DropdownMenuPrimitive.RadioItem data-slot="dropdown-menu-radio-item" - className={cn('focus:bg-accent ...', className)} + className={cn( + 'relative flex cursor-default select-none items-center rounded-sm py-1.5 pl-8 pr-2 text-sm outline-none ' + + 'focus:bg-accent focus:text-accent-foreground ' + + 'data-[disabled]:pointer-events-none data-[disabled]:opacity-50', + className, + )}
207-226: SubTrigger styling still contains placeholder; add open-state and inset stylesProvide real classes and support for open state visuals.
- <DropdownMenuPrimitive.SubTrigger + <DropdownMenuPrimitive.SubTrigger data-slot="dropdown-menu-sub-trigger" data-inset={inset} - className={cn('focus:bg-accent data-[inset]:pl-8 ...', className)} + className={cn( + 'flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none ' + + 'focus:bg-accent data-[state=open]:bg-accent ' + + 'data-[inset]:pl-8', + className, + )}frontend/src/App.tsx (1)
3-3: Incorrect import: BrowserRouter must come from react-router-domImporting BrowserRouter from 'react-router' will fail. It’s provided by 'react-router-dom'.
Apply this diff:
-import { BrowserRouter } from 'react-router'; +import { BrowserRouter } from 'react-router-dom';frontend/src/components/FolderPicker/DeleteSelectedImagePage.tsx (1)
70-91: Confirm handler re-opens the dialog; close it on confirm to avoid a stuck modal.handleAddSelectedImages sets openDialog(true) even though it is the confirm handler. This can keep the dialog open unexpectedly and conflicts with user expectations.
Apply this diff to stop reopening and to close on success:
- // Final delete function after dialog confirm + // Final delete function after dialog confirm const handleAddSelectedImages = async (isFromDevice: boolean) => { - setOpenDialog(true); + // Dialog is already open; do not reopen here console.log('Selected Images = ', selectedImages); if (isFromDevice) { console.log('Yes , Want to delete from this Device too'); } else { console.log('Only want to delete from this Application'); } if (selectedImages.length > 0) { try { await deleteMultipleImages({ paths: selectedImages, isFromDevice }); console.log('Selected Images : ', selectedImages); + setOpenDialog(false); setSelectedImages([]); if (!isLoading) { setIsVisibleSelectedImage(true); } } catch (err) { onError('Error during deleting images', err); } } };frontend/src/components/Album/AlbumForm.tsx (1)
42-57: Avoid duplicate form reset/close; rely on onSuccess or awaited resultWith mutateAsync, await the call and let onSuccess handle cleanup. Remove the immediate resets/close to prevent double side-effects.
const handleCreateAlbum = async () => { if (newAlbumName.trim()) { try { - createAlbum({ + await createAlbum({ name: newAlbumName.trim(), description: newAlbumDescription.trim(), is_hidden: isHidden, password: isHidden ? password : '', // Only send password if hidden }); - // Reset form - setNewAlbumName(''); - setNewAlbumDescription(''); - setPassword(''); - setIsHidden(false); - closeForm(); } catch (err) { onError('Error Creating Album', err); } } else { onError('Invalid Album Name', new Error('Album name cannot be empty')); // Validation check } };frontend/src/components/Album/Album.tsx (3)
99-105: Security concern: Password validation should be done server-sideUsing
prompt()for password input and comparing passwords client-side exposes the password in JavaScript. This is insecure as the password can be intercepted or viewed in browser dev tools.The password validation should be performed server-side. Consider:
- Send the password to the backend for validation
- Backend returns an access token or session
- Use that token for subsequent requests to view the album
- if (album?.is_hidden) { - const password = prompt('Enter the password for this hidden album:'); - if (password === album.password) { - setCurrentAlbum(albumId); - } else { - alert('Incorrect password.'); - } + if (album?.is_hidden) { + const password = prompt('Enter the password for this hidden album:'); + // Call backend to verify password + try { + const response = await verifyAlbumAccess({ + album_name: albumId, + password + }); + if (response.success) { + setCurrentAlbum(albumId); + } else { + alert('Incorrect password.'); + } + } catch (err) { + showErrorDialog('Authentication Failed', err); + }
100-100: Replace browser alerts with proper UI componentsUsing
prompt()andalert()provides poor UX and looks unprofessional. Consider using proper dialog components that match your UI design.Consider creating a password dialog component:
// PasswordDialog.tsx interface PasswordDialogProps { isOpen: boolean; albumName: string; onSubmit: (password: string) => void; onCancel: () => void; } // Then use it: {passwordDialogAlbum && ( <PasswordDialog isOpen={!!passwordDialogAlbum} albumName={passwordDialogAlbum} onSubmit={handlePasswordSubmit} onCancel={() => setPasswordDialogAlbum(null)} /> )}Also applies to: 104-104
112-118: Fix async/await usage in handleDeleteAlbumThe function is declared as
asyncbut usesawaiton a mutation function that doesn't return a promise in the expected way. ThedeleteAlbummutation function fromusePictoMutationshould be called withoutawait.- const handleDeleteAlbum = async (albumId: string) => { - try { - await deleteAlbum({ name: albumId }); - } catch (err) { - showErrorDialog('Error Deleting Album', err); - } + const handleDeleteAlbum = (albumId: string) => { + deleteAlbum( + { name: albumId }, + { + onError: (err) => { + showErrorDialog('Error Deleting Album', err); + } + } + );frontend/src/components/Album/ImageManagementDialog.tsx (1)
49-58: Fix async/await usage with mutationSimilar to the issue in Album.tsx, the mutation is being awaited incorrectly. The
mutatefunction fromusePictoMutationdoesn't return a promise by default.- const handleRemoveImage = async (imageUrl: string) => { + const handleRemoveImage = (imageUrl: string) => { if (albumName) { - try { - await removeImage({ album_name: albumName, path: imageUrl }); - onSuccess(); - } catch (err) { - onError('Error Removing Image', err); - } + removeImage( + { album_name: albumName, path: imageUrl }, + { + onSuccess: () => onSuccess(), + onError: (err) => onError('Error Removing Image', err) + } + ); } };frontend/src/components/Media/MediaGallery.tsx (2)
166-181: MediaView receives a stale itemsPerPage (always 20) instead of the selected page sizeYou define and use
pageNoas the effective page size across the component, but pass the constantitemsPerPage(20) toMediaView. This desynchronizes navigation within the viewer.- itemsPerPage={itemsPerPage} + itemsPerPage={pageNo}
127-161: Align DropdownMenu import with your UI wrapperMediaGallery.tsx is importing Radix primitives directly, but the app uses the ShadCN wrapper in
src/components/ui/dropdown-menu.tsx. Update the import at the top of the file:
- File:
frontend/src/components/Media/MediaGallery.tsx(around lines 15–19)Replace:
import { DropdownMenu, DropdownMenuContent, DropdownMenuRadioGroup, DropdownMenuRadioItem, DropdownMenuTrigger, } from '@radix-ui/react-dropdown-menu';With:
import { DropdownMenu, DropdownMenuContent, DropdownMenuRadioGroup, DropdownMenuRadioItem, DropdownMenuTrigger, } from '@/components/ui/dropdown-menu';frontend/src/components/AITagging/AIgallery.tsx (1)
7-8: Rename hook file and update imports to include missing “n”The hook file is currently named
useQueryExtensio.ts(missing the final “n”), so although the import in AIgallery.tsx resolves today, it’s clearly a typo that will confuse future maintainers and could break if the file gets renamed elsewhere.• Rename:
frontend/src/hooks/useQueryExtensio.ts→frontend/src/hooks/useQueryExtension.ts
• Update imports:- In
frontend/src/components/AITagging/AIgallery.tsx, change- import { usePictoQuery } from '@/hooks/useQueryExtensio'; + import { usePictoQuery } from '@/hooks/useQueryExtension';- Search and update any other references to
@/hooks/useQueryExtensioacross the codebase.frontend/src/components/AITagging/FilterControls.tsx (1)
333-359: Using RadioGroup for multi-select is semantically wrong and breaks onValueChangeYou’re preventing default selection on radio items to emulate multi-select; this means
onValueChangewon’t fire reliably, and accessibility is degraded. UseDropdownMenuCheckboxItemfor multi-select.- <DropdownMenuRadioGroup - className="overflow-auto" - onValueChange={handleFilterFlag} - > - {selectedFlags.map((ele, index) => ( - <DropdownMenuRadioItem - key={ele.tag} - value={ele.tag} - onSelect={(event) => { - selectedFlags[index].isChecked - ? handleRemoveFlag(index) - : handleAddFlag(index); - event.preventDefault(); - }} - className="cursor-pointer" - > - <input - type="checkbox" - className="mr-2 cursor-pointer" - value={ele.tag} - checked={selectedFlags[index].isChecked} - /> - {ele.tag} - </DropdownMenuRadioItem> - ))} - </DropdownMenuRadioGroup> + <div className="overflow-auto p-1"> + {selectedFlags.map((ele, index) => ( + <DropdownMenuCheckboxItem + key={ele.tag} + checked={ele.isChecked} + onCheckedChange={(checked) => { + checked ? handleAddFlag(index) : handleRemoveFlag(index); + // Apply filters immediately after toggle + handleFilterFlag(); + }} + className="cursor-pointer" + > + {ele.tag} + </DropdownMenuCheckboxItem> + ))} + </div>Note: Make sure
DropdownMenuCheckboxItemis exported from your../ui/dropdown-menuwrapper.
|
@rahulharpal1603 I will make the required changes. |
Hi, |
|
@rahulharpal1603 Sorry for the delay , will be completing it in the next 2-3 days at max . |
ssz2605
force-pushed
the
onboarding-layout
branch
from
c97b429 to
d42d220
Compare
|
@rahulharpal1603 Now can you please review the changes. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)
27-35: Stop passing onNext/onBack to children to fix TS prop errorsAvatarSelectionStep, FolderSetupStep, and ThemeSelectionStep don’t declare onNext/onBack props. Passing them causes TypeScript errors at these call sites. Minimal safe fix: remove onNext/onBack from sharedProps.
const sharedProps = { stepIndex, totalSteps: VISIBLE_STEPS.length, - onNext, - onBack, };Optional cleanup (only if the parent doesn’t rely on these): also drop onNext/onBack from the component props and destructuring to avoid unused-prop lint warnings.
interface OnboardingStepProps { stepIndex: number; stepName: string; - onNext: () => void; - onBack: () => void; } ... export const OnboardingStep: React.FC<OnboardingStepProps> = ({ stepIndex, stepName, - onNext, - onBack, }) => {frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
86-92: Replace alert() with inline validation for better UX and testsBlocking alerts are jarring and hard to test. Use local state to show an inline error.
+ const [validationError, setValidationError] = useState<string>(''); const handleNextClick = () => { if (name && selectedAvatar) { dispatch(markCompleted(stepIndex)); + setValidationError(''); } else { - alert('Please enter your name and select an avatar.'); + setValidationError('Please enter your name and select an avatar.'); } };Add an inline message under the inputs:
@@ {/* Name Input */} <div className="mb-5"> @@ </div> + {validationError && ( + <p className="text-destructive text-xs mb-3">{validationError}</p> + )}Also clear the error on change:
- const handleAvatarSelect = (avatar: string) => { - dispatch(setAvatar(avatar)); - }; + const handleAvatarSelect = (avatar: string) => { + dispatch(setAvatar(avatar)); + setValidationError(''); + }; @@ - const handleNameChange = (value: string) => { - dispatch(setName(value)); - }; + const handleNameChange = (value: string) => { + dispatch(setName(value)); + setValidationError(''); + };
5-10: Fix incorrect store import pathRootState should come from '@/app/store' in this codebase. Current path likely fails module resolution.
-import { RootState } from '@/store'; +import { RootState } from '@/app/store';Optional: also type dispatch for better inference.
- const dispatch = useDispatch(); + const dispatch = useDispatch<AppDispatch>();(Requires: import { AppDispatch } from '@/app/store')
🧹 Nitpick comments (6)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (2)
50-53: Wrapper width conflicts with child layout (max-w-4xl vs. max-w-7xl)Children render their own responsive two-panel layouts sized for max-w-7xl. Constraining them inside max-w-4xl can clip or squash the design. Let the step component own its layout.
- return ( - <div className="bg-background text-foreground flex min-h-screen w-full items-center justify-center p-10"> - <div className="w-full max-w-4xl">{renderStepComponent()}</div> - </div> - ); + return renderStepComponent();Alternatively, bump the wrapper to max-w-7xl to match children.
45-47: Provide a meaningful fallback for unknown stepNameReturning an empty div hampers debugging. Render a lightweight fallback (or null) with a console.warn.
- default: - return <div></div>; + default: + console.warn('Unknown onboarding step:', stepName); + return null;frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
145-149: Missing base border utility; border-muted won’t render without borderTailwind’s color utilities don’t add a border by themselves. Add the base border class so the unselected avatar has a visible outline.
- className={`bg-background relative inline-flex h-20 w-20 items-center justify-center rounded-full transition-all duration-300 ${ + className={`bg-background relative inline-flex h-20 w-20 items-center justify-center rounded-full transition-all duration-300 border ${ isSelected ? 'border-primary ring-primary ring-offset-background ring-2 ring-offset-2' : 'border-muted' }`}
141-159: Improve keyboard and screen reader accessibility for avatar choicesThese act like a single-select control. Consider RadioGroup semantics (you already ship a RadioGroup component) or add appropriate ARIA:
- Wrap items in a role="radiogroup"
- Each button: role="radio", aria-checked={isSelected}, aria-label={
Avatar ${i+1}}- Support keyboard navigation (ArrowLeft/Right)
I can provide a patch if you want to adopt the RadioGroup version.
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
126-147: Use stable keys and add accessible labels for remove buttonsIndex keys can cause reconciliation bugs when items are removed. Use the folder path as the key. Also add an aria-label to the remove control.
- {folders.map((folder, index) => ( + {folders.map((folder) => ( <div - key={index} + key={folder} className="bg-muted flex items-center justify-between rounded-md px-4 py-2 text-sm" > @@ <button type="button" onClick={() => handleRemoveFolder(folder)} - className="text-destructive hover:text-red-600" + className="text-destructive hover:text-red-600" + aria-label={`Remove folder ${folder}`} >
151-162: Disable “Next” until at least one folder is selectedPrevents accidental progression with an empty selection and aligns UX with validation in other steps.
- <Button onClick={handleNext} className="px-6 py-3 text-base"> + <Button + onClick={handleNext} + className="px-6 py-3 text-base" + disabled={folders.length === 0} + > Next </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (9)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/avatars/avatar1.pngis excluded by!**/*.pngfrontend/public/avatars/avatar2.pngis excluded by!**/*.pngfrontend/public/avatars/avatar3.pngis excluded by!**/*.pngfrontend/public/avatars/avatar4.pngis excluded by!**/*.pngfrontend/public/avatars/avatar5.pngis excluded by!**/*.pngfrontend/public/avatars/avatar6.pngis excluded by!**/*.pngfrontend/public/avatars/avatar7.pngis excluded by!**/*.pngfrontend/public/avatars/avatar8.pngis excluded by!**/*.png
📒 Files selected for processing (10)
frontend/package.json(1 hunks)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/OnboardingStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/UpdateStep.tsx(2 hunks)frontend/src/components/ui/label.tsx(1 hunks)frontend/src/components/ui/radio-group.tsx(1 hunks)frontend/src/constants/steps.ts(1 hunks)frontend/src/features/onboardingSlice.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/components/ui/radio-group.tsx
- frontend/src/features/onboardingSlice.ts
- frontend/src/constants/steps.ts
- frontend/src/components/ui/label.tsx
- frontend/package.json
- frontend/src/components/OnboardingSteps/UpdateStep.tsx
- frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
frontend/src/app/store.ts (2)
AppDispatch(14-14)RootState(12-12)frontend/src/features/onboardingSlice.ts (4)
addFolder(38-42)removeFolder(43-47)markCompleted(48-59)previousStep(60-67)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (4)
frontend/src/constants/steps.ts (1)
STEPS(1-5)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
AvatarSelectionStep(57-202)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
FolderSetupStep(52-196)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
ThemeSelectionStep(52-204)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
frontend/src/app/store.ts (1)
RootState(12-12)frontend/src/features/onboardingSlice.ts (3)
setAvatar(29-31)setName(32-34)markCompleted(48-59)
🪛 GitHub Check: Linting
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
[warning] 176-176:
Headings must have content and the content must be accessible by a screen reader
⏰ 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). (2)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Tests
🔇 Additional comments (1)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
70-74: Confirm placeholder folder selection won’t shipThe mock path generation is fine for scaffolding, but ensure a real folder picker (or platform-native dialog) replaces this before release.
Would you like me to open a follow-up issue with acceptance criteria for the real folder picker (including multi-select, duplicates handling, and error states)?
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
Outdated
Show resolved
Hide resolved
I have requested some small changes; please apply those changes. |
|
@ssz2605 any updates? This was the final review, after the changes are made I will be merging the PR. |
|
@rahulharpal1603 I'm currently working on the changes and will update the PR soon. Apologies for the delay, I’ve been tied up with a hectic schedule. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
5-5: Incorrect store import pathImport RootState/AppDispatch from '@/app/store' to match the codebase.
-import { AppDispatch, RootState } from '@/store'; +import { AppDispatch, RootState } from '@/app/store';frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)
31-36: Stop passing unsupported onNext/onBack to childrenChild steps don’t declare onNext/onBack props; spreading them causes TS errors. Keep them on the wrapper if needed, but don’t spread to steps.
const sharedProps = { stepIndex, totalSteps: VISIBLE_STEPS.length, - onNext, - onBack, };frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
5-5: Incorrect store import pathUse '@/app/store' for RootState.
-import { RootState } from '@/store'; +import { RootState } from '@/app/store';frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
15-15: Fix incorrect store import pathRootState and AppDispatch should come from '@/app/store'.
-import { RootState, AppDispatch } from '@/store'; +import { RootState, AppDispatch } from '@/app/store';
🧹 Nitpick comments (7)
frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
50-52: Fix empty heading to satisfy a11y lint ruleThe self-closing h2 renders no accessible name and triggers the linter warning. Remove it or keep the CardHeader without a heading.
Apply this diff:
- <CardHeader className="p-6 pb-4"> - <h2 className="text-center text-xl font-semibold" /> - </CardHeader> + <CardHeader className="p-6 pb-4" />frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
39-39: Remove unused statefeatureIndex/setFeatureIndex are not used; drop the state to silence lint.
- const [featureIndex, setFeatureIndex] = useState(0);
56-69: “System” theme should react to OS changesWhen selectedTheme === 'system', attach a matchMedia listener to update on OS theme changes. Optional but improves UX.
useEffect(() => { if (selectedTheme === 'system') { - const isDark = window.matchMedia('(prefers-color-scheme: dark)').matches; - document.documentElement.setAttribute( - 'data-theme', - isDark ? 'dark' : 'light', - ); + const mql = window.matchMedia('(prefers-color-scheme: dark)'); + const apply = () => + document.documentElement.setAttribute('data-theme', mql.matches ? 'dark' : 'light'); + apply(); + mql.addEventListener?.('change', apply); + return () => mql.removeEventListener?.('change', apply); } else if (selectedTheme) { document.documentElement.setAttribute('data-theme', selectedTheme); } if (selectedTheme) { localStorage.setItem('theme', selectedTheme); } }, [selectedTheme]);frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
13-13: Drop unused CardContent importIt’s not used in this component.
-import { Card, CardHeader, CardContent } from '@/components/ui/card'; +import { Card, CardHeader } from '@/components/ui/card';
53-59: Replace alert() with inline validation for better UXToast or inline error beats a blocking alert. Minimal inline approach below.
- const handleNextClick = () => { + const [validationError, setValidationError] = React.useState(''); + const handleNextClick = () => { if (name && selectedAvatar) { dispatch(markCompleted(stepIndex)); + setValidationError(''); } else { - alert('Please enter your name and select an avatar.'); + setValidationError('Please enter your name and select an avatar.'); } };Render under the inputs (e.g., after the avatar grid):
</div> </div> + {validationError && ( + <p className="text-destructive mt-2 text-sm" role="alert" aria-live="polite"> + {validationError} + </p> + )}frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
97-101: Use stable key for folder list itemsKeys should be stable and unique; folder path is unique already.
- {folders.map((folder, index) => ( + {folders.map((folder) => ( <div - key={index} + key={folder} className="bg-muted flex items-center justify-between rounded-md px-4 py-2 text-sm" >
37-40: Follow-up: Replace mock folder selectionThe mock path is fine for scaffolding, but wire this to the real folder picker (e.g., Tauri dialog) before shipping.
I can draft a small adapter around a Tauri/IPC folder picker and update this handler; want me to open a task and provide a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/OnboardingStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/appFeatures.tsx(1 hunks)frontend/src/constants/steps.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/constants/steps.ts
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
frontend/src/components/ui/card.tsx (3)
Card(85-85)CardHeader(86-86)CardContent(91-91)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (3)
frontend/src/app/store.ts (2)
AppDispatch(14-14)RootState(12-12)frontend/src/features/onboardingSlice.ts (3)
setTheme(35-37)markCompleted(48-59)previousStep(60-67)frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
AppFeatures(36-76)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (3)
frontend/src/app/store.ts (2)
AppDispatch(14-14)RootState(12-12)frontend/src/features/onboardingSlice.ts (4)
addFolder(38-42)removeFolder(43-47)markCompleted(48-59)previousStep(60-67)frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
AppFeatures(36-76)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (5)
frontend/src/constants/steps.ts (1)
STEPS(1-6)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
AvatarSelectionStep(35-145)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
FolderSetupStep(30-137)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
ThemeSelectionStep(30-151)frontend/src/components/OnboardingSteps/UpdateStep.tsx (1)
UpdateStep(12-57)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (3)
frontend/src/app/store.ts (1)
RootState(12-12)frontend/src/features/onboardingSlice.ts (3)
setAvatar(29-31)setName(32-34)markCompleted(48-59)frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
AppFeatures(36-76)
🪛 GitHub Check: Linting
frontend/src/components/OnboardingSteps/appFeatures.tsx
[warning] 51-51:
Headings must have content and the content must be accessible by a screen reader
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
[warning] 39-39:
'setFeatureIndex' is assigned a value but never used
[warning] 39-39:
'featureIndex' is assigned a value but never used
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
[warning] 13-13:
'CardContent' is defined but never used
[warning] 3-3:
'useState' is defined but never used
🪛 GitHub Actions: PR Check Build
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
[error] 3-3: TS6133: 'useState' is declared but its value is never read. Step: npm run tauri build.
⏰ 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). (1)
- GitHub Check: Tauri Tests
🔇 Additional comments (2)
frontend/src/components/OnboardingSteps/appFeatures.tsx (1)
48-75: LGTM overallAuto-rotating features with cleanup is correct; modulo logic prevents OOB access. Nice reuse across steps.
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)
19-23: The script will print out the top ~200 lines of the OnboardingStep component so we can verify how progress (and totalSteps) are calculated and whether it uses the same STEP_NAMES list. Once we have that, we can confirm if the update step ordering or progress‐bar logic needs adjusting. Please share the script output so we can complete the verification.
|
@ssz2605 Thanks for the contribution! |
|
Thanks for your guidance in resolving this issue, @rahulharpal1603. |
2 participants
Implemented Complete Onboarding Flow: Avatar, Name, Folder, and Theme Steps
This PR implements the full onboarding flow for PictoPy, including AvatarandNameSelection, FolderSetupStep, and ThemeSelection, fully aligned with Redux state management.
Key Changes
Avatar & Name Selection
Theme Selection
Folder Setup Step
SetUpScreeninside FolderSetupStep to use Redux for state management, keeping consistency across onboarding steps.Onboarding Steps Integration
AvatarSelectionandThemeSelection) alongside existing steps likeFolderSetupStep.STEPSarray inSteps.tsxto include the new steps, maintaining correct flow order.Onboarding Slice Updates
OnboardingStateand reducers inonboardingSliceto handleavatarandname.closes #473
Summary by CodeRabbit