feat: fixed link unfurling worker#49
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refines URL and link processing across multiple applications. Changes include URL extraction with deduplication via Set in realtime, Markdown link normalization in the web editor, delegation of redirect handling to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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 |
Summary
This PR introduces refactored link unfurling functionality with improved security and cleaner architecture:
Changes Made:
Link Unfurling Worker (apps/worker/src/jobs/link-unfurl.ts) - Completely new implementation
open-graph-scraperisSafeUrl()checks on DNS lookups, private IP ranges, and post-redirect URLsURL Extraction (apps/realtime/src/index.ts)
/https?:\/\/[^\s<>"[\]]+/gMarkdown Normalization (apps/web/src/lib/editor-utils.ts)
\[([^\]]+)\]\(\1\)++wrappersConfiguration (turbo.json)
stream-with-experimental-timestampstostreamConcerns:
Critical Bug:
OG_FETCH_TIMEOUT_MS = 5is set to 5 milliseconds, which is far too short for HTTP requests. This will cause timeout failures on virtually all link unfurl attempts. Should likely be5000(5 seconds).No Test Coverage: PR introduces substantial new functionality with no visible test cases. No unit or integration tests provided for the link unfurling logic.
Incomplete Context: The summary indicates code was removed (
-61lines in link-unfurl.ts), but the git history shows this as a new file, making it unclear what functionality was replaced.Strengths:
Confidence Score: 2/5
The PR shows solid engineering practices in security and architecture, but the timeout configuration appears to be a critical bug that would render the feature non-functional. The lack of test coverage for core functionality and the mysterious reference to removed code in the summary add uncertainty. This requires fixes before merging.