-
Notifications
You must be signed in to change notification settings - Fork 11
Add page screenshot #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a screenshot URL utility using the capture-node library, adds the dependency, and integrates server-side generation of per-page screenshot URLs into the pages index. The Pages component now accepts and renders these screenshots alongside existing page metadata in a two-column layout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant B as Browser
participant N as Next.js (pages/pages/index.tsx)
participant C as capture utils (utils/capture.ts)
participant Lib as capture-node (SDK)
U->>B: Navigate to /pages
B->>N: getServerSideProps()
N->>N: Map pages -> url (custom_domain or slug)
N->>C: getPageScreenshotUrl(url)
C->>Lib: buildImageUrl({ url, w:1280, h:720, scale:1.5 })
Lib-->>C: Signed image URL
C-->>N: Screenshot URL
N-->>B: props { pages, screenshots, ... }
B->>N: Render Pages component
Note over N,B: Pages renders two-column items with left screenshot and right metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/web/utils/capture.ts (1)
1-1
: Import shape may be wrongPlease confirm whether
capture-node
provides a default export or a namedCapture
. If it’s default, this import will throw at runtime.apps/web/pages/pages/index.tsx (4)
16-16
: Avoid bundling server-only deps into the clientMove the
getPageScreenshotUrl
import insidegetServerSideProps
via dynamic import socapture-node
never touches the client bundle.-import { getPageScreenshotUrl } from "../../utils/capture"; +// Moved to getServerSideProps via dynamic import to keep client bundle clean.Add inside getServerSideProps (shown in the next comment’s diff).
94-94
: Use the ROUTES constant for consistencyPrefer
ROUTES.NEW_PAGE
here (used elsewhere) to avoid hard-coded route drift.- ? "/pages/new" + ? ROUTES.NEW_PAGE
129-134
: Invalid Tailwind class: h-18
h-18
isn’t in Tailwind’s default scale; height won’t apply. Use a valid height and adddecoding="async"
for perf.- <img - className="w-48 h-18 object-cover rounded-md border border-gray-200 dark:border-gray-700" - src={screenshots[idx]} - alt={`Screenshot of ${page.title}`} - loading="lazy" - /> + <img + className="w-48 h-28 object-cover rounded-md border border-gray-200 dark:border-gray-700" + src={screenshots[idx]} + alt={`Screenshot of ${page.title}`} + loading="lazy" + decoding="async" + />
121-133
: Index-coupling between pages and screenshotsRelying on
idx
risks mismatch if the arrays ever diverge. Prefer returning a single array of objects{ page, screenshot }
fromgetServerSideProps
and map that.Example (outside current hunk):
// server const items = rows.map((page) => ({ page, screenshot: getPageScreenshotUrl( page.page_settings?.custom_domain ? `https://${page.page_settings.custom_domain}` : `https://${page.url_slug}.changes.page` ), })); return { props: { items, error } }; // client {items.map(({ page, screenshot }) => ( // use screenshot directly ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/web/package.json
(1 hunks)apps/web/pages/pages/index.tsx
(6 hunks)apps/web/utils/capture.ts
(1 hunks)
🔇 Additional comments (2)
apps/web/pages/pages/index.tsx (2)
202-202
: Nice a11y improvementAdding
<title>Go to page</title>
to the SVG is a good touch.
37-44
: Signed URL confirmed — no raw API secret exposedVerified: buildImageUrl returns a signed URL containing only the public API key + a generated hash (e.g., md5(api_secret + request_query)), not the raw secret — safe to use in an
. Treat generated URLs as public tokens (anyone with the full URL can fetch the image).
Summary by CodeRabbit