Conversation
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.
| 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.
|
@maxwellpeterson Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b304988 to
729d77c
Compare
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
|
||
| const skillCountLabel = (count: number) => `${count} ${count === 1 ? "skill" : "skills"}`; | ||
|
|
||
| const nodeId = (collectionId: string, node: SkillNavigatorNode) => node.type === "skill" |
There was a problem hiding this comment.
[P2] Keep a stable identity across skill moves
The shared list restores focus after an async move by looking up the moved item's id, but this ID includes manifestPath. Every successful cross-directory move changes that path, so when the refreshed navigator arrives the old row unmounts and HierarchicalListPrimitive cannot find the pending item; a keyboard move such as Alt+Left leaves focus on <body>. Preserve a stable item ID or explicitly map/refocus the destination path after the move.
| expandAll={expandAll} | ||
| dragAndDrop={{ | ||
| autoScroll: true, | ||
| canMoveTo: (item, parent) => { |
There was a problem hiding this comment.
[P2] Provide a keyboard path to every destination folder
canMoveSkillNavigatorNode rejects same-parent moves, but the shared keyboard model reaches a folder only with Alt+Right on the immediately preceding sibling. Because this navigator sorts directories before skills, most skills have another skill immediately before them and cannot enter any folder; even the first skill can enter only the final directory. Pointer users can drop on any directory, so keyboard-only users cannot perform the advertised cross-directory workflow. Add an accessible destination picker or a product-specific keyboard move mechanism rather than exposing only the generic positional shortcuts.
| /> | ||
| <GitTokenManager | ||
| collectionId={collection.id} | ||
| branch={collection.content.branch} |
There was a problem hiding this comment.
[P2] Use the edited branch in mirror setup instructions
This passes the persisted branch while the adjacent field edits local branch state. If a user changes main to release, creates a token, follows the shown instructions, and then saves, the mirror is configured to push main while the collection subsequently refreshes release. Pass the current branch value (or require saving before token setup) so the one-time credential workflow cannot produce a mismatched mirror.
| Add skill | ||
| </DropdownMenu.Item> | ||
| )} | ||
| <DropdownMenu.Item |
There was a problem hiding this comment.
[P2] Wait for viewer capabilities before enabling collection creation
The Add menu is interactive while status === "loading", but viewerInfo remains its default { isAdmin: false, supportsGitCollections: false } until every collection's documents and access metadata finish loading. An admin can open and submit this dialog during that window and create an irreversible private web collection without being offered public or Git options. Disable collection creation until capabilities are loaded, or load/store those capabilities independently of the full navigator request.
|
Posted 4 actionable inline review comments. |
…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
- Keep successful collections visible when one document load fails. - Validate every generated destination path during skill moves. - Validate rewritten manifest size during skill renames. - Update optimistic rename state with new manifest/directory paths. - Only restore focus after rename when completed via Enter/Escape. - Refresh navigator again after Git collection setup closes. - Clarify legacy directory compatibility in README.
729d77c to
b8f9c38
Compare
| let manifest = this.storage.documents.get(manifestPath); | ||
| if (!manifest) throw new Error(`Document not found: ${manifestPath}`); |
There was a problem hiding this comment.
🔴 Invalid manifests trigger subtree deletion
deleteContextSkill accepts any existing SKILL.md without validating it. A stale delete then removes the directory after the file stops being a skill.
Learn more
Skill deletion treats the containing directory as the skill bundle, so its broader deletion scope is safe only after the target is confirmed as a valid manifest. Creation, movement, and rename parse the manifest before changing a bundle, but deletion checks only the filename and existence. The collection editor can overwrite a manifest through putContextDocument, so a manifest displayed by an earlier navigator load can become invalid before the delete RPC arrives.
Example: The navigator loads release/SKILL.md as a skill with release/checklist.md. Another editor removes its frontmatter. Clicking the stale Delete action removes both files, although release/SKILL.md is now an ordinary invalid document.
Recommended fix: Decode and parse the stored manifest before deriving and deleting its directory, including the text-content requirement used by skill discovery. Reject invalid targets without deleting any documents.
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)