feat: Add ability to lock detections to protect them from cleanup #424
Conversation
- Implemented lock/unlock feature for detections - Added conditional rendering for delete and lock actions - Updated Alpine.js state management to track detection lock status - Introduced new event listeners for lock/unlock state changes
- Implemented lock management methods in datastore interfaces - Added NoteLock model to support note locking - Updated database migration to include NoteLock - Added methods for locking, unlocking, and checking note lock status - Modified note retrieval methods to preload and populate lock information - Added lock status check before note deletion - Excluded locked notes from clips qualifying for removal
- Introduced lock detection checkbox in review modal - Added dynamic display of lock section based on detection verification - Implemented client-side toggle for lock detection visibility - Provided explanatory text for detection locking functionality
- Refactored ReviewDetection handler to support more robust comment and review processing - Added support for locking detections during review - Implemented retry mechanism for database transactions to handle SQLite locking - Created separate methods for processing comments and reviews - Added new LockDetection handler for explicit note locking/unlocking - Improved error handling and notification mechanisms
- Added debug logging to processReview method for better traceability - Enhanced review modal with Alpine.js for dynamic lock state handling - Implemented client-side prevention of false positive marking for locked detections - Added lock state synchronization between server and client - Improved error handling and user feedback for locked detection scenarios - Simplified lock/unlock confirmation modal text
WalkthroughThis pull request introduces note locking functionality across multiple layers of the application. New methods are added to the datastore interface for locking, unlocking, and querying note locks, and the underlying data model is updated with a Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Web UI (HTML/Alpine.js)
participant HT as HTTP Controller
participant DS as DataStore
participant DB as Database
U->>UI: Initiates lock/unlock action
UI->>HT: Sends POST request to /detections/lock
HT->>DS: Calls LockDetection / processReview
DS->>DB: Queries/updates NoteLock and Note fields
DB-->>DS: Returns lock status result
DS-->>HT: Delivers processing outcome
HT-->>UI: Sends updated lock state response
UI->>UI: Updates UI state based on event triggers
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (6)
internal/datastore/interfaces.go (4)
169-170: Consider potential performance overhead.Preloading multiple relationships (Review, Lock, Comments) can be efficient for single-note retrievals, but watch out for possible performance impacts if this query is used in loops or high-volume endpoints.
181-183: Ensure test coverage for the newLockedfield.Confirm that unit tests account for various lock states to maintain correctness.
Would you like help creating tests that verify the
Lockedfield behavior across different scenarios?
735-759: Consider a unique constraint for lock enforcement.The upsert approach works, but multiple concurrent lock attempts could overwrite each other. A unique database constraint on
note_idwould prevent multiple locks from coexisting unintentionally.Below is an illustrative snippet you might add in a migration (not necessarily inline here), ensuring only one lock per note:
func (ds *DataStore) LockNote(noteID, description, lockedBy string) error { ... + // Example: enforce uniqueness in migrations or setup: + // db.Exec("CREATE UNIQUE INDEX IF NOT EXISTS idx_note_locks_note_id ON note_locks(note_id)") // Use upsert operation to either create or update the lock result := ds.DB.Where("note_id = ?", id). ... }
761-774: Logging unlock events.The unlock logic is straightforward. Consider capturing who unlocked the note if you need auditing or tighter control.
Example diff:
func (ds *DataStore) UnlockNote(noteID string) error { id, err := strconv.ParseUint(noteID, 10, 32) if err != nil { return fmt.Errorf("invalid note ID: %w", err) } + // Potentially store or log the user who performed the unlock, + // e.g. an "UnlockedBy" column or an audit trail. result := ds.DB.Where("note_id = ?", id).Delete(&NoteLock{}) ... }internal/httpcontroller/handlers/detections.go (2)
477-584: Potential concurrency concerns when overwriting locks.The logic properly prevents marking locked notes as false positives but might overwrite an existing lock from another user if verified as “correct” and lockDetection is true. Consider verifying the lock owner if multi-tenant usage is a concern.
668-741: Consider user validation when toggling locks.Lock/unlock logic is straightforward. If a user tries to unlock a note locked by someone else, you may want to enforce ownership checks or warnings, depending on business rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/datastore/interfaces.go(10 hunks)internal/datastore/manage.go(1 hunks)internal/datastore/model.go(2 hunks)internal/httpcontroller/handlers/detections.go(2 hunks)internal/httpcontroller/routes.go(1 hunks)views/elements/actionMenu.html(2 hunks)views/elements/reviewModal.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (25)
internal/datastore/interfaces.go (12)
50-54: New lock management interface methods.These methods broaden the store’s functionality to include locking operations. The signatures and parameters look consistent with the existing patterns.
195-203: Lock check before deletion.Preventing deletion for locked notes aligns with the new feature requirements. Looks good.
301-305: Lock exclusion in the subquery.Excluding locked notes from removal aligns with the locking logic. Implementation looks correct.
314-316: Lock exclusion in main query.Similar to the subquery logic, this ensures locked notes stay protected.
374-374: Preload theLockfor species detections.This helps retrieve lock information consistently.
389-389: Updated comment for clarity.No concerns.
394-394: Populate theLockedfield.Initializing this virtual field is consistent with the existing approach in other endpoints.
406-406: Preload lock inGetLastDetections.Good consistency across retrieval methods.
412-412: Populate virtual fields comment.No action needed.
417-417: Setting the note’sLockedproperty.Maintains consistency in marking the note’s lock state.
776-793: Retrieving the note lock.Implementation correctly returns nil when no record is found, which is consistent with typical “no lock” semantics.
795-802: Check if a note is locked.Straightforward logic that leverages
GetNoteLock.internal/httpcontroller/handlers/detections.go (2)
586-630: Unified review flow.The refactored workflow clearly separates comment and review handling. The new approach is cleaner and seems logically sound.
413-475: Clarify handling of multiple existing comments.Currently, only the first existing comment is updated. If multiple comments exist, others remain untouched. Verify this aligns with product requirements or consider updating all relevant comments.
Below is a script to locate other references to comment handling and confirm if multi-comment logic exists:
internal/datastore/manage.go (1)
27-27: IncludeNoteLockin migrations.Ensures the lock schema is created. Straightforward change and crucial for new lock features.
internal/datastore/model.go (3)
29-29: LGTM! The Lock field is properly configured.The one-to-one relationship with cascade delete behavior ensures that lock records are properly cleaned up when notes are deleted.
31-33: LGTM! Virtual fields are properly documented.The virtual fields maintain compatibility with templates and provide a clean way to expose lock status.
74-82: LGTM! The NoteLock struct is well-designed.The struct includes all necessary fields for tracking lock status:
IDandNoteIDfor proper relationshipsLockedAtfor timestamp trackingDescriptionfor lock reason documentationLockedByfor ownership trackinginternal/httpcontroller/routes.go (1)
131-132: LGTM! The lock route is properly configured.The route is:
- Using POST method appropriately for state modification
- Protected by authentication middleware
- Grouped with other detection-related routes
views/elements/reviewModal.html (3)
49-60: LGTM! Alpine.js state management is well-implemented.The component:
- Properly initializes lock state from server data
- Sets up event listeners for lock state changes
- Cleans up event listeners on component initialization
77-89: LGTM! Lock-related UI changes are user-friendly.The implementation:
- Properly disables false positive option when locked
- Shows clear warning message explaining why it's disabled
- Uses appropriate styling for disabled state
91-101: LGTM! Lock detection section is well-designed.The section:
- Only shows for correct detections
- Uses checkbox for intuitive toggling
- Provides clear explanation of locking purpose
views/elements/actionMenu.html (3)
6-19: LGTM! Lock state management is properly implemented.The implementation:
- Initializes lock state from server data
- Sets up event listeners for state changes
- Maintains consistency with other state management
92-125: LGTM! Lock/unlock action is well-designed.The implementation:
- Uses appropriate confirmation dialogs
- Provides clear explanations of lock/unlock effects
- Uses intuitive icons for lock states
129-163: LGTM! Delete action is properly secured.The implementation:
- Only shows delete option when detection is unlocked
- Maintains existing delete functionality
- Uses appropriate warning styling
- Removed unnecessary fields from NoteLock model - Simplified LockNote method signature - Removed description and lockedBy fields from lock mechanism - Updated datastore interface to reflect simplified locking approach
- Removed username parameter from processReview method - Simplified review status handling in processReview - Updated review modal to use Alpine.js for dynamic review status management - Improved lock state and review status visibility logic - Removed redundant username extraction in handlers - Simplified lock detection implementation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/datastore/interfaces.go (1)
735-800: Consider adding retry mechanism for database locks.The lock management implementations handle errors well but could benefit from retry logic for database locks, similar to other methods in the file.
func (ds *DataStore) LockNote(noteID string) error { id, err := strconv.ParseUint(noteID, 10, 32) if err != nil { return fmt.Errorf("invalid note ID: %w", err) } + maxRetries := 5 + baseDelay := 500 * time.Millisecond + var lastErr error + for attempt := 0; attempt < maxRetries; attempt++ { lock := &NoteLock{ NoteID: uint(id), LockedAt: time.Now(), } result := ds.DB.Where("note_id = ?", id). Assign(*lock). FirstOrCreate(lock) if result.Error != nil { + if strings.Contains(strings.ToLower(result.Error.Error()), "database is locked") { + delay := baseDelay * time.Duration(attempt+1) + time.Sleep(delay) + lastErr = result.Error + continue + } return fmt.Errorf("failed to lock note: %w", result.Error) } return nil + } + return fmt.Errorf("failed after %d attempts: %w", maxRetries, lastErr) }internal/httpcontroller/handlers/detections.go (1)
Line range hint
653-716: Consider adding transaction for atomic lock operations.The
LockDetectionhandler should wrap the lock state check and update in a transaction to prevent race conditions.func (h *Handlers) LockDetection(c echo.Context) error { id := c.QueryParam("id") if id == "" { h.SSE.SendNotification(Notification{ Message: "Missing detection ID", Type: "error", }) return h.NewHandlerError(fmt.Errorf("no ID provided"), "Missing detection ID", http.StatusBadRequest) } - // Get the current note to check its lock status - note, err := h.DS.Get(id) - if err != nil { - h.SSE.SendNotification(Notification{ - Message: fmt.Sprintf("Failed to get detection: %v", err), - Type: "error", - }) - return h.NewHandlerError(err, "Failed to get detection", http.StatusInternalServerError) - } - - // Check current lock status - isLocked, err := h.DS.IsNoteLocked(id) + var message string + err := h.DS.Transaction(func(tx *gorm.DB) error { + // Get the current note to check its lock status + note, err := h.DS.Get(id) + if err != nil { + return fmt.Errorf("failed to get detection: %w", err) + } + + // Check current lock status + isLocked, err := h.DS.IsNoteLocked(id) + if err != nil { + return fmt.Errorf("failed to check lock status: %w", err) + } + + if isLocked { + if err := h.DS.UnlockNote(id); err != nil { + return fmt.Errorf("failed to unlock detection: %w", err) + } + message = fmt.Sprintf("Detection of %s unlocked successfully", note.CommonName) + } else { + if err := h.DS.LockNote(id); err != nil { + return fmt.Errorf("failed to lock detection: %w", err) + } + message = fmt.Sprintf("Detection of %s locked successfully", note.CommonName) + } + return nil + }) if err != nil { h.SSE.SendNotification(Notification{ - Message: fmt.Sprintf("Failed to check lock status: %v", err), + Message: err.Error(), Type: "error", }) - return h.NewHandlerError(err, "Failed to check lock status", http.StatusInternalServerError) + return h.NewHandlerError(err, "Failed to process lock operation", http.StatusInternalServerError) } - var message string - if isLocked { - // Unlock the detection - if err := h.DS.UnlockNote(id); err != nil { - h.SSE.SendNotification(Notification{ - Message: fmt.Sprintf("Failed to unlock detection: %v", err), - Type: "error", - }) - return h.NewHandlerError(err, "Failed to unlock detection", http.StatusInternalServerError) - } - message = fmt.Sprintf("Detection of %s unlocked successfully", note.CommonName) - } else { - // Lock the detection - if err := h.DS.LockNote(id); err != nil { - h.SSE.SendNotification(Notification{ - Message: fmt.Sprintf("Failed to lock detection: %v", err), - Type: "error", - }) - return h.NewHandlerError(err, "Failed to lock detection", http.StatusInternalServerError) - } - message = fmt.Sprintf("Detection of %s locked successfully", note.CommonName) - } // Send success notification h.SSE.SendNotification(Notification{ Message: message, Type: "success", })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/datastore/interfaces.go(10 hunks)internal/datastore/model.go(2 hunks)internal/httpcontroller/handlers/detections.go(1 hunks)views/elements/reviewModal.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- views/elements/reviewModal.html
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/datastore/model.go (3)
29-29: LGTM! Well-designed one-to-one relationship.The
Lockfield is properly defined with cascade delete behavior and appropriate foreign key constraints.
31-33: LGTM! Clear documentation of virtual fields.The virtual fields are properly marked with
gorm:"-"and have clear comments explaining their population mechanism.
74-80: LGTM! Well-structured lock entity.The
NoteLockstruct is well-designed with:
- Proper primary key
- Unique index on NoteID to ensure one-to-one relationship
- Appropriate foreign key constraint with cascade behavior
- Required timestamp field for audit purposes
internal/datastore/interfaces.go (4)
50-54: LGTM! Well-defined lock management interface.The lock management methods provide a complete set of operations with clear signatures.
Line range hint
169-183: LGTM! Proper relationship preloading and virtual field population.The
Getmethod correctly:
- Preloads the Lock relationship alongside Review and Comments
- Populates the virtual Locked field based on Lock presence
195-202: LGTM! Proper lock protection.The
Deletemethod correctly checks for locks before deletion, preventing accidental removal of locked notes.
301-304: LGTM! Proper exclusion of locked notes.The query correctly excludes locked notes from cleanup using appropriate join and where clauses.
internal/httpcontroller/handlers/detections.go (3)
413-475: LGTM! Well-structured comment processing.The
processCommentfunction:
- Properly validates comment length
- Uses transactions effectively
- Implements retry mechanism for database locks
- Handles both new comments and updates
477-580: LGTM! Comprehensive review processing.The
processReviewfunction:
- Properly handles lock state changes
- Validates review status
- Prevents marking locked detections as false positive
- Uses transactions effectively
- Includes detailed debug logging
582-651: LGTM! Well-organized review handler.The
ReviewDetectionhandler:
- Separates concerns into helper functions
- Provides clear error messages
- Includes comprehensive debug logging
- Properly triggers UI refresh
…ogging - Added retry mechanism for LockNote and UnlockNote methods - Implemented exponential backoff for database lock contention - Added unique transaction ID for better logging and traceability - Updated LockDetection handler to use transactional retry approach - Improved error handling for database lock scenarios - Added logging for retry attempts and successful transactions
- Optimized GetNoteLock and IsNoteLocked methods in datastore - Added explicit lock existence check in UnlockNote method - Simplified LockDetection handler logic - Enhanced error handling and logging for lock operations - Improved transaction retry mechanism for database interactions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/httpcontroller/handlers/detections.go (3)
413-475: Improve error handling in processComment function.The function has good retry logic for database locks, but there are a few areas that could be improved:
- The comment length validation should be moved to a constant.
- The error messages could be more descriptive.
Apply this diff to improve the error handling:
+const maxCommentLength = 1000 func (h *Handlers) processComment(noteID uint, comment string, maxRetries int, baseDelay time.Duration) error { if comment == "" { return nil } // Validate comment - if len(comment) > 1000 { + if len(comment) > maxCommentLength { h.SSE.SendNotification(Notification{ - Message: "Comment exceeds maximum length of 1000 characters", + Message: fmt.Sprintf("Comment exceeds maximum length of %d characters", maxCommentLength), Type: "error", }) - return fmt.Errorf("comment too long") + return fmt.Errorf("comment length %d exceeds maximum of %d characters", len(comment), maxCommentLength) }
477-580: Enhance error handling and logging in processReview function.The function has good retry logic and transaction handling, but could benefit from more detailed error messages and validation.
Apply this diff to improve error handling and logging:
func (h *Handlers) processReview(noteID uint, verified string, lockDetection bool, maxRetries int, baseDelay time.Duration) error { h.Debug("processReview: Starting review process for note ID %d", noteID) h.Debug("processReview: Verified status: %s", verified) h.Debug("processReview: Lock detection: %v", lockDetection) // Validate review status if provided if verified != "" && verified != "correct" && verified != "false_positive" { - return fmt.Errorf("invalid verification status") + return fmt.Errorf("invalid verification status: %s (must be 'correct' or 'false_positive')", verified) } var lastErr error for attempt := 0; attempt < maxRetries; attempt++ { err := h.DS.Transaction(func(tx *gorm.DB) error { // Get note and existing review in a transaction note, err := h.DS.Get(strconv.FormatUint(uint64(noteID), 10)) if err != nil { h.Debug("processReview: Failed to retrieve note: %v", err) - return fmt.Errorf("failed to retrieve note: %w", err) + return fmt.Errorf("failed to retrieve note %d: %w", noteID, err) }
Line range hint
653-743: Enhance error handling in LockDetection function.The function has good retry logic but could benefit from more detailed error messages and validation.
Apply this diff to improve error handling:
func (h *Handlers) LockDetection(c echo.Context) error { id := c.QueryParam("id") if id == "" { + const msg = "Missing detection ID" h.SSE.SendNotification(Notification{ - Message: "Missing detection ID", + Message: msg, Type: "error", }) - return h.NewHandlerError(fmt.Errorf("no ID provided"), "Missing detection ID", http.StatusBadRequest) + return h.NewHandlerError(fmt.Errorf("no ID provided"), msg, http.StatusBadRequest) } // Get the current note first to have the species name for messages note, err := h.DS.Get(id) if err != nil { + const msg = "Failed to get detection" h.SSE.SendNotification(Notification{ - Message: fmt.Sprintf("Failed to get detection: %v", err), + Message: fmt.Sprintf("%s: %v", msg, err), Type: "error", }) - return h.NewHandlerError(err, "Failed to get detection", http.StatusInternalServerError) + return h.NewHandlerError(err, msg, http.StatusInternalServerError) }internal/datastore/interfaces.go (1)
735-868: Enhance error handling and retry logic in lock-related methods.The lock-related methods have good retry logic but could benefit from more detailed error messages and consistent error handling.
Apply this diff to improve error handling:
func (ds *DataStore) GetNoteLock(noteID string) (*NoteLock, error) { id, err := strconv.ParseUint(noteID, 10, 32) if err != nil { - return nil, fmt.Errorf("invalid note ID: %w", err) + return nil, fmt.Errorf("invalid note ID %q: %w", noteID, err) } var lock NoteLock // Check if the lock exists and get its details in one query err = ds.DB.Where("note_id = ?", id).First(&lock).Error if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return nil, nil // Return nil if no lock exists } - return nil, fmt.Errorf("error getting lock details: %w", err) + return nil, fmt.Errorf("error getting lock details for note %d: %w", id, err) } return &lock, nil } func (ds *DataStore) IsNoteLocked(noteID string) (bool, error) { id, err := strconv.ParseUint(noteID, 10, 32) if err != nil { - return false, fmt.Errorf("invalid note ID: %w", err) + return false, fmt.Errorf("invalid note ID %q: %w", noteID, err) } var count int64 err = ds.DB.Model(&NoteLock{}). Where("note_id = ?", id). Count(&count). Error if err != nil { - return false, fmt.Errorf("error checking lock status: %w", err) + return false, fmt.Errorf("error checking lock status for note %d: %w", id, err) } return count > 0, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/datastore/interfaces.go(10 hunks)internal/httpcontroller/handlers/detections.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/httpcontroller/handlers/detections.go (1)
582-651: LGTM! The ReviewDetection function is well-structured.The function effectively delegates tasks to specialized helper functions and includes comprehensive debug logging.
internal/datastore/interfaces.go (4)
50-54: LGTM! The interface changes are well-defined.The new lock management methods are clearly named and follow the existing interface pattern.
Line range hint
169-183: LGTM! The Get method changes properly handle lock preloading.The changes correctly preload the Lock relationship and populate the virtual Locked field.
195-203: LGTM! The Delete method properly checks lock status.The changes prevent deletion of locked notes, maintaining data integrity.
301-305: LGTM! The GetClipsQualifyingForRemoval method properly excludes locked notes.The changes ensure that locked notes are not considered for removal during cleanup.
Also applies to: 314-316
This PR adds functionality to lock important detections to prevent them from being deleted during regular cleanup operations.