settings page#169
Conversation
WalkthroughThis update introduces a comprehensive settings feature for the Metagram platform, including a main settings page, account info flow, notification toggles, data and storage management, and logout functionality. It adds new Svelte components, updates navigation and layout logic, and refines UI elements for improved responsiveness and user experience. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant Router
participant SettingsNavigationButton
participant SettingsToggleButton
User->>SettingsPage: Navigates to /settings
SettingsPage->>SettingsNavigationButton: Renders navigation buttons
User->>SettingsNavigationButton: Clicks on a navigation button
SettingsNavigationButton->>Router: Navigates to selected settings subpage
User->>SettingsToggleButton: Toggles a notification setting
SettingsToggleButton-->>SettingsPage: Updates toggle state (UI only)
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
platforms/metagram/src/routes/(protected)/+layout.svelte (2)
14-28:⚠️ Potential issueSame route matching issues as SideBar component.
This has the same substring matching problem as identified in the SideBar component, where
includes()could match unintended routes.Apply the same fix as suggested for the SideBar component to use precise route matching based on path segments.
42-42:⚠️ Potential issueHardcoded placeholder path in settings navigation.
Same issue as in SideBar - the
/settings/asdfpath contains placeholder text.Replace with the actual settings route:
-onclick={() => goto('settings/asdf')} +onclick={() => goto('/settings')}
🧹 Nitpick comments (5)
platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte (1)
1-19: LGTM! Well-structured responsive component.The component follows Svelte 5 best practices with proper TypeScript interfaces, responsive design, and flexible content rendering via snippets.
Consider the optional chaining on line 16 - while
asideContent?.()protects against undefined, theasideContentprop is required in the interface, so this might be unnecessary defensive coding:- {@render asideContent?.()} + {@render asideContent()}platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.stories.ts (1)
43-44: Remove unnecessary trailing empty lines.Consider removing the extra empty lines at the end of the file for cleaner formatting.
- -platforms/metagram/src/lib/fragments/BottomNav/BottomNav.svelte (1)
20-20: Potential redundancy: fullPath appears identical to _activeTab.Both
fullPathand_activeTabare derived frompage.url.pathname. Consider if both are necessary or if one can be removed for simplicity.If they serve the same purpose, consider consolidating:
- let _activeTab = $derived(page.url.pathname); - let fullPath = $derived(page.url.pathname); + let _activeTab = $derived(page.url.pathname);Then use
_activeTabin place offullPathon line 111.platforms/metagram/src/routes/(protected)/settings/[id]/+layout.svelte (1)
17-17: Remove commented code.Clean up the commented code blocks for better maintainability.
- <!-- <div class="block h-[100dvh] grid-cols-[auto_31vw] md:grid"> --> <div class="flex flex-col gap-5 px-5 py-2 md:gap-8"> ... </div> - <!-- </div> -->Also applies to: 104-104
platforms/metagram/src/routes/(protected)/+layout.svelte (1)
53-57: Placeholder content in RightAside component.The RightAside component contains placeholder text ("adcas", "sdfgdsf") which should be replaced with actual content before production.
Would you like me to help generate appropriate content for the RightAside component or create a TODO to track this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte(1 hunks)platforms/metagram/src/lib/fragments/BottomNav/BottomNav.svelte(7 hunks)platforms/metagram/src/lib/fragments/Header/Header.svelte(2 hunks)platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.stories.ts(2 hunks)platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte(2 hunks)platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte(7 hunks)platforms/metagram/src/lib/fragments/index.ts(1 hunks)platforms/metagram/src/lib/icons/Camera.svelte(1 hunks)platforms/metagram/src/lib/icons/Settings.svelte(1 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/post/+page.svelte(0 hunks)platforms/metagram/src/routes/(protected)/settings/[id]/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/[id]/account/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/[id]/support/+page.svelte(1 hunks)platforms/metagram/src/routes/+layout.svelte(1 hunks)
💤 Files with no reviewable changes (1)
- platforms/metagram/src/routes/(protected)/post/+page.svelte
🔇 Additional comments (26)
infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte (8)
2-5: Consistent import indentation
The adjustment to indent all imports by one level under<script>enhances readability and aligns with project conventions.
7-9: Uniform function block formatting
Indenting thehandleFinishdeclaration and its body consistently improves visual hierarchy within the<script>block.
12-14: Multiline<main>tag formatting
Breaking the<main>element’s attributes onto separate lines with consistent indentation makes the layout easier to scan and maintain.
16-20: Component prop alignment for<Hero>
Aligning each prop on its own line under the<Hero>tag provides clearer visibility of property values and simplifies future edits.
21-29: Multiline object literal in<IdentityCard>
Reformatting theuserDataobject to multiline syntax with aligned keys and values greatly enhances readability of the sample data.
33-36: Paragraph text indentation
Indenting each line of the descriptive<p>block uniformly under its parent<section>improves text block consistency.
38-38: Single-line component indentation
Adjusting the second<IdentityCard>to match surrounding indent levels keeps the markup balanced and predictable.
41-41: Closing tag alignment
Ensuring the closing</main>tag aligns with its opening tag reaffirms the structure and nesting of the component.platforms/metagram/src/lib/icons/Camera.svelte (1)
11-11:Details
❓ Verification inconclusive
Verify the prop usage change is intentional.
The SVG fill attribute now uses the
colorprop instead of thefillprop. This is a breaking change that could affect existing usage of this component.
🏁 Script executed:
#!/bin/bash # Description: Check all usages of the Camera icon to ensure they're compatible with the new prop structure # Expected: Verify that components using Camera icon are using the 'color' prop appropriately echo "Searching for Camera icon usage..." rg -A 3 -B 3 "Camera" --type svelte echo "Searching for fill prop usage with Camera..." rg -A 5 -B 5 "fill.*Camera|Camera.*fill"Length of output: 287
#!/bin/bash
Re-run search for component usage and prop passes with correct file globs
echo "🔍 Searching for Camera component usages in .svelte files..."
rg -g '*.svelte' '<Camera\b' -necho "🔍 Searching for fill prop passed to Camera..."
rg -g '.svelte' '<Camera[^>]\bfill=' -necho "🔍 Searching for color prop passed to Camera..."
rg -g '.svelte' '<Camera[^>]\bcolor=' -necho "🔍 Searching for Camera references in JS/TS files..."
rg -g '*.{js,ts}' '\bCamera\b' -n
#!/bin/bash
Re-run searches with explicit glob patterns since ripgrep has no built-in svelte type
echo "🔍 Searching for Camera imports in .svelte, .js, .ts files..."
rg -g '*.{svelte,js,ts}' 'import.*Camera' -necho "🔍 Searching for <Camera usage in Svelte templates..."
rg -g '*.svelte' '<Camera\b' -necho "🔍 Searching for fill= prop passed to Camera..."
rg -g '.svelte' '<Camera[^>]\bfill=' -necho "🔍 Searching for color= prop passed to Camera..."
rg -g '.svelte' '<Camera[^>]\bcolor=' -necho "🔍 Searching for dynamic component references (Camera) in JS/TS..."
rg -g '*.{js,ts}' '\bCamera\b' -n
Verify Camera icon prop change
This update makes the SVG’sfillattribute driven by thecolorprop instead of thefillprop—a breaking API change. Please manually confirm that all existing<Camera>usages have been updated to usecolorand notfill.
- Search for
<Camera … fill=occurrences in.svelte,.js, and.tsfiles.- Replace any
fill=prop withcolor=where appropriate.- Verify dynamic or programmatic references to the component (e.g. in storybooks, tests, or tooling) are updated.
platforms/metagram/src/routes/+layout.svelte (1)
39-39:Details
❓ Verification inconclusive
Verify mobile layout spacing after removing horizontal padding.
Removing the
px-4class eliminates horizontal padding on small screens. Please ensure that content doesn't touch the screen edges and maintains proper readability on mobile devices.
🏁 Script executed:
#!/bin/bash # Description: Check for other layout components that might provide horizontal spacing # Expected: Find components that handle mobile spacing or confirm this change is intentional # Search for padding classes in layout-related files rg -A 3 -B 3 "px-[0-9]|pl-[0-9]|pr-[0-9]" --type svelte platforms/metagram/src/routes/Length of output: 119
Let’s retry searching without the unsupported
--typeflag to locate any horizontal padding classes in Svelte files:
🏁 Script executed:
#!/bin/bash # Search all .svelte files for any px- classes, with 3 lines of context rg -g '*.svelte' -n -C 3 'px-[0-9]' platforms/metagram/src/routes/Length of output: 1214
Verify mobile layout spacing
I searched all route layouts and found no other horizontal padding on small screens—only
md:px-0remains in+layout.svelte, and the only otherpx-usage is in the protected settings sublayout. Removing the earlierpx-4means most pages will now be edge-to-edge on mobile.• File: platforms/metagram/src/routes/+layout.svelte, line 39
<main class="h-[100dvh] md:px-0">Please confirm this change is intentional or consider re-adding a small-breakpoint padding (e.g.
px-4 md:px-0) to prevent content from touching the screen edges.platforms/metagram/src/lib/fragments/index.ts (1)
11-11: LGTM!Clean addition of the new
RightAsidecomponent export following the established pattern.platforms/metagram/src/lib/fragments/Header/Header.svelte (2)
22-22: LGTM!Adding vertical padding to the primary variant text improves visual balance.
47-49:Details
❓ Verification inconclusive
Verify header layout across all variants and screen sizes.
This represents a significant change from simple padding (
p-4) to a complex responsive layout with margins, borders, and conditional styling. Please ensure:
- All header variants (primary, secondary, tertiary) render correctly
- The responsive border behavior works as expected across breakpoints
- The new spacing doesn't conflict with existing page layouts
🏁 Script executed:
#!/bin/bash # Description: Find all usages of Header component to verify impact # Expected: List all files using Header component for testing verification # Search for Header component usage rg -A 5 "import.*Header" --type svelte platforms/metagram/src/ rg -A 2 "<Header" --type svelte platforms/metagram/src/Length of output: 184
🏁 Script executed:
#!/bin/bash # Description: Find all usages of the Header component in .svelte files # Search for Header imports in Svelte files rg "import\s.*Header" -G '*.svelte' platforms/metagram/src/ # Search for Header component tags in Svelte templates rg "<Header" -G '*.svelte' platforms/metagram/src/Length of output: 924
🏁 Script executed:
#!/bin/bash # Description: Find all usages of the Header component in .svelte files # Using ripgrep with correct file globbing rg -H -g '*.svelte' "import\s.*Header" platforms/metagram/src/ rg -H -g '*.svelte' "<Header" platforms/metagram/src/ # Fallback with find + grep if ripgrep globbing fails find platforms/metagram/src/ -type f -name '*.svelte' -exec grep -H "import\s.*Header" {} + find platforms/metagram/src/ -type f -name '*.svelte' -exec grep -H "<Header" {} +Length of output: 2211
Verify Header layout in app and demo examples
Please visually confirm the updated Header styles in the following places:
• platforms/metagram/src/routes/(protected)/+layout.svelte
– Ensure the primary variant renders correctly with the new flex, margin, padding and border across mobile → desktop breakpoints.
– Verify the border appears below the header only on small screens and disappears atmd+.
– Check that the extra horizontal padding doesn’t introduce unwanted gaps in your page layout.• platforms/metagram/src/lib/fragments/Header/Header.svelte (demo usages)
– Quickly scan the built-in examples for secondary and tertiary variants to make sure they still look as intended.platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.stories.ts (1)
26-33: LGTM!Well-structured story that provides good test coverage for the new
profileSrcfunctionality. The story follows the established pattern and uses appropriate placeholder content.platforms/metagram/src/lib/fragments/BottomNav/BottomNav.svelte (2)
55-55: Good styling consistency with lighter fill color.The change to use the lighter burnt orange variant (
var(--color-brand-burnt-orange-300)) for active icon fills improves visual consistency across the navigation components.Also applies to: 70-70, 85-85, 100-100
111-111: Smart integration of settings route highlighting.Good logic to highlight the profile tab when on settings routes, maintaining visual context for users navigating between profile and settings.
platforms/metagram/src/lib/icons/Settings.svelte (2)
4-4: Good enhancement: Added fill prop for better customization.The addition of the
fillprop with a sensible default improves the icon's flexibility and aligns with the design system updates.
15-31:⚠️ Potential issueCritical issue: Duplicate path elements.
The same gear icon path is rendered twice (lines 15-18 and lines 25-30), which creates redundant SVG elements and could cause rendering issues.
Remove the duplicate path:
<path d="M21.011 14.0965C21.5329 13.9558 21.7939 13.8854 21.8969 13.7508C22 13.6163 22 13.3998 22 12.9669V11.0332C22 10.6003 22 10.3838 21.8969 10.2493C21.7938 10.1147 21.5329 10.0443 21.011 9.90358C19.0606 9.37759 17.8399 7.33851 18.3433 5.40087C18.4817 4.86799 18.5509 4.60156 18.4848 4.44529C18.4187 4.28902 18.2291 4.18134 17.8497 3.96596L16.125 2.98673C15.7528 2.77539 15.5667 2.66972 15.3997 2.69222C15.2326 2.71472 15.0442 2.90273 14.6672 3.27873C13.208 4.73448 10.7936 4.73442 9.33434 3.27864C8.95743 2.90263 8.76898 2.71463 8.60193 2.69212C8.43489 2.66962 8.24877 2.77529 7.87653 2.98663L6.15184 3.96587C5.77253 4.18123 5.58287 4.28891 5.51678 4.44515C5.45068 4.6014 5.51987 4.86787 5.65825 5.4008C6.16137 7.3385 4.93972 9.37763 2.98902 9.9036C2.46712 10.0443 2.20617 10.1147 2.10308 10.2492C2 10.3838 2 10.6003 2 11.0332V12.9669C2 13.3998 2 13.6163 2.10308 13.7508C2.20615 13.8854 2.46711 13.9558 2.98902 14.0965C4.9394 14.6225 6.16008 16.6616 5.65672 18.5992C5.51829 19.1321 5.44907 19.3985 5.51516 19.5548C5.58126 19.7111 5.77092 19.8188 6.15025 20.0341L7.87495 21.0134C8.24721 21.2247 8.43334 21.3304 8.6004 21.3079C8.76746 21.2854 8.95588 21.0973 9.33271 20.7213C10.7927 19.2644 13.2088 19.2643 14.6689 20.7212C15.0457 21.0973 15.2341 21.2853 15.4012 21.3078C15.5682 21.3303 15.7544 21.2246 16.1266 21.0133L17.8513 20.034C18.2307 19.8187 18.4204 19.711 18.4864 19.5547C18.5525 19.3984 18.4833 19.132 18.3448 18.5991C17.8412 16.6616 19.0609 14.6226 21.011 14.0965Z" {fill} /> <path d="M15.5 12C15.5 13.933 13.933 15.5 12 15.5C10.067 15.5 8.5 13.933 8.5 12C8.5 10.067 10.067 8.5 12 8.5C13.933 8.5 15.5 10.067 15.5 12Z" stroke={color} fill="white" stroke-width="1.5" /> - <path - d="M21.011 14.0965C21.5329 13.9558 21.7939 13.8854 21.8969 13.7508C22 13.6163 22 13.3998 22 12.9669V11.0332C22 10.6003 22 10.3838 21.8969 10.2493C21.7938 10.1147 21.5329 10.0443 21.011 9.90358C19.0606 9.37759 17.8399 7.33851 18.3433 5.40087C18.4817 4.86799 18.5509 4.60156 18.4848 4.44529C18.4187 4.28902 18.2291 4.18134 17.8497 3.96596L16.125 2.98673C15.7528 2.77539 15.5667 2.66972 15.3997 2.69222C15.2326 2.71472 15.0442 2.90273 14.6672 3.27873C13.208 4.73448 10.7936 4.73442 9.33434 3.27864C8.95743 2.90263 8.76898 2.71463 8.60193 2.69212C8.43489 2.66962 8.24877 2.77529 7.87653 2.98663L6.15184 3.96587C5.77253 4.18123 5.58287 4.28891 5.51678 4.44515C5.45068 4.6014 5.51987 4.86787 5.65825 5.4008C6.16137 7.3385 4.93972 9.37763 2.98902 9.9036C2.46712 10.0443 2.20617 10.1147 2.10308 10.2492C2 10.3838 2 10.6003 2 11.0332V12.9669C2 13.3998 2 13.6163 2.10308 13.7508C2.20615 13.8854 2.46711 13.9558 2.98902 14.0965C4.9394 14.6225 6.16008 16.6616 5.65672 18.5992C5.51829 19.1321 5.44907 19.3985 5.51516 19.5548C5.58126 19.7111 5.77092 19.8188 6.15025 20.0341L7.87495 21.0134C8.24721 21.2247 8.43334 21.3304 8.6004 21.3079C8.76746 21.2854 8.95588 21.0973 9.33271 20.7213C10.7927 19.2644 13.2088 19.2643 14.6689 20.7212C15.0457 21.0973 15.2341 21.2853 15.4012 21.3078C15.5682 21.3303 15.7544 21.2246 16.1266 21.0133L17.8513 20.034C18.2307 19.8187 18.4204 19.711 18.4864 19.5547C18.5525 19.3984 18.4833 19.132 18.3448 18.5991C17.8412 16.6616 19.0609 14.6226 21.011 14.0965Z" - stroke={color} - stroke-width="1.5" - stroke-linecap="round" - />Likely an incorrect or invalid review comment.
platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte (2)
10-10: Well-implemented component enhancement for profile images.The addition of the optional
profileSrcprop with conditional rendering is cleanly implemented. The styling is consistent with the existing design system, and the prop handling follows good patterns.Also applies to: 13-13, 18-18, 37-45
46-46: Good styling update for text consistency.Adding the
text-baseclass improves visual consistency with the overall design system.platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte (3)
58-61: Good refactoring of navigation handlers.The inline
onclickhandlers are more straightforward than the previoushandleNavClickfunction approach and improve code readability.Also applies to: 81-84, 104-107, 127-130, 150-153
68-68: Consistent icon fill color updates.The change to use the lighter variant
var(--color-brand-burnt-orange-300)for active icon fills provides better visual consistency.Also applies to: 91-91, 114-114, 137-137
3-3:Details
❌ Incorrect review comment
Verify removal of Camera icon import.
The Camera icon import was removed, suggesting the "Upload a photo" functionality was removed. Ensure this removal is intentional and doesn't break existing functionality.
🏁 Script executed:
#!/bin/bash # Search for any remaining Camera icon usage in the codebase rg "Camera" --type-add 'svelte:*.svelte' -t svelte -A 2 -B 2Length of output: 1051
No action needed: Camera import removal from SideBar.svelte is safe
The SideBar component never rendered a Camera icon—upload functionality lives in BottomNav.svelte, which still imports and uses the Camera icon. Removing it from SideBar.svelte does not break any existing feature.
• platforms/metagram/src/lib/fragments/BottomNav/BottomNav.svelte imports and renders
<Camera>for the “post” tab.Likely an incorrect or invalid review comment.
platforms/metagram/src/routes/(protected)/+layout.svelte (3)
37-47: Well-implemented responsive settings button.The conditional rendering of the settings button only on the profile route and only on small screens (
md:hidden) is a good responsive design approach.
35-48: Good layout structure with flex container.The flex container with
justify-betweenproperly aligns the header and settings button, providing a clean layout structure.
2-2: Proper imports for new functionality.The addition of necessary imports (
goto,RightAside,Settings) is correctly implemented to support the new features.Also applies to: 5-5, 7-7
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte (2)
22-36: Potential route matching issues with substring checks.The current implementation using
includes()could match unintended routes. For example,pathname.includes('home')would match/homepage,/home-admin, or/user-home-settings.Consider using more precise route matching:
-if (pathname.includes('/home')) { - activeTab = 'home'; -} else if (pathname.includes('/discover')) { - activeTab = 'discover'; -} else if (pathname.includes('/messages')) { - activeTab = 'messages'; -} else if (pathname.includes('/settings')) { - activeTab = 'settings'; -} else if (pathname.includes('/profile')) { - activeTab = 'profile'; -} else { - activeTab = ''; -} +const pathSegments = pathname.split('/'); +const currentRoute = pathSegments[1] || ''; +if (['home'].includes(currentRoute)) { + activeTab = 'home'; +} else if (['discover'].includes(currentRoute)) { + activeTab = 'discover'; +} else if (['messages'].includes(currentRoute)) { + activeTab = 'messages'; +} else if (['settings'].includes(currentRoute)) { + activeTab = 'settings'; +} else if (['profile'].includes(currentRoute)) { + activeTab = 'profile'; +} else { + activeTab = ''; +}
129-129: Hardcoded placeholder path in settings navigation.The path
/settings/asdfappears to contain placeholder text ("asdf") which should not be in production code.Replace with the actual settings route:
-goto('/settings/asdf'); +goto('/settings');Or if the route requires a dynamic parameter, use a proper ID:
-goto('/settings/asdf'); +// replace with actual user/context ID +goto(`/settings/${userId}`);
🧹 Nitpick comments (4)
platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte (1)
4-4: Remove unused import.The
Headerimport is not used anywhere in the component and should be removed.- import { Header } from '..';platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte (1)
20-20: Implement actual logout functionality instead of placeholder alert.The logout button currently only shows an alert. This needs to be replaced with actual logout logic that clears user session and redirects appropriately.
-<Button size="sm" variant="secondary" callback={() => alert('logout')}>Logout</Button> +<Button size="sm" variant="secondary" callback={handleLogout}>Logout</Button>Would you like me to help implement the logout functionality with proper session management and navigation?
platforms/metagram/src/lib/fragments/SettingsToggleButton/SettingsToggleButton.svelte (1)
4-5: Remove unused icon imports.The
ArrowRight01IconandHugeiconsIconimports are not used in the component. This adds unnecessary bundle size.-import { ArrowRight01Icon } from '@hugeicons/core-free-icons'; -import { HugeiconsIcon } from '@hugeicons/svelte';platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte (1)
19-29: Add accessibility improvements for navigation buttons.The navigation buttons would benefit from additional accessibility attributes for screen readers and keyboard navigation.
Consider adding ARIA labels and roles:
<SettingsNavigationButton + aria-label="Navigate to account settings" + role="button" onclick={() => goto('/settings/asdf/account')} profileSrc="https://picsum.photos/200/300" >Also consider adding keyboard event handlers for better accessibility.
Also applies to: 35-46, 49-60, 67-78, 81-92, 98-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
platforms/metagram/src/lib/fragments/Header/Header.svelte(2 hunks)platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte(2 hunks)platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Accounts.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Support.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsToggleButton/SettingsToggleButton.svelte(1 hunks)platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte(7 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/[id]/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/[id]/account/+page.svelte(1 hunks)platforms/metagram/src/routes/+layout.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte
- platforms/metagram/src/routes/(protected)/settings/[id]/+layout.svelte
🚧 Files skipped from review as they are similar to previous changes (5)
- platforms/metagram/src/routes/(protected)/settings/[id]/account/+page.svelte
- platforms/metagram/src/routes/+layout.svelte
- platforms/metagram/src/lib/fragments/Header/Header.svelte
- platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte
- platforms/metagram/src/routes/(protected)/+layout.svelte
🔇 Additional comments (10)
platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte (6)
3-3: LGTM! Clean import optimization.The removal of the unused
Cameraimport improves code cleanliness.
52-52: LGTM! Brand name styling update.The change to lowercase "pictique" appears intentional for design consistency.
58-61: LGTM! Inline navigation handlers are well-implemented.The conversion from a shared
handleNavClickfunction to inlineonclickhandlers is clean and maintains proper state management.Also applies to: 81-84, 104-107, 150-153
68-68: LGTM! Consistent color variant update.The change from
var(--color-brand-burnt-orange)tovar(--color-brand-burnt-orange-300)for fill colors provides a consistent lighter variant across all navigation icons.Also applies to: 91-91, 114-114, 137-137
123-144: LGTM! Settings navigation button implementation.The new settings button follows the same pattern as other navigation buttons with proper accessibility attributes and styling.
156-156: LGTM! Minor styling cleanup.The removal of
p-1padding class simplifies the profile image container styling.platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte (2)
6-11: Well-designed TypeScript interface and props handling.The interface extends
HTMLAttributes<HTMLElement>appropriately and the destructuring with rest props follows Svelte 5 best practices.
13-22: Good responsive design and defensive programming.The component implements proper responsive behavior and uses optional chaining for snippet rendering, which prevents runtime errors if snippets are undefined.
platforms/metagram/src/lib/fragments/SettingsToggleButton/SettingsToggleButton.svelte (1)
9-26: Well-structured reusable component with proper TypeScript integration.The component design is excellent with:
- Proper TypeScript interface extending HTMLButtonAttributes
- Good use of Svelte 5 snippets and props
- Clean separation of styling and functionality
- Appropriate use of the utility class function
platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte (1)
17-18: Good UX: Descriptive subtitle for Post Interactions setting.The subtitle "Likes and comments on your post" provides clear context for what this setting controls. Consider adding similar descriptive text to other settings where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte (4)
34-112: Consider refactoring repetitive navigation button patterns.The navigation buttons follow a repetitive pattern that could be abstracted to reduce code duplication and improve maintainability.
Consider creating a helper function or data structure:
+ const navigationSections = [ + { + title: 'Personalisation', + items: [ + { route: 'notifications', icon: Notification02FreeIcons, label: 'Notifications' }, + { route: 'direct-messages', icon: Message01Icon, label: 'Direct Messages' } + ] + }, + { + title: 'System', + items: [ + { route: 'data-and-storage', icon: DatabaseIcon, label: 'Data & Storage' }, + { route: 'support', icon: CustomerSupportIcon, label: 'Support' } + ] + } + ];Then render using loops to reduce duplication.
22-22:⚠️ Potential issueSecurity concern: Replace external image URL with secure alternative.
Using external image URLs from placeholder services like
picsum.photosin production poses security risks and reliability issues.Replace with a secure image handling approach:
- profileSrc="https://picsum.photos/200/300" + profileSrc={user?.profileImage || '/assets/default-avatar.png'}Ensure profile images are served from trusted sources or your own CDN.
26-27:⚠️ Potential issueReplace hardcoded user data with dynamic values.
The username and email are hardcoded and should be populated from actual user data or component props.
Replace hardcoded values with dynamic data:
- <h2 class="text-lg">_.ananyayaya._</h2> - <p class="text-sm">ananya@auvo.io</p> + <h2 class="text-lg">{user?.username || 'Username'}</h2> + <p class="text-sm">{user?.email || 'Email'}</p>
1-16: 🛠️ Refactor suggestionAdd error handling for navigation actions.
The navigation actions don't include error handling, which could lead to poor user experience if navigation fails.
Consider wrapping navigation calls with error handling:
+ const navigateTo = async (path: string) => { + try { + await goto(path); + } catch (error) { + console.error('Navigation failed:', error); + // Handle navigation error (show toast, etc.) + } + };Then use
navigateToinstead of directgotocalls throughout the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Accounts.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Support.svelte(1 hunks)platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte(7 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/Accounts.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte
- platforms/metagram/src/lib/fragments/RightAside/RightAside.svelte
- platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte
- platforms/metagram/src/routes/(protected)/+layout.svelte
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte (2)
6-6: Remove redundant checked prop.The
checked={false}prop is likely redundant since toggle buttons typically default to unchecked state.- <SettingsToggleButton checked={false}> + <SettingsToggleButton>
5-15: Consider accessibility enhancements.While the component structure is good, consider adding accessibility features:
- ARIA labels for better screen reader support
- Focus management for the toggle interaction
- Semantic association between the toggle and its description
Example enhancement:
<div class="flex flex-col gap-3"> - <SettingsToggleButton> + <SettingsToggleButton aria-describedby="dm-description"> {#snippet children()} <div class="flex flex-col items-start"> <h2 class="text-black-800 text-base">Allow messages from non-followers</h2> - <p class="text-black-600 text-sm">Anyone can send a DM</p> + <p id="dm-description" class="text-black-600 text-sm">Anyone can send a DM</p> </div> {/snippet} </SettingsToggleButton> <hr class="text-grey" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte
🔇 Additional comments (2)
platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte (2)
1-3: LGTM! Clean import structure.The import statement is properly structured and follows Svelte conventions.
14-14:Details
❓ Verification inconclusive
Verify CSS class validity.
The
text-greyclass may not be a standard Tailwind CSS class. Standard Tailwind uses specific gray shades liketext-gray-400ortext-gray-500.
🏁 Script executed:
#!/bin/bash # Search for usage of 'text-grey' class in the codebase to verify consistency rg "text-grey" --type-add 'svelte:*.svelte' -t svelteLength of output: 1842
🏁 Script executed:
#!/bin/bash # Locate Tailwind configuration file and check for any 'grey' color alias config=$(fd 'tailwind.config.*' --max-results 1) echo "Tailwind config file: $config" rg -n "grey" "$config" rg -n "colors" "$config" -A10Length of output: 227
Confirm
text-greyclass validity
I couldn’t locate a Tailwind config or any CSS rule defining atext-greyalias—Tailwind’s default palette uses American English (gray). However, this class appears in several components:• platforms/metagram/src/lib/fragments/SettingsRightAside/DirectMessages.svelte
• platforms/metagram/src/lib/fragments/SettingsRightAside/Accounts.svelte
• platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte
• platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte
• platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte
• platforms/metagram/src/lib/fragments/SettingsRightAside/Support.svelte
• platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.sveltePlease verify whether
text-greyis defined in your global CSS or Tailwind configuration. If it isn’t, update these to a valid Tailwind class (e.g.text-gray-400ortext-gray-500) for consistent styling.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
platforms/metagram/src/routes/(protected)/settings/notifications/+page.svelte (1)
6-38: Consider default notification states for better user experience.The implementation is well-structured and follows consistent patterns. However, all notification toggles default to
false, which might not provide the best user experience. Consider whether some notifications (like "Post Interactions") should be enabled by default to help users discover engagement on their content.If appropriate for your product strategy, consider enabling some notifications by default:
-<SettingsToggleButton checked={false}> +<SettingsToggleButton checked={true}> {#snippet children()} <div class="flex flex-col items-start"> <h2 class="text-black-800 text-base">Post Interactions</h2> <p class="text-black-600 text-sm">Likes and comments on your post</p> </div> {/snippet} </SettingsToggleButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Account.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsRightAside/Support.svelte(1 hunks)platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte(7 hunks)platforms/metagram/src/lib/fragments/index.ts(1 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/direct-messages/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/logout/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/notifications/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/support/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- platforms/metagram/src/routes/(protected)/settings/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (9)
- platforms/metagram/src/lib/fragments/SettingsRightAside/Support.svelte
- platforms/metagram/src/lib/fragments/index.ts
- platforms/metagram/src/lib/fragments/SettingsRightAside/Aside.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/Notifications.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/Logout.svelte
- platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte
- platforms/metagram/src/lib/fragments/SideBar/SideBar.svelte
- platforms/metagram/src/routes/(protected)/+layout.svelte
- platforms/metagram/src/lib/fragments/SettingsRightAside/DataStorage.svelte
🔇 Additional comments (2)
platforms/metagram/src/routes/(protected)/settings/direct-messages/+page.svelte (1)
1-16: Well-structured direct messages settings implementation.The implementation follows good practices with clear descriptive text, appropriate default state (unchecked for privacy), and consistent use of the SettingsToggleButton component. The layout is clean and user-friendly.
platforms/metagram/src/routes/(protected)/settings/+layout.svelte (1)
8-8: Unused variable: renderSidebar is calculated but not used.The
renderSidebarderived value is calculated but not utilized due to the commented conditional rendering block below.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
platforms/metagram/src/lib/ui/Helper/Helper.svelte (1)
15-17: Consider improving class concatenation logic.The current class concatenation approach has a potential issue where it always joins with a space, which could result in extra whitespace if
restProps.classis undefined.Consider this more robust approach:
-<p {...restProps} class={cn([cBase, restProps.class].join(' '))}> +<p {...restProps} class={cn(cBase, restProps.class)}>This leverages the
cnutility function's built-in ability to handle undefined/null values and proper spacing, making the code cleaner and more reliable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte(1 hunks)platforms/metagram/src/lib/ui/Helper/Helper.stories.snippet.svelte(1 hunks)platforms/metagram/src/lib/ui/Helper/Helper.stories.ts(1 hunks)platforms/metagram/src/lib/ui/Helper/Helper.svelte(1 hunks)platforms/metagram/src/lib/ui/Input/Input.stories.ts(1 hunks)platforms/metagram/src/lib/ui/index.ts(1 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/direct-messages/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/logout/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/notifications/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/support/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- platforms/metagram/src/lib/ui/index.ts
- platforms/metagram/src/lib/ui/Helper/Helper.stories.snippet.svelte
- platforms/metagram/src/lib/ui/Helper/Helper.stories.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- platforms/metagram/src/routes/(protected)/settings/+layout.svelte
- platforms/metagram/src/routes/(protected)/settings/support/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/notifications/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/logout/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/direct-messages/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte
- platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte
- platforms/metagram/src/routes/(protected)/+layout.svelte
- platforms/metagram/src/routes/(protected)/settings/account/+page.svelte
🔇 Additional comments (3)
platforms/metagram/src/lib/ui/Helper/Helper.svelte (2)
6-8: Well-structured interface definition.The
IHelperPropsinterface properly extendsHTMLAttributes<HTMLElement>and requires achildrenSnippet. This provides good type safety and flexibility for the component.
10-10: Clean props destructuring.The destructuring pattern separating
childrenfrom other props is a clean approach that allows for flexible prop forwarding while maintaining type safety.platforms/metagram/src/lib/ui/Input/Input.stories.ts (1)
7-7: Good separation of concerns with type annotation update.The removal of
helperTextfrom the type annotation aligns well with the introduction of the dedicatedHelpercomponent. This promotes cleaner component responsibilities and better maintainability.
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
platforms/metagram/src/routes/(protected)/settings/support/report-bug/+page.svelte (1)
1-17: 🛠️ Refactor suggestionPlaceholder functionality detected - consider implementing real bug reporting.
The component structure and UI layout look good, with proper use of form elements and file upload functionality. However, the bug reporting currently only shows an alert message.
Consider implementing actual bug reporting functionality that:
- Validates the input fields
- Handles file uploads properly
- Submits the data to a backend service
- Provides proper user feedback
🧹 Nitpick comments (4)
platforms/metagram/src/routes/(protected)/settings/account/email-address/+page.svelte (1)
6-9: Consider adding loading and error states.The component should handle cases where user data might be loading or unavailable.
<div> <Label>Your email address</Label> - <Input type="email" value={userEmail} disabled /> + {#if userEmail} + <Input type="email" value={userEmail} disabled /> + {:else} + <Input type="email" placeholder="Loading..." disabled /> + {/if} </div>platforms/metagram/src/routes/(protected)/settings/account/deactivate/+page.svelte (1)
1-1: Add TypeScript specification for consistency.The script tag should specify TypeScript to maintain consistency with other components in the codebase.
-<script> +<script lang="ts">platforms/metagram/src/lib/fragments/index.ts (1)
11-14: Consider maintaining alphabetical order for better maintainability.The new component exports follow the established pattern correctly. However, consider maintaining alphabetical order to improve code organization and maintainability.
Apply this diff to maintain alphabetical order:
export { default as Post } from './Post/Post.svelte'; -export { default as RightAside } from './RightAside/RightAside.svelte'; -export { default as SettingsToggleButton } from './SettingsToggleButton/SettingsToggleButton.svelte'; -export { default as SettingsDeleteButton } from './SettingsDeleteButton/SettingsDeleteButton.svelte'; +export { default as RightAside } from './RightAside/RightAside.svelte'; +export { default as SettingsDeleteButton } from './SettingsDeleteButton/SettingsDeleteButton.svelte'; +export { default as SettingsToggleButton } from './SettingsToggleButton/SettingsToggleButton.svelte';platforms/metagram/src/routes/(protected)/settings/data-and-storage/clear-cache/+page.svelte (1)
16-16: Consider making cache size dynamic instead of hard-coded.The cache size "14MB" appears to be hard-coded. For a production application, this should likely be calculated dynamically.
Consider fetching the actual cache size:
-This would clear the apps cache and free about 14MB of storage +This would clear the apps cache and free about {cacheSize} of storage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
platforms/metagram/src/lib/fragments/Header/Header.svelte(2 hunks)platforms/metagram/src/lib/fragments/SettingsDeleteButton/SettingsDeleteButton.svelte(1 hunks)platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte(2 hunks)platforms/metagram/src/lib/fragments/SettingsToggleButton/SettingsToggleButton.svelte(1 hunks)platforms/metagram/src/lib/fragments/index.ts(1 hunks)platforms/metagram/src/lib/ui/Label/Label.svelte(1 hunks)platforms/metagram/src/routes/(protected)/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/home/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/+layout.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/change-password/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/deactivate/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/email-address/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/username/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/active-devices/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/clear-cache/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/download-data/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/support/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/support/feedback/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/support/report-bug/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- platforms/metagram/src/lib/ui/Label/Label.svelte
- platforms/metagram/src/routes/(protected)/home/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (7)
- platforms/metagram/src/routes/(protected)/settings/+layout.svelte
- platforms/metagram/src/lib/fragments/Header/Header.svelte
- platforms/metagram/src/routes/(protected)/settings/support/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte
- platforms/metagram/src/routes/(protected)/+layout.svelte
- platforms/metagram/src/lib/fragments/SettingsToggleButton/SettingsToggleButton.svelte
- platforms/metagram/src/lib/fragments/SettingsNavigationButton/SettingsNavigationButton.svelte
🔇 Additional comments (2)
platforms/metagram/src/routes/(protected)/settings/account/+page.svelte (1)
7-29: Navigation routes look correct now.The navigation routes have been properly implemented following the previous review feedback. Each button now correctly navigates to its corresponding account-specific route:
- Username →
/settings/account/username- Email Address →
/settings/account/email-address- Change Password →
/settings/account/change-password- Deactivate Account →
/settings/account/deactivateThe component structure is clean and follows consistent patterns.
platforms/metagram/src/lib/fragments/SettingsDeleteButton/SettingsDeleteButton.svelte (1)
8-16: Well-structured component with proper TypeScript interfaces.The component follows excellent practices with proper TypeScript typing, modern Svelte 5 syntax, and good component composition patterns.
|
also please resolve code rabbit suggestions |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte (3)
25-25: Security concern: External image URL still present.The hardcoded
picsum.photosURL remains from previous reviews, which poses security and reliability risks in production.Replace with a secure image source:
- profileSrc="https://picsum.photos/200/300" + profileSrc={user.profileImage || '/assets/default-avatar.png'}
36-84: Code duplication: Repetitive navigation button patterns.The navigation buttons still follow repetitive patterns that could be abstracted for better maintainability, as identified in previous reviews.
Consider creating a data structure and loop:
+<script lang="ts"> + const navigationSections = [ + { + title: 'Personalisation', + items: [ + { route: 'notifications', icon: Notification02FreeIcons, label: 'Notifications' } + ] + }, + { + title: 'System', + items: [ + { route: 'data-and-storage', icon: DatabaseIcon, label: 'Data & Storage' }, + { route: 'logout', icon: Logout01Icon, label: 'Logout' } + ] + } + ]; +</script> +{#each navigationSections as section} + <hr class="text-grey" /> + <div class="flex flex-col gap-3"> + <h3 class="text-brand-burnt-orange text-base font-semibold">{section.title}</h3> + {#each section.items as item} + <div class="{route === `/settings/${item.route}` ? 'bg-grey' : ''} rounded-xl p-2"> + <SettingsNavigationButton onclick={() => goto(`/settings/${item.route}`)}> + {#snippet leadingIcon()} + <HugeiconsIcon + size="24px" + icon={item.icon} + color="var(--color-brand-burnt-orange)" + /> + {/snippet} + {#snippet children()} + {item.label} + {/snippet} + </SettingsNavigationButton> + </div> + {/each} + </div> +{/each}
24-84: Missing error handling for navigation actions.Navigation calls lack error handling which could lead to poor user experience if navigation fails, as noted in previous reviews.
Add error handling wrapper:
+<script lang="ts"> + const navigateTo = async (path: string) => { + try { + await goto(path); + } catch (error) { + console.error('Navigation failed:', error); + // Handle navigation error (show toast notification, etc.) + } + }; +</script>Then replace direct
gotocalls:- onclick={() => goto(`/settings/account`)} + onclick={() => navigateTo(`/settings/account`)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/+page.svelte(1 hunks)
🔇 Additional comments (4)
platforms/metagram/src/routes/(protected)/settings/account/+page.svelte (3)
1-4: LGTM! Clean imports and TypeScript setup.The script section properly imports the required navigation function and component with correct TypeScript configuration.
7-11: Navigation routes are correctly implemented.The navigation routes now properly point to account-specific endpoints (
/settings/account/usernameand/settings/account/deactivate) which addresses the previous issue where all buttons incorrectly navigated to/settings/data-and-storage.Also applies to: 13-17
6-19:Details
❓ Verification inconclusive
Verify if Email and Password settings were intentionally removed.
The current implementation only includes Username and Deactivate Account options, but based on past review comments, there were originally 4 options including Email Address and Change Password. Please confirm if this reduction is intentional or if these settings were moved to a different location.
🏁 Script executed:
#!/bin/bash # Description: Search for Email and Password settings pages to verify if they exist elsewhere # Expected: Find email and password related settings pages or components echo "Searching for email settings..." fd -e svelte -x grep -l "email\|Email" {} \; | grep -i settings echo "Searching for password settings..." fd -e svelte -x grep -l "password\|Password" {} \; | grep -i settings echo "Checking for account-related route files..." fd . platforms/metagram/src/routes --type f | grep -E "account|settings" | sortLength of output: 1664
Confirm Intentional Removal of Email & Change-Password Options
I searched the codebase and did not find any email or change-password routes under
settings/account. Please confirm whether these options were intentionally removed or relocated. If they should still exist, you’ll need to reintroduce the corresponding pages and update the navigation buttons.• No
change-passwordSvelte pages underplatforms/metagram/src/routes/(protected)/settings/account
• Current sub-routes include onlyusernameanddeactivateplatforms/metagram/src/lib/fragments/SettingsPage/SettingsPage.svelte (1)
1-20: Component structure and props look good.The imports, type definitions, and props setup are well-structured. The dynamic route derivation and proper TypeScript interface definition improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte (1)
6-10: Complete the placeholder implementation.The current implementation contains only a placeholder button with temporary text "Merul will add things here (as per out discussion)". This page appears to be incomplete and non-functional.
Based on the past review comments, the previous implementation had navigation issues that have been addressed by this placeholder approach, but now the page needs to be properly implemented with the intended data and storage functionality.
Would you like me to help implement the data and storage settings functionality based on common patterns for such pages?
platforms/metagram/src/routes/(protected)/settings/+page.svelte (2)
34-34: Extract route highlighting logic into a reusable function.The route-based conditional styling logic is repeated three times. Consider extracting this into a reusable function to improve maintainability.
+ function getActiveRouteClass(routePath: string): string { + return route === routePath ? 'bg-grey' : ''; + } - <div class="{route === `/settings/notifications` ? 'bg-grey' : ''} rounded-xl p-2"> + <div class="{getActiveRouteClass('/settings/notifications')} rounded-xl p-2"> - <div class="{route === `/settings/data-and-storage` ? 'bg-grey' : ''} rounded-xl p-2"> + <div class="{getActiveRouteClass('/settings/data-and-storage')} rounded-xl p-2"> - <div class="{route === `/settings/logout` ? 'bg-grey' : ''} rounded-xl p-2"> + <div class="{getActiveRouteClass('/settings/logout')} rounded-xl p-2">Also applies to: 52-52, 66-66
32-48: Consider extracting navigation sections into reusable components.The navigation sections follow a consistent pattern and could be extracted into a reusable component to reduce code duplication and improve maintainability.
Create a
SettingsSectioncomponent that accepts a title, items array, and current route:// SettingsSection.svelte <script lang="ts"> interface SettingsItem { path: string; label: string; icon: any; } interface Props { title: string; items: SettingsItem[]; currentRoute: string; } let { title, items, currentRoute }: Props = $props(); </script> <div class="flex flex-col gap-3"> <h3 class="text-brand-burnt-orange text-base font-semibold">{title}</h3> {#each items as item} <div class="{currentRoute === item.path ? 'bg-grey' : ''} rounded-xl p-2"> <SettingsNavigationButton onclick={() => goto(item.path)}> {#snippet leadingIcon()} <HugeiconsIcon size="24px" icon={item.icon} color="var(--color-brand-burnt-orange)" /> {/snippet} {#snippet children()} {item.label} {/snippet} </SettingsNavigationButton> </div> {/each} </div>Also applies to: 50-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
platforms/metagram/src/routes/(protected)/settings/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/deactivate/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/account/username/+page.svelte(1 hunks)platforms/metagram/src/routes/(protected)/settings/data-and-storage/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- platforms/metagram/src/routes/(protected)/settings/account/username/+page.svelte
- platforms/metagram/src/routes/(protected)/settings/account/deactivate/+page.svelte
There was a problem hiding this comment.
This is obsolete now :)
| goto(`/${newTab}`); | ||
| }; | ||
|
|
||
| $effect(() => { |
There was a problem hiding this comment.
Proposition:
const tabMap = ["home", "discover", "messages", "settings", "profile"];
$effect(() => {
const pathname = page.url.pathname;
activeTab =
tabMap.find((tab) => new RegExp(^/${tab}(/|$)).test(pathname)) ?? "";
});
| }; | ||
|
|
||
| $effect(() => { | ||
| heading = routeToHeading[route] ?? heading; |
There was a problem hiding this comment.
Proposition:
const headingOverrides = [
{ keyword: 'home', heading: 'Feed' },
{ keyword: 'discover', heading: 'Search' },
{ keyword: 'post', heading: 'Post' },
{ keyword: 'messages', heading: 'Messages' },
{ keyword: 'settings', heading: 'Settings' },
{ keyword: 'profile', heading: 'Profile' }
];
$effect(() => {
heading = headingOverrides.find(({ keyword }) => route.includes(keyword))?.heading
?? routeToHeading[route]
?? '';
});
| goto(`/${newTab}`); | ||
| }; | ||
|
|
||
| $effect(() => { |
There was a problem hiding this comment.
| $effect(() => { | |
| const tabMap = ["home", "discover", "messages", "settings", "profile"]; | |
| $effect(() => { | |
| const pathname = page.url.pathname; | |
| activeTab = | |
| tabMap.find((tab) => new RegExp(^/${tab}(/|$)).test(pathname)) ?? ""; | |
| }); |
3 participants
Description of change
settings page for pictique
Issue Number
closes #161
closes #162
closes #163
closes #164
closes #167
Type of change
How the change has been tested
Locally
Change checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Style