refactor: validators query cache and search handling#520
Open
petar-omni wants to merge 4 commits into
Open
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:
📝 WalkthroughWalkthroughConverts validator loading to an infinite-query offset paginator with per-address cache and search, migrates validator address lists to ValidatorDto across domain types/hooks, refactors provider-details to accept ValidatorDto, adds debounced validator search, and wires load-more pagination through SelectValidator UI and page contexts. ChangesValidator Infinite Query & Data Shape Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
petar-omni
changed the title
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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
🧹 Nitpick comments (2)
packages/widget/src/domain/types/action.ts (1)
109-120: Confirm action input token resolution no longer relies on yield metadata tokens
getYieldMetadataTokenshas no definition/usage in this repo, andgetActionInputToken(packages/widget/src/domain/types/action.ts, ~lines 109-120) searches onlyyieldDto.token/yieldDto.tokens, with fallback toyieldDto.token ?? yieldDto.tokens?.[0].yieldDto.__fallback__.metadata.rewardTokensexists in packages/widget/src/domain/types/yields.ts, but it’s used bygetYieldRewardTokens, not bygetActionInputToken; so metadata-only reward tokens won’t surface here.- If
actionDto.rawArguments?.inputTokenrefers to a token not present inyieldDto.token/yieldDto.tokens, the lookup will silently fall back—consider handling/reporting this mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widget/src/domain/types/action.ts` around lines 109 - 120, The current getActionInputToken logic only searches yieldDto.token and yieldDto.tokens (and then silently falls back to yieldDto.token ?? yieldDto.tokens?.[0]), so actionDto.rawArguments?.inputToken can be missed if it exists only in yield metadata; update getActionInputToken to also consult yieldDto.__fallback__?.metadata?.rewardTokens (or a shared getYieldMetadataTokens helper) when resolving the needle and, if still not found, surface the mismatch by returning an explicit error/undefined with a logged warning (or throw) rather than silently falling back; reference getActionInputToken, yieldDto, actionDto.rawArguments?.inputToken and yieldDto.__fallback__.metadata.rewardTokens when making the change.packages/widget/src/components/molecules/select-validator/index.tsx (1)
59-62: ⚡ Quick winGate
onLoadMoreto real next-page availability.Line 61 currently triggers pagination even when "View all" only reveals already-loaded validators (
hasMoreis false). Guarding this avoids unnecessary fetch attempts.♻️ Proposed change
const _onViewMoreClick = () => { onViewMoreClick?.(); - onLoadMore?.(); + if (hasMore) { + onLoadMore?.(); + } setViewMore(true); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widget/src/components/molecules/select-validator/index.tsx` around lines 59 - 62, The _onViewMoreClick handler currently always calls onLoadMore even when no next page exists; update _onViewMoreClick (which calls onViewMoreClick, onLoadMore, and setViewMore) to check the hasMore flag and only invoke onLoadMore() when hasMore is true, leaving onViewMoreClick() and setViewMore(true) to run as before so the UI still reveals already-loaded validators without triggering unnecessary fetches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widget/src/hooks/api/use-yield-validators.ts`:
- Around line 190-194: The merged result currently concatenates namePage.items
and addressPage.items which can produce duplicates when a validator matches both
filters; update the merge in the return of the function in
use-yield-validators.ts to deduplicate by validator address (e.g., use a Set
keyed on validator.address or validator.id) so items contains only unique
validators and set total to the unique count (instead of
Math.max(namePage.total, addressPage.total)); ensure you preserve
order/stability as needed when de-duplicating.
- Around line 273-279: The current Promise.all over remainingOffsets (computed
from firstPage.total / PAGE_SIZE) can spawn unbounded concurrent calls to
fetchPage; change this to a controlled concurrency strategy: either process
remainingOffsets in small chunks (e.g., batch size N) or use a concurrency
limiter like p-limit to call fetchPage({ preferred: true, offset }) with at most
N in flight, accumulate results into remainingPages, and preserve order; update
the code around remainingOffsets, remainingPages and fetchPage to use that
throttling approach and make N configurable (or set a sensible default) to avoid
request storms.
In `@packages/widget/src/hooks/use-provider-details.ts`:
- Around line 77-114: The selected-provider branch is incorrectly summing the
provider reward twice and keeps using the outer yieldDto for rewardType; update
the Maybe.chain/map so the selected provider's rewardRate is used once and its
rewardType is derived from that selected yield. Specifically: in the
Maybe.fromRecord(...) chain that uses yields and List.find, change the .map that
currently does v.rewardRate.total + v.rewardRate.total to return the single
selected yield's rewardRate (e.g., v.rewardRate.total) and map to an object
carrying both rewardRate and rewardType computed from the selected yield (use
getYieldRewardType on the found list item), while leaving the .orDefault
fallback as-is (validator.rewardRate?.total and getYieldRewardType(yieldDto)) so
consumer code (rewardRate, rewardType) receives the selected provider values
when present.
---
Nitpick comments:
In `@packages/widget/src/components/molecules/select-validator/index.tsx`:
- Around line 59-62: The _onViewMoreClick handler currently always calls
onLoadMore even when no next page exists; update _onViewMoreClick (which calls
onViewMoreClick, onLoadMore, and setViewMore) to check the hasMore flag and only
invoke onLoadMore() when hasMore is true, leaving onViewMoreClick() and
setViewMore(true) to run as before so the UI still reveals already-loaded
validators without triggering unnecessary fetches.
In `@packages/widget/src/domain/types/action.ts`:
- Around line 109-120: The current getActionInputToken logic only searches
yieldDto.token and yieldDto.tokens (and then silently falls back to
yieldDto.token ?? yieldDto.tokens?.[0]), so actionDto.rawArguments?.inputToken
can be missed if it exists only in yield metadata; update getActionInputToken to
also consult yieldDto.__fallback__?.metadata?.rewardTokens (or a shared
getYieldMetadataTokens helper) when resolving the needle and, if still not
found, surface the mismatch by returning an explicit error/undefined with a
logged warning (or throw) rather than silently falling back; reference
getActionInputToken, yieldDto, actionDto.rawArguments?.inputToken and
yieldDto.__fallback__.metadata.rewardTokens when making the change.
🪄 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: 82698f79-d064-4f31-b243-407ab38e5865
📒 Files selected for processing (41)
packages/widget/src/components/molecules/select-validator/index.tsxpackages/widget/src/components/molecules/select-validator/styles.css.tspackages/widget/src/domain/types/action.tspackages/widget/src/domain/types/positions.tspackages/widget/src/domain/types/yields.tspackages/widget/src/hooks/api/use-activity-actions.tspackages/widget/src/hooks/api/use-yield-validators.tspackages/widget/src/hooks/use-debounced-value.tspackages/widget/src/hooks/use-estimated-rewards.tspackages/widget/src/hooks/use-positions-data.tspackages/widget/src/hooks/use-provider-details.tspackages/widget/src/hooks/use-summary.tsxpackages/widget/src/pages-dashboard/activity/activity.page.tsxpackages/widget/src/pages-dashboard/overview/earn-page/utila-select-validator-section.tsxpackages/widget/src/pages-dashboard/position-details/components/position-details-actions.tsxpackages/widget/src/pages/complete/hooks/use-activity-complete.hook.tspackages/widget/src/pages/complete/pages/pending-complete.page.tsxpackages/widget/src/pages/complete/pages/stake-complete.page.tsxpackages/widget/src/pages/complete/pages/unstake-complete.page.tsxpackages/widget/src/pages/details/activity-page/hooks/use-action-list-item.tspackages/widget/src/pages/details/activity-page/state/activity-page.context.tsxpackages/widget/src/pages/details/activity-page/types.tspackages/widget/src/pages/details/earn-page/components/select-validator-section/index.tsxpackages/widget/src/pages/details/earn-page/components/select-validator-section/use-select-validator.tspackages/widget/src/pages/details/earn-page/state/earn-page-context.tsxpackages/widget/src/pages/details/earn-page/state/types.tspackages/widget/src/pages/details/earn-page/state/use-stake-enter-request-dto.tspackages/widget/src/pages/details/positions-page/hooks/use-position-list-item.tspackages/widget/src/pages/details/positions-page/hooks/use-positions.tspackages/widget/src/pages/position-details/hooks/use-position-details.tspackages/widget/src/pages/position-details/position-details.page.tsxpackages/widget/src/pages/steps/pages/activity-steps.page.tsxpackages/widget/src/pages/steps/pages/pending-steps.page.tsxpackages/widget/src/pages/steps/pages/stake-steps.page.tsxpackages/widget/src/pages/steps/pages/unstake-steps.page.tsxpackages/widget/src/providers/activity-provider/index.tsxpackages/widget/src/translation/English/translations.jsonpackages/widget/src/translation/French/translations.jsonpackages/widget/tests/components/select-validator-section.test.tsxpackages/widget/tests/hooks/use-debounced-value.test.tsxpackages/widget/tests/hooks/validator-loading.test.tsx
💤 Files with no reviewable changes (1)
- packages/widget/src/domain/types/yields.ts
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/widget/src/hooks/use-provider-details.ts (1)
86-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
rewardTypeshould be derived from the selected provider yield, notyieldDto.The doubling bug is fixed, but when a selected provider yield is found (line 83), the
rewardTypeshould come from that selected yield rather than the outeryieldDto. Currently both line 90 and line 106 usegetYieldRewardType(yieldDto), which returns incorrect reward type data for provider-backed validator details.🐛 Proposed fix
.chain(({ selectedProviderYieldId }) => yields.chain((list) => List.find((v) => v.id === selectedProviderYieldId, [...list]) ) ) - .map((v) => v.rewardRate.total) - .map<{ rewardRate: number | undefined; rewardType: RewardTypes }>( - (res) => ({ - rewardRate: res, - rewardType: getYieldRewardType(yieldDto), + .map<{ rewardRate: number | undefined; rewardType: RewardTypes }>( + (selectedYield) => ({ + rewardRate: selectedYield.rewardRate.total, + rewardType: getYieldRewardType(selectedYield), }) ) .orDefault({ rewardRate: validator.rewardRate?.total, rewardType: getYieldRewardType(yieldDto), }); return { logo: validator.logoURI, name: validator.name ?? validator.address, rewardRateFormatted: getRewardRateFormatted({ rewardRate, rewardType, }), rewardRate, - rewardType: getYieldRewardType(yieldDto), + rewardType, address: validator.address,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widget/src/hooks/use-provider-details.ts` around lines 86 - 106, The rewardType is incorrectly being derived from yieldDto everywhere; change it to derive from the selected provider yield when present. Replace usages of getYieldRewardType(yieldDto) in the map that builds {rewardRate, rewardType}, in the orDefault block, and in the returned object so they call getYieldRewardType(selectedProviderYield) (or compute a single const rewardType = getYieldRewardType(selectedProviderYield ?? yieldDto) and use that) instead of getYieldRewardType(yieldDto), ensuring rewardType matches the selected provider yield.
🧹 Nitpick comments (1)
packages/widget/src/hooks/api/use-yield-validators.ts (1)
299-301: ⚖️ Poor tradeoffConsider limiting concurrent fetches for remaining preferred pages.
Promise.alloverremainingOffsetscan fan out many concurrent requests when there are many preferred validators (e.g., 1000 validators = 10 concurrent requests). This could cause rate limiting or cascade latency under high totals.Consider using a concurrency limiter (e.g.,
p-limit) or sequential fetching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widget/src/hooks/api/use-yield-validators.ts` around lines 299 - 301, The current use of Promise.all over remainingOffsets in use-yield-validators.ts (the remainingPages = await Promise.all(remainingOffsets.map(offset => fetchPage({ preferred: true, offset })))) can spawn many concurrent fetchPage calls and cause rate limits; change to a concurrency-limited approach (e.g., use p-limit or implement a small batching/sequential loop) to cap concurrent fetchPage executions (suggest 3–5 concurrent requests), keeping the same fetchPage({ preferred: true, offset }) call signature and collecting results into remainingPages in the same order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widget/src/hooks/api/use-yield-validators.ts`:
- Around line 207-216: The returned total should reflect the server-side totals,
not the deduplicated items length; change the return to set total to
(namePage.total ?? 0) + (addressPage.total ?? 0) (instead of items.length) so
getRawNextOffset can correctly determine if more pages exist; update the return
in the function that calls deduplicateValidatorsByAddress (the block using
namePage, addressPage, items) so pagination logic relying on getRawNextOffset
works properly.
---
Duplicate comments:
In `@packages/widget/src/hooks/use-provider-details.ts`:
- Around line 86-106: The rewardType is incorrectly being derived from yieldDto
everywhere; change it to derive from the selected provider yield when present.
Replace usages of getYieldRewardType(yieldDto) in the map that builds
{rewardRate, rewardType}, in the orDefault block, and in the returned object so
they call getYieldRewardType(selectedProviderYield) (or compute a single const
rewardType = getYieldRewardType(selectedProviderYield ?? yieldDto) and use that)
instead of getYieldRewardType(yieldDto), ensuring rewardType matches the
selected provider yield.
---
Nitpick comments:
In `@packages/widget/src/hooks/api/use-yield-validators.ts`:
- Around line 299-301: The current use of Promise.all over remainingOffsets in
use-yield-validators.ts (the remainingPages = await
Promise.all(remainingOffsets.map(offset => fetchPage({ preferred: true, offset
})))) can spawn many concurrent fetchPage calls and cause rate limits; change to
a concurrency-limited approach (e.g., use p-limit or implement a small
batching/sequential loop) to cap concurrent fetchPage executions (suggest 3–5
concurrent requests), keeping the same fetchPage({ preferred: true, offset })
call signature and collecting results into remainingPages in the same order.
🪄 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: 01dcaf15-88df-41ed-8a3e-aef4f0f317f0
📒 Files selected for processing (3)
packages/widget/src/hooks/api/use-yield-validators.tspackages/widget/src/hooks/use-provider-details.tspackages/widget/tests/hooks/validator-loading.test.tsx
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)
packages/widget/src/hooks/api/use-yield-validators.ts (1)
106-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid caching
{ address }fallback placeholders forever ingetYieldValidatorsByAddresses(lines 106-110)
{ address }still satisfiesValidatorDtobecausepackages/widget/src/generated/api/yield.tsrequires onlyaddress; the rest of the fields are optional.- The issue is behavioral: on “not found” or fetch errors the code returns
{ address }, and React Query caches it withstaleTime: Number.POSITIVE_INFINITY, so later lookups for the sameyieldId/addressmay keep returning an incomplete placeholder indefinitely.- Consider not caching the fallback (filter it out), using a finite
staleTimefor fallback, or distinguishing “not found” from “error”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widget/src/hooks/api/use-yield-validators.ts` around lines 106 - 110, getYieldValidatorsByAddresses currently returns a placeholder object { address } on not-found/error which gets cached forever; change the function (getYieldValidatorsByAddresses) so that when a validator is missing or a fetch fails you return [address, undefined] (or null) instead of [address, { address }], and then filter out these undefined/null entries before building the final result that React Query will cache (or alternatively propagate the error so react-query treats it as an error); this prevents caching incomplete placeholder ValidatorDto objects indefinitely and ensures callers handle missing validators explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/widget/src/hooks/api/use-yield-validators.ts`:
- Around line 106-110: getYieldValidatorsByAddresses currently returns a
placeholder object { address } on not-found/error which gets cached forever;
change the function (getYieldValidatorsByAddresses) so that when a validator is
missing or a fetch fails you return [address, undefined] (or null) instead of
[address, { address }], and then filter out these undefined/null entries before
building the final result that React Query will cache (or alternatively
propagate the error so react-query treats it as an error); this prevents caching
incomplete placeholder ValidatorDto objects indefinitely and ensures callers
handle missing validators explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 745e8bf7-eda3-4ff3-8571-756f5af45427
📒 Files selected for processing (3)
packages/widget/src/hooks/api/use-yield-validators.tspackages/widget/src/hooks/use-provider-details.tspackages/widget/tests/hooks/validator-loading.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/widget/tests/hooks/validator-loading.test.tsx
- packages/widget/src/hooks/use-provider-details.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widget/src/main.tsx`:
- Around line 20-38: The stub external provider (bittensorExternalProviders with
provider.signMessage, switchChain, sendTransaction returning a fake tx hash)
must be gated by an explicit environment flag; update the code that creates
bittensorExternalProviders so it only assigns the fake provider when a flag like
USE_MOCK_EXTERNAL_PROVIDER (or equivalent config) is truthy, otherwise leave
provider undefined or load the real provider; ensure the fake sendTransaction no
longer runs in production by moving the stub implementation behind that flag
check and return no fake tx hash when the flag is not set.
In `@packages/widget/src/translation/English/translations.json`:
- Around line 261-262: Add matching French translations for the new English keys
to restore localization parity: update the French translations.json to include
entries for "pending_approvals" and "pending_approvals_desc" with appropriate
French copy (e.g., localized strings conveying "Pending approvals" and "Please
open your wallet to review and securely sign the transaction"), ensuring key
names exactly match the English keys so the i18n lookup finds them.
🪄 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: ea8398a6-34a4-4d51-8e21-36cc68fce610
📒 Files selected for processing (8)
packages/widget/src/main.tsxpackages/widget/src/pages/steps/pages/common.page.tsxpackages/widget/src/pages/steps/pages/styles.css.tspackages/widget/src/styles/theme/variant-overrides/utila.tspackages/widget/src/styles/tokens/colors/contract.tspackages/widget/src/styles/tokens/colors/values.tspackages/widget/src/translation/English/translations.jsonpackages/widget/src/translation/French/translations.json
✅ Files skipped from review due to trivial changes (3)
- packages/widget/src/pages/steps/pages/styles.css.ts
- packages/widget/src/styles/tokens/colors/values.ts
- packages/widget/src/translation/French/translations.json
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
1 participant
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
New Features
Bug Fixes / Improvements
Tests