Add 'View full screen' compare links in diff comments#7
Conversation
- Propagate jobId from fetchDiff() response - Pass serviceUrl into DiffCommentData - Add "View full screen" link next to each modified frame name linking to /compare/:jobId?frame=FrameName Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request extends diff comment functionality by adding Changes
Sequence DiagramsequenceDiagram
participant Inputs
participant Main as Main Handler
participant Service as Service Renderer
participant CommentBuilder as Comment Builder
participant Output as Diff Comment
Inputs->>Main: Provide serviceUrl input
Main->>Service: Submit diff request
Service->>Service: Poll diff completion
Service->>Service: Attach jobId to response
Main->>Main: Include serviceUrl in DiffCommentData
Main->>CommentBuilder: Call buildDiffComment with serviceUrl
CommentBuilder->>CommentBuilder: For each file, check jobId + serviceUrl availability
alt serviceUrl and jobId present
CommentBuilder->>CommentBuilder: Render "View full screen" link in frame names
else
CommentBuilder->>CommentBuilder: Render original frame names
end
CommentBuilder->>Output: Generate final diff comment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/comment-builder.ts`:
- Around line 396-403: The generated compare URL can get a double slash and uses
redundant optional chaining; normalize serviceUrl by trimming any trailing slash
before building compareUrl and use the already-in-scope diff.jobId instead of
file.diff?.jobId. Locate where compareUrl is created (variable compareUrl in the
block that references serviceUrl, file.diff, mod.frameName and escapeMarkdown)
and replace construction to use a normalized serviceUrl (remove trailing '/' if
present) and diff.jobId for the path, keeping encodeURIComponent(mod.frameName)
and existing escapeMarkdown(mod.frameName) logic unchanged.
| const compareUrl = serviceUrl && file.diff?.jobId | ||
| ? `${serviceUrl}/compare/${file.diff.jobId}?frame=${encodeURIComponent(mod.frameName)}` | ||
| : null; | ||
| lines.push( | ||
| compareUrl | ||
| ? `**${escapeMarkdown(mod.frameName)}** · [View full screen](${compareUrl})` | ||
| : `**${escapeMarkdown(mod.frameName)}**` | ||
| ); |
There was a problem hiding this comment.
Normalize serviceUrl before URL construction; use diff.jobId instead of file.diff?.jobId.
Two issues:
-
Trailing slash (minor):
serviceUrlis the raw value frominputs.serviceUrl, passed throughDiffCommentDatawithout normalization.ServiceRenderer's constructor strips trailing slashes (service.ts:98), but that doesn't apply here. If a user configuresserviceUrlashttps://svc.example.com/, the generated link becomeshttps://svc.example.com//compare/...(double slash). -
Redundant optional chaining (nitpick): At this point the code is already inside
if (file.diff)(line 377) withconst diff = file.diffin scope.file.diff?.jobIdshould bediff.jobId.
🔧 Proposed fix
- const compareUrl = serviceUrl && file.diff?.jobId
- ? `${serviceUrl}/compare/${file.diff.jobId}?frame=${encodeURIComponent(mod.frameName)}`
- : null;
+ const baseUrl = serviceUrl?.replace(/\/+$/, '');
+ const compareUrl = baseUrl && diff.jobId
+ ? `${baseUrl}/compare/${diff.jobId}?frame=${encodeURIComponent(mod.frameName)}`
+ : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/comment-builder.ts` around lines 396 - 403, The generated compare URL can
get a double slash and uses redundant optional chaining; normalize serviceUrl by
trimming any trailing slash before building compareUrl and use the
already-in-scope diff.jobId instead of file.diff?.jobId. Locate where compareUrl
is created (variable compareUrl in the block that references serviceUrl,
file.diff, mod.frameName and escapeMarkdown) and replace construction to use a
normalized serviceUrl (remove trailing '/' if present) and diff.jobId for the
path, keeping encodeURIComponent(mod.frameName) and existing
escapeMarkdown(mod.frameName) logic unchanged.
1 participant
Summary
jobIdfrom diff API response through to comment builderserviceUrlintoDiffCommentDatafor link generation/compare/:jobId?frame=FrameNameon the screenshot serviceDepends on: RemoteState/pencil-screenshot-service#10
Test plan
npm run buildpasses🤖 Generated with Claude Code
Summary by CodeRabbit