Delete multiple datasets at once #9107
Conversation
📝 WalkthroughWalkthroughAdds a bulk dataset deletion workflow in the folder sidebar (frontend mutation, per-item progress, confirmation modal), relaxes frontend Delete-menu visibility checks, removes an organization-admin authorization check in the backend delete controller, updates access-rights docs, and adds an unreleased changelog entry for multi-dataset deletion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (2)
300-303: Remove unnecessary whitespace.Line 301 has
{" "}which is unnecessary since the Button component handles spacing between icon and text internally.Apply this diff:
- <Button onClick={() => setShowConfirmDeleteModal(true)}> - {" "} - <DeleteOutlined /> Delete {selectedDatasets.length} datasets - </Button> + <Button + icon={<DeleteOutlined />} + onClick={() => setShowConfirmDeleteModal(true)} + > + Delete {selectedDatasets.length} datasets + </Button>
243-253: Consider parallel deletion for better performance.Sequential deletion may be slow for large selections. Once the API call is implemented, consider using
Promise.allSettledto delete datasets in parallel (respecting reasonable concurrency limits to avoid overloading the backend).Example approach:
const results = await Promise.allSettled( selectedDatasets.map(dataset => deleteDatasetOnDisk(dataset.id)) ); results.forEach((result, i) => { if (result.status === 'rejected') { Toast.error(`Failed to delete dataset ${selectedDatasets[i].name}.`); } setProgressInPercent(Math.round(((i + 1) / selectedDatasets.length) * 100)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx(3 hunks)
⏰ 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). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (3)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (3)
3-3: LGTM!The new imports are appropriate for the bulk delete functionality being added.
Also applies to: 12-12, 17-17, 19-19
227-229: LGTM!The state management setup appropriately tracks the deletion flow and modal visibility.
265-268: Ensure soft-delete implementation is tracked.The TODO indicates that this hard-delete warning is temporary until soft-delete is implemented. Ensure this is tracked in issue #9061 (mentioned in PR objectives) or in the PR's TODO checklist.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (2)
frontend/javascripts/types/api_types.ts (1)
APIDatasetCompact(271-276)frontend/javascripts/admin/rest_api.ts (1)
deleteDatasetOnDisk(1387-1391)
⏰ 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). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (3)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (3)
3-3: LGTM! Imports correctly support the new deletion functionality.All necessary dependencies for the bulk delete feature are properly imported.
Also applies to: 10-12, 19-19
227-230: LGTM! State management is properly initialized.The state variables correctly support the deletion flow, progress tracking, and modal visibility.
266-320: LGTM! Modal and UI implementation is well-structured.The deletion confirmation flow properly:
- Lists datasets to be deleted for user verification
- Shows progress during deletion with a progress bar
- Prevents modal closure during deletion
- Includes clear warning about the action being non-reversible
- Uses appropriate Ant Design components and patterns
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
278-312: Consider adding real cancellation support for long-running deletions
onCancelignores requests whileisDeletingis true, and the modal footer is removed in that state, so users can neither close the dialog nor abort the loop once deletion has started. For large selections this can feel like being “trapped” until all requests finish or time out.A lightweight improvement would be to introduce a cancellation flag checked inside the loop, e.g.:
const [isDeleting, setIsDeleting] = useState(false); +const [isCancelled, setIsCancelled] = useState(false); ... const deleteDatasets = async () => { setIsDeleting(true); - const deletedIds: string[] = []; + setIsCancelled(false); + const deletedIds: string[] = []; for (let i = 0; i < deletableDatasets.length; i++) { + if (isCancelled) { + break; + } const dataset = deletableDatasets[i]; ... } ... }; const onCancel = () => { - if (!isDeleting) { - setShowConfirmDeleteModal(false); - } + if (!isDeleting) { + setShowConfirmDeleteModal(false); + } else { + setIsCancelled(true); + } };This preserves the invariant that you can’t close the modal immediately while requests are in flight, but it lets users stop further deletions in a large batch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (2)
frontend/javascripts/types/api_types.ts (1)
APIDatasetCompact(271-276)frontend/javascripts/admin/rest_api.ts (1)
deleteDatasetOnDisk(1387-1391)
⏰ 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). (3)
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (2)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (2)
3-19: Imports and hooks align with new bulk-delete behaviourNew imports (
DeleteOutlined,useQueryClient,deleteDatasetOnDisk, extra antd components,useState) are all used correctly inDatasetsDetailsand keep dependencies localized; no issues here.
227-246: Cross-folder cache updates look correctThe
invalidateQuerieshelper now derivesuniqueFolderIdsfromselectedDatasetsand filters outdeletedIdsper["datasetsByFolder", folderId]entry, plus invalidates the dataset search queries. This addresses the earlier cross-folder stale-cache problem and keeps folder and search views consistent after multi-folder deletions.
| you do not have sufficient permissions. | ||
| </div> | ||
| )} | ||
| {cantBeUndoneMessage} |
There was a problem hiding this comment.
maybe inline that warning. There is nothing special about it to move it into a separate variable.
There was a problem hiding this comment.
I thought it would be removed soon anyway, with the trash implemented in #9061, and I thought it would be cleaner if one can just remove the variable
| <Modal | ||
| open={showConfirmDeleteModal} | ||
| title="Delete Datasets" | ||
| footer={isDeleting ? null : [cancelButton, okayButton]} |
There was a problem hiding this comment.
Instead of defining a custom footer for your two button, I would use the onOK, okType, okText, okButtonProps cancelText,onCancel props of the modal for the simple styling that you are doing anyway.
There was a problem hiding this comment.
right, I know its super basic styling but I want to hide the footer when the progress bar is shown. I think this is only possible with the custom footer (at least thats whats recommended in the docs)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
249-273: Consider tracking progress by attempts rather than successes.Currently,
setProgressInPercent(line 257) is inside thetryblock, so progress only advances for successful deletions. If several deletions fail consecutively, the progress bar may appear stuck even though the loop is actively processing items.Move the progress update outside the
tryblock to reflect overall processing progress:try { await deleteDatasetOnDisk(dataset.id); deletedIds.push(dataset.id); - setProgressInPercent(Math.round(((i + 1) / datasets.length) * 100)); } catch (_e) { Toast.error(`Failed to delete dataset ${dataset.name}.`); } + setProgressInPercent(Math.round(((i + 1) / datasets.length) * 100)); }Additionally, consider logging the caught error for debugging:
- } catch (_e) { + } catch (error) { Toast.error(`Failed to delete dataset ${dataset.name}.`); + console.error("Dataset deletion failed:", error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/dashboard/folders/details_sidebar.tsx
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (3)
frontend/javascripts/types/api_types.ts (1)
APIDatasetCompact(271-276)frontend/javascripts/admin/rest_api.ts (1)
deleteDatasetOnDisk(1387-1391)frontend/javascripts/libs/utils.ts (1)
pluralize(1171-1179)
⏰ 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). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (5)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (5)
3-3: LGTM! Imports are appropriate.All new imports are necessary for the bulk deletion functionality and are correctly used throughout the implementation.
Also applies to: 10-12, 19-19
227-231: LGTM! State and filtering logic correctly implemented.The component properly separates deletable from undeletable datasets based on the
isEditableproperty, ensuring permissions are respected throughout the deletion flow.
233-247: LGTM! Cache invalidation correctly handles cross-folder deletions.The logic properly updates all affected folder caches and invalidates search results. Only successfully deleted datasets (those in
deletedIds) are removed from the cache, ensuring correctness even when some deletions fail.
275-328: LGTM! Modal and confirmation flow well-implemented.The confirmation modal provides clear feedback with:
- Progress tracking during deletion
- Explicit list of datasets to be deleted (only deletable ones)
- Warning when some selected datasets cannot be deleted due to permissions
- Proper button states and modal behavior
The implementation correctly operates on
deletableDatasetsthroughout, ensuring the UI messaging matches the actual deletion behavior.
330-345: LGTM! Render logic is clean and user-friendly.The UI correctly displays the delete action only when there are deletable datasets and uses the appropriate icon prop for the button. Selection state is automatically managed—when datasets are deleted and the cache is invalidated, the parent component's
useEffectsyncs selected datasets against the fresh data, automatically filtering out any deleted items.
|
@hotzenklotz thanks for the quick review! I incorporated the feedback but left two threads open because I wanted to explain my train of thought. |
| </Modal> | ||
| ); | ||
|
|
||
| const showDeleteDatasetsButton = isUserAdmin && mayDeleteDatasets && selectedDatasets.length > 0; |
There was a problem hiding this comment.
See discussion about permissions rights: https://scm.slack.com/archives/C5AKLAV0B/p1764759287923069
hotzenklotz
left a comment
hotzenklotz
left a comment
There was a problem hiding this comment.
Your changes LGTM.
Please see the discussion around user permissions in Slack and update the PR and docs accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/users/access_rights.md(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(1 hunks)frontend/javascripts/dashboard/folders/details_sidebar.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/dashboard/folders/details_sidebar.tsx
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (3)
frontend/javascripts/types/api_types.ts (1)
APIDatasetCompact(271-276)frontend/javascripts/admin/rest_api.ts (1)
deleteDatasetOnDisk(1387-1391)frontend/javascripts/libs/utils.ts (1)
pluralize(1171-1179)
🪛 markdownlint-cli2 (0.18.1)
docs/users/access_rights.md
34-34: Hard tabs
Column: 54
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 63
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 82
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 98
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 113
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 54
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 63
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 82
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 98
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 113
(MD010, no-hard-tabs)
⏰ 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). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (7)
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx (1)
64-67: LGTM - Text accurately reflects expanded permissions.The updated text correctly documents that admins, dataset managers, and team managers can delete datasets, aligning with the access rights documentation changes.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
154-154: LGTM - Menu visibility broadened while maintaining backend permission checks.Removing the admin-only check allows the delete menu item to appear for all users when the feature flag is enabled. The actual deletion capability remains protected by backend permissions (the
isEditableproperty), ensuring users without sufficient privileges cannot delete datasets even if they can see the menu item.frontend/javascripts/dashboard/folders/details_sidebar.tsx (5)
2-2: LGTM - Imports support the new deletion workflow.All added imports are necessary for implementing the bulk dataset deletion feature with progress tracking, modal UI, and cache invalidation.
Also applies to: 9-11, 16-16, 19-19
219-223: LGTM - Proper separation of deletable vs. undeletable datasets.The code correctly filters datasets by
isEditableto identify which ones can be deleted, preventing unauthorized deletion attempts and enabling clear user feedback about mixed selections.
225-239: LGTM - Correctly handles multi-folder cache invalidation.The function properly updates caches for all affected folders by computing unique folder IDs and filtering deleted datasets from each folder's query data. This addresses the cross-folder deletion scenario mentioned in previous reviews.
241-265: LGTM - Well-structured mutation with proper error handling.The deletion mutation correctly:
- Uses
useMutationfor state management and lifecycle handling- Explicitly types
deletedIdsasstring[]- Handles individual deletion failures gracefully with error toasts while continuing the loop
- Provides per-item progress updates
- Cleans up state and invalidates queries in the
onSuccesscallback
267-322: LGTM - Modal and UI implementation follow best practices.The modal and UI components are well-implemented:
- Modal footer is properly hidden during deletion to prevent premature closure
- Warning message uses
Typography.Textfor theme compatibility- Button icon is passed via the
iconprop for proper spacing- Clear distinction between deletable and undeletable datasets in the UI
- Appropriate user feedback throughout the deletion process
Also applies to: 324-339
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
273-323: Clarify modal behavior while deletion is pending (closing vs. cancelling)While
deleteDatasetsMutation.isPending, the footer is hidden andonCancelignores cancel attempts, but the default close controls (close icon / mask click) still invokeonCancel, which then does nothing. This can feel like a broken close button with no feedback, and there’s still no way to cancel the remaining deletions.Two possible improvements:
- If deletions should not be cancellable: disable closing while pending (e.g.,
closable={!isPending},maskClosable={!isPending}) so the user is clearly locked in until completion.- If you want cancellability: introduce a cancellation flag checked inside
mutationFnand letonCancelset it, then close the modal once the loop stops.Either approach would make the UX more explicit and avoid “dead” close actions during a pending mutation.
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
225-239: Make cache invalidation independent of current selection to avoid rare stale-folder cases
invalidateQueriesderivesuniqueFolderIdsfromdeletableDatasets, which reflects the current selection, not necessarily the datasets that were actually passed tomutateand deleted. If the selection changes while the mutation is running, some folders of deleted datasets might not be updated.Consider basing folder IDs on the datasets passed into the mutation (e.g., have
mutationFnreturn bothdeletedIdsand the set of affected folder IDs, or return{ deletedIds, datasets }and compute folder IDs from that inonSuccess) so cache updates are always derived from the actual deletion input, independent of later selection changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-04T13:41:44.912Z
Learnt from: hotzenklotz
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/viewer/view/nml_upload_zone_container.tsx:186-186
Timestamp: 2025-12-04T13:41:44.912Z
Learning: In Ant Design v6, the Alert component's `message` prop has been deprecated and replaced by `title`. When migrating from v5 to v6, Alert components should use `title` for the main content (and optionally `description` for additional details).
Applied to files:
frontend/javascripts/dashboard/folders/details_sidebar.tsx
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/dashboard/folders/details_sidebar.tsx
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (3)
frontend/javascripts/types/api_types.ts (1)
APIDatasetCompact(271-276)frontend/javascripts/admin/rest_api.ts (1)
deleteDatasetOnDisk(1387-1391)frontend/javascripts/libs/utils.ts (1)
pluralize(1171-1179)
⏰ 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). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (1)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
219-271: Bulk delete flow and permission handling look solidThe overall mutation setup (per-item delete loop, progress tracking, success/error toasts) and the use of
deletableDatasetsthroughout the UI and mutation entry point align well with the permissions model and earlier feedback. No blocking issues here.
|
todo: fix bug where there is an endless redirect after deleting all datasets in a folder |
| const deletableDatasets = selectedDatasets.filter((ds) => ds.isEditable); | ||
| const numberOfUndeletableDatasets = selectedDatasets.length - deletableDatasets.length; | ||
|
|
||
| const invalidateQueries = (deletedIds: string[]) => { |
There was a problem hiding this comment.
updateAndInv
There was a problem hiding this comment.
- test last commit
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (1)
1-1: Folder error handling + MRU clearing look good; minor note on effect depsThe new flow around
mostRecentlyUsedActiveFolderIdand query invalidation is sensible:
- Tracking
mostRecentlyUsedActiveFolderIdviausePrevious(activeFolderId, true)preserves the last non-null folder id, and- Clearing it in the error branch (
didFolderLoadingError) together withsetActiveFolderId(null)avoids keeping around an invalid folder id and should help with redirect/selection edge cases.- Guarding
queryClient.invalidateQueries({ queryKey: ["folders"] })behind!folderHierarchyQuery.isFetchingis a reasonable way to avoid redundant refreshes.One small refinement:
clearMostRecentlyUsedActiveFolderIdis currently recreated on every render byusePrevious, so including it in the effect dependency array effectively makes this effect run on every render (repeating the localStorage set/remove work even whenactiveFolderId/folder/didFolderLoadingErrorhaven’t changed).- Once
usePreviousmemoizes itsclearFn(e.g., viauseCallbackwith a stable ref, similar touseIsMountedinlibs/react_hooks.ts), this effect will naturally only re-run when the relevant state changes.Also, please double‑check that the
"folders"query key here matches the key used byuseFolderHierarchyQuery(and related folder queries) so the invalidation hits the intended cache entries.Also applies to: 84-88, 101-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx(4 hunks)frontend/javascripts/libs/react_hooks.ts(2 hunks)frontend/javascripts/libs/toast.tsx(1 hunks)frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-11T15:25:53.526Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/admin/statistic/time_tracking_overview.tsx:261-279
Timestamp: 2025-12-11T15:25:53.526Z
Learning: Ant Design v6 Select: when using the Select component, consider supplying a prefix prop to render an icon or element before the input for better visual context. Apply this guideline to TS and TSX files across the codebase where Ant Design Select is used; ensure prefix usage is accessible (e.g., provide meaningful aria-label if needed) and avoid unnecessary prefixes on simple inputs.
Applied to files:
frontend/javascripts/libs/toast.tsxfrontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsxfrontend/javascripts/libs/react_hooks.tsfrontend/javascripts/dashboard/dataset/dataset_collection_context.tsx
📚 Learning: 2025-12-11T15:33:06.880Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/admin/task/task_search_form.tsx:151-151
Timestamp: 2025-12-11T15:33:06.880Z
Learning: In Ant Design v6, do not use optionFilterProp as a standalone prop on Select. Instead, pass it inside showSearch as optionFilterProp, e.g. showSearch={{ optionFilterProp: 'label' }}. showSearch accepts boolean or an object with keys like optionFilterProp, filterOption, autoClearSearchValue. Update all Select components that use optionFilterProp to adopt the new pattern and adjust types accordingly to maintain compatibility and avoid deprecation warnings.
Applied to files:
frontend/javascripts/libs/toast.tsxfrontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsxfrontend/javascripts/dashboard/dataset/dataset_collection_context.tsx
📚 Learning: 2025-12-11T15:54:47.778Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:131-134
Timestamp: 2025-12-11T15:54:47.778Z
Learning: In Ant Design v6, Dropdown uses a flat classNames structure: classNames={{ root: '…' }}. Other components (Select, AutoComplete, Cascader, TreeSelect) use a nested structure. The deprecated overlayClassName prop for Dropdown should be replaced with classNames.root. In reviews, flag Dropdown usage that relies on overlayClassName and replace it with classNames={{ root: '…' }}. If you encounter related components, verify the correct classNames shape (flat for Dropdown, nested for others) and update accordingly. This guideline covers TSX files under the frontend codebase where Ant Design components are used.
Applied to files:
frontend/javascripts/libs/toast.tsxfrontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsxfrontend/javascripts/dashboard/dataset/dataset_collection_context.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx
📚 Learning: 2025-02-05T20:16:45.712Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
Applied to files:
frontend/javascripts/libs/react_hooks.ts
🧬 Code graph analysis (2)
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
usePrevious(15-37)
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
usePrevious(15-37)
⏰ 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). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/libs/toast.tsx (1)
77-77: Verify that changing error toasts from sticky to auto-dismiss is intentional for bulk operations.The change on line 77 replaces
sticky: truewithtimeout: 13000, making error toasts auto-dismiss after 13 seconds instead of persisting until manually dismissed. While 13 seconds is longer than the default 6-second timeout and the implementation includes a sophisticated manual timeout mechanism that intelligently pauses during browser tab inactivity, this is still a behavioral change with UX implications.In bulk deletion scenarios where multiple errors could occur, users previously had persistent error messages to review. Confirm that the shift to auto-dismissing toasts aligns with the expected user experience for this feature.
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx (1)
115-116: usePrevious tuple adaptation looks correctDestructuring only the first element from
usePreviousfor the modifier flags keeps the previous-value semantics unchanged and correctly ignores the newclearFnin this context. No issues from a logic or typing perspective.
| export function usePrevious<T>( | ||
| value: T, | ||
| ignoreNullAndUndefined: boolean = false, | ||
| ): [T | null, () => void] { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat frontend/javascripts/libs/react_hooks.tsRepository: scalableminds/webknossos
Length of output: 10691
🏁 Script executed:
rg -n "usePrevious\(" frontend/javascripts -A 2Repository: scalableminds/webknossos
Length of output: 994
🏁 Script executed:
rg -n "clearMostRecentlyUsedActiveFolderId" frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx -A 5 -B 2Repository: scalableminds/webknossos
Length of output: 929
New usePrevious API is fine functionally; consider memoizing clearFn and updating docs
The new API [previousValue, clearFn] keeps the previous‑value semantics intact and exposes a useful reset hook, but there are two small improvements worth making:
-
Stabilize
clearFn's identityRight now
clearFnis recreated on every render. Since it's included in the dependency array of the effect inDatasetCollectionContextProvider(line 133 ofdataset_collection_context.tsx), this causes that effect to re-run on every render, unnecessarily re-touchinglocalStorage.You can make
clearFnstable, matching the pattern already used inuseIsMounted, by wrapping it inuseCallback:- const clearFn = () => { - ref.current = null; - }; + const clearFn = useCallback(() => { + ref.current = null; + }, []);
-
Update the JSDoc return description
The doc still says
@returns The previous value, or null if no previous value exists, but the function now returns a tuple[previousValue, clearFn]. Update it to reflect the new signature, e.g., "Returns a tuple[previousValue, clearFn], wherepreviousValueis the value from the previous render (ornullif none) andclearFnresets that stored value."
All usePrevious call sites are already correctly destructuring the tuple.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/libs/react_hooks.ts around lines 15 to 18, the newly
exposed clearFn is recreated every render which destabilizes effects that depend
on it; wrap clearFn in useCallback so its identity is stable across renders
(matching the useIsMounted pattern) and update the JSDoc @returns to describe
the new tuple return type, e.g. "Returns a tuple [previousValue, clearFn], where
previousValue is the value from the previous render (or null if none) and
clearFn resets that stored value."
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)