feat: separate manually added bookmarks from RSS subscriptions in UI and rules#2549
Open
VedantMadane wants to merge 5 commits intokarakeep-app:mainfrom
Open
feat: separate manually added bookmarks from RSS subscriptions in UI and rules#2549VedantMadane wants to merge 5 commits intokarakeep-app:mainfrom
VedantMadane wants to merge 5 commits intokarakeep-app:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds bookmark source awareness across the app: UI shows source indicators, the rule system gains a new bookmarkSourceIs condition with types and translations, and the crawler worker adds richer asset tracking and cleanup on failures. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx`:
- Around line 99-105: The tooltip always uses t("common.bookmark_sources.web")
for all manualSources, causing extension/mobile/cli to be labeled "Web"; update
the title logic in BookmarkLayoutAdaptingCard so it derives the correct
translation key from bookmark.source (e.g. map or compute
`common.bookmark_sources.${bookmark.source}` or use a small switch) instead of
hardcoding "web", and fall back to a generic label if the source key is missing;
adjust the title prop on the div where manualSources is checked to use that
dynamic translation.
- Around line 414-416: SourceIndicator is positioned in the top-right where
owner and hover action overlays also render, causing overlap; in
BookmarkLayoutAdaptingCard change its wrapper positioning (the div containing
<SourceIndicator bookmark={bookmark} />) to a non-conflicting corner or offset
(for example replace "absolute right-2 top-2 z-40" with "absolute left-2 top-2
z-40" or "absolute right-8 top-2 z-40") so it no longer overlaps owner/hover
indicators, and run/update any visual/snapshot tests affected by the layout
change.
In `@apps/workers/workers/crawlerWorker.ts`:
- Around line 1559-1619: The catch block unconditionally deletes
downloadedAssetId even if the DB transaction committed and the asset is now
live; modify handleAsAssetBookmark so cleanup only runs when the transaction did
not commit: after calling downloadAndStoreFile set downloadedAssetId, introduce
a boolean like transactionCommitted (default false), set it to true at the end
of the db.transaction callback (after updateAsset/insert/update/delete), then
call AssetPreprocessingQueue.enqueue; in the catch, only call
silentDeleteAsset(userId, downloadedAssetId) if !transactionCommitted (i.e., the
DB changes never committed). Reference symbols: downloadedAssetId,
downloadAndStoreFile, db.transaction, AssetPreprocessingQueue.enqueue,
silentDeleteAsset.
- Around line 1836-1848: The code races storeScreenshot/storePdf against
abortPromise but doesn’t cancel the underlying storage calls, which can complete
and create assets that never get tracked; modify the storage flow so storage is
abortable and/or cleaned up: update storeScreenshot and storePdf to accept an
AbortSignal (or return a handle) and have them abort any in-flight upload when
abortSignal is triggered, or after the storage promise resolves check
abortSignal.aborted and if aborted call the corresponding cleanup/delete
function for the returned asset before returning/adding to newAssetIds; ensure
you only push screenshotAssetInfo.assetId / pdfAssetInfo.assetId into
newAssetIds after confirming the operation wasn’t aborted (use
abortSignal.throwIfAborted or signal.aborted) and wire abortSignal into the
Promise.race path so no orphaned assets remain.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsxapps/web/components/dashboard/rules/RuleEngineConditionBuilder.tsxapps/web/lib/i18n/locales/en/translation.jsonapps/workers/workers/crawlerWorker.tspackages/shared/types/rules.tspackages/trpc/lib/ruleEngine.ts
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+1559
to
+1619
| let downloadedAssetId: string | undefined; | ||
| try { | ||
| const downloaded = await downloadAndStoreFile( | ||
| url, | ||
| userId, | ||
| jobId, | ||
| assetType, | ||
| abortSignal, | ||
| ); | ||
| if (!downloaded) { | ||
| return; | ||
| } | ||
| downloadedAssetId = downloaded.assetId; | ||
| const fileName = path.basename(new URL(url).pathname); | ||
| await db.transaction(async (trx) => { | ||
| await updateAsset( | ||
| undefined, | ||
| { | ||
| id: downloaded.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.BOOKMARK_ASSET, | ||
| contentType: downloaded.contentType, | ||
| size: downloaded.size, | ||
| fileName, | ||
| }, | ||
| trx, | ||
| ); | ||
| await trx.insert(bookmarkAssets).values({ | ||
| id: bookmarkId, | ||
| assetType, | ||
| assetId: downloaded.assetId, | ||
| content: null, | ||
| fileName, | ||
| sourceUrl: url, | ||
| }); | ||
| // Switch the type of the bookmark from LINK to ASSET | ||
| await trx | ||
| .update(bookmarks) | ||
| .set({ type: BookmarkTypes.ASSET }) | ||
| .where(eq(bookmarks.id, bookmarkId)); | ||
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | ||
| }); | ||
| await AssetPreprocessingQueue.enqueue( | ||
| { | ||
| id: downloaded.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.BOOKMARK_ASSET, | ||
| contentType: downloaded.contentType, | ||
| size: downloaded.size, | ||
| fileName, | ||
| fixMode: false, | ||
| }, | ||
| { | ||
| groupId: userId, | ||
| }, | ||
| trx, | ||
| ); | ||
| await trx.insert(bookmarkAssets).values({ | ||
| id: bookmarkId, | ||
| assetType, | ||
| assetId: downloaded.assetId, | ||
| content: null, | ||
| fileName, | ||
| sourceUrl: url, | ||
| }); | ||
| // Switch the type of the bookmark from LINK to ASSET | ||
| await trx | ||
| .update(bookmarks) | ||
| .set({ type: BookmarkTypes.ASSET }) | ||
| .where(eq(bookmarks.id, bookmarkId)); | ||
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | ||
| }); | ||
| await AssetPreprocessingQueue.enqueue( | ||
| { | ||
| bookmarkId, | ||
| fixMode: false, | ||
| }, | ||
| { | ||
| groupId: userId, | ||
| }, | ||
| ); | ||
| } catch (error) { | ||
| if (downloadedAssetId) { | ||
| logger.error( | ||
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | ||
| ); | ||
| await silentDeleteAsset(userId, downloadedAssetId); | ||
| } | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
Post-commit enqueue failure can delete a committed asset.
downloadedAssetId cleanup in the catch block runs even when the DB transaction already committed (Line 1573-1601). If enqueue fails afterward, this deletes live data and leaves the converted bookmark inconsistent.
🔧 Suggested fix
- let downloadedAssetId: string | undefined;
+ let downloadedAssetId: string | undefined;
+ let conversionCommitted = false;
try {
const downloaded = await downloadAndStoreFile(
url,
userId,
jobId,
assetType,
abortSignal,
);
if (!downloaded) {
return;
}
downloadedAssetId = downloaded.assetId;
const fileName = path.basename(new URL(url).pathname);
await db.transaction(async (trx) => {
await updateAsset(
undefined,
{
id: downloaded.assetId,
bookmarkId,
userId,
assetType: AssetTypes.BOOKMARK_ASSET,
contentType: downloaded.contentType,
size: downloaded.size,
fileName,
},
trx,
);
await trx.insert(bookmarkAssets).values({
id: bookmarkId,
assetType,
assetId: downloaded.assetId,
content: null,
fileName,
sourceUrl: url,
});
await trx
.update(bookmarks)
.set({ type: BookmarkTypes.ASSET })
.where(eq(bookmarks.id, bookmarkId));
await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId));
});
+ conversionCommitted = true;
- await AssetPreprocessingQueue.enqueue(
- {
- bookmarkId,
- fixMode: false,
- },
- {
- groupId: userId,
- },
- );
+ await AssetPreprocessingQueue.enqueue(
+ { bookmarkId, fixMode: false },
+ { groupId: userId },
+ );
} catch (error) {
- if (downloadedAssetId) {
+ if (!conversionCommitted && downloadedAssetId) {
logger.error(
`[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`,
);
await silentDeleteAsset(userId, downloadedAssetId);
}
throw error;
}📝 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
| let downloadedAssetId: string | undefined; | |
| try { | |
| const downloaded = await downloadAndStoreFile( | |
| url, | |
| userId, | |
| jobId, | |
| assetType, | |
| abortSignal, | |
| ); | |
| if (!downloaded) { | |
| return; | |
| } | |
| downloadedAssetId = downloaded.assetId; | |
| const fileName = path.basename(new URL(url).pathname); | |
| await db.transaction(async (trx) => { | |
| await updateAsset( | |
| undefined, | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| await AssetPreprocessingQueue.enqueue( | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| fixMode: false, | |
| }, | |
| { | |
| groupId: userId, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| await AssetPreprocessingQueue.enqueue( | |
| { | |
| bookmarkId, | |
| fixMode: false, | |
| }, | |
| { | |
| groupId: userId, | |
| }, | |
| ); | |
| } catch (error) { | |
| if (downloadedAssetId) { | |
| logger.error( | |
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | |
| ); | |
| await silentDeleteAsset(userId, downloadedAssetId); | |
| } | |
| throw error; | |
| } | |
| let downloadedAssetId: string | undefined; | |
| let conversionCommitted = false; | |
| try { | |
| const downloaded = await downloadAndStoreFile( | |
| url, | |
| userId, | |
| jobId, | |
| assetType, | |
| abortSignal, | |
| ); | |
| if (!downloaded) { | |
| return; | |
| } | |
| downloadedAssetId = downloaded.assetId; | |
| const fileName = path.basename(new URL(url).pathname); | |
| await db.transaction(async (trx) => { | |
| await updateAsset( | |
| undefined, | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| conversionCommitted = true; | |
| await AssetPreprocessingQueue.enqueue( | |
| { bookmarkId, fixMode: false }, | |
| { groupId: userId }, | |
| ); | |
| } catch (error) { | |
| if (!conversionCommitted && downloadedAssetId) { | |
| logger.error( | |
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | |
| ); | |
| await silentDeleteAsset(userId, downloadedAssetId); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workers/workers/crawlerWorker.ts` around lines 1559 - 1619, The catch
block unconditionally deletes downloadedAssetId even if the DB transaction
committed and the asset is now live; modify handleAsAssetBookmark so cleanup
only runs when the transaction did not commit: after calling
downloadAndStoreFile set downloadedAssetId, introduce a boolean like
transactionCommitted (default false), set it to true at the end of the
db.transaction callback (after updateAsset/insert/update/delete), then call
AssetPreprocessingQueue.enqueue; in the catch, only call
silentDeleteAsset(userId, downloadedAssetId) if !transactionCommitted (i.e., the
DB changes never committed). Reference symbols: downloadedAssetId,
downloadAndStoreFile, db.transaction, AssetPreprocessingQueue.enqueue,
silentDeleteAsset.
Comment on lines
+1836
to
+1848
| const screenshotAssetInfo = await Promise.race([ | ||
| storeScreenshot(screenshot, userId, jobId), | ||
| abortPromise(abortSignal), | ||
| ]); | ||
| if (screenshotAssetInfo) { | ||
| await updateAsset( | ||
| oldScreenshotAssetId, | ||
| { | ||
| id: screenshotAssetInfo.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.LINK_SCREENSHOT, | ||
| contentType: screenshotAssetInfo.contentType, | ||
| size: screenshotAssetInfo.size, | ||
| fileName: screenshotAssetInfo.fileName, | ||
| }, | ||
| txn, | ||
| ); | ||
| assetDeletionTasks.push( | ||
| silentDeleteAsset(userId, oldScreenshotAssetId), | ||
| ); | ||
| newAssetIds.push(screenshotAssetInfo.assetId); | ||
| } | ||
| abortSignal.throwIfAborted(); | ||
|
|
||
| const pdfAssetInfo = await Promise.race([ | ||
| storePdf(pdf, userId, jobId), | ||
| abortPromise(abortSignal), | ||
| ]); |
Contributor
There was a problem hiding this comment.
Abort racing can leave untracked assets behind.
Promise.race here does not cancel storeScreenshot/storePdf. If abort wins first, the storage promise can still complete and persist assets whose IDs never get added to newAssetIds.
🔧 Suggested fix
- const screenshotAssetInfo = await Promise.race([
- storeScreenshot(screenshot, userId, jobId),
- abortPromise(abortSignal),
- ]);
+ const screenshotAssetInfo = await storeScreenshot(
+ screenshot,
+ userId,
+ jobId,
+ );
+ abortSignal.throwIfAborted();
if (screenshotAssetInfo) {
newAssetIds.push(screenshotAssetInfo.assetId);
}
- abortSignal.throwIfAborted();
- const pdfAssetInfo = await Promise.race([
- storePdf(pdf, userId, jobId),
- abortPromise(abortSignal),
- ]);
+ const pdfAssetInfo = await storePdf(pdf, userId, jobId);
+ abortSignal.throwIfAborted();
if (pdfAssetInfo) {
newAssetIds.push(pdfAssetInfo.assetId);
}
- abortSignal.throwIfAborted();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workers/workers/crawlerWorker.ts` around lines 1836 - 1848, The code
races storeScreenshot/storePdf against abortPromise but doesn’t cancel the
underlying storage calls, which can complete and create assets that never get
tracked; modify the storage flow so storage is abortable and/or cleaned up:
update storeScreenshot and storePdf to accept an AbortSignal (or return a
handle) and have them abort any in-flight upload when abortSignal is triggered,
or after the storage promise resolves check abortSignal.aborted and if aborted
call the corresponding cleanup/delete function for the returned asset before
returning/adding to newAssetIds; ensure you only push
screenshotAssetInfo.assetId / pdfAssetInfo.assetId into newAssetIds after
confirming the operation wasn’t aborted (use abortSignal.throwIfAborted or
signal.aborted) and wire abortSignal into the Promise.race path so no orphaned
assets remain.
Greptile SummaryImplemented bookmark source filtering by adding a new
The implementation follows existing patterns and integrates well with the codebase, though there are minor issues with tooltip accuracy and type safety that should be addressed. Confidence Score: 4/5
Important Files Changed
Last reviewed commit: f188670 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/dashboard/rules/RuleEngineConditionBuilder.tsx
Outdated
Show resolved
Hide resolved
| Image as ImageIcon, | ||
| MousePointer2, | ||
| NotebookPen, | ||
| Plus, |
There was a problem hiding this comment.
Plus is imported but never used
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Line: 26
Comment:
`Plus` is imported but never used
How can I resolve this? If you propose a fix, please make it concise.
….tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…Card.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Made-with: Cursor
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.
Fixes #2526.
Summary
This PR adds the ability to distinguish between manually added bookmarks and those received via RSS subscriptions, both visually and through automated rules.
Key Changes: