feat: per-skill deletion protection#226
Merged
Merged
Conversation
Add a new `protectSlice` (SkillName[]) persisted via redux-persist. Wire it into the root reducer and create a migration that initialises the new key with an empty array in existing persisted states. Exports: addProtection, removeProtection, selectProtectedNamesSet (memoised ReadonlySet), selectIsProtected (plain predicate).
Add optional `protectedNames: ReadonlySet<Skill['name']>` third param (defaults to empty set). Protected skills are intercepted before the orphan / stale paths and returned in a new `protectedErrors` bucket with `outcome: 'error'` / `code: 'EPROTECTED'`. This lets callers distinguish intentional skips from genuine failures.
SkillItem: - Extract ProtectButton sub-component (Lock/LockOpen icons, aria-labels) - Suppress the delete X button when a skill is locked (isProtected gate) - ProtectButton position uses showDeleteButtonBase (not post-gate value) so locking a skill does not shift the button's right-position class skillItemHelpers: - getCardContentPaddingClass now accepts showProtect to count 3-overlay rows correctly (pr-36 when lock + bookmark + X all coexist) bulkDeleteCopy: - Add early-exit guard: if all selected skills are protected (trashCount=0 and all other counts=0), return 'All selected skills are protected and cannot be deleted.' before the ts-pattern match - Append 'N protected skills will be skipped.' sentence when protectedCount > 0 MainContent: - Pass protectedNames (selectProtectedNamesSet) to partitionGlobalDeleteTargets - Exclude protectedErrors from deleteItems: they are intentional skips, not failures — keeping them inflated formatCascadeSummary denominators and caused protected rows to flash red and re-enter the retry selection set
DESIGN.md: - Amber semantic cap (destructive only for delete/unlink, never lock) - Toggleable Protection Controls: opacity-40/70/100 progression, tooltip req - Bulk Destructive Dialogs with Skipped Items: skip count as secondary copy, all-protected → disable CTA TODOS.md: add P2/P3 follow-up items for the protection feature
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. Walkthrough新規 Redux スライス Changesスキル保護機能の追加
Sequence Diagram(s)sequenceDiagram
participant User
participant SkillItem
participant protectSlice
participant partitionGlobalDeleteTargets
participant MainContent
participant renderBulkDeleteDescription
User->>SkillItem: Lock ボタンクリック
SkillItem->>protectSlice: dispatch(addProtection(name))
protectSlice-->>SkillItem: selectIsProtected → true (Delete ボタン非表示)
User->>MainContent: バルク削除 Primary Action
MainContent->>protectSlice: selectProtectedNamesSet(state)
protectSlice-->>MainContent: protectedNamesSet
MainContent->>partitionGlobalDeleteTargets: skills, skillNames, protectedNamesSet
partitionGlobalDeleteTargets-->>MainContent: { deleteTargets, protectedErrors, ... }
MainContent->>MainContent: dispatch(setBulkConfirm({ protectedErrors, ... }))
MainContent->>renderBulkDeleteDescription: protectedCount = protectedErrors.length
renderBulkDeleteDescription-->>User: ダイアログに「N protected skills will be skipped」表示
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
==========================================
- Coverage 95.91% 95.84% -0.08%
==========================================
Files 184 185 +1
Lines 5580 5627 +47
Branches 1253 1272 +19
==========================================
+ Hits 5352 5393 +41
- Misses 1 4 +3
- Partials 227 230 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/skills/bulkDeleteCopy.tsx (1)
76-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick win保護スキル混在時の削除説明件数が誤表示になります
Line 76-92 の
hasBaseTrashCopyがprotectedCountを見ていないため、trashCount < totalCount（例: 一部が protected）でも「全件を trash に移動」と表示されます。破壊的操作の確認文言として誤解を招くため、trashCountベースの文言分岐に入るようにしてください。差分案
- const hasBaseTrashCopy = - orphanCleanupCount === 0 && - orphanRescanCount === 0 && - staleDeleteCount === 0 + const hasBaseTrashCopy = + orphanCleanupCount === 0 && + orphanRescanCount === 0 && + staleDeleteCount === 0 && + protectedCount === 0Also applies to: 123-127
🤖 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 `@src/renderer/src/components/skills/bulkDeleteCopy.tsx` around lines 76 - 92, The hasBaseTrashCopy condition does not account for protected skills, so when some skills are protected (protectedCount > 0), the copy still displays "moves all skills to trash" even though not all items can be moved due to the protection. Add a check for protectedCount === 0 to the hasBaseTrashCopy condition alongside the existing checks for orphanCleanupCount, orphanRescanCount, and staleDeleteCount. This ensures the "base trash copy" message only shows when there are genuinely no protected items. The same fix should be applied at the other affected site (around line 123-127) where similar logic handles protected item scenarios.
🤖 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 `@src/renderer/src/components/skills/SkillItem.tsx`:
- Around line 475-486: The handleDeleteClick function passes an empty Set() as
the protected skills parameter to partitionGlobalDeleteTargets, relying solely
on UI gating (the delete button being hidden for protected skills) to prevent
deletion of protected skills. This is fragile—if the UI gate assumption breaks,
protected skills can flow into deleteTargets. Pass the actual set of protected
skills to partitionGlobalDeleteTargets instead of an empty set at the call site
around line 485. Also apply the same fix at the other location around line
498-499 where partitionGlobalDeleteTargets is called with an empty protected
set. This ensures the business logic layer itself enforces protection exclusion
regardless of UI state.
- Around line 699-703: The X button existence check is duplicated across the
component, causing a padding mismatch. Line 702 in the ProtectButton component
uses showDeleteButtonBase to determine if the X button should be shown, while
the padding calculation around line 755 uses showDeleteButton which returns
false when protected. This inconsistency causes incorrect right padding (pr-24)
when the component is protected, leading to overlay text overlap. Unify the X
button existence judgment by using a single variable consistently across both
the ProtectButton hasXButton prop and the padding calculation logic. Create one
variable (such as shouldShowXButton or similar) that accurately reflects when
the X button will be displayed, and use this variable in both locations instead
of mixing showDeleteButtonBase and showDeleteButton.
---
Outside diff comments:
In `@src/renderer/src/components/skills/bulkDeleteCopy.tsx`:
- Around line 76-92: The hasBaseTrashCopy condition does not account for
protected skills, so when some skills are protected (protectedCount > 0), the
copy still displays "moves all skills to trash" even though not all items can be
moved due to the protection. Add a check for protectedCount === 0 to the
hasBaseTrashCopy condition alongside the existing checks for orphanCleanupCount,
orphanRescanCount, and staleDeleteCount. This ensures the "base trash copy"
message only shows when there are genuinely no protected items. The same fix
should be applied at the other affected site (around line 123-127) where similar
logic handles protected item scenarios.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c45d9822-daa7-4f01-87b8-00d58bf72cc2
📒 Files selected for processing (21)
DESIGN.mdTODOS.mdsrc/renderer/src/components/layout/MainContent.browser.test.tsxsrc/renderer/src/components/layout/MainContent.selectionToolbar.browser.test.tsxsrc/renderer/src/components/layout/MainContent.tsxsrc/renderer/src/components/skills/SkillItem.browser.test.tsxsrc/renderer/src/components/skills/SkillItem.tsxsrc/renderer/src/components/skills/SkillsList.browser.test.tsxsrc/renderer/src/components/skills/bulkDeleteCopy.test.tssrc/renderer/src/components/skills/bulkDeleteCopy.tsxsrc/renderer/src/components/skills/reviewedDestructiveTargets.test.tssrc/renderer/src/components/skills/reviewedDestructiveTargets.tssrc/renderer/src/components/skills/skillItemHelpers.test.tssrc/renderer/src/components/skills/skillItemHelpers.tssrc/renderer/src/redux/migrations.tssrc/renderer/src/redux/slices/protectSlice.test.tssrc/renderer/src/redux/slices/protectSlice.tssrc/renderer/src/redux/slices/uiSlice.test.tssrc/renderer/src/redux/slices/uiSlice.tssrc/renderer/src/redux/store.test.tssrc/renderer/src/redux/store.ts
- Pass real protection state to partitionGlobalDeleteTargets in single-item delete handler for defense-in-depth (no longer relies solely on UI gate) - Use showDeleteButtonBase (pre-gate) for CardContent padding so the lock button position and the right-padding slot count always agree
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/skills/SkillItem.tsx (1)
123-128:⚠️ Potential issue | 🟡 Minor
right-22は Tailwind で生成されません。任意値または拡張が必要です。Tailwind 4.3.1 のデフォルト spacing には
22が含まれず、tailwind.config.tsに拡張もありません。生成 CSS にright-22がないため、3 ボタン全て表示時（lock + bookmark + X）に ProtectButton の位置計算が失敗しています。
right-[5.5rem]などの任意値に修正するか、spacingを拡張してください。加えて、このスロット→クラス分岐ロジックをskillItemHelpers.tsの pure helper に抽出すれば、組み合わせを単体テストできます（coding guideline の「Extract conditional rendering logic into pure helper functions」に準拠）。修正例
const rightClass = showBookmark && hasXButton - ? 'right-22' + ? 'right-[5.5rem]' : hasXButton || showBookmark ? 'right-11' : 'right-0'または、ヘルパー関数化：
// skillItemHelpers.ts に追加 export function getProtectButtonRightClass(flags: { showBookmark: boolean showUnlinkButton: boolean showDeleteButton: boolean }): 'right-[5.5rem]' | 'right-11' | 'right-0' { const hasXButton = flags.showUnlinkButton || flags.showDeleteButton if (flags.showBookmark && hasXButton) return 'right-[5.5rem]' if (flags.showBookmark || hasXButton) return 'right-11' return 'right-0' }🤖 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 `@src/renderer/src/components/skills/SkillItem.tsx` around lines 123 - 128, The conditional logic for rightClass in SkillItem.tsx is using `right-22`, which is not a valid Tailwind spacing value in version 4.3.1 and will not generate any CSS. Replace `right-22` with the arbitrary Tailwind value `right-[5.5rem]` in the ternary expression. Additionally, extract this entire conditional logic into a pure helper function called getProtectButtonRightClass in skillItemHelpers.ts that accepts an object with showBookmark, showUnlinkButton (hasXButton condition), and showDeleteButton flags, and returns the appropriate class name string. This extraction follows the coding guideline for extracting conditional rendering logic and allows the combination logic to be unit tested independently before being used in the component.Source: Coding guidelines
🤖 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 `@src/renderer/src/components/skills/SkillItem.tsx`:
- Around line 483-489: The code is widening `SkillName` to `string` when
creating the set passed to `partitionGlobalDeleteTargets`, which breaks the type
contract that expects `ReadonlySet<Skill['name']>` (i.e.,
`ReadonlySet<SkillName>`). Remove the `as string` type cast from the conditional
expression and ensure both branches of the ternary operator create a
`Set<SkillName>` (or appropriate readonly set of SkillName) without widening to
the string type. This maintains type safety and prevents contract breakage if
SkillName becomes a branded type in the future.
---
Outside diff comments:
In `@src/renderer/src/components/skills/SkillItem.tsx`:
- Around line 123-128: The conditional logic for rightClass in SkillItem.tsx is
using `right-22`, which is not a valid Tailwind spacing value in version 4.3.1
and will not generate any CSS. Replace `right-22` with the arbitrary Tailwind
value `right-[5.5rem]` in the ternary expression. Additionally, extract this
entire conditional logic into a pure helper function called
getProtectButtonRightClass in skillItemHelpers.ts that accepts an object with
showBookmark, showUnlinkButton (hasXButton condition), and showDeleteButton
flags, and returns the appropriate class name string. This extraction follows
the coding guideline for extracting conditional rendering logic and allows the
combination logic to be unit tested independently before being used in the
component.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92b3f1af-d4e8-45e1-b77c-77bf29b6e8a1
📒 Files selected for processing (1)
src/renderer/src/components/skills/SkillItem.tsx
Avoid widening SkillName to string when building the protected-names set passed to partitionGlobalDeleteTargets
5 tasks
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
Adds per-skill deletion protection so users can lock individual skills against accidental deletion. Protected skills are excluded from both single-item and bulk delete flows at every layer — Redux state, business logic, and UI.
Changes
State (
protectSlice)SkillName[]state persisted viaredux-persistselectProtectedNamesSet(memoizedReadonlySet),selectIsProtectedprotect: []for existing persisted statesBusiness logic (
partitionGlobalDeleteTargets)protectedNamesparam (defaults to empty set)protectedErrorsbucket withcode: 'EPROTECTED'UI
ProtectButtonextracted fromSkillItem: Lock/LockOpen icons witharia-label="Lock {name}"/"Unlock {name}"and tooltipshowDeleteButtonBaseso locking never shifts adjacent controlsgetCardContentPaddingClassupdated topr-36for 3-overlay rows (lock + bookmark + X)MainContent.tsxexcludesprotectedErrorsfromdeleteItemsso protected rows never flash red or enter retry selectionDocs
DESIGN.md: amber semantic cap rule, toggleable protection control states (opacity-40/70/100), tooltip requirement for toggleable controlsTODOS.md: P2/P3 follow-up itemsSummary by CodeRabbit
新機能
変更・改善