Encourage coarser mags when annotating a high number of voxels #8961
Conversation
📝 WalkthroughWalkthroughAdds frontend monitoring and a user-facing warning for large numbers of bucket-save updates: new config and API field, Redux actions and dispatching from push queue, a sliding-window watcher and warning saga that shows a toast with suppression, tests, docs, styling, and toast API tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
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 |
| 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
| 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. |
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
Outdated
Show resolved
Hide resolved
What do you think about having the toast code in a saga? I think there are no hooks that can't be expressed within a saga. useRefs and useEffect should not be relevant and |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (1)
50-83: Cancellable timing + remove dev logs + dispatch via saga effects.
- setInterval is not cancelled on saga termination, leaking timers on RESTART_SAGA. Use a cancellable loop (sleep/delay) or an eventChannel, and clear in finally if needed.
- Remove console.log dev logging.
- Prefer yield* put over Store.dispatch inside timers.
- Include the current interval in the sum to avoid 1-tick detection lag (optional).
- Consider >= for threshold comparison and a default when features() lacks the value.
Apply this diff:
@@ function* watchForNumberOfBucketsInSaveQueue(): Saga<void> { - const bucketSaveWarningThreshold = features().bucketSaveWarningThreshold; + const bucketSaveWarningThreshold = + features().bucketSaveWarningThreshold ?? 1000; let bucketsForCurrentInterval = 0; let currentBucketCounts: Array<number> = []; const bucketCountArrayLength = Math.floor( CHECK_NUMBER_OF_BUCKETS_SLIDING_WINDOW_MS / CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL_MS, ); - yield* call( - setInterval, - () => { - const sumOfBuckets = _.sum(currentBucketCounts); - console.log( - "buckets in last interval: ", - bucketsForCurrentInterval, - "currentBucketsArray: ", - currentBucketCounts, - "sumOfBuckets: ", - sumOfBuckets, - ); - if (sumOfBuckets > bucketSaveWarningThreshold) { - Store.dispatch(showManyBucketUpdatesWarningAction()); - } - currentBucketCounts.push(bucketsForCurrentInterval); - if (currentBucketCounts.length > bucketCountArrayLength) { - currentBucketCounts.shift(); - } - bucketsForCurrentInterval = 0; - }, - CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL_MS, - ); + // Cancellable loop; cancels cleanly with the saga. + while (true) { + yield* call(sleep, CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL_MS); + const sumOfBuckets = _.sum(currentBucketCounts) + bucketsForCurrentInterval; + if (sumOfBuckets >= bucketSaveWarningThreshold) { + yield* put(showManyBucketUpdatesWarningAction()); + } + currentBucketCounts.push(bucketsForCurrentInterval); + if (currentBucketCounts.length > bucketCountArrayLength) { + currentBucketCounts.shift(); + } + bucketsForCurrentInterval = 0; + } yield* takeEvery("NOTIFY_ABOUT_UPDATED_BUCKETS", (action: NotifyAboutUpdatedBucketsAction) => { bucketsForCurrentInterval += action.count; }); }Dev logs removal was previously requested.
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (1)
29-34: Remove dev-only reset interval.Resetting the suppression flag every 120s overrides the user’s choice. Remove this block before merge.
- setInterval(() => { - UserLocalStorage.setItem("suppressBucketWarning", "false"); - console.log("resetting suppressBucketWarning to false every 120s for dev purposes"); - }, 120 * 1000); - //TODO_C dev + // no periodic resets; user choice persists
🧹 Nitpick comments (4)
frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
21-21: Wiring looks good; consider relocating saga out of “view/components”.Importing a saga from viewer/view/components couples model flow to view. Please move many_bucket_updates_warning into viewer/model/sagas (or viewer/view/sagas) and import from there to keep boundaries clean.
Also applies to: 92-93
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (1)
69-71: Use yield put instead of Store.dispatch from non-saga context.*Dispatching via Store inside timers bypasses saga effects and complicates testing. With the cancellable loop, prefer yield* put(showManyBucketUpdatesWarningAction()).
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (2)
7-9: Key scope: consider per-annotation or per-organization scoping.Currently WARNING_SUPPRESSION_USER_STORAGE_KEY is global. Consider a namespaced key (e.g., suppressBucketWarning::) to allow finer control if requested in the TODOs.
43-47: Docs link OK; consider a help modal fallback.If docs are unavailable (air‑gapped installs), consider routing to an in‑app help modal as a fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
conf/application.conf(1 hunks)docs/volume_annotation/import_export.md(1 hunks)frontend/javascripts/viewer/model/sagas/root_saga.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)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/volume_annotation/import_export.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#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/model/sagas/saving/save_saga.tsxfrontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx
🧬 Code graph analysis (2)
frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (1)
manyBucketUpdatesWarningSaga(94-96)
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)
⏰ 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 (7)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (5)
39-40: Good trigger point.Starting the bucket watcher on WK_READY is appropriate and consistent with other watchers.
45-49: Nice: explicit “_MS” suffixes.Clear naming for durations helps avoid confusion.
80-83: Listener placement is correct.Accumulating counts on NOTIFY_ABOUT_UPDATED_BUCKETS is the right hook.
1-15: Note on UI-only action type.Using a UI-only action for SHOW_MANY_BUCKET_UPDATES_WARNING (not part of state reducers) is fine here; just ensure typings reference it where needed in saga code paths only.
Based on learnings.
Also applies to: 21-23
42-49: Review comment is based on a false premise and should be disregarded.Extensive codebase search found no evidence of test documentation or steps mentioning "~1000 buckets within 60s". The code consistently uses a 120s window (
CHECK_NUMBER_OF_BUCKETS_SLIDING_WINDOW_MS = 120 * 1000), and no conflicting 60s window exists elsewhere in the codebase. Since the referenced "test steps" don't exist, there is no alignment issue to address.Likely an incorrect or invalid review comment.
frontend/javascripts/viewer/view/components/many_bucket_updates_warning.tsx (2)
74-87: Toast UI looks fine.Footer composition and stickiness are consistent with existing Toast usage.
94-96: Saga wiring is correct.Listening on WK_READY to install the watcher is consistent with other flows.
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (1)
63-70: Remove debug console.log statement.This console.log has been flagged in previous reviews and should be removed before merge.
Apply this diff:
- console.log( - "buckets in last interval: ", - bucketsForCurrentInterval, - "currentBucketsArray: ", - currentBucketCounts, - "sumOfBuckets: ", - sumOfBuckets, - );frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (5)
15-21: Fix takeEvery signature: action must be first parameter.The extra arguments passed to
takeEveryare provided to the worker function after the action parameter. Your current signature breaks session suppression because the action object is assigned todontShowAgainInThisSessionand the boolean value is assigned tosetDontShowAgainInThisSession.Apply this diff:
yield takeEvery( "SHOW_MANY_BUCKET_UPDATES_WARNING", showWarningToast, - dontShowAgainInThisSession, - setDontShowAgainInThisSession, + () => dontShowAgainInThisSession, + (v: boolean) => { + dontShowAgainInThisSession = v; + }, );
23-26: Fix worker function signature: action must be first parameter.Redux-saga passes the action as the first parameter to worker functions, followed by any extra arguments from
takeEvery. Your current signature will receive the action object in thedontShowAgainInThisSessionparameter, breaking the suppression logic.Apply this diff:
function* showWarningToast( - dontShowAgainInThisSession: boolean, + _action: unknown, + getDontShowAgainInThisSession: () => boolean, setDontShowAgainInThisSession: (value: boolean) => void, ): Saga<void> {Then update line 69:
+ const dontShowAgainInThisSession = getDontShowAgainInThisSession(); if (suppressManyBucketUpdatesWarning !== "true" && dontShowAgainInThisSession !== true) {
29-33: Remove dev-only setInterval that resets suppression.This setInterval resets the user's suppression preference every 120 seconds for development purposes and should be removed before merge.
Apply this diff:
- setInterval(() => { - UserLocalStorage.setItem("suppressBucketWarning", "false"); - console.log("resetting suppressBucketWarning to false every 120s for dev purposes"); - }, 120 * 1000); - //TODO_C dev
35-38: Only persist suppression when explicitly chosen.The
onClosehandler currently persists the suppression flag even when the checkbox is unchecked (writing "false"). Only write to localStorage when the user explicitly checks "Never show this again."Apply this diff:
const onClose = () => { Toast.notificationAPI?.destroy(TOO_MANY_BUCKETS_TOAST_KEY); - UserLocalStorage.setItem(WARNING_SUPPRESSION_USER_STORAGE_KEY, neverShowAgain.toString()); + if (neverShowAgain) { + UserLocalStorage.setItem(WARNING_SUPPRESSION_USER_STORAGE_KEY, "true"); + } };
87-87: Remove dev console.log statement.This debug log statement with TODO_C comment should be removed before merge.
Apply this diff:
- } else { - console.log("suppressing warning toast"); //TODO_C dev - } + }
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (1)
65-69: Consider converting localStorage value to boolean immediately.While the current string comparison works, converting to boolean upfront improves readability and allows simpler boolean logic.
Apply this diff:
- const suppressManyBucketUpdatesWarning = UserLocalStorage.getItem( + const suppressManyBucketUpdatesWarning = + UserLocalStorage.getItem( WARNING_SUPPRESSION_USER_STORAGE_KEY, - ); + ) === "true"; const dontShowAgainInThisSession = getDontShowAgainInThisSession(); - if (suppressManyBucketUpdatesWarning !== "true" && dontShowAgainInThisSession !== true) { + if (!suppressManyBucketUpdatesWarning && !dontShowAgainInThisSession) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/root_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/model/sagas/root_saga.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#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/model/sagas/many_bucket_updates_warning_saga.tsx
🧬 Code graph analysis (1)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (2)
frontend/javascripts/viewer/model/actions/save_actions.ts (1)
NotifyAboutUpdatedBucketsAction(17-17)frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
showManyBucketUpdatesWarningAction(379-382)
⏰ 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 (4)
frontend/javascripts/viewer/model/sagas/saving/save_saga.tsx (2)
2-2: LGTM: Imports updated to support bucket monitoring.The new imports are appropriate:
featuresfor the threshold config,delayfor typed saga effects, warning action creators, andStorefor cross-saga dispatch.Also applies to: 7-7, 10-14, 21-21
39-39: Sliding window logic is sound.The watcher correctly maintains a 2-minute sliding window with 10-second intervals, accumulates bucket counts, and dispatches the warning action when the threshold is exceeded. The use of
takeEveryinside the generator to listen for bucket notifications is a valid pattern.Also applies to: 45-80
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (2)
1-8: LGTM: Imports and constants are well-defined.The imports and constant definitions are appropriate for the warning toast implementation.
91-93: LGTM: Root saga properly wired.The exported saga correctly listens for
WK_READYto start the warning system.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (2)
24-27: Persist suppression only when explicitly enabled by the user.The onClose handler always writes to localStorage, even when
neverShowAgainis false. This should only persist when the user has checked the "Never show this again" checkbox.Apply this diff:
const onClose = () => { Toast.notificationAPI?.destroy(TOO_MANY_BUCKETS_TOAST_KEY); - UserLocalStorage.setItem(WARNING_SUPPRESSION_USER_STORAGE_KEY, neverShowAgain.toString()); + if (neverShowAgain) { + UserLocalStorage.setItem(WARNING_SUPPRESSION_USER_STORAGE_KEY, "true"); + } };
76-76: Remove dev-only console.log.This debug logging should be removed before merge per the PR objectives.
Apply this diff:
- } else { - console.log("suppressing warning toast"); //TODO_C dev - } + }
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (1)
10-84: Consider simplifying the nested saga structure.The current structure has three layers:
manyBucketUpdatesWarningSaga→manyBucketUpdatesWarning→showWarningToast. The middle layer exists primarily to hold session state via closure. Per the PR objectives ("clean up saga"), consider flattening to a more conventional two-layer pattern by moving session state to module-level or using a simpler direct takeEvery in the root.Example simplified structure:
let dontShowAgainInThisSession = false; function* showWarningToast(): Saga<void> { // ... existing toast logic ... } export default function* manyBucketUpdatesWarningSaga(): Saga<void> { yield* takeEvery("WK_READY", function* () { yield* takeEvery("SHOW_MANY_BUCKET_UPDATES_WARNING", showWarningToast); }); }Or directly:
let dontShowAgainInThisSession = false; function* showWarningToast(): Saga<void> { // ... existing toast logic ... } export default function* manyBucketUpdatesWarningSaga(): Saga<void> { yield* takeEvery("SHOW_MANY_BUCKET_UPDATES_WARNING", showWarningToast); }Note: If using the direct approach, ensure the saga starts at the appropriate time in the root saga wiring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#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/model/sagas/many_bucket_updates_warning_saga.tsx
⏰ 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 (3)
frontend/javascripts/viewer/model/sagas/many_bucket_updates_warning_saga.tsx (3)
1-8: LGTM!The imports and constants are well-structured and appropriate for the saga's functionality.
59-59: Clarify if console.warn is intentional.This console warning duplicates the toast message. If it's for production observability, consider keeping it; otherwise, remove it along with the other dev logging. Unlike the other console statements, this one isn't marked as dev-only.
79-79: Action type string is correct and consistent.The action type "SHOW_MANY_BUCKET_UPDATES_WARNING" matches between the action definition in
annotation_actions.ts(line 381) and the saga listener (line 79). No changes needed.
| setInterval(() => { | ||
| 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 code before merge.
This setInterval resets the user's suppression preference every 120 seconds, which would break the "never show again" functionality in production. Per the PR objectives, all dev-only changes must be removed before merge.
Apply this diff to remove the dev code:
- setInterval(() => {
- 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.
| setInterval(() => { | |
| 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/model/sagas/many_bucket_updates_warning_saga.tsx
around lines 15 to 19, remove the dev-only setInterval that repeatedly resets
UserLocalStorage.setItem("suppressBucketWarning", "false") and the console.log
so the user's "never show again" preference is preserved; delete the interval
and the TODO_C dev comment, and ensure no other dev-only side effects remain in
this file before merging.
| while (true) { | ||
| yield* delay(CHECK_NUMBER_OF_BUCKETS_IN_SAVE_QUEUE_INTERVAL_MS); | ||
| const sumOfBuckets = _.sum(currentBucketCounts); | ||
| console.log( |
I'd do it specific to the user. have a look at |
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)