fix: history intersection loader issue #1598
Conversation
WalkthroughThe pull request includes modifications to several files within the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/extension-polkagate/src/util/api/getTransfers.ts (1)
13-15: LGTM: Improved object formatting.The reformatting of
nullObjectenhances readability. Consider using consistent formatting for all properties (e.g., all on separate lines or all inline) for even better readability.packages/extension-polkagate/src/popup/history/index.tsx (3)
47-47: LGTM! Consider using a constant for theidvalue.The addition of
id='scrollArea'to theGridcomponent improves DOM targeting, which is beneficial for scroll-related functionality. This change aligns well with the PR objectives.For better maintainability and consistency, consider defining the
idvalue as a constant at the top of the file:const SCROLL_AREA_ID = 'scrollArea'; // Then use it in the JSX: <Grid container id={SCROLL_AREA_ID} ...>This approach makes it easier to reuse the
idvalue if needed elsewhere in the component or in related files.
74-74: LGTM! Consider adding a comment for clarity.The addition of the
divwithid='observerObj'andheight: '1px'is a good solution for creating an invisible but detectable element for the intersection observer. This addresses the PR objective of making the observer object visible.For improved code clarity, consider adding a brief comment explaining the purpose of this
div:{/* Invisible element for intersection observer to detect end of scroll area */} <div id='observerObj' style={{ height: '1px' }} />This comment will help other developers understand the purpose of this seemingly empty
divwithout needing to refer to external documentation.
Line range hint
47-74: Overall, these changes effectively address the history intersection loader issue.The modifications to the
TransactionHistorycomponent, including the addition ofid='scrollArea'to theGridand the newdivfor the intersection observer, successfully tackle the visibility issues mentioned in the PR objectives. These changes improve the component's structure without altering its core functionality.To further enhance the component:
- Consider extracting the intersection observer logic into a custom hook if it's not already done. This would improve reusability and separation of concerns.
- If the
scrollAreais used in multiple components, consider creating a reusableScrollableGridcomponent that encapsulates this functionality.These suggestions could be addressed in future PRs to continually improve the codebase's architecture.
packages/extension-polkagate/src/popup/history/modal/HistoryModal.tsx (2)
54-54: LGTM! Consider adding a comment for clarity.The addition of the
id='scrollArea'to theGridcomponent is a good change. It likely facilitates the targeting of this element for the intersection observer, addressing the loader issue mentioned in the PR objectives.Consider adding a brief comment explaining the purpose of this
id, which could help future maintainers understand its significance:- <Grid container id='scrollArea' item sx={{ gap: '5px', height: '70%', maxHeight: 650 - 145, overflowY: 'auto' }}> + {/* This Grid is targeted by the intersection observer for infinite scrolling */} + <Grid container id='scrollArea' item sx={{ gap: '5px', height: '70%', maxHeight: 650 - 145, overflowY: 'auto' }}>
96-96: LGTM! Consider adding aria attributes for accessibility.The addition of
style={{ height: '1px' }}to thedivwithid='observerObj'is a good change. It makes the element visible, which is crucial for the intersection observer to function correctly, addressing the issue mentioned in the PR objectives.For better accessibility, consider adding
aria-hidden="true"to thisdivsince it's not meant to be interacted with by users:- <div id='observerObj' style={{ height: '1px' }} /> + <div aria-hidden="true" id='observerObj' style={{ height: '1px' }} />packages/extension-polkagate/src/popup/history/useTransactionHistory.tsx (2)
183-191: Approve changes with a minor suggestion.The updates to the
observerCallbackfunction improve the handling of intersection observer events. The early return when the entry is not intersecting is a good optimization.Consider removing the console log statement on line 186 before merging to production:
- console.log('entry.isIntersecting:', entry.isIntersecting)
197-200: Approve changes with a minor suggestion.The refinement in the logic for disconnecting the observer when there's no more data to load is a good improvement.
Consider removing the console log statement on line 198 before merging to production:
- console.log('No more data to load, disconnecting observer.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/extension-polkagate/src/popup/history/index.tsx (2 hunks)
- packages/extension-polkagate/src/popup/history/modal/HistoryModal.tsx (2 hunks)
- packages/extension-polkagate/src/popup/history/useTransactionHistory.tsx (1 hunks)
- packages/extension-polkagate/src/util/api/getTransfers.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
packages/extension-polkagate/src/util/api/getTransfers.ts (4)
4-4: LGTM: Improved type import.The explicit import of
TransferRequesttype enhances code clarity and type safety.
18-18: LGTM: Improved function signature formatting.The added space before the opening parenthesis in the
getTxTransfersfunction signature enhances readability and maintains consistency with the coding style.
46-46: LGTM: Consistent function signature formatting.The added space before the opening parenthesis in the
postReqfunction signature maintains consistency with thegetTxTransfersfunction and improves overall code readability.
Line range hint
1-48: Summary: Improved code organization and readability.The changes in this file are primarily cosmetic, focusing on improving code organization and readability. While these changes don't directly address the history intersection loader issue mentioned in the PR objectives, they contribute to better maintainability of the codebase. The improved type imports and consistent formatting will make it easier for developers to work with and debug the transaction history functionality.
packages/extension-polkagate/src/popup/history/modal/HistoryModal.tsx (1)
Line range hint
54-96: Great job addressing the history intersection loader issue!The changes made in this file effectively tackle the problems mentioned in the PR objectives:
- Adding
id='scrollArea'to theGridcomponent provides a clear target for the intersection observer.- Setting
style={{ height: '1px' }}on theobserverObjdiv ensures it's visible, allowing the observer to function correctly.These modifications should resolve the loader issue and improve the overall functionality of the infinite scrolling feature in the history modal.
packages/extension-polkagate/src/popup/history/useTransactionHistory.tsx (3)
209-209: Approve the addition of the explanatory comment.The comment clearly explains the purpose of the threshold value, which enhances code readability and maintainability.
213-218: Approve changes addressing PR objectives.These changes directly address the issue mentioned in the PR objectives regarding the observer object's visibility. The null check before observing the target is a good practice to prevent potential errors.
This implementation aligns with the PR's goal of fixing the history intersection loader issue.
220-223: Approve the addition of the cleanup function.The cleanup function ensures that the intersection observer is properly disconnected when the component unmounts or when dependencies change. This is a crucial improvement that prevents potential memory leaks and unnecessary computations.
## [0.19.6](v0.19.5...v0.19.6) (2024-10-23) ### Bug Fixes * history intersection loader issue ([#1598](#1598)) ([8744720](8744720))
there were missing root label, and also the observerObj was not visible due to lack of height!
Summary by CodeRabbit
New Features
TransactionHistorycomponent for improved DOM structure with new identifiers for better targeting and styling.Bug Fixes
Chores