Skip to content

BL-16822 Inline images: the model, its persisted state, and its undo stack - #8320

Draft
hatton wants to merge 1 commit into
BL-16822-inline-2-book-cssfrom
BL-16822-inline-3-model
Draft

BL-16822 Inline images: the model, its persisted state, and its undo stack#8320
hatton wants to merge 1 commit into
BL-16822-inline-2-book-cssfrom
BL-16822-inline-3-model

Conversation

@hatton

@hatton hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member

The whole of what an inline (Word-style) image is on disk, and the operations that
read and write it. A picture is a div.bloom-inlineImage wrapper carrying one of four
dock classes, contenteditable="false", a data-bloom-inline-image-id shared by its
per-language copies, and three custom properties: a width percent, an aspect ratio,
and a vertical offset in layout pixels.

The one design point worth knowing before reading the code: a float only wraps the
text of the block it sits in, so a picture cannot be one element. It exists once in
every bloom-editable of the translation group, the hidden lang="z" prototype
included, and the copies are kept identical by syncInlineImagesFromEditable at
operation time and normalizeInlineImages at page setup. The prototype's copy matters
more than it looks: it is what TranslationGroupManager clones when a language is
added to the collection later.

Also here, because the model owns it: the feature's own undo stack. Inline-image
operations change the DOM programmatically in several editables at once, which
neither ckeditor nor the image-operation layer can reverse, so workspaceRoot's undo
command consults a third stack the way it already consults origami's and the image
operations'. A snapshot is taken before an operation and dropped again if the
operation ended where it began.

Nothing in this commit creates a picture -- the menu item and the gestures are the
next one -- so merged alone it changes no behaviour. INLINE-IMAGES-PLAN.md is the
design document.

featureStatus.ts gains a rejection handler: without it the promise never settles when
the api is unreachable, which is the case under unit test, and the error goes to the
global reporter.

Card: https://issues.bloomlibrary.org/youtrack/issue/BL-16822


🤖 Generated with Claude Code

https://claude.ai/code/session_014DCBGajN5YyAYPBenEf1yy

Devin review


This change is Reviewable

Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

Handled — see the outcome at the end of this comment.

[Devin] Bug: Feature failures silently disable access

src/BloomBrowserUI/react_components/featureStatus.ts:42 -- outside the diff, so this is not resolvable as a thread.

When features/status fails, getFeatureStatusAsync converts every server error into undefined. Callers treat that as disabled, silently hiding or denying subscribed features instead of exposing the dependency failure.


[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Agreed, and the change is gone: featureStatus.ts is back to what master has. That reject handler was only ever there so a unit test's promise would settle; swallowing the failure in shared code would have made every feature read as unavailable whenever the api hiccuped. The test that needed it now mocks the module instead (vi.mock in inlineImageInteractions.test.ts, in the next PR up).

Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

Assessed, no change needed — see the outcome at the end of this comment.

[Devin] Investigate: PR target conflicts with guidance

AGENTS.md:1 -- outside the diff, so this is not resolvable as a thread.

The merge base is on master, while repository guidance requires Version6.5 for new work. Verify this PR belongs on the release branch.


[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Not an issue -- same answer as on the first PR of the stack. This work targets master (6.6) by the developer's decision; the Version6.5 line is in AGENTS.md's TEMPORARY section.

@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin on 2026-09-07 12:46 MDT up to commit dcceab225a.

It raised 6 things. Each has its own review thread with the reasoning and the outcome; in short:

  • Formatting changes lose undo priority — real: after deleting a picture, bolding a word then pressing Undo brought the picture back instead of taking the bolding off. Fixed.
  • ID-less images multiply during normalization — real: markup that arrived without an identity gained another copy of the picture on every page setup. Fixed.
  • Feature failures silently disable access — agreed; the change is reverted, so a hiccup in the api no longer reads as 'this feature is off'.
  • Image geometry misses immediate saves — not an issue: the value repairs itself on the next page load.
  • Runtime integration remains deferred — correct and intended; this PR is deliberately inert.
  • PR target conflicts with guidance — not an issue, as on the other PRs of the stack.

1 informational observation were also reported and read but not mirrored as threads (low signal).

The fixes were amended into this branch after the review, so the commit above is not the current head; a fresh review is running against the new one. The only GitHub check on this PR is pr-automation (the review trigger), which passed; the front-end unit suite (915 tests), the C# spreadsheet tests (415) and the end-to-end suite (85) were all run locally and are green.

@hatton
hatton force-pushed the BL-16822-inline-3-model branch from 78244c2 to 471b275 Compare September 7, 2026 21:09
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin again on 2026-09-07 14:00 MDT, on commit 78244c25de (the head after the first round of fixes).

It raised 3 new things. Each has its own review thread with the reasoning and the outcome; in short:

  • Hidden language overwrites visible image — real: with language 1 turned off on the page, its invisible copy was still preferred as canonical and got stamped over the language the person was working in. Fixed, with a test.
  • Consecutive image undos become unreachable — real: the undo gate wanted a selected picture, which a delete (or an undone insert) leaves without, so the second ctrl+z fell through to text undo. Fixed, with a test.
  • Offset resizing remains unimplemented — true of this PR and deliberate; the consumer is in PR 8321, and the baseline now travels through a spreadsheet as well.

It also re-reported the findings the reviewed commit already fixes; those are the first round's threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the front-end unit suite (920 tests), the end-to-end suite (85 passed, 1 skipped), and the C# spreadsheet and translation-group tests (417 and 50).

@hatton
hatton force-pushed the BL-16822-inline-3-model branch from 471b275 to 170d1bd Compare September 7, 2026 21:55
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin a third time on 2026-09-07 15:55 MDT, on commit 471b27537a (the head after the second round of fixes).

It raised 1 new thing. Each has its own review thread with the reasoning and the outcome; in short:

  • Net-zero text edits misroute undo — real: the content comparison cannot see an edit that undid itself (a word typed and deleted again), and CKEditor holds undo points for both halves of it. An undo item now also carries a flag set when the page reports typing. Fixed, with two tests.

It also re-reported the findings the reviewed commit already fixes; those are the earlier rounds' threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the front-end unit suite (927 tests), the C# spreadsheet and translation-group tests (470), and the end-to-end suite.

@hatton
hatton force-pushed the BL-16822-inline-3-model branch from 170d1bd to 8b39889 Compare September 7, 2026 22:25
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin a fourth time on 2026-09-07 16:20 MDT, on commit 170d1bde83 (the head after the third round of fixes).

It raised 2 new things. Each has its own review thread with the reasoning and the outcome; in short:

  • Picture undo remains permanently disabled — real, and a regression I introduced last round: the "has this block been typed in" flag was a one-way latch, so after any typing the picture operation was unreachable for good and ctrl+z did nothing once CKEditor ran out. It now defers to CKEditor's own undo points rather than replacing them. Fixed, with a test.
  • Cross-block edits bypass undo ordering — real: only the top of the stack was marked, so undoing an operation in another block could expose a picture from before an edit made since. Fixed, with a test.

It also re-reported the findings the reviewed commit already fixes; those are the earlier rounds' threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the inline-image unit tests (47), the C# spreadsheet tests (422), and the nine end-to-end specs the cleanup touched.

@hatton
hatton force-pushed the BL-16822-inline-3-model branch from 8b39889 to 0a0f8b0 Compare September 7, 2026 22:56
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin a fifth time on 2026-09-07 16:50 MDT, on commit 8b3988922d (the head after the fourth round of fixes).

It raised 1 new thing. Each has its own review thread with the reasoning and the outcome; in short:

  • Older text edits block image undo — real, and mine: asking whether CKEditor holds anything meant that typing older than the picture operation deferred it too, so Ctrl+Z took back text from before the operation while the picture change stood. The gate now compares CKEditor's position in its own undo stack (undoManager.index) with the position recorded when the snapshot was taken. Fixed, with a test.

It also re-reported the findings the reviewed commit already fixes; those are the earlier rounds' threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the whole front-end unit suite, the C# spreadsheet tests (423), and the inline-image end-to-end specs.

…stack

The whole of what an inline (Word-style) image is on disk, and the operations that
read and write it. A picture is a div.bloom-inlineImage wrapper carrying one of four
dock classes, contenteditable="false", a data-bloom-inline-image-id shared by its
per-language copies, and three custom properties: a width percent, an aspect ratio,
and a vertical offset in layout pixels.

The one design point worth knowing before reading the code: a float only wraps the
text of the block it sits in, so a picture cannot be one element. It exists once in
every bloom-editable of the translation group, the hidden lang="z" prototype
included, and the copies are kept identical by syncInlineImagesFromEditable at
operation time and normalizeInlineImages at page setup. The prototype's copy matters
more than it looks: it is what TranslationGroupManager clones when a language is
added to the collection later.

Also here, because the model owns it: the feature's own undo stack. Inline-image
operations change the DOM programmatically in several editables at once, which
neither ckeditor nor the image-operation layer can reverse, so workspaceRoot's undo
command consults a third stack the way it already consults origami's and the image
operations'. A snapshot is taken before an operation and dropped again if the
operation ended where it began.

Nothing in this commit creates a picture -- the menu item and the gestures are the
next one -- so merged alone it changes no behaviour. INLINE-IMAGES-PLAN.md is the
design document.

featureStatus.ts gains a rejection handler: without it the promise never settles when
the api is unreachable, which is the case under unit test, and the error goes to the
global reporter.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DCBGajN5YyAYPBenEf1yy
@hatton
hatton force-pushed the BL-16822-inline-3-model branch from 0a0f8b0 to b4b0354 Compare September 7, 2026 23:30
Comment thread src/BloomBrowserUI/bookEdit/js/inlineImages.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin a sixth time on 2026-09-07 17:30 MDT, on commit 0a0f8b038e (the head after the fifth round of fixes).

It raised 1 new thing. Each has its own review thread with the reasoning and the outcome; in short:

  • Full text history misorders picture undo — real, and it defeated last round's fix: CKEditor keeps 20 undo snapshots, and once it has that many save drops the oldest before appending the newest, so its position stops counting up. Comparing positions therefore read fresh typing in a long block as no typing at all. An equal position is now read as typing whenever the stack is at its limit, which clears itself as that typing is undone. Fixed, with a test.

It also re-reported the findings the reviewed commit already fixes; those are the earlier rounds' threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the whole front-end unit suite, the C# spreadsheet tests (424), and the inline-image end-to-end specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant