feat: implement certificate sharing feature with public access via un… - #26
Conversation
…guessable token, allowing users to share their cat's certificate as a PDF or PNG
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds private certificate sharing with persistent share IDs, public certificate pages, social preview images, and PNG downloads. It updates certificate controls, metadata, routing, crawler rules, attribution, shared constants, error messages, and landing-page copy. ChangesCertificate sharing and export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Owner
participant CertificateSharePanel
participant setCertificateSharing
participant PublicCertificatePage
participant getPublicCertificate
participant PublicCertificateView
Owner->>CertificateSharePanel: enable sharing
CertificateSharePanel->>setCertificateSharing: submit catId and enabled state
setCertificateSharing-->>CertificateSharePanel: return shareId and enabled state
PublicCertificatePage->>getPublicCertificate: query shareId
getPublicCertificate-->>PublicCertificatePage: return public certificate payload
PublicCertificatePage->>PublicCertificateView: render certificate
PublicCertificateView-->>Owner: display public certificate
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
apps/web/modules/ceremony/ui/views/public-certificate-view.tsx (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
PublicCertificatetype instead of redefining it.
PublicCertificateDocduplicates the type already exported asPublicCertificatefromapps/web/lib/convex/http.ts(lines 6-8 there). Import it instead of redefining it here to avoid the two definitions drifting apart if the backend query's return shape changes.♻️ Proposed refactor
-import { api } from "`@workspace/backend/_generated/api`" import { APP_NAME } from "`@workspace/shared/constants/app`" import { Button } from "`@workspace/ui/components/button`" import { Skeleton } from "`@workspace/ui/components/skeleton`" -import type { FunctionReturnType } from "convex/server" import { dataComponent } from "`@/lib/data-component`" +import type { PublicCertificate } from "`@/lib/convex/http`" import { useCertificatePhotoDataUrl } from "`@/modules/ceremony/lib/use-certificate-download`" import type { CeremonyCertificateData } from "`@/modules/ceremony/ui/components/ceremony-certificate-document`" import { CeremonyCertificateDocument } from "`@/modules/ceremony/ui/components/ceremony-certificate-document`" -type PublicCertificateDoc = NonNullable< - FunctionReturnType<typeof api.certificate.getPublicCertificate> -> +import { api } from "`@workspace/backend/_generated/api`"Then replace remaining
PublicCertificateDocreferences withPublicCertificate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/modules/ceremony/ui/views/public-certificate-view.tsx` around lines 24 - 26, Remove the local PublicCertificateDoc definition and import the shared PublicCertificate type from the existing HTTP module. Replace every PublicCertificateDoc reference in the public certificate view with PublicCertificate, preserving the current type usage.apps/web/app/c/[shareId]/opengraph-image.tsx (1)
30-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLoad a serif font explicitly for
ImageResponse.
next/ogcannot access host system fonts. Without a matchingfontsentry,GeorgiaandTimes New Romanfall back to the built-in font. Pass the loaded font bytes through thefontsoption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/c/`[shareId]/opengraph-image.tsx around lines 30 - 108, Update the ImageResponse construction in the opengraph image handler to load the intended serif font bytes and pass them through the ImageResponse options under fonts. Keep the existing fontFamily styling and image layout unchanged, and ensure the font is loaded before constructing ImageResponse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/convex/http.ts`:
- Around line 10-27: Update fetchPublicCertificate to log a warning when
NEXT_PUBLIC_CONVEX_URL is missing before returning null, and wrap client.query
for api.certificate.getPublicCertificate in try/catch. Log query failures with
the error details and return null for both configuration and fetch failures so
public-facing callers retain the unavailable behavior without raw rejections.
In `@apps/web/proxy.ts`:
- Around line 12-13: Update the shared-certificate matcher in the proxy route
configuration from the broad "/c(.*)" pattern to the segment-aware "/c/:path*"
pattern, ensuring cats routes such as "/cats/[catId]" remain protected by
auth.protect().
---
Nitpick comments:
In `@apps/web/app/c/`[shareId]/opengraph-image.tsx:
- Around line 30-108: Update the ImageResponse construction in the opengraph
image handler to load the intended serif font bytes and pass them through the
ImageResponse options under fonts. Keep the existing fontFamily styling and
image layout unchanged, and ensure the font is loaded before constructing
ImageResponse.
In `@apps/web/modules/ceremony/ui/views/public-certificate-view.tsx`:
- Around line 24-26: Remove the local PublicCertificateDoc definition and import
the shared PublicCertificate type from the existing HTTP module. Replace every
PublicCertificateDoc reference in the public certificate view with
PublicCertificate, preserving the current type usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 501cb683-4dd9-4856-8b5f-47e0094ad7e4
📒 Files selected for processing (18)
apps/web/app/c/[shareId]/opengraph-image.tsxapps/web/app/c/[shareId]/page.tsxapps/web/app/robots.tsapps/web/lib/convex/http.tsapps/web/modules/ceremony/lib/use-certificate-download.tsapps/web/modules/ceremony/ui/components/ceremony-certificate-document.tsxapps/web/modules/ceremony/ui/components/certificate-share-panel.tsxapps/web/modules/ceremony/ui/views/cat-certificate-view.tsxapps/web/modules/ceremony/ui/views/public-certificate-view.tsxapps/web/modules/landing/lib/landing-faq.tsapps/web/modules/landing/ui/components/certificate-showcase.tsxapps/web/modules/landing/ui/components/how-it-works.tsxapps/web/modules/landing/ui/components/pricing-section.tsxapps/web/proxy.tspackages/backend/convex/certificate.tspackages/backend/convex/schema.tspackages/shared/src/constants/site.tspackages/shared/src/constants/staged-naming-errors.ts
| // Opt-in shared certificates (unguessable token; no auth) | ||
| "/c(.*)", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate app routes matching the "/c" prefix and check if they rely on auth.
fd -t d '^cats$' apps/web/app
fd . apps/web/app -t d --max-depth 3 | rg '^apps/web/app/c'
rg -n "auth\(\)|currentUser\(|useAuth\(" apps/web/app/cats 2>/dev/nullRepository: Daddyjohn63/naming-application
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files apps/web/proxy.ts apps/web/robots.ts 'apps/web/app/c*' | sed -n '1,120p'
printf '%s\n' '--- proxy outline ---'
ast-grep outline apps/web/proxy.ts --view expanded || true
printf '%s\n' '--- proxy ---'
cat -n apps/web/proxy.ts
printf '%s\n' '--- robots candidates ---'
fd -i 'robots' apps/web
for f in $(fd -i 'robots' apps/web); do
printf '%s\n' "--- $f ---"
cat -n "$f"
done
printf '%s\n' '--- c-prefixed app tree ---'
fd . apps/web/app -t f -t d --max-depth 5 | rg '^apps/web/app/c'
printf '%s\n' '--- cats tree and route/layout contents ---'
fd . apps/web/app/cats apps/web/app/c -t f -t d --max-depth 5
for f in $(fd . apps/web/app/cats apps/web/app/c -t f | sort); do
printf '%s\n' "--- $f ---"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- auth and Clerk configuration references ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'clerkMiddleware|authMiddleware|createRouteMatcher|publicRoutes|matcher|auth\(\)|currentUser\(|useAuth\(' \
apps/web package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | sed -n '1,240p'Repository: Daddyjohn63/naming-application
Length of output: 12219
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Clerk dependency declarations ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'"`@clerk/nextjs`"|"`@clerk/shared`"|createRouteMatcher|clerkMiddleware' \
package.json '**/package.json' pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | sed -n '1,220p'
printf '%s\n' '--- repository matcher-related tests/docs ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'createRouteMatcher|/c\(\\.\\*\\)|/c/\(\\.\\*\\)|dashboard\(\\.\\*\\)|path separator|public route' \
. | sed -n '1,240p'
printf '%s\n' '--- deterministic route-shape probe ---'
python3 - <<'PY'
import re
patterns = [r"^/c(.*)$", r"^/c/(.*)$"]
paths = ["/c", "/c/", "/c/abc", "/cats", "/careers", "/contact", "/checkout", "/cater"]
for pattern in patterns:
print(pattern)
compiled = re.compile(pattern)
for path in paths:
print(f" {path:10} {bool(compiled.fullmatch(path))}")
PYRepository: Daddyjohn63/naming-application
Length of output: 810
🌐 Web query:
Clerk Next.js createRouteMatcher "(.*)" path separator /dashboardx official documentation
💡 Result:
In the context of the Clerk Next.js SDK, createRouteMatcher is a helper function used to match route patterns [1]. Regarding your query about using (.) versus a path separator: 1. Recommended Approach: Clerk recommends using the:path syntax (e.g., /dashboard/:path*) instead of (.) for matching subtrees of routes [1]. This approach is preferred because it maps more accurately to how the Next.js router handles path segments [1]. Using (.) can lead to unintended matches; for example, /dashboard(.) would match /dashboardxyz, whereas /dashboard/:path correctly matches only /dashboard and its sub-paths [2]. 2. Status of (.): While (.) remains functional for backward compatibility, it is considered a legacy pattern [1][2]. You may see type-level suggestions in the SDK favoring:path* over (.*) [2]. 3. Deprecation Notice: Please be aware that createRouteMatcher itself has been deprecated by Clerk [1][3]. While it continues to function in the current version of the SDK, Clerk recommends migrating to resource-based authentication checks (using auth.protect directly within your server-side resources like pages, Route Handlers, and Server Actions) [1][3]. If you are currently using createRouteMatcher for non-authentication path logic, Clerk advises replacing it with native Next.js matching techniques, such as using the config.matcher in your middleware file or checking req.nextUrl.pathname directly [1][3].
Citations:
- 1: https://clerk.com/docs/reference/nextjs/clerk-middleware
- 2: fix(shared,nextjs): suggest :path* subtree form in createRouteMatcher path types clerk/javascript#9057
- 3: https://clerk.com/docs/guides/development/upgrading/upgrade-guides/migrate-from-create-route-matcher
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- AuthGuard definition and references ---'
rg -l --hidden -g '!node_modules' -g '!dist' -g '!build' \
'function AuthGuard|const AuthGuard|export .*AuthGuard|<AuthGuard' . | sed -n '1,120p'
for f in $(rg -l --hidden -g '!node_modules' -g '!dist' -g '!build' \
'function AuthGuard|const AuthGuard|export .*AuthGuard|<AuthGuard' . | sed -n '1,120p'); do
printf '%s\n' "--- $f ---"
cat -n "$f" | sed -n '1,240p'
done
printf '%s\n' '--- package manifests and Clerk versions ---'
find . -path '*/node_modules' -prune -o -type f -name 'package.json' -print0 |
xargs -0 rg -n '"`@clerk/nextjs`"|"`@clerk/shared`"' | sed -n '1,160p'Repository: Daddyjohn63/naming-application
Length of output: 3894
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cats view/module boundaries ---'
rg -l --hidden -g '!node_modules' -g '!dist' -g '!build' \
'CatCeremonyView|CatCertificateView|ConvexProviderWithClerk|ClerkProvider|Authenticated|ConvexReactClient' \
apps/web | sort | sed -n '1,160p'
for f in $(rg -l --hidden -g '!node_modules' -g '!dist' -g '!build' \
'CatCeremonyView|CatCertificateView|ConvexProviderWithClerk|ClerkProvider|Authenticated|ConvexReactClient' \
apps/web | sort | sed -n '1,160p'); do
printf '%s\n' "--- $f ---"
cat -n "$f" | sed -n '1,260p'
doneRepository: Daddyjohn63/naming-application
Length of output: 44132
Use a segment-aware matcher for shared certificates.
"/c(.*)" also matches /cats/[catId], so Clerk skips auth.protect() for the protected cats route. Replace it with "/c/:path*" so only /c descendants are public.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/proxy.ts` around lines 12 - 13, Update the shared-certificate
matcher in the proxy route configuration from the broad "/c(.*)" pattern to the
segment-aware "/c/:path*" pattern, ensuring cats routes such as "/cats/[catId]"
remain protected by auth.protect().
…on to improve robustness
1 participant
…guessable token, allowing users to share their cat's certificate as a PDF or PNG
Summary by CodeRabbit
New Features
Documentation
Bug Fixes