Improve POST /links endpoint performance
#2177
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This reverts commit 1c8001a.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance and reliability of the POST /links endpoint by refining link creation flows and error handling. Key changes include:
- Updating the sitemap importer to log valid and invalid links separately.
- Refactoring key generation functions (removing asynchronous duplicate checks) and introducing a retry mechanism.
- Enhancing bulk link creation to identify and separate duplicate links.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/scripts/sitemap-importer.ts | Updates logging to display separate validLinks/invalidLinks from bulkCreateLinks. |
| apps/web/lib/planetscale/index.ts | Removed redundant export for key existence check. |
| apps/web/lib/planetscale/get-random-key.ts | Removed duplicate key checking to delegate that responsibility; now returns a new random key directly. |
| apps/web/lib/api/links/utils/key-checks.ts | Removed dependency on checkIfKeyExists to streamline key validation. |
| apps/web/lib/api/links/update-link.ts, create-link.ts | Added try-catch blocks to wrap Prisma update/create calls with custom error messages. |
| apps/web/lib/api/links/create-link-with-key-retry.ts | New retry functionality for handling duplicate key (P2002) errors. |
| apps/web/lib/api/links/bulk-create-links.ts | Improved duplicate link detection and separation of valid and invalid links. |
| apps/web/lib/api/errors.ts | Simplified logging of error messages. |
| apps/web/app/api/links/* | Streamlined API routes to use the new key retry and bulk creation logic. |
Comments suppressed due to low confidence (2)
apps/web/lib/planetscale/get-random-key.ts:8
- Removing duplicate key checking from getRandomKey shifts the responsibility of handling duplicates to the caller. Please ensure that the new retry mechanism (in createLinkWithKeyRetry) reliably catches and resolves conflicts.
- const exists = await checkIfKeyExists({ domain, key });
apps/web/lib/api/links/bulk-create-links.ts:77
- [nitpick] The use of Array.find to map each duplicate link assumes that there is at most one occurrence per duplicate key. If duplicated entries in the incoming links array are possible, consider handling multiple occurrences explicitly.
link: links.find((l) => existingLink.shortLink === linkConstructorSimple({ domain: l.domain, key: l.key })) as ProcessedLinkProps,
WalkthroughThis update refactors link creation and update flows to improve error handling, duplicate detection, and key generation. It introduces a retry mechanism for random key generation, removes database existence checks for keys, and changes several API handlers to let errors propagate naturally. Return types and logging are updated to distinguish valid and invalid link creations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler
participant createLinkWithKeyRetry
participant createLink
participant getRandomKey
Client->>API_Handler: POST /api/links (with link data)
API_Handler->>createLinkWithKeyRetry: createLinkWithKeyRetry({ link, isRandomKey })
alt isRandomKey
loop up to maxRetries
createLinkWithKeyRetry->>createLink: createLink(link)
alt P2002 unique constraint error
createLinkWithKeyRetry->>getRandomKey: getRandomKey({ prefix, long })
createLinkWithKeyRetry->>createLink: createLink(link with new key)
else Other error
createLinkWithKeyRetry->>API_Handler: throw error
end
end
createLinkWithKeyRetry->>API_Handler: throw 409 if retries exhausted
else user-specified key
createLinkWithKeyRetry->>createLink: createLink(link)
alt P2002 unique constraint error
createLinkWithKeyRetry->>API_Handler: throw 409 conflict
else Other error
createLinkWithKeyRetry->>API_Handler: throw error
end
end
API_Handler->>Client: Return created link or error
sequenceDiagram
participant Client
participant API_BulkHandler
participant bulkCreateLinks
participant DB
Client->>API_BulkHandler: POST /api/links/bulk (with links)
API_BulkHandler->>bulkCreateLinks: bulkCreateLinks({ links })
bulkCreateLinks->>DB: Query for existing short links
alt Duplicates found
bulkCreateLinks->>API_BulkHandler: Return { validLinks, invalidLinks }
else No duplicates
bulkCreateLinks->>DB: Create new links
bulkCreateLinks->>API_BulkHandler: Return { validLinks, invalidLinks }
end
API_BulkHandler->>Client: Return valid and invalid link lists
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)apps/web/lib/api/links/bulk-create-links.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
apps/web/lib/api/links/bulk-create-links.ts (1)
18-30: 🛠️ Refactor suggestionSpecify an explicit return type for the new response shape
The function’s return signature changed from an array to an object but no explicit
Promise<…>type is declared.
This relies on inference and risks breaking downstream callers silently compiled toany, while callers that still expect an array will now fail at runtime instead of compile-time.+type BulkCreateLinksResult = { + validLinks: ProcessedLinkProps[]; + invalidLinks: { + error: string; + code: string; + link: ProcessedLinkProps; + }[]; +}; + -export async function bulkCreateLinks({ … }: { … }) { +export async function bulkCreateLinks({ … }: { … }): Promise<BulkCreateLinksResult> {
🧹 Nitpick comments (5)
apps/web/lib/api/links/update-link.ts (1)
170-170: Simplify unnecessary ternary expression.The static analysis tool correctly identified an unnecessary boolean literal in the conditional expression.
- webhooks: webhookIds ? true : false, + webhooks: !!webhookIds,Or more simply, if webhookIds is always a boolean context:
- webhooks: webhookIds ? true : false, + webhooks: Boolean(webhookIds),🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/lib/api/links/create-link.ts (1)
136-136: Simplify boolean ternary expression.The ternary operator is unnecessary when the result is already boolean.
-webhooks: webhookIds ? true : false, +webhooks: !!webhookIds,Or even simpler if webhookIds is expected to be truthy/falsy:
-webhooks: webhookIds ? true : false, +webhooks: Boolean(webhookIds),🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/lib/api/links/create-link-with-key-retry.ts (1)
48-54: Consider immutable link updates and type safety.The current implementation mutates the original link object, which could be unexpected behavior. Also, the bracket notation for accessing
prefixsuggests it might not be a direct property.-// Otherwise, generate a new random key and retry -const newKey = getRandomKey({ - prefix: link["prefix"], - long: link.domain === "loooooooo.ng", -}); - -link = { ...link, key: newKey }; +// Otherwise, generate a new random key and retry +const newKey = getRandomKey({ + prefix: link.prefix, + long: link.domain === "loooooooo.ng", +}); + +link = { ...link, key: newKey };This change assumes
prefixis a proper property ofProcessedLinkProps. If it's not, consider adding it to the type definition for better type safety.apps/web/lib/api/links/bulk-create-links.ts (2)
53-65: Remove thetakeclause – it risks missing duplicates
take: shortLinkToIndexMap.sizelimits the number of rows Prisma returns.
If two rows share the sameshortLink(unlikely but possible due to DB corruption or race), one duplicate might be skipped, leading to a false “valid” link that later violates a unique constraint.Simply omitting
takelets the DB return all matches and is still bounded byIN (...), so the query remains performant.
294-299: Minor: avoid repeatedtransformLinkallocations
transformLinkis called for every link even when it already returns aProcessedLinkProps.
If it’s an identity function for most fields, consider mapping once duringcreatedLinksDataconstruction to cut an extra loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web/app/api/links/[linkId]/route.ts(1 hunks)apps/web/app/api/links/bulk/route.ts(1 hunks)apps/web/app/api/links/random/route.ts(1 hunks)apps/web/app/api/links/route.ts(3 hunks)apps/web/app/api/links/upsert/route.ts(1 hunks)apps/web/lib/api/errors.ts(1 hunks)apps/web/lib/api/links/bulk-create-links.ts(3 hunks)apps/web/lib/api/links/create-link-with-key-retry.ts(1 hunks)apps/web/lib/api/links/create-link.ts(2 hunks)apps/web/lib/api/links/process-link.ts(1 hunks)apps/web/lib/api/links/update-link.ts(2 hunks)apps/web/lib/api/links/utils/key-checks.ts(0 hunks)apps/web/lib/planetscale/check-if-key-exists.ts(0 hunks)apps/web/lib/planetscale/get-random-key.ts(1 hunks)apps/web/lib/planetscale/index.ts(0 hunks)apps/web/scripts/sitemap-importer.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/web/lib/planetscale/index.ts
- apps/web/lib/api/links/utils/key-checks.ts
- apps/web/lib/planetscale/check-if-key-exists.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/lib/api/links/create-link.ts
[error] 136-136: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
apps/web/lib/api/links/update-link.ts
[error] 170-170: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
apps/web/lib/api/errors.ts (1)
125-125: LGTM: Cleaner error loggingRemoving the static prefix makes error logs more concise and readable while maintaining the essential error information.
apps/web/lib/api/links/process-link.ts (1)
257-260:✅ Verification successful
Verify that getRandomKey is now synchronous
The removal of
awaitaligns with the performance optimization to eliminate upfront database existence checks. This should improve endpoint performance by generating keys synchronously.
🏁 Script executed:
#!/bin/bash # Verify that getRandomKey function is now synchronous ast-grep --pattern 'export function getRandomKey($_) { $$$ }'Length of output: 978
Approved:
getRandomKeyis synchronousVerified that
getRandomKeyinapps/web/lib/planetscale/get-random-key.tsis defined as a regular (non-async) function, so removing theawaiton its call is correct.apps/web/app/api/links/random/route.ts (1)
20-22: LGTM: Consistent with getRandomKey synchronizationThe removal of
awaitis consistent with the performance optimization to make key generation synchronous.apps/web/app/api/links/[linkId]/route.ts (1)
172-191: LGTM: Simplified error handling architectureThe removal of the try-catch block allows errors to propagate naturally from
updateLink, which now handles Prisma constraint violations internally. This centralizes error handling and simplifies the control flow.apps/web/scripts/sitemap-importer.ts (1)
62-68: LGTM! Clean adaptation to the new bulkCreateLinks interface.The destructuring of the response into
validLinksandinvalidLinksarrays properly aligns with the updated API structure, and the enhanced logging provides better visibility into the bulk creation results.apps/web/app/api/links/bulk/route.ts (1)
248-257: Improved error handling for bulk creation results.The refactored response handling properly merges invalid links from
bulkCreateLinksinto the error list, ensuring that duplicates and validation failures detected during bulk creation are correctly reported to the client.apps/web/app/api/links/route.ts (1)
104-104: LGTM! Clean extraction of key for retry logic.The explicit extraction of the
keyvariable improves readability for the subsequentcreateLinkWithKeyRetrycall.apps/web/lib/api/links/update-link.ts (1)
85-187: Excellent centralized error handling for database operations.The explicit error handling for Prisma operations is well-structured:
- Specific handling for unique constraint violations (P2002) with appropriate "conflict" error
- Generic fallback for other database errors with "unprocessable_entity"
This aligns perfectly with the PR objective of letting errors propagate naturally while providing meaningful error codes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/app/api/links/upsert/route.ts (1)
146-163: LGTM: Simplified error handling.Removing the try-catch blocks and letting errors propagate naturally from the underlying
updateLinkfunction is a good simplification that aligns with the improved error handling strategy.apps/web/lib/planetscale/get-random-key.ts (1)
3-17: LGTM: Performance improvement through simplification.The conversion from async to sync function and removal of existence checks is an excellent performance optimization. This eliminates unnecessary database roundtrips during key generation, relying instead on the retry mechanism in
createLinkWithKeyRetryto handle conflicts.The prefix handling logic remains correct and the function is now more efficient.
apps/web/lib/api/links/create-link.ts (1)
55-153: LGTM: Well-implemented error handling for retry mechanism.The try-catch block properly handles Prisma unique constraint violations (P2002) and converts them to
DubApiErrorwith appropriate codes. This enables the retry mechanism increateLinkWithKeyRetryto detect conflicts and retry with new keys.The error handling is comprehensive and follows good practices by:
- Specifically catching known Prisma errors
- Providing meaningful error messages
- Falling back to generic error handling for unexpected cases
🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/lib/api/links/create-link-with-key-retry.ts (2)
23-59: LGTM: Well-implemented retry mechanism with good error handling.The retry logic correctly handles unique constraint violations and distinguishes between user-specified and randomly generated keys. The implementation includes proper safeguards:
- Only retries for randomly generated keys
- Respects max retry limits
- Provides meaningful error messages
- Handles unexpected errors appropriately
61-62: Good defensive programming.The fallback error case provides good protection against unexpected control flow, even though it should theoretically never be reached due to the while loop condition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/tests/links/bulk-create-link.test.ts (1)
125-134: Thorough error response validation.The error assertion comprehensively validates the expected response structure for duplicate key conflicts. The test checks:
- Error code and message match expected values
- Link details are preserved in the error response
- Proper error categorization as "conflict"
One suggestion for future enhancement: Consider extracting the error message "Duplicate key: This short link already exists." into a constant to improve maintainability if this error format is used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/tests/links/bulk-create-link.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/tests/links/bulk-create-link.test.ts (1)
apps/web/tests/utils/helpers.ts (1)
randomId(4-4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
apps/web/tests/links/bulk-create-link.test.ts (4)
71-84: LGTM! Well-structured test setup for duplicate key scenario.The test correctly creates an initial link with a specific key that will be reused to test duplicate detection. The approach of extracting the key for later use is clean and follows good testing practices.
85-103: Comprehensive test coverage for bulk link creation scenarios.The test data effectively covers all three important scenarios:
- Link without key (should auto-generate)
- Link with unique key (should succeed)
- Link with duplicate key (should fail)
This aligns well with the PR objective of improving duplicate detection handling.
111-115: Proper cleanup handling for the enhanced test.The cleanup correctly includes both the initially created link and all bulk-created links, preventing test pollution and ensuring proper resource management.
119-124: Correctly adjusted verification logic.The verification now only checks the first two successful links since the third is expected to be an error response. The use of
slice(0, 2)is appropriate and maintains the existing verification logic for successful cases.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor