feat: visual validation, notable components, and Figma context improvements#254
Open
rubenmarcus wants to merge 1 commit intomainfrom
Open
feat: visual validation, notable components, and Figma context improvements#254rubenmarcus wants to merge 1 commit intomainfrom
rubenmarcus wants to merge 1 commit intomainfrom
Conversation
…d Figma context - Add visual comparison validation (pixelmatch + LLM vision) that runs on final iterations when Figma screenshots are present, with automatic 2-iteration extension to fix visual issues - Add --no-visual-check CLI flag to disable visual validation - Add dev-server module for launching/capturing implementation screenshots - Detect notable sub-components in Figma sections (scroll indicators, social sidebars, nav elements) with position hints and scroll behavior - Improve context-builder Figma instructions: z-index stacking clarity, STRETCH→cover guidance, scroll indicator patterns, completeness checklist, absolute positioning precision - Enhance plan-generator with inner shadow CSS output, deeper node traversal (depth 8), and text-descendant inclusion at deep levels - Add vision call tracking to CostTracker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Issue Linking ReminderThis PR doesn't appear to have a linked issue. Consider linking to:
Using If this PR doesn't need an issue, you can ignore this message. |
github-actions
bot
added
candidate-release
Contributor
|
| Metric | Value |
|---|---|
| Base | 2234.84 KB |
| PR | 2314.06 KB |
| Diff | 79.22 KB (3.00%) |
Bundle breakdown
156K dist/auth
32K dist/automation
4.0K dist/cli.d.ts
4.0K dist/cli.d.ts.map
16K dist/cli.js
12K dist/cli.js.map
528K dist/commands
28K dist/config
4.0K dist/index.d.ts
4.0K dist/index.d.ts.map
4.0K dist/index.js
4.0K dist/index.js.map
828K dist/integrations
84K dist/llm
884K dist/loop
188K dist/mcp
32K dist/presets
92K dist/setup
40K dist/skills
392K dist/sources
76K dist/ui
124K dist/utils
336K dist/wizard
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+370
to
+397
| body: JSON.stringify({ | ||
| model: model || config.defaultModel, | ||
| max_tokens: 2048, | ||
| messages: [ | ||
| { role: 'system', content: VISION_SYSTEM_PROMPT }, | ||
| { | ||
| role: 'user', | ||
| content: [ | ||
| { type: 'text', text: 'TARGET DESIGN from Figma:' }, | ||
| { | ||
| type: 'image_url', | ||
| image_url: { url: `data:image/png;base64,${designBase64}` }, | ||
| }, | ||
| { type: 'text', text: 'CURRENT IMPLEMENTATION:' }, | ||
| { | ||
| type: 'image_url', | ||
| image_url: { url: `data:image/png;base64,${implBase64}` }, | ||
| }, | ||
| { type: 'text', text: 'PIXEL DIFF OVERLAY (red = different):' }, | ||
| { | ||
| type: 'image_url', | ||
| image_url: { url: `data:image/png;base64,${diffBase64}` }, | ||
| }, | ||
| { type: 'text', text: VISION_COMPARISON_PROMPT }, | ||
| ], | ||
| }, | ||
| ], | ||
| }), |
Check warning
Code scanning / CodeQL
File data in outbound network request Medium
Contributor
Greptile SummaryThis PR adds a three-layer visual validation pipeline (pixelmatch → LLM Vision → strict pixel gate) that automatically compares implementation screenshots against Figma design screenshots on the final loop iteration, with 2-iteration auto-extension to fix issues. It also introduces notable component detection (scroll indicators, social sidebars, nav elements) in Figma plan generation and improves Figma context with better z-index stacking guidance, STRETCH→cover mapping, and a completeness checklist.
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Executor as Loop Executor
participant VV as Visual Validation
participant DS as Dev Server
participant PW as Playwright
participant PM as pixelmatch
participant LLM as LLM Vision API
participant CT as Cost Tracker
Executor->>VV: runVisualValidation(cwd, screenshots)
VV->>PW: ensurePlaywright()
alt Not installed
PW-->>PW: npm install -g playwright
end
VV->>DS: startDevServer(cwd)
DS->>DS: detectDevCommand(cwd)
DS->>DS: waitForServer(url)
DS-->>VV: DevServerInfo {url, port, kill}
VV->>PW: captureScreenshot(url)
PW-->>VV: implScreenshot (PNG Buffer)
loop For each design screenshot
VV->>PM: pixelDiff(design, impl)
PM-->>VV: {diffRatio, diffImage}
alt diffRatio ≤ 5% (normal) or ≤ 2% (strict)
VV-->>VV: PASS — skip LLM
else diffRatio > threshold
VV->>LLM: compareWithLLMVision(design, impl, diff)
LLM-->>VV: {matches, issues, usage}
end
end
VV->>DS: server.kill()
VV-->>Executor: VisualValidationResult
Executor->>CT: recordVisionCall(usage)
alt Issues found & first pass
Executor-->>Executor: Extend loop +2 iterations
Note over Executor: Agent fixes issues
Executor->>VV: runVisualValidation(strictMode=true)
end
Last reviewed commit: c14b83e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+125
to
+128
| try { | ||
| await execa('npm', ['install', '-g', 'playwright'], { | ||
| timeout: 120_000, | ||
| reject: true, |
Contributor
There was a problem hiding this comment.
Global npm install -g has side effects and may fail in restricted environments
Auto-installing Playwright globally with npm install -g playwright can fail in CI, Docker containers, or systems without global npm write access. It also bypasses the project's package manager (the repo uses pnpm). Consider using the project's package manager (pnpm add -D playwright), installing locally, or documenting this requirement so users can pre-install Playwright.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/loop/visual-validation.ts
Line: 125-128
Comment:
**Global `npm install -g` has side effects and may fail in restricted environments**
Auto-installing Playwright globally with `npm install -g playwright` can fail in CI, Docker containers, or systems without global npm write access. It also bypasses the project's package manager (the repo uses pnpm). Consider using the project's package manager (`pnpm add -D playwright`), installing locally, or documenting this requirement so users can pre-install Playwright.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+123
to
+129
| return { | ||
| url, | ||
| port: devCmd.port, | ||
| process: null as unknown as ResultPromise, | ||
| kill: async () => {}, // Nothing to kill — not our process | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Unsafe cast of null to ResultPromise for pre-existing server
When a server is already running on the detected port, process is set to null as unknown as ResultPromise. Any code that accesses server.process (e.g., checking .pid, calling methods on it) will get a runtime error. Consider making process optional (process?: ResultPromise) in the DevServerInfo interface, or using a discriminated union, so callers handle this case explicitly.
Suggested change
| return { | |
| url, | |
| port: devCmd.port, | |
| process: null as unknown as ResultPromise, | |
| kill: async () => {}, // Nothing to kill — not our process | |
| }; | |
| } | |
| return { | |
| url, | |
| port: devCmd.port, | |
| process: undefined as unknown as ResultPromise, | |
| kill: async () => {}, // Nothing to kill — not our process | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/loop/dev-server.ts
Line: 123-129
Comment:
**Unsafe cast of `null` to `ResultPromise` for pre-existing server**
When a server is already running on the detected port, `process` is set to `null as unknown as ResultPromise`. Any code that accesses `server.process` (e.g., checking `.pid`, calling methods on it) will get a runtime error. Consider making `process` optional (`process?: ResultPromise`) in the `DevServerInfo` interface, or using a discriminated union, so callers handle this case explicitly.
```suggestion
return {
url,
port: devCmd.port,
process: undefined as unknown as ResultPromise,
kill: async () => {}, // Nothing to kill — not our process
};
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (2)
The Prompt To Fix With AIThis is a comment left during a code review.
Path: src/loop/cost-tracker.ts
Line: 454-456
Comment:
**`reset()` doesn't clear vision tracking state**
The `reset()` method clears `this.iterations` but does not reset the new `_visionCalls` and `_visionCost` fields. If `reset()` is called (e.g., between sessions), the vision metrics will carry stale data, producing incorrect stats.
```suggestion
reset(): void {
this.iterations = [];
this._visionCalls = 0;
this._visionCost = 0;
}
```
How can I resolve this? If you propose a fix, please make it concise.
The Lines affected:
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/integrations/figma/parsers/design-spec.ts
Line: 608
Comment:
**Inconsistent z-index guidance (1 vs 10)**
The `context-builder.ts` and `plan-generator.ts` were updated in this PR to recommend `z-index: 10` for text/content layers over background images. However, `design-spec.ts` still emits `z-index: 1` in several places (lines 608, 670, 674, 704). This creates contradictory guidance — the context tells the agent "use z-index 10+" while the spec output says "z-index: 1". When there are multiple stacked image layers (z-0 through z-4), z-index: 1 will place text *behind* higher image layers, which is exactly the bug this PR aims to fix.
Lines affected:
- Line 608: `z-index: 1` for child content over background image
- Line 670: `z-index: 1` for text/content in composite hero parallax
- Line 674: `z-index: 1` for content in smaller composite backgrounds
- Line 704: `z-index: 1` for text/content in non-text-overlay composites
```suggestion
` * All child content must use \`position: relative; z-index: 10\` to appear above the image`
```
How can I resolve this? If you propose a fix, please make it concise. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dev-server.ts(Vite launcher + screenshot capture),visual-validation.ts(pixelmatch + vision API comparison)--no-visual-checkflag, vision cost tracking in CostTrackerTest plan
pnpm build— passesralph-starter runwith a Figma spec that has screenshots inpublic/images/screenshots/— visual validation triggers on final iteration--no-visual-checkdisables visual validation🤖 Generated with Claude Code