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, client-side lazy fetching of PDS user profiles from /api/atproto/pds-users, avatar filtering and broken-image handling, and a conditional back button. Introduces two server endpoints: server/api/atproto/pds-users.get.ts (cached, batched profile fetches) and server/api/atproto/pds-graphs.get.ts (repos → profiles → follow links, batched and resilient). Adds type AtprotoProfile, new XRPC and error constants, registers /pds for prerendering, and updates canonical redirects. 🚥 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
server/api/atproto/pds-graphs.get.ts
Outdated
| @@ -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
app/pages/pds.vue
Outdated
| 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shared/utils/constants.ts (1)
20-21: Avoid hard-coding the Bluesky host twice.
BSKY_APP_VIEW_USER_PROFILES_XRPCrepeats the same base URL asBLUESKY_API, which increases drift risk when endpoints change. Compose it from the existing constant.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`app/pages/pds.vue (1)
48-48: Remove per-button focus-visible utility class.This button should rely on the global button focus-visible rule rather than an inline utility class.
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:
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }is defined globally inapp/assets/main.css, and per-element focus-visible utilities should not be used for buttons/selects.
ℹ️ 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/middleware/canonical-redirects.global.tsshared/utils/constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/middleware/canonical-redirects.global.ts
- server/api/atproto/pds-graphs.get.ts
- nuxt.config.ts
| <span | ||
| class="pointer-events-none absolute -top-9 inset-is-1/2 -translate-x-1/2 whitespace-nowrap rounded-md bg-gray-900 text-white dark:bg-gray-100 dark:text-gray-900 text-xs px-2 py-1 shadow-lg opacity-0 scale-95 transition-all duration-150 group-hover:opacity-100 group-hover:scale-100 z-10" | ||
| dir="ltr" | ||
| role="tooltip" | ||
| > |
There was a problem hiding this comment.
Tooltip semantics are incomplete for keyboard users.
role="tooltip" is declared, but the element is only shown on hover and is not wired to a trigger via aria-describedby. Either fully wire ARIA tooltip behaviour or treat this as visual-only text and support keyboard visibility.
Suggested patch
- class="pointer-events-none absolute -top-9 inset-is-1/2 -translate-x-1/2 whitespace-nowrap rounded-md bg-gray-900 text-white dark:bg-gray-100 dark:text-gray-900 text-xs px-2 py-1 shadow-lg opacity-0 scale-95 transition-all duration-150 group-hover:opacity-100 group-hover:scale-100 z-10"
+ class="pointer-events-none absolute -top-9 inset-is-1/2 -translate-x-1/2 whitespace-nowrap rounded-md bg-gray-900 text-white dark:bg-gray-100 dark:text-gray-900 text-xs px-2 py-1 shadow-lg opacity-0 scale-95 transition-all duration-150 group-hover:opacity-100 group-hover:scale-100 group-focus-within:opacity-100 group-focus-within:scale-100 z-10"
dir="ltr"
- role="tooltip"
+ aria-hidden="true"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span | |
| class="pointer-events-none absolute -top-9 inset-is-1/2 -translate-x-1/2 whitespace-nowrap rounded-md bg-gray-900 text-white dark:bg-gray-100 dark:text-gray-900 text-xs px-2 py-1 shadow-lg opacity-0 scale-95 transition-all duration-150 group-hover:opacity-100 group-hover:scale-100 z-10" | |
| dir="ltr" | |
| role="tooltip" | |
| > | |
| <span | |
| class="pointer-events-none absolute -top-9 inset-is-1/2 -translate-x-1/2 whitespace-nowrap rounded-md bg-gray-900 text-white dark:bg-gray-100 dark:text-gray-900 text-xs px-2 py-1 shadow-lg opacity-0 scale-95 transition-all duration-150 group-hover:opacity-100 group-hover:scale-100 group-focus-within:opacity-100 group-focus-within:scale-100 z-10" | |
| dir="ltr" | |
| aria-hidden="true" | |
| > |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
4 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: