Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a FinalizeCommunityBanner UI, switches community/account creation and various transaction flows to use a web broadcast adapter (runtime branching by login type), inlines signing across multiple flows (removing intermediate sign steps), updates transfer signing to accept asset overrides, and adds i18n entries and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as FinalizeCommunityBanner
participant SDK as SDK Hooks
participant Adapter as Web Broadcast Adapter
participant Blockchain as Blockchain
User->>UI: Submit admin & metadata
UI->>UI: Validate admin & title
rect rgba(100, 150, 200, 0.5)
Note over UI,SDK: Assign admin role via adapter
UI->>SDK: call useSetCommunityRole(..., { adapter })
SDK->>Adapter: prepare & sign set_role operation
Adapter->>Blockchain: broadcast set_role
Blockchain-->>Adapter: success
Adapter-->>SDK: mutation success
SDK-->>UI: role assignment complete
end
rect rgba(150, 100, 200, 0.5)
Note over UI,SDK: Update community metadata via adapter
UI->>SDK: call useUpdateCommunity(..., { adapter })
SDK->>Adapter: prepare & sign update_community operation
Adapter->>Blockchain: broadcast update_community
Blockchain-->>Adapter: success
Adapter-->>SDK: mutation success
SDK-->>UI: metadata update complete
end
UI->>User: Display success feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
apps/web/src/api/mutations/sign-transfer.ts (1)
35-40: Consider documenting theassetoverride field.The optional
assetfield inSignTransferPayloadallows overriding the hook-level asset, but this behavior isn't documented. A brief JSDoc comment would clarify when callers should use this override vs. relying on the hook parameter.📝 Suggested documentation
export interface SignTransferPayload { to: string; amount: string; memo: string; + /** Optional override for the asset type. If not provided, uses the asset passed to useSignTransfer. */ asset?: TransferAsset; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/mutations/sign-transfer.ts` around lines 35 - 40, Add a JSDoc comment to the SignTransferPayload interface describing the optional asset field: explain that asset?: TransferAsset overrides the hook-level/default asset for this specific transfer, when callers should provide it (e.g., sending a different token than the hook-configured asset), and any important format/validation expectations tied to the TransferAsset type; place the comment immediately above the SignTransferPayload declaration so consumers of sign-transfer.ts and the SignTransferPayload type see the behavior clearly.apps/web/src/features/shared/transfer/transfer-step-2.tsx (1)
33-41: Consider handling refetch errors or making it non-blocking.The
refetch()call is awaited, but if it fails (e.g., network error), the entire flow throws and step 3 is never reached—even though the transfer itself succeeded. Since the transfer is the critical operation, consider either:
- Not awaiting refetch (let it run in background)
- Catching refetch errors separately to still advance to step 3
💡 Option 1: Non-blocking refetch
const handleConfirmAndSign = useCallback(async () => { try { await signTransfer({ to, amount, memo }); - await refetch(); + refetch(); // Fire-and-forget; step 3 shows success regardless setStep(3); } catch (e) { error(...formatError(e)); } }, [amount, memo, refetch, setStep, signTransfer, to]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/transfer/transfer-step-2.tsx` around lines 33 - 41, The current handleConfirmAndSign awaits refetch(), which can block progression to setStep(3) if refetch errors; change the logic in handleConfirmAndSign (the async callback using signTransfer, refetch and setStep) so the transfer result drives flow: either call refetch() without awaiting (fire-and-forget) after successful signTransfer, or wrap refetch() in its own try/catch so any refetch failure does not throw out of the main try and you still call setStep(3); ensure references to signTransfer, refetch and setStep remain intact and errors from refetch are logged but do not prevent advancing to step 3.apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx (2)
24-26: Replace hardcoded query key arrays with SDK key sources.Direct arrays like
["community", "..."]should be replaced with a single SDK-derived query key source to avoid drift.💡 Suggested fix pattern
+import { getCommunityQueryOptions } from "@ecency/sdk"; ... +const communityKey = (name: string) => + getCommunityQueryOptions(name, "", true).queryKey; ... - queryClient.setQueryData(["community", "hive-123456"], null); + queryClient.setQueryData(communityKey("hive-123456"), null);As per coding guidelines "Use
QueryKeysfrom@ecency/sdkas the single source of truth for cache key management" and "Do not use hardcoded query key arrays—useQueryKeysfrom@ecency/sdk".Also applies to: 86-87, 96-97, 106-107, 119-120, 130-131, 142-143, 161-162, 195-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx` around lines 24 - 26, Replace hardcoded query key arrays in getCommunityQueryOptions and the other similar query option factories with the SDK QueryKeys source: import QueryKeys from `@ecency/sdk` and use QueryKeys.community(username) (or the appropriate QueryKeys method for each case) instead of literal arrays like ["community", username]; update the queryKey properties across the occurrences referenced (e.g., the getCommunityQueryOptions function and the other query option factories noted in the comment) so all cache keys derive from QueryKeys to keep a single source of truth.
3-6: UserenderWithQueryClientinstead of a custom QueryClientProvider wrapper.This spec manually wraps with
QueryClientProvider; the repo test utility already standardizes this path for React Query components.As per coding guidelines "Use
renderWithQueryClientfromsrc/specs/test-utils.tsxfor testing components that use React Query".Also applies to: 57-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx` around lines 3 - 6, Replace the manual QueryClientProvider wrapper in the spec for FinalizeCommunityBanner with the repository test helper: import and use renderWithQueryClient from the test-utils file instead of creating a new QueryClient and wrapping with QueryClientProvider; update the imports to remove QueryClient/QueryClientProvider and call renderWithQueryClient(<FinalizeCommunityBanner ... />) wherever the wrapper is used (also apply the same replacement at the occurrences around lines 57-63) to standardize React Query test setup.apps/web/src/specs/features/shared/boost.spec.tsx (1)
205-208: Drop the mock call-count assertions from these disabled-state tests.The disabled button is already the user-visible contract here.
not.toHaveBeenCalled()without any attempted interaction just couples the tests to the current mutation wiring. If you want extra safety, try clicking and assert the success state never appears. As per coding guidelines "Test user-visible behavior, not implementation details; usescreen.getByRoleovergetByTestIdwhen possible".Also applies to: 226-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/boost.spec.tsx` around lines 205 - 208, Remove the implementation-detail assertion that inspects the mock call count from the disabled-button tests: delete the expect(mockBoostPlus).not.toHaveBeenCalled() assertions in the tests that locate nextButton via screen.getByRole (and the similar assertion at 226-228). Keep the user-visible assertion expect(nextButton).toBeDisabled(); and, if you want extra safety, replace the mock-count check by attempting a user action (clicking nextButton) and asserting the success state never appears (e.g., no success message or no state change) rather than checking mockBoostPlus directly.apps/web/src/features/shared/boost/index.tsx (1)
159-160: Rename the CTA to match the new behavior.This button no longer moves the user to another step; it performs the boost. Keeping
g.nexthere makes the shortened flow read as navigation instead of submission. Switch to a submit/confirm label. As per coding guidelines "All new strings must be added toen-US.jsononly for internationalization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/boost/index.tsx` around lines 159 - 160, The CTA Button in the Boost component currently uses i18next.t("g.next") which implies navigation but the handler (handleSubmit) performs the boost action; change the translation key to a submit/confirm label (e.g., i18next.t("boost.submit") or i18next.t("g.confirm")) where the Button props (onClick={handleSubmit} disabled={!canSubmit || inProgress}) remain unchanged, and add the new English string to en-US.json only (follow existing key naming conventions) so the label is internationalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx:
- Around line 29-31: The adminUsername state is only initialized once from
activeUser and won't update if activeUser arrives later; add an effect that
watches activeUser (or activeUser.username) and calls
setAdminUsername(activeUser?.username ?? "") when a username becomes
available—preferably only when adminUsername is empty to avoid clobbering user
edits. Update the component containing adminUsername, setAdminUsername, and
activeUser (in finalize-community-banner.tsx) to sync state in a useEffect
dependent on activeUser.username.
- Line 18: Import the ChangeEvent type directly from React and update the event
parameter annotations to use that imported type: add ChangeEvent to the existing
import (e.g., import { useState, ChangeEvent } from "react") and replace
occurrences of React.ChangeEvent in the component's input/textarea handlers with
ChangeEvent<HTMLInputElement> or ChangeEvent<HTMLTextAreaElement> as appropriate
(the handlers currently using React.ChangeEvent are the event parameters
referenced at the three spots in finalize-community-banner.tsx). Ensure the
specific handler signatures use the correct generic element type so TypeScript
strict mode no longer errors.
In `@apps/web/src/app/perks/points/buy-with-hive/_page.tsx`:
- Around line 57-76: Add unit/integration tests for the new one-step signing
flow in the component that contains the onSubmit handler (the submit logic
calling sign, recordActivity, setStep, and error/formatError). Write tests that
(1) mock sign to resolve and assert recordActivity is called and
setStep("success") is reached on success, (2) mock sign to reject and assert the
error(...) path is called with formatted error details, and (3) simulate rapid
duplicate submits (e.g., double click) and assert sign is only invoked once and
UI/state does not trigger duplicate recordActivity/setStep transitions. Use the
existing test harness (react-testing-library + jest) and mock or spy on sign,
recordActivity, setStep, and error to validate behavior.
- Around line 57-75: The submit handler in the onSubmit async block can be
invoked multiple times causing duplicate transfers; add a submit lock (e.g.,
isSubmitting state) shared between the form component (buy-with-hive-form) and
the onSubmit handler to short-circuit further clicks while a transfer is in
progress, set the lock before calling sign({ ... }), clear it in a finally block
after recordActivity/setStep or error handling, and ensure the form's submit
button uses this lock to disable clicks so sign(...) cannot be invoked
concurrently.
In `@apps/web/src/app/perks/promote-post/_page.tsx`:
- Line 57: The call to recordActivity() is unhandled and can produce unhandled
promise rejections; update the call site that currently invokes recordActivity()
to handle its Promise either by awaiting it inside an async function with
try/catch or by chaining .catch(...) to the returned Promise, and ensure the
catch logs or silences the error (e.g., using console.warn/processLogger.warn)
so rejections are handled consistently with other usages of recordActivity().
In `@apps/web/src/features/shared/boost/index.tsx`:
- Around line 70-71: canSubmit currently becomes true as soon as account is
non-empty, allowing submission before the async boost-status and price/duration
initialization complete; update the guard used (the useMemo/boolean that reads
balanceError, isAlreadyBoosted, account) to also require the boost-status query
to be settled (e.g., !isLoadingBoostStatus && !isFetchingBoostStatus &&
boostStatusLoaded) and that duration has been derived from prices (e.g.,
duration > 0 or !isFetchingPrices), so include those symbols
(isLoadingBoostStatus / isFetchingBoostStatus / boostStatusLoaded and duration /
isFetchingPrices) in the dependency array and the boolean condition to block
submit until async checks finish.
- Around line 82-85: handleSubmit can be entered twice synchronously; add a
local synchronous re-entry guard (e.g., isSubmittingRef or a module-scoped
boolean) inside the handleSubmit callback to early-return if already running,
set the guard true immediately before calling boostPlus({ account, duration })
and clear it in a finally block (or after awaiting) so setStep(2) still runs
only once; reference the handleSubmit function, boostPlus call, setStep, and the
dependencies ([account, duration, boostPlus]) when implementing the guard.
In `@apps/web/src/specs/features/shared/boost.spec.tsx`:
- Around line 192-194: Tests seed cache with raw arrays
(queryClient.setQueryData(["boost-account"], ...) and ["points"]) which diverges
from SDK contract; replace these with the QueryKeys helpers from `@ecency/sdk`.
Import QueryKeys and call QueryKeys.promotions.boostPlusAccounts(account) for
the boost-account seed and QueryKeys.points.points(username, filter) for the
points seed (use the same account/username and filter values used in the test),
then pass those results into queryClient.setQueryData instead of the hardcoded
arrays.
In `@apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx`:
- Around line 7-8: Add an explicit module mock for the useActiveAccount hook
before any mockReturnValue calls: call
vi.mock('@/core/hooks/use-active-account', () => ({ useActiveAccount: vi.fn()
})), then import useActiveAccount as before and cast it to a mock when setting
return values (e.g., (useActiveAccount as vi.Mock).mockReturnValue(...)). Apply
this pattern to every existing mockReturnValue usage for useActiveAccount in the
test file so all calls use the explicitly mocked function.
---
Nitpick comments:
In `@apps/web/src/api/mutations/sign-transfer.ts`:
- Around line 35-40: Add a JSDoc comment to the SignTransferPayload interface
describing the optional asset field: explain that asset?: TransferAsset
overrides the hook-level/default asset for this specific transfer, when callers
should provide it (e.g., sending a different token than the hook-configured
asset), and any important format/validation expectations tied to the
TransferAsset type; place the comment immediately above the SignTransferPayload
declaration so consumers of sign-transfer.ts and the SignTransferPayload type
see the behavior clearly.
In `@apps/web/src/features/shared/boost/index.tsx`:
- Around line 159-160: The CTA Button in the Boost component currently uses
i18next.t("g.next") which implies navigation but the handler (handleSubmit)
performs the boost action; change the translation key to a submit/confirm label
(e.g., i18next.t("boost.submit") or i18next.t("g.confirm")) where the Button
props (onClick={handleSubmit} disabled={!canSubmit || inProgress}) remain
unchanged, and add the new English string to en-US.json only (follow existing
key naming conventions) so the label is internationalized.
In `@apps/web/src/features/shared/transfer/transfer-step-2.tsx`:
- Around line 33-41: The current handleConfirmAndSign awaits refetch(), which
can block progression to setStep(3) if refetch errors; change the logic in
handleConfirmAndSign (the async callback using signTransfer, refetch and
setStep) so the transfer result drives flow: either call refetch() without
awaiting (fire-and-forget) after successful signTransfer, or wrap refetch() in
its own try/catch so any refetch failure does not throw out of the main try and
you still call setStep(3); ensure references to signTransfer, refetch and
setStep remain intact and errors from refetch are logged but do not prevent
advancing to step 3.
In `@apps/web/src/specs/features/shared/boost.spec.tsx`:
- Around line 205-208: Remove the implementation-detail assertion that inspects
the mock call count from the disabled-button tests: delete the
expect(mockBoostPlus).not.toHaveBeenCalled() assertions in the tests that locate
nextButton via screen.getByRole (and the similar assertion at 226-228). Keep the
user-visible assertion expect(nextButton).toBeDisabled(); and, if you want extra
safety, replace the mock-count check by attempting a user action (clicking
nextButton) and asserting the success state never appears (e.g., no success
message or no state change) rather than checking mockBoostPlus directly.
In `@apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx`:
- Around line 24-26: Replace hardcoded query key arrays in
getCommunityQueryOptions and the other similar query option factories with the
SDK QueryKeys source: import QueryKeys from `@ecency/sdk` and use
QueryKeys.community(username) (or the appropriate QueryKeys method for each
case) instead of literal arrays like ["community", username]; update the
queryKey properties across the occurrences referenced (e.g., the
getCommunityQueryOptions function and the other query option factories noted in
the comment) so all cache keys derive from QueryKeys to keep a single source of
truth.
- Around line 3-6: Replace the manual QueryClientProvider wrapper in the spec
for FinalizeCommunityBanner with the repository test helper: import and use
renderWithQueryClient from the test-utils file instead of creating a new
QueryClient and wrapping with QueryClientProvider; update the imports to remove
QueryClient/QueryClientProvider and call
renderWithQueryClient(<FinalizeCommunityBanner ... />) wherever the wrapper is
used (also apply the same replacement at the occurrences around lines 57-63) to
standardize React Query test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4aff790-a349-48d3-8a56-dd8c0483425c
📒 Files selected for processing (13)
apps/web/src/api/mutations/pre-check-promote.tsapps/web/src/api/mutations/sign-transfer.tsapps/web/src/app/(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsxapps/web/src/app/communities/create/_components/community-create-sign-step.tsxapps/web/src/app/perks/points/buy-with-hive/_page.tsxapps/web/src/app/perks/promote-post/_page.tsxapps/web/src/features/shared/boost/index.tsxapps/web/src/features/shared/buy-sell-hive/index.tsxapps/web/src/features/shared/promote/index.tsxapps/web/src/features/shared/transfer/index.tsxapps/web/src/features/shared/transfer/transfer-step-2.tsxapps/web/src/specs/features/shared/boost.spec.tsxapps/web/src/specs/features/shared/finalize-community-banner.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/features/shared/promote/index.tsx
- apps/web/src/features/shared/transfer/index.tsx
Comment on lines
+29
to
+31
| const [adminUsername, setAdminUsername] = useState(activeUser?.username ?? ""); | ||
| const [submitting, setSubmitting] = useState(false); | ||
| const [done, setDone] = useState(false); |
Contributor
There was a problem hiding this comment.
Keep adminUsername in sync when activeUser arrives after first render.
Line 29 initializes from activeUser once; if account data resolves later, adminUsername can stay empty and keep submit disabled until user manually retypes.
💡 Suggested fix
-import { useState, type ChangeEvent } from "react";
+import { useEffect, useState, type ChangeEvent } from "react";
...
const [adminUsername, setAdminUsername] = useState(activeUser?.username ?? "");
+ useEffect(() => {
+ if (activeUser?.username && !adminUsername) {
+ setAdminUsername(activeUser.username);
+ }
+ }, [activeUser?.username, adminUsername]);📝 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.
Suggested change
| const [adminUsername, setAdminUsername] = useState(activeUser?.username ?? ""); | |
| const [submitting, setSubmitting] = useState(false); | |
| const [done, setDone] = useState(false); | |
| import { useEffect, useState, type ChangeEvent } from "react"; | |
| // ... other imports and code ... | |
| const [adminUsername, setAdminUsername] = useState(activeUser?.username ?? ""); | |
| const [submitting, setSubmitting] = useState(false); | |
| const [done, setDone] = useState(false); | |
| useEffect(() => { | |
| if (activeUser?.username && !adminUsername) { | |
| setAdminUsername(activeUser.username); | |
| } | |
| }, [activeUser?.username, adminUsername]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx
around lines 29 - 31, The adminUsername state is only initialized once from
activeUser and won't update if activeUser arrives later; add an effect that
watches activeUser (or activeUser.username) and calls
setAdminUsername(activeUser?.username ?? "") when a username becomes
available—preferably only when adminUsername is empty to avoid clobbering user
edits. Update the component containing adminUsername, setAdminUsername, and
activeUser (in finalize-community-banner.tsx) to sync state in a useEffect
dependent on activeUser.username.
Comment on lines
+57
to
76
| onSubmit={async (formAmount, formAsset, formPointsAmount) => { | ||
| setAmount(formAmount); | ||
| setAsset(formAsset); | ||
| setPointsAmount(formPointsAmount); | ||
|
|
||
| if (!activeUser) return; | ||
|
|
||
| try { | ||
| await sign({ | ||
| to: "esteem.app", | ||
| amount: formAmount, | ||
| memo: "points", | ||
| asset: formAsset as TransferAsset, | ||
| }); | ||
| recordActivity(); | ||
| setStep("success"); | ||
| } catch (e) { | ||
| error(...formatError(e)); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
Please add tests for the new one-step signing flow.
Lines 57-76 changed the core transaction UX (form submit now signs and completes in one step), but no corresponding test updates are included in the provided files. Please add coverage for at least success, transfer failure, and duplicate-click behavior.
As per coding guidelines, "All new features in @ecency/web require tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/perks/points/buy-with-hive/_page.tsx` around lines 57 - 76,
Add unit/integration tests for the new one-step signing flow in the component
that contains the onSubmit handler (the submit logic calling sign,
recordActivity, setStep, and error/formatError). Write tests that (1) mock sign
to resolve and assert recordActivity is called and setStep("success") is reached
on success, (2) mock sign to reject and assert the error(...) path is called
with formatted error details, and (3) simulate rapid duplicate submits (e.g.,
double click) and assert sign is only invoked once and UI/state does not trigger
duplicate recordActivity/setStep transitions. Use the existing test harness
(react-testing-library + jest) and mock or spy on sign, recordActivity, setStep,
and error to validate behavior.
Comment on lines
+70
to
+71
| () => !balanceError && !isAlreadyBoosted && account.length > 0, | ||
| [balanceError, isAlreadyBoosted, account] |
Contributor
There was a problem hiding this comment.
Block submit until the async checks have settled.
canSubmit flips to true as soon as account is non-empty. In the new one-click flow, that lets users submit before the boost-status query for the typed account finishes and before duration has been initialized from prices, so boostPlus can run with stale validation or duration === 0.
🛠️ Suggested guard
- const { data: alreadyBoostAccount } = useQuery(
+ const { data: alreadyBoostAccount, isLoading: isCheckingBoostStatus } = useQuery(
getBoostPlusAccountPricesQueryOptions(account, accessToken)
);
+ const hasValidDuration = useMemo(
+ () => !!prices?.some((item) => item.duration === duration),
+ [prices, duration]
+ );
const canSubmit = useMemo(
- () => !balanceError && !isAlreadyBoosted && account.length > 0,
- [balanceError, isAlreadyBoosted, account]
+ () =>
+ !isCheckingBoostStatus &&
+ !balanceError &&
+ !isAlreadyBoosted &&
+ hasValidDuration &&
+ account.length > 0,
+ [balanceError, isAlreadyBoosted, account, hasValidDuration, isCheckingBoostStatus]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/shared/boost/index.tsx` around lines 70 - 71, canSubmit
currently becomes true as soon as account is non-empty, allowing submission
before the async boost-status and price/duration initialization complete; update
the guard used (the useMemo/boolean that reads balanceError, isAlreadyBoosted,
account) to also require the boost-status query to be settled (e.g.,
!isLoadingBoostStatus && !isFetchingBoostStatus && boostStatusLoaded) and that
duration has been derived from prices (e.g., duration > 0 or !isFetchingPrices),
so include those symbols (isLoadingBoostStatus / isFetchingBoostStatus /
boostStatusLoaded and duration / isFetchingPrices) in the dependency array and
the boolean condition to block submit until async checks finish.
Comment on lines
+192
to
+194
| queryClient.setQueryData(["boost-account"], { | ||
| expires: new Date(Date.now() + 86400000).toISOString() | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the canonical QueryKeys helpers for the boost-account and points queries.
rg -n "QueryKeys\." packages/sdk/src apps/web/src -g '*.{ts,tsx}'
rg -n "getPointsQueryOptions|getBoostPlusAccountPricesQueryOptions|boostPlusAccounts" packages/sdk/src apps/web/src -g '*.{ts,tsx}'Repository: ecency/vision-next
Length of output: 40530
🏁 Script executed:
# Check the exact content of the boost.spec.tsx file at the target lines
head -220 apps/web/src/specs/features/shared/boost.spec.tsx | tail -35Repository: ecency/vision-next
Length of output: 1279
🏁 Script executed:
# Check the QueryKeys definitions in SDK
cat packages/sdk/src/modules/core/query-keys.ts | head -500Repository: ecency/vision-next
Length of output: 19132
Use QueryKeys instead of raw cache arrays in these test seeds.
The hardcoded arrays ["boost-account"] and ["points"] diverge from the SDK's cache contract. The actual query keys are:
QueryKeys.promotions.boostPlusAccounts(account)→["promotions", "boost-plus-accounts", account]QueryKeys.points.points(username, filter)→["points", username, filter]
These test seeds will drift if the cache key shape changes. Replace with the matching QueryKeys helpers from @ecency/sdk instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/specs/features/shared/boost.spec.tsx` around lines 192 - 194,
Tests seed cache with raw arrays (queryClient.setQueryData(["boost-account"],
...) and ["points"]) which diverges from SDK contract; replace these with the
QueryKeys helpers from `@ecency/sdk`. Import QueryKeys and call
QueryKeys.promotions.boostPlusAccounts(account) for the boost-account seed and
QueryKeys.points.points(username, filter) for the points seed (use the same
account/username and filter values used in the test), then pass those results
into queryClient.setQueryData instead of the hardcoded arrays.
Comment on lines
+7
to
+8
| import { useActiveAccount } from "@/core/hooks/use-active-account"; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -t f 'finalize-community-banner.spec.tsx' | head -n1)"
echo "Inspecting: $FILE"
echo "---- useActiveAccount references ----"
rg -n '\buseActiveAccount\b' "$FILE"
echo "---- explicit module mock presence ----"
rg -n 'vi\.mock\("@/core/hooks/use-active-account"' "$FILE" || true
echo "---- mockReturnValue usage ----"
rg -n 'mockReturnValue\(' "$FILE"Repository: ecency/vision-next
Length of output: 1151
🏁 Script executed:
head -n 200 "$(fd -t f 'finalize-community-banner.spec.tsx' | head -n1)"Repository: ecency/vision-next
Length of output: 6110
🏁 Script executed:
rg -l "useActiveAccount" "$(fd -t d 'specs' | head -n1)" --type tsx | head -5Repository: ecency/vision-next
Length of output: 89
🏁 Script executed:
# Check how other test files handle this - look for examples of testing components with useActiveAccount or similar internal hooks
rg -A5 "renderWithQueryClient\|vi\.mock.*use" apps/web/src/specs --type tsx | head -60Repository: ecency/vision-next
Length of output: 89
🏁 Script executed:
# Find test files using useActiveAccount
fd -t f '\.spec\.(ts|tsx)$' apps/web/src/specs | xargs rg -l "useActiveAccount" | head -5Repository: ecency/vision-next
Length of output: 369
🏁 Script executed:
# Check if renderWithQueryClient is used in other test files to see the pattern
fd -t f '\.spec\.(ts|tsx)$' apps/web/src/specs | xargs rg "renderWithQueryClient" -l | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'Repository: ecency/vision-next
Length of output: 4920
Add explicit mock for useActiveAccount hook.
The test file calls .mockReturnValue() on useActiveAccount without first mocking it via vi.mock(). This will cause runtime errors. Follow the pattern used in wallet-badge.spec.tsx:
Suggested fix
import { useActiveAccount } from "@/core/hooks/use-active-account";
+vi.mock("@/core/hooks/use-active-account", () => ({
+ useActiveAccount: vi.fn()
+}));
+
+const mockedUseActiveAccount = vi.mocked(useActiveAccount);
...
- (useActiveAccount as any).mockReturnValue({
+ mockedUseActiveAccount.mockReturnValue({
activeUser: { username: "regularuser" }
});Apply the same replacement for all mockReturnValue calls (lines 82-84, 92-94, 102-104, 115-117, 126-128, 138-140, 154-156, 189-191).
📝 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.
Suggested change
| import { useActiveAccount } from "@/core/hooks/use-active-account"; | |
| import { useActiveAccount } from "@/core/hooks/use-active-account"; | |
| vi.mock("@/core/hooks/use-active-account", () => ({ | |
| useActiveAccount: vi.fn() | |
| })); | |
| const mockedUseActiveAccount = vi.mocked(useActiveAccount); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx` around
lines 7 - 8, Add an explicit module mock for the useActiveAccount hook before
any mockReturnValue calls: call vi.mock('@/core/hooks/use-active-account', () =>
({ useActiveAccount: vi.fn() })), then import useActiveAccount as before and
cast it to a mock when setting return values (e.g., (useActiveAccount as
vi.Mock).mockReturnValue(...)). Apply this pattern to every existing
mockReturnValue usage for useActiveAccount in the test file so all calls use the
explicitly mocked function.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/web/src/app/perks/points/buy-with-hive/_page.tsx (2)
57-75:⚠️ Potential issue | 🔴 CriticalPrevent re-entrant submits for the transfer path.
Lines 57-75 run a broadcasting mutation, but the submit trigger in
apps/web/src/app/perks/points/buy-with-hive/_components/buy-with-hive-form.tsx(Line 95) can fire repeatedly. Add an in-flight lock (and disable submit while pending) so rapid clicks cannot send duplicate transfers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/perks/points/buy-with-hive/_page.tsx` around lines 57 - 75, Add an in-flight lock around the onSubmit transfer flow to prevent re-entrant submits: introduce an isSubmitting state (or prop) checked at the top of the onSubmit handler in the component containing onSubmit (return early if isSubmitting), set isSubmitting = true before calling sign(...) and ensure it is reset to false in a finally block after sign/recordActivity, and pass that state down to the BuyWithHiveForm component (buy-with-hive-form.tsx) to disable its submit button while pending; reference the onSubmit handler, sign({...}), recordActivity(), setStep("success") and the buy-with-hive-form submit trigger to locate where to add the lock and disable behavior.
57-75:⚠️ Potential issue | 🟠 MajorAdd test coverage for the one-step signing flow.
Line 57 onward changes core transaction behavior (submit now signs + finalizes). Please add tests for success, failure, and duplicate-submit behavior.
As per coding guidelines, "All new features in
@ecency/webrequire tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/perks/points/buy-with-hive/_page.tsx` around lines 57 - 75, Add unit/integration tests covering the new one-step signing flow in the BuyWithHive page: write tests that exercise the onSubmit handler (the async arrow passed to onSubmit in _page.tsx) to assert (1) successful sign() calls progress to setStep("success") and trigger recordActivity() being called, (2) sign() rejections call error(...formatError(e)) and do not set success, and (3) duplicate submits (calling onSubmit twice quickly) do not trigger multiple sign() invocations or double state transitions. Mock sign(), recordActivity(), formatError and any activeUser state, and assert interactions and final state updates for success, failure, and duplicate-submit scenarios.
🧹 Nitpick comments (1)
apps/web/src/app/(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx (1)
33-35: Consider memoizing the adapter to avoid re-creation on every render.
getWebBroadcastAdapter()is called on every render. While the hooks likely handle this internally, memoizing prevents unnecessary function invocations.💡 Suggested optimization
+import { useMemo } from "react"; ... - const adapter = getWebBroadcastAdapter(); + const adapter = useMemo(() => getWebBroadcastAdapter(), []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx around lines 33 - 35, getWebBroadcastAdapter() is being invoked on every render when creating adapter for useSetCommunityRole and useUpdateCommunity; memoize that call so the adapter instance is stable and not re-created each render. Wrap the getWebBroadcastAdapter() invocation in a useMemo (e.g., const adapter = useMemo(() => getWebBroadcastAdapter(), [])) and keep passing that memoized adapter to useSetCommunityRole and useUpdateCommunity to avoid unnecessary re-invocation and re-subscriptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/api/mutations/sign-transfer.ts`:
- Around line 89-99: Normalize and validate the parsed amount once before the
switch: parse the incoming amount into a single numeric variable (e.g.,
amountNum or amountFloat), check for NaN/invalid and throw or return an error,
then derive any formatted values (e.g., fullAmount = `${amountNum.toFixed(3)}
${effectiveAsset}` and amountInMillis = Math.round(amountNum * 1000)) and reuse
those in the transfer branches instead of re-parsing; update the branches that
call transferPoint.mutateAsync, transferSpk.mutateAsync, and
transferLarynx.mutateAsync to use the validated normalized values.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx:
- Around line 44-48: The current short-circuit return hides the finalize banner
when the community query errors (the condition using isMyProfile,
isCommunity(username), isLoading, isError, community !== null, done), so change
the logic to not bail out on isError; instead render the banner when isMyProfile
&& isCommunity(username) && !isLoading && !done and then inside the banner
component handle three states: success (community === null -> show finalization
UI), loading, and error (isError -> show a degraded state with an explicit retry
button or error message). Update the conditional around isError (and the
consumer of community) so the banner can render and display a retry action that
re-triggers the community query (or calls the queryRefetch) when the user clicks
retry.
- Around line 50-71: The current handleSubmit runs setRole before
updateCommunity which can leave an admin assigned while community metadata
fails; change the flow to call updateCommunity first and only call setRole if
updateCommunity resolves, add a local flag (e.g., didUpdate) or state to track
which step completed so retries can target the failed step, and improve the
error path (error(...)) to include whether the update or role assignment failed
(partial success) and surface that to the user; ensure setSubmitting and setDone
are updated appropriately (setDone only when both succeed) and that any rollback
or targeted retry logic uses the tracked step information.
- Around line 83-121: The labels in finalize-community-banner are not associated
with their inputs; add matching id props to each FormControl and htmlFor
attributes to the corresponding <label> so clicks and screen readers work: for
the title field (value/title) set label htmlFor="community-title" and
FormControl id="community-title"; for the about field (value/about) use
htmlFor="community-about" and FormControl id="community-about"; for the admin
username field (value/adminUsername) use htmlFor="community-admin-username" and
FormControl id="community-admin-username". Ensure the id values match exactly
and keep the existing onChange, value and disabled props on FormControl.
---
Duplicate comments:
In `@apps/web/src/app/perks/points/buy-with-hive/_page.tsx`:
- Around line 57-75: Add an in-flight lock around the onSubmit transfer flow to
prevent re-entrant submits: introduce an isSubmitting state (or prop) checked at
the top of the onSubmit handler in the component containing onSubmit (return
early if isSubmitting), set isSubmitting = true before calling sign(...) and
ensure it is reset to false in a finally block after sign/recordActivity, and
pass that state down to the BuyWithHiveForm component (buy-with-hive-form.tsx)
to disable its submit button while pending; reference the onSubmit handler,
sign({...}), recordActivity(), setStep("success") and the buy-with-hive-form
submit trigger to locate where to add the lock and disable behavior.
- Around line 57-75: Add unit/integration tests covering the new one-step
signing flow in the BuyWithHive page: write tests that exercise the onSubmit
handler (the async arrow passed to onSubmit in _page.tsx) to assert (1)
successful sign() calls progress to setStep("success") and trigger
recordActivity() being called, (2) sign() rejections call
error(...formatError(e)) and do not set success, and (3) duplicate submits
(calling onSubmit twice quickly) do not trigger multiple sign() invocations or
double state transitions. Mock sign(), recordActivity(), formatError and any
activeUser state, and assert interactions and final state updates for success,
failure, and duplicate-submit scenarios.
---
Nitpick comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx:
- Around line 33-35: getWebBroadcastAdapter() is being invoked on every render
when creating adapter for useSetCommunityRole and useUpdateCommunity; memoize
that call so the adapter instance is stable and not re-created each render. Wrap
the getWebBroadcastAdapter() invocation in a useMemo (e.g., const adapter =
useMemo(() => getWebBroadcastAdapter(), [])) and keep passing that memoized
adapter to useSetCommunityRole and useUpdateCommunity to avoid unnecessary
re-invocation and re-subscriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7b2f7fc-7326-48d8-995b-9b3ef6c91e5c
📒 Files selected for processing (7)
apps/web/src/api/mutations/sign-transfer.tsapps/web/src/app/(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsxapps/web/src/app/perks/points/buy-with-hive/_page.tsxapps/web/src/app/perks/promote-post/_page.tsxapps/web/src/features/shared/boost/index.tsxapps/web/src/features/shared/transfer/transfer-step-2.tsxapps/web/src/specs/features/shared/finalize-community-banner.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/specs/features/shared/finalize-community-banner.spec.tsx
- apps/web/src/features/shared/transfer/transfer-step-2.tsx
- apps/web/src/features/shared/boost/index.tsx
Comment on lines
+89
to
99
| const fullAmount = `${(+amount).toFixed(3)} ${effectiveAsset}`; | ||
| const requestId = Date.now() >>> 0; | ||
|
|
||
| switch (mode) { | ||
| case "transfer": | ||
| if (asset === "POINT") { | ||
| if (effectiveAsset === "POINT") { | ||
| await transferPoint.mutateAsync({ to, amount: fullAmount, memo }); | ||
| } else if (asset === "SPK") { | ||
| } else if (effectiveAsset === "SPK") { | ||
| await transferSpk.mutateAsync({ to, amount: parseFloat(amount) * 1000 }); | ||
| } else if (asset === "LARYNX") { | ||
| } else if (effectiveAsset === "LARYNX") { | ||
| await transferLarynx.mutateAsync({ to, amount: parseFloat(amount) * 1000 }); |
Contributor
There was a problem hiding this comment.
Normalize and validate amount parsing once before building payloads.
Line 89 can produce a NaN amount string, and Lines 97/99 parse the same input again via a different parser. Parse once, validate, and reuse for all branches.
Suggested patch
- mutateAsync: async ({ to, amount, memo, asset: overrideAsset }: SignTransferPayload) => {
+ mutateAsync: async ({ to, amount, memo, asset: overrideAsset }: SignTransferPayload) => {
const effectiveAsset = overrideAsset ?? asset;
- const fullAmount = `${(+amount).toFixed(3)} ${effectiveAsset}`;
+ const numericAmount = Number(amount);
+ if (!Number.isFinite(numericAmount)) {
+ throw new Error("Invalid transfer amount");
+ }
+ const fullAmount = `${numericAmount.toFixed(3)} ${effectiveAsset}`;
const requestId = Date.now() >>> 0;
@@
- } else if (effectiveAsset === "SPK") {
- await transferSpk.mutateAsync({ to, amount: parseFloat(amount) * 1000 });
- } else if (effectiveAsset === "LARYNX") {
- await transferLarynx.mutateAsync({ to, amount: parseFloat(amount) * 1000 });
+ } else if (effectiveAsset === "SPK") {
+ await transferSpk.mutateAsync({ to, amount: numericAmount * 1000 });
+ } else if (effectiveAsset === "LARYNX") {
+ await transferLarynx.mutateAsync({ to, amount: numericAmount * 1000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/api/mutations/sign-transfer.ts` around lines 89 - 99, Normalize
and validate the parsed amount once before the switch: parse the incoming amount
into a single numeric variable (e.g., amountNum or amountFloat), check for
NaN/invalid and throw or return an error, then derive any formatted values
(e.g., fullAmount = `${amountNum.toFixed(3)} ${effectiveAsset}` and
amountInMillis = Math.round(amountNum * 1000)) and reuse those in the transfer
branches instead of re-parsing; update the branches that call
transferPoint.mutateAsync, transferSpk.mutateAsync, and
transferLarynx.mutateAsync to use the validated normalized values.
Comment on lines
+44
to
+48
| // Only show if: this is a community account, user is logged in as this account, and community doesn't exist | ||
| const isMyProfile = activeUser?.username === username; | ||
| if (!isMyProfile || !isCommunity(username) || isLoading || isError || community !== null || done) { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Query errors silently hide the banner.
When isError is true, the banner returns null without feedback. If the community query fails (network issue, hivemind down), users won't see the finalization prompt even when needed. Consider either retrying the query or showing a degraded state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx
around lines 44 - 48, The current short-circuit return hides the finalize banner
when the community query errors (the condition using isMyProfile,
isCommunity(username), isLoading, isError, community !== null, done), so change
the logic to not bail out on isError; instead render the banner when isMyProfile
&& isCommunity(username) && !isLoading && !done and then inside the banner
component handle three states: success (community === null -> show finalization
UI), loading, and error (isError -> show a degraded state with an explicit retry
button or error message). Update the conditional around isError (and the
consumer of community) so the banner can render and display a retry action that
re-triggers the community query (or calls the queryRefetch) when the user clicks
retry.
Comment on lines
+50
to
+71
| const handleSubmit = async () => { | ||
| if (!adminUsername.trim() || !title.trim()) return; | ||
|
|
||
| setSubmitting(true); | ||
| try { | ||
| await setRole({ account: adminUsername.trim(), role: "admin" }); | ||
| await updateCommunity({ | ||
| title: title.trim(), | ||
| about: about.trim(), | ||
| lang: "en", | ||
| description: "", | ||
| flag_text: "", | ||
| is_nsfw: false | ||
| }); | ||
| success(i18next.t("communities-create.finalize-success")); | ||
| setDone(true); | ||
| } catch (e) { | ||
| error(...formatError(e)); | ||
| } finally { | ||
| setSubmitting(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Partial failure leaves community in an inconsistent state.
If setRole succeeds but updateCommunity fails, the admin role is assigned but community metadata remains empty. The user sees an error but the operations aren't atomic—retrying would attempt setRole again (which may fail or be a no-op) before retrying the update.
Consider:
- Reversing the order (update metadata first, then assign role) so the more critical operation succeeds first
- Tracking which step failed to allow targeted retry
- Clarifying the error message to indicate partial success
💡 Suggested approach: reorder operations
try {
+ // Update metadata first - this is the primary finalization step
+ await updateCommunity({
+ title: title.trim(),
+ about: about.trim(),
+ lang: "en",
+ description: "",
+ flag_text: "",
+ is_nsfw: false
+ });
+ // Then assign admin role
await setRole({ account: adminUsername.trim(), role: "admin" });
- await updateCommunity({
- title: title.trim(),
- about: about.trim(),
- lang: "en",
- description: "",
- flag_text: "",
- is_nsfw: false
- });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/finalize-community-banner.tsx
around lines 50 - 71, The current handleSubmit runs setRole before
updateCommunity which can leave an admin assigned while community metadata
fails; change the flow to call updateCommunity first and only call setRole if
updateCommunity resolves, add a local flag (e.g., didUpdate) or state to track
which step completed so retries can target the failed step, and improve the
error path (error(...)) to include whether the update or role assignment failed
(partial success) and surface that to the user; ensure setSubmitting and setDone
are updated appropriately (setDone only when both succeed) and that any rollback
or targeted retry logic uses the tracked step information.
This was referenced Mar 29, 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
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
Improvements
Localization
Tests