Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new generic ExpandableSection React component with styled MUI subcomponents, Storybook stories, Vitest tests, token balance fixtures, and sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ All snapshot tests passed |
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
`@src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsx`:
- Around line 63-70: Add an interaction assertion to the 'matches snapshot when
expansion is disabled' test for ExpandableSection: after rendering with
shouldExpand={false} and renderHeader={defaultHeader}, simulate a user click on
the header (use the header text from defaultHeader or query by role) and assert
that the section does not expand—e.g., expect the details/children not to be
visible or expect the header's aria-expanded (or equivalent attribute/state) to
remain false—so clicking the header does not toggle expansion.
In
`@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx`:
- Around line 65-70: The mapped elements returned from the array .map (the JSX
root StyledContent in the map) lack a stable key which causes React warnings;
update the mapping callback to supply a unique, stable key prop on StyledContent
(e.g., derive it from primaryBalance properties or from
getUniqueChains(balances) such as a concatenation of chain ids or
primaryBalance.id/address) so each child has a deterministic key across renders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72ebc5e5-5faf-46c8-92aa-f82589d55768
⛔ Files ignored due to path filters (1)
src/components/core/sections/ExpandableSection/__snapshots__/ExpandableSection.snapshot.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.stories.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.style.tssrc/components/core/sections/ExpandableSection/ExpandableSection.tsxsrc/components/core/sections/ExpandableSection/fixtures.tssrc/theme/typography.ts
| ].map((balances) => { | ||
| const hasMultipleChains = balances.length > 1; | ||
| const primaryBalance = balances[0]!; | ||
| const chainEntities = getUniqueChains(balances); | ||
| return ( | ||
| <StyledContent |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i "ExpandableSection.stories.tsx" | xargs -r rg -n -C3 '\.map\(\(balances\)|<StyledContent|key='Repository: jumperexchange/jumper-exchange
Length of output: 490
🏁 Script executed:
sed -n '60,85p' $(fd -i "ExpandableSection.stories.tsx")Repository: jumperexchange/jumper-exchange
Length of output: 884
🏁 Script executed:
# Check if other stories use keys in map
fd -e stories.tsx | xargs rg -l "\.map\(" | head -5 | xargs rg -B2 -A4 "\.map\(\(" | grep -A4 "key=" | head -20Repository: jumperexchange/jumper-exchange
Length of output: 1255
🏁 Script executed:
# Check ExpandableSection component to understand structure
fd -i "ExpandableSection.tsx" | grep -v stories | xargs cat 2>/dev/null | head -80Repository: jumperexchange/jumper-exchange
Length of output: 2079
Add a stable key for mapped children.
Elements returned by this .map() are missing key props, which causes React warnings and unstable reconciliation.
Suggested patch
- ].map((balances) => {
+ ].map((balances) => {
const hasMultipleChains = balances.length > 1;
const primaryBalance = balances[0]!;
const chainEntities = getUniqueChains(balances);
return (
<StyledContent
+ key={`${primaryBalance.token.symbol}-${balances.length}`}
hideCursor={false}
direction="row"📝 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.
| ].map((balances) => { | |
| const hasMultipleChains = balances.length > 1; | |
| const primaryBalance = balances[0]!; | |
| const chainEntities = getUniqueChains(balances); | |
| return ( | |
| <StyledContent | |
| ].map((balances) => { | |
| const hasMultipleChains = balances.length > 1; | |
| const primaryBalance = balances[0]!; | |
| const chainEntities = getUniqueChains(balances); | |
| return ( | |
| <StyledContent | |
| key={`${primaryBalance.token.symbol}-${balances.length}`} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx`
around lines 65 - 70, The mapped elements returned from the array .map (the JSX
root StyledContent in the map) lack a stable key which causes React warnings;
update the mapping callback to supply a unique, stable key prop on StyledContent
(e.g., derive it from primaryBalance properties or from
getUniqueChains(balances) such as a concatenation of chain ids or
primaryBalance.id/address) so each child has a deterministic key across renders.
Playwright test resultsDetails
Flaky testschromium › portfolioPage.spec.ts › Portfolio page › Verify elements and filters on portfolio page › verify value filter is cleared when clicking clear filter button (Qase ID: 57) Skipped testschromium › themeManipulation.spec.ts › Switch between dark and light theme and check the background color › Partner theme should appear in theme menu and apply background color (Qase ID: 49) |
946bd3e to
be07f91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx (1)
77-101: Consider deduplicating sharedTokenAmountprops.Both branches repeat the same visual props/sx; extracting shared props would reduce drift risk in future edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx` around lines 77 - 101, The TokenAmount usage is duplicated across the hasMultipleChains branches; extract the common visual props (amountUSDVariant, amountVariant, gap, sx: {textAlign:'right', marginLeft:'auto', minWidth:0}) into a sharedProps object and spread it into a single TokenAmount component, then conditionally pass the differing prop (balances when hasMultipleChains is true, otherwise balance={primaryBalance}) so TokenAmount, hasMultipleChains, balances, primaryBalance and defaultConfig are the referenced symbols to locate and update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx`:
- Around line 46-49: The renderBalanceGroup function assumes balances is
non-empty by using balances[0]! (assigned to primaryBalance) which can throw if
fixtures change; add a defensive guard at the start of renderBalanceGroup (e.g.,
if balances.length === 0) and return an appropriate fallback (null, empty
fragment, or placeholder) before computing hasMultipleChains or calling
getUniqueChains; update any callers/tests to accept the fallback if needed so
primaryBalance and getUniqueChains are only invoked for non-empty BalanceGroup
inputs.
---
Nitpick comments:
In
`@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx`:
- Around line 77-101: The TokenAmount usage is duplicated across the
hasMultipleChains branches; extract the common visual props (amountUSDVariant,
amountVariant, gap, sx: {textAlign:'right', marginLeft:'auto', minWidth:0}) into
a sharedProps object and spread it into a single TokenAmount component, then
conditionally pass the differing prop (balances when hasMultipleChains is true,
otherwise balance={primaryBalance}) so TokenAmount, hasMultipleChains, balances,
primaryBalance and defaultConfig are the referenced symbols to locate and
update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4afce86c-47c5-4924-8023-ebfbb28d52f0
⛔ Files ignored due to path filters (11)
src/components/Cards/EarnCard/__snapshots__/EarnCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Cards/HeroEarnCard/__snapshots__/HeroEarnCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Cards/ProtocolCard/__snapshots__/ProtocolCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Link/__snapshots__/ExternalLink.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/WalletBalanceCard/__snapshots__/WalletBalanceCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/cards/MultiViewCard/__snapshots__/MultiViewCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/cards/ProcessingTransactionCard/__snapshots__/ProcessingTransactionCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/AvatarStack/__snapshots__/AvatarStack.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/charts/LineChart/__snapshots__/LineChart.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/form/Select/__snapshots__/Select.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/sections/ExpandableSection/__snapshots__/ExpandableSection.snapshot.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.stories.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.style.tssrc/components/core/sections/ExpandableSection/ExpandableSection.tsxsrc/components/core/sections/ExpandableSection/fixtures.tssrc/theme/typography.ts
✅ Files skipped from review due to trivial changes (2)
- src/theme/typography.ts
- src/components/core/sections/ExpandableSection/fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsx
- src/components/core/sections/ExpandableSection/ExpandableSection.style.ts
- src/components/core/sections/ExpandableSection/ExpandableSection.tsx
| const renderBalanceGroup = (balances: BalanceGroup) => { | ||
| const hasMultipleChains = balances.length > 1; | ||
| const primaryBalance = balances[0]!; | ||
| const chainEntities = getUniqueChains(balances); |
There was a problem hiding this comment.
Guard against empty balance groups before balances[0]!.
Line 48 assumes non-empty input and can throw if fixtures/data change. A small runtime guard makes the story resilient.
🛡️ Suggested hardening
const renderBalanceGroup = (balances: BalanceGroup) => {
+ if (balances.length === 0) {
+ return null;
+ }
const hasMultipleChains = balances.length > 1;
const primaryBalance = balances[0]!;
const chainEntities = getUniqueChains(balances);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx`
around lines 46 - 49, The renderBalanceGroup function assumes balances is
non-empty by using balances[0]! (assigned to primaryBalance) which can throw if
fixtures change; add a defensive guard at the start of renderBalanceGroup (e.g.,
if balances.length === 0) and return an appropriate fallback (null, empty
fragment, or placeholder) before computing hasMultipleChains or calling
getUniqueChains; update any callers/tests to accept the fallback if needed so
primaryBalance and getUniqueChains are only invoked for non-empty BalanceGroup
inputs.
🤖 Partneros QA ReviewSummary: Adds a generic, typed Risk Areas:
Missing Coverage:
Playwright Tests: test/partneros-pr-2809 |
be07f91 to
f518f15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/core/sections/ExpandableSection/ExpandableSection.tsx (1)
18-45: MakeisExpandeda controlled prop or clarify it as initial-value only.Currently,
isExpandedis renamed toinitialIsExpandedon destructure (line 34) and used only once to initialize state (line 44). There's nouseEffectto sync prop changes, so updates toisExpandedafter mount are silently ignored. Either add auseEffectto make it controlled, addonExpandedcallback to let parents listen to state changes, or rename the prop todefaultIsExpandedto clarify it's initial-value only.Current implementation (lines 34, 44)
isExpanded: initialIsExpanded = false, ... const [isExpanded, setIsExpanded] = useState<boolean>(initialIsExpanded);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/sections/ExpandableSection/ExpandableSection.tsx` around lines 18 - 45, The prop named isExpanded is only used to initialize state (initialIsExpanded) and is not synced, so make ExpandableSection properly controlled by: accept an optional isExpanded prop and optional onExpanded callback in ExpandableSectionProps, replace the current destructure (isExpanded: initialIsExpanded) with isExpanded (allow undefined), keep internal state via useState but add a useEffect that updates setIsExpanded(isExpanded) when the incoming isExpanded prop is not undefined, and call onExpanded(newValue) whenever setIsExpanded is invoked (e.g., in your toggle handler) so parents can control or observe expansion; alternatively, if you prefer uncontrolled behavior, rename the prop to defaultIsExpanded and remove syncing/notifications — but implement one of these two approaches consistently for the ExpandableSection component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/core/sections/ExpandableSection/ExpandableSection.tsx`:
- Around line 82-90: The click handler is currently attached whenever
onItemClick exists regardless of isDetailsItemClickable; update the onClick prop
on the row (the element rendering ExpandableSection items) to only set a handler
when both onItemClick and isDetailsItemClickable are true (e.g.,
onClick={onItemClick && isDetailsItemClickable ? () => onItemClick(item) :
undefined}); also ensure the useDefaultCursor prop remains consistent with
isDetailsItemClickable so the cursor state matches clickability.
- Around line 81-93: The interactive row (StyledContent) currently only handles
mouse clicks via onItemClick; add keyboard semantics by setting role="button"
and tabIndex={onItemClick ? 0 : undefined}, and implement an onKeyDown handler
on StyledContent that calls onItemClick(item) when Enter or Space is pressed
(and prevents default for Space to avoid scrolling); keep the existing onClick
and preserve useDefaultCursor/isDetailsItemClickable behavior and renderItem
output so accessibility is added without changing visuals.
---
Nitpick comments:
In `@src/components/core/sections/ExpandableSection/ExpandableSection.tsx`:
- Around line 18-45: The prop named isExpanded is only used to initialize state
(initialIsExpanded) and is not synced, so make ExpandableSection properly
controlled by: accept an optional isExpanded prop and optional onExpanded
callback in ExpandableSectionProps, replace the current destructure (isExpanded:
initialIsExpanded) with isExpanded (allow undefined), keep internal state via
useState but add a useEffect that updates setIsExpanded(isExpanded) when the
incoming isExpanded prop is not undefined, and call onExpanded(newValue)
whenever setIsExpanded is invoked (e.g., in your toggle handler) so parents can
control or observe expansion; alternatively, if you prefer uncontrolled
behavior, rename the prop to defaultIsExpanded and remove syncing/notifications
— but implement one of these two approaches consistently for the
ExpandableSection component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e38d969e-ef2e-4a16-b4ca-11e9e8c9691f
⛔ Files ignored due to path filters (11)
src/components/Cards/EarnCard/__snapshots__/EarnCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Cards/HeroEarnCard/__snapshots__/HeroEarnCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Cards/ProtocolCard/__snapshots__/ProtocolCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/Link/__snapshots__/ExternalLink.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/WalletBalanceCard/__snapshots__/WalletBalanceCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/cards/MultiViewCard/__snapshots__/MultiViewCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/composite/cards/ProcessingTransactionCard/__snapshots__/ProcessingTransactionCard.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/AvatarStack/__snapshots__/AvatarStack.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/charts/LineChart/__snapshots__/LineChart.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/form/Select/__snapshots__/Select.snapshot.spec.tsx.snapis excluded by!**/*.snapsrc/components/core/sections/ExpandableSection/__snapshots__/ExpandableSection.snapshot.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.stories.tsxsrc/components/core/sections/ExpandableSection/ExpandableSection.style.tssrc/components/core/sections/ExpandableSection/ExpandableSection.tsxsrc/components/core/sections/ExpandableSection/fixtures.tssrc/theme/typography.ts
✅ Files skipped from review due to trivial changes (3)
- src/components/core/sections/ExpandableSection/fixtures.ts
- src/theme/typography.ts
- src/components/core/sections/ExpandableSection/ExpandableSection.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/core/sections/ExpandableSection/ExpandableSection.snapshot.spec.tsx
| <StyledContent | ||
| useDefaultCursor={!isDetailsItemClickable} | ||
| direction="row" | ||
| spacing={2} | ||
| useFlexGap | ||
| justifyContent="space-between" | ||
| alignItems="center" | ||
| key={`ExpandableSectionItem-${index}`} | ||
| sx={detailsItemSx} | ||
| onClick={onItemClick ? () => onItemClick(item) : undefined} | ||
| > | ||
| {renderItem(item)} | ||
| </StyledContent> |
There was a problem hiding this comment.
Clickable rows are mouse-only; add keyboard interaction semantics.
StyledContent is interactive when onItemClick is set, but there is no keyboard activation path (Enter/Space), role, or tab stop.
Suggested fix
+ {items.map((item, index) => {
+ const isItemClickable = Boolean(onItemClick) && isDetailsItemClickable;
+ return (
<StyledContent
useDefaultCursor={!isDetailsItemClickable}
direction="row"
spacing={2}
useFlexGap
justifyContent="space-between"
alignItems="center"
key={`ExpandableSectionItem-${index}`}
sx={detailsItemSx}
- onClick={onItemClick ? () => onItemClick(item) : undefined}
+ onClick={isItemClickable ? () => onItemClick?.(item) : undefined}
+ role={isItemClickable ? 'button' : undefined}
+ tabIndex={isItemClickable ? 0 : undefined}
+ onKeyDown={
+ isItemClickable
+ ? (event) => {
+ if (event.key === 'Enter' || event.key === ' ') {
+ event.preventDefault();
+ onItemClick?.(item);
+ }
+ }
+ : undefined
+ }
>
{renderItem(item)}
</StyledContent>
+ );
+ })}
- ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/core/sections/ExpandableSection/ExpandableSection.tsx` around
lines 81 - 93, The interactive row (StyledContent) currently only handles mouse
clicks via onItemClick; add keyboard semantics by setting role="button" and
tabIndex={onItemClick ? 0 : undefined}, and implement an onKeyDown handler on
StyledContent that calls onItemClick(item) when Enter or Space is pressed (and
prevents default for Space to avoid scrolling); keep the existing onClick and
preserve useDefaultCursor/isDetailsItemClickable behavior and renderItem output
so accessibility is added without changing visuals.
| useDefaultCursor={!isDetailsItemClickable} | ||
| direction="row" | ||
| spacing={2} | ||
| useFlexGap | ||
| justifyContent="space-between" | ||
| alignItems="center" | ||
| key={`ExpandableSectionItem-${index}`} | ||
| sx={detailsItemSx} | ||
| onClick={onItemClick ? () => onItemClick(item) : undefined} |
There was a problem hiding this comment.
isDetailsItemClickable is not enforced in the click handler.
Line 90 still attaches onClick whenever onItemClick exists, so rows remain clickable even when isDetailsItemClickable={false}.
Suggested fix
- onClick={onItemClick ? () => onItemClick(item) : undefined}
+ onClick={
+ onItemClick && isDetailsItemClickable
+ ? () => onItemClick(item)
+ : undefined
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/core/sections/ExpandableSection/ExpandableSection.tsx` around
lines 82 - 90, The click handler is currently attached whenever onItemClick
exists regardless of isDetailsItemClickable; update the onClick prop on the row
(the element rendering ExpandableSection items) to only set a handler when both
onItemClick and isDetailsItemClickable are true (e.g., onClick={onItemClick &&
isDetailsItemClickable ? () => onItemClick(item) : undefined}); also ensure the
useDefaultCursor prop remains consistent with isDetailsItemClickable so the
cursor state matches clickability.
Which Jira task belongs to this PR?
Closes https://linear.app/lifi-linear/issue/JUM-801/portfolio-page-create-generic-expandable-assets-card-component
Testing steps
Expandable SectionNote
The enclosing card is not added in this component, so we can easily re-use it in different views
Why did I implement it this way?
Checklist before requesting a review
Summary by CodeRabbit
New Features
Tests
Style