Fix/dreamsync recommendations #691
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR migrates wishlist summaries from single strings to JSON arrays, updates summarization and matching services to operate on arrays with stricter validation (confidence > 0.85), adds a TypeORM migration and a standalone migration script to populate new fields, and includes minor docs and gitignore cleanups. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
coodos
force-pushed
the
fix/dreamsync-recommendations
branch
from
2fec9ff to
6d7c819
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
platforms/dreamsync-api/src/services/WishlistSummaryService.ts (1)
96-101: Logic issue: Wishlists with legitimately empty offers/wants get re-processed.The condition requires both
summaryWants.length > 0ANDsummaryOffers.length > 0. If a user genuinely has no offers (e.g., only wants things), this wishlist will be re-summarized on every call toensureSummaries, even though the summary was already computed correctly.Consider checking for the presence of the arrays (not their length) or accepting that empty arrays are valid summaries:
🐛 Proposed fix
async ensureSummaries(wishlist: Wishlist): Promise<Wishlist> { - if (wishlist.summaryWants && wishlist.summaryWants.length > 0 && - wishlist.summaryOffers && wishlist.summaryOffers.length > 0) { + // Arrays exist (even if empty) means summary was already generated + if (Array.isArray(wishlist.summaryWants) && Array.isArray(wishlist.summaryOffers)) { return wishlist; } return this.summarizeAndPersist(wishlist); }
🤖 Fix all issues with AI agents
In `@platforms/dreamsync-api/src/database/migrations/1768904445609-migration.ts`:
- Around line 6-11: The migration currently drops and re-adds summaryWants and
summaryOffers which destroys existing data; update the up(queryRunner:
QueryRunner) implementation to convert in-place instead of DROP/ADD by using
ALTER TABLE "wishlists" ALTER COLUMN "<column>" TYPE jsonb USING
to_json("<column>")::jsonb for both summaryWants and summaryOffers (remove the
DROP and ADD statements), so existing text summaries are preserved and cast to
jsonb; if deletion was intentional, add a clear comment and ensure the
migrate-summaries.ts runs immediately after deployment instead of changing the
migration.
🧹 Nitpick comments (4)
platforms/dreamsync-api/src/services/WishlistSummaryService.ts (1)
135-174: Performance concern: Re-summarizing all wishlists on every platform start is expensive.This method makes an OpenAI API call for every active wishlist on each platform restart. With
Nwishlists, this incursNAPI calls (cost, latency, rate limits) even if summaries already exist.Consider either:
- Checking if summaries already exist before calling OpenAI (like the migration script does)
- Making this opt-in via environment variable or CLI flag
- Using
backfillMissingSummaries()for startup instead♻️ Proposed optimization
for (const wishlist of allWishlists) { try { + // Skip if already has valid array summaries + if (Array.isArray(wishlist.summaryWants) && Array.isArray(wishlist.summaryOffers)) { + console.log(`[${wishlist.id}] Already has summaries, skipping`); + continue; + } + const summary = await this.summarizeWishlistContent(wishlist.content);platforms/dreamsync-api/scripts/migrate-summaries.ts (2)
35-68: Consider adding rate limiting for large datasets.The migration script processes wishlists sequentially without delays. For large datasets, this could trigger OpenAI rate limits. Consider adding a small delay between API calls:
♻️ Optional: Add rate limiting
+const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); + for (const wishlist of wishlists) { try { // ... existing code ... console.log(` Saved successfully\n`); processedCount++; + + // Small delay to avoid rate limits + await delay(200); } catch (error) {
23-27: Minor: Unuseduserrelation.The query loads
relations: ["user"]but the user data is never used in the migration logic. Consider removing it to reduce query overhead.♻️ Proposed cleanup
const wishlists = await wishlistRepository.find({ where: { isActive: true }, - relations: ["user"], order: { updatedAt: "DESC" }, });platforms/dreamsync-api/src/services/AIMatchingService.ts (1)
396-429: Inconsistency: Uses raw content instead of summary arrays.This prompt uses
wishlist.content(raw text) whileSharedMatchingService.buildAnalysisPromptuses the structuredsummaryWants/summaryOffersarrays. This inconsistency could lead to different matching behavior depending on which code path is used.Consider aligning this method to use the array-based summaries for consistency:
♻️ Suggested alignment with SharedMatchingService
private buildAnalysisPrompt(wishlistA: Wishlist, wishlistB: Wishlist): string { + const wantsA = (wishlistA.summaryWants || []).join(', '); + const offersA = (wishlistA.summaryOffers || []).join(', '); + const wantsB = (wishlistB.summaryWants || []).join(', '); + const offersB = (wishlistB.summaryOffers || []).join(', '); + return ` Analyze these two wishlists to determine if there's a meaningful connection: **User A (${wishlistA.user.ename}):** -Title: ${wishlistA.title} -Content: ${wishlistA.content} +Wants: ${wantsA || 'None specified'} +Offers: ${offersA || 'None specified'} **User B (${wishlistB.user.ename}):** -Title: ${wishlistB.title} -Content: ${wishlistB.content} +Wants: ${wantsB || 'None specified'} +Offers: ${offersB || 'None specified'}
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(`ALTER TABLE "wishlists" DROP COLUMN "summaryWants"`); | ||
| await queryRunner.query(`ALTER TABLE "wishlists" ADD "summaryWants" jsonb`); | ||
| await queryRunner.query(`ALTER TABLE "wishlists" DROP COLUMN "summaryOffers"`); | ||
| await queryRunner.query(`ALTER TABLE "wishlists" ADD "summaryOffers" jsonb`); | ||
| } |
There was a problem hiding this comment.
Data loss risk: Dropping columns destroys existing summaries.
The migration drops summaryWants and summaryOffers columns before re-adding them as jsonb. Any existing text summaries in production will be permanently lost. The migrate-summaries.ts script cannot help because it runs after the migration has already deleted the data.
Consider using ALTER COLUMN ... TYPE jsonb USING to preserve and convert existing data:
🐛 Proposed fix to preserve existing data
public async up(queryRunner: QueryRunner): Promise<void> {
- await queryRunner.query(`ALTER TABLE "wishlists" DROP COLUMN "summaryWants"`);
- await queryRunner.query(`ALTER TABLE "wishlists" ADD "summaryWants" jsonb`);
- await queryRunner.query(`ALTER TABLE "wishlists" DROP COLUMN "summaryOffers"`);
- await queryRunner.query(`ALTER TABLE "wishlists" ADD "summaryOffers" jsonb`);
+ // Convert existing text to jsonb arrays, wrapping single strings in arrays
+ await queryRunner.query(`
+ ALTER TABLE "wishlists"
+ ALTER COLUMN "summaryWants" TYPE jsonb
+ USING CASE
+ WHEN "summaryWants" IS NULL THEN NULL
+ ELSE to_jsonb(ARRAY["summaryWants"])
+ END
+ `);
+ await queryRunner.query(`
+ ALTER TABLE "wishlists"
+ ALTER COLUMN "summaryOffers" TYPE jsonb
+ USING CASE
+ WHEN "summaryOffers" IS NULL THEN NULL
+ ELSE to_jsonb(ARRAY["summaryOffers"])
+ END
+ `);
}Alternatively, if re-generating all summaries is intentional, document this clearly and ensure the migration script runs immediately after on deployment.
🤖 Prompt for AI Agents
In `@platforms/dreamsync-api/src/database/migrations/1768904445609-migration.ts`
around lines 6 - 11, The migration currently drops and re-adds summaryWants and
summaryOffers which destroys existing data; update the up(queryRunner:
QueryRunner) implementation to convert in-place instead of DROP/ADD by using
ALTER TABLE "wishlists" ALTER COLUMN "<column>" TYPE jsonb USING
to_json("<column>")::jsonb for both summaryWants and summaryOffers (remove the
DROP and ADD statements), so existing text summaries are preserved and cast to
jsonb; if deletion was intentional, add a clear comment and ensure the
migrate-summaries.ts runs immediately after deployment instead of changing the
migration.
There was a problem hiding this comment.
nack this data is essentially platform level logic which can be discarded and is not important, the only truly important thing is keeping the actual wishlist safe
There was a problem hiding this comment.
@coodos, understood! Since summaryWants and summaryOffers are derived from the wishlist content and can be regenerated via the migrate-summaries.ts script, dropping them is safe. The source data remains intact. Thanks for the clarification.
✏️ Learnings added
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 691
File: platforms/dreamsync-api/src/database/migrations/1768904445609-migration.ts:6-11
Timestamp: 2026-01-20T10:41:46.622Z
Learning: In the DreamSync platform (platforms/dreamsync-api), the `summaryWants` and `summaryOffers` fields in the Wishlist entity are derived/computed data that can be safely discarded and regenerated from the wishlist content. Only the actual wishlist content is considered critical data that must be preserved.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
coodos
force-pushed
the
fix/dreamsync-recommendations
branch
from
6d7c819 to
2fec9ff
Compare
coodos
force-pushed
the
fix/dreamsync-recommendations
branch
from
2fec9ff to
4e92c08
Compare
coodos
force-pushed
the
fix/dreamsync-recommendations
branch
from
998f088 to
344b9da
Compare
coodos
force-pushed
the
fix/dreamsync-recommendations
branch
from
aabae04 to
41a6292
Compare
Description of change
Wishlist Summarization Flow
Current System Architecture
flowchart TD A[User Creates/Updates Wishlist] --> B[Raw Content Stored] B --> C[WishlistSummaryService.summarizeWishlistContent] C --> D[AI Request: Generate Arrays] D --> E{AI Response} E -->|Success| F[Parse JSON Arrays] E -->|Failure| G[Fallback: Extract from Markdown] F --> H["summaryWants: string array"] F --> I["summaryOffers: string array"] G --> H G --> I H --> J[Save to DB as jsonb] I --> J K[Platform Startup] --> L[summarizeAllWishlists] L --> M[Load All Active Wishlists] M --> N[For Each Wishlist] N --> C N --> O[Log Raw Content + Summary Arrays] O --> P[Save to Database] Q[Matching Process] --> R[Load Wishlists with Summaries] R --> S{Has Valid Arrays?} S -->|No| T[Skip Wishlist] S -->|Yes| U[Join Arrays with Semicolons] U --> V[Create CSV Format] V --> W["userId|userEname|userName|wants|offers"] W --> X[Send to AI Matching] X --> Y[Generate Matches]Issue Number
closes #642
closes #644
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.