Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request implements deterministic TID (Temporal ID) generation for blog posts and introduces AT Protocol site standard publication metadata. The changes include migrating the TID library from 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/standard-site-sync.ts (1)
238-251:⚠️ Potential issue | 🟠 MajorThe
instanceof XrpcResponseErrorcheck is incorrect and will prevent synchronisation.The code checks
checkForBlogResult instanceof XrpcResponseError, butxrpcSafereturns a discriminated union with a.successproperty, not an XrpcResponseError instance. This check will always be false, causing the code to skip theRecordNotFoundlogic and fall through to the error log instead.Elsewhere in the same file (line 272), the code correctly uses
if (publicationCheck.success). Use the same pattern here:if (!checkForBlogResult.success) { if (checkForBlogResult.error === 'RecordNotFound') {
🧹 Nitpick comments (2)
shared/utils/constants.ts (1)
13-13: Avoid duplicating the DID literal in two exported constants.Line 54 duplicates Line 13 exactly. Prefer aliasing to a single source of truth to prevent value drift.
Proposed refactor
-export const NPMX_DEV_DID = 'did:plc:u5zp7npt5kpueado77kuihyz' +export const NPMX_DEV_DID = NPMX_DIDAlso applies to: 54-54
modules/standard-site-sync.ts (1)
89-90: Consider extracting the delay duration to a named constant.The 2-second delay is pragmatic for allowing indexers to catch up, but the magic number could benefit from being a named constant with documentation explaining its purpose. Additionally, fixed delays can be fragile if indexer performance varies.
💡 Optional improvement
+/** Delay to allow firehose/indexers to process the new publication record */ +const INDEXER_CATCHUP_DELAY_MS = 2_000 + // ...inside the hook: - // Wait for the firehose and indexers to catch up if we create a publication - await new Promise(sleepResolve => setTimeout(sleepResolve, 2_000)) + // Wait for the firehose and indexers to catch up if we create a publication + await new Promise(sleepResolve => setTimeout(sleepResolve, INDEXER_CATCHUP_DELAY_MS))
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
app/components/global/BlogPostWrapper.vuemodules/standard-site-sync.tsnuxt.config.tspackage.jsonserver/routes/.well-known/site.standard.publication.get.tsserver/utils/atproto/utils/likes.tsshared/utils/atproto.tsshared/utils/constants.tstest/unit/server/utils/likes-evolution.spec.ts
| @@ -1,5 +1,6 @@ | |||
| <script setup lang="ts"> | |||
| import type { BlogPostFrontmatter } from '#shared/schemas/blog' | |||
| import { generateBlogTID } from '#shared/utils/atproto' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/components/global/BlogPostWrapper.vue | head -30Repository: npmx-dev/npmx.dev
Length of output: 1162
🏁 Script executed:
fd constants -type f -path "*/shared/*" | head -20Repository: npmx-dev/npmx.dev
Length of output: 231
🏁 Script executed:
rg "NPMX_DEV_DID" --type ts --type js -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 1638
Import NPMX_DEV_DID before using it on line 22.
The constant is used in the useHead block but not imported, which will fail type-checking and the build.
Proposed fix
import type { BlogPostFrontmatter } from '#shared/schemas/blog'
import { generateBlogTID } from '#shared/utils/atproto'
+import { NPMX_DEV_DID } from '#shared/utils/constants'📝 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 { generateBlogTID } from '#shared/utils/atproto' | |
| import type { BlogPostFrontmatter } from '#shared/schemas/blog' | |
| import { generateBlogTID } from '#shared/utils/atproto' | |
| import { NPMX_DEV_DID } from '#shared/utils/constants' |
| @@ -14,6 +15,15 @@ useSeoMeta({ | |||
| ...(props.frontmatter.draft ? { robots: 'noindex, nofollow' } : {}), | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd app/components/global && wc -l BlogPostWrapper.vueRepository: npmx-dev/npmx.dev
Length of output: 83
🏁 Script executed:
cd app/components/global && cat -n BlogPostWrapper.vue | sed -n '1,50p'Repository: npmx-dev/npmx.dev
Length of output: 2026
🏁 Script executed:
cd app/components/global && grep -r "site.standard.document" --include="*.ts" --include="*.tsx" --include="*.vue" --include="*.js" | head -20Repository: npmx-dev/npmx.dev
Length of output: 263
🏁 Script executed:
grep -r "NPMX_DEV_DID" --include="*.ts" --include="*.tsx" --include="*.vue" --include="*.js" | head -10Repository: npmx-dev/npmx.dev
Length of output: 670
🏁 Script executed:
grep -r "generateBlogTID" --include="*.ts" --include="*.tsx" --include="*.vue" --include="*.js" -A 3 | head -30Repository: npmx-dev/npmx.dev
Length of output: 1466
🏁 Script executed:
cat app/modules/standard-site-sync.tsRepository: npmx-dev/npmx.dev
Length of output: 126
🏁 Script executed:
grep -r "frontmatter.draft" --include="*.ts" --include="*.tsx" --include="*.vue" --include="*.js" -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 849
🏁 Script executed:
fd "standard-site-sync" --type fRepository: npmx-dev/npmx.dev
Length of output: 90
🏁 Script executed:
fd "modules" --type d | head -5Repository: npmx-dev/npmx.dev
Length of output: 69
🏁 Script executed:
cat modules/standard-site-sync.tsRepository: npmx-dev/npmx.dev
Length of output: 9654
Guard site.standard.document link emission for draft posts.
Line 15 prevents search engine indexing of drafts with noindex, nofollow, but lines 18-25 unconditionally emit a site.standard.document link containing the draft's identifier. The AT Protocol sync module (standard-site-sync.ts) correctly filters drafts from being synced, so this link points to a non-existent record. However, the link header itself leaks the draft's existence to anything monitoring the page source.
Proposed fix
-useHead({
- link: [
- {
- rel: 'site.standard.document',
- href: `at://${NPMX_DEV_DID}/site.standard.document/${generateBlogTID(props.frontmatter.date, props.frontmatter.slug)}`,
- },
- ],
-})
+if (!props.frontmatter.draft) {
+ useHead({
+ link: [
+ {
+ rel: 'site.standard.document',
+ href: `at://${NPMX_DEV_DID}/site.standard.document/${generateBlogTID(props.frontmatter.date, props.frontmatter.slug)}`,
+ },
+ ],
+ })
+}📝 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.
| ...(props.frontmatter.draft ? { robots: 'noindex, nofollow' } : {}), | |
| useHead({ | |
| link: props.frontmatter.draft ? [] : [ | |
| { | |
| rel: 'site.standard.document', | |
| href: `at://${NPMX_DEV_DID}/site.standard.document/${generateBlogTID(props.frontmatter.date, props.frontmatter.slug)}`, | |
| }, | |
| ], | |
| }) |
| @@ -0,0 +1,6 @@ | |||
| import { npmxPublicationRkey } from '#shared/utils/atproto' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/routes/.well-known/site.standard.publication.get.tsRepository: npmx-dev/npmx.dev
Length of output: 357
🏁 Script executed:
rg "NPMX_DEV_DID" --type ts --type tsx -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
rg "NPMX_DEV_DID" -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 1638
🏁 Script executed:
rg "defineEventHandler|setResponseHeader" shared/utils/constants.tsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
fd "nuxt.config" -type fRepository: npmx-dev/npmx.dev
Length of output: 231
🏁 Script executed:
grep -r "auto-import\|defineEventHandler" . --include="*.config.*" --include="*.json" 2>/dev/null | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
find server/routes -name "*.ts" -type f | head -5 | xargs catRepository: npmx-dev/npmx.dev
Length of output: 3058
🏁 Script executed:
cat -n server/routes/.well-known/site.standard.publication.get.ts | head -10Repository: npmx-dev/npmx.dev
Length of output: 357
Import NPMX_DEV_DID from #shared/utils/constants.
Line 5 references NPMX_DEV_DID without importing it, which will cause a runtime error.
Proposed fix
import { npmxPublicationRkey } from '#shared/utils/atproto'
+import { NPMX_DEV_DID } from '#shared/utils/constants'📝 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 { npmxPublicationRkey } from '#shared/utils/atproto' | |
| import { npmxPublicationRkey } from '#shared/utils/atproto' | |
| import { NPMX_DEV_DID } from '#shared/utils/constants' |
| useHead({ | ||
| link: [ | ||
| { | ||
| rel: 'site.standard.document', | ||
| href: `at://${NPMX_DEV_DID}/site.standard.document/${generateBlogTID(props.frontmatter.date, props.frontmatter.slug)}`, | ||
| }, | ||
| ], | ||
| }) |
There was a problem hiding this comment.
this should probably be done only for production
There was a problem hiding this comment.
import { isProduction } from '../config/env' the best check for that?
2 participants
🧭 Context
Our blog posts should now show on readers like https://pckt.blog/read. Will need to clear all previous site.standard.* records from npmx.dev. Can use pdsls for this https://pdsls.dev/at://did:plc:u5zp7npt5kpueado77kuihyz