Fix/pipeline robustness - #32
Conversation
…e, queue ages, expired leases, almost-publishable events, enrichment failure rate and slowed down the pipeline a bit
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR adds comprehensive pipeline observability and monitoring infrastructure by introducing a new "Pipeline Doctor" admin query, restructuring run logging across ingestion/enrichment/clustering, switching feed "recent" sort to use ChangesPipeline Observability and Ordering System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/convex/events.ts (1)
224-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecent sort fallback still uses old publish-time ordering.
When
sort === "recent"withouttopicId, Line 226 still paginates onby_first_published_at. That makes “Latest” inconsistent with the updatedlastUpdatedAtlogic used elsewhere.Proposed fix
- events = await ctx.db - .query("publicEventPreviews") - .withIndex("by_first_published_at") - .order("desc") - .paginate(paginationOpts); + events = await ctx.db + .query("publicEventPreviews") + .withIndex("by_last_updated_at") + .order("desc") + .paginate(paginationOpts);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/convex/events.ts` around lines 224 - 228, The recent-sort branch still paginates using the old "by_first_published_at" index, causing "Latest" to ignore lastUpdatedAt; update the query that sets events to use the appropriate index when sort === "recent" (and topicId is not provided) by switching .withIndex("by_first_published_at") to .withIndex("by_last_updated_at") for the "recent" case (leave the existing index for other sorts), ensuring the same paginationOpts and .order("desc") are used so latest results follow the lastUpdatedAt logic used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pipeline-operations-runbook.md`:
- Line 36: Update the sentence containing "qGB" so the abbreviation is defined
on first use (e.g., "qGB (query gigabytes)" or the project's exact full term) —
change the line "Enter observed last-24h qGB only when the Convex dashboard
materially differs from the estimate." to include the full phrase for qGB and,
if helpful, add a brief parenthetical clarifying that it refers to the Convex
dashboard vector/query budget metric; keep the rest of the instruction
unchanged.
In `@packages/backend/convex/lib/publicEventPreviews.ts`:
- Around line 161-167: The patch branch currently updates existing rows via
ctx.db.patch(existing._id, payload) but never sets createdAt, so
existing.createdAt can remain undefined; modify the patch logic in
publicEventPreviews.ts so that before calling ctx.db.patch you ensure
payload.createdAt is set to existing.createdAt if present, otherwise to now (the
same timestamp used for inserts), e.g., compute a createdAt value from
existing.createdAt || now and merge it into the payload when calling
ctx.db.patch to avoid losing or leaving createdAt undefined.
In `@packages/backend/convex/pipeline.ts`:
- Around line 534-537: The code currently builds latestHiddenByTrending by
filtering latestRows against trendingIds and slicing to 10, which makes its
.length a sampled count rather than the true hidden total; change this by
computing a separate totalHiddenCount (e.g., const latestHiddenTotal =
latestRows.filter(row => !trendingIds.has(row.eventId)).length) and also keep a
sampled array for display (e.g., const latestHiddenByTrending =
latestRows.filter(...).slice(0,10).map(...)); update any downstream references
to use latestHiddenTotal for counts and latestHiddenByTrending for the displayed
items so the UI receives both the true count and the sample.
In `@packages/backend/convex/shareAssetsNode.ts`:
- Around line 412-415: The early-return branch that returns { generated: false,
reason: "disabled" } leaves the queued record in "pending" so a future re-enable
is blocked by the already_queued short-circuit in ensureEventShareAssetQueued;
update the branch to transition or clear the queued entry when returning
disabled (e.g., update the queued record status or remove the pending queue
entry) and/or make ensureEventShareAssetQueued aware of the "disabled" outcome
so it doesn't treat that signature as already_queued; touch the code paths that
emit { generated: false, reason: "disabled" and the ensureEventShareAssetQueued
check for already_queued to ensure the queue is cleared or the status updated
before returning.
---
Outside diff comments:
In `@packages/backend/convex/events.ts`:
- Around line 224-228: The recent-sort branch still paginates using the old
"by_first_published_at" index, causing "Latest" to ignore lastUpdatedAt; update
the query that sets events to use the appropriate index when sort === "recent"
(and topicId is not provided) by switching .withIndex("by_first_published_at")
to .withIndex("by_last_updated_at") for the "recent" case (leave the existing
index for other sorts), ensuring the same paginationOpts and .order("desc") are
used so latest results follow the lastUpdatedAt logic used elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ac4d360-0690-4768-a436-7c9d92c63407
📒 Files selected for processing (16)
apps/web/src/lib/i18n/strings.tsapps/web/src/routes/admin.pipeline.tsxdocs/pipeline-operations-runbook.mdpackages/backend/convex/clustering.tspackages/backend/convex/config.tspackages/backend/convex/crons.tspackages/backend/convex/enrichment.tspackages/backend/convex/enrichmentNode.tspackages/backend/convex/events.tspackages/backend/convex/ingestion.tspackages/backend/convex/lib/publicEventPreviews.tspackages/backend/convex/pipeline.tspackages/backend/convex/schema.tspackages/backend/convex/shareAssets.tspackages/backend/convex/shareAssetsNode.tspackages/backend/convex/singletonCleanup.ts
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores