Conversation
…igator - Add skill creation dialog for collections and directories - Add delete confirmation dialogs for skills, folders, and collections - Add inline rename with auto-formatting (kebab-case for skills) - Add helper modules for add/delete/rename operations - Apply optimistic local updates to avoid blink on save - Snapshot dialog data so labels stay stable during close animations
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return <SkillsNavigatorPage onSelectSkill={(collectionId, manifestPath) => { | ||
| setSelectedCollection(collectionId); | ||
| setSelectedDoc(manifestPath); | ||
| }} />; |
There was a problem hiding this comment.
🔴 Context-only collections become inaccessible
When a collection has no valid skill manifest, the navigator provides no route into its editor. onSelectSkill runs only for skill rows, leaving existing documents inaccessible in the management UI.
Learn more
The management page previously let users open every enabled collection. The replacement page enters CollectionEditor only after onSelectSkill supplies a manifest path. Collections containing ordinary context documents but no valid SKILL.md therefore have no selectable row that enters the editor. This affects read-only collections permanently because their users cannot add a placeholder skill.
Example: A public collection contains handbook.md and no skill manifest. It appears as an empty collection in the navigator, but clicking it only toggles expansion. The user cannot open handbook.md, although the previous collection row opened the editor.
Recommended fix: Add a collection-selection callback to SkillsNavigatorPage and SkillsNavigatorTree, and invoke it when a collection row is activated. Open CollectionEditor with a null selected path so CollectionOverview and the complete document tree remain reachable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let directory = dirName(path); | ||
| let directoryOccupied = !!(directory && this.storage.documents.get(directory)); | ||
| if (directory && !directoryOccupied) { | ||
| for (let record of this.storage.documents.list({ prefix: directory + "/" })) { | ||
| directoryOccupied = record.path.length > 0; | ||
| break; | ||
| } | ||
| } | ||
| if (directoryOccupied) { | ||
| throw new Error(`Skill directory already exists: ${directory}`); | ||
| } |
There was a problem hiding this comment.
🟡 Skill creation permits invalid nesting
When path lies below another skill or a missing folder, createContextSkill accepts it. It checks only the new directory, creating unsupported hierarchies that bundle mutations can affect unexpectedly.
Learn more
Skill creation receives a full manifest path from the RPC caller. The code verifies that the prospective skill directory is empty, but it never verifies that its parent exists or that no ancestor contains SKILL.md. A stale UI request can recreate a deleted legacy path, and any caller can place one skill beneath another. Later delete, move, and rename operations treat a non-root skill's whole directory as its bundle, so the parent operation also captures the nested skill.
Example: With alpha/SKILL.md already stored, creating alpha/beta/SKILL.md passes because nothing exists under alpha/beta/. Deleting alpha/SKILL.md then deletes both manifests, although the navigator presents them as separate skills.
Recommended fix: Before writing, derive the requested skill's parent. Require it to be the collection root or an existing inferred directory, and reject it when that parent or any ancestor contains SKILL.md. Keep these checks and the create-only write in one storage transaction.
Was this helpful? React with 👍 or 👎 to provide feedback.
| canMoveTo: (item, parent) => Boolean( | ||
| parent | ||
| && moveSourcesById.has(item.id) | ||
| && collectionIdsByItemId.get(item.id) === collectionIdsByItemId.get(parent.id), | ||
| ), |
There was a problem hiding this comment.
🟡 Same-folder moves report false success
Sibling drops and Alt+Arrow moves pass canMoveTo, although moveSkillNavigatorNode ignores unchanged parent directories. The list announces success, but the skill remains in its sorted position.
Learn more
The hierarchical list supports positional reordering, while the skills navigator persists only a destination directory. canMoveTo currently permits every destination parent in the same collection. For a sibling insertion, moveSkillNavigatorNode detects the unchanged directory and returns a fulfilled promise. The shared list interprets that fulfillment as a successful reorder and announces a new position, but rebuilding the navigator restores its name-sorted order.
Example: Audit and Deploy are siblings. Moving Deploy above Audit with Alt+ArrowUp calls the no-op path, announces “Deploy moved to position 1,” and leaves Deploy second.
Recommended fix: Make canMoveTo reject destinations whose mapped directory equals the source skill's current parent directory. Expose only directory-changing moves, since sibling ordering is neither stored nor rendered.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (const document of documents.get(collectionId) ?? []) { | ||
| let directory = dirName(document.path); | ||
| while (directory) { | ||
| existing.add(directory); | ||
| directory = dirName(directory); | ||
| } |
There was a problem hiding this comment.
🟡 Extensionless files defeat skill suffixing
When an extensionless document matches the requested directory, uniqueSkillDirectory returns the occupied name. It records only ancestor directories, so creation fails instead of choosing a suffix.
Learn more
The server treats an exact document at the proposed directory path as occupying that directory. The client collision set only includes directories inferred from each document's parent path. An extensionless root document therefore remains absent from the set even though the server rejects a skill directory with that name.
Example: A collection contains the document release. Adding a skill named “Release” selects release/SKILL.md, then fails with “Skill directory already exists: release” instead of creating release-2/SKILL.md.
Recommended fix: Add each document's exact path to existing before adding its ancestor directories. Exact paths with extensions are harmless, while extensionless paths then receive the same collision suffixing as inferred directories.
| for (const document of documents.get(collectionId) ?? []) { | |
| let directory = dirName(document.path); | |
| while (directory) { | |
| existing.add(directory); | |
| directory = dirName(directory); | |
| } | |
| for (const document of documents.get(collectionId) ?? []) { | |
| existing.add(document.path); | |
| let directory = dirName(document.path); | |
| while (directory) { | |
| existing.add(directory); | |
| directory = dirName(directory); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| await context.createContextCollection( | ||
| trimmedTitle, | ||
| description.trim(), | ||
| "private", | ||
| icon, | ||
| ); |
There was a problem hiding this comment.
🔍 Collection creation narrows available types
The new dialog always creates private, web-backed collections. Admin-public and Git-backed creation disappeared with the old flow, so verify this product reduction is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let document = parseDocument(frontmatter); | ||
| if (document.errors.length > 0) throw new Error("Skill frontmatter is not valid YAML."); | ||
| document.set("name", newName); | ||
| return joinFrontmatter(document.toString().trimEnd(), content); |
There was a problem hiding this comment.
🔍 Rename rewrites frontmatter formatting
Renaming serializes the entire YAML document and standardizes surrounding whitespace. Comments, quoting, BOM, CRLF, and body-leading blank lines can change despite preservation claims.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const loadedDocuments = documentResults.map(([id, documents]) => [ | ||
| id, | ||
| documents ?? [], | ||
| ] as const); | ||
| setData({ | ||
| collections, | ||
| documents: new Map(loadedDocuments), | ||
| writableCollectionIds: new Set(writableIds.filter((id): id is string => | ||
| id !== null && !failedCollectionIds.has(id))), | ||
| status: "ready", |
There was a problem hiding this comment.
🔍 Partial load failures appear empty
A failed document request becomes an empty collection under ready status. Users see zero skills without an error or retry path.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@FBalint Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
| ? [id] | ||
| : [])), | ||
| viewerInfo, | ||
| status: failedCollectionIds.size > 0 ? "error" : "ready", |
There was a problem hiding this comment.
🟡 Partial failure hides every collection
When one document request fails, status becomes error and hides every successfully loaded collection. Users cannot access unaffected skills until all collections load.
Learn more
Each collection's documents are loaded independently. Failed requests already become empty document lists, and their IDs are excluded from writableCollectionIds, so those collections fail closed without affecting successful results. Setting the global status to error makes SkillsNavigatorPage discard all of those partial results.
Example: Collections A and B load successfully, while collection C times out. The hook builds documents and permissions for A and B, but the page shows only “Skills could not be loaded” instead of their skills.
Recommended fix: Reserve the global error status for failure to load the collection list. Represent per-collection document failures separately, keep successful collections visible, and leave failed collections outside writableCollectionIds.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (let record of this.storage.documents.list({ prefix: sourcePrefix })) { | ||
| moves.push({ | ||
| record, | ||
| newPath: destinationPrefix + record.path.slice(sourcePrefix.length), | ||
| }); |
There was a problem hiding this comment.
🟡 Moved files bypass path limits
A deep skill move can generate support-file paths beyond 1,024 characters. Later document mutations reject those paths, leaving the moved files uneditable.
Learn more
Document mutations enforce a 1,024-character path limit through validateDocumentPath. A skill move generates every destination path by combining the new directory with each suffix, but only the shorter destination directory is validated before storage. The transaction can therefore persist paths that the ordinary document API refuses to mutate.
Example: A valid support file has a 1,020-character path under review/. Moving review/ beneath archive/ produces a 1,028-character path. The move succeeds, but saving or individually moving that support file fails with “Document path is too long.”
Recommended fix: Build the complete move set first and call validateDocumentPath for every newPath before collision checks or any transaction. Apply the same validation to root-manifest moves.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let updatedBody = updateSkillManifestName(body, newName); | ||
| // Validate the rewritten manifest before mutating storage. | ||
| parseSkillManifest(manifestPath, updatedBody); | ||
|
|
||
| let sourceDirectory = dirName(manifestPath); | ||
| let parentDirectory = dirName(sourceDirectory); | ||
| let destinationDirectory = joinPath(parentDirectory, newName); | ||
| validateDocumentPath(destinationDirectory); | ||
| await this.#moveContextSkill(manifestPath, destinationDirectory, updatedBody); |
There was a problem hiding this comment.
🟡 Renames bypass document size limit
Renaming a near-limit manifest to a longer name can exceed MAX_DOCUMENT_BODY_BYTES. The oversized manifest remains stored although normal writes reject it.
Learn more
Normal document writes encode the record and reject its complete serialized size above MAX_DOCUMENT_BODY_BYTES. Skill rename creates a larger encoded body and passes it directly into the move transaction without that check.
Example: A valid manifest sits within 20 bytes of the limit and has name: a. Renaming it to a 64-character name adds more than 20 bytes, yet the rename succeeds; saving the unchanged document later fails the normal size check.
Recommended fix: Before moving, construct the transformed manifest record and apply the same encoded-record size calculation used by #writeContextDocument. Reject the rename before entering the storage transaction when it exceeds the limit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return <SkillsNavigatorPage onSelectSkill={(collectionId, manifestPath) => { | ||
| setSelectedCollection(collectionId); | ||
| setSelectedDoc(manifestPath); | ||
| }} />; |
There was a problem hiding this comment.
🔍 Context discovery is no longer available
The new landing page projects only skill manifests. Confirm that context-only collections and ordinary documents no longer need first-class discovery.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Existing organizational directories remain fully usable for backwards compatibility. Skills can be | ||
| created in them and moved into, out of, or between them. Restricting movement to only one direction | ||
| would make existing layouts confusing to manage and could prevent users from reorganizing their | ||
| skills before eventually flattening a collection. |
There was a problem hiding this comment.
🔍 Legacy compatibility claim exceeds behavior
Legacy directories can receive skills or be deleted, but cannot be renamed or moved here. Reconcile this with the “fully usable” compatibility claim.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async createContextSkill( | ||
| path: string, | ||
| doc: { description: string; body: string; contentType?: string }): Promise<void> { | ||
| if (!isSkillManifestPath(path)) throw new Error("Skill manifest filename must be SKILL.md."); | ||
| parseSkillManifest(path, doc.body); | ||
| let directory = dirName(path); | ||
| let directoryOccupied = !!(directory && this.storage.documents.get(directory)); | ||
| if (directory && !directoryOccupied) { | ||
| for (let record of this.storage.documents.list({ prefix: directory + "/" })) { | ||
| directoryOccupied = record.path.length > 0; | ||
| break; | ||
| } | ||
| } | ||
| if (directoryOccupied) { | ||
| throw new Error(`Skill directory already exists: ${directory}`); | ||
| } | ||
| let parentDirectory = dirName(directory); | ||
| if (parentDirectory) { | ||
| let parentExists = false; | ||
| for (let record of this.storage.documents.list({ prefix: parentDirectory + "/" })) { | ||
| parentExists = record.path.length > 0; | ||
| break; | ||
| } | ||
| if (!parentExists) throw new Error(`Directory not found: ${parentDirectory}`); | ||
| let ancestor = parentDirectory; | ||
| while (ancestor) { | ||
| if (this.storage.documents.get(joinPath(ancestor, "SKILL.md"))) { | ||
| throw new Error("Cannot create a skill inside another skill."); | ||
| } | ||
| ancestor = dirName(ancestor); | ||
| } | ||
| } | ||
| await this.#writeContextDocument(path, doc, true); |
There was a problem hiding this comment.
🔍 Atomic mutations lack storage-level tests
Current tests stop at frontend delegation. Add worker coverage for collisions, subtree preservation, document counts, and skill-index updates across each new mutation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onCreated(); | ||
| if (collection.content.source === "git") { | ||
| setGitSetup({ collection, token: null }); | ||
| await createToken(collection); |
There was a problem hiding this comment.
🔍 Git creation refreshes mid-workflow
onCreated reloads the navigator before token creation and setup completion. Confirm that background refresh during this modal workflow is intentional.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
This stacked PR adopts the shared hierarchical list from #484 in the Context Library and adds the Skills Navigator management workflows.
Dependency
mainafter that PR mergesTesting
pnpm --filter @gadgets/ui test:run(58 tests)pnpm --filter @gadgets/gatekeeper-context test:run(75 tests)pnpm --filter @gadgets/ui buildpnpm --filter @gadgets/gatekeeper-context buildpnpm lint:check(existing warnings only)