Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors avatar rendering into an internal AvatarContent with image-error fallback; tweaks Grant Rep button styling; simplifies GrantRepDialog layout and routes Cancel via child onCancel; moves RepSearchState into a new enum module; converts rep-search state to derived/memoized values, simplifies dropdown rendering, and threads optional onCancel through rep components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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.
🧹 Nitpick comments (1)
components/common/OverlappingAvatars.tsx (1)
37-70:avatarRingprop is ignored in the fallback case.The
avatarRingprop is accepted but only applied to the successful image render (line 67). The fallback<div>(lines 52-56) uses hardcoded ring styling instead of the passed prop. While the current hardcoded values happen to match the usage at line 83-84, this creates a maintenance risk if the ring style needs to change.♻️ Suggested fix to apply avatarRing consistently
if (!pfpUrl || imgError) { return ( - <div className="tw-flex tw-h-full tw-w-full tw-items-center tw-justify-center tw-rounded-full tw-bg-iron-700 tw-ring-[1.5px] tw-ring-black"> + <div className={`tw-flex tw-h-full tw-w-full tw-items-center tw-justify-center ${avatarRing}`}> <span className="tw-text-[0.625rem] tw-font-semibold tw-text-iron-300"> {fallback ?? "?"} </span> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/common/OverlappingAvatars.tsx` around lines 37 - 70, The AvatarContent component ignores the avatarRing prop for the fallback UI: when pfpUrl is missing or imgError is true the fallback <div> uses hardcoded ring classes instead of using avatarRing. Fix AvatarContent by applying the avatarRing variable to the fallback div’s className (e.g., merge avatarRing with the existing layout classes) and remove or replace the hardcoded ring classes (tw-ring-[1.5px] tw-ring-black) so the same ring styling is used whether rendering the <img> or the fallback; keep other classes (tw-flex, tw-rounded-full, etc.) and ensure ariaLabel/fallback behavior stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/common/OverlappingAvatars.tsx`:
- Around line 37-70: The AvatarContent component ignores the avatarRing prop for
the fallback UI: when pfpUrl is missing or imgError is true the fallback <div>
uses hardcoded ring classes instead of using avatarRing. Fix AvatarContent by
applying the avatarRing variable to the fallback div’s className (e.g., merge
avatarRing with the existing layout classes) and remove or replace the hardcoded
ring classes (tw-ring-[1.5px] tw-ring-black) so the same ring styling is used
whether rendering the <img> or the fallback; keep other classes (tw-flex,
tw-rounded-full, etc.) and ensure ariaLabel/fallback behavior stays unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/common/OverlappingAvatars.tsxcomponents/user/rep/header/UserPageRepHeader.tsxcomponents/user/rep/new-rep/GrantRepDialog.tsxcomponents/user/rep/new-rep/UserPageRepNewRepSearch.tsxcomponents/user/rep/new-rep/UserPageRepNewRepSearchDropdown.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/common/OverlappingAvatars.tsx (1)
61-71: Duplicate CSS classes in className.Line 69 includes
tw-object-cover tw-rounded-fullexplicitly, butavatarRing(defined at line 85-86) already contains both of these classes. This results in redundant class names in the output.♻️ Suggested fix
<Image src={getScaledImageUri(pfpUrl, ImageScale.W_AUTO_H_50)} alt={ariaLabel ?? "Profile"} fill sizes="28px" unoptimized onError={() => setImgError(true)} - className={`tw-object-cover tw-rounded-full ${avatarRing}`} + className={avatarRing} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/common/OverlappingAvatars.tsx` around lines 61 - 71, The Image component in OverlappingAvatars.tsx is producing duplicate CSS classes because you append "tw-object-cover tw-rounded-full" while avatarRing already contains those; remove the redundancy by using only avatarRing in the Image's className (or alternatively remove those classes from avatarRing and keep them inline) so that className for the Image is not duplicating styles; update the Image JSX (and/or the avatarRing definition) accordingly and ensure setImgError/onError handling remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/common/OverlappingAvatars.tsx`:
- Around line 61-71: The Image component in OverlappingAvatars.tsx is producing
duplicate CSS classes because you append "tw-object-cover tw-rounded-full" while
avatarRing already contains those; remove the redundancy by using only
avatarRing in the Image's className (or alternatively remove those classes from
avatarRing and keep them inline) so that className for the Image is not
duplicating styles; update the Image JSX (and/or the avatarRing definition)
accordingly and ensure setImgError/onError handling remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/user/rep/new-rep/UserPageRepNewRepSearch.tsx (1)
101-110: Consider using a primitive value to key the prefill instead of object identity.The render-phase prefill pattern here is safe with
useRepAllocationbecauserepStateis a stable state value—it only changes when the useEffect dependencies change. However, relying on object identity comparison is fragile. Since you need to detect whenrater_contributionchanges, key off that primitive instead:const [prevContribution, setPrevContribution] = useState(repState?.rater_contribution); if (repState?.rater_contribution !== prevContribution) { setPrevContribution(repState.rater_contribution); setAmountStr(`${repState.rater_contribution}`); }This is more resilient and makes the intent clearer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx` around lines 101 - 110, Replace the object-identity prefill check using prevRepState with a primitive-based check keyed on repState.rater_contribution: track a prevContribution via useState and compare repState?.rater_contribution to prevContribution; when they differ call setPrevContribution(repState.rater_contribution) and setAmountStr(`${repState.rater_contribution}`). Update references to prevRepState/setPrevRepState to use prevContribution/setPrevContribution and remove the old object comparison logic that used repState identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx`:
- Around line 200-204: The submit handler onSearchSubmit is using debouncedValue
which can ignore Enter presses inside the debounce window; change it to use the
live input state (the uncontrolled/controlled input variable used to track the
current text, e.g., value or searchValue) instead of debouncedValue, keep
event.preventDefault(), and call onRepSelect with the current live input
(optionally trimmed) so Enter always triggers a submit regardless of debounce.
- Around line 161-162: The endpoint construction in UserPageRepNewRepSearch
currently uses `profiles/${profile.query ?? ""}/rep/rating`, which can produce
an invalid path when profile.query is missing; update the submit flow (where
endpoint is built) to validate that `profile.query` is present before posting
and fail fast (return/throw or disable the submit) if it's falsy, e.g., guard in
the submit handler or form validation inside the component so you never call the
API with an empty profile segment; ensure any error path logs or surfaces a
clear message and reference the same identifiers (`profile.query` and the
endpoint assembly) when implementing the guard.
---
Nitpick comments:
In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx`:
- Around line 101-110: Replace the object-identity prefill check using
prevRepState with a primitive-based check keyed on repState.rater_contribution:
track a prevContribution via useState and compare repState?.rater_contribution
to prevContribution; when they differ call
setPrevContribution(repState.rater_contribution) and
setAmountStr(`${repState.rater_contribution}`). Update references to
prevRepState/setPrevRepState to use prevContribution/setPrevContribution and
remove the old object comparison logic that used repState identity.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__tests__/components/user/rep/new-rep/UserPageRepNewRepSearchDropdown.test.tsxcomponents/user/rep/new-rep/GrantRepDialog.tsxcomponents/user/rep/new-rep/UserPageRepNewRep.tsxcomponents/user/rep/new-rep/UserPageRepNewRepSearch.tsxcomponents/user/rep/new-rep/UserPageRepNewRepSearchDropdown.tsxcomponents/user/rep/new-rep/rep-search-types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- components/user/rep/new-rep/GrantRepDialog.tsx
- components/user/rep/new-rep/UserPageRepNewRepSearchDropdown.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx`:
- Around line 130-133: The onRepSelect handler (and the submit flow around the
same input usage) currently validates and submits the raw rep string; trim the
input first to avoid accepting whitespace-only or padded values—i.e., call
.trim() on the incoming rep value before comparing against SEARCH_LENGTH.MIN/MAX
in onRepSelect and use the trimmed value when passing to the submit function
(the same change should be applied where the input is read/submitted around
lines 201–205), ensuring all validations and downstream calls reference the
trimmedRep variable instead of the original raw input.
- Around line 101-110: Move the prefill logic out of render into a useEffect:
stop comparing and calling setPrevRepState/setAmountStr during render; instead
create a useEffect that depends on selectedCategory (and repState) and inside it
check whether the category has changed (track a lastPrefilledCategory ref/state)
before calling setAmountStr and updating prevRepState; reference the existing
symbols repState, setPrevRepState, setAmountStr, and the
useRepAllocation/selectedCategory to locate the logic and ensure the effect only
runs when selectedCategory changes to avoid overwriting user edits or causing
render-time state updates.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
components/user/rep/new-rep/UserPageRepNewRepSearch.tsx (2)
101-110:⚠️ Potential issue | 🟠 MajorMove render-time prefill state updates into an effect.
Line 105 updates state during render and compares object identity (
repState !== prevRepState), which can cause rerender churn and unexpectedamountStrresets whenrepStatereference changes. PreferuseEffectkeyed by category transition (or a stable repState key) to avoid render-phase updates.Suggested refactor
-import { useContext, useMemo, useRef, useState } from "react"; +import { useContext, useEffect, useMemo, useRef, useState } from "react"; @@ - // Pre-fill amountStr when repState changes (e.g. after category selection). - // Uses the React "adjust state during render" pattern so the user can still - // freely edit the value after pre-fill. - const [prevRepState, setPrevRepState] = useState(repState); - if (repState !== prevRepState) { - setPrevRepState(repState); - if (repState) { - setAmountStr(`${repState.rater_contribution}`); - } - } + const lastPrefilledCategoryRef = useRef<string | null>(null); + useEffect(() => { + if (!selectedCategory || !repState) return; + if (lastPrefilledCategoryRef.current === selectedCategory) return; + setAmountStr(`${repState.rater_contribution}`); + lastPrefilledCategoryRef.current = selectedCategory; + }, [selectedCategory, repState]);#!/bin/bash set -euo pipefail # Verify whether `repState` from useRepAllocation is reference-stable or recreated frequently. HOOK_FILE="$(fd 'useRepAllocation.(ts|tsx)$' | head -n1)" SEARCH_FILE="$(fd 'UserPageRepNewRepSearch.tsx$' | head -n1)" echo "=== useRepAllocation source ===" cat -n "$HOOK_FILE" | sed -n '1,260p' echo "=== repState construction/memo hints ===" rg -n -C3 'repState|useMemo|return \{' "$HOOK_FILE" echo "=== render-time state update sites in UserPageRepNewRepSearch ===" rg -n -C3 'prevRepState|setPrevRepState|setAmountStr' "$SEARCH_FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx` around lines 101 - 110, The render-time state update should be moved into a useEffect to avoid changing state during render: replace the inline comparison of repState !== prevRepState with a useEffect that depends on a stable key (e.g. repState?.id or repState?.rater_contribution) and inside the effect call setPrevRepState(...) and setAmountStr(`${repState.rater_contribution}`) only when that key indicates a real category transition; update references to prevRepState, setPrevRepState, repState, and setAmountStr accordingly and ensure the effect’s dependency array uses the stable key (or repState?.rater_contribution) rather than the object identity.
130-143:⚠️ Potential issue | 🟡 MinorNormalize category input before validation and submission.
Line 131 and Line 203 use raw input; whitespace-padded values can pass length checks and be sent downstream. Trim once and use the normalized value consistently in validation, availability check, and state updates.
Suggested fix
const onRepSelect = async (rep: string) => { - if (rep.length < SEARCH_LENGTH.MIN || rep.length > SEARCH_LENGTH.MAX) + const normalizedRep = rep.trim(); + if ( + normalizedRep.length < SEARCH_LENGTH.MIN || + normalizedRep.length > SEARCH_LENGTH.MAX + ) return; @@ await commonApiFetch<boolean>({ endpoint: "/rep/categories/availability", params: { - param: rep, + param: normalizedRep, }, }); - setSelectedCategory(rep); - setRepSearch(rep); + setSelectedCategory(normalizedRep); + setRepSearch(normalizedRep); @@ const onSearchSubmit = (event: React.FormEvent<HTMLFormElement>) => { event.preventDefault(); - if (!repSearch) return; - void onRepSelect(repSearch); + const searchValue = repSearch.trim(); + if (!searchValue) return; + void onRepSelect(searchValue); };Also applies to: 201-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx` around lines 130 - 143, Trim the incoming category string once at the start of onRepSelect (e.g., const normalized = rep.trim()), use that normalized value for length validation, the commonApiFetch params, and when calling setSelectedCategory and setRepSearch so you never validate or send whitespace-padded values; apply the same normalization pattern to the other handler that updates/searches the rep (the one that sets rep search state around lines 201–205) so all validations, API calls (commonApiFetch), and state updates use the trimmed value consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/user/rep/new-rep/UserPageRepNewRepSearch.tsx`:
- Around line 101-110: The render-time state update should be moved into a
useEffect to avoid changing state during render: replace the inline comparison
of repState !== prevRepState with a useEffect that depends on a stable key (e.g.
repState?.id or repState?.rater_contribution) and inside the effect call
setPrevRepState(...) and setAmountStr(`${repState.rater_contribution}`) only
when that key indicates a real category transition; update references to
prevRepState, setPrevRepState, repState, and setAmountStr accordingly and ensure
the effect’s dependency array uses the stable key (or
repState?.rater_contribution) rather than the object identity.
- Around line 130-143: Trim the incoming category string once at the start of
onRepSelect (e.g., const normalized = rep.trim()), use that normalized value for
length validation, the commonApiFetch params, and when calling
setSelectedCategory and setRepSearch so you never validate or send
whitespace-padded values; apply the same normalization pattern to the other
handler that updates/searches the rep (the one that sets rep search state around
lines 201–205) so all validations, API calls (commonApiFetch), and state updates
use the trimmed value consistently.
simo6529
approved these changes
Feb 27, 2026
|
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Style
Refactor
New Features
Tests