Skip to content

Fix bug where initial proofreading split action failed#9412

Merged
MichaelBuessemeyer merged 4 commits intomasterfrom
fix-initial-split
Mar 23, 2026
Merged

Fix bug where initial proofreading split action failed#9412
MichaelBuessemeyer merged 4 commits intomasterfrom
fix-initial-split

Conversation

@MichaelBuessemeyer
Copy link
Copy Markdown
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Mar 23, 2026

Fixes a bug noticed in #9102. Might be good to get this easy fix merged before #9102 is merged. Should be an easy win to fix this imo.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open an annotation with an agglomerate mapping
  • Activate the agglomerate mapping
  • Activeate the proofread tool
  • Make a split -> should succeed.
  • Try the same on master -> should result in an error

Issues:


(Please delete unneeded items, merge only when none are left open)

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 23, 2026

Warning

Rate limit exceeded

@MichaelBuessemeyer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 16 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 95ff62a4-c21a-47fb-9950-af01fb83fb49

📥 Commits

Reviewing files that changed from the base of the PR and between 4002172 and 3b72c1f.

📒 Files selected for processing (1)
  • unreleased_changes/9412.md
📝 Walkthrough

Walkthrough

A bug fix ensures proofreading annotations no longer fail during split operations. The fix defers retrieval of the annotation version in the prepareSplitOrMerge function to capture the latest state after potential mutations, with a corresponding changelog entry documenting the resolution.

Changes

Cohort / File(s) Summary
Proofreading Split/Merge Logic
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Deferred annotation version retrieval in prepareSplitOrMerge to execute immediately before returning the Preparation object, ensuring the version reflects the latest state after potential editable mapping creation and related mutations.
Changelog Documentation
unreleased_changes/9412.md
Added unreleased changelog entry documenting bug fix for proofreading annotation failures during split workflow operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • philippotto
  • normanrz

Poem

🐰 A split that once would break and fail,
Now hops along without travail,
The version waits in patient grace,
Until mutations find their place! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix bug where initial proofreading split action failed' directly summarizes the main change: fixing a bug in the proofreading split functionality, which matches the changeset modifications in proofread_saga.ts and the changelog entry.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The pull request description clearly references a bug fix in #9102 and provides specific testing steps to verify the proofreading split action works correctly.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-initial-split

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines +1412 to +1414
// Getting latest annotation version as it might have changed due to e.g. making the mapping editable.
const annotationVersion = yield* select((state) => state.annotation.version);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is easy: Load the annotation version !after! the mapping was potentially made editable (which changes the version).

The returned annotationVersion of this function as later used in the sagas handling split operations. On the master the version before making the mapping editable was returned.
=> This lead to the proofreading sagas requesting edges to split with an annotation version, where the version was actually lower than the version where it was made editable. Thus, the server correctly replied with an error.

=> Moving the select "lower" in the function fixed the bug easily

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@unreleased_changes/9412.md`:
- Line 2: Replace the changelog sentence "Fixed a bug where the initial
operation of a proofreading annotation failed if it a split action." with the
corrected, concise wording "Fixed a bug where the initial operation of a
proofreading annotation failed if it involved a split action." ensuring the rest
of the entry remains unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c4c5312c-801b-4c0b-8f67-79010dc0a777

📥 Commits

Reviewing files that changed from the base of the PR and between 8763cae and 4002172.

📒 Files selected for processing (2)
  • frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
  • unreleased_changes/9412.md

@@ -0,0 +1,2 @@
### Fixed
- Fixed a bug where the initial operation of a proofreading annotation failed if it a split action.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix wording typo in changelog sentence.

Line 2 has a grammar issue (“if it a split action”). Suggested concise rewrite:

  • Fixed a bug where the initial operation of a proofreading annotation failed if it involved a split action.

Based on learnings, changelog entries should remain concise and avoid unnecessary detail.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unreleased_changes/9412.md` at line 2, Replace the changelog sentence "Fixed
a bug where the initial operation of a proofreading annotation failed if it a
split action." with the corrected, concise wording "Fixed a bug where the
initial operation of a proofreading annotation failed if it involved a split
action." ensuring the rest of the entry remains unchanged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks for the pointer. Fixed it now. But involved seems like a wrong verb here. Imo "is" is better. Is this grammatically correct now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelBuessemeyer, glad the typo is fixed! Using "is" introduces a tense mismatch — the sentence uses past tense ("failed") but "is" is present tense. For changelog entries, keeping consistent past tense is standard style.

Here are a few grammatically correct options, all in past tense:

  1. "…failed if it involved a split action." — the operation included/comprised a split
  2. "…failed if it was a split action." — the operation was of the type split
  3. "…failed when performing a split action." — slightly more direct

Any of these work well. Option 1 (involved) or option 3 (when performing) would be my recommendation for clarity. 🐇

Copy link
Copy Markdown
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I cherry-picked this fix at some point for one of my branches, so it's tested already :)

Co-authored-by: Daniel <daniel.werner@scalableminds.com>
@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) March 23, 2026 11:01
@MichaelBuessemeyer MichaelBuessemeyer merged commit fe99e73 into master Mar 23, 2026
5 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the fix-initial-split branch March 23, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants