Skip to content

refactor(paint): extract transforms/stacking/clip_path helpers from paint/mod.rs - #164

Merged
AndrewAltimit merged 2 commits into
mainfrom
refactor/paint-mod-split
May 2, 2026
Merged

refactor(paint): extract transforms/stacking/clip_path helpers from paint/mod.rs#164
AndrewAltimit merged 2 commits into
mainfrom
refactor/paint-mod-split

Conversation

@AndrewAltimit

Copy link
Copy Markdown
Owner

Summary

Splits three cohesive helper clusters out of the 2957-line crates/oasis-browser/src/paint/mod.rs into dedicated sibling modules:

  • paint/transforms.rs -- transforms_have_3d, resolve_transform_origin, resolve_perspective_origin, compute_transform_matrix, compute_transform_offsets (+ unit tests for each).
  • paint/stacking.rs -- creates_stacking_context, creates_compositing_layer, is_positioned.
  • paint/clip_path.rs -- resolve_clip_path_rect, intersect_rects. The 3 clip-path tests that lived in mod.rs move with it, and 2 new intersect_rects tests are added (the helper had none).

paint/mod.rs keeps these names available via pub(crate) use re-exports, so paint/record.rs (the only sibling caller, via super::) needs no edits at the call site. paint_box's preserve3d_child_z helper stays in mod.rs because it threads PaintContext and is called only from the recursive walker in the same file.

Result: paint/mod.rs shrinks 2957 -> 2584 lines (-373). The remaining bulk is the paint_box recursive walker plus its integration tests.

Test plan

  • cargo build --workspace
  • cargo test -p oasis-browser --lib -- 1954 passed, 0 failed
  • cargo clippy --workspace -- -D warnings -- clean
  • cargo fmt --all -- --check -- clean
  • Screenshot regression unchanged (CI will verify)

Generated with Claude Code

…aint/mod.rs

Moves three cohesive helper clusters out of the 2957-line `paint/mod.rs`
into dedicated sibling modules:

- `paint/transforms.rs` — `transforms_have_3d`, `resolve_transform_origin`,
  `resolve_perspective_origin`, `compute_transform_matrix`,
  `compute_transform_offsets` (+ unit tests)
- `paint/stacking.rs` — `creates_stacking_context`,
  `creates_compositing_layer`, `is_positioned`
- `paint/clip_path.rs` — `resolve_clip_path_rect`, `intersect_rects`
  (+ the 3 clip-path tests that lived in mod.rs, plus 2 new
  `intersect_rects` tests)

`paint/mod.rs` keeps these names available via `pub(crate) use`
re-exports, so `record.rs` (the only sibling caller, via `super::`)
needs no change. `paint_box`'s `preserve3d_child_z` helper stays
in mod.rs because it threads `PaintContext`.

Result: mod.rs shrinks from 2957 → 2584 lines (-373). All 1954
oasis-browser unit tests pass; workspace clippy is clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Claude AI Security & Correctness Review

Issues (if any)

(none)

Previous Issues (for incremental reviews)

(none)

Suggestions (if any)

  • crates/oasis-browser/src/paint/transforms.rs:119 - compute_transform_matrix is pub(crate) but is not included in mod.rs's pub(crate) use transforms::{...} re-export list. This is functionally fine (callers can reach it via super::transforms::compute_transform_matrix), but it's inconsistently published compared to its four siblings. If any future caller in record.rs [UNVERIFIED - File does not exist and is not in PR] uses the flat super:: path, it will be a compile error. Either add it to the re-exports for consistency, or make it pub(super) to signal it's internal to the transforms module.

Notes

  • 1 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • This is a mechanical extraction refactor. The logic in all three new modules (clip_path.rs, stacking.rs, transforms.rs) is byte-for-byte identical to what was in mod.rs; no behavioral change is possible.

  • The visibility promotions (fnpub(crate) fn for resolve_clip_path_rect and intersect_rects) are correct — they are now called across module boundaries via the re-export.

  • The m.e as i32 / m.f as i32 casts in compute_transform_offsets (NaN → 0, saturating on overflow) are pre-existing behavior, not introduced here.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Openrouter AI General Review

Issues (if any)

  • [BUG] crates/oasis-browser/src/paint/mod.rs:31 - compute_transform_matrix is missing from the pub(crate) use transforms::{...} re-exports.
    • The PR states record.rs [UNVERIFIED - File does not exist and is not in PR] calls helpers via super::. Without this re-export, any call to super::compute_transform_matrix will fail to compile.
    • Add compute_transform_matrix to the re-export list in mod.rs.

Previous Issues (for incremental reviews)

(none)

Suggestions (if any)

  • crates/oasis-browser/src/paint/transforms.rs:119 - If compute_transform_matrix is only consumed by compute_transform_offsets and record.rs [UNVERIFIED - File does not exist and is not in PR], consider marking it pub(super) instead of pub(crate) to accurately reflect its module boundary scope.

Notes

  • 1 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • The extraction is clean and maintains byte-for-byte logic parity with the original mod.rs.

  • All new modules correctly import their own dependencies, allowing mod.rs to safely drop unused imports (PerspectiveOrigin, TransformOrigin, ClipPath, etc.).

  • Test coverage for intersect_rects and transform helpers was appropriately added during the move.

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Claude AI Incremental Architecture & Quality Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [STILL UNRESOLVED] crates/oasis-browser/src/paint/mod.rs:38 - compute_transform_matrix is absent from the pub(crate) use transforms::{...} re-export block while all four sibling functions are included.
    • Confirmed not a compile error: the only call site is internal to transforms.rs:102 (compute_transform_offsets calls it within the same file). The mod.rs:480 reference is a doc comment, not a call.
    • Severity is lower than the original [BUG] report — this is an API surface inconsistency, not a broken build. The function is pub(crate) but is only reachable externally via super::transforms::compute_transform_matrix, not the flat super:: path that every other extracted helper uses.

Suggestions (if any)

  • crates/oasis-browser/src/paint/transforms.rs:77 - Since compute_transform_matrix is only called from within transforms.rs itself, change its visibility to pub(super) to match its actual access pattern. This closes the inconsistency without requiring a re-export addition, and signals to future readers that it is a module-private helper rather than a crate-wide utility.

Notes

  • The three-way split (clip_path.rs, stacking.rs, transforms.rs) is a clean mechanical extraction with no behavioral changes. The pub(crate) promotions on resolve_clip_path_rect and intersect_rects are correct since they now cross a module boundary via re-export.
  • intersect_rects gained two tests that were missing from the original — good coverage improvement.
  • The m.e as i32 / m.f as i32 truncating casts in compute_transform_offsets:102 are pre-existing behavior, not introduced here.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

Automated fix by Claude in response to AI review feedback.

Iteration: 1/5

Co-Authored-By: AI Review Agent <noreply@anthropic.com>
@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 1)

Status: Changes committed, pushing...

Commit: 8bb5808

Fixed Issues

  • crates/oasis-browser/src/paint/transforms.rs:77 — Changed compute_transform_matrix visibility from pub(crate) to pub(super) to resolve the API surface inconsistency raised by all three reviewers. Verified the function is only called within transforms.rs itself (line 102); the mod.rs:480 reference is a doc comment, not a call site. pub(super) keeps it reachable from the parent paint module if ever needed while signaling it is not a crate-wide utility, which is the cleaner of the two reviewer-suggested fixes (the alternative being to add it to the re-export list).

Ignored Issues

  • The original [BUG] claim that omission of compute_transform_matrix from re-exports would cause a compile error was a false positive — the third reviewer (Claude Quality, incremental) downgraded it correctly to a consistency suggestion, since no external caller exists. The fix above resolves the consistency concern at the visibility level instead of the re-export level.

Deferred to Human

  • (none)

Notes

  • Verified cargo build -p oasis-browser and cargo clippy -p oasis-browser -- -D warnings both pass after the change.
  • Two reviewers also flagged [UNVERIFIED - File does not exist] warnings about record.rs. That file does exist at crates/oasis-browser/src/paint/record.rs (referenced from paint/mod.rs:26); the reviewers' filter incorrectly flagged the path. No action needed.

Automated summary of agent fixes.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Claude AI Incremental Architecture & Quality Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-browser/src/paint/mod.rs:31 - compute_transform_matrix missing from pub(crate) use transforms::{...} re-exports.
    • Fixed by changing visibility to pub(super) in transforms.rs:77, which is correct since the only call site is compute_transform_offsets within the same file. The re-export list in mod.rs is now consistent with actual access patterns.

Suggestions (if any)

  • crates/oasis-browser/src/paint/transforms.rs:120 - The doc link [`compute_transform_matrix`] in compute_transform_offsets's rustdoc is valid today (same module), but if compute_transform_matrix is ever inlined or removed, rustdoc will emit a broken-link warning. No action required — just worth knowing the link is module-local.

Notes

  • The transforms.rs extraction is clean: pub(super) on compute_transform_matrix correctly signals it is a module-private helper rather than a crate-wide utility, closing the API surface inconsistency from the previous review cycle.
  • All nine tests in transforms.rs cover the distinct code paths (empty list, 3D variants, 2D-only, origin defaults, offset passthrough, translate shift). Coverage is sufficient.
  • No behavioral changes in any of the three extracted modules — byte-for-byte logic parity with the original mod.rs is maintained.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Openrouter AI Incremental General Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-browser/src/paint/mod.rs:31 - compute_transform_matrix missing from pub(crate) use transforms::{...} re-exports.
    • Fixed by changing visibility to pub(super) in transforms.rs:77, correctly scoping it to the module since it's only called internally by compute_transform_offsets.

Suggestions (if any)

(none)

Notes

  • Mechanical extraction is clean: logic in clip_path.rs, stacking.rs, and transforms.rs is byte-for-byte identical to the original mod.rs implementation.
  • Visibility adjustment for compute_transform_matrix (pub(super)) accurately reflects its internal-only usage within transforms.rs, resolving the API surface inconsistency flagged in prior reviews.
  • Import cleanup in mod.rs correctly drops TransformFunction, ClipPath, ClipLength, PerspectiveOrigin, and TransformOrigin as they are now encapsulated in their respective sibling modules.
  • Test coverage for intersect_rects and transform helpers was appropriately added/moved, covering edge cases like collapsed insets, disjoint rectangles, and 3D transform detection.
  • intersect_rects and resolve_clip_path_rect correctly clamp negative dimensions to 0.0, preventing invalid layout rects from propagating.

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Claude AI Incremental Security & Correctness Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-browser/src/paint/mod.rs:31compute_transform_matrix missing from pub(crate) use transforms::{...} re-exports.
    • Fixed in commit 8bb5808 by changing visibility to pub(super) in transforms.rs:77. The only call site (compute_transform_offsets, same file at line 102) is within the module, making pub(super) the correct scope. All subsequent reviewers confirmed resolution.

Suggestions (if any)

(none)

Notes

  • All three extracted modules (clip_path.rs, stacking.rs, transforms.rs) are byte-for-byte logic parity with the original mod.rs. No behavioral change is possible from a mechanical extraction.
  • compute_transform_matrix's pub(super) visibility is correct and consistent: it is called only within transforms.rs itself, and pub(super) keeps the parent paint module reachable if ever needed while correctly signaling it is not a crate-wide utility.
  • The m.e as i32 / m.f as i32 truncating casts in compute_transform_offsets are pre-existing behavior (NaN → 0, saturating on overflow since Rust 1.45); no new risk introduced.
  • resolve_clip_path_rect and intersect_rects correctly promoted to pub(crate) since they now cross a module boundary via re-export.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 2)

Status: No changes needed

Fixed Issues

  • (none)

Ignored Issues

  • The original compute_transform_matrix re-export inconsistency was already fixed in prior iteration (commit 8bb5808): visibility changed to pub(super) at crates/oasis-browser/src/paint/transforms.rs:77. All three incremental reviews on that commit explicitly mark it [RESOLVED].
  • Claude Quality's note about a rustdoc link to compute_transform_matrix in compute_transform_offsets's doc — explicitly labeled "No action required" by the reviewer; the link is currently valid.

Deferred to Human

  • (none)

Notes

  • All three latest reviews (Claude Quality, Claude Security, OpenRouter General) on commit 8bb5808 report no issues and no actionable suggestions. The PR is in a clean review state.
  • Verified transforms.rs:77 shows pub(super) fn compute_transform_matrix(...) as expected from the prior iteration's fix.

The agent reviewed feedback but determined no code changes were required.

@AndrewAltimit
AndrewAltimit merged commit 13a140f into main May 2, 2026
8 checks passed
@AndrewAltimit
AndrewAltimit deleted the refactor/paint-mod-split branch May 2, 2026 22:39
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