feat: add db search to externalId#3315
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis change introduces asynchronous identity searching to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExternalIdField
participant useSearchIdentities
participant TRPC_Server
User->>ExternalIdField: Types in search input
ExternalIdField->>useSearchIdentities: Passes search query
useSearchIdentities->>TRPC_Server: searchIdentities(query)
TRPC_Server-->>useSearchIdentities: Returns matching identities
useSearchIdentities-->>ExternalIdField: Provides searchResults, isSearching
ExternalIdField-->>User: Updates combobox options
User->>ExternalIdField: Selects or creates identity
ExternalIdField->>ParentComponent: onChange(identityId, externalId)
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@chronark This is for fireworks, since they have 12321312312321 identities its impossible for them to find it without a db call. We can merge this after ownerID pr |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
apps/dashboard/lib/trpc/routers/index.ts (1)
26-27: Import order nit – optionalThe new
searchIdentitiesimport is inserted mid-block, breaking the existing (mostly) alpha order of the identity‐related imports above/below. Not critical but keeping a consistent order eases merge conflicts.-import { queryIdentities } from "./identity/query"; -import { searchIdentities } from "./identity/search"; +import { queryIdentities } from "./identity/query"; +import { searchIdentities } from "./identity/search";apps/dashboard/lib/trpc/routers/key/create.ts (1)
50-52: Null-coalesce nullable columns before insert
identityIdandownerIdare optional in the incoming payload.
Passingundefinedinstead ofnullmay cause the ORM to omit the column or break when the DB column is declaredNULL DEFAULT NULL.- identityId: input.identityId, - ownerId: input.externalId, + identityId: input.identityId ?? null, + ownerId: input.externalId ?? null,Keeping the assignment explicit also makes the intent clearer.
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
198-199: Use?? nullwhen persisting optional fieldsFor consistency with the V1 path (and to avoid accidentally writing
undefined), coalesce the two assignments:- identityId: input.identity.id ?? null, - ownerId: input.identity.externalId, + identityId: input.identity.id ?? null, + ownerId: input.identity.externalId ?? null,apps/dashboard/lib/trpc/routers/identity/search.ts (2)
31-36: Remove redundant query validationThis validation is redundant as the input schema already validates that the query is non-empty through
.min(1)and the whitespace refinement.- if (!query.trim()) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Search query cannot be empty", - }); - }
60-68: Remove unnecessary data transformationThe transformation logic is redundant as the data already matches the expected format. The null coercion for
updatedAtcan be handled by the schema validation.- const transformedIdentities = identitiesQuery.map((identity) => ({ - id: identity.id, - externalId: identity.externalId, - workspaceId: identity.workspaceId, - environment: identity.environment, - meta: identity.meta, - createdAt: identity.createdAt, - updatedAt: identity.updatedAt ? identity.updatedAt : null, - })); - return { - identities: transformedIdentities, + identities: identitiesQuery, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx(7 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/use-search-identities.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-create-identity.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-fetch-identities/index.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/components/batch-edit-external-id.tsx(4 hunks)apps/dashboard/lib/trpc/routers/identity/query.ts(1 hunks)apps/dashboard/lib/trpc/routers/identity/search.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
🧬 Code Graph Analysis (4)
apps/dashboard/lib/trpc/routers/identity/search.ts (3)
apps/dashboard/lib/trpc/routers/identity/query.ts (1)
IdentityResponseSchema(11-19)apps/dashboard/lib/trpc/trpc.ts (3)
t(7-7)requireWorkspace(22-35)withRatelimit(112-127)apps/dashboard/lib/db.ts (1)
db(5-26)
apps/dashboard/lib/trpc/routers/index.ts (1)
apps/dashboard/lib/trpc/routers/identity/search.ts (1)
searchIdentities(13-81)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)
ExternalIdField(18-223)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/use-search-identities.tsx (1)
apps/dashboard/lib/trpc/server.ts (1)
trpc(7-14)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-fetch-identities/index.ts (1)
7-7: Default page size reduced – double-check downstream expectationsDropping the default from 50 → 10 dramatically increases the number of network round-trips during infinite scroll. Please verify that:
- UI/UX remains acceptable (scroll jitter, loading spinners, etc.).
- Any call-sites that relied on the previous implicit “first 50” behaviour consciously override the new default when necessary.
If a majority of screens still want 50, consider passing the limit explicitly from callers instead of changing the default here.
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-create-identity.ts (1)
16-18: 👍 Added cache-bust foridentity.searchGood catch adding
trpcUtils.identity.search.invalidate()so search results reflect the newly created identity without a hard refresh.apps/dashboard/lib/trpc/routers/index.ts (1)
230-231: Router entry added but permissions unchanged
identity.searchis now publicly exposed the same way asidentity.query. Ifquerypreviously required a certain permission scope/rate-limit weight, make suresearchis covered identically; otherwise consumers could bypass quota by callingsearchinstead of paginatedquery.apps/dashboard/lib/trpc/routers/identity/query.ts (1)
11-19: Schema re-export looks safeExporting
IdentityResponseSchemaimproves reuse and typing; no functional changes introduced.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx (1)
40-42: Possible backend contract mismatch forkeyIds
keyIdsis passed as a string but the batch edit hook likely expectsstring[](consistent with the multi-selection component). Confirm the mutation signature to avoid runtime validation errors.- keyIds: keyDetails.id, + keyIds: [keyDetails.id],apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/components/batch-edit-external-id.tsx (1)
22-22: LGTM! Consistent handling of identityId and externalIdThe component correctly manages both
selectedIdentityIdandselectedExternalIdstates, properly passing them together in mutation payloads and handling the updatedonChangecallback signature.Also applies to: 36-36, 62-62, 147-150
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/use-search-identities.tsx (1)
1-34: Well-implemented debounced search hookThe hook correctly implements debouncing with proper cleanup, memoizes results to avoid unnecessary recalculations, and provides accurate loading states. The 30-second cache time is reasonable for search results.
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (2)
25-26: Excellent UX improvements with smooth transitionsThe implementation of React transitions and conditional loading states provides a smooth user experience during search operations. The opacity transitions and "Searching..." indicator clearly communicate the loading state to users.
Also applies to: 109-110, 121-124, 134-152
130-131: Proper handling of identity selectionThe code correctly finds the identity by ID and safely handles the case where no identity is found, passing null values for both identityId and externalId.
...uthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx
Show resolved
Hide resolved
...uthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts
Show resolved
Hide resolved
|
this one has a ton of rabbit comments and merge conflicts, can you address/close the comments first? |
|
lets go pr by pr. you seen any issues in roles PR except that coderabbit comment? |
|
let's merge this last. i'll check it and test it myself again |
|
I found some bugs myself actually, but it should be good to go now |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx(6 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-fetch-identities/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.310Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/index.ts (1)
39-39: Clean integration of search functionality.The import and route mapping follow the established patterns in the codebase and properly integrate the new search capability into the identity router.
Also applies to: 272-272
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/hooks/use-fetch-identities/index.ts (1)
7-8: Good optimization for search-enabled workflow.Reducing the default fetch limit from 50 to 10 makes sense when search functionality is available, as users can find specific identities without loading large pages.
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)
142-142: Well-implemented loading states and UX enhancements.The loading state management effectively handles both initial loading and search states, providing clear feedback to users. The conditional loading indicators and disabled states create a smooth user experience.
Also applies to: 218-223, 237-246
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Outdated
Show resolved
Hide resolved
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Outdated
Show resolved
Hide resolved
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (3)
42-57: Consider simplifying the data merging logic.The current logic for combining identities from different sources is complex and could be error-prone. Consider extracting this logic into a custom hook or utility function for better testability and maintainability.
72-83: Avoid creating mock identity objects with placeholder data.Creating a mock identity with empty strings for
workspaceId,environment, and arbitrary timestamps could lead to unexpected behavior or data inconsistency. Consider refactoring to either ensure current identity has all required fields or handle the missing current identity case more explicitly.
240-241: Resolve type safety issue instead of suppressing it.The
@ts-expect-errorcomment indicates a type mismatch that should be addressed rather than suppressed, especially since it mentions being "fixed in following PRs."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.310Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
🔇 Additional comments (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx (1)
154-156: Verify the onChange behavior matches expectations.The
onChangehandler only updates the search value but doesn't trigger the identity selection. Ensure this is the intended behavior and that selection only happens through theonSelecthandler.Based on the retrieved learning, this pattern is intentional where the Controller is bound to "identityId" while "externalId" is managed separately. However, please verify that this search-only behavior in onChange doesn't interfere with form validation or user expectations.
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Show resolved
Hide resolved
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Show resolved
Hide resolved
...shboard/app/(app)/apis/[apiId]/_components/create-key/components/external-id-field/index.tsx
Outdated
Show resolved
Hide resolved
ogzhanolguncu
changed the title
ogzhanolguncu
force-pushed
the
add-db-search-to-externalid
branch
from
29f7520 to
72a3298
Compare
|
@mcstepp can you approve again? I had to fix the build issue |
3 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Test: Try to search for something in the externalId dropdown that hasn't been loaded yet. If it's not in the memory dropdown should make a rpc call.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
API Updates