feat(dashboard): add delete button to Skills page - #247
Conversation
Adds a trash icon delete button that appears on hover for each skill card.
- Calls DELETE /api/skills/{id} with confirmation dialog
- Uses existing handleSkillDelete backend endpoint
- Button visible only on hover to avoid accidental clicks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Previously the whole skill card was a clickable link, but now only the text area is wrapped in the
<a>while the padding/hover background is on the outer<div>; if preserving the full-card click target is desired, consider moving thehrefback to the outer container and handling delete viaevent.stopPropagation()instead. - Instead of using an inline
onclick="deleteSkill(this)", consider attaching the click listener in JavaScript (e.g., via event delegation on the container) to keep behavior separate from markup and make future JS changes easier to manage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Previously the whole skill card was a clickable link, but now only the text area is wrapped in the `<a>` while the padding/hover background is on the outer `<div>`; if preserving the full-card click target is desired, consider moving the `href` back to the outer container and handling delete via `event.stopPropagation()` instead.
- Instead of using an inline `onclick="deleteSkill(this)"`, consider attaching the click listener in JavaScript (e.g., via event delegation on the container) to keep behavior separate from markup and make future JS changes easier to manage.
## Individual Comments
### Comment 1
<location path="cmd/app/dashboard/pages/skills.html" line_range="148-157" />
<code_context>
container.innerHTML = html;
}
+async function deleteSkill(btn) {
+ const id = btn.dataset.skillId;
+ const name = btn.dataset.skillName;
+ if (!confirm(`Delete skill "${name}"? This cannot be undone.`)) {
+ return;
+ }
+
+ try {
+ const response = await fetch(`/api/skills/${id}`, {
+ method: 'DELETE'
+ });
+ if (!response.ok) {
+ alert('Failed to delete skill: HTTP ' + response.status);
+ return;
+ }
+ const data = await response.json();
+
+ if (data.success) {
</code_context>
<issue_to_address>
**issue:** Handle DELETE responses that might not include a JSON body (e.g. 204 No Content) to avoid runtime errors.
The function currently calls `await response.json()` unconditionally. Many DELETE endpoints return `204 No Content`, where this will throw. Guard the JSON parsing (e.g., based on status or `content-type`/`content-length`), or wrap it in try/catch and treat 204 as a success with no body.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request adds deletion support to the skills management dashboard. The skills list page now displays a trash icon button for each skill row that triggers an inline delete handler. When clicked, the handler prompts the user to confirm deletion by skill name, then sends a DELETE request to the backend API endpoint. Upon successful deletion, the skills list automatically refreshes to reflect the removal. The changes are contained to the client-side HTML template and include both the new delete confirmation logic and the updated markup that wires the delete buttons to the handler function. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d198461e63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (data.success) { | ||
| // Reload the list | ||
| loadSkillsList(); |
There was a problem hiding this comment.
In the normal delete flow this reload can race the backend: handleSkillDelete only removes the file and explicitly leaves removal from memory to the file watcher (cmd/app/skills.go:628-636), while /api/skills/list is built from the in-memory skill list (plugin/manager.go:161-164). The watcher polls once per second and then waits another 500ms debounce before calling Skills.Delete (plugin/sql/plugin.go:201-226, plugin/sql/plugin.go:270-289), so an immediate loadSkillsList() often re-renders the just-deleted skill and makes the delete appear to have failed until a manual refresh/later reload. Consider removing the card optimistically or delaying/polling until the list no longer contains the id.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/app/dashboard/pages/skills.html (2)
163-163: ⚡ Quick winAdd defensive error handling for JSON parsing.
response.json()can throw if the response body is not valid JSON. While your backend should return JSON, defensive coding improves robustness, especially if the server returns HTML error pages for 5xx errors or the response Content-Type is unexpected.🛡️ Proposed fix with try-catch for JSON parsing
if (!response.ok) { alert('Failed to delete skill: HTTP ' + response.status); return; } - const data = await response.json(); + + let data; + try { + data = await response.json(); + } catch (parseErr) { + alert('Failed to parse server response: ' + parseErr.message); + return; + } if (data.success) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/app/dashboard/pages/skills.html` at line 163, The call to response.json() can throw on non-JSON responses; wrap the JSON parsing in a try-catch and handle non-OK or non-JSON responses gracefully: first check response.ok (or status) and then attempt await response.json() inside a try block, catch parsing errors, optionally call await response.text() for debugging, and log or surface a clear error before returning/throwing. Update the code around the existing response.json() usage so that failures don’t crash the page and provide a useful error path.
151-151: Consider custom dialogs for consistency with design system.The native
confirm()andalert()dialogs work but don't match your polished UI design (custom colors, rounded corners, transitions). A custom modal component would provide a more consistent user experience.This is a minor polish item that could be deferred to a future refactor.
Also applies to: 160-160, 169-169, 172-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/app/dashboard/pages/skills.html` at line 151, Replace the native confirm() and alert() usages with the app's custom modal/dialog component to match the design system: create or reuse an async confirmDialog(showMessage, options) that returns a boolean and call it in place of confirm(`Delete skill "${name}"?...`), and replace alert() calls with a styled infoDialog/showToast; update the deleteSkill handler and the other places where confirm()/alert() are used so they await the modal result and proceed only if true, preserving existing message text and success/error flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/app/dashboard/pages/skills.html`:
- Line 107: The current code builds HTML using unescaped user data (skill.name,
skill.description, skill.error) and assigns it via container.innerHTML,
introducing an XSS risk; change rendering to set safe text content instead of
injecting raw HTML: keep the existing HTML shell but remove direct interpolation
of skill fields, assign container.innerHTML = html; then iterate the rendered
blocks (use container.querySelectorAll('.block') or the same selector used now),
and for each block set its h4.textContent = skills[i].name, set the description
paragraph's textContent = skills[i].description, and set the error paragraph's
textContent and title via Element.textContent and Element.setAttribute
respectively so no user string is interpreted as HTML or script; alternatively
perform server-side HTML-escaping of skill.name/description/error before they
reach this script.
- Around line 148-174: The deleteSkill function allows multiple rapid clicks;
fix it by early-return if btn.disabled is true, then set btn.disabled = true
immediately after extracting id/name to block further clicks, perform the async
fetch as before, and on non-success paths (response not ok, data.success false,
or catch error) re-enable the button (btn.disabled = false) so the user can
retry; do not re-enable on success since loadSkillsList() will re-render the
list. Reference: deleteSkill, btn, loadSkillsList.
- Around line 129-135: The delete button (the <button> with title "Delete skill"
containing the trash SVG) is fully hidden via "opacity-0
group-hover:opacity-100", which prevents touch, keyboard, and screen-reader
users from accessing it; change its utility classes so it is visible by default
at low opacity and becomes fully opaque on hover/focus — e.g., replace the base
"opacity-0" with a subtle visible state like "opacity-60", keep
"hover:opacity-100", and add keyboard/touch accessibility classes such as
"focus:opacity-100", "focus-visible:opacity-100" and
"group-focus-within:opacity-100" (or a responsive rule to always show on small
screens) so the button is discoverable for keyboard, touch, and assistive tech
users.
---
Nitpick comments:
In `@cmd/app/dashboard/pages/skills.html`:
- Line 163: The call to response.json() can throw on non-JSON responses; wrap
the JSON parsing in a try-catch and handle non-OK or non-JSON responses
gracefully: first check response.ok (or status) and then attempt await
response.json() inside a try block, catch parsing errors, optionally call await
response.text() for debugging, and log or surface a clear error before
returning/throwing. Update the code around the existing response.json() usage so
that failures don’t crash the page and provide a useful error path.
- Line 151: Replace the native confirm() and alert() usages with the app's
custom modal/dialog component to match the design system: create or reuse an
async confirmDialog(showMessage, options) that returns a boolean and call it in
place of confirm(`Delete skill "${name}"?...`), and replace alert() calls with a
styled infoDialog/showToast; update the deleteSkill handler and the other places
where confirm()/alert() are used so they await the modal result and proceed only
if true, preserving existing message text and success/error flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 903ee4b5-50b2-48a4-9210-b4c1205e485e
📒 Files selected for processing (1)
cmd/app/dashboard/pages/skills.html
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/app/dashboard/pages/skills.html">
<violation number="1" location="cmd/app/dashboard/pages/skills.html:128">
P1: Escape `skill.name` before inserting it into `data-skill-name` to prevent attribute injection from crafted skill names.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address code review comments: - Restore full card as clickable link (whole card is now <a>) - Event delegation for delete button instead of inline onclick - Handle 204 No Content responses without JSON body - Use content-type header check before parsing JSON response - Use event.stopPropagation() to prevent card link navigation when clicking delete
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/app/dashboard/pages/skills.html">
<violation number="1" location="cmd/app/dashboard/pages/skills.html:110">
P2: This introduces a nested interactive element (`button` inside `<a>`), which is invalid markup and can cause inconsistent interaction behavior.</violation>
<violation number="2" location="cmd/app/dashboard/pages/skills.html:148">
P1: `container` is out of scope here, causing a runtime `ReferenceError` and breaking page script execution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address multiple code review issues:
1. Fix nested interactive elements (button in <a>): use overlay <a> pattern instead
2. Fix delete button accessibility: change opacity-0 to opacity-60 with focus support
3. Fix XSS vulnerability: add escapeHtml() function for skill.name, description, error
4. Fix double-click prevention: add deletingSkills Set to track in-flight deletions
5. Fix container scope issue: use document.getElementById('skills-list') explicitly
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/app/dashboard/pages/skills.html">
<violation number="1" location="cmd/app/dashboard/pages/skills.html:125">
P2: The edit link is hidden from keyboard and assistive tech, making skill editing inaccessible for non-pointer users.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| <div class="flex items-start justify-between"> | ||
| <div class="flex-1 min-w-0"> | ||
| <div class="block p-4 hover:bg-surfaceHover transition-colors group relative"> | ||
| <a href="/skills/edit/${skill.id}" class="absolute inset-0" tabindex="-1" aria-hidden="true"></a> |
There was a problem hiding this comment.
P2: The edit link is hidden from keyboard and assistive tech, making skill editing inaccessible for non-pointer users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/app/dashboard/pages/skills.html, line 125:
<comment>The edit link is hidden from keyboard and assistive tech, making skill editing inaccessible for non-pointer users.</comment>
<file context>
@@ -102,19 +113,23 @@ <h2 class="text-lg font-semibold text-text">Loaded Skills</h2>
- <div class="flex items-start justify-between">
- <div class="flex-1 min-w-0">
+ <div class="block p-4 hover:bg-surfaceHover transition-colors group relative">
+ <a href="/skills/edit/${skill.id}" class="absolute inset-0" tabindex="-1" aria-hidden="true"></a>
+ <div class="flex items-start justify-between relative z-10">
+ <div class="flex-1 min-w-0 pr-16">
</file context>
| <a href="/skills/edit/${skill.id}" class="absolute inset-0" tabindex="-1" aria-hidden="true"></a> | |
| <a href="/skills/edit/${skill.id}" class="absolute inset-0" aria-label="Edit skill ${escapedName}"></a> |
Tip: Review your code locally with the cubic CLI to iterate faster.
2 participants
Summary
Adds delete functionality to the Skills page in the dashboard.
Changes
Testing
Related Issue
Fixes #246