feat(pipeline-catalog): enhance pipeline catalog resolver with addtl data sources#227
Conversation
…onal data sources - Updated the pipeline catalog resolver to merge data from three sources for improved cold-start stability. - Introduced a new REST catalog endpoint to enrich the pipeline catalog with models and regions, addressing issues with incomplete data during refresh. - Added utility functions for filtering and merging catalog entries, ensuring a comprehensive and organized output for the dashboard.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pipeline catalog resolver now integrates a third data source by fetching from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
seanhanca
left a comment
seanhanca
left a comment
There was a problem hiding this comment.
Review Summary
Good fix — merging a third data source (/v1/pipelines) addresses the intermittent single-pipeline issue cleanly. The unionCatalogEntries merge is correct (Set-based dedup, sorted output). Two items worth addressing:
-
No tests for the new functions —
unionCatalogEntriesandpipelinesEndpointToDashboardare pure data transforms that are easy to unit test. Adding a few test cases (empty inputs, overlapping models/regions, disjoint pipelines) would prevent regressions. -
PR checklist items unchecked — tests pass, lint, and build are all unchecked in the PR description. Confirm these pass before merge.
See inline comments for details.
| })); | ||
| } | ||
|
|
||
| /** Union pipeline ids, merging model and region sets (order-stable). */ |
There was a problem hiding this comment.
Missing tests: unionCatalogEntries and pipelinesEndpointToDashboard are pure functions ideal for unit testing. Consider adding a test file (e.g. __tests__/pipeline-catalog.test.ts) with cases for:
- Empty arrays from all three sources
- Overlapping pipeline IDs with different model/region sets
PIPELINE_DISPLAY[entry.id] === nullfiltering
This prevents silent regressions if the merge logic or display filtering changes.
| }); | ||
| continue; | ||
| } | ||
| for (const m of e.models) { |
There was a problem hiding this comment.
The .catch(() => []) silently swallows errors from getRawPipelineCatalog(). The console.warn is good, but if this endpoint consistently fails, there's no observability beyond server logs. Consider emitting a structured warning (or at minimum, ensure the warn includes enough context to diagnose — you already do with the err parameter, so this is fine as-is). Just flagging that persistent failures here will silently degrade the catalog without alerting.
2 participants
Summary
Resolves issue with only live-video-to-video pipeline showing intermittently
Changes
Type
Plugin(s) Affected
Checklist
npm run lint)npm run build)Breaking Changes
None
Screenshots / Recordings
Summary by CodeRabbit