feat: codex skill for custom component generation#1069
Open
briantstephan wants to merge 8 commits intomainfrom
Open
feat: codex skill for custom component generation#1069briantstephan wants to merge 8 commits intomainfrom
briantstephan wants to merge 8 commits intomainfrom
Conversation
Contributor
WalkthroughAdds a new VE HTML Template Generator skill consisting of documentation, an OpenAI agent interface, and multiple Python CLI tools. Documentation files define template contracts, slot/field patterns, section normalization, layout containment, parity/testing workflows, quality checklists, and reuse maps. The OpenAI agent interface (openai.yaml) and README/SKILL docs are included. New scripts provide HTML section extraction, scaffolding for smoke/section/visual parity tests, and a comprehensive client-template validator. All changes are documentation and tooling; no runtime product code or exported public APIs were modified. Sequence Diagram(s)sequenceDiagram
participant User
participant Agent as "VE HTML Template\nGenerator Agent"
participant Generator as "Template\nGenerator (skill)"
participant FS as "Repository\n/Starter Files"
participant TestRunner as "Parity/Test\nRunner"
participant Validator as "validate_client_template.py"
User->>Agent: submit client slug + HTML path + options
Agent->>Generator: invoke generation workflow / produce artifacts
Generator->>FS: write client components, starter templates, config
FS->>User: generated files available
User->>TestRunner: scaffold/run parity & smoke tests
TestRunner->>FS: read templates and source HTML, capture screenshots
TestRunner->>User: present diffs / parity results
User->>Validator: run validation checks on client template
Validator->>FS: analyze files, categories, wiring
Validator-->>User: report failures or OK
alt parity or validation failures
User->>FS: apply corrections
User->>TestRunner: rerun tests
end
User->>FS: finalize and build starter/package
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
skills/ve-html-template-generator/references/ve-field-patterns.md (1)
156-165: Clarify conflicting URL guidance for background media.Line 156 suggests URL fields for “background URLs,” but Line 164 disallows URL controls for core background image/map sources. Tighten this section so link URLs and media/map source rules are unambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/references/ve-field-patterns.md` around lines 156 - 165, Clarify the conflicting guidance by separating link URL use from core media/map sources: state that simple link fields should use the existing pattern (e.g. ctaUrl: YextField("CTA URL", { type: "text" })) and for localized links use a translatable-string strategy rather than entityField with type.url; explicitly prohibit using plain text URL fields for core background media (background images, map embeds, directions source URLs) and instruct authors to use entity-backed/shared patterns (e.g. entityField or designated shared-media patterns) for those cases so the notes about ctaUrl and entityField/type.url are unambiguous.skills/ve-html-template-generator/scripts/extract_page_sections.py (2)
60-62: Inconsistentto_pascal_caseimplementation across scripts.This implementation uses
part.capitalize()which lowercases the rest of each word (e.g., "FAQ" → "Faq"), while other scripts in this directory usepart[:1].upper() + part[1:]which preserves case (e.g., "FAQ" → "FAQ"). This could cause naming mismatches if outputs from this script are used with the other scaffold/validate scripts.♻️ Align with other scripts' implementation
def to_pascal_case(text: str) -> str: parts = [part for part in to_slug(text).split("-") if part] - return "".join(part.capitalize() for part in parts) or "Section" + return "".join(part[:1].upper() + part[1:] for part in parts) or "Section"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/extract_page_sections.py` around lines 60 - 62, The to_pascal_case function uses part.capitalize() which lowercases the rest of each segment and breaks acronyms like "FAQ"; change the casing logic in to_pascal_case to use the same approach as other scripts (preserve existing character case) by transforming each part with part[:1].upper() + part[1:] before joining, ensuring the function (to_pascal_case) produces PascalCase while preserving internal capitalization and matching the other scaffold/validate scripts.
25-28: Nested semantic tags won't be handled correctly.The regex
(.*?)</\1>will match to the first closing tag of the same type, not the correctly paired one. For example,<section><section>inner</section></section>will extract only<section>inneras the inner content. Given the docstring explicitly states "best-effort helper," this is acceptable, but worth noting in case users encounter malformed extractions on complex HTML.skills/ve-html-template-generator/scripts/validate_client_template.py (1)
691-693: Redundant condition in hours type check.The check
'types: ["type.hours"]' in text or 'types: ["type.hours"]' in text.replace(" ", "")is redundant—if the first condition matches, the second will also match (since removing spaces doesn't remove existing matches). The intent seems to be checking both spaced and non-spaced variants.♻️ Simplify by checking the no-spaces variant only
- if 'types: ["type.hours"]' in text or 'types: ["type.hours"]' in text.replace( - " ", "" - ): + if 'types:["type.hours"]' in text.replace(" ", ""): has_hours_field_binding = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/validate_client_template.py` around lines 691 - 693, The condition checking for the hours type is redundant; normalize the input by removing spaces once and check the no-space pattern only (e.g., use text_no_spaces = text.replace(" ", "") and then test if 'types:["type.hours"]' in text_no_spaces) instead of the current OR with both variants; update the conditional that currently uses 'types: ["type.hours"]' in text or 'types: ["type.hours"]' in text.replace(" ", "") to this single normalized check so only one membership test is performed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/ve-html-template-generator/references/template-contract.md`:
- Around line 67-70: The imports in <Client>SectionsCategory.tsx and
<Client>SlotsCategory.tsx currently reference
../generated/<client>/components/... which conflicts with the expected
custom/<client>/components layout; update those import statements to import
section/slot components from custom/<client>/components/... (matching the
directory layout used earlier in this file), and ensure you still export the
props interfaces and the <Client>...CategoryComponents maps (the same named
exports in these files) so consumers get the correct component mappings.
In
`@skills/ve-html-template-generator/references/test-and-screenshot-workflow.md`:
- Around line 18-21: The documentation uses incorrect scaffold command paths;
update the three commands that call the script
scaffold_client_template_smoke_test.py so they point to the actual script
location under skills/ve-html-template-generator/scripts/ instead of scripts/;
specifically, replace occurrences of "python3
scripts/scaffold_client_template_smoke_test.py" with "python3
skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py"
(keeping the existing --client-slug, --template-path and --config-path arguments
intact) in the affected commands within the file.
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py`:
- Around line 292-326: getSectionKeywords currently contains brand-specific
tokens (e.g., "mcdonald", "westminster") and is too permissive, and pairSections
allows reusing the same source section; update getSectionKeywords to return
small, generic, client-agnostic keyword sets per sectionType (e.g.,
["menu","orders"] for header, ["hours","map"] for operations, avoid brand/place
names), and modify pairSections to enforce one-to-one pairing by tracking and
marking source sections as used so a source section cannot be paired to multiple
generated sections; update references to the functions getSectionKeywords and
pairSections accordingly.
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py`:
- Line 106: config_symbol is built by concatenating client_slug into a
TypeScript identifier which breaks for slugs with hyphens; create and use a
sanitization helper (e.g., slug_to_camel or sanitize_identifier) to convert
client_slug into a valid camelCase TS identifier (remove/replace invalid chars,
convert hyphenated parts to camelCase, ensure it starts with a letter or
underscore and prefix with '_' if it starts with a digit) and then construct
config_symbol as f"{sanitized_slug}Config"; update any places that reference
config_symbol in TypeScript imports/exports to use the sanitized value.
In `@skills/ve-html-template-generator/SKILL.md`:
- Around line 78-80: Update all occurrences of the command "python3
scripts/extract_page_sections.py ..." in SKILL.md so the script path is correct:
either prepend the skill directory
("skills/ve-html-template-generator/scripts/extract_page_sections.py ...") or
add a clear note telling users to run the examples from the skill root
(skills/ve-html-template-generator/) so the relative "scripts/..." path works;
ensure you change every repeated block that uses
"scripts/extract_page_sections.py" to one consistent approach.
---
Nitpick comments:
In `@skills/ve-html-template-generator/references/ve-field-patterns.md`:
- Around line 156-165: Clarify the conflicting guidance by separating link URL
use from core media/map sources: state that simple link fields should use the
existing pattern (e.g. ctaUrl: YextField("CTA URL", { type: "text" })) and for
localized links use a translatable-string strategy rather than entityField with
type.url; explicitly prohibit using plain text URL fields for core background
media (background images, map embeds, directions source URLs) and instruct
authors to use entity-backed/shared patterns (e.g. entityField or designated
shared-media patterns) for those cases so the notes about ctaUrl and
entityField/type.url are unambiguous.
In `@skills/ve-html-template-generator/scripts/extract_page_sections.py`:
- Around line 60-62: The to_pascal_case function uses part.capitalize() which
lowercases the rest of each segment and breaks acronyms like "FAQ"; change the
casing logic in to_pascal_case to use the same approach as other scripts
(preserve existing character case) by transforming each part with
part[:1].upper() + part[1:] before joining, ensuring the function
(to_pascal_case) produces PascalCase while preserving internal capitalization
and matching the other scaffold/validate scripts.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py`:
- Around line 691-693: The condition checking for the hours type is redundant;
normalize the input by removing spaces once and check the no-space pattern only
(e.g., use text_no_spaces = text.replace(" ", "") and then test if
'types:["type.hours"]' in text_no_spaces) instead of the current OR with both
variants; update the conditional that currently uses 'types: ["type.hours"]' in
text or 'types: ["type.hours"]' in text.replace(" ", "") to this single
normalized check so only one membership test is performed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
skills/ve-html-template-generator/README.mdskills/ve-html-template-generator/SKILL.mdskills/ve-html-template-generator/agents/openai.yamlskills/ve-html-template-generator/references/baseline-reuse-map.mdskills/ve-html-template-generator/references/client-config-category-pattern.mdskills/ve-html-template-generator/references/layout-containment.mdskills/ve-html-template-generator/references/map-image-parity.mdskills/ve-html-template-generator/references/quality-checklist.mdskills/ve-html-template-generator/references/section-normalization.mdskills/ve-html-template-generator/references/slot-paradigm.mdskills/ve-html-template-generator/references/template-contract.mdskills/ve-html-template-generator/references/test-and-screenshot-workflow.mdskills/ve-html-template-generator/references/ve-field-patterns.mdskills/ve-html-template-generator/scripts/extract_page_sections.pyskills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.pyskills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.pyskills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.pyskills/ve-html-template-generator/scripts/validate_client_template.py
skills/ve-html-template-generator/references/test-and-screenshot-workflow.md
Outdated
Show resolved
Hide resolved
skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py
Show resolved
Hide resolved
skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/ve-html-template-generator/scripts/validate_client_template.py (1)
34-36: Inconsistentto_pascal_caseimplementation across scripts.This implementation uses
re.split(r"[^A-Za-z0-9]+", value)which handles all non-alphanumeric characters, while other scaffold scripts usevalue.replace("_", "-").split("-")which only handles hyphens and underscores. This implementation is more robust but the inconsistency could cause subtle differences in behavior.Consider consolidating these utilities into a shared module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/validate_client_template.py` around lines 34 - 36, The to_pascal_case function in validate_client_template.py uses re.split(r"[^A-Za-z0-9]+", value) which is more permissive than other scaffold scripts that use value.replace("_", "-").split("-"); extract this PascalCase utility into a shared helper (e.g., a new common utils module) and replace the local implementation in validate_client_template.py with an import from that module, or conversely update all scaffold scripts to import the re.split-based implementation so behavior is consistent; ensure the shared function name is to_pascal_case and update all callers to use it (and add tests or comments documenting the splitting behavior).skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py (1)
12-22: Duplicate utility functions across scaffold scripts.
to_pascal_caseandnormalize_import_pathare duplicated acrossscaffold_client_template_section_parity_test.py,scaffold_client_template_smoke_test.py, andscaffold_client_template_visual_parity_test.py. Consider extracting these to a shared utility module to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py` around lines 12 - 22, The two helper functions to_pascal_case and normalize_import_path are duplicated across the three scaffold scripts; extract them into a new shared utility module (e.g., scaffold_utils) and replace the local definitions with imports. Specifically, create a module exposing to_pascal_case and normalize_import_path, ensure it imports Path and os as needed, then update scaffold_client_template_section_parity_test.py, scaffold_client_template_smoke_test.py, and scaffold_client_template_visual_parity_test.py to remove the local function definitions and add from scaffold_utils import to_pascal_case, normalize_import_path (adjusting the import path to the correct package-relative location). Run tests or linters to confirm there are no missing imports or name collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py`:
- Around line 12-22: The two helper functions to_pascal_case and
normalize_import_path are duplicated across the three scaffold scripts; extract
them into a new shared utility module (e.g., scaffold_utils) and replace the
local definitions with imports. Specifically, create a module exposing
to_pascal_case and normalize_import_path, ensure it imports Path and os as
needed, then update scaffold_client_template_section_parity_test.py,
scaffold_client_template_smoke_test.py, and
scaffold_client_template_visual_parity_test.py to remove the local function
definitions and add from scaffold_utils import to_pascal_case,
normalize_import_path (adjusting the import path to the correct package-relative
location). Run tests or linters to confirm there are no missing imports or name
collisions.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py`:
- Around line 34-36: The to_pascal_case function in validate_client_template.py
uses re.split(r"[^A-Za-z0-9]+", value) which is more permissive than other
scaffold scripts that use value.replace("_", "-").split("-"); extract this
PascalCase utility into a shared helper (e.g., a new common utils module) and
replace the local implementation in validate_client_template.py with an import
from that module, or conversely update all scaffold scripts to import the
re.split-based implementation so behavior is consistent; ensure the shared
function name is to_pascal_case and update all callers to use it (and add tests
or comments documenting the splitting behavior).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
skills/ve-html-template-generator/SKILL.mdskills/ve-html-template-generator/references/template-contract.mdskills/ve-html-template-generator/references/test-and-screenshot-workflow.mdskills/ve-html-template-generator/references/ve-field-patterns.mdskills/ve-html-template-generator/scripts/extract_page_sections.pyskills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.pyskills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.pyskills/ve-html-template-generator/scripts/validate_client_template.py
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
benlife5
reviewed
Mar 3, 2026
skills/ve-html-template-generator/references/client-config-category-pattern.md
Outdated
Show resolved
Hide resolved
…editor into ai-generation-skill
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
skills/ve-html-template-generator/SKILL.md (1)
267-268:⚠️ Potential issue | 🟠 MajorUnify script paths across all command examples.
Line 267, Line 334, Line 384, and Line 398 still use
scripts/...while earlier examples useskills/ve-html-template-generator/scripts/.... Keep one convention so commands are runnable from the documented working directory.Suggested doc fix
- python3 scripts/validate_client_template.py --client-path starter/src/templates/<client> + python3 skills/ve-html-template-generator/scripts/validate_client_template.py --client-path starter/src/templates/<client>- python3 scripts/scaffold_client_template_section_parity_test.py \ + python3 skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py \- python3 scripts/scaffold_client_template_smoke_test.py \ + python3 skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py \- python3 scripts/scaffold_client_template_visual_parity_test.py \ + python3 skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py \Also applies to: 334-339, 384-388, 398-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/SKILL.md` around lines 267 - 268, Multiple example commands in SKILL.md use the short path "scripts/..." while earlier examples use the full path "skills/ve-html-template-generator/scripts/..."; update every instance of commands like "python3 scripts/validate_client_template.py --client-path starter/src/templates/<client>" (and the other occurrences that start with "scripts/...") to use the full path "skills/ve-html-template-generator/scripts/..." so all examples are runnable from the documented working directory and consistent across the file (update all remaining occurrences referenced in the review).skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py (1)
112-113:⚠️ Potential issue | 🔴 CriticalSanitize
client_slugbefore building the TypeScript import symbol.Using
config_symbol = f"{client_slug}Config"can generate invalid identifiers (e.g.,galaxy-grillConfig), which will break parsing in the generated test file.Suggested fix
import argparse import os +import re from pathlib import Path from shutil import copyfile +def slug_to_camel_identifier(value: str) -> str: + parts = [part for part in re.split(r"[^A-Za-z0-9]+", value) if part] + if not parts: + candidate = "client" + else: + first = parts[0] + candidate = first[:1].lower() + first[1:] + candidate += "".join(part[:1].upper() + part[1:] for part in parts[1:]) + if not re.match(r"^[A-Za-z_]", candidate): + candidate = f"_{candidate}" + return candidate @@ - config_symbol = f"{client_slug}Config" + config_symbol = f"{slug_to_camel_identifier(client_slug)}Config"#!/bin/bash set -euo pipefail # Verify raw slug interpolation is still used for config symbol generation rg -n 'config_symbol\s*=\s*f"\{client_slug\}Config"' \ skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py` around lines 112 - 113, The code assigns config_symbol = f"{client_slug}Config" which can produce invalid TypeScript identifiers (e.g., "galaxy-grillConfig"); sanitize client_slug first by normalizing non-identifier characters to underscores and ensuring it doesn't start with a digit (e.g., use a sanitized_slug = re.sub(r'\W+', '_', client_slug) and if sanitized_slug[0].isdigit() prefix with '_' ), then build config_symbol = f"{sanitized_slug}Config" so the generated import symbol is a valid TS identifier; update all uses of config_symbol accordingly (reference symbols: client_slug and config_symbol).skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py (1)
87-87:⚠️ Potential issue | 🔴 Critical
config_symbolgeneration can produce invalid TypeScript.Raw slug concatenation here can emit invalid import bindings for common slugs with
-, causing generated smoke tests to fail to compile.Suggested fix
- config_symbol = f"{client_slug}Config" + config_symbol = f"{slug_to_camel_identifier(client_slug)}Config"#!/bin/bash set -euo pipefail # Verify this scaffold still uses unsanitized slug for config symbol rg -n 'config_symbol\s*=\s*f"\{client_slug\}Config"' \ skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py` at line 87, The current config_symbol = f"{client_slug}Config" can produce invalid TypeScript identifiers when client_slug contains hyphens or other non-alphanumeric chars; replace this with a sanitized PascalCase identifier derived from client_slug (e.g., split on non-alphanumerics, capitalize each segment, join, and if the first character is a digit prefix with an underscore) and then append "Config" (i.e., set config_symbol to the sanitizedName + "Config"); implement the sanitizer as a small helper function and use it where config_symbol is assigned so generated import bindings are valid TS identifiers.
🧹 Nitpick comments (1)
skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py (1)
13-38: Consider centralizing scaffold helper utilities.
to_pascal_case/normalize_import_path/ identifier-sanitization behavior is now split across scripts. Moving these into a shared helper module would reduce drift and prevent repeat regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py` around lines 13 - 38, The helper functions to_pascal_case, slug_to_camel_identifier, and normalize_import_path are duplicated across scripts and should be centralized: extract these functions into a new shared module (e.g., scaffold_helpers) and replace local definitions with imports in scaffold_client_template_visual_parity_test.py and other scripts that define them; ensure you preserve exact behavior (including the regex and leading-underscore logic in slug_to_camel_identifier and the leading "./" behavior in normalize_import_path), update any references to call scaffold_helpers.to_pascal_case, scaffold_helpers.slug_to_camel_identifier, and scaffold_helpers.normalize_import_path, and run/adjust existing tests or add a small unit test to confirm parity after moving.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/ve-html-template-generator/README.md`:
- Line 187: The README mentions clearing saved layout data using the incorrect
field name document.__.layout; change this to document.__layout to match the
rest of the docs and codebase conventions (search for and replace
document.__.layout occurrences, e.g., in the guidance line that explains
clearing/avoiding old saved layout data) so the field name aligns with the
actual property used elsewhere.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py`:
- Around line 1205-1209: The current regex allows extra characters between the
sibling client name and the trailing token, causing false positives when one
client name prefixes another; update the pattern used where sibling_pascal is
checked (the re.search call that appends to sibling_symbol_leaks) to require the
suffix to follow the sibling name immediately instead of permitting
[A-Za-z0-9_]* — e.g. remove that character class and use a pattern like
rf"\b{re.escape(sibling_pascal)}(?:Section|Slot|Props|Config|Template|Category)\b"
so only exact Pascal-case sibling names with those exact suffixes are matched;
keep using source_file.name in the leak entry as before.
- Around line 691-693: The current detection strips only literal spaces via
text_no_spaces = text.replace(" ", "") then checks for the exact substring
'types:["type.hours"]', which misses tabs/newlines and other whitespace; update
the logic that sets has_hours_field_binding by either normalizing all whitespace
(e.g., use a regex to remove \s+) before matching or, better, use a regex search
that tolerates arbitrary whitespace/quotes around types and the array (e.g.,
match types\s*:\s*\[\s*["']type\.hours["']\s*\]) against the original text
variable so multiline/tabbed declarations are correctly detected.
- Around line 1239-1241: Remove the unreliable mtime-based early-exit that skips
copy-detection: delete the conditional check using target_template_mtime and
sibling_template.stat().st_mtime (the "if target_template_mtime <=
sibling_template.stat().st_mtime: continue" branch) so that the subsequent
layout-matching and SequenceMatcher similarity logic always runs for sibling
templates; keep the existing content-based similarity threshold and
layout-sequence analysis unchanged.
---
Duplicate comments:
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.py`:
- Around line 112-113: The code assigns config_symbol = f"{client_slug}Config"
which can produce invalid TypeScript identifiers (e.g., "galaxy-grillConfig");
sanitize client_slug first by normalizing non-identifier characters to
underscores and ensuring it doesn't start with a digit (e.g., use a
sanitized_slug = re.sub(r'\W+', '_', client_slug) and if
sanitized_slug[0].isdigit() prefix with '_' ), then build config_symbol =
f"{sanitized_slug}Config" so the generated import symbol is a valid TS
identifier; update all uses of config_symbol accordingly (reference symbols:
client_slug and config_symbol).
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.py`:
- Line 87: The current config_symbol = f"{client_slug}Config" can produce
invalid TypeScript identifiers when client_slug contains hyphens or other
non-alphanumeric chars; replace this with a sanitized PascalCase identifier
derived from client_slug (e.g., split on non-alphanumerics, capitalize each
segment, join, and if the first character is a digit prefix with an underscore)
and then append "Config" (i.e., set config_symbol to the sanitizedName +
"Config"); implement the sanitizer as a small helper function and use it where
config_symbol is assigned so generated import bindings are valid TS identifiers.
In `@skills/ve-html-template-generator/SKILL.md`:
- Around line 267-268: Multiple example commands in SKILL.md use the short path
"scripts/..." while earlier examples use the full path
"skills/ve-html-template-generator/scripts/..."; update every instance of
commands like "python3 scripts/validate_client_template.py --client-path
starter/src/templates/<client>" (and the other occurrences that start with
"scripts/...") to use the full path
"skills/ve-html-template-generator/scripts/..." so all examples are runnable
from the documented working directory and consistent across the file (update all
remaining occurrences referenced in the review).
---
Nitpick comments:
In
`@skills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.py`:
- Around line 13-38: The helper functions to_pascal_case,
slug_to_camel_identifier, and normalize_import_path are duplicated across
scripts and should be centralized: extract these functions into a new shared
module (e.g., scaffold_helpers) and replace local definitions with imports in
scaffold_client_template_visual_parity_test.py and other scripts that define
them; ensure you preserve exact behavior (including the regex and
leading-underscore logic in slug_to_camel_identifier and the leading "./"
behavior in normalize_import_path), update any references to call
scaffold_helpers.to_pascal_case, scaffold_helpers.slug_to_camel_identifier, and
scaffold_helpers.normalize_import_path, and run/adjust existing tests or add a
small unit test to confirm parity after moving.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (10)
skills/ve-html-template-generator/README.mdskills/ve-html-template-generator/SKILL.mdskills/ve-html-template-generator/references/client-config-category-pattern.mdskills/ve-html-template-generator/references/layout-containment.mdskills/ve-html-template-generator/references/slot-paradigm.mdskills/ve-html-template-generator/references/template-contract.mdskills/ve-html-template-generator/scripts/scaffold_client_template_section_parity_test.pyskills/ve-html-template-generator/scripts/scaffold_client_template_smoke_test.pyskills/ve-html-template-generator/scripts/scaffold_client_template_visual_parity_test.pyskills/ve-html-template-generator/scripts/validate_client_template.py
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/ve-html-template-generator/references/layout-containment.md
|
|
||
| Use this before evaluating a new generation: | ||
|
|
||
| 1. Clear or avoid old saved layout data for the target entity (`document.__.layout`). |
Contributor
There was a problem hiding this comment.
Fix likely typo in saved-layout field name.
Line 187 references document.__.layout; this likely should be document.__layout to match the rest of the guidance.
Suggested doc fix
- 1. Clear or avoid old saved layout data for the target entity (`document.__.layout`).
+ 1. Clear or avoid old saved layout data for the target entity (`document.__layout`).📝 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
| 1. Clear or avoid old saved layout data for the target entity (`document.__.layout`). | |
| 1. Clear or avoid old saved layout data for the target entity (`document.__layout`). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/ve-html-template-generator/README.md` at line 187, The README mentions
clearing saved layout data using the incorrect field name document.__.layout;
change this to document.__layout to match the rest of the docs and codebase
conventions (search for and replace document.__.layout occurrences, e.g., in the
guidance line that explains clearing/avoiding old saved layout data) so the
field name aligns with the actual property used elsewhere.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai document.__.layout is actually correct, it's the mentions of document.__layout that would need to be fixed
Contributor
There was a problem hiding this comment.
@briantstephan, thank you for the correction! You're absolutely right—I apologize for the error. I'll make a note that document.__.layout (with the double underscore as part of the property name) is the correct field name for saved layout data in this codebase.
✏️ Learnings added
Learnt from: briantstephan
Repo: yext/visual-editor PR: 1069
File: skills/ve-html-template-generator/README.md:187-187
Timestamp: 2026-03-03T17:54:57.785Z
Learning: In the visual-editor codebase, the correct field name for saved layout data on entities is `document.__.layout` (with double underscore: `__.layout`), not `document.__layout` (single underscore: `__layout`).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+691
to
+693
| text_no_spaces = text.replace(" ", "") | ||
| if 'types:["type.hours"]' in text_no_spaces: | ||
| has_hours_field_binding = True |
Contributor
There was a problem hiding this comment.
type.hours detection misses valid formatting variants.
Line 691 removes only literal spaces, so Line 692 can miss valid multiline/tabbed declarations and trigger false failures at Line 933.
Proposed fix
- text_no_spaces = text.replace(" ", "")
- if 'types:["type.hours"]' in text_no_spaces:
+ if re.search(r'types\s*:\s*\[\s*["\']type\.hours["\']\s*\]', text):
has_hours_field_binding = True📝 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
| text_no_spaces = text.replace(" ", "") | |
| if 'types:["type.hours"]' in text_no_spaces: | |
| has_hours_field_binding = True | |
| if re.search(r'types\s*:\s*\[\s*["\']type\.hours["\']\s*\]', text): | |
| has_hours_field_binding = True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py` around
lines 691 - 693, The current detection strips only literal spaces via
text_no_spaces = text.replace(" ", "") then checks for the exact substring
'types:["type.hours"]', which misses tabs/newlines and other whitespace; update
the logic that sets has_hours_field_binding by either normalizing all whitespace
(e.g., use a regex to remove \s+) before matching or, better, use a regex search
that tolerates arbitrary whitespace/quotes around types and the array (e.g.,
match types\s*:\s*\[\s*["']type\.hours["']\s*\]) against the original text
variable so multiline/tabbed declarations are correctly detected.
Comment on lines
+1205
to
+1209
| if re.search( | ||
| rf"\b{re.escape(sibling_pascal)}[A-Za-z0-9_]*(?:Section|Slot|Props|Config|Template|Category)\b", | ||
| text, | ||
| ): | ||
| sibling_symbol_leaks.append(f"{source_file.name}->{sibling_pascal}") |
Contributor
There was a problem hiding this comment.
Sibling symbol leak regex can false-positive on shared client prefixes.
Line 1206 allows arbitrary characters after the sibling prefix. If one client name is a prefix of another (e.g., Galaxy vs GalaxyGrill), valid local symbols can be flagged as leaks.
Proposed fix
- if re.search(
- rf"\b{re.escape(sibling_pascal)}[A-Za-z0-9_]*(?:Section|Slot|Props|Config|Template|Category)\b",
- text,
- ):
- sibling_symbol_leaks.append(f"{source_file.name}->{sibling_pascal}")
+ symbol_matches = re.findall(
+ rf"\b({re.escape(sibling_pascal)}[A-Za-z0-9_]*(?:Section|Slot|Props|Config|Template|Category))\b",
+ text,
+ )
+ for symbol in symbol_matches:
+ if symbol.startswith(client_pascal):
+ continue
+ sibling_symbol_leaks.append(f"{source_file.name}->{sibling_pascal}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py` around
lines 1205 - 1209, The current regex allows extra characters between the sibling
client name and the trailing token, causing false positives when one client name
prefixes another; update the pattern used where sibling_pascal is checked (the
re.search call that appends to sibling_symbol_leaks) to require the suffix to
follow the sibling name immediately instead of permitting [A-Za-z0-9_]* — e.g.
remove that character class and use a pattern like
rf"\b{re.escape(sibling_pascal)}(?:Section|Slot|Props|Config|Template|Category)\b"
so only exact Pascal-case sibling names with those exact suffixes are matched;
keep using source_file.name in the leak entry as before.
Comment on lines
+1239
to
+1241
| # Avoid blaming the older template when a newer template appears copied from it. | ||
| if target_template_mtime <= sibling_template.stat().st_mtime: | ||
| continue |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read the context around lines 1239-1241
if [ -f "skills/ve-html-template-generator/scripts/validate_client_template.py" ]; then
echo "=== File found ==="
wc -l "skills/ve-html-template-generator/scripts/validate_client_template.py"
echo ""
echo "=== Lines 1230-1250 context ==="
sed -n '1230,1250p' "skills/ve-html-template-generator/scripts/validate_client_template.py"
else
echo "File not found at expected path"
# Try to find it
find . -name "validate_client_template.py" 2>/dev/null
fiRepository: yext/visual-editor
Length of output: 1049
🏁 Script executed:
#!/bin/bash
# Run the diagnostic script from the review to check for shared mtimes
python - <<'PY'
from pathlib import Path
templates = sorted(Path("starter/src/templates").glob("*/*-template.tsx"))
print(f"templates_found={len(templates)}")
if not templates:
print("No templates found in starter/src/templates")
import os
if os.path.exists("starter/src/templates"):
print("Directory exists, listing contents:")
for root, dirs, files in os.walk("starter/src/templates"):
level = root.replace("starter/src/templates", "").count(os.sep)
indent = " " * 2 * level
print(f"{indent}{os.path.basename(root)}/")
sub_indent = " " * 2 * (level + 1)
for file in files[:5]:
print(f"{sub_indent}{file}")
raise SystemExit(0)
groups = {}
for p in templates:
groups.setdefault(p.stat().st_mtime, []).append(str(p))
same_mtime_groups = [g for g in groups.values() if len(g) > 1]
print(f"shared_exact_mtime_groups={len(same_mtime_groups)}")
print(f"total_groups_with_unique_mtime={len([g for g in groups.values() if len(g) == 1])}")
for idx, group in enumerate(same_mtime_groups[:10], 1):
print(f"\n[group {idx}] size={len(group)}")
for entry in group[:10]:
print(f" {entry}")
PYRepository: yext/visual-editor
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for the copy-detection logic and related functions
echo "=== Searching for copy detection code ==="
rg -n "copy" "skills/ve-html-template-generator/scripts/validate_client_template.py" -i | head -20
echo ""
echo "=== Searching for similarity or duplicate related code ==="
rg -n "similarity|duplicate|sibling" "skills/ve-html-template-generator/scripts/validate_client_template.py" -i | head -20Repository: yext/visual-editor
Length of output: 2019
🏁 Script executed:
#!/bin/bash
# Read broader context around the mtime check
echo "=== Lines 1220-1280 (full loop context) ==="
sed -n '1220,1280p' "skills/ve-html-template-generator/scripts/validate_client_template.py"Repository: yext/visual-editor
Length of output: 2717
🏁 Script executed:
#!/bin/bash
# Search for all references to the loop and error handling
echo "=== Context after similarity check ==="
sed -n '1240,1310p' "skills/ve-html-template-generator/scripts/validate_client_template.py"Repository: yext/visual-editor
Length of output: 3177
🏁 Script executed:
#!/bin/bash
# Find where target_template_mtime is set and how it's obtained
echo "=== Search for target_template_mtime ==="
rg -n "target_template_mtime" "skills/ve-html-template-generator/scripts/validate_client_template.py" -B2 -A2Repository: yext/visual-editor
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Check if the similarity check is the only mechanism to detect copies
echo "=== Check if there are other copy-detection paths ==="
sed -n '1200,1350p' "skills/ve-html-template-generator/scripts/validate_client_template.py"Repository: yext/visual-editor
Length of output: 6708
Remove unreliable mtime-based gate that silently skips copy-detection analysis.
The mtime check at lines 1239-1241 gates the only copy-detection mechanism. In git checkouts (fresh clone, post-merge, after git checkout .), files frequently share identical mtimes, causing target_template_mtime <= sibling_template.stat().st_mtime to evaluate true and skip the entire similarity and layout-sequence analysis via continue. The content-based detection downstream (layout matching + SequenceMatcher similarity at 0.96+ threshold) is sound; the unreliable filesystem timestamp gate should not bypass it.
Proposed fix
- # Avoid blaming the older template when a newer template appears copied from it.
- if target_template_mtime <= sibling_template.stat().st_mtime:
- continue📝 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
| # Avoid blaming the older template when a newer template appears copied from it. | |
| if target_template_mtime <= sibling_template.stat().st_mtime: | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/ve-html-template-generator/scripts/validate_client_template.py` around
lines 1239 - 1241, Remove the unreliable mtime-based early-exit that skips
copy-detection: delete the conditional check using target_template_mtime and
sibling_template.stat().st_mtime (the "if target_template_mtime <=
sibling_template.stat().st_mtime: continue" branch) so that the subsequent
layout-matching and SequenceMatcher similarity logic always runs for sibling
templates; keep the existing content-based similarity threshold and
layout-sequence analysis unchanged.
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
2 participants
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.
New Codex skill for generating sets of custom components for clients.