feat: added settings dialog and user settings page#17
Merged
BuckyMcYolo merged 2 commits intomainfrom Mar 9, 2026
Merged
Conversation
📝 WalkthroughWalkthroughImplements avatar presign upload API and UI, extends user schema with bio/status, adds a Settings dialog and provider plus My Account settings UI, introduces new breadcrumb and sidebar UI libraries, and applies minor chat UI and style token tweaks. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web App
participant API as API Server
participant S3 as S3 Storage
User->>Web: Choose avatar file
Web->>Web: Validate MIME & size (<= 2MB)
Web->>API: POST /uploads/avatar/presign (filename, contentType, size)
API->>S3: Request presigned PUT URL (server-side)
S3-->>API: Return signed URL
API-->>Web: Return { uploadUrl, fileUrl }
Web->>S3: PUT file to uploadUrl
S3-->>Web: Confirm upload
Web->>API: PATCH /user (image=fileUrl, name, bio, status)
API-->>Web: Return success
Web->>User: Show updated avatar/confirmation
sequenceDiagram
actor User
participant App as Authenticated Layout
participant SettingsCtx as SettingsProvider
participant Dialog as SettingsDialog
participant MyAccount as MyAccountSettings
participant API as API Server
User->>App: Click settings button
App->>SettingsCtx: openSettings()
SettingsCtx-->>App: isOpen = true
App->>Dialog: Render settings (open)
User->>Dialog: Select "My Account"
Dialog->>MyAccount: Render form
User->>MyAccount: Upload avatar (triggers presign flow)
MyAccount->>API: avatar presign -> upload -> updateUser
API-->>MyAccount: Success
MyAccount-->>Dialog: Update UI / close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/uploads/handlers.ts`:
- Around line 10-11: Replace the local relative imports in
apps/api/src/routes/v1/uploads/handlers.ts with the project path aliases: change
the import of AvatarPresignRoute and PresignRoute from "./routes" to
"@/routes/v1/uploads/routes" (or the appropriate alias path to that module) and
change the import of MAX_AVATAR_SIZE and PRESIGNED_URL_EXPIRY_SECONDS from
"./schema" to "@/routes/v1/uploads/schema" (or the correct alias path); ensure
the symbols AvatarPresignRoute, PresignRoute, MAX_AVATAR_SIZE, and
PRESIGNED_URL_EXPIRY_SECONDS are referenced from the alias imports so tsc-alias
rewrites them correctly during build.
In `@apps/api/src/routes/v1/uploads/routes.ts`:
- Around line 11-16: The import block in routes.ts uses a relative path
("./schema") which violates the package rule to use path aliases; update the
import to use the `@/` alias (e.g. import { avatarPresignRequestSchema,
avatarPresignResponseSchema, presignRequestSchema, presignResponseSchema } from
"@/routes/v1/uploads/schema") so tsc-alias can rewrite it during build and keep
the same named symbols unchanged.
In `@apps/web/src/components/chat/message-action-bar.tsx`:
- Around line 45-52: The timeout created in handleMoreOpenChange can fire after
unmount and should be cleared: store the timeout id in a ref (e.g., timeoutRef)
when calling setTimeout inside handleMoreOpenChange, add useEffect cleanup that
clearsTimeout(timeoutRef.current) on unmount, and ensure handleMoreOpenChange
clears any existing timeout before setting a new one; add useRef to imports and
reference timeoutRef in the cleanup and handler.
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 48-63: The avatar preview blob URL created in setAvatarFromFile
(and the similar handler at lines 153-157) must be revoked to avoid memory
leaks: before calling URL.createObjectURL(file) revoke the current avatarPreview
URL if present, store the new URL in setAvatarPreview, and also revoke the last
preview in a useEffect cleanup (return function) that runs on unmount or when
avatarPreview changes so any superseded or unsaved blob URLs are released;
update both setAvatarFromFile and the other file-select handler to follow this
revoke-then-create pattern and add the cleanup useEffect referencing
avatarPreview.
In `@apps/web/src/components/settings/settings-dialog.tsx`:
- Around line 76-110: The sidebar is hidden on small screens (Sidebar
className="hidden ... md:flex") so activeItem can’t change on mobile; add an
alternate picker rendered when the Sidebar is hidden (e.g., detect small screens
or invert the responsive class) that maps filteredNav into a compact control (a
Select, Tabs, or a horizontal list) which calls setActiveItem when an entry is
chosen; update the UI near SidebarHeader (or replace Input area) to show the
current activeItem/icon and open the picker on small screens so filteredNav,
SidebarMenuButton, SidebarMenuItem, and activeItem continue to drive selection
on mobile.
- Around line 81-88: The search Input is removed from keyboard tab order by
tabIndex={-1}; update the Input in settings-dialog.tsx (the Input element that
uses placeholder="Search settings...", value={search}, onChange={(e) =>
setSearch(e.target.value)}) to participate in the modal's tab sequence by
removing the tabIndex prop or setting tabIndex={0}, and ensure autoFocus
behavior is handled via autoFocus prop rather than manipulating tabIndex so
keyboard users can reach the field.
In `@packages/auth/src/lib/auth.ts`:
- Around line 88-97: The profile fields bio and status were marked returned:
true but the user projection logic in the DM and guild handlers (the user DTOs
used by the handlers) still select a fixed subset and drop them, and the guild
presence schema already uses a status field which would conflict; either thread
the new fields through the user projection/DTOs and update the DM handlers and
guild handlers to include bio and status, or rename the profile field now
(recommend renaming to customStatus) and update all referencers: the user
projection/DTOs used by the DM handlers (user projection function/DTO) and the
guild handlers (guild user DTO) plus the guild presence schema so there is no
name collision with the existing presence status; ensure consistent naming in
the auth returned schema (bio, customStatus) and update any code that maps or
consumes these DTOs to use the new property.
In `@packages/db/src/schemas/users.ts`:
- Around line 36-37: The schema added new columns bio and status in
packages/db/src/schemas/users.ts but you haven't generated the Drizzle
migrations or updated the generated schema; run the migration generation in the
db package (e.g., cd packages/db && npm run db:generate), verify the new
migration files appear under packages/db/drizzle/ and that
packages/db/src/generated-schema.ts includes the new bio and status fields,
commit those migration files, and ensure you apply them (db:push or your
migration runner) before deploying so packages/auth/src/lib/auth.ts's returned
fields match the DB schema at runtime.
In `@packages/ui/src/components/breadcrumb.tsx`:
- Around line 80-94: BreadcrumbEllipsis currently uses aria-hidden="true" which
prevents the nested sr-only label from being announced; update
BreadcrumbEllipsis so the ellipsis is accessible by removing aria-hidden and
role="presentation" and adding an accessible label (e.g., aria-label="More") on
the root span (keep MoreHorizontal icon but remove the nested <span
className="sr-only">More</span>), or alternatively keep it decorative by keeping
aria-hidden and removing the sr-only element — choose the accessible approach
(use aria-label on BreadcrumbEllipsis) and adjust className/props usage
accordingly.
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 73-86: The sidebar state is written to a cookie in setOpen but
never read back on initialization, so _open is always set from defaultOpen;
update the initialization of _open/_setOpen (the React.useState call) to first
read and parse the SIDEBAR_COOKIE_NAME cookie (fallback to defaultOpen if absent
or invalid) so persisted state is restored after reload; ensure
openProp/setOpenProp still control the component when provided and keep using
document.cookie in setOpen to update the cookie.
- Around line 475-483: The outline variant inside sidebarMenuButtonVariants is
wrapping CSS token variables in hsl(...), which breaks the new oklch(...)
tokens; update the outline variant's shadow strings to use the tokens directly
(e.g., use var(--sidebar-border) and var(--sidebar-accent) rather than
hsl(var(--sidebar-border)) / hsl(var(--sidebar-accent))) so the border and hover
shadow colors render correctly with the oklch values defined in globals.css.
In `@packages/ui/src/hooks/use-mobile.ts`:
- Around line 8-16: The useEffect sets up a MediaQueryList as mql but the
onChange handler reads window.innerWidth; change onChange to use mql.matches
(and the initial setIsMobile call to use mql.matches instead of
window.innerWidth) so the state is derived from the MediaQueryList result;
update references in the React.useEffect block (mql, onChange, setIsMobile,
MOBILE_BREAKPOINT) and keep addEventListener/removeEventListener logic the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 499e4211-231d-477e-8e6d-81c711085259
📒 Files selected for processing (19)
ROADMAP.mdapps/api/src/routes/v1/uploads/handlers.tsapps/api/src/routes/v1/uploads/index.tsapps/api/src/routes/v1/uploads/routes.tsapps/api/src/routes/v1/uploads/schema.tsapps/web/src/components/chat/message-action-bar.tsxapps/web/src/components/chat/message-edit-input.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/components/settings/my-account-settings.tsxapps/web/src/components/settings/settings-dialog.tsxapps/web/src/components/sidebar/channel-panel/user-bar.tsxapps/web/src/context/settings-context.tsxapps/web/src/routes/_authenticated.tsxpackages/auth/src/lib/auth.tspackages/db/src/schemas/users.tspackages/ui/src/components/breadcrumb.tsxpackages/ui/src/components/sidebar.tsxpackages/ui/src/hooks/use-mobile.tspackages/ui/src/styles/globals.css
Comment on lines
+10
to
+11
| import type { AvatarPresignRoute, PresignRoute } from "./routes" | ||
| import { MAX_AVATAR_SIZE, PRESIGNED_URL_EXPIRY_SECONDS } from "./schema" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Switch these local imports to @/ aliases.
These new relative specifiers add another inconsistency inside apps/api/src. Keep them on the alias path so the source tree follows the same tsc-alias rewrite contract everywhere.
Suggested change
-import type { AvatarPresignRoute, PresignRoute } from "./routes"
-import { MAX_AVATAR_SIZE, PRESIGNED_URL_EXPIRY_SECONDS } from "./schema"
+import type { AvatarPresignRoute, PresignRoute } from "@/routes/v1/uploads/routes"
+import { MAX_AVATAR_SIZE, PRESIGNED_URL_EXPIRY_SECONDS } from "@/routes/v1/uploads/schema"As per coding guidelines, "Use @/* path aliases for imports in compiled packages like apps/api, with tsc-alias rewriting them to relative .js paths in the dist directory`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/handlers.ts` around lines 10 - 11, Replace the
local relative imports in apps/api/src/routes/v1/uploads/handlers.ts with the
project path aliases: change the import of AvatarPresignRoute and PresignRoute
from "./routes" to "@/routes/v1/uploads/routes" (or the appropriate alias path
to that module) and change the import of MAX_AVATAR_SIZE and
PRESIGNED_URL_EXPIRY_SECONDS from "./schema" to "@/routes/v1/uploads/schema" (or
the correct alias path); ensure the symbols AvatarPresignRoute, PresignRoute,
MAX_AVATAR_SIZE, and PRESIGNED_URL_EXPIRY_SECONDS are referenced from the alias
imports so tsc-alias rewrites them correctly during build.
Owner
Author
There was a problem hiding this comment.
why is this flagged as major?
There was a problem hiding this comment.
@BuckyMcYolo, fair pushback — "Major" was too aggressive here. This is really a Minor style/consistency issue. Same-directory relative imports (./routes, ./schema) resolve correctly at build time without any help from tsc-alias, so there's no functional or runtime risk. The flag is purely about keeping the file internally consistent — the rest of handlers.ts already uses @/ aliases for all other imports, and these two new ones break that pattern.
In short: it won't cause a bug, it's just an inconsistency. I'll keep that in mind for future severity calibration.
🧠 Learnings used
Learnt from: CR
Repo: BuckyMcYolo/townhall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T18:54:27.248Z
Learning: Applies to apps/api/src/**/*.ts : Use `@/*` path aliases for imports in compiled packages like `apps/api`, with `tsc-alias` rewriting them to relative `.js` paths in the dist directory
Learnt from: CR
Repo: BuckyMcYolo/townhall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T18:54:27.248Z
Learning: Applies to packages/{db,env,auth,ui,typescript-config}/src/**/*.ts : Use relative imports (not path aliases) in uncompiled packages like `packages/db`, `packages/env`, `packages/auth` that are resolved at runtime by Node
Comment on lines
+11
to
+16
| import { | ||
| avatarPresignRequestSchema, | ||
| avatarPresignResponseSchema, | ||
| presignRequestSchema, | ||
| presignResponseSchema, | ||
| } from "./schema" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @/ imports in apps/api sources.
This new relative import block introduces an exception inside apps/api/src, even though this package is supposed to rely on @/* specifiers and let tsc-alias rewrite them during build.
Suggested change
import {
avatarPresignRequestSchema,
avatarPresignResponseSchema,
presignRequestSchema,
presignResponseSchema,
-} from "./schema"
+} from "@/routes/v1/uploads/schema"As per coding guidelines, "Use @/* path aliases for imports in compiled packages like apps/api, with tsc-alias rewriting them to relative .js paths in the dist directory`."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| avatarPresignRequestSchema, | |
| avatarPresignResponseSchema, | |
| presignRequestSchema, | |
| presignResponseSchema, | |
| } from "./schema" | |
| import { | |
| avatarPresignRequestSchema, | |
| avatarPresignResponseSchema, | |
| presignRequestSchema, | |
| presignResponseSchema, | |
| } from "@/routes/v1/uploads/schema" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/routes.ts` around lines 11 - 16, The import
block in routes.ts uses a relative path ("./schema") which violates the package
rule to use path aliases; update the import to use the `@/` alias (e.g. import {
avatarPresignRequestSchema, avatarPresignResponseSchema, presignRequestSchema,
presignResponseSchema } from "@/routes/v1/uploads/schema") so tsc-alias can
rewrite it during build and keep the same named symbols unchanged.
Comment on lines
+48
to
+63
| useEffect(() => { | ||
| if (!user) return | ||
| setName(user.name ?? "") | ||
| setBio((user.bio as string) ?? "") | ||
| setStatus((user.status as string) ?? "") | ||
| }, [user]) | ||
|
|
||
| const setAvatarFromFile = useCallback((file: File) => { | ||
| const error = validateAvatarFile(file) | ||
| if (error) { | ||
| toast.error(error) | ||
| return | ||
| } | ||
| setAvatarFile(file) | ||
| setAvatarPreview(URL.createObjectURL(file)) | ||
| }, []) |
There was a problem hiding this comment.
Revoke superseded avatar preview URLs.
Every call to URL.createObjectURL() allocates a blob URL, but the current cleanup only runs after a successful save. Selecting another file or closing the dialog without saving leaves the previous blob pinned in memory.
Suggested fix
useEffect(() => {
if (!user) return
setName(user.name ?? "")
setBio((user.bio as string) ?? "")
setStatus((user.status as string) ?? "")
}, [user])
+
+ useEffect(() => {
+ if (!avatarPreview?.startsWith("blob:")) return
+
+ return () => URL.revokeObjectURL(avatarPreview)
+ }, [avatarPreview])
const setAvatarFromFile = useCallback((file: File) => {
const error = validateAvatarFile(file)
if (error) {
toast.error(error)
@@
await authClient.updateUser({
name: name.trim(),
image: imageUrl ?? undefined,
bio: bio.trim() || undefined,
status: status.trim() || undefined,
})
setAvatarFile(null)
- if (avatarPreview) {
- URL.revokeObjectURL(avatarPreview)
- setAvatarPreview(null)
- }
+ setAvatarPreview(null)
toast.success("Profile updated")Also applies to: 153-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/settings/my-account-settings.tsx` around lines 48 -
63, The avatar preview blob URL created in setAvatarFromFile (and the similar
handler at lines 153-157) must be revoked to avoid memory leaks: before calling
URL.createObjectURL(file) revoke the current avatarPreview URL if present, store
the new URL in setAvatarPreview, and also revoke the last preview in a useEffect
cleanup (return function) that runs on unmount or when avatarPreview changes so
any superseded or unsaved blob URLs are released; update both setAvatarFromFile
and the other file-select handler to follow this revoke-then-create pattern and
add the cleanup useEffect referencing avatarPreview.
Comment on lines
+107
to
+133
| const uploadAvatar = useCallback(async (file: File): Promise<string> => { | ||
| const res = await apiClient.v1.uploads.avatar.presign.$post({ | ||
| json: { | ||
| filename: file.name, | ||
| contentType: file.type, | ||
| size: file.size, | ||
| }, | ||
| }) | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error("Failed to get upload URL") | ||
| } | ||
|
|
||
| const { uploadUrl, fileUrl } = await res.json() | ||
|
|
||
| const uploadRes = await fetch(uploadUrl, { | ||
| method: "PUT", | ||
| body: file, | ||
| headers: { "Content-Type": file.type }, | ||
| }) | ||
|
|
||
| if (!uploadRes.ok) { | ||
| throw new Error("Failed to upload avatar") | ||
| } | ||
|
|
||
| return fileUrl | ||
| }, []) |
There was a problem hiding this comment.
Avoid the two-step avatar write without compensation.
uploadAvatar() writes the file to storage before authClient.updateUser() stores the URL on the user record. If the profile update fails after the PUT, the new object is orphaned and every retry uploads another unused avatar. This needs a compensating delete or a server-side finalize flow.
Also applies to: 139-151
Comment on lines
+73
to
+86
| const [_open, _setOpen] = React.useState(defaultOpen) | ||
| const open = openProp ?? _open | ||
| const setOpen = React.useCallback( | ||
| (value: boolean | ((value: boolean) => boolean)) => { | ||
| const openState = typeof value === "function" ? value(open) : value | ||
| if (setOpenProp) { | ||
| setOpenProp(openState) | ||
| } else { | ||
| _setOpen(openState) | ||
| } | ||
|
|
||
| // This sets the cookie to keep the sidebar state. | ||
| document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}` | ||
| }, |
There was a problem hiding this comment.
This never restores the persisted sidebar state.
setOpen() writes sidebar_state, but _open is always initialized from defaultOpen. After a full reload the cookie is ignored and the sidebar resets instead of restoring the user's last choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/sidebar.tsx` around lines 73 - 86, The sidebar
state is written to a cookie in setOpen but never read back on initialization,
so _open is always set from defaultOpen; update the initialization of
_open/_setOpen (the React.useState call) to first read and parse the
SIDEBAR_COOKIE_NAME cookie (fallback to defaultOpen if absent or invalid) so
persisted state is restored after reload; ensure openProp/setOpenProp still
control the component when provided and keep using document.cookie in setOpen to
update the cookie.
Comment on lines
+475
to
+483
| const sidebarMenuButtonVariants = cva( | ||
| "peer/menu-button flex w-full items-center gap-2 overflow-hidden rounded-md p-2 text-left text-sm ring-sidebar-ring outline-hidden transition-[width,height,padding] group-has-data-[sidebar=menu-action]/menu-item:pr-8 group-data-[collapsible=icon]:size-8! group-data-[collapsible=icon]:p-2! hover:bg-sidebar-accent hover:text-sidebar-accent-foreground focus-visible:ring-2 active:bg-sidebar-accent active:text-sidebar-accent-foreground disabled:pointer-events-none disabled:opacity-50 aria-disabled:pointer-events-none aria-disabled:opacity-50 data-[active=true]:bg-sidebar-accent data-[active=true]:font-medium data-[active=true]:text-sidebar-accent-foreground data-[state=open]:hover:bg-sidebar-accent data-[state=open]:hover:text-sidebar-accent-foreground [&>span:last-child]:truncate [&>svg]:size-4 [&>svg]:shrink-0", | ||
| { | ||
| variants: { | ||
| variant: { | ||
| default: "hover:bg-sidebar-accent hover:text-sidebar-accent-foreground", | ||
| outline: | ||
| "bg-background shadow-[0_0_0_1px_hsl(var(--sidebar-border))] hover:bg-sidebar-accent hover:text-sidebar-accent-foreground hover:shadow-[0_0_0_1px_hsl(var(--sidebar-accent))]", | ||
| }, |
There was a problem hiding this comment.
Use the sidebar tokens directly here.
packages/ui/src/styles/globals.css now defines --sidebar-border and --sidebar-accent as full oklch(...) values. Wrapping them in hsl(...) makes these shadow colors invalid, so the outline variant loses its border and hover treatment.
Suggested fix
variant: {
default: "hover:bg-sidebar-accent hover:text-sidebar-accent-foreground",
outline:
- "bg-background shadow-[0_0_0_1px_hsl(var(--sidebar-border))] hover:bg-sidebar-accent hover:text-sidebar-accent-foreground hover:shadow-[0_0_0_1px_hsl(var(--sidebar-accent))]",
+ "bg-background ring-1 ring-sidebar-border hover:bg-sidebar-accent hover:text-sidebar-accent-foreground hover:ring-sidebar-accent",
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/sidebar.tsx` around lines 475 - 483, The outline
variant inside sidebarMenuButtonVariants is wrapping CSS token variables in
hsl(...), which breaks the new oklch(...) tokens; update the outline variant's
shadow strings to use the tokens directly (e.g., use var(--sidebar-border) and
var(--sidebar-accent) rather than hsl(var(--sidebar-border)) /
hsl(var(--sidebar-accent))) so the border and hover shadow colors render
correctly with the oklch values defined in globals.css.
Comment on lines
+8
to
+16
| React.useEffect(() => { | ||
| const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`) | ||
| const onChange = () => { | ||
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) | ||
| } | ||
| mql.addEventListener("change", onChange) | ||
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) | ||
| return () => mql.removeEventListener("change", onChange) | ||
| }, []) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using mql.matches for consistency with the MediaQueryList approach.
The media query is correctly set up, but the onChange handler checks window.innerWidth directly instead of using the MediaQueryList's matches property. Using mql.matches is more idiomatic and ensures the check stays in sync with the media query definition.
♻️ Suggested improvement
React.useEffect(() => {
const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`)
const onChange = () => {
- setIsMobile(window.innerWidth < MOBILE_BREAKPOINT)
+ setIsMobile(mql.matches)
}
mql.addEventListener("change", onChange)
- setIsMobile(window.innerWidth < MOBILE_BREAKPOINT)
+ setIsMobile(mql.matches)
return () => mql.removeEventListener("change", onChange)
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| React.useEffect(() => { | |
| const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`) | |
| const onChange = () => { | |
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) | |
| } | |
| mql.addEventListener("change", onChange) | |
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) | |
| return () => mql.removeEventListener("change", onChange) | |
| }, []) | |
| React.useEffect(() => { | |
| const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`) | |
| const onChange = () => { | |
| setIsMobile(mql.matches) | |
| } | |
| mql.addEventListener("change", onChange) | |
| setIsMobile(mql.matches) | |
| return () => mql.removeEventListener("change", onChange) | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/hooks/use-mobile.ts` around lines 8 - 16, The useEffect sets
up a MediaQueryList as mql but the onChange handler reads window.innerWidth;
change onChange to use mql.matches (and the initial setIsMobile call to use
mql.matches instead of window.innerWidth) so the state is derived from the
MediaQueryList result; update references in the React.useEffect block (mql,
onChange, setIsMobile, MOBILE_BREAKPOINT) and keep
addEventListener/removeEventListener logic the same.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
apps/web/src/components/settings/my-account-settings.tsx (1)
118-162:⚠️ Potential issue | 🟠 MajorMake the avatar save flow atomic.
Line 133 uploads the blob before Line 157 updates the user record. If
authClient.updateUser()fails after the PUT, the new object is orphaned and every retry leaks another avatar. This still needs a compensating delete or a server-side finalize step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/my-account-settings.tsx` around lines 118 - 162, The avatar save flow currently uploads the blob in uploadAvatar before calling authClient.updateUser in handleSave, causing orphaned uploads if the user update fails; modify the flow to be atomic by changing uploadAvatar to upload to a temporary location or return a temporary upload token (e.g., tempKey) instead of a final fileUrl, then call authClient.updateUser with that tempKey and, after a successful update, call a new finalize endpoint (or an existing finalizeUpload method) to make the file permanent; if updateUser fails, call a deleteUpload or abortUpload endpoint using the tempKey to remove the orphaned blob (ensure uploadAvatar, handleSave, and the finalize/delete endpoints are referenced and wired accordingly).packages/ui/src/components/sidebar.tsx (1)
71-87:⚠️ Potential issue | 🟠 MajorRestore the persisted sidebar state during initialization.
This is still unresolved:
setOpen()writessidebar_state, but_openalways starts fromdefaultOpen, so a full reload drops the user's last choice. Initialize from the cookie first, with atypeof document !== "undefined"guard for prerendered client components.💾 Suggested fix
- const [_open, _setOpen] = React.useState(defaultOpen) + const [_open, _setOpen] = React.useState(() => { + if (typeof document === "undefined") { + return defaultOpen + } + + const cookie = document.cookie + .split("; ") + .find((value) => value.startsWith(`${SIDEBAR_COOKIE_NAME}=`)) + ?.split("=")[1] + + if (cookie === "true") { + return true + } + + if (cookie === "false") { + return false + } + + return defaultOpen + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/sidebar.tsx` around lines 71 - 87, The component currently always initializes _open from defaultOpen so persisted sidebar state written to SIDEBAR_COOKIE_NAME is dropped on reload; change the initialization of _open (the useState for _open/_setOpen) to read the cookie first (guarded with typeof document !== "undefined") and parse its value to a boolean, falling back to defaultOpen if the cookie is absent or unparsable; keep existing setOpen, SIDEBAR_COOKIE_NAME and SIDEBAR_COOKIE_MAX_AGE usage and ensure the cookie-read logic runs only on the client to avoid SSR/prerender errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 91-98: The drag depth counter in handleDragLeave can go negative;
change the decrement logic so dragCountRef.current is clamped at zero (e.g.,
compute newCount = Math.max(0, dragCountRef.current - 1) and assign it back to
dragCountRef.current) and then call setIsDragging(false) only when the clamped
count is 0; keep the existing e.preventDefault() and e.stopPropagation()
behavior. Reference: handleDragLeave, dragCountRef.current, setIsDragging.
- Around line 76-82: The avatar file input handler handleAvatarSelect currently
reads the selected file and calls setAvatarFromFile but doesn't reset the input
value, so selecting the same file twice won't trigger a change; after grabbing
e.target.files?.[0] and calling setAvatarFromFile(file), clear the input by
setting e.currentTarget.value = '' (or e.target.value = '') to reset the file
input so subsequent identical selections emit change events.
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 281-302: SidebarRail currently sets onClick={toggleSidebar} but
then spreads {...props} which lets a consumer-supplied props.onClick overwrite
the built-in toggle; change SidebarRail to merge handlers like SidebarTrigger by
creating a combined onClick that calls toggleSidebar(e) and then calls
props.onClick?.(e) (or vice‑versa if ordering matters), and pass that combined
onClick to the button while still spreading the remaining props so consumer
handlers are invoked without replacing the internal toggle.
- Around line 153-204: The mobile branch of Sidebar is dropping className and
DOM props by spreading {...props} onto Sheet instead of the actual DOM
container; update the mobile branch so Sheet receives only open={openMobile} and
onOpenChange={setOpenMobile}, and move the {...props} spread (including
className, style, id, data-*, etc.) onto SheetContent (the real DOM container),
merging className with the existing class string (e.g., via cn) and preserving
the inline style object for --sidebar-width; keep
SheetHeader/SheetTitle/SheetDescription and children unchanged.
- Around line 95-109: The keyboard handler in the React.useEffect (handleKeyDown
/ SIDEBAR_KEYBOARD_SHORTCUT / toggleSidebar) currently prevents default for
every Ctrl/Cmd+B even when focus is in editable fields; update handleKeyDown to
early-return when the event target is an input, textarea, select, or any element
with isContentEditable true (use event.target instanceof HTMLElement and check
tagName or isContentEditable) so the shortcut is ignored while typing; retain
the existing meta/ctrl + key check and only call event.preventDefault() and
toggleSidebar() when focus is not in an editable element.
---
Duplicate comments:
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 118-162: The avatar save flow currently uploads the blob in
uploadAvatar before calling authClient.updateUser in handleSave, causing
orphaned uploads if the user update fails; modify the flow to be atomic by
changing uploadAvatar to upload to a temporary location or return a temporary
upload token (e.g., tempKey) instead of a final fileUrl, then call
authClient.updateUser with that tempKey and, after a successful update, call a
new finalize endpoint (or an existing finalizeUpload method) to make the file
permanent; if updateUser fails, call a deleteUpload or abortUpload endpoint
using the tempKey to remove the orphaned blob (ensure uploadAvatar, handleSave,
and the finalize/delete endpoints are referenced and wired accordingly).
In `@packages/ui/src/components/sidebar.tsx`:
- Around line 71-87: The component currently always initializes _open from
defaultOpen so persisted sidebar state written to SIDEBAR_COOKIE_NAME is dropped
on reload; change the initialization of _open (the useState for _open/_setOpen)
to read the cookie first (guarded with typeof document !== "undefined") and
parse its value to a boolean, falling back to defaultOpen if the cookie is
absent or unparsable; keep existing setOpen, SIDEBAR_COOKIE_NAME and
SIDEBAR_COOKIE_MAX_AGE usage and ensure the cookie-read logic runs only on the
client to avoid SSR/prerender errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bae61ea-c18a-4aaa-87fc-b2b0f4a84f3a
📒 Files selected for processing (4)
apps/web/src/components/chat/message-action-bar.tsxapps/web/src/components/settings/my-account-settings.tsxpackages/ui/src/components/breadcrumb.tsxpackages/ui/src/components/sidebar.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR implements a comprehensive settings dialog and user account management feature, including avatar upload functionality, user profile editing (name, bio, status), and the underlying API routes and UI infrastructure to support these features.
Key Changes
Frontend Components
apps/web/src/components/settings/settings-dialog.tsx): Two-pane settings UI with searchable navigation and main content; renders My Account settings and "coming soon" placeholders for other sections.apps/web/src/components/settings/my-account-settings.tsx): Account settings with:Backend API Routes
apps/api/src/routes/v1/uploads/):Database & Auth
packages/db/src/schemas/users.ts): adds bio (varchar 255) and status (varchar 128).packages/auth/src/lib/auth.ts): exposes bio and status in better-auth user additionalFields (returned: true, optional).Note: I inspected the repository and confirmed that packages/db/src/schemas/users.ts already contains bio and status, and packages/auth/src/lib/auth.ts includes bio/status in additionalFields. There are no obvious explicit migration files under packages/db/, though the project includes drizzle config and generated schema.
UI Infrastructure & Misc
apps/web/src/context/settings-context.tsx): provider and useSettings hook to control settings dialog open/close.apps/web/src/routes/_authenticated.tsx).packages/ui/src/components/breadcrumb.tsx)packages/ui/src/components/sidebar.tsx)packages/ui/src/hooks/use-mobile.ts)Quality Assessment
Strengths:
Concerns / Notes:
Confidence Score: 4/5
Changes are well-scoped and follow existing patterns; types and validations look sound. The primary remaining checklist items are ensuring the database schema is migrated/applied in environments that don't yet have the new columns and a quick pass for additional tests or minor accessibility nits.