Show used storage in dashboard dataset list #9025
Conversation
📝 WalkthroughWalkthroughAdds per-dataset storage accounting: backend computes and conditionally returns Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
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 |
fm3
changed the title
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/postgres/dbtool.js (1)
423-439: Make organization reset configurable.
Hard-coding_id = 'sample_organization'means this command no-ops on instances whose primary organization uses a different id (typical outside the seeded demo data), so administrators still need to wait for the next scheduler tick to kick off the scan. Please either target all organizations or accept the organization id (or name) as a parameter so the command behaves consistently across environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/DatasetController.scala(1 hunks)app/controllers/WKRemoteDataStoreController.scala(1 hunks)app/models/dataset/Dataset.scala(6 hunks)app/models/dataset/DatasetService.scala(1 hunks)frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx(7 hunks)frontend/javascripts/types/api_types.ts(2 hunks)package.json(1 hunks)tools/postgres/dbtool.js(1 hunks)unreleased_changes/9025.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-12T13:07:29.637Z
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
Applied to files:
app/controllers/DatasetController.scalaapp/models/dataset/Dataset.scala
🧬 Code graph analysis (4)
app/controllers/WKRemoteDataStoreController.scala (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/UnfinishedUpload.scala (2)
UnfinishedUpload(7-16)UnfinishedUpload(18-20)app/models/dataset/Dataset.scala (1)
dataSourceId(94-94)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (2)
frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/libs/format_utils.ts (1)
formatCountToDataAmountUnit(333-346)
app/models/dataset/DatasetService.scala (1)
util/src/main/scala/com/scalableminds/util/time/Instant.scala (1)
now(48-48)
app/models/dataset/Dataset.scala (1)
app/utils/sql/SqlEscaping.scala (1)
parseArrayLiteral(28-46)
⏰ 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 (5)
app/controllers/WKRemoteDataStoreController.scala (1)
113-119: Good stylistic improvement!Using the
applymethod withoutnewfor case class instantiation is idiomatic Scala and improves readability.package.json (1)
107-108: New script fits nicely.
Registering the CLI entry point here keeps local workflows aligned with the dbtool addition.app/models/dataset/Dataset.scala (1)
92-348: Storage exposure logic looks solid.
The newusedStorageBytesfield plus the conditionalOptionwrapping ensure per-dataset storage is available while still respecting organization boundaries and hiding non-positive values.unreleased_changes/9025.md (1)
1-2: Changelog entry captured.
Thanks for documenting the dashboard-facing change.frontend/javascripts/types/api_types.ts (1)
269-300: API surface stays in sync.
PullingusedStorageBytesinto the compact types and converter keeps the frontend aligned with the backend payload.
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (1)
425-425: This issue was already flagged in a previous review.The cached
isUserAdminOrDatasetManagerfield becomes stale when props update, affecting both dataset filtering (line 478) and the visibility of the storage column (line 625). See the previous review comment for the recommended fix using a getter.Also applies to: 478-478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx(9 hunks)frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (2)
frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/types/globals.d.ts (1)
EmptyObject(31-31)
⏰ 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 (5)
frontend/javascripts/dashboard/dataset/dataset_collection_context.tsx (1)
3-3: LGTM!The storage usage data is correctly retrieved from Redux state and exposed through context. The optional chaining handles the case where
activeOrganizationmight be undefined, and the value is properly included in theuseMemodependency array.Also applies to: 58-58, 88-88, 259-259, 285-285
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (4)
11-11: LGTM!The imports for
FastTooltipandformatCountToDataAmountUnitare correctly added to support the storage column rendering.Also applies to: 26-26
399-401: LGTM!Returning
nullfor folder rows is appropriate since storage metrics only apply to datasets.
625-641: Storage column implementation is sound, but depends on stale cached value.The conditional column logic is well-structured with appropriate sorting and rendering. However, it relies on
this.isUserAdminOrDatasetManagerwhich is stale (see earlier comment at line 425). Once that issue is fixed, this column will display correctly for all authorized users.
655-655: Good fix!Correctly changed from an empty object
{}to an empty function() => {}to match the expected function type for theeditFolderprop.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (1)
425-425: Compute admin/dataset-manager flag from props each time.
isUserAdminOrDatasetManageris captured once during construction, so whenisUserAdmin/isUserDatasetManagerlater update (e.g., after the current user is fetched), filtering and the Size column keep using the stale value and newly privileged users still can't see storage until the component remounts. Derive the flag fromthis.propson every access instead of caching it.Apply this diff:
- isUserAdminOrDatasetManager: boolean = this.props.isUserAdmin || this.props.isUserDatasetManager; + get isUserAdminOrDatasetManager(): boolean { + return this.props.isUserAdmin || this.props.isUserDatasetManager; + }
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (1)
625-641: Storage column implementation looks good.The conditional column insertion and rendering logic is well-structured. The column is properly positioned and includes appropriate sorting.
Consider treating
nullstorage values distinctly in sorting rather than defaulting to 0. Currently, datasets without storage data sort as if they have 0 bytes, which could mix them with datasets that genuinely have 0 bytes measured. You could sortnullvalues to the end:sorter: Utils.compareBy<RowRenderer>((rowRenderer) => - isRecordADataset(rowRenderer.data) && rowRenderer.data.usedStorageBytes - ? rowRenderer.data.usedStorageBytes - : 0, + isRecordADataset(rowRenderer.data) && rowRenderer.data.usedStorageBytes != null + ? rowRenderer.data.usedStorageBytes + : Number.MAX_SAFE_INTEGER, ),This would push unmeasured datasets to the bottom when sorting ascending.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (2)
frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/libs/format_utils.ts (1)
formatCountToDataAmountUnit(333-346)
⏰ 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 (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (1)
291-297: LGTM! Syntax error has been resolved.The storage column rendering logic is correct. The conditional check for
usedStorageBytesis appropriate, and the tooltip provides helpful context about measurement limitations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (1)
641-645: Consider explicit null check for clarity.The sorter condition relies on the falsy nature of
0to work correctly. While functionally correct, using an explicit!= nullcheck would make the intent clearer.Apply this diff:
sorter: Utils.compareBy<RowRenderer>((rowRenderer) => - isRecordADataset(rowRenderer.data) && rowRenderer.data.usedStorageBytes + isRecordADataset(rowRenderer.data) && rowRenderer.data.usedStorageBytes != null ? rowRenderer.data.usedStorageBytes : 0, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (2)
frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/libs/format_utils.ts (1)
formatCountToDataAmountUnit(333-346)
⏰ 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 (5)
frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx (5)
11-11: LGTM: Imports are correct.The new imports for
FastTooltipandformatCountToDataAmountUnitare properly used in the storage column rendering logic.Also applies to: 26-26
291-297: LGTM: Storage column rendering is well-implemented.The method properly handles null values, provides helpful context via the tooltip, and formats the storage value appropriately for display.
399-401: LGTM: Folder stub method is appropriate.Returning null for folders is correct since storage information only applies to datasets.
425-427: LGTM: Getter method correctly addresses previous concern.This implementation computes the admin/dataset-manager flag from current props on each access, ensuring it stays in sync when props update. This properly addresses the stale-value issue raised in previous reviews.
629-649: Conditional column implementation is solid.The storage column is appropriately gated behind admin/manager permissions and only shown when the organization has used storage. The column placement and configuration are correct.
URL of deployed dev instance (used for testing):
Steps to test:
yarn enable-storage-scan, wait for next tick (every 10 minutes, 1 minute after startup, can be changed in application.conf)TODOs:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)