From 44139cd4907c6b9e20b12941b89bbedd1700ab2d Mon Sep 17 00:00:00 2001 From: Kevin Kenyon Date: Fri, 4 Sep 2026 11:50:41 -0500 Subject: [PATCH] fix(native): publish tour before rendering --- .../TourWorkspace/TourWorkspace.swift | 55 ++++++-- .../TourWorkspaceFeatureTests.swift | 125 ++++++++++++++++++ 2 files changed, 171 insertions(+), 9 deletions(-) diff --git a/native/Features/TourWorkspace/TourWorkspace.swift b/native/Features/TourWorkspace/TourWorkspace.swift index 3368d20..fb90c3e 100644 --- a/native/Features/TourWorkspace/TourWorkspace.swift +++ b/native/Features/TourWorkspace/TourWorkspace.swift @@ -22,6 +22,9 @@ public final class TourWorkspaceModel: ObservableObject { @Published public private(set) var history: TourHistorySnapshot? @Published public private(set) var resolvedSlices: [DiffSliceReference: ResolvedDiffSlice] = [:] @Published public private(set) var diagramLayouts: [String: DiagramLayout] = [:] + /// Material diagram categories the selected validated document does not contain. + /// This is evidence only: rendering never synthesizes a diagram from an intent. + @Published public private(set) var unavailableDiagramKinds: [DiagramKind] = [] @Published public var selectedSectionID = "overview" public let reviewID: ReviewID @@ -31,6 +34,7 @@ public final class TourWorkspaceModel: ObservableObject { private let artifacts: any TourArtifactResolving private let navigate: @MainActor (ReviewAnchor) -> Void private var generationTask: Task? + private var renderingTask: Task? public init( reviewID: ReviewID, revision: RevisionIdentity, @@ -42,7 +46,7 @@ public final class TourWorkspaceModel: ObservableObject { self.jobs = jobs; self.artifacts = artifacts; self.navigate = navigate } - deinit { generationTask?.cancel() } + deinit { generationTask?.cancel(); renderingTask?.cancel() } public var document: ValidatedTourDocument? { history?.selectedTour } public var canGenerate: Bool { @@ -51,6 +55,8 @@ public final class TourWorkspaceModel: ObservableObject { public func load() async { status = .loading + discardRenderableContent() + unavailableDiagramKinds = [] do { _ = try await jobs.resumePending() var snapshot = try await jobs.history(reviewID: reviewID, revision: revision) @@ -61,21 +67,40 @@ public final class TourWorkspaceModel: ObservableObject { snapshot = try await jobs.history(reviewID: reviewID, revision: revision) } history = snapshot - if let selected = snapshot.selectedTour { - try await prepareForRendering(selected) - } else { - discardRenderableContent() - } let selectedRun = snapshot.selectedTour.flatMap { selected in snapshot.runs.first { $0.tourID == selected.id } } applyStatus(from: selectedRun ?? snapshot.runs.first) + unavailableDiagramKinds = unavailableDiagramKinds(for: snapshot.selectedTour, run: selectedRun) + prepareRendering(snapshot.selectedTour) } catch { discardRenderableContent() status = .failed("Tour history could not be loaded.") } } + /// Publishing a strictly validated persisted document must not wait for optional + /// rendering decoration (syntax spans and bounded diagram layout). In particular, + /// first-open fallback remains visible even if decoration is slow. + private func prepareRendering(_ document: ValidatedTourDocument?) { + renderingTask?.cancel() + guard let document else { return } + let documentID = document.id + renderingTask = Task { [weak self] in + guard let self else { return } + do { + try await prepareForRendering(document) + } catch is CancellationError { + return + } catch { + // The validated document remains visible. Unavailable rendering data is + // deliberately omitted rather than replaced with executable content. + guard self.document?.id == documentID else { return } + discardRenderableContent() + } + } + } + public func generate() { guard let configuration, generationTask == nil else { return } status = .generating(.init(phase: "queued", fraction: 0)) @@ -160,6 +185,18 @@ public final class TourWorkspaceModel: ObservableObject { document.overview + document.chapters.flatMap(\.blocks) } + private func unavailableDiagramKinds( + for document: ValidatedTourDocument?, run: TourRunRecord? + ) -> [DiagramKind] { + let available = Set(document.map(allBlocks)?.compactMap { block -> String? in + if case let .diagram(diagram) = block { return diagram.kind.rawValue } + return nil + } ?? []) + return (run?.diagramIntents ?? []).compactMap { intent in + intent.material && !available.contains(intent.kind.rawValue) ? intent.kind : nil + } + } + private func discardRenderableContent() { resolvedSlices = [:]; diagramLayouts = [:] } @@ -294,10 +331,10 @@ public struct TourWorkspaceView: View { .font(.caption).foregroundStyle(RTCDesign.color(.textSecondary)) Text("Context \(run.contextDigest.hex.prefix(12))") .font(.caption2.monospaced()).foregroundStyle(RTCDesign.color(.textSecondary)) - ForEach(run.diagramIntents.filter(\.material), id: \.kind.rawValue) { intent in + ForEach(model.unavailableDiagramKinds, id: \.rawValue) { kind in Label( - "\(intent.kind.rawValue) diagram material", - systemImage: "point.3.connected.trianglepath.dotted" + "\(kind.rawValue) diagram unavailable (no validated diagram)", + systemImage: "exclamationmark.triangle" ) .font(.caption).foregroundStyle(RTCDesign.color(.textSecondary)) } diff --git a/native/Tests/TourWorkspaceFeatureTests/TourWorkspaceFeatureTests.swift b/native/Tests/TourWorkspaceFeatureTests/TourWorkspaceFeatureTests.swift index 8c06b73..9a507bb 100644 --- a/native/Tests/TourWorkspaceFeatureTests/TourWorkspaceFeatureTests.swift +++ b/native/Tests/TourWorkspaceFeatureTests/TourWorkspaceFeatureTests.swift @@ -24,6 +24,8 @@ struct TourWorkspaceFeatureTests { try await modelStreamingIsBoundedByWorkAndDeadline(fixture) try await reviewStateIsAuthoritative(fixture) try await materialSignalsRequireGroundedDiagrams(fixture) + try await firstOpenPublishesFallbackBeforeRenderingCompletes(fixture) + try await unavailableMaterialDiagramIsExplicit(fixture) try await cancellationIsExplicitAndDurable(fixture) try await deterministicFallbackIsStable(fixture) try await noChangesIsTruthful(fixture) @@ -603,6 +605,85 @@ struct TourWorkspaceFeatureTests { "material diagram intent was not recorded") } + private static func firstOpenPublishesFallbackBeforeRenderingCompletes(_ fixture: Fixture) async throws { + let manifest = materialManifest(fixture) + let root = FileManager.default.temporaryDirectory.appendingPathComponent( + "rtc-tour-first-open-\(UUID().uuidString)") + defer { try? FileManager.default.removeItem(at: root) } + let store = try SQLiteStore(rootURL: root) + let gate = RenderGate() + let artifacts = GatedManifestResolver(manifest: manifest, gate: gate) + let jobs = TourGenerationJobHandler( + persistence: SQLiteTourPersistence(store: store), jobs: JobQueue(store: store), artifacts: artifacts) + let model = await MainActor.run { + TourWorkspaceModel( + reviewID: fixture.revision.reviewID, revision: fixture.revision, + jobs: jobs, artifacts: artifacts, navigate: { _ in }) + } + + let started = ContinuousClock.now + let load = Task { await model.load() } + await gate.waitUntilEntered() + + // This proves persistence completed while the optional rendering resolver is + // blocked; the screen must already have left Loading without tab re-entry. + let persisted = try await jobs.history(reviewID: fixture.revision.reviewID, revision: fixture.revision) + try expect(persisted.selectedTour?.producer == .deterministicFallback, "fallback was not persisted") + let visible = await MainActor.run { () -> Bool in + if case .fallback = model.status { return model.document != nil } + return false + } + try expect(visible, "first open remained loading after a persisted fallback") + try expect( + started.duration(to: .now) < .seconds(1), + "persisted fallback was not published to the first-open model promptly") + await gate.release() + await load.value + let revisitStarted = ContinuousClock.now + await model.load() + try expect( + revisitStarted.duration(to: .now) < .seconds(1), + "persisted fallback did not retain the fast tab-reentry path") + } + + private static func unavailableMaterialDiagramIsExplicit(_ fixture: Fixture) async throws { + let manifest = materialManifest(fixture) + let root = FileManager.default.temporaryDirectory.appendingPathComponent( + "rtc-tour-no-diagram-\(UUID().uuidString)") + defer { try? FileManager.default.removeItem(at: root) } + let store = try SQLiteStore(rootURL: root) + let artifacts = ManifestTourArtifactResolver(manifest: manifest) + let jobs = TourGenerationJobHandler( + persistence: SQLiteTourPersistence(store: store), jobs: JobQueue(store: store), artifacts: artifacts) + let model = await MainActor.run { + TourWorkspaceModel( + reviewID: fixture.revision.reviewID, revision: fixture.revision, + jobs: jobs, artifacts: artifacts, navigate: { _ in }) + } + await model.load() + let unavailable = await MainActor.run { model.unavailableDiagramKinds.map(\.rawValue) } + try expect( + unavailable == [DiagramKind.controlFlow.rawValue], + "missing material diagram was not exposed as unavailable") + } + + private static func materialManifest(_ fixture: Fixture) -> ReviewManifest { + let lines = (10..<14).map { number -> DiffLine in + let text = "if branch\(number) { return }" + return DiffLine( + kind: .addition, oldLine: nil, newLine: number, text: text, + contextHash: SHA256Digest(data: Data(text.utf8))) + } + let hunk = DiffHunk( + header: "@@ -9,0 +10,4 @@", oldStart: 9, oldLines: 0, + newStart: 10, newLines: 4, lines: lines) + let file = DiffArtifact( + path: fixture.file.path, status: .modified, additions: 4, + deletions: 0, binary: false, truncated: false, + oldLineCount: 9, newLineCount: 13, hunks: [hunk]) + return fixture.manifest(files: [file]) + } + private static func deterministicFallbackIsStable(_ fixture: Fixture) async throws { let source = ManifestTourArtifactSource(manifest: fixture.manifest()) let request = TourGenerationRequest(revision: fixture.revision, contextDigest: fixture.contextDigest) @@ -765,6 +846,50 @@ private struct FakeGit: ExactGitService { func cancel(_ cancellation: GitCancellation) async {} } +private actor RenderGate { + private var entered = false + private var released = false + private var waiters: [CheckedContinuation] = [] + + func block() async { + entered = true + guard !released else { return } + await withCheckedContinuation { waiters.append($0) } + } + + func waitUntilEntered() async { + while !entered { await Task.yield() } + } + + func release() { + released = true + let pending = waiters + waiters.removeAll() + pending.forEach { $0.resume() } + } +} + +private struct GatedManifestResolver: TourArtifactResolving, Sendable { + let manifest: ReviewManifest + let gate: RenderGate + private var base: ManifestTourArtifactResolver { ManifestTourArtifactResolver(manifest: manifest) } + + func manifest(for revision: RevisionIdentity) async throws -> ReviewManifest { + try await base.manifest(for: revision) + } + + func resolve( + _ reference: DiffSliceReference, revision: RevisionIdentity + ) async throws -> ResolvedDiffSlice { + await gate.block() + return try await base.resolve(reference, revision: revision) + } + + func layout(_ diagram: DiagramDocument) throws -> DiagramLayout { + try base.layout(diagram) + } +} + private struct SecretCredentials: ModelCredentialLookup { func credential(for key: String) async throws -> String? { "secret-value-that-must-not-be-persisted" } }