fix: check import quota before importing bookmarks #2232
Conversation
Add quota validation before bookmark import to prevent users from exceeding their bookmark limits. The implementation includes: - New QuotaService.canImportBookmarks() method to check if user can import N bookmarks - New tRPC checkImportQuota procedure for client-side quota validation - Updated useBookmarkImport hook to parse files and check quota before import - Added error banner in ImportExport component to display quota errors - Optimized file parsing to avoid reading the file twice The quota check displays remaining bookmarks and provides clear error messages when the import would exceed the user's quota.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds quota validation to bookmark imports: a parse step counts bookmarks, checks against user quota, surfaces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const remaining = | ||
| quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used; | ||
|
|
||
| if (remaining < bookmarkCount) { | ||
| const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`; |
There was a problem hiding this comment.
The new quota guard compares the raw number of parsed rows to the remaining quota and throws when bookmarkCount exceeds remaining. This treats every entry in the file as consuming quota even though the import pipeline later deduplicates and counts alreadyExisted items separately, so a user near their limit is now prevented from importing a file that mostly contains bookmarks already stored (e.g., 4 existing + 1 new with 1 slot left will be rejected). This misestimates quota usage and unnecessarily blocks legitimate imports.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/components/settings/ImportExport.tsx(3 hunks)apps/web/lib/hooks/useBookmarkImport.ts(5 hunks)packages/shared/import-export/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/shared/import-export/index.tsapps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/shared/import-export/index.tsapps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/shared/import-export/index.tsapps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
packages/shared/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize shared code and types in the
packages/shareddirectory for use across packages
Files:
packages/shared/import-export/index.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
🧬 Code graph analysis (2)
apps/web/components/settings/ImportExport.tsx (2)
apps/web/lib/hooks/useBookmarkImport.ts (1)
useBookmarkImport(35-202)apps/web/components/ui/alert.tsx (3)
Alert(60-60)AlertTitle(60-60)AlertDescription(60-60)
apps/web/lib/hooks/useBookmarkImport.ts (2)
packages/shared/import-export/index.ts (1)
parseImportFile(4-4)packages/shared/import-export/parsers.ts (1)
parseImportFile(336-365)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: typecheck
🔇 Additional comments (7)
packages/shared/import-export/index.ts (1)
4-4: LGTM!Clean re-export that enables the quota-aware import flow by exposing the parser function for pre-parsing files before quota validation.
apps/web/components/settings/ImportExport.tsx (2)
4-4: LGTM!Appropriate imports for the quota error alert UI components.
Also applies to: 19-19
135-136: LGTM!Correctly consumes the new quota error state from the hook.
apps/web/lib/hooks/useBookmarkImport.ts (4)
16-16: LGTM!Appropriate imports for TRPC client access and file parsing functionality.
Also applies to: 21-21
41-41: LGTM!Correctly initializes quota error state and TRPC utilities for quota validation.
Also applies to: 43-43
197-198: LGTM!Correctly exposes the new quota error state and clear function in the hook's public API.
156-161: LGTM! Clever optimization to avoid re-parsing the file by providing the already-parsed bookmarks via a custom parser. This prevents redundant parsing work.
| {quotaError && ( | ||
| <Alert variant="destructive" className="relative"> | ||
| <AlertCircle className="h-4 w-4" /> | ||
| <AlertTitle>Import Quota Exceeded</AlertTitle> | ||
| <AlertDescription>{quotaError}</AlertDescription> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="absolute right-2 top-2 h-6 w-6 p-0" | ||
| onClick={clearQuotaError} | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </Button> | ||
| </Alert> | ||
| )} |
There was a problem hiding this comment.
Hardcoded English text breaks i18n consistency.
The Alert title "Import Quota Exceeded" is hardcoded in English, while the rest of the component uses t() for internationalization.
Apply this diff to make the text translatable:
- <AlertTitle>Import Quota Exceeded</AlertTitle>
+ <AlertTitle>{t("settings.import.quota_exceeded_title")}</AlertTitle>Additionally, the quotaError message from the hook is also in English. Consider whether the error message construction should also use i18n for consistency across the application.
Optional: Consider relative positioning for the dismiss button.
The absolute positioning at right-2 top-2 might overlap with the alert content on smaller screens or with longer error messages. Consider using a flex layout instead for more robust responsive behavior.
| // Clear any previous quota error | ||
| setQuotaError(null); | ||
|
|
||
| // First, parse the file to count bookmarks | ||
| const textContent = await file.text(); | ||
| const parsedBookmarks = parseImportFile(source, textContent); | ||
| const bookmarkCount = parsedBookmarks.length; | ||
|
|
||
| // Check quota before proceeding | ||
| if (bookmarkCount > 0) { | ||
| const quotaUsage = | ||
| await apiUtils.client.subscriptions.getQuotaUsage.query(); | ||
|
|
||
| if (!quotaUsage.bookmarks.unlimited) { | ||
| const remaining = | ||
| quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used; | ||
|
|
||
| if (remaining < bookmarkCount) { | ||
| const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`; | ||
| setQuotaError(errorMsg); | ||
| throw new Error(errorMsg); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate error messaging: both Alert and toast are shown.
When quota is exceeded, the error is both stored in state (line 77) and thrown (line 78). The thrown error triggers onError (line 186), which displays a toast. This results in the user seeing both the persistent Alert (rendered in ImportExport.tsx) and a temporary toast with the same message.
Consider one of these approaches:
Option 1: Don't throw after setting quotaError (recommended)
if (remaining < bookmarkCount) {
const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`;
setQuotaError(errorMsg);
- throw new Error(errorMsg);
+ return { counts: { total: 0, successes: 0, failures: 0, alreadyExisted: 0 } };
}Option 2: Only throw without setting state
if (remaining < bookmarkCount) {
const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`;
- setQuotaError(errorMsg);
throw new Error(errorMsg);
}Then remove the Alert UI since toast will handle it.
Non-null assertion could cause runtime error.
Line 73 uses quotaUsage.bookmarks.quota! assuming quota is always defined when unlimited is false. Verify this is guaranteed by the API contract.
Consider adding a runtime check:
if (!quotaUsage.bookmarks.unlimited) {
+ if (quotaUsage.bookmarks.quota === null || quotaUsage.bookmarks.quota === undefined) {
+ throw new Error("Quota information unavailable");
+ }
const remaining =
quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used;Or use nullish coalescing if there's a reasonable default:
- const remaining = quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used;
+ const remaining = (quotaUsage.bookmarks.quota ?? 0) - quotaUsage.bookmarks.used;Consider adding loading state during file parsing.
Lines 62-63 parse the entire file synchronously (from the user's perspective). For large import files, this could cause the UI to appear unresponsive. Consider showing a loading indicator or moving this to a Web Worker.
📝 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.
| // Clear any previous quota error | |
| setQuotaError(null); | |
| // First, parse the file to count bookmarks | |
| const textContent = await file.text(); | |
| const parsedBookmarks = parseImportFile(source, textContent); | |
| const bookmarkCount = parsedBookmarks.length; | |
| // Check quota before proceeding | |
| if (bookmarkCount > 0) { | |
| const quotaUsage = | |
| await apiUtils.client.subscriptions.getQuotaUsage.query(); | |
| if (!quotaUsage.bookmarks.unlimited) { | |
| const remaining = | |
| quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used; | |
| if (remaining < bookmarkCount) { | |
| const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`; | |
| setQuotaError(errorMsg); | |
| throw new Error(errorMsg); | |
| } | |
| } | |
| } | |
| // Clear any previous quota error | |
| setQuotaError(null); | |
| // First, parse the file to count bookmarks | |
| const textContent = await file.text(); | |
| const parsedBookmarks = parseImportFile(source, textContent); | |
| const bookmarkCount = parsedBookmarks.length; | |
| // Check quota before proceeding | |
| if (bookmarkCount > 0) { | |
| const quotaUsage = | |
| await apiUtils.client.subscriptions.getQuotaUsage.query(); | |
| if (!quotaUsage.bookmarks.unlimited) { | |
| const remaining = | |
| quotaUsage.bookmarks.quota! - quotaUsage.bookmarks.used; | |
| if (remaining < bookmarkCount) { | |
| const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`; | |
| setQuotaError(errorMsg); | |
| return { counts: { total: 0, successes: 0, failures: 0, alreadyExisted: 0 } }; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/lib/hooks/useBookmarkImport.ts around lines 58 to 81, when quota is
exceeded the code both sets quotaError state and throws an Error which leads to
duplicate UI messages (persistent Alert + toast); remove the throw after calling
setQuotaError so only the Alert is shown (or alternatively remove setting state
and only throw if you prefer toast-only behavior), and replace the non-null
assertion on quota (quota!) with a runtime check that handles missing quota
(e.g., bail out or treat as zero/unlimited based on API contract) to avoid
potential runtime errors; optionally, add a brief loading state while parsing
the file or move parsing to a worker to keep the UI responsive for large
imports.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/web/lib/hooks/useBookmarkImport.ts (3)
61-64: Consider showing a loading state during file parsing.For large import files,
await file.text()andparseImportFile()can take noticeable time and make the UI appear unresponsive. Consider adding a loading indicator or moving parsing to a Web Worker.
78-81: Remove the throw to avoid duplicate error messages.Setting
quotaError(line 80) displays a persistent Alert in the UI, while throwing the error (line 81) triggersonError(line 189), which shows a toast. This results in users seeing the same error message twice.Apply this diff to show only the Alert:
if (remaining < bookmarkCount) { const errorMsg = `Cannot import ${bookmarkCount} bookmarks. You have ${remaining} bookmark${remaining === 1 ? "" : "s"} remaining in your quota of ${quotaUsage.bookmarks.quota}.`; setQuotaError(errorMsg); - throw new Error(errorMsg); + return { counts: { total: 0, successes: 0, failures: 0, alreadyExisted: 0 } }; }
64-84: Quota check over-estimates usage by counting already-existing bookmarks.The quota validation compares the raw count of parsed bookmarks (
bookmarkCountat line 64) against remaining quota. However, the import pipeline later deduplicates bookmarks, andalreadyExisteditems (line 175) don't consume additional quota. This means users near their quota limit are incorrectly blocked from importing files that contain mostly bookmarks they already have.For example, a user with 1 slot remaining would be blocked from importing a file with 5 bookmarks if 4 of them already exist, even though the import would only add 1 new bookmark.
Consider one of these approaches:
Option 1: Move quota check after deduplication (recommended)
This requires restructuring to identify duplicates before creating bookmarks. The import logic would need to:
- Parse bookmarks
- Query existing bookmarks by URL/content hash
- Calculate actual new bookmarks
- Check quota against new count
- Proceed with import
Option 2: Document this as a known limitation
If fixing this is too complex for the current PR, clearly document that the quota check is conservative and may block imports that would succeed.
apps/web/components/settings/ImportExport.tsx (1)
140-146: Add a dismiss button to allow users to close the alert.The alert appears when
quotaErroris set but provides no way for users to dismiss it. This creates a poor UX as the error message persists on screen even after the user has read it.Apply this diff to add a dismiss button:
- {quotaError && ( - <Alert variant="destructive" className="relative"> - <AlertCircle className="h-4 w-4" /> - <AlertTitle>Import Quota Exceeded</AlertTitle> - <AlertDescription>{quotaError}</AlertDescription> - </Alert> - )} + {quotaError && ( + <Alert variant="destructive" className="relative"> + <AlertCircle className="h-4 w-4" /> + <AlertTitle>{t("settings.import.quota_exceeded_title")}</AlertTitle> + <AlertDescription>{quotaError}</AlertDescription> + <button + onClick={clearQuotaError} + className="absolute right-2 top-2 rounded-md p-1 hover:bg-destructive/20" + aria-label="Dismiss" + > + <X className="h-4 w-4" /> + </button> + </Alert> + )}Note: This also addresses the i18n issue flagged in a previous review by using
t()for the title. You'll need to add the translation keysettings.import.quota_exceeded_titleand importXfromlucide-react.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/settings/ImportExport.tsx(3 hunks)apps/web/lib/hooks/useBookmarkImport.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
apps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/settings/ImportExport.tsxapps/web/lib/hooks/useBookmarkImport.ts
🧬 Code graph analysis (1)
apps/web/lib/hooks/useBookmarkImport.ts (2)
packages/shared/import-export/index.ts (1)
parseImportFile(4-4)packages/shared/import-export/parsers.ts (1)
parseImportFile(336-365)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: typecheck
🔇 Additional comments (5)
apps/web/components/settings/ImportExport.tsx (1)
4-4: LGTM!The new Alert-related imports are correctly used in the quota error UI.
Also applies to: 19-19
apps/web/lib/hooks/useBookmarkImport.ts (4)
16-16: LGTM!The new imports and utilities are correctly set up to support quota checking before import.
Also applies to: 21-21, 43-43
41-41: LGTM!The
quotaErrorstate is properly initialized and integrated into the hook's flow.
159-164: LGTM!The custom parser optimization correctly reuses the already-parsed bookmarks, avoiding redundant parsing.
200-201: LGTM!The hook correctly exposes
quotaErrorandclearQuotaErrorto enable quota error handling in the UI.
| const { importProgress, quotaError, runUploadBookmarkFile } = | ||
| useBookmarkImport(); |
There was a problem hiding this comment.
Destructure clearQuotaError to enable dismissing the alert.
The hook provides clearQuotaError (see line 201 in useBookmarkImport.ts), but it's not destructured here. This means users cannot dismiss the quota error alert once it appears, forcing them to refresh the page or attempt another import.
Apply this diff to add dismiss functionality:
- const { importProgress, quotaError, runUploadBookmarkFile } =
- useBookmarkImport();
+ const { importProgress, quotaError, clearQuotaError, runUploadBookmarkFile } =
+ useBookmarkImport();📝 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.
| const { importProgress, quotaError, runUploadBookmarkFile } = | |
| useBookmarkImport(); | |
| const { importProgress, quotaError, clearQuotaError, runUploadBookmarkFile } = | |
| useBookmarkImport(); |
🤖 Prompt for AI Agents
In apps/web/components/settings/ImportExport.tsx around lines 135-136, the
useBookmarkImport hook return value is missing the clearQuotaError function, so
the quota error alert cannot be dismissed; update the destructuring to include
clearQuotaError from useBookmarkImport and wire that function to the alert
dismissal handler (e.g., onClose or dismiss action) so users can clear the quota
error without refreshing the page.
No description provided.