Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds image upload utilities and inputs, introduces banner helpers to distinguish image URLs vs hex colors, restructures the user page header layout (banner, pfp, name variants, follow/stats/about blocks), updates several UI/styling choices across header and settings components, and adjusts tests to exercise the expanded name API. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as BannerImageInput / ImgSelect
participant Hook as useImageUpload
participant Header as UserPageHeaderClient
participant Upload as multiPartUpload
participant API as ProfileMutation/API
User->>UI: drag/select file
UI->>Hook: onFileChange(file)
Hook->>Hook: validate type & size
alt invalid
Hook-->>UI: error + shake
UI-->>User: show validation error
else valid
Hook-->>Header: setFile(file)
Header-->>UI: show preview
User->>Header: save/submit banner change
Header->>Upload: multiPartUpload(file)
Upload-->>Header: uploadedImageUrl
Header->>API: mutate profile (banner_1 = uploadedImageUrl or gradient)
API-->>Header: mutation result
Header-->>User: update UI with new banner
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🤖 Fix all issues with AI agents
In `@components/user/settings/UserSettingsBannerImageInput.tsx`:
- Around line 52-69: The drag state can remain true after a file is dropped;
modify handleDrop to call setDragging(false) (before/after invoking
onFileChange) so the UI highlight is cleared on drop; locate the handleDrop
function and add setDragging(false) alongside the existing
e.preventDefault/e.stopPropagation and onFileChange usage (handleDrag already
handles drop, but ensure handleDrop explicitly resets dragging).
In `@components/user/user-page-header/about/UserPageHeaderAbout.tsx`:
- Around line 52-61: The outer <button> in UserPageHeaderAbout.tsx wraps the
UserPageHeaderAboutStatement component (which itself renders a "See more"
<button>), creating nested interactive elements; replace the outer element with
a non-button wrapper (e.g., a div or span) that preserves button semantics by
keeping the onClick handler (onEditClick), aria-label, className, and adding
role="button", tabIndex={0}, and an onKeyDown handler that triggers onEditClick
for Enter/Space so keyboard users still activate it; target the wrapper around
UserPageHeaderAboutStatement in UserPageHeaderAbout.tsx and keep
UserPageHeaderAboutStatement unchanged.
In `@components/user/user-page-header/name/UserPageHeaderName.tsx`:
- Around line 41-43: profileEnabledAt may be a malformed string so new
Date(profileEnabledAt).getTime() can return NaN and produce "Invalid Date";
update the logic that computes profileEnabledLabel (the const
profileEnabledLabel) to first parse the timestamp (e.g., Date.parse or new
Date(profileEnabledAt).getTime()) and guard with a finite check
(Number.isFinite) before calling formatTimestampToMonthYear; if the parsed
timestamp is not finite, set profileEnabledLabel to null so
formatTimestampToMonthYear is only invoked with a valid numeric timestamp.
🧹 Nitpick comments (1)
components/user/user-page-header/about/UserPageHeaderAboutStatement.tsx (1)
20-43: Avoid character-count heuristics for toggle visibility.
Line 20’s length check can miss real truncation on narrow screens, leaving clamped text with no way to expand. Consider detecting actual overflow instead.🔧 Possible refactor to compute toggle visibility from overflow
-import { useState } from "react"; +import { useLayoutEffect, useRef, useState } from "react"; @@ - const showToggle = statement.statement_value.length > 240; + const contentRef = useRef<HTMLDivElement | null>(null); + const [showToggle, setShowToggle] = useState(false); + useLayoutEffect(() => { + if (expanded) return; + const el = contentRef.current; + if (!el) return; + const compute = () => { + setShowToggle(el.scrollHeight > el.clientHeight + 1); + }; + compute(); + const ro = new ResizeObserver(compute); + ro.observe(el); + return () => ro.disconnect(); + }, [statement.statement_value, expanded]); @@ - <div + <div + ref={contentRef} className={`tw-mb-0 tw-text-md tw-text-zinc-300 tw-leading-relaxed tw-font-normal tw-whitespace-pre-line ${clampClass}`} >
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/user/user-page-header/about/UserPageHeaderAbout.tsx`:
- Around line 50-53: The className in the UserPageHeaderAbout component
currently concatenates without a space so when canEdit is true it yields
"tw-gap-2tw-group" and the group styles don't apply; update the className
expression around the div (the JSX that builds the class string using canEdit)
to include a leading space before "tw-group" (e.g., change the conditional from
? "tw-group" : "" to ? " tw-group" : "") so the final class list is properly
spaced and allows group hover/focus behavior.
In `@components/user/user-page-header/banner/UserPageHeaderBanner.tsx`:
- Around line 36-44: The inline style for backgroundImage in
UserPageHeaderBanner is interpolating scaledBannerUrl directly which can break
CSS or allow injection; update the backgroundImage construction to sanitize the
value with CSS.escape(scaledBannerUrl) before inserting into the url() so the
inline style uses an escaped URL (refer to the backgroundImage property and the
scaledBannerUrl variable in UserPageHeaderBanner.tsx).
🧹 Nitpick comments (4)
components/user/user-page-header/pfp/UserPageHeaderPfpWrapper.tsx (1)
55-55: Redundant background opacity classes.The overlay has conflicting/redundant classes:
tw-bg-black tw-bg-black/50 tw-bg-opacity-50. Thetw-bg-blacksets a solid black background, whiletw-bg-black/50sets 50% opacity black, andtw-bg-opacity-50is another opacity modifier. Onlytw-bg-black/50is needed for a semi-transparent black overlay.Proposed fix
- <div className="edit-profile tw-absolute tw-inset-0 tw-z-20 tw-rounded-xl tw-bg-black tw-bg-black/50 tw-bg-opacity-50 tw-transition tw-duration-300 tw-ease-out"> + <div className="edit-profile tw-absolute tw-inset-0 tw-z-20 tw-rounded-xl tw-bg-black/50 tw-transition tw-duration-300 tw-ease-out">components/user/settings/UserSettingsBannerImageInput.tsx (1)
52-52: Consider using specific event types instead ofany.The event handlers use
anytype which loses type safety. Consider using the appropriate React event types for better type checking.Proposed fix
- const handleDrop = (e: any) => { + const handleDrop = (e: React.DragEvent<HTMLDivElement>) => { e.preventDefault(); e.stopPropagation(); if (e?.dataTransfer?.files?.length) { onFileChange(e.dataTransfer.files[0]); } }; - const handleDrag = (e: any) => { + const handleDrag = (e: React.DragEvent<HTMLDivElement>) => { e.preventDefault(); e.stopPropagation();And for the input onChange:
- onChange={(e: any) => { + onChange={(e: React.ChangeEvent<HTMLInputElement>) => {Also applies to: 60-60, 148-148
components/user/user-page-header/banner/UserPageHeaderEditBanner.tsx (1)
154-179: RedundantsetIsUploading(false)in catch block.The
setIsUploading(false)on line 171 is redundant since thefinallyblock on line 174 will always execute and set it to false. The catch-block call can be removed.Proposed fix
} catch (error) { const message = error instanceof Error ? error.message : "Failed to upload banner image"; setToast({ message, type: "error", }); - setIsUploading(false); return; } finally { setIsUploading(false); }components/user/user-page-header/UserPageHeaderClient.tsx (1)
184-202: Consider using a singleUserPageHeaderNamewithvariant="full"for simplicity.Two separate
UserPageHeaderNamecomponents are rendered withvariant="title"andvariant="meta". Unless specific layout spacing between them is required, a single component withvariant="full"could simplify this section.If the current layout with
tw-mt-2between title and meta is intentional for specific spacing control, this is fine as-is.
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/user/settings/UserSettingsBannerImageInput.tsx`:
- Around line 34-40: The MIME-type check currently rejects files with empty
file.type; update the validation in UserSettingsBannerImageInput to treat an
empty MIME as a fallback case by deriving a MIME (or a comparable token) from
file.name extension and then checking against ACCEPTED_FORMATS instead of only
file.type; specifically, if file.type is falsy, extract the extension from
file.name (after the last dot), map it to the appropriate image MIME (or
normalize to the extension string format your ACCEPTED_FORMATS uses), then
perform the ACCEPTED_FORMATS lookup and call setToast({type: "error", message:
"Invalid file type"}) only when that combined/derived check fails. Ensure you
reference ACCEPTED_FORMATS, file.type, file.name and the setToast call when
applying the fix.
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/user/settings/UserSettingsImgSelectFile.tsx`:
- Line 41: Remove the dead inputRef from the UserSettingsImgSelectFile
component: delete the declaration const inputRef =
useRef<HTMLInputElement>(null) and remove any ref={inputRef} prop on the file
input element since the hidden input is a child of the label and does not
require programmatic access; ensure no other code in the component references
inputRef before committing the removal.
🧹 Nitpick comments (1)
components/user/settings/useImageUpload.ts (1)
37-38: Consider cleaning up the shake timeout on unmount.The
setTimeoutfor resetting shake state isn't cleaned up if the component unmounts during the 300ms window. This is a minor concern given the short duration, but for robustness you could use a ref or cleanup function.♻️ Optional fix using useEffect cleanup pattern
+ const shakeTimeoutRef = useRef<NodeJS.Timeout | null>(null); + + // Cleanup on unmount + useEffect(() => { + return () => { + if (shakeTimeoutRef.current) { + clearTimeout(shakeTimeoutRef.current); + } + }; + }, []); + const onFileChange = useCallback( (file: File) => { setError(null); // ... if (file.size > maxSizeBytes) { setError(`File size must be less than ${maxSizeLabel}`); setShake(true); - setTimeout(() => setShake(false), 300); + shakeTimeoutRef.current = setTimeout(() => setShake(false), 300); return; }
Signed-off-by: ragnep <ragneinfo@gmail.com>
Signed-off-by: ragnep <ragneinfo@gmail.com>
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/user/user-page-header/banner/UserPageHeaderEditBanner.tsx (1)
43-44:⚠️ Potential issue | 🟡 MinorModal can be dismissed during save operation.
useClickAwayanduseKeyPressEventwill close the modal even whileisSavingis true, potentially interrupting the upload/save operation. The Cancel button is correctly disabled during save (line 255), but these handlers bypass that protection.Consider wrapping
onCloseto checkisSavingstate:🛡️ Proposed fix
+ const handleClose = () => { + if (isSaving) return; + onClose(); + }; + const modalRef = useRef<HTMLDivElement>(null); - useClickAway(modalRef, onClose); - useKeyPressEvent("Escape", onClose); + useClickAway(modalRef, handleClose); + useKeyPressEvent("Escape", handleClose);Note: Since
isSavingdepends onisUploadingstate (line 116), which is set inside the component, you may need to use a ref to track the saving state for the callbacks, or restructure to definehandleCloseafter the state declarations.
🤖 Fix all issues with AI agents
In `@components/user/settings/UserSettingsImgSelectFile.tsx`:
- Around line 19-30: The ALLOWED_EXTENSIONS lookup can miss valid files because
isValidImageType falls back to ALLOWED_EXTENSIONS.has(getFileExtension(file))
without normalizing; update isValidImageType to normalize the extension from
getFileExtension(file) (strip any leading '.' and convert to upper case) before
checking ALLOWED_EXTENSIONS, keeping the existing MIME check against
ACCEPTED_FORMATS unchanged; reference functions/constants: isValidImageType,
getFileExtension, ALLOWED_EXTENSIONS, ACCEPTED_FORMATS.
- Around line 40-50: The circular dependency is caused by isValidImageType,
ACCEPTED_FORMATS, and ACCEPTED_FORMATS_DISPLAY being defined in
UserSettingsImgSelectFile and imported into useImageUpload; extract those three
symbols into a new shared module (e.g., imageValidation.ts) in the same
directory, export them, then update both UserSettingsImgSelectFile.tsx and the
useImageUpload module to import isValidImageType, ACCEPTED_FORMATS, and
ACCEPTED_FORMATS_DISPLAY from the new imageValidation.ts file so both modules
depend on the utility module instead of each other.
In `@components/user/user-page-header/banner/UserPageHeaderEditBanner.tsx`:
- Around line 205-210: The backdrop button currently calls onClose directly
which allows closing during a save; update the button in
UserPageHeaderEditBanner to use the guarded close handler (e.g., handleClose) or
wrap onClose with the save guard (e.g., onClick={() => { if (!isSaving)
onClose(); }}), referencing the existing handleClose/isSaving logic you added
earlier so the backdrop respects the "prevent close while saving" behavior.
🧹 Nitpick comments (1)
components/user/settings/UserSettingsImgSelectFile.tsx (1)
64-70: Addsizesfor Next.jsImagewithfill.Without
sizes, Next.js emits warnings and can’t optimize layout sizing. A fixed container can use a fixed size string.📐 Suggested update
<Image src={imageToShow} alt="Profile image" fill + sizes="160px" className="tw-rounded-sm tw-object-contain" />
GelatoGenesis
left a comment
GelatoGenesis
left a comment
There was a problem hiding this comment.
Please address the stuff I mentioned in PR wave. Both the validation and prescaling.
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/user/user-page-header/banner/UserPageHeaderBanner.tsx`:
- Around line 45-52: The gradient interpolation uses banner1Color/banner2Color
from getBannerColorValue without validating that non-image strings are safe CSS
colors; update getBannerColorValue (used in UserPageHeaderBanner) to validate
non-image values against a whitelist of allowed color formats (e.g., hex
`#RRGGBB/`#RGB with optional alpha, rgb()/rgba(), hsl()/hsla(), or a controlled
list of CSS color names) and return a safe fallback (e.g., a hard-coded hex)
when validation fails; ensure the component uses these validated/sanitized
banner1Color and banner2Color values for the linear-gradient so untrusted API
strings cannot inject arbitrary CSS or external resources.
Signed-off-by: ragnep <ragneinfo@gmail.com>
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/user/user-page-header/banner/UserPageHeaderEditBanner.tsx (1)
116-121:⚠️ Potential issue | 🟡 MinorInconsistent error handling in
onErrorcallback.The
error as stringcast on line 118 won't properly extract the message iferroris anErrorobject - it will display[object Object]or similar. TheuploadBannerImagefunction (lines 139-142) handles this correctly; apply the same pattern here.🛠️ Proposed fix
onError: (error: unknown) => { + const message = + error instanceof Error ? error.message : String(error); setToast({ - message: error as string, + message, type: "error", }); },
🧹 Nitpick comments (1)
components/user/settings/UserSettingsBannerImageInput.tsx (1)
73-75: Consider deriving the displayed formats fromACCEPTED_FORMATS_DISPLAYto avoid drift.The hardcoded text
"JPEG, JPG, PNG, GIF — max 2MB"duplicates information already available from the importedACCEPTED_FORMATS_DISPLAYconstant. If the accepted formats change inimageValidation.ts, this display text won't update automatically, leading to inconsistency between what's shown and what's actually validated.Suggested refactor to use the constant
<p className="tw-text-xs tw-font-normal tw-text-iron-400"> - JPEG, JPG, PNG, GIF — max 2MB + {ACCEPTED_FORMATS_DISPLAY.toUpperCase().replaceAll(".", "")} — max 2MB </p>Alternatively, consider passing
maxSizeLabelas a prop or importing it from shared config for full consistency.
Signed-off-by: ragnep <ragneinfo@gmail.com>
Signed-off-by: ragnep <ragneinfo@gmail.com>
Signed-off-by: ragnep <ragneinfo@gmail.com>
|
2 participants
Summary by CodeRabbit
New Features
Style
Tests