Skip to content

Fix first-open Tour fallback publication #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kvkenyon wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix first-open Tour fallback publication #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions native/Features/TourWorkspace/TourWorkspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -31,6 +34,7 @@ public final class TourWorkspaceModel: ObservableObject {
private let artifacts: any TourArtifactResolving
private let navigate: @MainActor (ReviewAnchor) -> Void
private var generationTask: Task<Void, Never>?
private var renderingTask: Task<Void, Never>?

public init(
reviewID: ReviewID, revision: RevisionIdentity,
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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))
Expand Down Expand Up @@ -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 = [:]
}
Expand Down Expand Up @@ -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))
}
Expand Down
125 changes: 125 additions & 0 deletions native/Tests/TourWorkspaceFeatureTests/TourWorkspaceFeatureTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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<Void, Never>] = []

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" }
}
Expand Down
Loading