fix(category-dialog): defer state updates until after save succeeds#38
fix(category-dialog): defer state updates until after save succeeds#38
Conversation
…y list Automatically assign number keys (1-9, then 0) to newly created categories for quick toggling. Display action labels in the hotkey list sidebar. Features: - Auto-assign first available number key when creating a category - Check keys in order: 1, 2, 3, 4, 5, 6, 7, 8, 9, then 0 - Only use unassigned number keys without modifiers - Show action labels (e.g., 'Toggle Keep', 'Next Image') in hotkey list - Extract formatActionLabel to reusable utility function Changes: - Add findUnassignedNumberKey() to find available number keys - Add autoAssignHotkeyToCategory() to create and save hotkeys - Add formatActionLabel() to format action strings for display - Update CategoryDialog to auto-assign hotkeys on category creation - Update HotkeyList to display action labels alongside key combinations - Add comprehensive tests for all new functionality - Update README with documentation for new features Fixes #34
Implement optimistic updates with rollback mechanism to ensure state remains consistent with persistent storage. Separate error handling for saveAppData() and autoAssignHotkeyToCategory() with distinct error messages. Dialog closes on save success even if hotkey assignment fails, preventing non-persistent categories from appearing in UI.
WalkthroughThis PR introduces automatic hotkey assignment for new categories, adding utility functions to find unassigned numeric keys and auto-assign them with persistence, integrating this into the category creation flow with error handling and rollback, and updating documentation and UI to reflect this behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant CategoryDialog
participant AppState
participant HotkeyUtil
participant Storage
User->>CategoryDialog: Create new category
activate CategoryDialog
CategoryDialog->>AppState: Update state optimistically
CategoryDialog->>Storage: Save category
activate Storage
Storage-->>CategoryDialog: Save successful
deactivate Storage
rect rgb(220, 240, 230)
Note over CategoryDialog,HotkeyUtil: Auto-assign hotkey (new flow)
CategoryDialog->>HotkeyUtil: autoAssignHotkeyToCategory()
activate HotkeyUtil
HotkeyUtil->>HotkeyUtil: findUnassignedNumberKey()
HotkeyUtil->>Storage: Save hotkey assignment
activate Storage
Storage-->>HotkeyUtil: Success
deactivate Storage
HotkeyUtil-->>CategoryDialog: true (hotkey assigned)
deactivate HotkeyUtil
end
CategoryDialog-->>User: Dialog closes
deactivate CategoryDialog
alt Save fails
activate CategoryDialog
Storage-->>CategoryDialog: Failure
CategoryDialog->>AppState: Rollback state
CategoryDialog-->>User: Show error notification
deactivate CategoryDialog
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #38 +/- ##
==========================================
+ Coverage 94.31% 94.45% +0.13%
==========================================
Files 19 19
Lines 3045 3121 +76
Branches 535 568 +33
==========================================
+ Hits 2872 2948 +76
Misses 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/HotkeyList.tsx (1)
13-13: Nit: Extra blank line.There's an unnecessary extra blank line after the
formatHotkeyDisplayfunction.} - export function HotkeyList() {src/components/CategoryDialog.tsx (2)
104-129: Variable shadowing:errorMessageshadows the state variable.The
const errorMessageon line 126 shadows the state variableerrorMessagedeclared on line 31. While this works correctly, it reduces readability and could cause confusion during future maintenance.} catch (error) { // Rollback state on save failure setCategories(originalCategories); // Show error to user and keep dialog open so they can retry or cancel - const errorMessage = error instanceof Error ? error.message : String(error); - setErrorMessage(`Failed to save category: ${errorMessage}`); + const saveErrorMsg = error instanceof Error ? error.message : String(error); + setErrorMessage(`Failed to save category: ${saveErrorMsg}`); setShowError(true); }
138-169: Well-structured optimistic update with rollback and non-fatal hotkey assignment.The implementation correctly:
- Stores original state before optimistic update
- Attempts save and rolls back on failure
- Treats hotkey assignment failure as non-fatal (shows notification but still closes dialog)
Same shadowing issue as the edit path applies here on line 166.
} catch (error) { // Rollback state on save failure setCategories(originalCategories); // Show error to user and keep dialog open so they can retry or cancel - const errorMessage = error instanceof Error ? error.message : String(error); - setErrorMessage(`Failed to save category: ${errorMessage}`); + const saveErrorMsg = error instanceof Error ? error.message : String(error); + setErrorMessage(`Failed to save category: ${saveErrorMsg}`); setShowError(true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(6 hunks)src/components/CategoryDialog.tsx(3 hunks)src/components/HotkeyList.tsx(2 hunks)src/ui/hotkeys.test.ts(1 hunks)src/ui/hotkeys.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
Use soft tabs (spaces) for indentation, never hard tabs, following the existing codebase style of typically 2 spaces for TypeScript/React files
Files:
src/ui/hotkeys.test.tssrc/components/HotkeyList.tsxsrc/ui/hotkeys.tssrc/components/CategoryDialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
When calling Tauri commands from TypeScript, use
camelCasefor parameters (e.g.,dataFilePath: value). Do not use snake_case in TypeScript
Files:
src/ui/hotkeys.test.tssrc/components/HotkeyList.tsxsrc/ui/hotkeys.tssrc/components/CategoryDialog.tsx
🧬 Code graph analysis (4)
src/ui/hotkeys.test.ts (3)
src/utils/jotaiStore.ts (1)
store(6-6)src/state.ts (2)
hotkeysAtom(12-12)categoriesAtom(14-14)src/ui/hotkeys.ts (3)
findUnassignedNumberKey(304-320)autoAssignHotkeyToCategory(327-356)formatActionLabel(364-411)
src/components/HotkeyList.tsx (3)
src/ui/hotkeys.ts (1)
formatActionLabel(364-411)src/types.ts (1)
HotkeyConfig(30-35)src/state.ts (2)
hotkeysAtom(12-12)categoriesAtom(14-14)
src/ui/hotkeys.ts (4)
src/utils/jotaiStore.ts (1)
store(6-6)src/state.ts (1)
hotkeysAtom(12-12)src/types.ts (1)
HotkeyConfig(30-35)src/ui/categories.ts (1)
saveAppData(167-192)
src/components/CategoryDialog.tsx (2)
src/ui/categories.ts (1)
saveAppData(167-192)src/ui/hotkeys.ts (1)
autoAssignHotkeyToCategory(327-356)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codecov
🔇 Additional comments (9)
README.md (1)
17-17: LGTM!The documentation updates accurately describe the new automatic hotkey assignment behavior, including the key ordering (1-9, then 0), the fallback when all keys are used, and the ability to edit/remove auto-assigned hotkeys. This aligns well with the implementation.
Also applies to: 30-30, 44-44, 128-128, 145-146, 173-181
src/components/HotkeyList.tsx (1)
3-5: LGTM!The integration of
formatActionLabelwith categories data is well implemented. The component now properly reads categories from state and displays human-readable action labels for each hotkey entry.Also applies to: 16-16, 44-44
src/components/CategoryDialog.tsx (1)
7-8: LGTM!Clean imports for the new auto-assign functionality and error notification. The alias
showErrorNotificationavoids confusion with the localshowErrorstate variable.src/ui/hotkeys.test.ts (3)
1099-1197: LGTM!Comprehensive test coverage for
findUnassignedNumberKeyincluding:
- Empty state returning "1"
- Sequential assignment behavior
- Full key ordering (1-9, then 0)
- Null return when exhausted
- Correctly ignoring keys with modifiers
- Handling non-number keys
- Finding gaps in assigned keys
1200-1336: LGTM!Thorough test coverage for
autoAssignHotkeyToCategoryincluding the critical rollback scenario whensaveAppDatafails. The tests verify both success and failure paths.
1338-1415: LGTM!Good coverage of
formatActionLabelincluding all action types, category name resolution, fallback to IDs for missing categories, and legacy action handling.src/ui/hotkeys.ts (3)
299-320: LGTM!Clean implementation of
findUnassignedNumberKey. The key ordering (1-9, then 0) is correct, and the check formodifiers.length === 0ensures only unmodified number keys are considered.
327-356: Well-implemented optimistic update with rollback.The implementation correctly:
- Checks for available keys first (early return)
- Stores original state before mutation
- Rolls back on save failure
- Returns boolean to indicate success
One minor consideration:
hotkey_${Date.now()}could theoretically collide if called within the same millisecond, but this is extremely unlikely in the UI context where this is triggered by user actions.
358-411: LGTM!The action prefix matching is correctly ordered -
toggle_category_next_is checked beforetoggle_category_to avoid incorrect partial matches. The fallback to displaying the category ID when the category is not found provides good UX for edge cases (e.g., deleted categories).
1 participant
Summary
Fixes error handling in CategoryDialog to ensure state remains consistent with persistent storage.
Changes
saveAppData()andautoAssignHotkeyToCategory()Problem
Previously, the component updated local state with new categories before persisting them. If
saveAppData()failed, the UI would show a non-persistent category. IfautoAssignHotkeyToCategory()failed, the error message incorrectly blamed saving.Solution
saveAppData()reads from state)Testing
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.