LESSONS — pre-skill test validates v0.3 + surfaces fluidity gap - #5
Merged
Conversation
Two first LESSONS.md entries from Pelle's pre-implement-skill test (2026-05-08) comparing bare Figma MCP vs mapping-skill-only on a 404 page in a third-party Figma kit. Entry 1 [confirmation]: Six failure modes documented in his test (deleted existing page, raw CSS props, parallel styling, random colors, cache leak, generic literal strings) are all addressed by v0.3 rules — rule #5/#2/#3/#12 + B4.5/B7 + mapping-side .gitignore. First dogfood target should be the same 404 page to verify the fixes actually prevent what he observed. Entry 2 [correction]: One gap remains — both bare-MCP and mapping output filled ~40% of viewport instead of 100vw, because the Figma frame is designed at 1440×900 but should be viewport-fill in code. Implement rule #4 translates Figma fill/hug correctly but page-level "fill viewport" intent is not documented in mapping artifacts. Cannot be solved in implement alone. Added DEFERRED-FIXES suggestion #6 (🟢 nice-to-have, mapping repo): "Mapping-side fluidity-intent for page-level frames" — proposes a field in tokens.md or per-component specs marking viewport-fill / max-width / fluid-grid intent so implement can emit the right fluid expression instead of literal Figma dimensions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAppends planning and lessons docs: records v0.3 validation results, notes mapping lacks page-level viewport-fill intent, and proposes a mapping-side ChangesPlanning & Lessons Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRsPoem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@LESSONS.md`:
- Around line 14-17: The lesson entry "[LESSON — 2026-05-08] [correction]" is
currently 4 lines and violates the file's "Strict 5-line format"; fix it by
adding a single blank separator line after the existing fourth line so the entry
becomes five lines total (matching the other entries), preserving the exact
wording and append-only policy in LESSONS.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42a49b7a-3efd-4001-ae16-a35c9b5d6092
📒 Files selected for processing (2)
DEFERRED-FIXES.mdLESSONS.md
Comment on lines
+14
to
+17
| [LESSON — 2026-05-08] [correction] | ||
| Situation: Pelle reported the bare-MCP and mapping-only output filled only ~40% of viewport instead of 100vw, despite the Figma frame being a full-page 1440×900 design. Implement rule #4 covers Figma `fill/hug/gap` translation, but page-level "fill viewport" intent is not explicitly captured in mapping artifacts. | ||
| What did not work: Mapping skill currently has no convention for marking "this page-level frame is a full-viewport intent, emit 100vw/100vh not 1440px". Implement consumes whatever mapping documents — without an intent marker, emit defaults to literal Figma pixels. | ||
| Proposal: Mapping-side enhancement (added as suggestion in DEFERRED-FIXES.md) — extend `tokens.md § Auto-layout conventions` or per-component spec with a "fluidity intent" field for page-level frames. Cannot be solved in implement alone. |
There was a problem hiding this comment.
Add missing blank line to complete 5-line format.
The entry correctly documents the fluidity gap and aligns well with DEFERRED-FIXES.md, but it has only 4 lines. Per the file's own rule (line 5: "Strict 5-line format"), each lesson entry must have exactly 5 lines, including a blank separator. The first entry demonstrates this with a blank line 13.
📝 Proposed fix to add blank separator
Situation: Pelle reported the bare-MCP and mapping-only output filled only ~40% of viewport instead of 100vw, despite the Figma frame being a full-page 1440×900 design. Implement rule `#4` covers Figma `fill/hug/gap` translation, but page-level "fill viewport" intent is not explicitly captured in mapping artifacts.
What did not work: Mapping skill currently has no convention for marking "this page-level frame is a full-viewport intent, emit 100vw/100vh not 1440px". Implement consumes whatever mapping documents — without an intent marker, emit defaults to literal Figma pixels.
Proposal: Mapping-side enhancement (added as suggestion in DEFERRED-FIXES.md) — extend `tokens.md § Auto-layout conventions` or per-component spec with a "fluidity intent" field for page-level frames. Cannot be solved in implement alone.
+As per coding guidelines: "Strict 5-line format, append-only, one rule per entry" (LESSONS.md line 5).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [LESSON — 2026-05-08] [correction] | |
| Situation: Pelle reported the bare-MCP and mapping-only output filled only ~40% of viewport instead of 100vw, despite the Figma frame being a full-page 1440×900 design. Implement rule #4 covers Figma `fill/hug/gap` translation, but page-level "fill viewport" intent is not explicitly captured in mapping artifacts. | |
| What did not work: Mapping skill currently has no convention for marking "this page-level frame is a full-viewport intent, emit 100vw/100vh not 1440px". Implement consumes whatever mapping documents — without an intent marker, emit defaults to literal Figma pixels. | |
| Proposal: Mapping-side enhancement (added as suggestion in DEFERRED-FIXES.md) — extend `tokens.md § Auto-layout conventions` or per-component spec with a "fluidity intent" field for page-level frames. Cannot be solved in implement alone. | |
| [LESSON — 2026-05-08] [correction] | |
| Situation: Pelle reported the bare-MCP and mapping-only output filled only ~40% of viewport instead of 100vw, despite the Figma frame being a full-page 1440×900 design. Implement rule `#4` covers Figma `fill/hug/gap` translation, but page-level "fill viewport" intent is not explicitly captured in mapping artifacts. | |
| What did not work: Mapping skill currently has no convention for marking "this page-level frame is a full-viewport intent, emit 100vw/100vh not 1440px". Implement consumes whatever mapping documents — without an intent marker, emit defaults to literal Figma pixels. | |
| Proposal: Mapping-side enhancement (added as suggestion in DEFERRED-FIXES.md) — extend `tokens.md § Auto-layout conventions` or per-component spec with a "fluidity intent" field for page-level frames. Cannot be solved in implement alone. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@LESSONS.md` around lines 14 - 17, The lesson entry "[LESSON — 2026-05-08]
[correction]" is currently 4 lines and violates the file's "Strict 5-line
format"; fix it by adding a single blank separator line after the existing
fourth line so the entry becomes five lines total (matching the other entries),
preserving the exact wording and append-only policy in LESSONS.md.
Per maintenance policy: skill files (LESSONS.md, DEFERRED-FIXES.md, PLAN.md) should not name individual contributors. Findings stay, attribution shifts to "pre-implement-skill internal testing". Changes: - LESSONS.md entries 1+2: "Pelle ran/reported" → "Pre-implement-skill internal testing ran/reported". Findings unchanged. - DEFERRED-FIXES.md suggestion #6: "Surfaced by Pelle's pre-skill test" → "Surfaced by pre-implement-skill internal testing". Same for "responsiveness gap that Pelle reported" → "responsiveness gap reported in pre-skill testing". - PLAN.md Fase 5: "dogfood op WorQX of Pelle's project" → "dogfood op een echt project met bestaande mapping (b.v. WorQX of een ander team-project)". No skill repo file references individual contributors anymore. Naming is not relevant to the skill itself; what matters is the findings and how the skill addresses them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
k-rutten
changed the title
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
First two LESSONS.md entries based on pre-implement-skill internal testing (2026-05-08). The same 404 page was run through bare Figma MCP and mapping-skill-only — before implement existed — documenting seven specific failure modes plus three recommendations.
Six of seven failure modes are addressed by v0.3 rules. One gap (responsiveness / "design hoeft niet EXACT") needs mapping-side work — added as DEFERRED-FIXES suggestion #6.
Findings mapped against v0.3
setupaddsfigma-context/to.gitignoreWhat this PR contains
LESSONS.md— two entries (one confirmation, one correction):DEFERRED-FIXES.md— new suggestion #6 (🟢 nice-to-have, mapping repo):"Mapping-side fluidity-intent for page-level frames" — proposes a field in
tokens.md § Auto-layout conventionsor per-component specs marking viewport-fill / max-width / fluid-grid intent.Second commit anonymizes skill files — no individual contributor names. Findings stay, attribution shifts to "pre-implement-skill internal testing".
Why this matters
This is the strongest available evidence that v0.3 actually addresses real-world failure modes, because the test predates the skill. It is not a constructed example — it is what bare-MCP and mapping-only produced before implement skill existed.
Logical first dogfood target: re-run the same 404 page with v0.3 installed and verify the six fixes actually prevent the observed failure modes.
Out of scope
Test plan
Summary by CodeRabbit