Fix cleanup_unsubmitted_forms issues and add UI component breakdown#13
Fix cleanup_unsubmitted_forms issues and add UI component breakdown#13Scecil044 wants to merge 2 commits into
Conversation
…ts. Issues fixed include: 1: Date calculation bug (Initial implementation did not include milliseconds). 2:Added proper null safety with filter(Boolean) for entityIds to prevent runtime errors. 3:Fixed transaction order to delete relationships first, avoiding foreign key constraint violations. 4.Corrected logic flaw: changed from narrow 24-hour window to properly find records older than 7 days using lt operator. 5.Eliminated N+1 query performance issue by using include to fetch relationships in single query. 6. Replaced individual delete operations with batch deleteMany for better performance. Note:Single database query with include instead of loop with individual queries, Batch delete operations reduce database round trips, and Proper filtering eliminates unnecessary delete attempts
…ar component architecture for the portfolio preview page
WalkthroughAdds a new documentation file describing the Portfolio Overview UI component hierarchy. Refactors the unsubmitted forms cleanup job to a batched, transactional process using a single age threshold, filtered selection, early exit on no-op, and ordered deletions within a single Prisma transaction, with existing error handling and job status updates retained. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JR as JobRunner
participant CF as cleanup_unsubmitted_forms
participant DB as Prisma/DB
JR->>CF: start(jobId)
CF->>DB: query tokens lt sevenDaysAgo<br/>include relationships(status=="new")
CF->>CF: filter tokens with non-empty entityId<br/>and ≥1 "new" relationship
alt No tokens to process
CF->>DB: update job status = "completed"
CF-->>JR: done (no-op)
else Tokens to delete
note over CF,DB: Single transaction
CF->>DB: tx.begin
CF->>DB: delete related relationships (FK-order)
CF->>DB: delete tokens
CF->>DB: delete corpus items by entityIds
CF->>DB: delete entities by entityIds
CF->>DB: tx.commit
CF->>DB: update job status = "completed"
CF-->>JR: done (success)
end
opt On error
CF->>DB: update job status = "failed"
CF-->>JR: throw error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (6)
61-65: Deduplicate IDs before IN queries.Small perf tidy-up: avoid duplicate IDs in IN lists.
- const entityIds = tokensToDelete.map((token) => token.entityId).filter(Boolean); - const tokenIds = tokensToDelete.map((token) => token.token); - const relationshipIds = tokensToDelete.flatMap((token) => - token.relationship.map((rel) => rel.id) - ); + const entityIds = Array.from(new Set(tokensToDelete.map(t => t.entityId!).filter(Boolean))); + const tokenIds = tokensToDelete.map(t => t.token); + const relationshipIds = tokensToDelete.flatMap(t => t.relationship.map(rel => rel.id));
67-67: Set transaction timeouts to reduce lock risk.Interactive transactions can hold locks longer than intended under load. Add explicit timeouts.
- await prisma.$transaction(async (tx) => { + await prisma.$transaction(async (tx) => { // ... - }); + }, { maxWait: 5000, timeout: 30000 });Also applies to: 91-92
31-33: Batch large cleanups; avoid unbounded single-transaction work.If the backlog is large, this single transaction can be big, slow, and locky. Prefer chunking by e.g. 500–2000 tokens per batch (ordered by createdAt asc) in a loop until no more rows match. Gate each batch in its own transaction.
I can provide a batched version if helpful.
Also applies to: 60-66, 67-92
38-39: Fix misleading comment.“Less than 7 days ago = older than 7 days” is inverted.
- lt: sevenDaysAgo, // Less than 7 days ago = older than 7 days + lt: sevenDaysAgo, // Earlier than the threshold => older than 7 days
41-46: Prefer typed status enum over string literal.If
statusis a Prisma enum (e.g., RelationshipStatus), import and use it to avoid typos.Example:
import { RelationshipStatus } from '@prisma/client'; // ... where: { status: RelationshipStatus.new }
94-98: Improve error context.Log the job id to simplify incident tracing.
- console.error("Error cleaning up unsubmitted forms:", error); + console.error(`Error cleaning up unsubmitted forms (jobId=${job.id}):`, error);tests/breakdown_ui/component-breakdown.md (3)
5-5: Add a language to the fenced code block (markdownlint MD040).Specify a language to satisfy linting and improve rendering.
-``` +```text
109-109: Typo: use “prop drilling” (singular), not “props drilling”.-**Trade-off**: Slightly more props drilling, but gains modularity and reusability +**Trade-off**: Slightly more prop drilling, but gains modularity and reusability @@ -**Cost**: More props drilling, slightly more complexity +**Cost**: More prop drilling, slightly more complexityAlso applies to: 115-115
66-70: Optional: document accessibility and empty/error states.
- TableHeader: note keyboard interaction and ARIA attributes for sorted columns.
- GroupRow/CompanyRow: specify focus management when expanding/collapsing.
- Page-level: define empty, loading, and error states (skeletons, toasts).
Also applies to: 71-76, 77-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
tests/breakdown_ui/component-breakdown.md(1 hunks)tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
tests/breakdown_ui/component-breakdown.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (2)
24-28: Sanity-check Prisma schema & field names before merge (schema.prisma not found)schema.prisma couldn't be located in the repository; confirm the Prisma schema path and that the following delegates/fields referenced by the test match exactly (case-sensitive):
- tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (lines 24–28, 35–48, 80–90): publicFormsTokens (fields: token, entityId, relationship), Relationship/relationship, New_corpus/new_corpus, Entity/entity.
- Also confirm any status enum/value used by the cleanup job (e.g., "new") exists with identical spelling/casing.
80-90: Guard entity deletes: don't delete entities still referenced by other tokens.File: tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (approx. lines 80–90)
Don't delete an entity (and its corpus) if it is still referenced by other tokens — add a survivorship check inside the transaction:
- if (entityIds.length > 0) { - await tx.new_corpus.deleteMany({ - where: { entity_id: { in: entityIds } }, - }); - - // Delete entities last - await tx.entity.deleteMany({ - where: { id: { in: entityIds } }, - }); - } + if (entityIds.length > 0) { + // Keep entities that still have other tokens + const survivors = await tx.publicFormsTokens.findMany({ + where: { entityId: { in: entityIds }, token: { notIn: tokenIds } }, + select: { entityId: true }, + }); + const survivorSet = new Set(survivors.map(s => s.entityId)); + const entityIdsSafeToDelete = entityIds.filter(id => !survivorSet.has(id)); + + if (entityIdsSafeToDelete.length > 0) { + await tx.new_corpus.deleteMany({ + where: { entity_id: { in: entityIdsSafeToDelete } }, + }); + await tx.entity.deleteMany({ + where: { id: { in: entityIdsSafeToDelete } }, + }); + } + }Confirm whether an entity can have multiple tokens and whether entities may be referenced by relationships outside the "new" status; if so, gate deletion on the absence of any other tokens or any non-"new" relationships for the entity (not just per-token relationships).
| const expiredTokens = await prisma.publicFormsTokens.findMany({ | ||
| where: { | ||
| createdAt: { | ||
| gte: sevenDaysAgo, // greater than or equal to 7 days ago | ||
| lt: sevenDaysAgoPlusOneDay, // but less than 7 days ago + 1 day | ||
| lt: sevenDaysAgo, // Less than 7 days ago = older than 7 days | ||
| }, | ||
| }, | ||
| include: { | ||
| relationship: { | ||
| where: { | ||
| status: "new", | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Prevent accidental data loss; include zero-relationship tokens; push filtering into the DB.
Today we: (a) skip tokens with zero relationships and (b) might delete tokens/entities even when there are non-"new" relationships present (we only include "new" relationships but never exclude the presence of others). Both are correctness/data‑loss risks.
- Include tokens that have zero relationships (still unsubmitted).
- Exclude any token that has at least one non-"new" relationship.
- Reduce payload by selecting only fields we actually use.
- Then drop the in-memory filter.
Apply:
- const expiredTokens = await prisma.publicFormsTokens.findMany({
- where: {
- createdAt: {
- lt: sevenDaysAgo, // Less than 7 days ago = older than 7 days
- },
- },
- include: {
- relationship: {
- where: {
- status: "new",
- },
- },
- },
- });
+ const expiredTokens = await prisma.publicFormsTokens.findMany({
+ where: {
+ createdAt: { lt: sevenDaysAgo },
+ entityId: { not: null },
+ // Vacuously true when there are zero relationships -> included.
+ relationship: { every: { status: "new" } },
+ },
+ select: {
+ token: true,
+ entityId: true,
+ relationship: {
+ where: { status: "new" },
+ select: { id: true },
+ },
+ },
+ });
@@
- const tokensToDelete = expiredTokens.filter(
- (token) => token.entityId && token.relationship.length > 0
- );
+ const tokensToDelete = expiredTokens;Also applies to: 50-53
1 participant
Fixed critical issues in cleanup_unsubmitted_forms and completed breakdown_ui analysis.
Changes Made
component-breakdown.md
Summary by CodeRabbit
Refactor
Documentation