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:
📝 WalkthroughWalkthroughIntroduces multi-currency support: adds currency types/utilities, server-side USD→fiat conversion with caching, persists user currency, returns converted balances and token reference pricing in assets APIs, updates DB schema and UI/hooks to surface and format chosen currencies. Changes
Sequence DiagramsequenceDiagram
participant UI as Frontend
participant API as Assets API
participant DB as Database
participant Cache as NodeCache
participant CG as CoinGecko
UI->>API: GET /api/v1/spaces/[slug]/assets?currency=EUR
activate API
API->>DB: findSelf(authToken) (optional)
DB-->>API: user { currency?: "USD" } (or undefined)
API->>Cache: get usd_to_eur
alt cache miss
API->>CG: GET /simple/price?ids=usd&vs_currencies=eur,...
CG-->>API: { usd: { eur: 0.92, ... } }
API->>Cache: set usd_to_eur = 0.92
else cache hit
Cache-->>API: 0.92
end
API->>API: compute usdBalance = sum(assets.usdEqual)
API->>API: converted = usdBalance * rate
API-->>UI: { balance: converted, currency: "EUR", assets: [...] }
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 unit tests (beta)
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 |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/epics/src/treasury/hooks/use-user-assets.ts (1)
32-57: Extract the asset payload shape into a shared type.
AssetItem/UseAssetsReturnnow mirror fields that are also being threaded through server token metadata and asset display props. Pulling this into a common type will make the new currency/reference fields much less likely to drift out of sync.Based on learnings: DSanich prefers extracting repeated type declarations into shared modules for reusability and maintainability, particularly for token types that are used across multiple files in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/treasury/hooks/use-user-assets.ts` around lines 32 - 57, The AssetItem and UseAssetsReturn types are duplicated elsewhere; extract their shape into a shared exported type (e.g., export type AssetItem and export type UseAssetsReturn) in a common module and replace the local declarations in use-user-assets.ts by importing those shared types; update all other places that currently mirror these fields (server token metadata, asset display props, etc.) to import and use the shared types so the new currency/reference fields (referencePrice, referenceCurrency) and optional space fields remain consistent across the codebase.packages/storage-postgres/src/schema/people.ts (1)
19-19: Constrainpeople.currencyat the schema layer too.The app now validates this field against
SUPPORTED_CURRENCIES, but the column still accepts arbitrary text. A DB enum or check constraint here would keep non-validated write paths from persisting unsupported currency codes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage-postgres/src/schema/people.ts` at line 19, The people.schema currently defines currency as text('currency') allowing any string; add a database-level constraint so only SUPPORTED_CURRENCIES are persisted—modify the schema definition for the people table (the currency column) to use a DB enum type or a CHECK constraint referencing the allowed currency codes (instead of text('currency')) so writes that bypass app validation are rejected; update any migration/DDL for the people table and ensure the enum/check uses the same SUPPORTED_CURRENCIES set and that the column symbol named currency is altered/created accordingly.packages/epics/src/treasury/hooks/use-assets.ts (1)
46-47: Reuse the shared currency union in this hook contract.The new currency-bearing fields are still plain
strings, so consumers lose the type safety that the rest of this PR just centralized. ThreadingSupportedCurrencythrough here keeps the hook/API boundary aligned and makes impossible currency values a compile-time error.♻️ Proposed type-only cleanup
+import type { SupportedCurrency } from '@hypha-platform/core/client'; + type AssetItem = { icon: string; name: string; symbol: string; @@ - referenceCurrency?: string | null; + referenceCurrency?: SupportedCurrency | null; }; @@ - currency?: string; + currency?: SupportedCurrency; }; @@ - currency: string; + currency: SupportedCurrency; };Based on learnings, DSanich prefers extracting repeated type declarations into shared modules for reusability and maintainability, particularly for token types that are used across multiple files in the codebase.
Also applies to: 54-55, 61-62, 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/treasury/hooks/use-assets.ts` around lines 46 - 47, Update the hook's asset/type contract to use the shared SupportedCurrency union instead of plain strings: replace any occurrences of referenceCurrency?: string | null (and other currency-bearing fields in the same type/hook at the other spots mentioned) with referenceCurrency?: SupportedCurrency | null (and analogous fields), and add an import for SupportedCurrency from the shared types module; ensure all references in useAssets/use-assets types and return types are updated to preserve nullability and compile-time currency safety.packages/core/src/common/currency.ts (1)
17-42: Derive labels and symbols from one metadata map.
SUPPORTED_CURRENCIES,CURRENCY_OPTIONS, andCURRENCY_SYMBOLSnow repeat the same codes in three places. A singleRecord<SupportedCurrency, { label: string; symbol: string }>would let you derive both exports and prevent the options list from drifting the next time a currency is added or removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/common/currency.ts` around lines 17 - 42, Create a single source-of-truth map (e.g., CURRENCY_META: Record<SupportedCurrency, { label: string; symbol: string }>) and replace the duplicated definitions by deriving CURRENCY_OPTIONS and CURRENCY_SYMBOLS from that map; update code that references SUPPORTED_CURRENCIES (if present) to use Object.keys/values from CURRENCY_META (or build a SUPPORTED_CURRENCIES array from it) so additions/removals only happen in one place and both CURRENCY_OPTIONS and CURRENCY_SYMBOLS are generated from CURRENCY_META.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/common/server/get-fiat-conversion-rate.ts`:
- Around line 33-48: The request is using `ids=usd` which is likely not a valid
CoinGecko coin id; change the implementation in get-fiat-conversion-rate (file:
get-fiat-conversion-rate.ts) to call the CoinGecko /exchange_rates endpoint
instead of simple/price, parse the returned data.rates to find rates for the
normalized target currency and for "usd" (e.g., data.rates[normalizedCurrency]
and data.rates.usd), compute the conversion as targetRate / usdRate, and use
that as the rate; also surface/log any fetch or parse errors instead of silently
falling back to `{ currency: 'USD', rate: 1 }`.
In `@packages/storage-postgres/migrations/meta/0041_snapshot.json`:
- Around line 358-398: The migration snapshot defines row-level policies for the
table public.people (crud-authenticated-policy-select, -insert, -update,
-delete) but isRLSEnabled is false and the paired SQL migration
(0041_rainy_green_goblin.sql) does not enable RLS; update the SQL migration to
run ALTER TABLE public.people ENABLE ROW LEVEL SECURITY and ensure the migration
creates the four policies (or leaves policy creation in sync), and update the
snapshot's isRLSEnabled flag to true so the policies become effective for
public.people.
---
Nitpick comments:
In `@packages/core/src/common/currency.ts`:
- Around line 17-42: Create a single source-of-truth map (e.g., CURRENCY_META:
Record<SupportedCurrency, { label: string; symbol: string }>) and replace the
duplicated definitions by deriving CURRENCY_OPTIONS and CURRENCY_SYMBOLS from
that map; update code that references SUPPORTED_CURRENCIES (if present) to use
Object.keys/values from CURRENCY_META (or build a SUPPORTED_CURRENCIES array
from it) so additions/removals only happen in one place and both
CURRENCY_OPTIONS and CURRENCY_SYMBOLS are generated from CURRENCY_META.
In `@packages/epics/src/treasury/hooks/use-assets.ts`:
- Around line 46-47: Update the hook's asset/type contract to use the shared
SupportedCurrency union instead of plain strings: replace any occurrences of
referenceCurrency?: string | null (and other currency-bearing fields in the same
type/hook at the other spots mentioned) with referenceCurrency?:
SupportedCurrency | null (and analogous fields), and add an import for
SupportedCurrency from the shared types module; ensure all references in
useAssets/use-assets types and return types are updated to preserve nullability
and compile-time currency safety.
In `@packages/epics/src/treasury/hooks/use-user-assets.ts`:
- Around line 32-57: The AssetItem and UseAssetsReturn types are duplicated
elsewhere; extract their shape into a shared exported type (e.g., export type
AssetItem and export type UseAssetsReturn) in a common module and replace the
local declarations in use-user-assets.ts by importing those shared types; update
all other places that currently mirror these fields (server token metadata,
asset display props, etc.) to import and use the shared types so the new
currency/reference fields (referencePrice, referenceCurrency) and optional space
fields remain consistent across the codebase.
In `@packages/storage-postgres/src/schema/people.ts`:
- Line 19: The people.schema currently defines currency as text('currency')
allowing any string; add a database-level constraint so only
SUPPORTED_CURRENCIES are persisted—modify the schema definition for the people
table (the currency column) to use a DB enum type or a CHECK constraint
referencing the allowed currency codes (instead of text('currency')) so writes
that bypass app validation are rejected; update any migration/DDL for the people
table and ensure the enum/check uses the same SUPPORTED_CURRENCIES set and that
the column symbol named currency is altered/created accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa01ae66-e1bf-4a94-955e-f0f92400a769
📒 Files selected for processing (23)
apps/web/src/app/api/v1/people/[personSlug]/assets/route.tsapps/web/src/app/api/v1/spaces/[spaceSlug]/assets/route.tspackages/core/src/common/currency.tspackages/core/src/common/index.tspackages/core/src/common/server/get-fiat-conversion-rate.tspackages/core/src/common/server/index.tspackages/core/src/common/server/web3-rpc/get-token-meta.tspackages/core/src/governance/types.tspackages/core/src/people/server/queries.tspackages/core/src/people/types.tspackages/core/src/people/validation.tspackages/epics/src/people/components/edit-person-section.tsxpackages/epics/src/treasury/components/assets/asset-card.tsxpackages/epics/src/treasury/components/assets/assets-list.tsxpackages/epics/src/treasury/components/common/reference-currency-field.tsxpackages/epics/src/treasury/hooks/use-assets-section.tspackages/epics/src/treasury/hooks/use-assets.tspackages/epics/src/treasury/hooks/use-user-assets-section.tspackages/epics/src/treasury/hooks/use-user-assets.tspackages/storage-postgres/migrations/0041_rainy_green_goblin.sqlpackages/storage-postgres/migrations/meta/0041_snapshot.jsonpackages/storage-postgres/migrations/meta/_journal.jsonpackages/storage-postgres/src/schema/people.ts
2ccf329 to
92ca59d
Compare
ef1cf3a to
f1c540e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/epics/src/treasury/hooks/use-user-assets.ts (1)
32-50: Consider extractingAssetItemtype to a shared module.The
AssetItemtype (including the newreferencePriceandreferenceCurrencyfields) is now duplicated between this file anduse-assets.ts. Extracting it to a shared types module would improve maintainability and ensure consistency.Based on learnings: "DSanich prefers extracting repeated type declarations into shared modules for reusability and maintainability, particularly for token types that are used across multiple files in the codebase."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/treasury/hooks/use-user-assets.ts` around lines 32 - 50, Extract the duplicated AssetItem type into a shared types module and import it where needed: create a new exported type (e.g., AssetItem) in a common types file that includes icon, name, symbol, value, usdEqual, type, chartData: OneChartPoint[], transactions: TransactionCardProps[], closeUrl, slug, address, optional space, and the new referencePrice and referenceCurrency fields; then remove the inline AssetItem declaration from both use-user-assets.ts and use-assets.ts and update those files to import the shared AssetItem type (and ensure OneChartPoint and TransactionCardProps are imported or re-exported as needed).packages/core/src/people/server/queries.ts (1)
110-125: Currency field added correctly; consider extracting shared field selection.The
currencyfield is correctly included. However, this field selection duplicatesgetDefaultFields()(minustotal). Consider extracting a base field set to avoid maintaining two nearly-identical field lists.♻️ Optional: Extract shared base fields
+const getBaseFields = () => ({ + id: people.id, + slug: people.slug, + avatarUrl: people.avatarUrl, + description: people.description, + email: people.email, + location: people.location, + currency: people.currency, + name: people.name, + surname: people.surname, + nickname: people.nickname, + createdAt: people.createdAt, + updatedAt: people.updatedAt, + address: people.address, + leadImageUrl: people.leadImageUrl, +}); + export const getDefaultFields = () => { return { - id: people.id, - slug: people.slug, - avatarUrl: people.avatarUrl, - description: people.description, - email: people.email, - location: people.location, - currency: people.currency, - name: people.name, - surname: people.surname, - nickname: people.nickname, - createdAt: people.createdAt, - updatedAt: people.updatedAt, - address: people.address, - leadImageUrl: people.leadImageUrl, + ...getBaseFields(), total: sql<number>`cast(count(*) over() as integer)`, }; };Then use
getBaseFields()infindAllPeopleWithoutPagination.Based on learnings, DSanich prefers extracting repeated declarations into shared modules for reusability and maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/people/server/queries.ts` around lines 110 - 125, This selection in the people.select block duplicates getDefaultFields() (except total); extract the repeated field list into a shared helper (e.g., getBaseFields()) that returns the common field selection, then update findAllPeopleWithoutPagination to call getBaseFields() (and have getDefaultFields() reuse or extend getBaseFields() if needed); update references to the explicit list in people.select to use that helper so the base fields are maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/people/server/queries.ts`:
- Around line 110-125: This selection in the people.select block duplicates
getDefaultFields() (except total); extract the repeated field list into a shared
helper (e.g., getBaseFields()) that returns the common field selection, then
update findAllPeopleWithoutPagination to call getBaseFields() (and have
getDefaultFields() reuse or extend getBaseFields() if needed); update references
to the explicit list in people.select to use that helper so the base fields are
maintained in one place.
In `@packages/epics/src/treasury/hooks/use-user-assets.ts`:
- Around line 32-50: Extract the duplicated AssetItem type into a shared types
module and import it where needed: create a new exported type (e.g., AssetItem)
in a common types file that includes icon, name, symbol, value, usdEqual, type,
chartData: OneChartPoint[], transactions: TransactionCardProps[], closeUrl,
slug, address, optional space, and the new referencePrice and referenceCurrency
fields; then remove the inline AssetItem declaration from both
use-user-assets.ts and use-assets.ts and update those files to import the shared
AssetItem type (and ensure OneChartPoint and TransactionCardProps are imported
or re-exported as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 132bc0fc-00c5-4833-aa66-cbed6b671a05
📒 Files selected for processing (23)
apps/web/src/app/api/v1/people/[personSlug]/assets/route.tsapps/web/src/app/api/v1/spaces/[spaceSlug]/assets/route.tspackages/core/src/common/currency.tspackages/core/src/common/index.tspackages/core/src/common/server/get-fiat-conversion-rate.tspackages/core/src/common/server/index.tspackages/core/src/common/server/web3-rpc/get-token-meta.tspackages/core/src/governance/types.tspackages/core/src/people/server/queries.tspackages/core/src/people/types.tspackages/core/src/people/validation.tspackages/epics/src/people/components/edit-person-section.tsxpackages/epics/src/treasury/components/assets/asset-card.tsxpackages/epics/src/treasury/components/assets/assets-list.tsxpackages/epics/src/treasury/components/common/reference-currency-field.tsxpackages/epics/src/treasury/hooks/use-assets-section.tspackages/epics/src/treasury/hooks/use-assets.tspackages/epics/src/treasury/hooks/use-user-assets-section.tspackages/epics/src/treasury/hooks/use-user-assets.tspackages/storage-postgres/migrations/0041_rainy_green_goblin.sqlpackages/storage-postgres/migrations/meta/0041_snapshot.jsonpackages/storage-postgres/migrations/meta/_journal.jsonpackages/storage-postgres/src/schema/people.ts
✅ Files skipped from review due to trivial changes (10)
- packages/core/src/common/index.ts
- packages/core/src/common/server/index.ts
- packages/storage-postgres/migrations/0041_rainy_green_goblin.sql
- packages/core/src/people/types.ts
- packages/epics/src/treasury/components/assets/assets-list.tsx
- packages/epics/src/treasury/components/common/reference-currency-field.tsx
- packages/storage-postgres/src/schema/people.ts
- packages/storage-postgres/migrations/meta/_journal.json
- packages/core/src/common/currency.ts
- packages/storage-postgres/migrations/meta/0041_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/epics/src/treasury/hooks/use-assets-section.ts
- packages/core/src/common/server/web3-rpc/get-token-meta.ts
- packages/epics/src/people/components/edit-person-section.tsx
- packages/epics/src/treasury/components/assets/asset-card.tsx
- packages/core/src/people/validation.ts
- apps/web/src/app/api/v1/people/[personSlug]/assets/route.ts
- packages/epics/src/treasury/hooks/use-assets.ts
- packages/core/src/common/server/get-fiat-conversion-rate.ts
- packages/epics/src/treasury/hooks/use-user-assets-section.ts
- apps/web/src/app/api/v1/spaces/[spaceSlug]/assets/route.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy-preview.yml (1)
101-117:⚠️ Potential issue | 🟠 MajorPrevent blank branch DB envs from being deployed when migrations are not run.
Line 103-Line 116 reads
steps.create-branch.outputs.*even whencreate-branchis skipped; those values can be empty and still get pushed viavercel deploy --env ..., overriding runtime envs with blanks.💡 Proposed fix
- name: Build Project Artifacts - env: - BRANCH_DB_URL: ${{ steps.create-branch.outputs.db_url_with_pooler }} - BRANCH_DB_ANONYMOUS_URL: ${{ steps.create-branch.outputs.db_url_with_pooler }} - BRANCH_DB_AUTHENTICATED_URL: ${{ steps.create-branch.outputs.db_url }} run: | vercel env pull --environment=preview --token=${{ env.VERCEL_TOKEN }} vercel build --token=${{ env.VERCEL_TOKEN }} - name: Deploy Preview to Vercel id: deploy - env: - BRANCH_DB_URL: ${{ steps.create-branch.outputs.db_url_with_pooler }} - BRANCH_DB_ANONYMOUS_URL: ${{ steps.create-branch.outputs.db_url_with_pooler }} - BRANCH_DB_AUTHENTICATED_URL: ${{ steps.create-branch.outputs.db_url }} - run: echo preview_url=$(vercel deploy --prebuilt --token=${{ env.VERCEL_TOKEN }} --env BRANCH_DB_URL=${{ env.BRANCH_DB_URL }} --env BRANCH_DB_ANONYMOUS_URL=${{ env.BRANCH_DB_ANONYMOUS_URL }} --env BRANCH_DB_AUTHENTICATED_URL=${{ env.BRANCH_DB_AUTHENTICATED_URL }}) >> $GITHUB_OUTPUT + run: | + DEPLOY_ARGS=(--prebuilt --token=${{ env.VERCEL_TOKEN }}) + [[ -n "${BRANCH_DB_URL:-}" ]] && DEPLOY_ARGS+=(--env "BRANCH_DB_URL=${BRANCH_DB_URL}") + [[ -n "${BRANCH_DB_ANONYMOUS_URL:-}" ]] && DEPLOY_ARGS+=(--env "BRANCH_DB_ANONYMOUS_URL=${BRANCH_DB_ANONYMOUS_URL}") + [[ -n "${BRANCH_DB_AUTHENTICATED_URL:-}" ]] && DEPLOY_ARGS+=(--env "BRANCH_DB_AUTHENTICATED_URL=${BRANCH_DB_AUTHENTICATED_URL}") + echo "preview_url=$(vercel deploy "${DEPLOY_ARGS[@]}")" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-preview.yml around lines 101 - 117, The Deploy Preview step is passing empty branch DB outputs (steps.create-branch.outputs.*) into vercel deploy which can override runtime envs with blanks when create-branch is skipped; modify the Deploy Preview "run" to construct the --env arguments only when each output is non-empty (e.g., build a shell variable env_args and append "--env BRANCH_DB_URL=…", "--env BRANCH_DB_ANONYMOUS_URL=…", "--env BRANCH_DB_AUTHENTICATED_URL=…" only if the corresponding steps.create-branch.outputs.* value is not empty), then call vercel deploy with ${env_args}; alternatively gate the whole step with a condition referencing steps.create-branch.outcome or non-empty outputs so it doesn't pass blank values. Ensure you update the "Deploy Preview to Vercel" step that references BRANCH_DB_URL/ANONYMOUS/AUTHENTICATED to use the conditional env_args or step-level if.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/common/server/get-db.ts`:
- Around line 16-23: The resolveDbUrl function returns BRANCH_AUTHENTICATED
using the nullish coalescing operator which allows an empty string to slip
through and it also omits the BRANCH fallback; update resolveDbUrl to treat
empty strings as missing (use truthiness checks instead of ??) and mirror the
anonymous branch logic by falling back to BRANCH when BRANCH_AUTHENTICATED is
not usable, ensuring the final value satisfies the invariant check (refer to
resolveDbUrl, BRANCH_AUTHENTICATED, DEFAULT_AUTHENTICATED, BRANCH,
BRANCH_ANONYMOUS, DEFAULT_ANONYMOUS and the invariant validation).
---
Outside diff comments:
In @.github/workflows/deploy-preview.yml:
- Around line 101-117: The Deploy Preview step is passing empty branch DB
outputs (steps.create-branch.outputs.*) into vercel deploy which can override
runtime envs with blanks when create-branch is skipped; modify the Deploy
Preview "run" to construct the --env arguments only when each output is
non-empty (e.g., build a shell variable env_args and append "--env
BRANCH_DB_URL=…", "--env BRANCH_DB_ANONYMOUS_URL=…", "--env
BRANCH_DB_AUTHENTICATED_URL=…" only if the corresponding
steps.create-branch.outputs.* value is not empty), then call vercel deploy with
${env_args}; alternatively gate the whole step with a condition referencing
steps.create-branch.outcome or non-empty outputs so it doesn't pass blank
values. Ensure you update the "Deploy Preview to Vercel" step that references
BRANCH_DB_URL/ANONYMOUS/AUTHENTICATED to use the conditional env_args or
step-level if.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84996a4f-895e-4c56-9f73-33fb053075b6
📒 Files selected for processing (2)
.github/workflows/deploy-preview.ymlpackages/core/src/common/server/get-db.ts
ebf1a00 to
262626e
Compare
Summary by CodeRabbit
New Features
Chores