Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Adebesin-Cell
left a comment
Adebesin-Cell
left a comment
There was a problem hiding this comment.
Interesting PR!
Screen.Recording.2026-02-02.at.08.07.12.mov
Connecting with any of the socials results in a 404 error.
It attempts to navigate to: https://npmxdev-git-fork-fatfingers23-feat-likes-poetry.vercel.app/package/api/auth/atproto
app/pages/package/[...package].vue
Outdated
| if (user.value?.handle == null) { | ||
| authModal.open() | ||
| } else { | ||
| const result = likesData.value?.userHasLiked ? await unlikePackage() : await likePackage() |
There was a problem hiding this comment.
I think this approach could be improved a bit. Right now, the API request is sent immediately on every click.
It might be better to optimistically update the UI and debounce the request, so users can toggle the button freely and only the final state is sent to the API after a short delay. This would also allow us to provide immediate feedback that the action was registered, and handle request failures more gracefully (e.g., by rolling back the UI state or showing an error).
This would avoid unnecessary API calls and result in a smoother UX overall.
There was a problem hiding this comment.
i agree, optimistic updates would fit nicely here and shouldn't be too difficult 👍
server/api/social/like.post.ts
Outdated
| if (hasLiked) { | ||
| throw createError({ | ||
| status: 400, | ||
| message: 'User has already liked the package', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Suggestion: silently succeed here instead of throwing an error.
Scenario: user likes the package twice with quirky internet connection and/or slower than usual response times.
Potential result on the UI: user see the like added then seeing the error message that liking has failed.
Been there, done that, with an app with more than 500,000 users :D
server/api/auth/social/like.post.ts
Outdated
| const body = await readBody<{ packageName: string }>(event) | ||
|
|
||
| if (!body.packageName) { | ||
| throw createError({ | ||
| status: 400, | ||
| message: 'packageName is required', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Could leverage valibot here, this way you wouldn't have to resort to custom null checks to ensure that body is actually of type you expect it to be.
| * Gets the definite answer if the user has liked a npm package. Either from the cache or the network | ||
| * @param packageName | ||
| * @param usersDid | ||
| * @returns |
There was a problem hiding this comment.
How about making xrpc routes (sth like /xrpc/npmx.feed.like.create)?
There was a problem hiding this comment.
Right now we're just using the the server side oauth client so if we made this endpoint an XRPC it wouldn't be quite correct since it uses the cookie for authentication. But there is a plan for some XRPC endpoints for other things just need to make a middleware for it to authenticate the service auth jwt
nuxt.config.ts
Outdated
| driver: 'fsLite', | ||
| base: './.cache/atproto-oauth/session', | ||
| }, | ||
| 'generic-cache': { |
There was a problem hiding this comment.
we probably also need to update modules/cache.ts to configure production redis
There was a problem hiding this comment.
May have to leave that one to you 😅. Do you have something to read up on that? The local generic-cache defined there is used just locally for dev and then uses upstash redis like you did for the oauth session lock when in production if that makes a difference.
server/utils/atproto/utils/likes.ts
Outdated
| @@ -0,0 +1,246 @@ | |||
| import { getCacheAdatper } from '../../cache' | |||
| import { $nsid as likeNsid } from '#shared/types/lexicons/dev/npmx/feed/like.defs' | |||
| import type { Backlink } from '~~/shared/utils/constellation' | |||
There was a problem hiding this comment.
here and some other places too:
| import type { Backlink } from '~~/shared/utils/constellation' | |
| import type { Backlink } from '#shared/utils/constellation' |
fatfingers23
force-pushed
the
feat/likes
branch
from
3807cb3 to
3b8d928
Compare
|
looking good! i left a bunch of comments, mostly about code organisation though tbh |
app/composables/useAtproto.ts
Outdated
| const data = ref<PackageLikes | null>(null) | ||
| const error = ref<Error | null>(null) | ||
| const pending = ref(false) |
There was a problem hiding this comment.
| const data = ref<PackageLikes | null>(null) | |
| const error = ref<Error | null>(null) | |
| const pending = ref(false) | |
| const data = shallowRef<PackageLikes | null>(null) | |
| const error = shallowRef<Error | null>(null) | |
| const pending = shallowRef(false) |
There was a problem hiding this comment.
By the way, I dont think we need these as composables. We almost never want the data from the ref itself, but rather the data returned by mutate. Just use $fetch where we need it or capsule just the $fetch in a util (IMO)
app/pages/package/[...package].vue
Outdated
| server: false, | ||
| }) | ||
|
|
||
| const { mutate: likePackage } = useLikePackage(packageName.value) |
There was a problem hiding this comment.
passing packageName.value means, that when packageName updates the likePackage will like the initial value.
fatfingers23
force-pushed
the
feat/likes
branch
from
911bcd2 to
a8f9a02
Compare
|
Thank you everyone for all the feedback! I think I have addressed everything brought up. About to log off for a few hours and going come back to do some final testing and I have 2 todos I want to hit
|
|
it seems that likes aren't persisted, right? as in, when I navigate back to a package, it shows up as 'unliked' |
fatfingers23
force-pushed
the
feat/likes
branch
from
ca67066 to
60211db
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/auth/atproto.get.ts (1)
49-51:⚠️ Potential issue | 🟡 MinorNormalise OAuth callback query params before
URLSearchParams.
getQueryreturns values that can bestring | string[]when query parameters are duplicated; the cast toRecord<string, string>hides this and breaks type safety. Per coding guidelines, ensure strictly type-safe code with explicit array value handling. The current code would silently convert arrays to comma-separated strings (e.g., on lines 32–33 with.toString()calls).🛠️ Suggested normalisation
- const { session: authSession } = await atclient.callback( - new URLSearchParams(query as Record<string, string>), - ) + const params = new URLSearchParams() + for (const [key, value] of Object.entries(query)) { + if (Array.isArray(value)) { + throw createError({ + status: 400, + message: `Duplicate query parameter: ${key}`, + }) + } + if (value != null) params.set(key, value.toString()) + } + const { session: authSession } = await atclient.callback(params)
| export async function handleAuthError( | ||
| fetchError: FetchError, | ||
| userHandle?: string | null, | ||
| ): Promise<never> { | ||
| const errorMessage = fetchError?.data?.message | ||
| if (errorMessage === ERROR_NEED_REAUTH && userHandle) { | ||
| await authRedirect(userHandle) | ||
| } | ||
| throw fetchError | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking for nuxt.config files ==="
fd -t f -e 'ts' -e 'js' 'nuxt.config' .
echo -e "\n=== Searching for ERROR_NEED_REAUTH usage across codebase ==="
rg "ERROR_NEED_REAUTH" -n --type ts --type js
echo -e "\n=== Checking if ERROR_NEED_REAUTH is defined in constants ==="
fd -t f 'constants' | head -20 | xargs cat 2>/dev/null | grep -B 2 -A 2 "ERROR_NEED_REAUTH" || echo "No direct matches found"
echo -e "\n=== Checking app/utils/atproto/helpers.ts for all imports ==="
cat app/utils/atproto/helpers.tsRepository: npmx-dev/npmx.dev
Length of output: 1492
🏁 Script executed:
#!/bin/bash
echo "=== Checking nuxt.config.ts for auto-import settings ==="
cat nuxt.config.ts | grep -A 30 "imports" || echo "No imports section found"
echo -e "\n=== Checking server/utils/atproto/oauth.ts for ERROR_NEED_REAUTH import ==="
head -40 server/utils/atproto/oauth.tsRepository: npmx-dev/npmx.dev
Length of output: 2915
🏁 Script executed:
#!/bin/bash
echo "=== Checking server/utils/atproto/oauth.ts around line 76 where ERROR_NEED_REAUTH is used ==="
sed -n '1,90p' server/utils/atproto/oauth.ts | tail -50Repository: npmx-dev/npmx.dev
Length of output: 1873
🏁 Script executed:
#!/bin/bash
echo "=== Checking complete imports in server/utils/atproto/oauth.ts ==="
head -20 server/utils/atproto/oauth.tsRepository: npmx-dev/npmx.dev
Length of output: 1019
Import ERROR_NEED_REAUTH from shared constants.
ERROR_NEED_REAUTH is used on line 26 but not imported. Add the missing import:
import { ERROR_NEED_REAUTH } from '#shared/utils/constants'This constant is defined in shared/utils/constants.ts and must be explicitly imported, as Nuxt's auto-import configuration only covers composables, not shared utilities.
| import type { Redis } from '@upstash/redis' | ||
|
|
||
| /** | ||
| * Redis cache storage with TTL handled by redis for use in production | ||
| */ | ||
| export class RedisCacheAdapter implements CacheAdapter { |
There was a problem hiding this comment.
Import CacheAdapter to avoid a compile-time error.
CacheAdapter is used but not imported, which will fail type-checking.
🧩 Proposed fix
-import type { Redis } from '@upstash/redis'
+import type { Redis } from '@upstash/redis'
+import type { CacheAdapter } from './shared'📝 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.
| import type { Redis } from '@upstash/redis' | |
| /** | |
| * Redis cache storage with TTL handled by redis for use in production | |
| */ | |
| export class RedisCacheAdapter implements CacheAdapter { | |
| import type { Redis } from '@upstash/redis' | |
| import type { CacheAdapter } from './shared' | |
| /** | |
| * Redis cache storage with TTL handled by redis for use in production | |
| */ | |
| export class RedisCacheAdapter implements CacheAdapter { |
| async set<T>(key: string, value: T, ttl?: number): Promise<void> { | ||
| const formattedKey = this.formatKey(key) | ||
| if (ttl) { | ||
| await this.redis.setex(formattedKey, ttl, value) | ||
| } else { | ||
| await this.redis.set(formattedKey, value) | ||
| } |
There was a problem hiding this comment.
Treat ttl = 0 explicitly instead of falling through.
if (ttl) skips TTL when ttl is 0. If 0 is meaningful in your usage, check for undefined instead.
🧯 Proposed fix
- if (ttl) {
+ if (ttl !== undefined) {
await this.redis.setex(formattedKey, ttl, value)
} else {
await this.redis.set(formattedKey, value)
}
|
Hello :) I just saw the new feature, nice work! It has a minor issue, the translation |
8 participants
Our first social feature 🥳 This closes #79
Adds
useStoragelocally and redis in prod with set expireCould be Improved
Concerns
I probably won't be able to address feedback till tomorrow around 22:00 UTC
Zen.mp4