refactor(direction): centralize last paragraph isRtl reads on helper (SD-2778)#3289
Merged
caio-pizzol merged 2 commits intoMay 14, 2026
Merged
Conversation
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a38d8e9dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…(SD-2778) Migrate the two remaining direct reads of attrs.direction/.dir on the paragraph inline-direction axis onto getParagraphInlineDirection: - layout-bridge/src/position-hit.ts: isRtlBlock - layout-resolved/src/resolveParagraph.ts: isRtl Behavior is unchanged on the typed directionContext path and strictly broader on fallback (the helper also covers paragraphProperties.rightToLeft). After this, no consumer outside the helper reads the legacy scalar fields; a follow-up can stop pm-adapter from writing them and drop them from ParagraphAttrs.
…ack cases
Two coverage gaps caught in review:
- layout-resolved/src/resolveLayout.test.ts ("preserves increasing
first-line marker anchor for nested RTL list levels") used
attrs.direction: 'rtl'. The pre-migration code read attrs.direction
directly, so that fixture would have passed against the old
implementation. Switch to directionContext.inlineDirection so the test
only passes through the new helper-driven typed path.
- layout-bridge/test/position-hit.test.ts: switching isRtlBlock to
getParagraphInlineDirection is strictly broader on fallback (the helper
also picks up paragraphProperties.rightToLeft when no directionContext
is present). Pin that case so the broadening is intentional and not a
regression vector.
caio-pizzol
force-pushed
the
caio-pizzol/SD-2778-collapse-paragraph-direction-reads
branch
from
4a38d8e to
2699e3a
Compare
caio-pizzol
added a commit
that referenced
this pull request
May 14, 2026
…778) After #3289 centralized every paragraph isRtl read on getParagraphInlineDirection, the scalar attrs.direction field has no remaining consumers. Drop it from the producer and the type: - pm-adapter no longer writes the conditional direction spread on ParagraphAttrs. directionContext.inlineDirection is the only source. - contracts/index.ts: remove direction?: 'ltr' | 'rtl' from ParagraphAttrs and point the directionContext doc at the helper. - contracts/direction-context.ts: tighten getParagraphInlineDirection's signature and body to drop the attrs.direction/.dir/.rtl fallbacks. paragraphProperties.rightToLeft fallback stays for PM-node / editor paths that read direction off the raw OOXML properties. Tests that asserted on attrs.direction or constructed hand-rolled FlowBlocks with { direction: 'rtl' } now use directionContext.inlineDirection. Expect additive layout JSON snapshot drift once the direction field disappears from paragraph attrs in serialized layouts. Tests: contracts 229, pm-adapter 1838, layout-bridge 1210, layout-resolved 118, painter-dom 1070, super-editor 12836, style-engine 129 - all green.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
caio-pizzol
deleted the
caio-pizzol/SD-2778-collapse-paragraph-direction-reads
branch
Contributor
|
🎉 This PR is included in superdoc v1.30.0-next.88 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.94 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.138 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.3.0-next.140 |
Contributor
|
🎉 This PR is included in superdoc-cli v0.8.0-next.109 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.92 |
caio-pizzol
added a commit
that referenced
this pull request
May 14, 2026
…778) (#3290) * refactor(direction): centralize last paragraph isRtl reads on helper (SD-2778) Migrate the two remaining direct reads of attrs.direction/.dir on the paragraph inline-direction axis onto getParagraphInlineDirection: - layout-bridge/src/position-hit.ts: isRtlBlock - layout-resolved/src/resolveParagraph.ts: isRtl Behavior is unchanged on the typed directionContext path and strictly broader on fallback (the helper also covers paragraphProperties.rightToLeft). After this, no consumer outside the helper reads the legacy scalar fields; a follow-up can stop pm-adapter from writing them and drop them from ParagraphAttrs. * test(direction): pin SD-2778 migration via typed-path + broader-fallback cases Two coverage gaps caught in review: - layout-resolved/src/resolveLayout.test.ts ("preserves increasing first-line marker anchor for nested RTL list levels") used attrs.direction: 'rtl'. The pre-migration code read attrs.direction directly, so that fixture would have passed against the old implementation. Switch to directionContext.inlineDirection so the test only passes through the new helper-driven typed path. - layout-bridge/test/position-hit.test.ts: switching isRtlBlock to getParagraphInlineDirection is strictly broader on fallback (the helper also picks up paragraphProperties.rightToLeft when no directionContext is present). Pin that case so the broadening is intentional and not a regression vector. * refactor(direction): drop ParagraphAttrs.direction scalar field (SD-2778) After #3289 centralized every paragraph isRtl read on getParagraphInlineDirection, the scalar attrs.direction field has no remaining consumers. Drop it from the producer and the type: - pm-adapter no longer writes the conditional direction spread on ParagraphAttrs. directionContext.inlineDirection is the only source. - contracts/index.ts: remove direction?: 'ltr' | 'rtl' from ParagraphAttrs and point the directionContext doc at the helper. - contracts/direction-context.ts: tighten getParagraphInlineDirection's signature and body to drop the attrs.direction/.dir/.rtl fallbacks. paragraphProperties.rightToLeft fallback stays for PM-node / editor paths that read direction off the raw OOXML properties. Tests that asserted on attrs.direction or constructed hand-rolled FlowBlocks with { direction: 'rtl' } now use directionContext.inlineDirection. Expect additive layout JSON snapshot drift once the direction field disappears from paragraph attrs in serialized layouts. Tests: contracts 229, pm-adapter 1838, layout-bridge 1210, layout-resolved 118, painter-dom 1070, super-editor 12836, style-engine 129 - all green. * refactor(direction): finish dropping attrs.direction (diff.ts + stale fixtures) Follow-up to 19b13fc. Two gaps caught on review: - layout-bridge/src/diff.ts:372 still compared a.direction !== b.direction inside paragraphAttrsEqual. Vitest passed because esbuild/swc skips cross-package types; the tsup DTS build was the one that caught it. Migrate to getParagraphInlineDirection(a) !== getParagraphInlineDirection(b) so the diff respects directionContext + paragraphProperties fallback. - Test fixtures across versionSignature.test.ts, rtl-date-parity.test.ts, cache.test.ts, and diff.test.ts still constructed hand-rolled FlowBlocks with { direction: 'rtl' }. They typecheck via Record<string, unknown> casts but no longer model the actual ParagraphAttrs shape. Migrate them all onto directionContext. Build sweep (contracts → pm-adapter → layout-bridge → layout-resolved → painter-dom) now passes; tests still green across all five. * docs(direction): retire ParagraphAttrs.direction mention in pm-adapter direction README After SD-2778 drops the scalar `direction` field, the README sentence that pointed consumers at it is stale. Point them at `getParagraphInlineDirection` instead so the next reader lands on the helper that knows about the typed context + paragraphProperties fallback. * test(direction): pin producer contract that attrs.direction is not emitted SD-2778 removes ParagraphAttrs.direction from the type, but TypeScript's structural typing permits index-signature extra keys. Add a runtime assertion inside the SD-2778 describe block that pm-adapter never emits the legacy scalar field. Catches accidental re-introduction via a future spread that TypeScript would let through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the read-side half of SD-2778. After Phase 1B (#3285) lands, every paragraph-direction consumer except two stragglers already reads from the centralized
getParagraphInlineDirectionhelper. This PR migrates the last two.What lands
layout-bridge/src/position-hit.ts:isRtlBlocknow delegates togetParagraphInlineDirection. The inlinedirectionContext?.inlineDirection+attrs.direction ?? attrs.dirladder is gone.layout-resolved/src/resolveParagraph.ts: theisRtllocal inresolveParagraphContentgoes through the helper instead of readingattrs.directiondirectly.Behavior
Strictly equivalent on the resolved-context path and strictly broader on fallback. The helper also covers
paragraphProperties.rightToLeft, which neither call site was checking. No valid typed case that returnedtruebefore now returnsfalse; the only new positives are paragraphs where the style cascade carriesrightToLeftbut nodirectionContextgot populated, which is the right answer.Tests: pm-adapter 1838, layout-bridge 1210, layout-resolved 118, painter-dom 1070. All green.
Stack: based on #3285 (SD-3138 Phase 1B). After this lands, no consumer outside the helper reads `ParagraphAttrs.direction` / `.dir` / `.rtl`. A follow-up can then stop pm-adapter from writing the scalar fields and remove them from the `ParagraphAttrs` type. Keeping that change separate so this PR stays "move reads onto a helper" without touching the producer side.