Add diff review mode with before/after comparisons#4
Conversation
Introduces `review-mode: 'diff'` option that fetches base branch .pen files via GitHub API, calls the screenshot service's /api/v1/diff endpoint, and posts PR comments with side-by-side before/after screenshots for modified frames, new screenshots for added frames, and strikethrough listings for removed frames. Unchanged frames are collapsed in a details section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request introduces a "diff mode" review feature alongside the existing full render mode. Users can now select between reviewing complete design changes or comparing frame-level differences. The implementation adds configuration support, new diff-rendering logic in comment builders, file content fetching utilities, and orchestration changes to handle both rendering modes. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub API
participant Main as Main Workflow
participant ServiceR as Service Renderer
participant CommentB as Comment Builder
participant Output as PR Output
alt Review Mode: Diff
Main->>GH: getFileContentAtRef(base ref)
GH-->>Main: base content (base64)
Main->>GH: getChangedPenFiles(head)
GH-->>Main: changed files
Main->>Main: readLocalFileAsBase64(head file)
Main-->>Main: head content (base64)
Main->>ServiceR: fetchDiff(basePenBase64, headPenBase64)
ServiceR->>ServiceR: submit diff job
ServiceR->>ServiceR: poll for completion
ServiceR-->>Main: DiffResponse
Main->>CommentB: buildDiffComment(diffData)
CommentB-->>Main: markdown comment
Main->>Output: post comment + outputs
else Review Mode: Full
Main->>GH: getChangedPenFiles
GH-->>Main: changed files
Main->>ServiceR: renderFullFile(penContent)
ServiceR->>ServiceR: fetch all frames
ServiceR-->>Main: rendered frames
Main->>CommentB: buildComment(fullData)
CommentB-->>Main: markdown comment
Main->>Output: post comment + outputs
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Add example-diff.yml showing the diff review mode configuration. Modify Dashboard, Analytics, and Settings frames in random.pen to demonstrate before/after comparison in diff mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/github/files.ts (2)
125-162: Solid implementation with proper blob fallback for large files.The two-tier approach (content API → blob API) handles GitHub's content truncation well. One minor robustness note: the 404 detection on line 157 uses
(error as any).status— this works with Octokit'sRequestError, but consider checkingerrormore specifically if Octokit typing is available (e.g., importingRequestErrorfrom@octokit/request-error).
167-169: No error handling for missing files.
readLocalFileAsBase64will throw an unhandledENOENTif the file doesn't exist at the local path. The callers inmain.tsdo wrap this intry/catch, so it won't crash the action — but a more descriptive error message here would improve debuggability.💡 Optional: wrap with a friendlier error
export function readLocalFileAsBase64(filePath: string): string { + if (!fs.existsSync(filePath)) { + throw new Error(`Local file not found: ${filePath}`); + } const content = fs.readFileSync(filePath); return content.toString('base64'); }src/renderers/service.ts (1)
200-253:fetchDifffollows established patterns but inherits the unsafe-cast concern.The method mirrors the existing
fetchAllFramesflow (submit → poll → cast). Line 239 performsas DiffResponseon unvalidated JSON, which could produce confusing runtime errors if the service returns an unexpected shape. This is the same pattern as line 333 (as ServiceResponse), so it's not a regression, but worth noting.Consider adding a lightweight runtime guard (e.g., checking
result.type === 'diff'andresult.summary) before returning, to fail fast with a clear message if the service contract changes.💡 Suggested guard after polling
// Poll for completion (reuses existing polling logic) - const result = await this.pollForCompletion(submitData.jobId) as DiffResponse; + const raw = await this.pollForCompletion(submitData.jobId); + const result = raw as DiffResponse; + if (!result || result.type !== 'diff' || !result.summary) { + throw new Error(`Unexpected diff response shape from job ${submitData.jobId}`); + }src/comment-builder.ts (2)
362-377: Image URLs in HTML are not escaped.
mod.base.imageUrlandmod.head.imageUrlare interpolated directly into<img src="...">without HTML-attribute escaping. If the service ever returns a URL with"or other special characters, it could break the HTML. GitHub does sanitize PR comment HTML, so this is not exploitable in practice, but escaping quotes would be more robust.
413-442: Frame counting for added files may double-count if types aren't mutually exclusive.Lines 424-429 aggregate from
file.diff.summaryand lines 432-434 aggregate fromfile.framesfor added files. If aDiffFileCommentDataever has bothdiffandframespopulated,totalAddedFrameswould be inflated. Current callers inmain.tsnever set both — but the types allow it. Consider adding a guard or a comment documenting the mutual exclusivity.src/main.ts (2)
113-165:runFullModeduplicates the rendering logic already extracted intorenderFullFile.The per-file rendering in
runFullMode(lines 113-151) does the same document-load → frame-slice → render-loop → push-results asrenderFullFile(lines 324-342). Consider havingrunFullModedelegate torenderFullFileto reduce duplication. The only additionsrunFullModeneeds on top are the inlinetotalFramesRenderedcounter and the log line — both easily handled after the call.♻️ Sketch of deduplication
try { - // Parse the .pen file to get top-level frames (screens/artboards) - const document = await loadPenDocument(file.path); - const frames = getTopLevelFrames(document); - - // Limit frames if configured - const framesToProcess = - inputs.maxFramesPerFile > 0 - ? frames.slice(0, inputs.maxFramesPerFile) - : frames; - - core.info(`Found ${frames.length} top-level frames, processing ${framesToProcess.length}`); - - // Render each frame - const frameResults: FrameCommentData[] = []; - - for (const frame of framesToProcess) { - const outputPath = getOutputPath(inputs.outputDir, file.path, frame, inputs.imageFormat); - - const result = await renderer.renderFrame(file.path, frame, outputPath); - - frameResults.push({ - id: frame.id, - name: frame.name, - screenshotUrl: result.imageUrl, - screenshotPath: result.success ? result.screenshotPath : undefined, - error: result.error, - }); - - if (result.success) { - totalFramesRendered++; - } - } + const frameResults = await renderFullFile(renderer, file, inputs); + totalFramesRendered += frameResults.filter(f => !f.error).length; fileResults.push({ path: file.path,Also applies to: 319-343
222-222: Unsafeas ServiceRenderercast — safe at runtime but fragile.This cast is guarded by the
inputs.renderer === 'service'check on line 64, so it's correct today. But ifinitializeRendererever returns a differentBaseRenderersubtype for the service path, this would silently break. A narrowing check would be more resilient.💡 Optional: runtime narrowing
- const renderer = await initializeRenderer(inputs) as ServiceRenderer; + const renderer = await initializeRenderer(inputs); + if (!(renderer instanceof ServiceRenderer)) { + throw new Error('Diff mode requires a ServiceRenderer'); + }
🎨 Design Review📁 1 design file (1 modified) 📝
|
| Frame | ID | Status |
|---|---|---|
| 1. Dashboard | J1VeO |
❌ Screenshot service returned 429: {"error":"Monthly screenshot limit exceeded for RemoteState/pencil-actions","code":"USAGE_LIMIT_EXCEEDED","usage":{"current":101,"limit":100,"remaining":0}} |
| 2. Interviews | iVES0 |
❌ Frame iVES0 not found in API response |
| 3. Candidates | x7cUJ |
❌ Frame x7cUJ not found in API response |
| 4. Questions Bank | Mvlj1 |
❌ Frame Mvlj1 not found in API response |
| 5. AI Settings | lwglP |
❌ Frame lwglP not found in API response |
| 6. Analytics | pdVTW |
❌ Frame pdVTW not found in API response |
| 7. Job Positions | pCH9A |
❌ Frame pCH9A not found in API response |
| 8. Settings | 7b2O7 |
❌ Frame 7b2O7 not found in API response |
| Components | 0KJfI |
❌ Frame 0KJfI not found in API response |
Generated by Pencil Design Review for commit c722298
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- New `comment-id` input namespaces the HTML comment marker so multiple workflows can post separate comments without overwriting each other - Change default review-mode from 'full' to 'diff' - Update all example workflows to use `./` and distinct comment-ids (basic, visual, diff, test) so PR #4 shows all modes side by side Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🎨 Design Review📁 1 design file (1 modified) 📝
|
🎨 Design Review📁 1 design file (1 modified) 📝
|
| Before | After |
|---|---|
 |
 |
6. Analytics
| Before | After |
|---|---|
 |
 |
8. Settings
| Before | After |
|---|---|
 |
 |
✅ 6 unchanged frames
-
- Interviews (
iVES0)
- Interviews (
-
- Candidates (
x7cUJ)
- Candidates (
-
- Questions Bank (
Mvlj1)
- Questions Bank (
-
- AI Settings (
lwglP)
- AI Settings (
-
- Job Positions (
pCH9A)
- Job Positions (
- Components (
0KJfI)
Generated by Pencil Design Review for commit c722298
🎨 Design Review📁 1 design file (1 modified) 📝
|
| Frame | ID | Status |
|---|---|---|
| 1. Dashboard | J1VeO |
❌ Failed to download image: 401 |
| 2. Interviews | iVES0 |
❌ Failed to download image: 401 |
| 3. Candidates | x7cUJ |
❌ Failed to download image: 401 |
| 4. Questions Bank | Mvlj1 |
❌ Failed to download image: 401 |
| 5. AI Settings | lwglP |
❌ Failed to download image: 401 |
| 6. Analytics | pdVTW |
❌ Failed to download image: 401 |
| 7. Job Positions | pCH9A |
❌ Failed to download image: 401 |
| 8. Settings | 7b2O7 |
❌ Failed to download image: 401 |
| Components | 0KJfI |
❌ Failed to download image: 401 |
Generated by Pencil Design Review for commit c722298
🎨 Design Review📁 1 design file (1 modified) 📝
|
| Before | After |
|---|---|
 |
 |
6. Analytics
| Before | After |
|---|---|
 |
 |
8. Settings
| Before | After |
|---|---|
 |
 |
✅ 6 unchanged frames
-
- Interviews (
iVES0)
- Interviews (
-
- Candidates (
x7cUJ)
- Candidates (
-
- Questions Bank (
Mvlj1)
- Questions Bank (
-
- AI Settings (
lwglP)
- AI Settings (
-
- Job Positions (
pCH9A)
- Job Positions (
- Components (
0KJfI)
Generated by Pencil Design Review for commit c722298
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
review-mode: 'diff'action input that shows only changed frames instead of rendering everything.penfiles via GitHub API, sends both versions to the screenshot service's/api/v1/diffendpoint<details>for unchanged framesreview-mode: 'full'behavior is unchangedDepends on: Service-side PR RemoteState/pencil-screenshot-service#6 (diff endpoint)
Files changed
action.ymlreview-modeinput (default'full')src/types.tsReviewMode,DiffResponse, diff comment data typessrc/config.tsreview-modesrc/github/files.tsgetFileContentAtRef(),readLocalFileAsBase64(), capturepreviousPathfor renamessrc/renderers/service.tsfetchDiff()method, widenedpollForCompletionreturn typesrc/comment-builder.tsbuildDiffComment(), before/after HTML tables, collapsed unchangedsrc/main.tsrunDiffMode(), extractedrunFullMode(), branching logicTest plan
review-mode: 'diff'— verify modified files show before/afterreview-mode: 'full'— verify full mode still works unchanged🤖 Generated with Claude Code
Summary by CodeRabbit