Remove dead options, update tiers, link artifacts in PR comments#6
Remove dead options, update tiers, link artifacts in PR comments#6ritikrishu merged 1 commit intomainfrom
Conversation
…ents - Remove `renderer` and `output-dir` inputs from action.yml (service is the only renderer; output dir is hardcoded) - Delete src/renderers/metadata.ts (unused since service renderer is mandatory) - Clean up types, config, and main.ts to remove all RendererType/outputDir refs - Update service-api-key description with correct tier limits - Wire artifactUrl into CommentData/DiffCommentData so PR comments include a download link for screenshot artifacts - Fix createArtifactsSummary leading \n\n (spacing handled by comment builder) - Remove renderer row from README inputs table Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR consolidates the codebase to use only the service renderer, removing the metadata renderer option entirely. It eliminates the renderer and output-dir configuration inputs, replaces user-configured output directories with a centralized constant, and updates comment generation to include artifact download links. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/github/artifacts.ts (1)
88-97:⚠️ Potential issue | 🟡 MinorThe
fileCountparameter represents frame counts, not file counts — the label(N files)is misleading.In
buildComment, this receivessummary.successfulRenders(a frame count), and inbuildDiffComment, it receivestotalAddedFrames + totalModifiedFrames(also a frame count). The rendered markdown will say something like "Download all screenshots (12 files)" when the number actually refers to rendered frames, not files.Consider renaming the parameter and adjusting the display text:
Proposed fix
export function createArtifactsSummary( - artifactUrl: string | undefined, - fileCount: number + artifactUrl: string | undefined, + screenshotCount: number ): string { if (!artifactUrl) { return ''; } - return `📦 [Download all screenshots (${fileCount} files)](${artifactUrl})`; + return `📦 [Download all screenshots (${screenshotCount} frames)](${artifactUrl})`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/github/artifacts.ts` around lines 88 - 97, The label misrepresents frames as files: update createArtifactsSummary to accept a descriptively named parameter (e.g., frameCount or renderedFrameCount) instead of fileCount and change the returned text to reflect frames (e.g., "Download all screenshots (N frames)"); then update callers (buildComment and buildDiffComment) that pass summary.successfulRenders and totalAddedFrames + totalModifiedFrames to use the renamed parameter so the displayed markdown matches the actual value.
🧹 Nitpick comments (3)
src/main.ts (2)
350-358: Non-null assertion oninputs.serviceUrl!is safe but brittle.This works because
getInputs()throws whenserviceUrlis falsy, but the type still declares it asstring | undefined. If the type inActionInputsis tightened toserviceUrl: string(as suggested in theconfig.tsreview), this assertion can be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 350 - 358, The code uses a brittle non-null assertion on inputs.serviceUrl in initializeRenderer when calling createServiceRenderer; instead update the ActionInputs type so serviceUrl is declared as string (not string | undefined) and adjust getInputs() to always return that type, then remove the unnecessary '!' from inputs.serviceUrl in initializeRenderer (call createServiceRenderer(inputs.serviceUrl, ...)). This keeps initializeRenderer, ActionInputs, getInputs, and createServiceRenderer consistent and removes the brittle assertion.
221-221: Redundantas ServiceRenderercast.
initializeRenderer(line 350) already returnsPromise<ServiceRenderer>, so theas ServiceRenderercast here is unnecessary.Proposed fix
- const renderer = await initializeRenderer(inputs) as ServiceRenderer; + const renderer = await initializeRenderer(inputs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` at line 221, The code redundantly casts the result of initializeRenderer to ServiceRenderer; remove the unnecessary "as ServiceRenderer" cast and simply assign with: const renderer = await initializeRenderer(inputs); ensure you update any usages expecting ServiceRenderer only if typescript inference needs an explicit type annotation (prefer a typed declaration like const renderer: ServiceRenderer = await initializeRenderer(inputs) only if necessary), referencing the initializeRenderer function and the ServiceRenderer type.src/config.ts (1)
18-24:serviceUrl || undefinedon line 24 is dead code after the guard on line 18.Since you throw on line 18 when
!serviceUrl, line 24 is guaranteed to have a truthyserviceUrl— the|| undefinedfallback can never be reached.Additionally,
ActionInputs.serviceUrlis typed asstring | undefined(optional), but this function now enforces its presence. Consider tightening the type toserviceUrl: string(non-optional) so callers likeinitializeRendererdon't need the!non-null assertion.Proposed fix for the redundant fallback
const inputs: ActionInputs = { githubToken: core.getInput('github-token', { required: true }), - serviceUrl: serviceUrl || undefined, + serviceUrl, serviceApiKey: serviceApiKey || undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 18 - 24, The guard in the config loader throws if serviceUrl is falsy, so the "serviceUrl: serviceUrl || undefined" fallback is dead; remove the "|| undefined" and make ActionInputs.serviceUrl a non-optional string (i.e. change its type to string) so callers (e.g., initializeRenderer) no longer need to use the non-null assertion; update the ActionInputs interface/type to reflect serviceUrl: string and adjust any callers that assumed undefined to use the guaranteed string instead.
🤖 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 305-311: The artifact download count currently uses
totalAddedFrames + totalModifiedFrames (computed as totalRendered) which
overstates files for diff-mode artifacts because modified frames are rendered
via the diff endpoint and may not produce local files; update the call that
builds the artifact link (the createArtifactsSummary invocation in
src/comment-builder.ts) to pass a correct count—either use only
data.summary.totalAddedFrames for diff-mode artifacts or compute the actual
number of uploaded/output files from the upload/OUTPUT_DIR logic and pass that
value—so the label matches the real artifact contents.
---
Outside diff comments:
In `@src/github/artifacts.ts`:
- Around line 88-97: The label misrepresents frames as files: update
createArtifactsSummary to accept a descriptively named parameter (e.g.,
frameCount or renderedFrameCount) instead of fileCount and change the returned
text to reflect frames (e.g., "Download all screenshots (N frames)"); then
update callers (buildComment and buildDiffComment) that pass
summary.successfulRenders and totalAddedFrames + totalModifiedFrames to use the
renamed parameter so the displayed markdown matches the actual value.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 18-24: The guard in the config loader throws if serviceUrl is
falsy, so the "serviceUrl: serviceUrl || undefined" fallback is dead; remove the
"|| undefined" and make ActionInputs.serviceUrl a non-optional string (i.e.
change its type to string) so callers (e.g., initializeRenderer) no longer need
to use the non-null assertion; update the ActionInputs interface/type to reflect
serviceUrl: string and adjust any callers that assumed undefined to use the
guaranteed string instead.
In `@src/main.ts`:
- Around line 350-358: The code uses a brittle non-null assertion on
inputs.serviceUrl in initializeRenderer when calling createServiceRenderer;
instead update the ActionInputs type so serviceUrl is declared as string (not
string | undefined) and adjust getInputs() to always return that type, then
remove the unnecessary '!' from inputs.serviceUrl in initializeRenderer (call
createServiceRenderer(inputs.serviceUrl, ...)). This keeps initializeRenderer,
ActionInputs, getInputs, and createServiceRenderer consistent and removes the
brittle assertion.
- Line 221: The code redundantly casts the result of initializeRenderer to
ServiceRenderer; remove the unnecessary "as ServiceRenderer" cast and simply
assign with: const renderer = await initializeRenderer(inputs); ensure you
update any usages expecting ServiceRenderer only if typescript inference needs
an explicit type annotation (prefer a typed declaration like const renderer:
ServiceRenderer = await initializeRenderer(inputs) only if necessary),
referencing the initializeRenderer function and the ServiceRenderer type.
| // Add artifact download link | ||
| const totalRendered = data.summary.totalAddedFrames + data.summary.totalModifiedFrames; | ||
| const artifactLink = createArtifactsSummary(data.artifactUrl, totalRendered); | ||
| if (artifactLink) { | ||
| lines.push(artifactLink); | ||
| lines.push(''); | ||
| } |
There was a problem hiding this comment.
The screenshot count passed for diff mode may not reflect actual artifact contents.
totalAddedFrames + totalModifiedFrames is used as the display count, but in diff mode, modified frames are rendered via the service's diff endpoint and may not produce local files in OUTPUT_DIR. The actual artifact would primarily contain screenshots from added files only. This could show a higher number than the actual files in the downloaded artifact.
Consider using only totalAddedFrames or computing the count from the actual files uploaded, for a more accurate label.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/comment-builder.ts` around lines 305 - 311, The artifact download count
currently uses totalAddedFrames + totalModifiedFrames (computed as
totalRendered) which overstates files for diff-mode artifacts because modified
frames are rendered via the diff endpoint and may not produce local files;
update the call that builds the artifact link (the createArtifactsSummary
invocation in src/comment-builder.ts) to pass a correct count—either use only
data.summary.totalAddedFrames for diff-mode artifacts or compute the actual
number of uploaded/output files from the upload/OUTPUT_DIR logic and pass that
value—so the label matches the real artifact contents.
1 participant
Summary
rendererandoutput-dirfromaction.ymland all supporting code (types, config, main). Deletesrc/renderers/metadata.tsentirely.service-api-keynow reflects correct limits (1K free / 5K with key / 10K with demo)artifactUrlthroughCommentDataandDiffCommentDataso both full-mode and diff-mode comments show a download link for screenshot artifacts\n\nfromcreateArtifactsSummary(spacing handled by comment builder)Test plan
npm run build && npm run packagepasses locally (verified)test/design-update, trigger with empty commit, verify:🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
rendererconfiguration optionoutput-dirconfiguration option