-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce excess CL position queries #3291
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
WalkthroughThe recent updates focus on improving the handling of liquidity positions across various components and APIs. Key changes include the restructuring of how position data is queried and calculated, shifting data sources from Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedBiome
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
packages/web/components/cards/my-position/index.tsx (1)
Line range hint
72-84
: Add keyboard event handlers for accessibility.+ onKeyUp={handleKeyUp}
Ensure that actions triggered by mouse events also have corresponding keyboard events to support accessibility.
Also applies to: 85-94
packages/server/src/queries/complex/concentrated-liquidity/index.ts (1)
Line range hint
60-69
: Replace.map().flat()
with.flatMap()
for better performance.- .map(...).flat(); + .flatMap(...);This change simplifies the code and may improve performance by reducing the complexity of operations on arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
packages/web/components/complex/my-positions-section.tsx (1)
Line range hint
62-62
: Remove redundantBoolean
call for simplicity.- showLinkToPool={!Boolean(forPoolId)} + showLinkToPool={!forPoolId}packages/web/components/cards/my-position/index.tsx (1)
Line range hint
72-84
: Add keyboard event handlers for accessibility.+ onKeyUp={handleKeyUp}
Ensure that actions triggered by mouse events are also accessible via keyboard.
Also applies to: 85-94
packages/web/components/complex/portfolio-page.tsx (1)
Line range hint
54-54
: Use optional chaining for safer and cleaner code.- wallet?.address ?? "" + wallet?.addressApply this change wherever optional chaining can replace the current logic.
Also applies to: 83-83, 156-156, 198-198
packages/web/components/cards/my-position/expanded.tsx (3)
Line range hint
151-151
: Add keyboard event handlers to complement mouse events for accessibility.+ onKeyUp={handleKeyUp} + onKeyDown={handleKeyDown} + onKeyPress={handleKeyPress}Ensure that actions triggered by mouse clicks are also accessible via keyboard events to support accessibility best practices.
Line range hint
342-342
: Remove redundant Boolean calls.- disabled={Boolean(account?.txTypeInProgress) || !Boolean(account)} + disabled={account?.txTypeInProgress || !account}The
Boolean
function is redundant here as the values are automatically coerced to boolean in JavaScript. Simplifying this can improve code readability.Also applies to: 354-354, 371-371
Line range hint
307-312
: Explicitly set the button type to prevent unintended form submissions.- <button onClick={...}> + <button type="button" onClick={...}>Setting the button type to 'button' prevents it from submitting a form unintentionally, which is the default behavior when placed inside a form element.
This reverts commit edd9e07.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
packages/web/components/cards/my-position/index.tsx (1)
Line range hint
72-84
: Add keyboard accessibility to clickable elements.+ onKeyUp={(e) => { if (e.key === 'Enter') setCollapsed(false); }} + onKeyUp={(e) => { if (e.key === 'Enter') setCollapsed(true); }}According to accessibility best practices, interactive elements triggered by mouse clicks should also be operable by keyboard events. This is crucial for users who rely on keyboard navigation.
Also applies to: 85-94
const { t } = useTranslation(); | ||
const [collapsed, setCollapsed] = useState(true); | ||
const featureFlags = useFeatureFlags(); | ||
|
||
const { data: positionPerformance } = | ||
api.local.concentratedLiquidity.getPositionHistoricalPerformance.useQuery( | ||
{ | ||
positionId: id, | ||
position: position.position, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the API calls are updated to the new edge API.
- api.local.concentratedLiquidity.getPositionHistoricalPerformance.useQuery
+ api.edge.concentratedLiquidity.getPositionHistoricalPerformance.useQuery
- api.local.concentratedLiquidity.getPositionDetails.useQuery
+ api.edge.concentratedLiquidity.getPositionDetails.useQuery
The PR summary indicates a shift from local to edge API for performance optimization. These lines should reflect that change.
Also applies to: 55-55
Committable suggestion was skipped due to low confidence.
What is the purpose of the change:
Linear Task
Linear Task URL
Brief Changelog
Testing and Verifying