add waitlist-page skill#555
Conversation
|
Hi @itsmeved24! 🎉 Thanks for the contribution — the waitlist-page skill looks like a solid addition with clean example code and proper documentation structure. I will run a deep review and get back to you within 24h. Thanks for making open-design better! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85bb6f5013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lefarcen
left a comment
There was a problem hiding this comment.
Hi @itsmeved24! Thanks for the well-structured skill submission. The waitlist-page concept is solid and fills a real need, but there are some correctness issues in the templates that need fixing before this can ship.
Key issues:
- Form validation is bypassed (empty/invalid emails show success)
- Template has unsafe token insertion without escaping guidance
- DESIGN.md color token assumptions will lead agents to invent colors
- Checklist contradicts the skill doc on countdown timers
See inline comments for specifics. The good news: these are all fixable without changing the core structure.
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for adding the waitlist-page skill — the structure, example, and checklist are nicely organized. I found one blocking form-flow issue repeated in the template and example: the success state is shown without validating the required email field, so the main waitlist capture path can confirm invalid submissions. Details are inline so each artifact can be fixed directly. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode- Remove novalidate from example.html form - Ensure checkValidity() guard present in both template and example - Remove required from firstname input in template - Add token escaping rules to SKILL.md workflow (step 9) - Add token mapping/fallback rules for BORDER/SUCCESS/STRIPE/DECO (step 7) - Fix mobile quality gate to be measurable (375x667, 390x844) - Promote hardcoded #fff, rgba(0,0,0,0.9), rgba(255,255,255,0.9) to CSS variables (--btn-label, --ticker-bg, --ticker-fg) in template - Create references/checklist.md with P0/P1/P2 tiers; countdown timer is now a hard P0 prohibition; a11y gate split into six specific checks"
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the thoughtful follow-up fixes here — the form validation path is much stronger now, and the skill/checklist structure is moving in the right direction. I found a couple of remaining P0 contract mismatches in the template artifacts that would be good to fix before merging. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode
lefarcen
left a comment
There was a problem hiding this comment.
Nice work addressing the blockers, @itsmeved24! 🎉 All the P1 issues are fixed:
- ✅ Form validation now gates the success state
- ✅ First name is optional as documented
- ✅ Token escaping guidance added
- ✅ DESIGN.md color derivation rules clarified
Two P2 (non-blocking) improvements remain — see inline. The core skill is solid and ready for maintainer approval once these polish items land.
- Add role=status to success messages for screen reader announcement - Replace all hardcoded hex/rgba colors with template tokens - Update SKILL.md with comprehensive color token mapping rules - SVG decorations now use CSS variables instead of hardcoded strokes
lefarcen
left a comment
There was a problem hiding this comment.
Hi @itsmeved24! 👋 Great work fixing the P0 issues from the last review — the template.html now properly derives all colors from tokens and includes aria-live for the success message.
However, this new commit introduces a critical scope issue that needs fixing before merge:
❌ Scope Drift — Must Fix
The PR is titled "add waitlist-page skill" but this commit deletes an entire existing skill (pricing-page):
- ❌
skills/pricing-page/SKILL.md(deleted) - ❌
skills/pricing-page/example.html(deleted)
Pricing-page was a working skill on main. There's no mention in the PR description or commit messages about why it should be removed. This looks like the wrong files were committed by accident.
Could you:
- Remove the pricing-page deletions from this PR (git restore those files or revert the commit that deleted them)
- Remove
temp-original.html(appears to be a scratch file) - Keep only the waitlist-page additions
One PR should do one thing — adding waitlist-page shouldn't delete pricing-page. If pricing-page truly needs removal, that should be a separate PR with its own discussion.
⚠️ Remaining Issues
P0: example.html still has hardcoded colors
skills/waitlist-page/example.html contains hardcoded hex/rgba values throughout (not wrapped in token definitions like template.html does). The checklist says only #2D6A4F for --success is allowed as a hardcoded exception. Either wrap these in CSS custom properties at the top (following template.html's pattern), or derive them from DESIGN.md tokens per the skill workflow.
P1: Scratch file committed
temp-original.html should not be committed. Please remove it from the PR.
Once these three items are fixed (scope + example colors + temp file), the PR will be ready for final approval!
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for continuing to tighten up the waitlist-page skill — the validation and accessibility fixes are good progress. I found one blocking regression outside the new waitlist artifacts, plus a couple of merge-safe template issues that would be worth addressing while you are here. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencodeFixes:
- Restore pricing-page files accidentally deleted in previous commit
(skills/pricing-page/SKILL.md and example.html now back on branch)
- Remove temp-original.html scratch file from commit
- Fix mobile viewport scrolling: change 'height: 100vh; overflow: hidden'
to 'min-height: 100svh; overflow-x: hidden; overflow-y: auto'
so content doesn't clip on 375×667 and 390×844 screens
- Split font tokens into URL-safe and CSS-safe variants:
* {{DISPLAY_FONT_URL}} and {{DISPLAY_FONT_CSS}} for display fonts
* {{BODY_FONT_URL}} and {{BODY_FONT_CSS}} for body fonts
This fixes encoding: spaces as '+' in Google Fonts URL, literal in CSS
- Update SKILL.md frontmatter with new font input fields
- Update token escaping rules to document the split
lefarcen
left a comment
There was a problem hiding this comment.
Excellent progress, @itsmeved24! 🎉 You've fixed the critical scope issue and both P2 template concerns:
✅ Scope drift fixed — pricing-page is restored, temp-original.html is gone. The PR now adds only waitlist-page as intended.
✅ Font token split — template.html now uses separate URL-encoded and CSS-quoted font tokens.
✅ Viewport scrolling — template + example now use min-height: 100svh with overflow-y: auto, no more clipping on short mobile screens.
One P0 remains:
❌ example.html still has hardcoded colors — the runnable example contains direct hex/rgba values (lines 12-20, 47, 114, 139, 142, 151, 211, 281, 306, 331) instead of deriving all colors from design system tokens like template.html does. The checklist P0 gate says only #2D6A4F for --success is allowed as a hardcoded exception.
Could you either:
- Wrap example.html colors in CSS custom properties at the top (like template.html's token vars), OR
- Derive them from DESIGN.md tokens per the skill workflow
Once example.html matches the color contract, this will be ready for final approval!
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the continued fixes here — the prior validation, accessibility, scope, and mobile scrolling issues look much improved. I found one remaining token-contract mismatch in the template that can make generated pages emit invalid CSS when the skill instructions are followed. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode… example.html
P0 Fixes:
- Remove all hardcoded colors from example.html (except #2D6A4F for --success)
- Use CSS variables for all color values: --btn-label, --ticker-bg, --ticker-fg, --deco-stroke
- Fix gradient to use var(--deco) instead of hardcoded #D1632B
- Apply consistent color expressions across decorations and text
Token Contract Fixes:
- template.html now uses full CSS expressions for opacity-based colors:
* {{BORDER_EXPRESSION}} instead of {{BORDER_HEX}} (no # prefix)
* {{BTN_LABEL_EXPRESSION}} instead of {{BTN_LABEL_HEX}}
* {{TICKER_BG_EXPRESSION}}, {{TICKER_FG_EXPRESSION}}, {{DECO_STROKE_EXPRESSION}}
- Remove extra quotes from font tokens in template:
* --font-body: {{BODY_FONT_CSS}} instead of '{{BODY_FONT_CSS}}'
* Font tokens are already quoted if needed, no wrapping
- Update SKILL.md frontmatter with all color expression inputs and descriptions
- Update token mapping rules to clarify the new contract:
* Hex tokens: simple six-digit colors
* Expression tokens: full CSS values (rgba/color-mix), no # prefix
* Font tokens: CSS font-family values, no extra wrapping
- Update token escaping rules to reflect new contract
This ensures agents can follow SKILL.md instructions without producing invalid CSS.
lefarcen
left a comment
There was a problem hiding this comment.
Great progress, @itsmeved24! 🎉 You've now fixed both the template token contract issue (mrcfps's P0) and most of the example.html hardcoded colors. Just a few residual hardcoded colors remain in example.html — fix these and it's ready!
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for continuing to tighten this up — the template token contract now looks consistent, and the earlier form/accessibility/scope fixes are good progress. I found one remaining blocking mismatch in the runnable example's color-token wiring that keeps it from satisfying the skill's P0 color contract. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode- Button text: #fff → var(--btn-label) - Ticker background: rgba(0,0,0,0.9) → var(--ticker-bg) - Ticker text: rgba(255,255,255,0.9) → var(--ticker-fg) - Logo text: fill=white → fill=var(--btn-label) All colors now derive from design system tokens. Only #2D6A4F (--success) allowed hardcoded exception.
lefarcen
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the follow-up — the workflow blocker is now resolved. SKILL.md explicitly instructs agents to read assets/template.html first and run references/checklist.md P0 gates, so the hardened path is properly wired into the skill execution flow.
All issues from the full review history (validation, token contracts, color derivation, accessibility, i18n registry, and workflow instructions) are resolved. Ready from my side. 🎉
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful follow-up fixes — the validation, token wiring, accessibility, i18n, and template workflow are much stronger now. I found one remaining blocker in the hardened template path: following the new “replace only placeholders” instruction can still emit the visible logo placeholder instead of a real brand mark. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode- Replaced `[LOGO]` with `{{LOGO_MARK}}` in template.html
- Added `logo_mark` to inputs in SKILL.md
- Updated mapping rules in SKILL.md to handle raw SVG or text for logo
- Updated P0 validation gates in SKILL.md and checklist.md to ensure logo replacement
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful follow-up on the waitlist-page skill — the template/checklist path is much stronger now. I found one remaining blocker in the new logo token handling because it explicitly permits raw, unsanitized HTML from an input. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode… token - Mandate HTML-escaping for text initials. - Enforce strict allowlist-based sanitization for inline SVG (stripping `<script>`, `on*`, `<foreignObject>`, `href`, `xlink:href`, `url()`). - Add fallback to escaped text initials for invalid/unsafe SVG.
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful follow-up here — the template/checklist path and logo sanitization guidance are much stronger now. I found one remaining blocker where the frontmatter input description still contradicts the hardened logo rule, so agents can still treat the logo as raw HTML. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode- Updated the `logo_mark` input description in the SKILL.md frontmatter to explicitly outline the new requirements for HTML-escaped text or strict allowlist-sanitized SVG.
lefarcen
left a comment
There was a problem hiding this comment.
Perfect, @itsmeved24! 🎉 The final P0 is resolved — the logo_mark frontmatter description now correctly matches the hardened sanitization rule (escaped text initials by default, or allowlist-sanitized inline SVG only with unsafe markup rejected/falling back to text).
All issues from the entire review cycle are addressed:
✅ Form validation enforced (email required, checkValidity() gate)
✅ Design system colors properly derived (no hardcoded exceptions beyond allowed #2D6A4F)
✅ Token escaping + sanitization complete (HTML-escaped text, strict SVG allowlist)
✅ Accessibility gates met (aria-live + focus indicator)
✅ Mobile responsive (scrollable at short viewports)
✅ Integration complete (i18n registry updated)
✅ Workflow instructions wire template + checklist
✅ Frontmatter + logo rule now consistent (no XSS vulnerability)
The waitlist-page skill is complete. Looks good to me; deferring final approval to a maintainer. Thanks for the thorough iteration across all the reviews! 🚀
— open-design team
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful follow-up on the waitlist-page skill — the template/checklist path and logo sanitization rules are much stronger now. I found one remaining blocker in the template fallback path where text-initial logos can disappear against the default badge color, so generated pages can still miss the visible brand mark requirement. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode…ogo mark text fallback
- Added `--logo-fg` CSS variable mapped to `{{LOGO_FG_EXPRESSION}}`.
- Updated `.logo-container` in `template.html` to inherit typography styles and apply `--logo-fg` for safe fallback when rendering escaped initials.
- Enforced WCAG AA contrast for logo initials against container background in `checklist.md`.
mrcfps
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful iteration on the waitlist-page skill — the template/checklist path, i18n registration, accessibility, and logo handling are much stronger now. I found one remaining blocker in the new color-token contract before this ships. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode…s to prevent CSS injection
lefarcen
left a comment
There was a problem hiding this comment.
Perfect, @itsmeved24! 🎉 The final P0 is resolved — the logo text visibility issue is fixed:
✅ .logo-container now has explicit color: var(--logo-fg); plus font styling for text initials
✅ {{LOGO_FG_EXPRESSION}} is documented as a required input with contrast guidance
✅ Token mapping is complete (SKILL.md inputs → template placeholders → CSS variables)
All issues from the full review history are addressed:
✅ Form validation enforced (email required, checkValidity() gate)
✅ Design system colors properly derived (now supports full CSS expressions: rgba(), color-mix(), etc.)
✅ Token escaping + sanitization complete (HTML-escaped text, strict SVG allowlist)
✅ Accessibility gates met (aria-live + focus indicator + logo text contrast)
✅ Mobile responsive (scrollable at short viewports)
✅ Integration complete (i18n registry updated)
✅ Workflow instructions wire template + checklist
✅ Logo text contrast ensured (explicit --logo-fg token)
One minor P2 (non-blocking documentation nit): SKILL.md:159 still mentions the old hex regex in the validation summary — consider changing it to say "color tokens must be valid CSS expressions" to match the new token model. But this doesn't block merge.
The waitlist-page skill is complete and merge-ready from my side. Thanks for the thorough iteration across all the reviews! 🚀
— open-design team
lefarcen
left a comment
There was a problem hiding this comment.
@itsmeved24 thanks for the careful iteration here — the color-token validation is now complete. 🎉
The latest push addresses mrcfps's final P0: SKILL.md now explicitly constrains color expression tokens to a strict color-only grammar (#hex, rgb/rgba, hsl/hsla, oklch, color-mix() with local vars only) and documents hard rejects for CSS injection primitives (;, {}, <>, comments, @, url(, external refs). The same rule is mirrored in references/checklist.md P0 gate.
All issues from the full review cycle are now resolved. This waitlist-page skill is ready to ship. 🚀
|
All blockers from the review cycle are resolved and checks are passing. Ready for final approval. |
|
@mrcfps requesting final approval!! |
|
Hey @lefarcen, thanks again for the detailed review cycle and guidance
throughout this PR — learned a lot from it.
Just wanted to check what the next step is from here. Since all blockers
are resolved and the skill is ready to ship, should I wait for final
approvals/merge now or is there anything else I should do on my end?
…On Wed, 6 May 2026 at 01:37, lefarcen ***@***.***> wrote:
***@***.**** commented on this pull request.
@itsmeved24 <https://github.com/itsmeved24> thanks for the careful
iteration here — the color-token validation is now complete. 🎉
The latest push addresses mrcfps's final P0: SKILL.md now explicitly
constrains color expression tokens to a strict color-only grammar (#hex,
rgb/rgba, hsl/hsla, oklch, color-mix() with local vars only) and
documents hard rejects for CSS injection primitives (;, {}, <>, comments,
@, url(, external refs). The same rule is mirrored in
references/checklist.md P0 gate.
All issues from the full review cycle are now resolved. This waitlist-page
skill is ready to ship. 🚀
—
Reply to this email directly, view it on GitHub
<#555 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASDJZZZYHSQRVUXL6PYIJNT4ZJCW7AVCNFSM6AAAAACYRZTOTCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DEMZRGI3TENZQGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hey @itsmeved24! 🙏 Thanks for the kind words and for the thorough iteration throughout this review cycle — you've shipped a really solid skill here. Current status: All code issues are resolved from my side. The PR is technically ready to merge, but it's currently blocked by stale review state from an earlier iteration. @mrcfps's last CHANGES_REQUESTED review was on an old commit (441c02c), and you've since addressed that final blocker on the current head (86746ae). Next steps: You're all set from your end — no further changes needed. The PR just needs either:
I've already flagged this to the team. Just sit tight for now — they'll pick it up when available. 🚀 Thanks for making open-design better! |
lefarcen
left a comment
There was a problem hiding this comment.
Thanks for the follow-up push here — verified that all prior P0/P1 fixes remain intact (validation gating, token safety, logo sanitization/contrast, mobile/accessibility, i18n registration). No new issues found in the current diff.
Looks good to me; deferring final approval to a maintainer.
What
A new skill for building pre-launch landing pages. Clean, minimal design with a brand logo, email capture form, and optional decorative section with wavy separator, accent stripe, perspective grid, and animated ticker ribbon. Fully responsive from mobile (375px) to desktop (1440px+).
Why
Pre-launch pages are a core artifact type for product launches and beta signups. This skill provides a reusable template that reads the active design system for colors and typography, so every page stays on-brand.
Files added
Quality checklist
references/checklist.mdwith P0 quality gatesTesting
Renders cleanly at 375px, 768px, 1440px. Form submission hides the form and shows success message. All SVG graphics (coil, grid, ticker) animate smoothly.