Uh oh!
There was an error while loading. Please reload this page.
refactor(skills): one definition of the skill fields across every surface - #5916
Conversation
…face The Name / Description / Content trio was implemented three times — once in the canvas modal and once each in the create and detail pages — and had already drifted: the name hint appeared on create, was missing entirely on detail, and sat in a different slot in the modal; the content editor was 260px on the pages and 200px in the modal; only the modal marked the fields required. Extract SkillFields, the DetailSection form both full-page surfaces render, and move the shared copy into skill-copy.ts. The modal keeps ChipModalField — that is required inside a ChipModalBody, so it cannot share the pages' JSX — but now reads the same placeholder, hint, and max-length constants, so the wording can no longer diverge. The detail page's local FieldLockTooltip moves into SkillFields and is now available to both pages, and the dynamic rich-editor import drops from three copies to two. No behavioral change beyond the drift being resolved: the name hint now shows on the detail page as it already did on create.
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
In Reviewed by Cursor Bugbot for commit 210c288. Configure here. |
Greptile SummaryRefactors skill editing fields into shared definitions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain. Important Files Changed
Reviews (2): Last reviewed commit: "refactor(skills): derive modal saving st..." | Re-trigger Greptile |
…ge line
From the cleanup passes over this branch:
- SkillModal mirrored `mutation.isPending` into a local `saving` useState, which
.claude/rules/sim-hooks.md forbids. It also carried a latent bug: the
`finally { setSaving(false) }` ran after `onSave()` had closed the modal, so it
set state on an unmounting component, and a throw from `onSave()` would leave
the modal stuck in a saving state. Derived from the two mutations instead.
- The hint/error line under a field was written three times in SkillFields, in
three slightly different shapes. One FieldMessage helper now renders all three.
- The name hint is an authoring instruction, so it no longer shows under a field
that cannot be edited (a built-in skill, or a viewer who is not an editor).waleedlatif1
commented
Jul 24, 2026
waleedlatif1
commented
Jul 24, 2026
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 210c288. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The skill Name / Description / Content trio was implemented three times — in the canvas
SkillModal, and again in each of the/skills/newand/skills/[skillId]pages. They had already drifted:hintslotrequiredmarkersThis extracts
SkillFields— theDetailSectionform both full-page surfaces render — and moves the shared strings intoskill-copy.ts.The modal keeps
ChipModalField, because.claude/rules/sim-styling.mdrequires every labeled field inside aChipModalBodyto be one, so it genuinely cannot share the pages' JSX. It now reads the same placeholder / hint / max-length constants instead, so the wording can no longer diverge.Also folded in:
FieldLockTooltipmoves intoSkillFields, so both pages get the "why is this locked" tooltip−219 / +72.
Behavior
No intended change other than resolving the drift: the name hint now shows on the detail page, matching create. Everything else renders as before — the canvas modal is untouched apart from sourcing its copy from constants.
Type of Change
Testing
tsc,biome, and the six repo gates (react-query,client-boundary,api-validation,utils,boundaries,migrations) all pass; 1,195 tests inapp/workspacepass.Not visually verified — worth a glance at the three surfaces to confirm the fields render identically.
Checklist