feed: integrate Certified Feed Service transport - #238
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds a certified-feed service transport with strict response validation, service and indexer home-feed implementations, normalized event rendering, pagination recovery, updated development fixtures, and hydration-safe desktop navigation behavior. ChangesHome-feed service migration
Hydration-safe desktop chrome
Process documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant HomeFeed
participant useHomeFeed
participant fetchCertifiedFeed
participant CertifiedFeedService
participant HomeFeedBody
HomeFeed->>useHomeFeed: select service source and provide viewer/filter state
useHomeFeed->>fetchCertifiedFeed: request initial or continuation page
fetchCertifiedFeed->>CertifiedFeedService: POST certified feed request
CertifiedFeedService-->>fetchCertifiedFeed: validated or error response
fetchCertifiedFeed-->>useHomeFeed: CertifiedFeedPage or CertifiedFeedError
useHomeFeed-->>HomeFeedBody: events, pagination, retry, and error state
HomeFeedBody-->>HomeFeed: render rows or retry/load-more controls
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/lib/atproto/certified-feed.ts (1)
263-265: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBody-size guard runs after the body is fully buffered.
await response.text()on Line 244 already materializes the whole payload, so the 512 KiB check can only report an oversized response, never prevent the allocation. If the intent is protection, cap while streaming; otherwise the check is documentation only.🤖 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 `@src/lib/atproto/certified-feed.ts` around lines 263 - 265, Update the response-body handling around response.text() and the MAX_ERROR_BODY guard to enforce the size limit while reading the response stream, before the full payload is buffered. Track accumulated bytes or characters, abort and throw contractError once the limit is exceeded, then decode the bounded content for the existing error-processing flow.src/hooks/use-home-feed.ts (1)
172-176: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRefs mutated during render (also Lines 472-475 in
useLegacyHomeFeed).
stateRef.current = state/currentRequestKeyRef.current = requestKey(andfollowedRef.current) run in the render body, which React may replay or discard; the configured React Doctor rule flags these as errors (no-ref-current-in-render). Since both refs exist only soloadMore/retryInitialcan read the latest snapshot, syncing them in a layout/insertion effect (or deriving the guards from the callback deps directly) keeps render pure without changing the concurrency semantics. Please confirm whether this rule is enforced in CI before merging.🤖 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 `@src/hooks/use-home-feed.ts` around lines 172 - 176, The refs used by the home-feed callbacks are mutated during render, violating the configured no-ref-current-in-render rule. In both the primary hook and useLegacyHomeFeed, move stateRef, currentRequestKeyRef, and followedRef synchronization into a layout or insertion effect, preserving the latest-snapshot behavior for loadMore and retryInitial; also verify whether this rule is enforced in CI.Source: Linters/SAST tools
src/lib/dev/fixtures/feed.ts (1)
112-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixture actor shape doesn't match the certified-feed contract.
feedActorBlockemitsavatarCid, butparseActorinsrc/lib/atproto/certified-feed.ts(Lines 397-402) readsavataras anorg.hypercerts.defs#uri/#smallImageobject and ignores unknown keys — so service-mode previews always render a null avatar. Harmless today because everyMOCK_ACTORSentry hasavatarCid: null, but the fixture will diverge from the real payload as soon as one gains an avatar. Emitting a properavatarblob (or dropping the field) keeps the fixture honest.🤖 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 `@src/lib/dev/fixtures/feed.ts` around lines 112 - 124, Update certifiedFeedPage’s feedActorBlock output to match parseActor’s certified-feed contract by emitting the supported avatar object under avatar, or omit the field when no avatar exists; do not emit avatarCid, which parseActor ignores. Preserve null-avatar behavior for current MOCK_ACTORS entries while ensuring future actors with avatars serialize in the same shape as real payloads.src/components/home/home-feed.tsx (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant Escape handler.
useClickOutsideClose(line 103) already registers a documentkeydownlistener that closes on Escape, so this effect duplicates it. Dropping it removes a listener and keeps the close path in one place.🤖 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 `@src/components/home/home-feed.tsx` around lines 104 - 111, Remove the redundant Escape-key useEffect and its document listener from the filter-open handling, leaving useClickOutsideClose as the sole Escape-close path. Preserve the existing filterOpen state behavior and click-outside handling.src/components/home/__tests__/cert-preview-location-icon.test.tsx (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing the fixture.
Annotating with
satisfies ActivityHomeFeedViewmakes this test fail at typecheck if the view contract changes, instead of silently drifting.🤖 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 `@src/components/home/__tests__/cert-preview-location-icon.test.tsx` around lines 22 - 31, Type the view fixture in the cert preview test using satisfies ActivityHomeFeedView before passing it to CertPreview. Keep the existing fixture values and render call unchanged, ensuring contract changes cause a typecheck failure.src/lib/utils/__tests__/group-feed.test.ts (1)
57-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a three-endorsement case.
With only two events the test exercises the single→group promotion branch; the append-into-existing-group branch (
last.subjects.push) ingroupConsecutiveEndorsementsstays uncovered. A third consecutive endorsement from the same actor would cover it and pin subject ordering.🤖 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 `@src/lib/utils/__tests__/group-feed.test.ts` around lines 57 - 72, Extend the test case for groupConsecutiveEndorsements with a third consecutive endorsement from the same actor, then assert the resulting single group contains all three subjects in input order. Preserve the existing hydration expectations and verify the append-into-existing-group behavior through the third subject.src/components/home/home-feed-rows.tsx (2)
185-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the nested
setVisibleCountout of thesetExpandedupdater.State updaters must be pure; React may invoke them more than once. Here the reset happens to be idempotent, so no visible bug, but reading
expandeddirectly keeps the updater clean.♻️ Proposed refactor
- onClick={() => - setExpanded((current) => { - if (current) setVisibleCount(GROUP_EXPAND_PAGE) - return !current - }) - } + onClick={() => { + if (expanded) setVisibleCount(GROUP_EXPAND_PAGE) + setExpanded(!expanded) + }}🤖 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 `@src/components/home/home-feed-rows.tsx` around lines 185 - 193, In the Button onClick handler, move the GROUP_EXPAND_PAGE reset out of the setExpanded updater and condition it on the current expanded value before calling setExpanded. Keep the updater responsible only for returning the toggled state, using the existing expanded state to preserve the current behavior.Source: Linters/SAST tools
57-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one
useResolvedActorhook for the legacy identity fallback. Four wrapper components repeat the sameuseAuthorInfo(did)call and the sameinfo?.x || actor.x || nullmerge, so the precedence rules are copied in four places and can drift independently.
src/components/home/home-feed-rows.tsx#L57-L70: replaceLegacyFeedCardHead's inline merge withuseResolvedActor(props.actorProfile)and pass the result asactorProfile.src/components/home/home-feed-rows.tsx#L255-L274: replaceLegacyEndorsementGroupSummary's merge withuseResolvedActor(first).src/components/home/home-feed-rows.tsx#L307-L319: replaceLegacyEndorsedAccountLink's merge withuseResolvedActor(subject).src/components/home/home-feed-rows.tsx#L535-L547: replaceLegacyEndorsementSentence's merge withuseResolvedActor(subject).Suggested helper:
function useResolvedActor(actor: HomeFeedActor): HomeFeedActor { const { info } = useAuthorInfo(actor.did) return { ...actor, handle: info?.handle || actor.handle || null, displayName: info?.displayName || actor.displayName || null, avatarUrl: info?.avatarUrl || actor.avatarUrl || null, } }Note
LegacyFeedCardHeadcurrently resolvesprops.actorwhile the others resolve the profile'sdid; keeping one helper also settles that inconsistency.🤖 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 `@src/components/home/home-feed-rows.tsx` around lines 57 - 70, Extract a shared useResolvedActor hook that resolves actor identity fields using useAuthorInfo(actor.did) with the existing info, actor, null precedence. In src/components/home/home-feed-rows.tsx lines 57-70, replace LegacyFeedCardHead’s inline merge with useResolvedActor(props.actorProfile); apply the same replacement with useResolvedActor(first) in lines 255-274, useResolvedActor(subject) in lines 307-319, and useResolvedActor(subject) in lines 535-547, passing each resolved value as actorProfile or the corresponding actor prop.src/components/home/__tests__/home-feed-empty-pagination.test.tsx (1)
45-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded auto-load budget.
The 25/26 magic numbers mirror
MAX_AUTO_LOADSinhome-feed.tsx. Exporting and importing that constant would keep this test honest if the budget changes.🤖 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 `@src/components/home/__tests__/home-feed-empty-pagination.test.tsx` around lines 45 - 55, Replace the hardcoded 25/26 auto-load counts in the test around HomeFeedBody with the exported MAX_AUTO_LOADS constant from home-feed.tsx. Use the constant for the loop bound and expected loadMore call counts, preserving the existing manual-button assertion behavior.
🤖 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 `@src/components/home/home-feed.tsx`:
- Around line 190-201: Remove the unsafe OrganizationQuality[] cast in the
organizationQuality useMemo. Define or reuse a shared input type for allowed
organization tiers, or filter with a type-narrowing predicate, so
organizationQuality.allowed can only contain values from ORGLABEL_TIERS while
preserving the existing includeUnrated behavior before passing it to
useHomeFeed.
In `@src/lib/atproto/certified-feed.ts`:
- Around line 236-243: Update fetchCertifiedFeed’s feed-service fetch to enforce
an internal timeout in addition to options.signal, composing the signals with
supported AbortSignal APIs and providing a compatible fallback where necessary.
Ensure timeout aborts reject through the existing error/retry flow while
preserving caller cancellation behavior.
---
Nitpick comments:
In `@src/components/home/__tests__/cert-preview-location-icon.test.tsx`:
- Around line 22-31: Type the view fixture in the cert preview test using
satisfies ActivityHomeFeedView before passing it to CertPreview. Keep the
existing fixture values and render call unchanged, ensuring contract changes
cause a typecheck failure.
In `@src/components/home/__tests__/home-feed-empty-pagination.test.tsx`:
- Around line 45-55: Replace the hardcoded 25/26 auto-load counts in the test
around HomeFeedBody with the exported MAX_AUTO_LOADS constant from
home-feed.tsx. Use the constant for the loop bound and expected loadMore call
counts, preserving the existing manual-button assertion behavior.
In `@src/components/home/home-feed-rows.tsx`:
- Around line 185-193: In the Button onClick handler, move the GROUP_EXPAND_PAGE
reset out of the setExpanded updater and condition it on the current expanded
value before calling setExpanded. Keep the updater responsible only for
returning the toggled state, using the existing expanded state to preserve the
current behavior.
- Around line 57-70: Extract a shared useResolvedActor hook that resolves actor
identity fields using useAuthorInfo(actor.did) with the existing info, actor,
null precedence. In src/components/home/home-feed-rows.tsx lines 57-70, replace
LegacyFeedCardHead’s inline merge with useResolvedActor(props.actorProfile);
apply the same replacement with useResolvedActor(first) in lines 255-274,
useResolvedActor(subject) in lines 307-319, and useResolvedActor(subject) in
lines 535-547, passing each resolved value as actorProfile or the corresponding
actor prop.
In `@src/components/home/home-feed.tsx`:
- Around line 104-111: Remove the redundant Escape-key useEffect and its
document listener from the filter-open handling, leaving useClickOutsideClose as
the sole Escape-close path. Preserve the existing filterOpen state behavior and
click-outside handling.
In `@src/hooks/use-home-feed.ts`:
- Around line 172-176: The refs used by the home-feed callbacks are mutated
during render, violating the configured no-ref-current-in-render rule. In both
the primary hook and useLegacyHomeFeed, move stateRef, currentRequestKeyRef, and
followedRef synchronization into a layout or insertion effect, preserving the
latest-snapshot behavior for loadMore and retryInitial; also verify whether this
rule is enforced in CI.
In `@src/lib/atproto/certified-feed.ts`:
- Around line 263-265: Update the response-body handling around response.text()
and the MAX_ERROR_BODY guard to enforce the size limit while reading the
response stream, before the full payload is buffered. Track accumulated bytes or
characters, abort and throw contractError once the limit is exceeded, then
decode the bounded content for the existing error-processing flow.
In `@src/lib/dev/fixtures/feed.ts`:
- Around line 112-124: Update certifiedFeedPage’s feedActorBlock output to match
parseActor’s certified-feed contract by emitting the supported avatar object
under avatar, or omit the field when no avatar exists; do not emit avatarCid,
which parseActor ignores. Preserve null-avatar behavior for current MOCK_ACTORS
entries while ensuring future actors with avatars serialize in the same shape as
real payloads.
In `@src/lib/utils/__tests__/group-feed.test.ts`:
- Around line 57-72: Extend the test case for groupConsecutiveEndorsements with
a third consecutive endorsement from the same actor, then assert the resulting
single group contains all three subjects in input order. Preserve the existing
hydration expectations and verify the append-into-existing-group behavior
through the third subject.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 946af034-fbc8-4ae1-a7b2-659a962d55d1
📒 Files selected for processing (23)
.env.local.exampleAGENTS.mdREADME.mdsrc/components/dev/mock-fetch-provider.tsxsrc/components/home/__tests__/cert-preview-location-icon.test.tsxsrc/components/home/__tests__/endorsement-group-row.test.tsxsrc/components/home/__tests__/home-feed-empty-pagination.test.tsxsrc/components/home/__tests__/home-feed-indexer-source-isolation.test.tsxsrc/components/home/__tests__/home-feed-service-render.test.tsxsrc/components/home/__tests__/home-feed-source-isolation.test.tsxsrc/components/home/home-feed-rows.tsxsrc/components/home/home-feed.tsxsrc/components/layout/__tests__/desktop-top-bar-hydration.test.tsxsrc/components/layout/desktop-top-bar.tsxsrc/hooks/__tests__/use-home-feed-invalid-cursor.test.tsxsrc/hooks/__tests__/use-home-feed-loadmore-abort.test.tsxsrc/hooks/__tests__/use-home-feed-service.test.tsxsrc/hooks/use-home-feed.tssrc/lib/atproto/__tests__/certified-feed.test.tssrc/lib/atproto/certified-feed.tssrc/lib/dev/fixtures/feed.tssrc/lib/utils/__tests__/group-feed.test.tssrc/lib/utils/group-feed.ts
Add the typed public XRPC client, service-backed home-feed hook and rendering path, guarded indexer rollback mode, configuration docs, and regression coverage for pagination and source isolation. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Keep the height-reserving top-bar placeholder through the first client snapshot so auth updates cannot race a deferred Suspense boundary. Continue suppressing app chrome entirely on editorial and embed routes. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Keep the PR focused on implementation and public configuration documentation. Co-Authored-By: OpenAI Codex <noreply@openai.com>
6bf6aae to
09d3aa8
Compare
Add bounded timeout-aware feed reads, preserve caller cancellation, align service fixtures with the wire contract, and remove React render/update hazards. Extend regression coverage for transport limits and endorsement grouping. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
(reply generated by OpenAI Codex) Addressed the CodeRabbit review in
Left the pagination test’s literal Validated with 1,267 passing tests, production and test typechecks, ESLint, and a successful production build. |
Translate the home-feed request and response at the transport boundary while preserving the existing UI event model. Handle URI-only subjects and omitted service timestamps safely. Co-Authored-By: OpenAI Codex <noreply@openai.com>
1 participant
Summary
app.certified.feed.beta.getFeedXRPCDesktopTopBarhydration when auth resolves before its Suspense boundaryRollout
The source remains
indexerby default. Enabling the service path requires:The app origin must also be present in the feed service's exact CORS allowlist. Changing either source requires a rebuild/redeploy.
Breaking changes
None. The legacy indexer transport remains available through the observation window.
Out of scope
Test plan
npm run typechecknpm run typecheck:testnpx eslint src/ --ext .ts,.tsxnpm test— 149 files, 1,226 tests passednpm run buildSummary by CodeRabbit