道のbboxによる削除#16
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a bulk delete feature for paths using rectangle selection on an interactive map. Users can draw a bounding box on the map to select and delete multiple paths within that area.
- Adds rectangle selection UI with crosshair cursor and visual feedback
- Implements backend API endpoint for bulk deletion within a bounding box
- Integrates auto-generated API client code for the new endpoint
- Adds development dependency ipykernel for Jupyter notebook support
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/MapTerrain.tsx | Adds rectangle selection mode with mouse interaction handlers and visual overlay |
| frontend/src/components/Map.tsx | Passes through onDeletePaths callback prop |
| frontend/src/app/page.tsx | Implements handleDeletePaths handler with confirmation dialog and data refresh |
| frontend/src/app/api/lib/paths/paths.ts | Adds auto-generated API client function for bulk delete endpoint |
| frontend/src/app/api/lib/models/* | Adds auto-generated TypeScript type definitions for bulk delete request/response |
| backend/paths/views.py | Implements bulk_delete action to delete paths within bounding box |
| backend/docs/openapi.yaml | Documents the new bulk delete API endpoint |
| backend/pyproject.toml | Adds ipykernel dependency |
| backend/uv.lock | Updates lock file with ipykernel and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const pathsBulkDeleteCreate = async ( | ||
| path: NonReadonly<Path>, | ||
| params: PathsBulkDeleteCreateParams, | ||
| options?: RequestInit, | ||
| ): Promise<pathsBulkDeleteCreateResponse> => { | ||
| return customFetch<pathsBulkDeleteCreateResponse>( | ||
| getPathsBulkDeleteCreateUrl(params), | ||
| { | ||
| ...options, | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json", ...options?.headers }, | ||
| body: JSON.stringify(path), | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The auto-generated function signature requires a path: NonReadonly<Path> parameter that is sent as request body, but the backend endpoint doesn't use the request body (it only uses query parameters). This mismatch forces callers to pass a dummy object (e.g., {} as any on line 126 of page.tsx). Consider regenerating the API client with correct specification or documenting this workaround more prominently.
| if (onDeletePaths) { | ||
| const confirmed = window.confirm( | ||
| `選択した範囲内のパスを削除しますか？\n範囲: (${minLat.toFixed(4)}, ${minLon.toFixed(4)}) - (${maxLat.toFixed(4)}, ${maxLon.toFixed(4)})`, | ||
| ); | ||
|
|
||
| if (confirmed) { | ||
| try { | ||
| await onDeletePaths({ minLat, minLon, maxLat, maxLon }); | ||
| alert("パスの削除が完了しました"); | ||
| } catch (error) { | ||
| console.error("削除エラー:", error); | ||
| alert("パスの削除に失敗しました"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] Using window.confirm() and window.alert() for delete confirmations and feedback blocks the UI thread and provides a poor user experience. Consider using a custom modal dialog component with proper styling and async handling that matches the application's design system.
| search_bbox.srid = 4326 | ||
|
|
||
| queryset = self.get_queryset().filter(bbox__intersects=search_bbox) | ||
| deleted_count, _ = queryset.delete() |
There was a problem hiding this comment.
The bulk delete endpoint lacks authentication and authorization checks. Any user can delete paths without permission verification. Add appropriate permission classes (e.g., IsAuthenticated, IsAdminUser) or implement custom permission logic to restrict this destructive operation.
| const handleMouseUp = useCallback(async () => { | ||
| if (!isDrawing || !startPoint || !currentPoint || !map.current) { | ||
| setIsDrawing(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[nitpick] The handleMouseUp function handles UI state cleanup, coordinate transformation, and API calls all in one callback. Consider extracting the deletion logic into a separate function to improve testability and separation of concerns.
| "inflection>=0.5.1", | ||
| "uritemplate>=4.1.1", | ||
| "drf-spectacular>=0.28.0", | ||
| "ipykernel>=7.1.0", |
There was a problem hiding this comment.
Adding ipykernel as a production dependency is unusual. This package is typically used for Jupyter notebook development and shouldn't be required for the Django application runtime. Consider moving this to a development dependencies section or removing it if not needed.
| "ipykernel>=7.1.0", |
| ); | ||
|
|
||
| // マウスアップ: 矩形選択完了 | ||
| const handleMouseUp = useCallback(async () => { |
There was a problem hiding this comment.
The handleMouseUp callback is async but multiple rapid clicks could trigger overlapping delete operations. Consider adding a loading state to prevent concurrent deletion requests while one is already in progress.
No description provided.