Update LayoutComputer #258
Conversation
WalkthroughThis update introduces several new files and protocols to enhance layout computation, caching, and tracing in the rendering framework. A type-erased box pattern replaces the previous delegate-based design for layout engines, enabling support for tracing and safer mutation. New protocols ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LayoutComputer
participant AnyLayoutEngineBox
participant LayoutEngine
participant LayoutTrace
Client->>LayoutComputer: sizeThatFits(proposal)
LayoutComputer->>AnyLayoutEngineBox: sizeThatFits(proposal)
AnyLayoutEngineBox->>LayoutEngine: sizeThatFits(proposal)
alt Tracing Enabled
LayoutEngine->>LayoutTrace: traceSizeThatFits(...)
LayoutTrace-->>LayoutEngine: traced result
end
LayoutEngine-->>AnyLayoutEngineBox: CGSize
AnyLayoutEngineBox-->>LayoutComputer: CGSize
LayoutComputer-->>Client: CGSize
sequenceDiagram
participant LeafView
participant LeafLayoutEngine
participant ViewSizeCache
LeafView->>LeafLayoutEngine: sizeThatFits(proposal)
LeafLayoutEngine->>ViewSizeCache: get(proposal)
alt Cache Hit
ViewSizeCache-->>LeafLayoutEngine: cached CGSize
else Cache Miss
LeafLayoutEngine->>LeafView: sizeThatFits(in: proposal)
LeafView-->>LeafLayoutEngine: CGSize
LeafLayoutEngine->>ViewSizeCache: store CGSize
ViewSizeCache-->>LeafLayoutEngine: CGSize
end
LeafLayoutEngine-->>LeafView: CGSize
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift (1)
34-36: Consider using@autoclosurefor lighter call‑sites (optional)If callers usually pass an expression rather than an explicit closure, turning
makeValueinto an@autoclosurecan make the API more ergonomic without
affecting behaviour.- package mutating func get(_ k: _ProposedSize, makeValue: () -> CGSize) -> CGSize { + package mutating func get( + _ k: _ProposedSize, + makeValue: @autoclosure () -> CGSize + ) -> CGSize { cache.get(ProposedViewSize(k), makeValue: makeValue) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift(1 hunks)Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift(1 hunks)Sources/OpenSwiftUICore/Render/RendererLeafView.swift(2 hunks)Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (3)
Sources/OpenSwiftUICore/Layout/Geometry/Spacing.swift (1)
spacing(138-150)Sources/OpenSwiftUICore/Shape/Shape.swift (2)
sizeThatFits(73-75)sizeThatFits(149-151)Sources/OpenSwiftUICore/Shape/ShapeView.swift (1)
sizeThatFits(149-151)
🪛 SwiftLint (0.57.0)
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift
[Warning] 10-10: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 16-16: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 20-20: TODOs should be resolved (OGGraphRegisterNamedTraceEvent)
(todo)
[Warning] 42-42: TODOs should be resolved
(todo)
Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift
[Error] 260-260: Force casts should be avoided
(force_cast)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2021, 18.0)
- GitHub Check: Execute compatibility tests for iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute compatibility tests for macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2021)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute tests on Ubuntu (6.0.1)
🔇 Additional comments (4)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (1)
24-31: Unimplemented stubs crash at runtime
contains(points:size:)andmakeLeafViewcurrently call
preconditionFailure("TODO"). Any code path that reaches these methods will
terminate the app. If this file is checked in, consider at least returning a
placeholder implementation or marking the methods withfatalErrorguarded by
#if DEBUGso production builds do not accidentally crash.Would you like help drafting non‑crashing default implementations?
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift (1)
83-90: Forced unwrap ofrecordercan still crash in edge casesThe wrappers rely on
recorder!being non‑nil, assuming aTracingLayoutEngineBoxexists only whenLayoutTrace.isEnabled == true.
All good – until another call site uses these APIs directly without first callingregister(graph:), or ifrecorderis cleared during teardown.Consider:
-recorder!.traceSizeThatFits(attribute, proposal: proposal, block) +guard let r = recorder else { return block() } // or assert in DEBUG only +r.traceSizeThatFits(attribute, proposal: proposal, block)This avoids unexpected production crashes while still surfacing mistakes in debug builds.
Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift (2)
147-156: Tracing initialiser sends content description beforerecorderis guaranteed
LayoutTrace.recorder?.traceContentDescriptionis invoked inside the initialiser, but nothing prevents clients from constructing aLayoutComputerbefore callingLayoutTrace.register(graph:).
Result: the description is silently lost.Either:
- Require registration at startup and
preconditionifrecorder == nil, or- Cache the description locally and emit it the first time
recorderis set.This guarantees trace completeness and surfaces mis‑ordering early.
159-174: Good use of higher‑order closure to keep tracing low‑overheadWrapping the engine calls with
LayoutTrace.trace*retains a single evaluation point and avoids duplicated code. Nicely done.
| // Audited for iOS 18.0 | ||
| // Status: Complete | ||
|
|
||
| package import Foundation |
There was a problem hiding this comment.
Invalid access modifier on import statement – breaks compilation
package import Foundation is not valid Swift syntax.
The package access level can only be applied to declarations, not to import directives, so the file will fail to compile.
-package import Foundation
+import Foundation📝 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.
| package import Foundation | |
| import Foundation |
| // Audited for iOS 18.0 | ||
| // Status: Complete | ||
|
|
||
| package import Foundation |
There was a problem hiding this comment.
Same compilation blocker: package import is illegal
Replace with the standard import Foundation, otherwise the file will not build.
-package import Foundation
+import Foundation📝 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.
| package import Foundation | |
| import Foundation |
| package protocol DerivedLayoutEngine: LayoutEngine { | ||
| var base: LayoutComputer { get } | ||
| } |
There was a problem hiding this comment.
base must be mutable if you forward mutating calls
spacing(), sizeThatFits, lengthThatFits, etc. mutate LayoutComputer.
Forwarding those calls through a read‑only base property triggers the
compiler error
“Cannot use mutating member on immutable value: ‘base’ is a get‑only property”.
Expose a mutable accessor:
-package protocol DerivedLayoutEngine: LayoutEngine {
- var base: LayoutComputer { get }
+package protocol DerivedLayoutEngine: LayoutEngine {
+ // `_modify` or at least `get set` lets conforming types hand out
+ // an in‑place mutable reference.
+ var base: LayoutComputer { get set }
}Conforming types will then store base as a var, allowing the forwarded
mutations to propagate correctly.
📝 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.
| package protocol DerivedLayoutEngine: LayoutEngine { | |
| var base: LayoutComputer { get } | |
| } | |
| package protocol DerivedLayoutEngine: LayoutEngine { | |
| // `_modify` or at least `get set` lets conforming types hand out | |
| // an in‑place mutable reference. | |
| var base: LayoutComputer { get set } | |
| } |
| package struct LeafLayoutEngine<V>: LayoutEngine where V: LeafViewLayout { | ||
| package let view: V | ||
|
|
||
| private var cache: ViewSizeCache | ||
|
|
||
| package init(_ view: V) { | ||
| self.view = view | ||
| self.cache = ViewSizeCache() | ||
| } | ||
|
|
||
| package func spacing() -> Spacing { | ||
| view.spacing() | ||
| } | ||
|
|
||
| package mutating func sizeThatFits(_ proposedSize: _ProposedSize) -> CGSize { | ||
| let view = view | ||
| return cache.get(proposedSize) { | ||
| view.sizeThatFits(in: proposedSize) | ||
| } | ||
| } |
There was a problem hiding this comment.
LeafLayoutEngine likely does not satisfy LayoutEngine in full
Only spacing() and sizeThatFits are implemented. The protocol additions
introduced elsewhere (layoutPriority, ignoresAutomaticPadding,
lengthThatFits, childGeometries, explicitAlignment, …) are missing, so
this type will not compile once linked against the updated LayoutEngine
definition.
Two quick fixes:
- Make
LeafLayoutEngineconform toDerivedLayoutEngineand forward the
remaining calls to aLayoutComputerbase, or - Provide concrete implementations for the missing requirements.
Let me know if you’d like a sample forwarding implementation.
| @@ -3,9 +3,13 @@ | |||
| // OpenSwiftUICore | |||
| // | |||
| // Audited for iOS 18.0 | |||
| // ID: 65609C35608651F66D749EB1BD9D2226 (SwiftUICore?) | |||
| // Status: WIP | |||
|
|
|||
| package import Foundation | |||
There was a problem hiding this comment.
Compilation blocker – package import is not valid
Every Swift file that contains package import Foundation will fail to compile.
Replace with a plain import.
-package import Foundation
+import Foundation📝 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.
| package import Foundation | |
| import Foundation |
| // TODO: OGGraphRegisterNamedTraceEvent | ||
| recorder = Recorder(graph: graph) | ||
| } |
There was a problem hiding this comment.
Tracing API is wired, but every call aborts at runtime
All public wrappers (traceSizeThatFits, traceLengthThatFits, …) ultimately hit preconditionFailure("TODO"), so the first time tracing is enabled the app will crash.
If tracing is truly work in progress, gate the feature behind a compile‑time flag or make the stubs return the underlying closure’s value so the program keeps running:
- func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize {
- preconditionFailure("TODO")
- }
+ func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize {
+ // TODO: record start‑/end‑event
+ return block()
+ }Apply the same pattern to every other trace* method until real instrumentation is added.
This allows shipping non‑crashing code while keeping the hooks in place.
Also applies to: 40-44, 45-67
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 20-20: TODOs should be resolved (OGGraphRegisterNamedTraceEvent)
(todo)
| // Audited for iOS 18.0 | ||
| // Status: WIP | ||
|
|
||
| package import OpenGraphShims |
There was a problem hiding this comment.
🛠️ Refactor suggestion
package import is not valid Swift syntax
Swift’s access‑control modifier package cannot be applied to an import statement.
Compilation will fail on this line.
-package import OpenGraphShims
+import OpenGraphShims // access control is irrelevant for imports📝 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.
| package import OpenGraphShims | |
| import OpenGraphShims // access control is irrelevant for imports |
| override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { | ||
| guard Engine.self == E.self else { | ||
| preconditionFailure("Mismatched engine type") | ||
| } | ||
| return withUnsafePointer(to: &engine) { ptr in | ||
| body(&UnsafeMutableRawPointer(mutating: UnsafeRawPointer(ptr)).assumingMemoryBound(to: E.self).pointee) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
mutateEngine uses undefined‑behaviour pointer gymnastics
Re‑binding memory through withUnsafePointer/UnsafeMutableRawPointer violates Swift’s strict aliasing rules and can break under optimisation or future compilers.
The goal is to call the closure with an inout E referring to the boxed engine; you can achieve this safely:
- return withUnsafePointer(to: &engine) { ptr in
- body(&UnsafeMutableRawPointer(mutating: UnsafeRawPointer(ptr)).assumingMemoryBound(to: E.self).pointee)
- }
+ var copy = unsafeBitCast(engine, to: E.self)
+ let result = body(©)
+ engine = unsafeBitCast(copy, to: Engine.self)
+ return resultThis keeps type information, avoids illegal pointer casts, and copies back any mutation.
📝 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.
| override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { | |
| guard Engine.self == E.self else { | |
| preconditionFailure("Mismatched engine type") | |
| } | |
| return withUnsafePointer(to: &engine) { ptr in | |
| body(&UnsafeMutableRawPointer(mutating: UnsafeRawPointer(ptr)).assumingMemoryBound(to: E.self).pointee) | |
| } | |
| } | |
| override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { | |
| guard Engine.self == E.self else { | |
| preconditionFailure("Mismatched engine type") | |
| } | |
| var copy = unsafeBitCast(engine, to: E.self) | |
| let result = body(©) | |
| engine = unsafeBitCast(copy, to: Engine.self) | |
| return result | |
| } |
| package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { | ||
| if hasValue { | ||
| let oldEngine = value.box as! LayoutEngineBox<E> | ||
| if oldEngine.engine != engine { | ||
| oldEngine.engine = engine | ||
| value.seed &+= 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Force‑cast to LayoutEngineBox<E> risks runtime crash
If the stored LayoutComputer was created with a different engine type, this cast will trap.
We can avoid it by safely down‑casting and falling back to replacement:
- let oldEngine = value.box as! LayoutEngineBox<E>
- if oldEngine.engine != engine {
- oldEngine.engine = engine
- value.seed &+= 1
- }
+ if let oldBox = value.box as? LayoutEngineBox<E>,
+ oldBox.engine != engine {
+ oldBox.engine = engine
+ value.seed &+= 1
+ } else {
+ value = LayoutComputer(engine)
+ }This keeps the optimisation when the types match, without sacrificing safety.
📝 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.
| package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { | |
| if hasValue { | |
| let oldEngine = value.box as! LayoutEngineBox<E> | |
| if oldEngine.engine != engine { | |
| oldEngine.engine = engine | |
| value.seed &+= 1 | |
| package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { | |
| if hasValue { | |
| if let oldBox = value.box as? LayoutEngineBox<E>, | |
| oldBox.engine != engine { | |
| oldBox.engine = engine | |
| value.seed &+= 1 | |
| } else { | |
| value = LayoutComputer(engine) | |
| } | |
| } | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 260-260: Force casts should be avoided
(force_cast)
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new layout engine protocol with a type-erased box pattern, adds a caching mechanism for view sizes, and implements layout tracing functionality to support debugging and performance analysis. Key changes include:
- The addition of a Tracing module (LayoutTrace) for layout events with placeholder implementations.
- The refactor of the LayoutComputer to use a type-erased engine box that supports both standard and tracing variants.
- Updates to the caching logic and derived layout engine protocol to streamline layout computation.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift | Introduces layout tracing stubs using a Recorder class with placeholder implementations. |
| Sources/OpenSwiftUICore/Render/RendererLeafView.swift | Adds caching support and new layout protocols for leaf view rendering. |
| Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift | Implements a cache for view sizes to optimize repeated size computations. |
| Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift | Refactors the LayoutComputer to use a type-erased box and updates engine mutation logic. |
| Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift | Defines a DerivedLayoutEngine protocol that leverages the updated LayoutComputer API. |
|
|
||
| package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { | ||
| if hasValue { | ||
| let oldEngine = value.box as! LayoutEngineBox<E> |
There was a problem hiding this comment.
The forced cast to LayoutEngineBox could lead to a runtime crash if the underlying type does not match. Consider using a conditional cast (e.g. 'as?') and handling the failure case gracefully.
| let oldEngine = value.box as! LayoutEngineBox<E> | |
| guard let oldEngine = value.box as? LayoutEngineBox<E> else { | |
| fatalError("Failed to cast value.box to LayoutEngineBox<E>. Ensure the type matches.") | |
| } |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CoreFoundation.CGSize) -> CGSize { | ||
| preconditionFailure("TODO") |
There was a problem hiding this comment.
The placeholder implementation using preconditionFailure("TODO") in 'traceSizeThatFits' may not provide enough context during debugging. Consider adding a more descriptive message or a temporary fallback to assist with tracing until the real implementation is ready.
| preconditionFailure("TODO") | |
| // Log a warning or provide a descriptive error message for debugging. | |
| print("Warning: traceSizeThatFits is not yet implemented. Executing fallback block.") | |
| // Execute the provided block as a temporary fallback. | |
| return block() |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift (1)
221-221: Verify centering calculation.The expression
(setting - size.value) / 2 + .zeroadds.zerowhich seems unnecessary. Ensure this calculation produces the expected centering behavior.- ViewGeometry(origin: ViewOrigin((setting - size.value) / 2 + .zero), dimensions: self) + ViewGeometry(origin: ViewOrigin((setting - size.value) / 2), dimensions: self)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSize.swift(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2021, 18.0)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute tests on Ubuntu (6.0.1)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute compatibility tests for iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2021)
🔇 Additional comments (7)
Sources/OpenSwiftUICore/Layout/View/ViewSize.swift (4)
11-14: Well-documented struct enhances code understandability.The detailed documentation clearly explains the purpose and behavior of the
ViewSizestruct, which is essential for understanding the layout system's behavior.
19-22: Good implementation detail in documentation.Explicitly documenting the use of NaN values to represent nil dimensions in the proposal is helpful for developers who might need to understand or debug the internal representation.
98-100: Proper equality implementation ensures correct comparison semantics.The explicit implementation of
==correctly compares both the actual size (value) and the proposed size (_proposal), which is important for maintaining the relationship between proposals and actual sizes during layout operations.
126-136:✅ Verification successful
Verify handling of NaN values in inset method.
The implementation replaces NaN values with 0 in the proposal (lines 132-133). This might be intentional, but it's worth confirming that this is the desired behavior rather than preserving NaN to indicate nil dimensions.
🏁 Script executed:
#!/bin/bash # Search for other places in the codebase where NaN is handled to confirm consistency rg -A 2 "isNaN" --type swiftLength of output: 2430
Confirm consistent NaN‐to‑zero policy in inset(by:)
Other parts of OpenSwiftUICore clamp NaN to 0 (e.g. CGSize+Extension, CGPoint+Extension) so treating newWidth/newHeight the same way here is consistent. Sincemax(..., 0)already guarantees non‑NaN, theisNaN ? 0checks are effectively no‑ops but match the codebase’s policy of never propagating NaN. No change required unless you intended to preserve NaN as “no proposal.”Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift (3)
195-203: Useful static constants improve API usability.The addition of
invalidValueandzerostatic constants provides convenient ways to create commonViewDimensionsinstances, which enhances the API's usability.
205-222: Helpful geometry creation methods.The
at(_:)andcentered(in:)methods provide convenient ways to createViewGeometryinstances, which simplifies common layout operations.
224-244: Simplified subscript implementations.The subscript implementations have been simplified by removing assertions and directly returning values from the underlying
guideComputer. This makes the code more concise and readable.
| extension ViewSize: Animatable { | ||
| /// The animatable data for the view size. | ||
| /// | ||
| /// This property allows the size to be animated smoothly between values. | ||
| package var animatableData: CGSize.AnimatableData { | ||
| get { value.animatableData } | ||
| set { value.animatableData = newValue } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Animatable conformance appears incomplete.
While animatableData correctly gets/sets value.animatableData, the didSetAnimatableData method only updates _proposal (lines 94-96). Consider whether value should also be updated during animations, or if this asymmetric behavior is intentional.
package mutating func didSetAnimatableData(_ value: CGSize) {
_proposal = value
+ self.value = value // Consider whether value should also be updated
}📝 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.
| extension ViewSize: Animatable { | |
| /// The animatable data for the view size. | |
| /// | |
| /// This property allows the size to be animated smoothly between values. | |
| package var animatableData: CGSize.AnimatableData { | |
| get { value.animatableData } | |
| set { value.animatableData = newValue } | |
| } | |
| } | |
| package mutating func didSetAnimatableData(_ value: CGSize) { | |
| _proposal = value | |
| self.value = value // Consider whether value should also be updated | |
| } |
| /// Updates the proposal based on animatable data. | ||
| /// | ||
| /// This method is called during animations to update the view's proposal. | ||
| /// | ||
| /// - Parameter value: The new proposed size value. | ||
| package mutating func didSetAnimatableData(_ value: CGSize) { | ||
| _proposal = value | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify animation behavior of ViewSize.
The didSetAnimatableData method only updates _proposal but not value. Confirm that this is intentional and that value is expected to be updated through a different mechanism during animations.
🏁 Script executed:
#!/bin/bash
# Search for other places where didSetAnimatableData is used
rg -A 5 "didSetAnimatableData" --type swiftLength of output: 581
🏁 Script executed:
#!/bin/bash
# Display the beginning of ViewSize.swift to inspect the struct definition
sed -n '1,200p' Sources/OpenSwiftUICore/Layout/View/ViewSize.swiftLength of output: 5684
Update Animatable conformance in ViewSize
Currently, the animatableData setter only updates value and never invokes didSetAnimatableData(_:), so _proposal remains unchanged during animations. You’ll need to decide whether proposals should animate alongside actual sizes:
- In
extension ViewSize: Animatable, calldidSetAnimatableData(_:)after settingvalue.animatableData = newValue, for example:package var animatableData: CGSize.AnimatableData { get { value.animatableData } set { value.animatableData = newValue didSetAnimatableData(CGSize(width: newValue.first, height: newValue.second)) } }
- If you do not intend to animate the proposal, remove the unused
didSetAnimatableData(_:)method altogether to avoid dead code.
| package subscript(key: AlignmentKey) -> CGFloat { | ||
| Update.assertIsLocked() | ||
| return self[explicit: key] ?? key.id.defaultValue(in: self) | ||
| self[explicit: key] ?? key.id.defaultValue(in: self) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling in alignment subscript.
The original implementation may have included assertions to catch programming errors. Confirm that the new implementation provides appropriate error handling or debugging capabilities if a requested alignment key is invalid.
🏁 Script executed:
#!/bin/bash
# Search for any assertions or error handling related to alignment keys
rg -A 3 "alignment|AlignmentKey" --type swift | grep -E "assert|precondition|fatalError"Length of output: 863
Ensure alignment subscript surfaces missing keys during development
The current subscript on line 232 in ViewDimensions.swift silently falls back to the default alignment when no explicit value is found. To catch unintended cases, wrap the fallback in a debug assertion:
• File: Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift
• Line: 232
Suggested diff:
- self[explicit: key] ?? key.id.defaultValue(in: self)
+ if let value = self[explicit: key] {
+ return value
+ } else {
+ assertionFailure("No explicit alignment set for \(key); using default.")
+ return key.id.defaultValue(in: self)
+ }📝 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.
| self[explicit: key] ?? key.id.defaultValue(in: self) | |
| if let value = self[explicit: key] { | |
| return value | |
| } else { | |
| assertionFailure("No explicit alignment set for \(key); using default.") | |
| return key.id.defaultValue(in: self) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
- Coverage 21.06% 20.84% -0.23%
==========================================
Files 334 337 +3
Lines 15322 15484 +162
==========================================
Hits 3227 3227
- Misses 12095 12257 +162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (1)
9-9:⚠️ Potential issueFix invalid Swift syntax for import statement.
The
packagekeyword cannot be applied to animportstatement. This will cause a compilation error.-package import Foundation +import FoundationSources/OpenSwiftUICore/Tracing/LayoutTrace.swift (7)
66-68:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Same issue as above - implement an empty stub that doesn't crash.
- func traceContentDescription(_ attribute: AnyAttribute?, _ description: String) { - preconditionFailure("TODO") - } + func traceContentDescription(_ attribute: AnyAttribute?, _ description: String) { + // TODO: Implement proper tracing logic + }
8-9:⚠️ Potential issueFix invalid Swift syntax for import statements.
The
packagekeyword cannot be applied toimportstatements. This will cause compilation errors.-package import Foundation -package import OpenGraphShims +import Foundation +import OpenGraphShims
54-56:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Same issue as above - implement an empty stub that doesn't crash.
- func traceCacheLookup(_ proposal: _ProposedSize, _ hit: Bool) { - preconditionFailure("TODO") - } + func traceCacheLookup(_ proposal: _ProposedSize, _ hit: Bool) { + // TODO: Implement proper tracing logic + }
62-64:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Same issue as above - implement a stub that returns the result of the provided block.
- func traceChildGeometries(_ attribute: AnyAttribute?, at parentSize: ViewSize, origin: CGPoint, _ block: () -> [ViewGeometry]) -> [ViewGeometry] { - preconditionFailure("TODO") - } + func traceChildGeometries(_ attribute: AnyAttribute?, at parentSize: ViewSize, origin: CGPoint, _ block: () -> [ViewGeometry]) -> [ViewGeometry] { + // TODO: Implement proper tracing logic + return block() + }
58-60:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Same issue as above - implement an empty stub that doesn't crash.
- func traceCacheLookup(_ proposal: CGSize, _ hit: Bool) { - preconditionFailure("TODO") - } + func traceCacheLookup(_ proposal: CGSize, _ hit: Bool) { + // TODO: Implement proper tracing logic + }
46-48:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Using
preconditionFailurewill crash the app if tracing is enabled. Implement a stub that returns the result of the provided block to ensure the application can continue running while tracing hooks are being developed.- func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CoreFoundation.CGSize) -> CGSize { - preconditionFailure("TODO") - } + func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CoreFoundation.CGSize) -> CGSize { + // TODO: Implement proper tracing logic + return block() + }
50-52:⚠️ Potential issueReplace preconditionFailure with a non-crashing implementation.
Same issue as above - implement a stub that returns the result of the provided block.
- func traceLengthThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, in axis: Axis, _ block: () -> CoreFoundation.CGFloat) -> CGFloat { - preconditionFailure("TODO") - } + func traceLengthThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, in axis: Axis, _ block: () -> CoreFoundation.CGFloat) -> CGFloat { + // TODO: Implement proper tracing logic + return block() + }Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift (2)
258-268:⚠️ Potential issueFix force casting to avoid potential runtime crashes.
Using force casting (
as!) can lead to runtime crashes if the type doesn't match. Use conditional casting and handle the failure case properly.- package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { - if hasValue { - let oldEngine = value.box as! LayoutEngineBox<E> - if oldEngine.engine != engine { - oldEngine.engine = engine - value.seed &+= 1 - } - } else { - value = LayoutComputer(engine) - } - } + package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { + if hasValue { + if let oldEngine = value.box as? LayoutEngineBox<E> { + if oldEngine.engine != engine { + oldEngine.engine = engine + value.seed &+= 1 + } + } else { + value = LayoutComputer(engine) + } + } else { + value = LayoutComputer(engine) + } + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 260-260: Force casts should be avoided
(force_cast)
103-110:⚠️ Potential issueFix unsafe pointer manipulation in mutateEngine.
The current implementation uses unsafe pointer operations that could lead to undefined behavior. Use a safer approach for type conversion.
- override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { - guard Engine.self == E.self else { - preconditionFailure("Mismatched engine type") - } - return withUnsafePointer(to: &engine) { ptr in - body(&UnsafeMutableRawPointer(mutating: UnsafeRawPointer(ptr)).assumingMemoryBound(to: E.self).pointee) - } - } + override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { + guard Engine.self == E.self else { + preconditionFailure("Mismatched engine type") + } + var copy = unsafeBitCast(engine, to: E.self) + let result = body(©) + engine = unsafeBitCast(copy, to: Engine.self) + return result + }
🧹 Nitpick comments (2)
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift (1)
11-17: Uncomment and implement once NamedTraceEvent functionality is ready.Consider either implementing these trace events or removing the commented code until implementation is ready. Commented code can lead to confusion about what is actually implemented.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 11-11: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 17-17: Prefer at least one space after slashes for comments
(comment_spacing)
Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift (1)
229-229: Improve precondition failure message for childGeometries.The current message "implement or don't call me!" is not very descriptive. Consider providing a more detailed error message to help developers understand why this method must be implemented.
- preconditionFailure("implement or don't call me!") + preconditionFailure("childGeometries(at:origin:) must be implemented by concrete LayoutEngine types")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift(1 hunks)Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSize.swift(3 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift(1 hunks)Sources/OpenSwiftUICore/Render/RendererLeafView.swift(2 hunks)Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift
- Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift
- Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift
- Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift
- Sources/OpenSwiftUICore/Layout/View/ViewSize.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (3)
Sources/OpenSwiftUICore/Layout/Geometry/Spacing.swift (1)
spacing(138-150)Sources/OpenSwiftUICore/Shape/Shape.swift (2)
sizeThatFits(73-75)sizeThatFits(149-151)Sources/OpenSwiftUICore/Shape/ShapeView.swift (1)
sizeThatFits(149-151)
🪛 SwiftLint (0.57.0)
Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift
[Error] 260-260: Force casts should be avoided
(force_cast)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift
[Warning] 12-12: TODOs should be resolved (])
(todo)
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift
[Warning] 11-11: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 17-17: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 21-21: TODOs should be resolved (OGGraphRegisterNamedTraceEvent)
(todo)
[Warning] 43-43: TODOs should be resolved
(todo)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2021, 18.0)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute compatibility tests for macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute tests on Ubuntu (6.0.1)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2021)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2024)
🔇 Additional comments (10)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (6)
10-10: LGTM - Required import for graphical operations.The import of OpenGraphShims is correctly added to support graphical operations needed for layout.
41-44: LGTM - Default implementation for spacing().Good implementation providing a default empty
Spacing()for leaf views that don't need custom spacing behavior.
46-52: LGTM - Conditional layout computer assignment.The method correctly checks if layout computation is requested before assigning a layout computer, which is an optimization to avoid unnecessary work.
56-68: LGTM - Well-designed layout computer structure.The
LeafLayoutComputerstruct follows best practices with appropriate protocol conformance and clean separation of concerns.
72-84: LGTM - Correct implementation of LayoutEngine.The
LeafLayoutEnginecorrectly implements the required methods for theLayoutEngineprotocol and properly delegates to the view for spacing.
86-91: Great use of caching for layout performance.Efficiently uses the
ViewSizeCacheto avoid redundant size calculations with the same proposed size, which is a performance optimization.Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift (2)
20-23: Implement the TODO in register method to avoid initialization issues.The TODO comment in the register method needs implementation to properly initialize the tracing system. Currently, the recorder is created but the named trace events are not registered.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 21-21: TODOs should be resolved (OGGraphRegisterNamedTraceEvent)
(todo)
74-76: LGTM - Well-designed tracing API.Good use of the
@inline(__always)attribute to ensure performance for the tracing wrapper methods.Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift (2)
17-23: LGTM - Elegant tracing integration.Good design choice to conditionally create a tracing box when tracing is enabled, avoiding overhead when tracing is disabled.
147-156: LGTM - Properly handling attribute for tracing.Good implementation capturing the current attribute during initialization and tracing content description when available.
| package import Foundation | ||
| package import OpenGraphShims |
There was a problem hiding this comment.
Fix invalid Swift syntax for import statements.
The package keyword cannot be applied to import statements. This will cause compilation errors.
-package import Foundation
-package import OpenGraphShims
+import Foundation
+import OpenGraphShims📝 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.
| package import Foundation | |
| package import OpenGraphShims | |
| import Foundation | |
| import OpenGraphShims |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (2)
72-91:LeafLayoutEngineneeds to conform toDerivedLayoutEngineor implement all required methodsThe current implementation only provides
spacing()andsizeThatFits(_:)methods, butLayoutEngineprotocol includes other requirements likelayoutPriority(),ignoresAutomaticPadding(),lengthThatFits(_:in:),childGeometries(at:origin:), andexplicitAlignment(_:at:).Since
LayoutEngineprotocol has default implementations for most methods, consider:
- Keeping the current implementation and relying on default implementations
- Explicitly conforming to
DerivedLayoutEngineand forwarding calls to a baseLayoutComputer#!/bin/bash # Check LayoutEngine protocol requirements rg -A 3 "protocol LayoutEngine" --glob "*.swift"
9-9:⚠️ Potential issueFix invalid import syntax
The
packagekeyword cannot be applied toimportstatements in Swift. This will cause compilation errors.-package import Foundation +import FoundationSources/OpenSwiftUICore/Tracing/LayoutTrace.swift (2)
8-9:⚠️ Potential issueFix invalid import syntax
The
packagekeyword cannot be applied toimportstatements in Swift. This will cause compilation errors.-package import Foundation -package import OpenGraphShims +import Foundation +import OpenGraphShims
46-48:⚠️ Potential issueUpdate tracing methods to avoid runtime crashes
All tracing methods currently abort with
preconditionFailure("TODO"), which will crash the app when tracing is enabled. This makes the tracing infrastructure unusable until fully implemented.Consider implementing a non-crashing fallback that allows the program to continue running:
- func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize { - preconditionFailure("TODO") - } + func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize { + // TODO: Implement proper tracing + return block() // Execute the block as fallback + }Apply the same pattern to all other
trace*methods to make them functional.Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift (3)
9-10:⚠️ Potential issueFix invalid import syntax
The
packagekeyword cannot be applied toimportstatements in Swift. This will cause compilation errors.-package import Foundation -package import OpenGraphShims +import Foundation +import OpenGraphShims
103-110:⚠️ Potential issueUnsafe pointer gymnastics in
mutateEngineviolates Swift's strict aliasing rulesRe-binding memory through
withUnsafePointer/UnsafeMutableRawPointercan break under optimization or with future compilers.Use a safer approach to mutate the engine:
- override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { - guard Engine.self == E.self else { - preconditionFailure("Mismatched engine type") - } - return withUnsafePointer(to: &engine) { ptr in - body(&UnsafeMutableRawPointer(mutating: UnsafeRawPointer(ptr)).assumingMemoryBound(to: E.self).pointee) - } + override func mutateEngine<E, R>(as type: E.Type, do body: (inout E) -> R) -> R where E: LayoutEngine { + guard Engine.self == E.self else { + preconditionFailure("Mismatched engine type") + } + var copy = unsafeBitCast(engine, to: E.self) + let result = body(©) + engine = unsafeBitCast(copy, to: Engine.self) + return result + }
258-264:⚠️ Potential issueReplace force cast with safe downcast to avoid potential runtime crashes
Using
as!inupdateIfNotEqualcan crash at runtime if the box was created with a different engine type.Implement a safer approach:
- package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { - if hasValue { - let oldEngine = value.box as! LayoutEngineBox<E> - if oldEngine.engine != engine { - oldEngine.engine = engine - value.seed &+= 1 - } - } else { - value = LayoutComputer(engine) - } + package mutating func updateIfNotEqual<E>(to engine: E) where E: Equatable, E: LayoutEngine { + if hasValue { + if let oldEngine = value.box as? LayoutEngineBox<E> { + if oldEngine.engine != engine { + oldEngine.engine = engine + value.seed &+= 1 + } + } else { + value = LayoutComputer(engine) + } + } else { + value = LayoutComputer(engine) + } + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 260-260: Force casts should be avoided
(force_cast)
🧹 Nitpick comments (2)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (2)
86-91: Consider adding synchronization for ViewSizeCacheThe
sizeThatFitsmethod updates the cache but isn't marked assynchronizedor using any locking mechanism. If multiple threads access this method concurrently, it could lead to race conditions with the cache.Consider adding thread safety to the cache access:
- package mutating func sizeThatFits(_ proposedSize: _ProposedSize) -> CGSize { - let view = view - return cache.get(proposedSize) { - view.sizeThatFits(in: proposedSize) - } + package mutating func sizeThatFits(_ proposedSize: _ProposedSize) -> CGSize { + let view = view + // Use a synchronized block or ensure thread safety + return cache.get(proposedSize) { + view.sizeThatFits(in: proposedSize) + }
12-12: Resolve TODO commentThere's a placeholder TODO marker in the code. Consider addressing it before finalizing this PR.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 12-12: TODOs should be resolved (])
(todo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift(1 hunks)Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift(1 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSize.swift(3 hunks)Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift(1 hunks)Sources/OpenSwiftUICore/Render/RendererLeafView.swift(2 hunks)Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Sources/OpenSwiftUICore/Layout/View/ViewSizeCache.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/OpenSwiftUICore/Layout/View/ViewFrame.swift
- Sources/OpenSwiftUICore/Layout/LayoutComputer/DerivedLayoutEngine.swift
- Sources/OpenSwiftUICore/Layout/View/ViewDimensions.swift
- Sources/OpenSwiftUICore/Layout/View/ViewSize.swift
🧰 Additional context used
🧬 Code Graph Analysis (2)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift (3)
Sources/OpenSwiftUICore/Layout/Geometry/Spacing.swift (1)
spacing(138-150)Sources/OpenSwiftUICore/Shape/Shape.swift (2)
sizeThatFits(73-75)sizeThatFits(149-151)Sources/OpenSwiftUICore/Shape/ShapeView.swift (1)
sizeThatFits(149-151)
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift (2)
Sources/OpenSwiftUICore/Data/Environment/CachedEnvironment.swift (1)
attribute(27-35)Sources/OpenSwiftUICore/Layout/View/ViewGeometry.swift (1)
origin(40-40)
🪛 SwiftLint (0.57.0)
Sources/OpenSwiftUICore/Layout/LayoutComputer/LayoutComputer.swift
[Error] 260-260: Force casts should be avoided
(force_cast)
Sources/OpenSwiftUICore/Render/RendererLeafView.swift
[Warning] 12-12: TODOs should be resolved (])
(todo)
Sources/OpenSwiftUICore/Tracing/LayoutTrace.swift
[Warning] 11-11: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 17-17: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 21-21: TODOs should be resolved (OGGraphRegisterNamedTraceEvent)
(todo)
[Warning] 43-43: TODOs should be resolved
(todo)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Execute tests on macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute tests on Ubuntu (6.0.1)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2021, 18.0)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute compatibility tests for macOS (macos-15, 16.0, 2024)
- GitHub Check: Execute compatibility tests for iOS (macos-15, 16.0, 2024, 18.0)
| package static func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize { | ||
| recorder!.traceSizeThatFits(attribute, proposal: proposal, block) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil check for recorder in trace wrapper methods
The static wrapper methods force-unwrap recorder! which will crash if tracing is enabled (via isEnabled) but recorder is nil for any reason.
Consider adding a safety check:
- package static func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize {
- recorder!.traceSizeThatFits(attribute, proposal: proposal, block)
- }
+ package static func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize {
+ guard let recorder = recorder else {
+ return block() // Fallback if recorder is nil
+ }
+ return recorder.traceSizeThatFits(attribute, proposal: proposal, block)
+ }📝 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.
| package static func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize { | |
| recorder!.traceSizeThatFits(attribute, proposal: proposal, block) | |
| } | |
| package static func traceSizeThatFits(_ attribute: AnyAttribute?, proposal: _ProposedSize, _ block: () -> CGSize) -> CGSize { | |
| guard let recorder = recorder else { | |
| return block() // Fallback if recorder is nil | |
| } | |
| return recorder.traceSizeThatFits(attribute, proposal: proposal, block) | |
| } |
| package func childGeometries(at parentSize: ViewSize, origin: CGPoint) -> [ViewGeometry] { | ||
| preconditionFailure("implement or don't call me!") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update default childGeometries implementation to avoid runtime crashes
The default implementation calls preconditionFailure, which will crash at runtime if called. This can make debugging difficult.
Consider returning an empty array as a safe default or adding more guidance in the error message:
- package func childGeometries(at parentSize: ViewSize, origin: CGPoint) -> [ViewGeometry] {
- preconditionFailure("implement or don't call me!")
- }
+ package func childGeometries(at parentSize: ViewSize, origin: CGPoint) -> [ViewGeometry] {
+ preconditionFailure("LayoutEngine.childGeometries(at:origin:) must be implemented for non-leaf views.")
+ // Alternative non-crashing approach: return []
+ }📝 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.
| package func childGeometries(at parentSize: ViewSize, origin: CGPoint) -> [ViewGeometry] { | |
| preconditionFailure("implement or don't call me!") | |
| } | |
| package func childGeometries(at parentSize: ViewSize, origin: CGPoint) -> [ViewGeometry] { | |
| preconditionFailure("LayoutEngine.childGeometries(at:origin:) must be implemented for non-leaf views.") | |
| // Alternative non‑crashing approach: return [] | |
| } |
Summary by CodeRabbit
New Features
Refactor