Encourage coarser mags when annotating a high number of voxels #8961
Conversation
📝 WalkthroughWalkthroughAdds a configurable bucket-save warning threshold, tracks bucket updates via Redux/sagas, emits a warning action when many buckets are updated, and displays a persistent toast with suppression options; also updates docs, styles, tests, and related types/config. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ 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 |
| console.warn(warningMessage + " For more info, visit: " + linkToDocs); | ||
| } | ||
|
|
||
| console.log(`Save queue length: ${saveQueueLength}`, saveQueue); |
There was a problem hiding this comment.
remove dev change
| Store.dispatch(pushSaveQueueTransaction(items)); | ||
|
|
||
| Store.dispatch(notifyAboutUpdateBucketAction(items.length)); | ||
| console.log("notify about ", items.length, " items"); |
| console.log( | ||
| "buckets in last interval: ", | ||
| bucketsForCurrentInterval, | ||
| "currentBucketsArray: ", | ||
| currentBuckets, | ||
| "sumOfBuckets: ", | ||
| sumOfBuckets, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
frontend/javascripts/viewer/model/bucket_data_handling/pushqueue.ts (1)
158-160: Remove dev logging before merge.The dispatch of
notifyAboutUpdatedBucketsActionis correct, but the console.log on Line 159 is a dev artifact and should be removed.Apply this diff:
Store.dispatch(pushSaveQueueTransaction(items)); Store.dispatch(notifyAboutUpdatedBucketsAction(items.length)); - console.log("notify about ", items.length, " items"); this.compressingBucketCount -= batch.length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/backend-snapshot-tests/__snapshots__/misc.e2e.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
conf/application.conf(1 hunks)docs/volume_annotation/import_export.md(1 hunks)frontend/javascripts/libs/toast.tsx(2 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(2 hunks)frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts(1 hunks)frontend/javascripts/types/api_types.ts(1 hunks)frontend/javascripts/viewer/model/actions/annotation_actions.ts(2 hunks)frontend/javascripts/viewer/model/actions/save_actions.ts(3 hunks)frontend/javascripts/viewer/model/bucket_data_handling/pushqueue.ts(2 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx(2 hunks)frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx(1 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(2 hunks)frontend/stylesheets/main.less(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
frontend/javascripts/features.ts (1)
__setFeatures(9-12)
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (1)
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (1)
ManyBucketUpdatesWarning(11-99)
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (2)
frontend/javascripts/libs/react_helpers.tsx (1)
useInterval(11-34)frontend/javascripts/viewer/model/helpers/listener_helpers.ts (1)
useReduxActionListener(47-56)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (2)
frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
showManyBucketUpdatesWarningAction(379-382)frontend/javascripts/viewer/model/actions/save_actions.ts (1)
NotifyAboutUpdatedBucketsAction(17-17)
frontend/javascripts/viewer/model/bucket_data_handling/pushqueue.ts (1)
frontend/javascripts/viewer/model/actions/save_actions.ts (1)
notifyAboutUpdatedBucketsAction(66-67)
⏰ 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 (11)
conf/application.conf (1)
173-173: LGTM! Configuration value is reasonable.The threshold of 1000 buckets aligns well with the PR objectives to warn users when annotating large volumes at fine magnifications.
frontend/javascripts/test/helpers/apiHelpers.ts (2)
55-55: Good defensive test practice.Importing and using
__setFeaturesensures tests start with a clean feature state, preventing feature-driven behavior from causing test flakiness.
381-381: Correct placement for feature reset.Resetting features before
Model.fetchensures the model initializes with a clean feature configuration in tests.frontend/javascripts/libs/toast.tsx (2)
20-22: Backward-compatible toast configuration extension.The new optional properties
customFooterandclassNameextend toast capabilities without breaking existing usage.
153-155: Implementation follows existing patterns.The className defaults to an empty string when not provided, and customFooter is mapped to the btn property for antd's notification API.
docs/volume_annotation/import_export.md (1)
21-34: Excellent user-facing documentation.The new "Restricting magnifications" section provides clear, actionable guidance for users annotating large structures. The explanation of mag notation and step-by-step instructions align well with the PR's objective to encourage appropriate magnification usage.
frontend/javascripts/types/api_types.ts (1)
763-763: Type definition matches backend configuration.The new
bucketSaveWarningThresholdproperty correctly mirrors the backend configuration inconf/application.confand ensures type safety throughout the frontend.frontend/stylesheets/main.less (1)
721-725: Minimal styling for custom notification footer.The styling correctly targets the notification button to remove top margin, ensuring proper alignment of the custom footer in the bucket updates warning.
frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts (1)
315-317: Test correctly reflects new notification dispatch.The updated expectation accounts for the additional
notifyAboutUpdatedBucketsActiondispatch introduced in the push queue flow, while still verifying the original save queue transaction is dispatched with the correct payload.frontend/javascripts/viewer/model/actions/save_actions.ts (1)
17-17: LGTM! Clean action definition.The new
NotifyAboutUpdatedBucketsActionfollows the established pattern, correctly derives its type from the action creator, and integrates cleanly into theSaveActionunion.Also applies to: 35-35, 66-67
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (1)
53-53: LGTM! Appropriate component placement.The
ManyBucketUpdatesWarningcomponent is correctly placed in the header area when the tracing status is loaded, ensuring the warning system is active during annotation sessions.Also applies to: 368-368
| function* watchForNumberOfBucketsInSaveQueue(): Saga<void> { | ||
| const bucketSaveWarningThreshold = features().bucketSaveWarningThreshold; | ||
| let bucketsForCurrentInterval = 0; | ||
| let currentBuckets: Array<number> = []; | ||
| const bucketCountArrayLength = Math.floor( | ||
| CHECK_NUMBER_OF_BUCKETS_SLIDING_WINDOW / CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL, | ||
| ); | ||
| yield* call( | ||
| setInterval, | ||
| () => { | ||
| const sumOfBuckets = _.sum(currentBuckets); | ||
| console.log( | ||
| "buckets in last interval: ", | ||
| bucketsForCurrentInterval, | ||
| "currentBucketsArray: ", | ||
| currentBuckets, | ||
| "sumOfBuckets: ", | ||
| sumOfBuckets, | ||
| ); | ||
| if (sumOfBuckets > bucketSaveWarningThreshold) { | ||
| Store.dispatch(showManyBucketUpdatesWarningAction()); | ||
| } | ||
| currentBuckets.push(bucketsForCurrentInterval); | ||
| if (currentBuckets.length > bucketCountArrayLength) { | ||
| currentBuckets.shift(); | ||
| } | ||
| bucketsForCurrentInterval = 0; | ||
| }, | ||
| CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL, | ||
| ); | ||
| yield* takeEvery("NOTIFY_ABOUT_UPDATED_BUCKETS", (action: NotifyAboutUpdatedBucketsAction) => { | ||
| bucketsForCurrentInterval += action.count; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Remove dev logging before merge.
The sliding-window bucket tracking logic is sound, but the console.log statements (Lines 59-66) are dev artifacts and should be removed.
Apply this diff:
yield* call(
setInterval,
() => {
const sumOfBuckets = _.sum(currentBuckets);
- console.log(
- "buckets in last interval: ",
- bucketsForCurrentInterval,
- "currentBucketsArray: ",
- currentBuckets,
- "sumOfBuckets: ",
- sumOfBuckets,
- );
if (sumOfBuckets > bucketSaveWarningThreshold) {
Store.dispatch(showManyBucketUpdatesWarningAction());
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx around lines 48
to 81, remove the development console.log(...) call (the multi-argument logging
between lines ~59-66) so no dev logging remains; simply delete that console.log
invocation (and any leftover trailing commas or extra whitespace) and keep the
rest of the sliding-window logic intact.
| useInterval(() => { | ||
| UserLocalStorage.setItem("suppressBucketWarning", "false"); | ||
| console.log("resetting suppressBucketWarning to false every 120s for dev purposes"); | ||
| }, 120 * 1000); //TODO_C dev |
There was a problem hiding this comment.
Remove dev-only interval before merge.
The useInterval hook that periodically resets the suppression flag is a dev/testing artifact and should be removed before merging.
Apply this diff:
- useInterval(() => {
- UserLocalStorage.setItem("suppressBucketWarning", "false");
- console.log("resetting suppressBucketWarning to false every 120s for dev purposes");
- }, 120 * 1000); //TODO_C dev
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useInterval(() => { | |
| UserLocalStorage.setItem("suppressBucketWarning", "false"); | |
| console.log("resetting suppressBucketWarning to false every 120s for dev purposes"); | |
| }, 120 * 1000); //TODO_C dev |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
around lines 21 to 24, remove the dev-only useInterval that resets
UserLocalStorage.setItem("suppressBucketWarning", "false") every 120s and its
console.log/TODO comment; delete the entire interval block so the suppression
flag is only set by real user actions and not periodically overwritten for
testing, and ensure no lingering TODO/dev comments remain.
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| dontShowAgainInThisSessionRef.current = true; | ||
| } else { | ||
| console.log("suppressing warning toast"); //TODO_C dev |
There was a problem hiding this comment.
Remove dev logging before merge.
The console.log at Line 94 is a dev artifact and should be removed.
Apply this diff:
dontShowAgainInThisSessionRef.current = true;
} else {
- console.log("suppressing warning toast"); //TODO_C dev
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("suppressing warning toast"); //TODO_C dev | |
| dontShowAgainInThisSessionRef.current = true; | |
| } else { | |
| } |
🤖 Prompt for AI Agents
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
around line 94: remove the development console.log statement left in the code
("suppressing warning toast")—delete that line so no debug logging remains in
production; run lint/tests and ensure no unused vars or references cause errors
after removal.
|
@philippotto I am very interested to hear what you think, and whether you have a more straight-forward idea to build this. To me it did not feel 100% ideal to build this toast as a react component, although this is what I needed to use hooks. |
3 participants
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
feedback:
https://scm.slack.com/archives/C5AKLAV0B/p1760367097181289
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)