Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a new PDS landing page at /pds (app/pages/pds.vue) with SEO/OpenGraph setup and a client-side avatar gallery. Introduces server APIs to fetch PDS user profiles (cached, batched requests) and to build a graph of PDS repos and follows, plus a lightweight AtprotoProfile type and new constants for XRPC endpoints. Registers /pds for static prerendering and includes it in canonical redirects. Server handlers batch actor requests in groups of 25 and perform resilient error handling/logging. Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ 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 |
![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 (1)
app/pages/pds.vue (1)
48-48: Drop inline buttonfocus-visibleutility and rely on global focus styling.Line 48 adds
focus-visible:outline-accent/70on a<button>, but this project already standardises button focus-visible styles globally inapp/assets/main.css.Suggested patch
- class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings: "focus-visible styling for button and select elements is implemented globally in app/assets/main.css ... Do not apply per-element inline utility classes ... Rely on the global rule for consistency and maintainability."
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/pages/pds.vuenuxt.config.tsserver/api/atproto/pds-graphs.get.tsserver/api/atproto/pds-users.get.tsserver/middleware/canonical-redirects.global.ts
| for (const did of dids) { | ||
| const followResponse = await fetch( | ||
| `https://public.api.bsky.app/xrpc/app.bsky.graph.getFollows?actor=${did}`, | ||
| ) | ||
|
|
||
| if (!followResponse.ok) { | ||
| console.warn(`Failed to fetch atproto profiles: ${followResponse.status}`) | ||
| continue | ||
| } | ||
|
|
||
| const followData = await followResponse.json() | ||
|
|
||
| for (const followedUser of followData.follows) { | ||
| if (localDids.has(followedUser.did)) { | ||
| links.push({ source: did, target: followedUser.did }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'pds-graphs.get.ts' | head -n1)"
if [ -z "$file" ]; then
echo "File not found"
exit 1
fi
echo "=== File: $file ==="
wc -l "$file"
echo
echo "=== Lines 45-110 (context around 53-70) ==="
nl -ba "$file" | sed -n '45,110p'
echo
echo "=== Loop/fetch/error-handling markers ==="
rg -n "for \(const did|await fetch|try \{|catch \(" "$file"
echo
echo "=== Function signature and outer context (lines 1-52) ==="
nl -ba "$file" | sed -n '1,52p'Repository: npmx-dev/npmx.dev
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'pds-graphs.get.ts' | head -n1)"
if [ -z "$file" ]; then
echo "File not found"
exit 1
fi
echo "=== File: $file ==="
wc -l "$file"
echo
echo "=== Lines 45-110 (context around 53-70) ==="
cat -n "$file" | sed -n '45,110p'
echo
echo "=== Loop/fetch/error-handling markers ==="
rg -n "for \(const did|await fetch|try \{|catch \(" "$file"
echo
echo "=== Full file content ==="
cat -n "$file"Repository: npmx-dev/npmx.dev
Length of output: 4361
Follow-edge collection lacks consistent error handling and is vulnerable to runtime failures.
Lines 53-70 violate the coding guideline "Use error handling patterns consistently". Unlike the profiles batch loop above (lines 35-50, which wraps fetch/json in try/catch), the follows loop has no error handling wrapper. The await followResponse.json() call on line 63 can throw unhandled errors if the response body is invalid, crashing the entire endpoint. Additionally, line 65 accesses followData.follows without verifying it exists, violating strict type-safety requirements. The sequential per-DID approach also creates unnecessary latency compared to the batched pattern used for profiles.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/api/atproto/pds-users.get.ts (1)
13-13:⚠️ Potential issue | 🟠 MajorAdd timeouts to outbound fetches.
Line 13 and Line 35 perform blocking network calls without an abort signal; slow upstream responses can tie up server capacity.
🩹 Suggested patch
+const REQUEST_TIMEOUT_MS = 10_000 + export default defineCachedEventHandler( async (): Promise<AtprotoProfile[]> => { @@ - const response = await fetch(ONE_THOUSAND_NPMX_USER_ACCOUNTS_XRPC) + const response = await fetch(ONE_THOUSAND_NPMX_USER_ACCOUNTS_XRPC, { + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), + }) @@ - fetch(url.toString()) + fetch(url.toString(), { + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), + })#!/bin/bash set -euo pipefail file="server/api/atproto/pds-users.get.ts" echo "Inspecting fetch callsites and timeout usage in $file" rg -n '\bfetch\s*\(' "$file" rg -n 'AbortSignal\.timeout|signal\s*:' "$file" || trueAlso applies to: 35-35
app/pages/pds.vue (1)
2-2:⚠️ Potential issue | 🔴 CriticalFix the type import source to resolve the type-check failure.
Line 2 imports a named export that is not provided by the server route module.
🩹 Suggested patch
-import type { AtprotoProfile } from '#server/api/atproto/pds-users.get.ts' +import type { AtprotoProfile } from '#shared/types/atproto'#!/bin/bash set -euo pipefail echo "Verify AtprotoProfile export source and current import usage" rg -n 'export type AtprotoProfile' shared/types/atproto.ts rg -n 'import type \{ AtprotoProfile \}' app/pages/pds.vue rg -n 'AtprotoProfile' server/api/atproto/pds-users.get.ts
🧹 Nitpick comments (3)
shared/utils/constants.ts (1)
19-20: Derive the AppView URL fromBLUESKY_APIto avoid origin drift.Small DRY improvement: keep a single source of truth for the base URL.
♻️ Suggested patch
export const BSKY_APP_VIEW_USER_PROFILES_XRPC = - 'https://public.api.bsky.app/xrpc/app.bsky.actor.getProfiles' + `${BLUESKY_API}/xrpc/app.bsky.actor.getProfiles`server/api/atproto/pds-users.get.ts (1)
22-23: Avoid unchecked JSON casts for untrusted payloads.The direct cast can crash at runtime if
reposis missing or malformed. Prefer guarded parsing before mapping DIDs.🛡️ Suggested patch
- const listRepos = (await response.json()) as { repos: { did: string }[] } - const dids = listRepos.repos.map(repo => repo.did) + const payload: unknown = await response.json() + const repos = Array.isArray((payload as { repos?: unknown }).repos) + ? (payload as { repos: Array<{ did?: unknown }> }).repos + : [] + const dids = repos.flatMap(repo => (typeof repo.did === 'string' ? [repo.did] : []))As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
app/pages/pds.vue (1)
48-48: Remove per-elementfocus-visibleutility from the button class.This should rely on the global
button:focus-visiblerule for consistency across the app.♻️ Suggested patch
- class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings: "focus-visible styling for button and select elements is implemented globally in app/assets/main.css ... Do not apply per-element inline utility classes".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/pages/pds.vueserver/api/atproto/pds-users.get.tsshared/types/atproto.tsshared/utils/constants.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/pages/pds.vue (2)
46-54: Remove inlinefocus-visibleutility and addaria-labelfor accessibility.Two issues with this button:
The
focus-visible:outline-accent/70class should be removed—focus-visible styling for buttons is handled globally viamain.css.On smaller screens, the text is hidden (
hidden sm:inline) leaving only the icon visible. Without anaria-label, screen reader users have no context for what this button does.♻️ Suggested fix
<button type="button" - class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0" `@click`="router.back()" v-if="canGoBack" + aria-label="Go back" >Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css… individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
129-160: Consider adding a fallback for an empty community list.The current logic handles
pendinganderrorstates well, but if the fetch succeeds andusersWithAvatarsis empty (e.g., no users have avatars or all images fail to load), nothing is rendered. A brief message would improve the UX.♻️ Suggested addition after the `ul` block
</ul> + <p v-else class="text-fg-subtle text-sm"> + No community members to display yet. + </p> </div>
fatfingers23
left a comment
fatfingers23
left a comment
There was a problem hiding this comment.
Looking good! Added a few comments, mostly just using atproto/lex client in more places so we have the types/validation already and are not relying on fetch and creating our own types
| @@ -0,0 +1,76 @@ | |||
| import type { AtprotoProfile } from '~~/server/api/atproto/pds-users.get' | |||
| export default defineCachedEventHandler( | ||
| async (): Promise<AtprotoProfile[]> => { | ||
| // INFO: Request npmx.social PDS for every hosted user account | ||
| const response = await fetch(ONE_THOUSAND_NPMX_USER_ACCOUNTS_XRPC) |
There was a problem hiding this comment.
Might not hurt to go ahead and do this in a loop too for when we get over 1k users. Can also use the Client in atproto/lex for these XRPC calls. This client has all the types and validation baked in
https://npmx.dev/package/@atproto/lex#user-content-making-simple-xrpc-requests
|
|
||
| for (let i = 0; i < dids.length; i += USER_BATCH_AMOUNT) { | ||
| const batch = dids.slice(i, i + USER_BATCH_AMOUNT) | ||
| const url = new URL(BSKY_APP_VIEW_USER_PROFILES_XRPC) |
There was a problem hiding this comment.
This can use a client as well
| <h2 id="community-heading" class="text-lg text-fg uppercase tracking-wider mb-4"> | ||
| Who is here | ||
| </h2> | ||
| <p class="text-fg-muted leading-relaxed mb-6"> |
There was a problem hiding this comment.
Be nice to add a count like "Home to 192 accounts" or something and the count is only active accounts
| Failed to load PDS community. | ||
| </div> | ||
| <ul | ||
| v-else-if="usersWithAvatars.length" |
There was a problem hiding this comment.
May also be fun to do something here where if they do not have an avatar to show just the npmx logo and a count of how many that is beside it to show how many are new to the atmosphere accounts
3 participants
🔗 Linked issue
resolves #1646
🧭 Context
This PR inherits the original work from #1742 by @pauliecodes. It will also include some minor cleanups and additions.
Work Completed Prior to Handover
/pds📚 Description
State of the page at handoff: