Conversation
- Drop the public/private membership split on /groups: a single sorted
list now shows every group the user belongs to.
- Remove the per-row Accept (UserCheck) and Remove-public (UserX)
buttons and their handlers.
- Surface the joinedAt timestamp from the group service on each row
("Joined Mon DD, YYYY"). Plumbed through Group type and resolveGroups.
- Drop the now-unused .org-list__divider/__accept-btn/__remove-public-btn
styles and add a small .org-list__item-meta style for the date line.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a dropdown above the group list with four sort modes: joined oldest first (default), joined newest first, name A→Z, name Z→A. Rows missing joinedAt always sort to the bottom of joined-* views. The control is hidden when there are 0 or 1 groups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the labeled sort dropdown below the description with a single ArrowUpDown icon next to the group count. The native select sits transparently on top of the icon so clicking it opens the OS menu with the four sort options. Current mode is reflected in the button's title attribute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Accepted items from docs/groups-list-improvements/review-round-1.md: - Sort comparator: drop redundant lowercasing, use locale-aware case-insensitive comparison (sensitivity: "base"). - Decorate-sort-undecorate: parse joinedAt once per group instead of on every comparison, and reuse the display label. - aria-hidden -> aria-hidden="true" on the icon for codebase consistency. - Validate the SortMode value in the select onChange against the hardcoded option set before calling setState. - Move the title attribute onto the <select> (so keyboard users see the tooltip) and surface the current mode in aria-label so screen readers announce it. - Replace the dead-end "Owners can't leave the group" tooltip with actionable guidance pointing at group settings. - Extract a local displayLabel const inside renderOrgItem to remove six repetitions of "displayName || handle". Rejected items recorded with rationale in the same doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both round-2 reviewers returned ship verdicts. Accepted items (see docs/groups-list-improvements/review-round-2.md): - Drop the "current: <label>" suffix from the <select>'s aria-label; native screen readers already announce the selected option text after the role, making the suffix duplicate the announcement. Title attribute kept for sighted keyboard/mouse users. - Update stale "lowercased label" comment in sortedOrgs and add a one-liner noting that ES2019 sort stability handles equal-key ties. - Extract isSortMode type predicate for the onChange validator; widen SORT_VALUES to ReadonlySet<string> so the predicate input is plain string and the two `as SortMode` casts go away. Rejected items (with rationale) recorded in the same doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feat(groups): unify list, show join date, add sort control
Previously the profile display rendered an empty 180px-tall grey gradient placeholder when the user had no banner. Now the entire .profile-card__banner block is omitted in that case. The edit-profile page is unchanged — its banner upload affordance still surfaces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The xrpc proxy collapses every status >= 500 to "Internal server error" and the OAuth restore catch silently turns every failure into "Session expired". Both branches dropped the original error on the floor, leaving us nothing to diagnose with when a user hits a save failure. Add console.error in both spots so the real upstream cause (XRPC status, name, message, cause, stack) lands in the Vercel logs while the client-facing message stays generic. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… on bsky PDS Saves to repos hosted on bsky.social were failing with `Error: expected non-null body source` when the PDS responded 401 + DPoP-Nonce on the first request and the OAuth client retried without a re-readable body. 0.6.1 reworks the DPoP fetch path so the body survives the retry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sky PDS The DPoP-nonce retry against bsky.social was crashing with "expected non-null body source" because @atproto/api 0.13 brought in @atproto/xrpc 0.6, which builds a Request with a one-shot body that can't survive the second attempt. 0.19 uses xrpc 0.7, which keeps the body re-sendable. Also drops the deprecated rkeyEnd/rkeyStart params from the listRecords proxy and routes the per-method body casts through `unknown` to satisfy the stricter input schemas. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vercel/next.js#90826: on Node ≥ 24.14, the patched fetch crashes with "expected non-null body source" when given a Request whose body has been consumed and the response is an error. The atproto DPoP wrapper passes a Request to fetch, and bsky's PDS reliably returns 401 + DPoP-Nonce on the first hit, so every putRecord against bsky.social was failing. Buffer the body and re-issue with (url, init) form before Next.js's wrapper sees it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements group list sorting improvements, refactors the groups page UI to show joined dates with unified rendering, enhances OAuth error handling via request-body-aware fetch, improves xRPC error logging and type casting, fixes profile banner rendering, and updates the ChangesGroups List Improvements
Auth Infrastructure & Supporting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/app/groups/page.tsx (1)
13-24: 💤 Low valueConsider deferring date locale to the user's runtime locale.
date.toLocaleDateString("en-US", JOINED_DATE_FORMAT)always renders dates inMon DD, YYYYform regardless of where the user is. Passingundefined(or omitting the argument) letsIntl.DateTimeFormatpick up the browser/runtime locale, which keeps the rest of the field structure (year/month/day) but formats per the user's expectations (e.g.,15 Jan 2024in en-GB,15. Jan. 2024in de). If en-US is a deliberate product choice for consistency with the rest of the UI, ignore.♻️ Proposed change
- return date.toLocaleDateString("en-US", JOINED_DATE_FORMAT) + return date.toLocaleDateString(undefined, JOINED_DATE_FORMAT)🤖 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/app/groups/page.tsx` around lines 13 - 24, The formatJoinedDate helper currently forces the "en-US" locale when calling date.toLocaleDateString, which prevents using the user's runtime/browser locale; update formatJoinedDate to call date.toLocaleDateString without the explicit "en-US" argument (i.e., pass only JOINED_DATE_FORMAT or undefined for the locale) so Intl.DateTimeFormat uses the user's locale, keeping the JOINED_DATE_FORMAT structure; leave the constant JOINED_DATE_FORMAT and validation in place and only change the toLocaleDateString call in formatJoinedDate (unless "en-US" is intentionally required).src/lib/auth/oauth-client.ts (1)
133-165: 💤 Low value
safeFetchis correctly implemented; TDZ concern is not an issue in practice.The wrapper properly converts
Request→(url, init)to work around vercel/next.js#90826, and the...initspread last preserves standard fetch precedence. Good comment explaining the bug.The module-level
const safeFetchis declared aftergetOAuthClientreferences it, which initially suggests a potential temporal dead zone (TDZ) issue. However, this is not a problem:@atproto/oauth-client-node's constructor does not call fetch synchronously, so it won't try to access thefetchoption during construction. The const binding is fully established during module initialization beforegetOAuthClientis ever invoked at runtime.Minor note:
body: buffer && buffer.byteLength > 0 ? buffer : undefinedsilently converts an explicit zero-length body into a no-body request. Unlikely to affect OAuth/DPoP flows (bodies are non-empty form data) but technically a semantic shift; acceptable given the workaround context.As a purely optional improvement for code clarity,
safeFetchcould be declared as afunction(which benefits from declaration hoisting) and moved beforegetOAuthClient. This eliminates any reader confusion about module evaluation order, but the current code is functionally safe.🤖 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/auth/oauth-client.ts` around lines 133 - 165, Move the safeFetch implementation before getOAuthClient and convert the module-level const safeFetch into a hoisted function declaration (keep the same parameter types/behavior and use globalThis.fetch) so readers won't worry about a temporal-dead-zone; update any references to safeFetch (used when constructing NodeOAuthClient in getOAuthClient) to call the new function name (safeFetch) — keep the Request → (url, init) buffering logic and the final ...init precedence unchanged.
🤖 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 `@package.json`:
- Line 12: Update the version guidance in AGENTS.md to reflect the bumped
`@atproto/api` major/minor (0.19.x) used in package.json instead of the old 0.13
pin: locate the guideline entry that pins `@atproto/api` to 0.13 (around the
section that also references `@atproto/oauth-client-node` and `@atproto/jwk-jose`)
and change the text to indicate `@atproto/api` 0.19.x is allowed/used in this
repo; also add a short note clarifying that `@atproto/oauth-client` (browser) is
at 0.6.1 transitively while `@atproto/oauth-client-node` remains pinned at 0.3.17
as per the existing guideline.
In `@src/app/api/xrpc/`[...method]/route.ts:
- Line 208: The handler currently double-casts request bodies to types like
ComAtprotoRepoCreateRecord.InputSchema (e.g., where body is cast before calling
agent.com.atproto.repo.createRecord) which bypasses runtime checks; add runtime
schema validation (for each case: com.atproto.repo.createRecord and the other
listed cases at lines referenced) using a validator such as Zod or AJV that
mirrors the InputSchema shape, call safeParse/validate on the raw body, return a
400 JSON response on validation failure, and only then pass the validated data
(parsed.data) into the SDK call instead of the direct double-cast.
In `@src/components/profile/profile-client.tsx`:
- Around line 137-141: The banner image in the ProfileClient component is
decorative and currently has alt="", so add aria-hidden="true" to the <img>
element rendering the banner (the image inside the JSX block that checks
bannerUrl in profile-client.tsx) so screen readers skip it; locate the JSX
fragment with {bannerUrl && (<div className="profile-card__banner"><img ...
/></div>)} and add the aria-hidden="true" attribute to that img element.
---
Nitpick comments:
In `@src/app/groups/page.tsx`:
- Around line 13-24: The formatJoinedDate helper currently forces the "en-US"
locale when calling date.toLocaleDateString, which prevents using the user's
runtime/browser locale; update formatJoinedDate to call date.toLocaleDateString
without the explicit "en-US" argument (i.e., pass only JOINED_DATE_FORMAT or
undefined for the locale) so Intl.DateTimeFormat uses the user's locale, keeping
the JOINED_DATE_FORMAT structure; leave the constant JOINED_DATE_FORMAT and
validation in place and only change the toLocaleDateString call in
formatJoinedDate (unless "en-US" is intentionally required).
In `@src/lib/auth/oauth-client.ts`:
- Around line 133-165: Move the safeFetch implementation before getOAuthClient
and convert the module-level const safeFetch into a hoisted function declaration
(keep the same parameter types/behavior and use globalThis.fetch) so readers
won't worry about a temporal-dead-zone; update any references to safeFetch (used
when constructing NodeOAuthClient in getOAuthClient) to call the new function
name (safeFetch) — keep the Request → (url, init) buffering logic and the final
...init precedence unchanged.
🪄 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
Run ID: 5478d940-7547-4aec-a039-e931c7b3419a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
docs/groups-list-improvements/review-round-1.mddocs/groups-list-improvements/review-round-2.mdpackage.jsonsrc/app/api/xrpc/[...method]/route.tssrc/app/globals.csssrc/app/groups/page.tsxsrc/components/profile/profile-client.tsxsrc/lib/auth/oauth-client.tssrc/lib/groups/api.tssrc/lib/groups/types.ts
| case "com.atproto.repo.createRecord": { | ||
| const result = await agent.com.atproto.repo.createRecord( | ||
| body as ComAtprotoRepoCreateRecord.InputSchema | ||
| body as unknown as ComAtprotoRepoCreateRecord.InputSchema |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Consider adding runtime schema validation before double-cast.
The change to body as unknown as <InputSchema> bypasses TypeScript's type checking. While the code includes some field-level validation (repo, collection allowlist), there's no comprehensive schema validation ensuring the body matches the expected InputSchema shape before passing it to the @atproto/api SDK.
Risk: Malformed payloads could reach the SDK, potentially causing runtime errors or unexpected behavior.
Recommendation: Consider using a runtime validation library (e.g., Zod) to validate the body shape against the expected schema, or document why the double-cast is necessary (e.g., if @atproto/api SDK performs its own validation).
Example with Zod validation
// At the top of the file, define schemas matching InputSchema shapes
import { z } from 'zod'
const createRecordSchema = z.object({
repo: z.string(),
collection: z.string(),
rkey: z.string().optional(),
validate: z.boolean().optional(),
record: z.record(z.unknown()),
// ... other fields from ComAtprotoRepoCreateRecord.InputSchema
})
// In the handler
case "com.atproto.repo.createRecord": {
const parsed = createRecordSchema.safeParse(body)
if (!parsed.success) {
return NextResponse.json(
{ error: "Invalid request body" },
{ status: 400 }
)
}
const result = await agent.com.atproto.repo.createRecord(
parsed.data as ComAtprotoRepoCreateRecord.InputSchema
)
return NextResponse.json(result.data)
}Also applies to: 214-214, 220-220, 258-258, 265-265, 271-271, 281-281
🤖 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/app/api/xrpc/`[...method]/route.ts at line 208, The handler currently
double-casts request bodies to types like ComAtprotoRepoCreateRecord.InputSchema
(e.g., where body is cast before calling agent.com.atproto.repo.createRecord)
which bypasses runtime checks; add runtime schema validation (for each case:
com.atproto.repo.createRecord and the other listed cases at lines referenced)
using a validator such as Zod or AJV that mirrors the InputSchema shape, call
safeParse/validate on the raw body, return a 400 JSON response on validation
failure, and only then pass the validated data (parsed.data) into the SDK call
instead of the direct double-cast.
Automated review (3 agents, parallel)Must address before merging to main🔴 Token leakage in upstream-error logging — flagged by 2 of 3 reviewers, and we have direct evidence from today's runtime logs that this is happening.
Recommended: drop If the team treats Vercel logs as confidential and is comfortable with that posture, document it in a comment so the next reviewer doesn't re-flag. Should consider —
|
…eview nits Review feedback from PR #54. - xrpc proxy: drop `err.cause` and `err.stack` from `[xrpc] upstream error` logs and route the message through a `redactSecrets` helper that strips JWTs (covers DPoP proofs and bearer tokens), `Authorization`/`DPoP`/`Cookie` header lines, `access_token`/`refresh_token`/`id_token` query/body params, and email addresses. The bare `err` previously logged at oauth-restore failure now goes through the same helper. Confirmed against a sample containing a real DPoP JWT, an email, and an `access_token=` param. - safeFetch: preserve "had a body" instead of "had bytes" so a legitimate zero-byte POST/PUT keeps `Content-Length: 0`. Drop the `...init` spread since the dpop wrapper never passes a second argument and a future caller passing `init.body` would re-introduce the bug we're fixing. Comment the uploadBlob double-buffer so future readers don't try to "fix" it. - AGENTS.md: bump the documented `@atproto/api` line to 0.19, note that `@atproto/oauth-client` 0.6 is pulled in transitively. - Banner img: add `aria-hidden="true"` since it's decorative (alt=""). - Groups join-date: drop the forced "en-US" locale from `toLocaleDateString` so it follows the user's runtime locale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
Brings 11 commits from
stagingintomain. Three themes:*.host.bsky.network(i.e. any Bluesky-hosted handle) were 500ing withexpected non-null body source. Root cause is vercel/next.js#90826 (Node ≥ 24.14): Next.js's patched fetch re-reads a Request body for tracing on error responses, but undici has tightened stream locking. The atproto OAuth DPoP wrapper passes a Request to fetch, and bsky's PDS reliably returns 401 + DPoP-Nonce on the first hit, so every authenticated write to a bsky-side PDS triggered the bug. Fixed by wrapping thefetchpassed toNodeOAuthClientso any incoming Request is deconstructed to (url, init) before reaching Next.js's wrapper. Also bumped@atproto/api0.13 → 0.19 and@atproto/oauth-clientto 0.6.1 (and dropped the now-deprecatedrkeyEnd/rkeyStartfrom the listRecords proxy).Internal server error(this is what made the bsky-PDS bug above debuggable in the first place).A sibling-repo issue tracking the same bsky-PDS bug for
certs-socialis filed at hypercerts-org/certs-social#86 — the recommended fix there is the identicalsafeFetchwrapper validated here.Test plan
holke.xyz, DID resolves toamanita.us-east.host.bsky.network) onstaging.certified.app; edit profile and save →POST /api/xrpc/com/atproto/repo/putRecordreturns 200, redirect to/profile/[did], saved values render. Verified end-to-end before this PR was opened.npx tsc --noEmit).mainhas all the same env vars staging has (COOKIE_SECRET, PUBLIC_URL, UPSTASH_*, etc.) — production is on a separate Vercel environment and won't inherit from staging.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores