feat(skills-editor): ew skills editor with MCP governance and accessibility hardening#399
Open
anfibiacreativa wants to merge 40 commits intoewfrom
Open
feat(skills-editor): ew skills editor with MCP governance and accessibility hardening#399anfibiacreativa wants to merge 40 commits intoewfrom
anfibiacreativa wants to merge 40 commits intoewfrom
Conversation
ARIA tablist with keyboard nav, S2 tokens, tab-change event. Made-with: Cursor
Heading, subheading, badge and actions slots, S2 tokens. Made-with: Cursor
…ation Port skills lab from exp-workspace to nx2 with S2 tokens, semantic ARIA markup, and non-blocking data loading. Wire into canvas after-panel. Made-with: Cursor
- rename "handoff" → "suggestion" throughout (state, CSS, events, sessionStorage key) - fix SVG node sharing: store outerHTML string, render via unsafeHTML so each card gets its own node - fix timer leak: store _statusTimer, clear on disconnect and on re-call - add error handling to _onDeleteSkill and _onDeletePrompt - remove dead dynamic import of registerMcpServer (already statically imported) - declare _loadedKey and _statusTimer in constructor (no more implicit properties) - extract STATUS constants, replace all 'approved'/'draft' magic strings - fix textarea flex-fill: remove calc(100vh) hack, propagate flex through .textarea-wrap - remove redundant margin-top on .tool-row (parent gap already provides spacing) - use S2 checkmark SVG via loadHrefSvg for approved status icon (CSS circles) Made-with: Cursor
… sync - Full MCP editor with built-in servers, enable/disable toggles, edit mode - Generated tools panel with web worker sandbox execution and Try It panel - Dual-write skills to both .md file and config sheet on save/delete - Sync mechanism merges .md body with config sheet metadata on load - Replace loadHrefSvg/unsafeHTML with <img> tags for checkmark icons
Convert Skills Lab from a three-panel to a master-detail layout — slide-in flex drawer, per-tab dirty-form tracking, browser history + sessionStorage persistence, tools selector checkbox fix, and all lint errors resolved.
…tab removed Prompts - Show row action buttons always (remove opacity:0 hover reveal) - Add Delete button to each row; remove Associated Tools from editor form MCPs - Clicking a card row now opens the editor (was a no-op for custom MCPs) - Editor title shows 'Edit: <key>' vs 'Register MCP Server' — inline 'Editing XYZ' hint removed; _clearForm() now also resets MCP state so the label can't linger when switching tabs or clicking Register - Add Description field (textarea-sm) saved to config sheet Skills editor regression - _clearForm() was not clearing _editingMcpKey/_mcpKey/_mcpUrl; after visiting MCPs, switching to Skills and clicking a skill silently failed to open the editor — now fixed Generated Tools tab - Remove 'generated' from CATALOG_TABS; tab is no longer reachable Memory - Editor body gets .editor-body-memory class → justify-content:flex-end pushes memory cont the bottom of the viewport
…d code
API layer:
- deleteSkillMdFile now returns { ok, status } instead of void; callers
can detect failure (404 treated as success)
- loadAgentPresets spreads preset fields flat ({ id, ...preset }) so
_renderAgentCard reads label/description/tools without a .preset hop
- loadAgentPresets and fetchMcpToolsFromAgent fire-and-forget chains
now have .catch() to prevent unhandled rejection warnings
Naming conventions (boolean Lit reactive properties):
- _loading → _isLoading
- _suggestion → _hasSuggestion
- _editorOpen → _isEditorOpen
- _formDirty → _isFormDirty
- _saveBusy → _isSaveBusy
- _agentViewTools → _isAgentViewTools
- _formIsEdit → _isFormEdit
- _formPromptIsEdit → _isFormPromptEdit
CSS state modifier classprefix:
- drawer-open → is-drawer-open
- suggestion-wrap → is-suggestion
- dot-active → is-dot-active
- dot-inactive → is-dot-inactive
Correctness:
- Skill ID input now calls _markDirty() on change
- _loadMemory uses null for fetch error vs '' for empty file; renderer
handles the distinction correctly
- inert attribute added to .col-editor when drawer is closed
- _onToggleMcpEnabled redundant APPROVED double-check removed
- Dead tab === 'generated' branch removed from _renderListCol()
Polish:
- .card-menu-delete removed; uses .card-menu specificity
- Single-letter vars renamed: q→searchQuery/toolFilter, s→server,
t→tool, f→status
- 12px font sizes replaced with var(--s2-body-size-xs) token
- Badge padding 2px 8px replaced with spacing tokens
…I contract
Data integrity:
- Sequence skill writes (file first, then config) so a failing config
write never leaves an .md file without a config entry and vice versa
- Sequence skill deletes (file first, then config) for the same reason
- Prompt CRUD callers updated to use the new ok-shape consistently
API contract (fetchDaConfigSheets / all mutators):
- fetchDaConfigSheets now returns ok:false on 401 instead of EMPTY_CONFIG
(ok:true); callers can no longer mistake an auth failure for empty data
- All exported mutators (upsertSkillInConfig, deleteSkillFromConfig,
upsertPromptInConfig, deletePromptFromConfig, upsertGeneratedTool,
deleteGeneratedTool) now return { ok, error?, status? } consistently
- AUTH_FAIL sentinel extracted to avoid duplicating the 401 return shape
- boot.finally() now has .catch() to prevent unhandled rejection risk
- fetchDaConfigSheets bare catch now surfaces err.message in the return
- Single-letter vars renamed: o→payload, k/u→serverKey/serverUrl, def→toolDef
Stale async guards:
- _onEditSkill: capture skillId and tab at request time; discard response
if user navigated away before the readSkillMdFile fetch resolved
- _reload fire-and-forgets: loadAgentPresets and fetchMcpToolsFromAgent
now check the load key before writing state, preventing late responses
from a previous org/site from overwriting the current view
Form dirty tracking:
- Tool checkbox changes in _renderAssociatedToolsSelector now call
_markDirty() so tool selection edits are tracked and persisted
Focus management (keyboard accessibility):
- Opening the drawer moves focus to the first interactive element inside
- Closing the drawer restores focus to the element that triggered the open
- _editorTrigger stored on each open entry point (openEditor, onEditSkill,
openNewMcpEdetc.) and cleared after focus restoration
Filter state:
- _catalogFilter reset to 'all' on every tab change and back/forward
navigation; it was previously leaking across unrelated tabs
Misc:
- _renderAgentCard parameter renamed a→agent; tool list item t→tool
- _renderAgentsCatalog map var a→agent
…state, CSS deduplication
Critical fixes:
- Add compensating rollback to _onSaveSkill (delete orphan file if config save fails)
and _onDeleteSkill/_onDeleteSkillById (re-create file if config delete fails)
- Fix writeSkillMdFile to return { ok: false, error } on validation failure
- Replace raw _editorTrigger element reference with stable CSS selector (_editorTriggerSelector)
Simplification:
- Extract upsertSheetRow/deleteSheetRow generic helpers in skills-editor-api.js; all
config CRUD mutators now delegate to them, eliminating copy-pasted load/mutate/save flows
- Add FRESH_FORM_STATE constant; _clearForm, _captureForm, _restoreForm are now
tab-agnostic — no more per-mode branching when adding w field
- Add TAB_ACTIONS config map; list header renders one data-driven new-button instead of
four conditional blocks; _editorTitle() extracts drawer title logic from the template
- Extract .btn-icon CSS primitive; .close-btn, .more-btn, .row-action-btn now declare
only size/color overrides
Chat/event compatibility:
- Support both legacy (da-skills-lab-*) and new (da-skills-editor-*) event/sessionStorage
namespaces so suggestion handoff and prompt-to-chat work during the migration window
- chat.js listens for both prompt-add-to-chat and prompt-send families with dedup guard
Prompt edit parity:
- Track _formPromptOriginalTitle so prompt rename upserts the correct row
- Track _formPromptIcon so the icon field is preserved on save
- Restore Add to Chat button in prompt rows and editor footer
Agent parity:
- Store and render aut ignored in UI)
- loadAgentPresets() handles both label/tools and name/mcpServers preset shapes
- New Agent path now creates /.da/agents/{id}.json via saveAgentPresetFile()
- Dedicated _renderAgentForm() / _onSaveAgent() with id, name, and starter preset
Generated Tools tab remains removed (unreachable by design); data still loads.
Add MCP tool listing and per-tool enable/disable controls with config persistence, pass MCP auth headers through chat/agent requests, and stabilize the tree throttle test timing assertion.
…cessibility Normalize sheet boolean parsing for tool overrides and MCP enablement, make MCP cards keyboard-activatable, and stabilize unit tests against current skills-editor behavior.
Guard MCP card click/keydown activation so nested menu buttons do not trigger card open, and add regression tests for keyboard and click bubbling behavior.
Drop nx2/img/adobe.svg since it is not required for the skills-editor workstream.
…nd refresh Render skills-editor immediately from per-site session snapshot, refresh data in the background with an auto-refresh progress indicator, lazy-load agents/MCP tools by tab, and keep orphan-skill sync off the critical path.
…keyboard a11y unit tests
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
Commits
|
The chat controller was passing the raw agent text straight into the message list without detecting [SKILL_SUGGESTION] blocks. renderers.js then parsed the block as markdown, producing garbled output (SKILL_ID: lines, <hr> from the --- markers, etc.). extractSkillSuggestion() now detects the block, extracts id/body/prose, writes the payload to sessionStorage, and dispatches the da-skills-editor-suggestion-handoff event so the skills editor opens the suggestion form. The raw block is stripped from both the live streaming preview (onDelta) and the final persisted message (onText). 13 unit tests added under nx2/test/unit/chat/chat-controller.test.js.
…ix toggle rollback
Member
Author
And I totally agree with you. I am not sure we want to split the code too much which may make it harder to clean up when we migrate to the OneAgent? |
Member
Author
Ok, I did this, I moved the renderers out of the skills-editor cutting the code by half and reducing responsibility. But to do that I also had to update the ESlint config no-underscore-dangle that was already disabled for test files (which access el._xxx) but not for source files. Moving render methods to a standalone renderers.js exposed the same pattern: host._xxx on a parameter rather than this. Extended the disable to all source files since the project uses _-prefixed private members by convention throughout — the rule was just inconsistently scoped. I propose the following roadmap to further decouple with less risks and let this PR land
|
Reactive state fixes: - _newAgentId, _newAgentName, _formPromptIcon, _formPromptOriginalTitle were mutated as state but not declared in static properties — LitElement never re-rendered the agent/prompt forms on user input; now registered as reactive Event listener fix: - _boundOnSuggestion/ClearForm/Popstate were re-created in connectedCallback on every re-attach, leaking the previous listeners; moved to class field arrow functions so removeEventListener provably removes the exact function added Constants: - Added TAB_SKILLS/AGENTS/PROMPTS/MCPS/MEMORY to constants.js; all 15 magic tab-stng literals in renderers.js replaced; CATALOG_TABS and TAB_ACTIONS now use these constants internally renderers.js: - renderEditorFooter now accepts (host, tab) and derives its own booleans; removed the 4-boolean parameter list from the call site - Dynamic opener dispatch guarded with typeof check to prevent silent no-op on typo in constants.js Frontmatter: - Injected frontmatter now uses `name:` (Anthropic SKILL.md requirement) with the skill ID as the value (already lowercase + hyphens, matching the format) rather than a title-cased prettified string - Extracted parseFrontmatter / validateSkillFrontmatter / ensureSkillFrontmatter to nx2/utils/skill-frontmatter.js; _onSaveSkill delegates to ensureSkillFrontmatter - 33 unit tests covering all branches and boundary conditions of the util - Updated manual testing doc and OpenAPI example to use `name:` frontmatter
- extractTitle and extractToolRefs moved from renderers.js / skills-editor-api.js to nx2/utils/markdown.js — pure markdown helpers with no block dependency - parseSheetBoolean and normaliseRowKey moved from skills-editor-api.js to nx2/utils/sheet-utils.js — generic config sheet coercion/normalisation - skillKeyMatch and mcpKeyMatch now delegate to normaliseRowKey, removing duplicated row-key normalisation logic - skills-editor-api.js re-exports extractToolRefs for backward compatibility - nx-skills-editor.test.js import updated to point at the util - 31 new unit tests across markdown.test.js andet-utils.test.js
…ad rules - nx-skills-editor.css: structural only (host, layout, gate, chat drawer, responsive) - catalog.css: list, cards, badges, filter chips, card menu - editor-panel.css: editor panel, forms, editor-actions, status, animations - tools.css: tools selector, MCP tools/auth/server, memory All 4 sheets loaded in parallel via Promise.all before connectedCallback. Dead .save-row selector removed (never used in templates). .section-h and .tools-selector-heading consolidated. Single-property multi-line rules collapsed to one line. Net: 1194 → 1126 lines across 4 files.
Thank you for the restructuring! However regarding underscore linting rule, I wouldn't recommend disabling it globally. 2 options:
|
Member
Author
You are absolutely right, and it was my incorrect assumption that Claude was following a pattern across the repo in the refactoring, but it was not. I will reestablish the rule and refactor the renderer to match the existing pattern. |
… no-underscore-dangle
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.
Summary
nx-skills-editorexperience with a master-detail layout for Skills, Prompts, Agents, MCPs, and Memory.Why
What Changed
nx2/blocks/shared/card/*reusable card component.nx2/blocks/shared/tabs/*reusable tabs component.nx2/blocks/skills-editor/*new Skills Editor block + API module.mcpServersandmcpServerHeadersin agent requests.da-skills-lab-*andda-skills-editor-*namespaces).test/utils/tree.test.js.nx2/test/unit/skills-editor/nx-skills-editor.test.js.This PR introduces global utils and shared components, as described below
Test Plan
npm run test:unit -- nx2/test/unit/skills-editor/nx-skills-editor.test.jsda-nxunit suite to verify shared card/tabs integration.tests/playwright-e2ebranch (keyboard/a11y and regression paths).e2e test live here -> https://github.com/adobe/da-live/compare/tests/playwright-e2e?expand=1
Manual:
Risks / Follow-ups
nx-skills-editordoes not mount; add clearer readiness skip messaging if needed. These tests are in da-live. Non-blocking to merge PRMust be integrated together with:
adobe-rnd/da-agent#23
Stage URLs:
https://ew-skills--da-nx--adobe.aem.page/
https://da.live/apps/skills?nx=ew-skills&nxver=2#/exp-workspace/frescopa
https://da.live/canvas?nx=ew-skills&nxver=2#/exp-workspace/frescopa/index