From 39270268f72f56f09338a9e8942246d07fa6d4e6 Mon Sep 17 00:00:00 2001 From: phranck Date: Wed, 22 Jul 2026 01:40:53 +0200 Subject: [PATCH 1/2] Fix: Derive the image placeholder instead of mutating state in traversal - _ImageCore no longer writes its phase/lastSource state boxes while rendering; the placeholder for a changed source is derived from lastSource != source, so it still shows in the same frame - The mounted loading task is now the only writer: it commits phase first, then lastSource, and skips the commit when cancelled, so a stale load can never pair with a newer source - Since #58 every image frame emitted a body-side-effect RuntimeDiagnostic (unconditional lastSource write); rendering is now diagnostic-free - Add ImageRenderPhaseTests and an injectable TUIContext on FrameHarness --- Sources/TUIkit/Views/_ImageCore.swift | 64 +++++---- Tests/TUIkitTests/ImageRenderPhaseTests.swift | 131 ++++++++++++++++++ Tests/TUIkitTests/Support/FrameHarness.swift | 6 +- 3 files changed, 172 insertions(+), 29 deletions(-) create mode 100644 Tests/TUIkitTests/ImageRenderPhaseTests.swift diff --git a/Sources/TUIkit/Views/_ImageCore.swift b/Sources/TUIkit/Views/_ImageCore.swift index b994a255..10c33c4f 100644 --- a/Sources/TUIkit/Views/_ImageCore.swift +++ b/Sources/TUIkit/Views/_ImageCore.swift @@ -13,7 +13,11 @@ private enum StateIndex { /// Stores the loading phase (`ImageLoadingPhase`). static let phase = 0 - /// Stores the last loaded source for change detection (`ImageSource`). + /// Stores the source of the last committed load (`ImageSource?`). + /// + /// Written exclusively by the mounted loading task, never during + /// traversal. A mismatch with the current source means the stored + /// phase belongs to an outdated source. static let lastSource = 1 } @@ -78,14 +82,18 @@ struct _ImageCore: View, Renderable, Layoutable { stateStorage.markActive(identity) } - // Track the last loaded source to detect changes + // Track the source of the last committed load let sourceKey = StateStorage.StateKey(identity: identity, propertyIndex: StateIndex.lastSource) let lastSourceBox: StateBox = stateStorage.storage(for: sourceKey, default: nil) - // Detect source change and show the placeholder for the new load. - // Task cancellation and restart are handled by the updateTask - // restart ID below (the source itself). - resetLoadingPhaseIfNeeded(phaseBox: phaseBox, lastSourceBox: lastSourceBox) + // The placeholder for a changed source is DERIVED instead of + // written: state writes during traversal violate the render-phase + // contract (they would emit RuntimeDiagnostics). The stored phase + // only counts while it belongs to the current source; the mounted + // task commits the new phase and source after the frame. + let phase: ImageLoadingPhase = lastSourceBox.value == source + ? phaseBox.value + : .loading // The loading task is a lifetime effect bound to this image's // structural identity. The SOURCE is the restart ID: an unchanged @@ -95,6 +103,7 @@ struct _ImageCore: View, Renderable, Layoutable { if context.phase == .render { mountLoadingTask( phaseBox: phaseBox, + lastSourceBox: lastSourceBox, identity: identity, context: context, lifecycle: lifecycle, @@ -105,8 +114,8 @@ struct _ImageCore: View, Renderable, Layoutable { ) } - // Render based on current phase - switch phaseBox.value { + // Render based on the derived phase + switch phase { case .loading: return renderPlaceholder( width: width, @@ -141,8 +150,13 @@ extension _ImageCore { /// Mounts the async loading task for the current source (lifetime /// effect: recorded for the frame commit inside RenderLoop passes, /// immediate on the live path). + /// + /// The task is the only writer of `phaseBox` and `lastSourceBox`; it + /// commits them together once the load finishes, from outside the + /// traversal window. private func mountLoadingTask( phaseBox: StateBox, + lastSourceBox: StateBox, identity: ViewIdentity, context: RenderContext, lifecycle: LifecycleManager, @@ -159,6 +173,7 @@ extension _ImageCore { id: src, priority: .userInitiated ) { + let loadedPhase: ImageLoadingPhase do { let rawImage: RGBAImage switch src { @@ -174,14 +189,23 @@ extension _ImageCore { } // Store the raw image; conversion happens per render pass. - // StateBox.didSet triggers setNeedsRender() automatically. - // The runtime delivers that invalidation to its async event loop. - phaseBox.value = .success(rawImage) + loadedPhase = .success(rawImage) } catch let loadError as ImageLoadError { - phaseBox.value = .failure(loadError.description) + loadedPhase = .failure(loadError.description) } catch { - phaseBox.value = .failure(error.localizedDescription) + loadedPhase = .failure(error.localizedDescription) } + + // A cancelled task lost its slot to a newer source; + // committing its result would overwrite the replacement's. + guard !Task.isCancelled else { return } + + // Phase first, then source: a render between the two writes + // still derives the placeholder instead of pairing the fresh + // phase with the outdated source. StateBox.didSet triggers + // setNeedsRender() for each write. + phaseBox.value = loadedPhase + lastSourceBox.value = src } } if let pendingEffects = context.environment.pendingFrameEffects { @@ -190,20 +214,6 @@ extension _ImageCore { mount() } } - - /// Resets the loading phase when the image source changes. - /// - /// Mutates state during rendering on purpose: the placeholder must show - /// in the SAME frame the new source appears, not one frame later. - private func resetLoadingPhaseIfNeeded( - phaseBox: StateBox, - lastSourceBox: StateBox - ) { - if let lastSource = lastSourceBox.value, lastSource != source { - phaseBox.value = .loading - } - lastSourceBox.value = source - } } // MARK: - Image Rendering diff --git a/Tests/TUIkitTests/ImageRenderPhaseTests.swift b/Tests/TUIkitTests/ImageRenderPhaseTests.swift new file mode 100644 index 00000000..89a310ff --- /dev/null +++ b/Tests/TUIkitTests/ImageRenderPhaseTests.swift @@ -0,0 +1,131 @@ +// 🖥️ TUIKit — Terminal UI Kit for Swift +// ImageRenderPhaseTests.swift +// +// License: MIT + +import Foundation +import Testing + +@testable import TUIkit + +/// Specifies the render-phase contract for `_ImageCore` (issue #27 slice): +/// rendering an image must never mutate state inside the traversal window. +/// The placeholder for a changed source is DERIVED while rendering; the +/// mounted loading task commits phase and last source from outside the +/// window, so no frame ever emits a body-side-effect diagnostic. +@MainActor +@Suite("Image Render Phase", .serialized) +struct ImageRenderPhaseTests { + + @Test("Rendering an image emits no traversal diagnostics", .timeLimit(.minutes(1))) + func renderingEmitsNoTraversalDiagnostics() async throws { + let tuiContext = TUIContext(imageLoader: StubImageLoader()) + let harness = FrameHarness(app: SwappableImageApp(), tuiContext: tuiContext) + + harness.renderFrame() + + #expect(tuiContext.runtimeDiagnostics.messages.isEmpty) + + let phaseBox = try #require(imagePhaseBox(in: tuiContext)) + while case .loading = phaseBox.value { await Task.yield() } + harness.renderFrame() + + #expect(tuiContext.runtimeDiagnostics.messages.isEmpty) + } + + @Test("Source change derives the placeholder without state writes", .timeLimit(.minutes(1))) + func sourceChangeDerivesPlaceholder() async throws { + let tuiContext = TUIContext(imageLoader: StubImageLoader()) + let app = SwappableImageApp() + let harness = FrameHarness(app: app, tuiContext: tuiContext) + + harness.renderFrame() + let phaseBox = try #require(imagePhaseBox(in: tuiContext)) + while case .loading = phaseBox.value { await Task.yield() } + harness.renderFrame() + + app.cell.source = .file("/stub/b.png") + let outputCountBeforeChange = harness.terminal.writtenOutput.count + harness.renderFrame() + + // The placeholder appears in the SAME frame as the new source … + let changeFrameOutput = harness.terminal.writtenOutput[outputCountBeforeChange...].joined() + #expect(changeFrameOutput.contains(placeholderSpinnerGlyph)) + // … and nothing mutated state during the traversal. + #expect(tuiContext.runtimeDiagnostics.messages.isEmpty) + + // The replacement task commits phase and source after the frame. + let sourceBox = try #require(imageLastSourceBox(in: tuiContext)) + while sourceBox.value != .file("/stub/b.png") { await Task.yield() } + harness.renderFrame() + + #expect(tuiContext.runtimeDiagnostics.messages.isEmpty) + } +} + +// MARK: - State Access + +/// The spinner glyph `_ImageCore.renderPlaceholder` draws while loading. +private let placeholderSpinnerGlyph = "⠋" + +/// Returns the image view's phase box (`_ImageCore` property index 0). +/// +/// The fixtures render exactly one image, so the single stored identity is +/// the image's. Returns `nil` when no frame has hydrated state yet. +@MainActor +private func imagePhaseBox(in tuiContext: TUIContext) -> StateBox? { + guard let identity = tuiContext.stateStorage.storedIdentities.first else { return nil } + return tuiContext.stateStorage.storage( + for: StateStorage.StateKey(identity: identity, propertyIndex: 0), + default: .loading + ) +} + +/// Returns the image view's last-source box (`_ImageCore` property index 1). +@MainActor +private func imageLastSourceBox(in tuiContext: TUIContext) -> StateBox? { + guard let identity = tuiContext.stateStorage.storedIdentities.first else { return nil } + return tuiContext.stateStorage.storage( + for: StateStorage.StateKey(identity: identity, propertyIndex: 1), + default: nil + ) +} + +// MARK: - Fixtures + +/// Serves a deterministic single-pixel image for any request, keeping the +/// file system and network out of the tests. +private struct StubImageLoader: ImageLoader { + private let image = RGBAImage( + width: 1, + height: 1, + pixels: [RGBA(r: 255, g: 255, b: 255)] + ) + + func loadImage(from path: String) throws -> RGBAImage { + image + } + + func loadImage(from data: Data) throws -> RGBAImage { + image + } +} + +/// Mutable source slot shared between a test and its app fixture. +@MainActor +private final class ImageSourceCell { + var source: ImageSource = .file("/stub/a.png") +} + +/// Renders one image whose source the test swaps between frames. +private struct SwappableImageApp: App { + let cell = ImageSourceCell() + + init() {} + + var body: some Scene { + WindowGroup { + Image(cell.source) + } + } +} diff --git a/Tests/TUIkitTests/Support/FrameHarness.swift b/Tests/TUIkitTests/Support/FrameHarness.swift index c683fe84..0e582704 100644 --- a/Tests/TUIkitTests/Support/FrameHarness.swift +++ b/Tests/TUIkitTests/Support/FrameHarness.swift @@ -15,6 +15,9 @@ /// The default terminal is 40×24 with system status-bar items hidden, so a /// frame's content height is `24 − headerHeight` and height-gated fixtures /// can distinguish the measurement pass (full 24 rows) from output passes. +/// +/// Tests that need deterministic services (stub image loaders, injected +/// clocks, …) pass their own `TUIContext`; by default a fresh one is created. @MainActor final class FrameHarness { let app: A @@ -23,8 +26,7 @@ final class FrameHarness { private let renderLoop: RenderLoop - init(app: A, width: Int = 40, height: Int = 24) { - let tuiContext = TUIContext() + init(app: A, width: Int = 40, height: Int = 24, tuiContext: TUIContext = TUIContext()) { let terminal = MockTerminal() terminal.size = (width, height) tuiContext.statusBar.showSystemItems = false From acdbf7dd60bfd62e2ba5d571cc3e9e79e7c5b102 Mon Sep 17 00:00:00 2001 From: phranck Date: Wed, 22 Jul 2026 01:42:45 +0200 Subject: [PATCH 2/2] Refactor: Read image task dependencies from the render context - mountLoadingTask reads lifecycle, loader, cache, timeout, and pixel limit from context.environment instead of taking nine parameters (SwiftLint function_parameter_count) --- Sources/TUIkit/Views/_ImageCore.swift | 31 +++++++++------------------ 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/Sources/TUIkit/Views/_ImageCore.swift b/Sources/TUIkit/Views/_ImageCore.swift index 10c33c4f..21848787 100644 --- a/Sources/TUIkit/Views/_ImageCore.swift +++ b/Sources/TUIkit/Views/_ImageCore.swift @@ -49,7 +49,6 @@ struct _ImageCore: View, Renderable, Layoutable { func renderToBuffer(context: RenderContext) -> FrameBuffer { let stateStorage = context.environment.stateStorage! - let lifecycle = context.environment.lifecycle! let identity = context.identity let width = context.availableWidth @@ -67,10 +66,6 @@ struct _ImageCore: View, Renderable, Layoutable { let aspectRatioOverride = context.environment.imageAspectRatio let placeholderText = context.environment.imagePlaceholderText let showSpinner = context.environment.imagePlaceholderSpinner - let maxPixelCount = context.environment.imageMaxPixelCount - let urlTimeout = context.environment.imageURLTimeout - let imageLoader = context.environment.imageLoader - let imageCache = context.environment.imageCache // Retrieve or create persistent phase state let phaseKey = StateStorage.StateKey(identity: identity, propertyIndex: StateIndex.phase) @@ -104,13 +99,7 @@ struct _ImageCore: View, Renderable, Layoutable { mountLoadingTask( phaseBox: phaseBox, lastSourceBox: lastSourceBox, - identity: identity, - context: context, - lifecycle: lifecycle, - imageLoader: imageLoader, - imageCache: imageCache, - urlTimeout: urlTimeout, - maxPixelCount: maxPixelCount + context: context ) } @@ -153,19 +142,19 @@ extension _ImageCore { /// /// The task is the only writer of `phaseBox` and `lastSourceBox`; it /// commits them together once the load finishes, from outside the - /// traversal window. + /// traversal window. Loader, cache, and limits are read from the + /// render context's environment. private func mountLoadingTask( phaseBox: StateBox, lastSourceBox: StateBox, - identity: ViewIdentity, - context: RenderContext, - lifecycle: LifecycleManager, - imageLoader: any ImageLoader, - imageCache: URLImageCache, - urlTimeout: TimeInterval, - maxPixelCount: Int? + context: RenderContext ) { - let taskIdentity = identity.scoped("image.load") + let lifecycle = context.environment.lifecycle! + let imageLoader = context.environment.imageLoader + let imageCache = context.environment.imageCache + let urlTimeout = context.environment.imageURLTimeout + let maxPixelCount = context.environment.imageMaxPixelCount + let taskIdentity = context.identity.scoped("image.load") let src = source let mount = { [imageLoader, imageCache] in _ = lifecycle.updateTask(