delete button#112
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a delete button feature to the visualization feed page, allowing users to remove visualizations from both the database and the cached views. The UI is enhanced with a tabbed interface that separates the View and Code display.
Changes:
- Added
on_deletefunction to handle deletion of visualizations from database and cache - Introduced a delete button with danger styling for each visualization
- Enhanced UI with tabs separating View (iframe) and Code (CodeEditor) display
- Adjusted iframe height from 200px to 300px to accommodate the new tabbed layout
| def on_delete(snippet_id): | ||
| """Handle deletion of a visualization.""" | ||
| # Delete from database | ||
| get_db().delete_snippet(snippet_id) | ||
| # Remove from cache | ||
| if snippet_id in pn.state.cache["views"]: | ||
| del pn.state.cache["views"][snippet_id] | ||
| # Refresh feed | ||
| update_chat() |
There was a problem hiding this comment.
The on_delete function does not handle potential errors from the database deletion operation. If get_db().delete_snippet() fails or raises an exception, the cache could be left in an inconsistent state (the cache entry might be deleted while the database record remains). Consider wrapping the deletion logic in a try-except block and only clearing the cache if the database deletion succeeds. The delete_snippet method returns a bool indicating success, which should be checked before modifying the cache.
| def on_delete(snippet_id): | ||
| """Handle deletion of a visualization.""" | ||
| # Delete from database | ||
| get_db().delete_snippet(snippet_id) | ||
| # Remove from cache | ||
| if snippet_id in pn.state.cache["views"]: | ||
| del pn.state.cache["views"][snippet_id] | ||
| # Refresh feed | ||
| update_chat() |
There was a problem hiding this comment.
There is a potential race condition between the on_delete function and the update_chat periodic callback (which runs every 1 second per line 125). When a snippet is deleted, update_chat() is called to refresh the feed. However, the periodic callback could also be executing simultaneously, potentially attempting to recreate the deleted view from the database before the deletion completes or while cache is being modified. Consider adding synchronization or checking if snippets exist in the database before caching their views.
| def on_delete(snippet_id): | ||
| """Handle deletion of a visualization.""" | ||
| # Delete from database | ||
| get_db().delete_snippet(snippet_id) | ||
| # Remove from cache | ||
| if snippet_id in pn.state.cache["views"]: | ||
| del pn.state.cache["views"][snippet_id] | ||
| # Refresh feed | ||
| update_chat() |
There was a problem hiding this comment.
The new delete functionality (on_delete function and delete button) lacks test coverage. While there are tests for the underlying database.delete_snippet method, there are no tests verifying the feed_page delete behavior, including cache invalidation and feed refresh logic. Consider adding tests that verify: 1) successful deletion removes items from both database and cache, 2) the feed is properly refreshed after deletion, and 3) error cases are handled gracefully.
No description provided.