feat(profile): About bio, settings UX, and updateUser (1/4) - #434
Conversation
Add bio validation and updateUser/updateUserAvatar mutations, surface About on profile/settings, and fix theme toggle, background Save dirty state, and toast close button placement. Co-authored-by: Cursor <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Use lazy useState init for profile background (matching main) and restore an eslint disable for intentional themePreference sync on login. Co-authored-by: Cursor <[email protected]>
Remove unused ts-expect-error after store avatar typing, coerce staged chat avatar to string, and parse settings avatar objects to URLs. Co-authored-by: Cursor <[email protected]>
motirebuma
left a comment
There was a problem hiding this comment.
hey @niamao nice work, this is a very well put-together PR! Good breadth of tests, thorough backend validation, a few things to tidy up but nothing major. Let me put together some thoughts below.
What's good
-
Bio validation via
normalizeBio()with html rejection, length limiting, and trimming is exactly what we need for user-supplied plaintext. The regex pattern is also simple and straightforward (/<\/?[a-z][\s\S]>/iis a good match for any HTML tag). Symmetric validation on the BE and FE (same 500 character limit, HTML escaping) means the user gets immediate feedback in the UI while the stricter checks (length, HTML) are still enforced on the server. Great work! -
updateUserresolver has good checks in place: user must sign in, can only update their own account (except forcontributorBadgewhich is admin-only), uniqueness checks for username/email, and usesbcryptto update the password. Also good that you did$setthe fields rather than replacing the entire document, to avoid accidental deletion of fields that the user didn't submit -
updateUserAvatarresolver looks good - separates avatar update logic from the user update, properly validates input. All good! -
UX improvements to the settings page: char counter for bio, dirty checking for profile background (pattern/color), using
setThemeto apply theme rather thantoggleTheme, moving toast close button to the right, etc - all very good details! -
Unit tests for the bio validation, resolver tests for
updateUser(auth, owns account, admin, bad HTML), tests forupdateUserAvatar, and settings page e2e tests all look good.
Things to address
-
ReputationDisplay.formatDatehas a type mismatch: you've changed the component to acceptdateValue: string | Date | null | undefinedand inUserReputation.tsit seems like the backend resolver is also handling numbers as dates (e.g.lastCalculatedin'parses numeric timestamps for lastCalculated'test). However, in the component, theformatDatefunction now expectsstring | Dateinput, but the resolver returns empty string for invalid dates. If the resolver returns empty string, that would show up as "Not available" in the reputation display which is correct, but it's possible that the test'parses numeric timestamps for lastCalculated'is passing because the resolver is getting a string representation of a number (e.g. "1645679200") and then.lean()is turning that into a number. I think it would be good to make sure that the API is returning strings (via.toISOString()or something similar) rather than numbers, since the client code expectsstring | Date. -
ThemeContextProvidersubscribes tothemePreferencebut doesn't actually use it to set the theme anywhere - I don't think you've usedthemePreferencein theThemeContextProviderin this PR, because you setconst themePreference = useAppStore(...)but the diff doesn't show any use of that variable. The comment says "store will hydrate on mount", but if the user updates their device settings (e.g. enables dark mode), do we want their theme to change? If not, that's fine, but then why subscribe tothemePreferenceat all? -
The login response now includes
avatarandbio- in theaddCreatorToUserchange, I think it's a good idea to includeavatarin the login response, however, the type for the avatar isstring | nullin the response while the actual field in the database is an object (avataaars qualities). Is the avatar in the login response stringified JSON or is it sent as an object directly (viaJSON.stringify()in the JWT)? Either way, make sure that the client code can handle it correctly.
Nitpicks (non-critical)
-
ProfileControllernow checks!targetUsername || loadingbefore rendering - this is a good change to prevent hydration errors, buttargetUsernameshould be empty only when the route param and the store's username are both empty, i.e. the user is not authenticated and is trying to visit/dashboard/profiledirectly. In that case, it would be better to redirect them to the dashboard/login rather than show a loading spinner indefinitely. -
Changed "Return to homepage" link from
/searchto/- looks good since the app now uses/rather than/searchas the homepage.
Thank you @niamao
Serialize reputation dates to ISO strings on the API, clarify theme preference sync is account-based (not OS), type login avatar as object or string, and redirect empty /dashboard/profile to login after hydrate. Co-authored-by: Cursor <[email protected]>
|
Addressed review feedback in
|
motirebuma
left a comment
There was a problem hiding this comment.
hey @niamao, I've reviewed the latest changes and noticed all the previous issues were resolved. Well done with the follow-up commit.
I've double-checked all the items mentioned in my initial review below and didn't find any recurring issues this time.
Previously raised items
-
ThemeContextProvider themePreference - FIXED. The theme preference from Zustand store is correctly picked up in a useEffect and persisted in localStorage via writeStoredThemeMode(). Cross-device theme consistency should be working now
-
ProfileController redirect - FIXED. The useSyncExternalStore() correctly picks up the hydration status of Zustand persist and the router is redirected to /auths/login without showing the loading spinner when there's no username available.
-
Login avatar handling - the addCreatorToUser change correctly passes the avatar object (not stringified) to the client, and the test confirms that both avataaars quality objects and URLs are accepted and handled properly by the frontend.
What's good (not changed from my original review)
Bio validation via normalizeBio() - consistent checks on both frontend and backend, HTML escaping, max length and trim
updateUser resolver - user ownership + auth checks, contributorBadge permission logic, name and username uniqueness validation, password encryption with bcrypt
updateUserAvatar resolver - proper separation of concerns, input validation
Settings UX - bio textarea with character counter, dirty-checking for background color/pattern, theme preference checkbox fixed, toast close button on the right
Tests coverage - all resolvers and settings frontend have necessary tests
Verdict: approved, ready to merge.
Thank you @niamao
Stack
Part 1 of 4 — merge this first.
profile-bioavatar-consistencypresence-persistapi-wiringRelated issue: #362
Summary
updateUser/updateUserAvatarwith bio validation (plain text, max length)bio(andavatarfor later stack parts)CI follow-ups (on this branch)
set-state-in-effectin theme / profile background hooks@ts-expect-error, staged-chat avatar coercion, settings avatar URL parseTest plan