Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GitHub OAuth and starring support alongside existing Atproto auth: new GitHub composable and GitHub modal UI, AccountMenu updates to show GitHub connection and stacked avatars, server API routes for GitHub OAuth and star management, new shared schema and session github field, Atproto endpoints moved under Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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: 8
🧹 Nitpick comments (3)
shared/schemas/github.ts (1)
3-6: Consider adding non-empty string validation forownerandrepo.
v.string()accepts empty strings, which would pass validation but fail silently at the GitHub API level. AminLength(1)pipe rejects these at the schema boundary.♻️ Suggested fix
export const GitHubStarBodySchema = v.object({ - owner: v.string(), - repo: v.string(), + owner: v.pipe(v.string(), v.minLength(1)), + repo: v.pipe(v.string(), v.minLength(1)), })server/api/auth/github/session.delete.ts (1)
3-11: Consider returning a structured JSON response for consistency.Other endpoints in this PR return JSON objects (e.g.,
{ starred: false }). Returning a plain string here makes it harder for clients to handle responses uniformly. A minor inconsistency, but worth aligning.Suggested fix
- return 'GitHub disconnected' + return { disconnected: true }server/api/github/star.put.ts (1)
7-47: Significant code duplication withstar.delete.ts.This handler is nearly identical to
star.delete.ts— the only differences are the HTTP method (PUTvsDELETE), theContent-Lengthheader, and the returnedstarredboolean. Consider extracting a shared helper to reduce duplication:Sketch of shared helper
// server/utils/github-star.ts export async function toggleGitHubStar(event: H3Event, method: 'PUT' | 'DELETE') { const session = await useServerSession(event) const github = session.data.github if (!github?.accessToken) { throw createError({ statusCode: 401, message: 'GitHub account not connected.' }) } const { owner, repo } = v.parse(GitHubStarBodySchema, await readBody(event)) // ... validation, fetch, error handling ... return { starred: method === 'PUT' } }Note: the path traversal concern flagged on
star.delete.tsapplies equally here.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/components/Header/AccountMenu.client.vueapp/components/Header/AtprotoModal.client.vueapp/components/Header/GitHubModal.client.vueapp/composables/atproto/useAtproto.tsapp/composables/github/useGitHub.tsapp/composables/github/useGitHubStar.tsapp/pages/package/[[org]]/[name].vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonnuxt.config.tsserver/api/auth/atproto/index.get.tsserver/api/auth/atproto/session.delete.tsserver/api/auth/atproto/session.get.tsserver/api/auth/github/index.get.tsserver/api/auth/github/session.delete.tsserver/api/auth/github/session.get.tsserver/api/github/star.delete.tsserver/api/github/star.put.tsserver/api/github/starred.get.tsserver/utils/auth.tsshared/schemas/github.tsshared/types/userSession.ts
| const { data: starStatus, refresh } = useFetch<StarStatus>( | ||
| () => `/api/github/starred?owner=${owner.value}&repo=${repo.value}`, | ||
| { | ||
| server: false, | ||
| immediate: false, | ||
| default: () => ({ starred: false, connected: false }), | ||
| watch: false, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Query parameters are not URL-encoded.
owner.value and repo.value are interpolated directly into the URL string. If either value contains characters like &, =, or spaces, the URL will be malformed. Use encodeURIComponent or pass a query object.
Suggested fix using query object
- const { data: starStatus, refresh } = useFetch<StarStatus>(
- () => `/api/github/starred?owner=${owner.value}&repo=${repo.value}`,
- {
- server: false,
- immediate: false,
- default: () => ({ starred: false, connected: false }),
- watch: false,
- },
- )
+ const { data: starStatus, refresh } = useFetch<StarStatus>(
+ '/api/github/starred',
+ {
+ server: false,
+ immediate: false,
+ default: () => ({ starred: false, connected: false }),
+ watch: false,
+ query: computed(() => ({ owner: owner.value, repo: repo.value })),
+ },
+ )| <TooltipApp | ||
| v-if="isGitHubRepo && isGitHubConnected" | ||
| :text=" | ||
| isStarred ? $t('package.github_star.unstar') : $t('package.github_star.star') | ||
| " | ||
| position="bottom" | ||
| strategy="fixed" | ||
| > | ||
| <ButtonBase | ||
| size="small" | ||
| :title=" | ||
| isStarred ? $t('package.github_star.unstar') : $t('package.github_star.star') | ||
| " | ||
| :aria-label=" | ||
| isStarred ? $t('package.github_star.unstar') : $t('package.github_star.star') | ||
| " | ||
| :aria-pressed="isStarred" | ||
| :disabled="isStarActionPending" | ||
| :classicon="isStarred ? 'i-lucide:star text-yellow-400' : 'i-lucide:star'" | ||
| @click="toggleStar" | ||
| > | ||
| {{ compactNumberFormatter.format(stars) }} | ||
| </ButtonBase> | ||
| </TooltipApp> |
There was a problem hiding this comment.
Star count does not update after toggling.
stars (Line 914) comes from useRepoMeta, which fetches the repository metadata once. After toggleStar, isStarred updates via the optimistic UI in useGitHubStar, but stars remains stale — the count won't increment/decrement. This creates a confusing UX where the icon changes but the number doesn't.
Consider deriving a display count that adjusts based on the toggle delta:
const displayStars = computed(() => {
if (!isGitHubConnected.value) return stars.value
// Adjust based on the original fetched state vs current toggled state
// (requires tracking the initial starred state from the API)
...
})Alternatively, pass the stars value into useGitHubStar so it can return a reactive adjusted count.
| :aria-pressed="isStarred" | ||
| :disabled="isStarActionPending" | ||
| :classicon="isStarred ? 'i-lucide:star text-yellow-400' : 'i-lucide:star'" |
There was a problem hiding this comment.
Star states are distinguished only by colour, which is an accessibility concern.
Both starred and unstarred use i-lucide:star; the only visual difference is text-yellow-400. Users who cannot perceive colour differences won't see the state change. The aria-pressed attribute helps assistive technology, but consider adding a visible text label (e.g., "Starred" / "Star") or using distinct icons (the PR description notes Lucide lacks filled variants — a different icon set could help here).
| await serverSession.update({ | ||
| public: undefined, | ||
| oauthSession: undefined, | ||
| oauthState: undefined, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd server && git ls-files --exclude-standard | head -20Repository: npmx-dev/npmx.dev
Length of output: 727
🏁 Script executed:
# Find the file being reviewed and its context
find . -name "session.delete.ts" -o -name "*session*" -type f | grep -E "\.(ts|tsx)$" | head -20Repository: npmx-dev/npmx.dev
Length of output: 307
🏁 Script executed:
# Search for session type definitions and useSession usage
rg -n "useSession|SessionData|ServerSession" --type ts -A 5 -B 2 | head -100Repository: npmx-dev/npmx.dev
Length of output: 6867
🏁 Script executed:
# Look at the file being reviewed to understand context
cat -n server/api/auth/atproto/session.delete.tsRepository: npmx-dev/npmx.dev
Length of output: 565
🏁 Script executed:
# Search for where public, oauthSession, oauthState are used/defined
rg -n "public.*oauth|oauth.*public" --type ts -C 3Repository: npmx-dev/npmx.dev
Length of output: 576
🏁 Script executed:
# Find session type augmentation or declarations
rg -n "declare.*session|interface.*[Ss]ession|type.*[Ss]ession" --type ts -A 3 | head -80Repository: npmx-dev/npmx.dev
Length of output: 3806
🏁 Script executed:
cat -n shared/types/userSession.tsRepository: npmx-dev/npmx.dev
Length of output: 869
🏁 Script executed:
cat -n server/api/auth/github/session.delete.tsRepository: npmx-dev/npmx.dev
Length of output: 390
🏁 Script executed:
# Check if there's a github field in UserServerSession
rg -A 20 "interface UserServerSession" --type tsRepository: npmx-dev/npmx.dev
Length of output: 1212
🏁 Script executed:
# Search for where github session data is stored
rg -n "session.*github\|github.*session" --type ts -C 2 | head -60Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check package.json to see h3 version
cat -n package.json | grep -A 5 -B 5 '"h3"'Repository: npmx-dev/npmx.dev
Length of output: 435
🌐 Web query:
h3 session.update undefined remove key semantics
💡 Result:
In h3 sessions, session.update() is just a thin wrapper around updateSession(event, config, update?). It merges the provided object into session.data using Object.assign only if update is truthy—there is no special “undefined means delete this key” behavior. [1]
What happens with undefined
await session.update(undefined)(or no argument) does not changesession.data; it just re-seals/writes the session cookie with the current data. [1]await session.update({ foo: undefined })setsfootoundefined(the property still exists; it’s not removed). [1]
How to remove a key
Use the functional form and delete the property (then return nothing so the Object.assign path is skipped, but your mutation remains on session.data):
await session.update((data) => {
delete data.foo
})This works because when update is a function, h3 calls it with session.data, and if it returns undefined, h3 won’t Object.assign, but it will still seal/write the (now mutated) session. [1]
How to remove the whole session
Use await session.clear(). [2]
Sources: h3 useSession/updateSession implementation [1], h3 sessions docs (update/clear usage) [2].
🏁 Script executed:
# Check if there are any tests or documentation about session handling
find . -name "*.md" -o -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "session.update\|useSession" 2>/dev/null | head -5Repository: npmx-dev/npmx.dev
Length of output: 43
Use the functional form to properly remove ATProto session keys.
The current approach of setting fields to undefined does not delete them from the session in h3. The keys will persist with undefined values, leaving stale session data:
Corrected update pattern
await session.update((data) => {
delete data.public
delete data.oauthSession
delete data.oauthState
})Alternatively, since this clears only ATProto-specific fields and GitHub is stored separately in the github field, ensure the functional form is used to fully remove the keys rather than just nullifying them.
|
Was attempting to update the star count, but it seems like GitHub's api doesn't immediately update. I can optimistically update the UI, but I'm not sure how to persist the star count on refresh/navigate. |
|
Could also use some guidance for #1613 (comment) since I'm not too familiar with h3 (moreso just want to know the accuracy of this statement) |
2 participants
🔗 Linked issue
#479
🧭 Context
Implements GitHub OAuth, displays whether a repo is starred or not, and the ability to star/unstar through the UI. This does not implement the #32 portion.
Demo
demo.mp4
📚 Description
.envrequiresGITHUB_CLIENT_IDandGITHUB_CLIENT_SECRETfrom an OAuth app in order to run. I renamed/moved some of the AT Protocol files to distinguish between the auth providers since there's more than one now. I based the GH auth off the existing AT route, and I didn't run into any issues, but it's been a while since I've manually handled oauth flows, so there's a chance I've missed an edge case or something.For the starring API routes, I just left them all under
/githubbecause I wasn't sure if a/starpath was needed/wanted.A couple of questions:
auth.githubbecause I wasn't sure on the process of updating keys (is a find-and-replace enough?), but I will move these toauth.modal