From e97fb627b9381d1530c4f226ea037c9ecb328677 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Mon, 21 Sep 2026 20:36:05 -0400 Subject: [PATCH 01/11] docs: add adversarial architecture audit --- ...uperdeck_architecture_adversarial_audit.md | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md new file mode 100644 index 00000000..31027a54 --- /dev/null +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md @@ -0,0 +1,160 @@ +# SuperDeck architecture plan: adversarial audit + +**Decision:** keep the main direction, but revise the execution plan before treating it as implementation-ready. + +**Repository:** `conceptadev/superdeck` +**Reviewed baseline:** `main@d160766c862a4ea7670a41c98f085bfae33a24f7` +**Date:** 21 September 2026 +**Status:** source and plan audit; new application regressions were not executed. + +## Context + +This document is an adversarial second pass over the earlier SuperDeck architecture review, action plan, and backlog. It checks those artifacts against Matt Pocock's `improve-codebase-architecture` and `codebase-design` guidance, the repository's `AGENTS.md`, and selected implementation files. + +It is a correction addendum. It does not turn the earlier 36-item backlog into 36 confirmed defects or mandatory architecture changes. The earlier work used the skills' source guidance and vocabulary, but did not execute the complete tool-driven process described by the skill, including an independent sub-agent exploration pass and the full candidate grilling loop. + +## Findings that remain justified + +### Active-deck publication + +The Wizard releases the open saved deck before invoking the generated-result applier. That leaves a correctness-sensitive ownership rule in the caller. Keep the publication finding and add a regression for saved deck A followed by a failed or invalidated generated replacement B. + +The desired invariant is: prepare a replacement completely, publish it only while it still owns the operation, and otherwise leave the previous deck coherent. + +### Memory loader + +`MemoryDeckLoader.reload()` is empty while runtime reload sets loading state to true. This is a direct contract mismatch. Fix it without inventing a generic event bus or changing every loader adapter. + +Replay behavior needs stronger tests around the subscription handoff: no lost update, no duplicate replay to existing listeners, and no stale snapshot after a newer state. + +### PDF lifecycle + +Keep the focused PDF lifecycle work. The inspected export path has no terminal `finally` cleanup for captured PNG references and mutates shared run state before its `try` block. + +However, narrow the architecture claim. The package's supported application-facing interface is already small: the public barrel exports the plugin and options, and the plugin opens the internal dialog. Application callers do not assemble the PageView, capture keys, and readiness scopes. + +Therefore: +- keep focused cleanup, cancellation, and stale-callback fixes; +- treat a private capture-host extraction as conditional; +- do not add a public export-session abstraction merely to satisfy deep-module terminology. + +### Existing architecture decisions + +Do not reopen the Markdown renderer, shared slide assembly, state-management approach, AI provider/model split, or persistence format as side effects of this work. Existing maintainer decisions already cover several of these areas. + +## Corrections to the execution plan + +### 1. Describe the method accurately + +Call the earlier work a **source-reviewed architecture plan informed by `improve-codebase-architecture` and `codebase-design`; implementation and behavioral verification pending**. + +Each claim should distinguish: +- inspected source; +- inferred consequence; +- executed check; +- proposed design. + +Do not present this self-review as independent corroboration. + +### 2. Do not make every proposed module mandatory + +A deep module is justified when it removes real caller obligations and concentrates a behavior rule. A new class or file is not evidence of increased depth. + +Names such as `ActiveDeckPublication` should remain proposed homes for proven ownership rules until the smallest correct implementation is known. + +### 3. Remove soft scheduling dependencies + +Several backlog dependencies describe a preferred order rather than a hard prerequisite. In particular: +- the saved-to-generated handoff regression need not wait for full memory replay work; +- refresh operation ownership can be fixed independently; +- stale PDF callbacks can be guarded in the current dialog before a session redesign; +- writer serialization can be tested and fixed inside the current writer; +- ADRs and domain terms should be written with the decision they record, not postponed until the end; +- obsolete coordination should be removed in the PR that proves its replacement. + +### 4. Tighten acceptance tests + +**Publication:** observe state during publication, not only after the overall future completes. A method that calls several existing setters is not automatically atomic. + +**Replay:** test an update that occurs while a subscriber attaches. Also specify the behavior for valid deck A followed by an invalid update and then a new subscriber. + +**Asset lifetime:** block a capture of an old revision, publish a replacement, run cleanup, then prove either that the old capture keeps its own asset view or that it is explicitly cancelled before saving. Verify eventual release. + +### 5. Make merge gates scope-sensitive + +Checks required for behavior changed by a PR must pass. A relevant blocked check remains an unverified guarantee unless the maintainer explicitly accepts a documented, limited exception. + +Record exact command, tested SHA, host, result, and exclusions for each implemented change. Existing green CI is baseline evidence, not proof of newly proposed race scenarios. + +### 6. Separate three work tracks + +**Focused regressions and fixes** +- publication handoff; +- reload completion; +- refresh ownership; +- thumbnail/artwork identity; +- PDF cleanup and stale callbacks; +- writer exclusivity where supported calls justify it. + +**Conditional structural changes** +- neutral publication owner; +- broader render-revision plumbing; +- private PDF capture host. + +Choose these only when the focused fixes still leave material caller obligations. + +**Platform and release decisions** +- supported macOS/Xcode combination; +- export limits; +- interruption guarantees; +- DCM setup and debt work; +- distributed-client credential policy. + +These require explicit maintainer decisions and should not silently expand into unrelated rewrites. + +## Revised first milestone + +1. Establish the pinned baseline. +2. Add the saved-A/failed-B regression and the smallest passing fix. +3. Add the memory-reload terminal-result regression and the smallest passing fix. +4. Add PDF terminal-cleanup and stale-callback regressions and the smallest passing fixes. +5. Reassess whether enough coordination remains to justify the larger module shapes. +6. Record each structural decision alongside the change that implements it. + +These slices can progress independently unless implementation evidence shows a real shared prerequisite. + +## Structural artifact check + +The supplied review artifacts were checked for internal structure: + +- 36 work items; +- 9 proposed PR groups; +- 30 regression rows; +- 44 source-register entries; +- no duplicate task or regression IDs; +- no unknown task/regression/source/PR references in task records; +- no declared dependency cycles; +- no unassigned regression rows; +- no duplicate HTML element IDs or broken internal fragment links in the two checked HTML reports. + +These checks do **not** prove test coverage, dependency necessity, runtime correctness, or that the proposed priorities are correct. + +See `superdeck_architecture_audit_checks.json` for the machine-readable results. + +## Evidence + +- Matt Pocock, `improve-codebase-architecture`: https://github.com/mattpocock/skills/blob/main/skills/engineering/improve-codebase-architecture/SKILL.md +- Matt Pocock, `codebase-design`: https://github.com/mattpocock/skills/blob/main/skills/engineering/codebase-design/SKILL.md +- SuperDeck repository guidance: `AGENTS.md` +- PDF public entry point: `packages/plugins/pdf/lib/superdeck_pdf.dart` +- PDF plugin: `packages/plugins/pdf/lib/src/pdf_plugin.dart` +- PDF controller: `packages/plugins/pdf/lib/src/pdf_controller.dart` +- Wizard handoff: `packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart` +- Document store: `packages/playground/lib/core/domain/stores/deck_document_store.dart` +- Memory loader: `packages/playground/lib/core/data/data_sources/memory_deck_loader.dart` +- Runtime session state: `packages/superdeck/lib/src/deck/deck_session_state.dart` +- Recorded renderer/assembly decisions: `docs/maintainers/renderer-and-assembly-evaluation.md` + +## Final disposition + +The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Apply the corrections above, require executed evidence for each behavior an implementation claims, and prefer the smallest change that removes a proven caller obligation. From 36ff132a97d52b89379e4a1ad96c3b23b169b46f Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Mon, 21 Sep 2026 20:36:13 -0400 Subject: [PATCH 02/11] docs: add architecture audit check results --- .../superdeck_architecture_audit_checks.json | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json diff --git a/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json b/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json new file mode 100644 index 00000000..63111394 --- /dev/null +++ b/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json @@ -0,0 +1,39 @@ +{ + "check_scope": "Artifact structure only. Not Dart analysis, Flutter tests, link HTTP checks, or UI validation.", + "repository": "conceptadev/superdeck", + "reviewed_sha": "d160766c862a4ea7670a41c98f085bfae33a24f7", + "input_sha256": { + "superdeck_architecture_review.md": "1765d7de23ef6f04082ff0768999229fd39dfb4502c3d1657b21c2a6a49149a7", + "superdeck_architecture_review.html": "484da210666ad845477d3f35c257609a1299bfde2e54c57cbb7dc698fb597fc8", + "superdeck_architecture_action_plan.md": "2cb561662165608281a7d4e3e7097927392907a038cd732c81165d2a80ee3bb9", + "superdeck_architecture_action_plan.html": "b672ba6ff75e2056d0eb6b77d576fefd6c46e24b1c989b39b1943f38c1a6cd58", + "superdeck_architecture_backlog.json": "2accbee68528eacbd7e30e988396ac2eccb9ea163aed8c708c29100fa8fd3cec" + }, + "counts": { + "tasks": 36, + "proposed_pr_groups": 9, + "regression_rows": 30, + "source_entries": 44 + }, + "duplicate_task_ids": [], + "duplicate_regression_ids": [], + "unknown_references": [], + "dependency_cycles": [], + "unassigned_regressions": [], + "tasks_without_regression_ids": [ + "SD-32", + "SD-33", + "SD-35" + ], + "html_checks": { + "superdeck_architecture_review.html": { + "duplicate_ids": [], + "broken_fragment_links": [] + }, + "superdeck_architecture_action_plan.html": { + "duplicate_ids": [], + "broken_fragment_links": [] + } + }, + "semantic_caveat": "Acyclic references do not establish necessary dependencies, correct priorities, or correct behavior." +} From 343d9e3b95d24982b41af2289b701da9a1c804ee Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Mon, 21 Sep 2026 20:36:59 -0400 Subject: [PATCH 03/11] docs: add rendered architecture audit --- ...erdeck_architecture_adversarial_audit.html | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html new file mode 100644 index 00000000..fec3e5b2 --- /dev/null +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html @@ -0,0 +1,87 @@ + + + + + +SuperDeck — Adversarial architecture audit + + + +

SuperDeck / Architecture review / Audit addendum

+
+

SuperDeck architecture plan: adversarial audit

+

Decision: keep the main direction, but revise the execution plan before treating it as implementation-ready. The strongest work remains publication correctness, memory-loader completion, asset identity, and PDF lifecycle safety.

+

Repository: conceptadev/superdeck
+Reviewed baseline: main@d160766c862a4ea7670a41c98f085bfae33a24f7
+Status: source and plan audit; new application regressions were not executed.

+ +
Purpose. This is a correction addendum to the earlier architecture review, action plan, and backlog. It checks the plan against the deep-module guidance and repository conventions. It does not turn the 36 planning items into 36 confirmed defects.
+ +

What remains justified

+

Active-deck publication

+

The Wizard releases the saved deck before invoking the generated-result applier. Keep the publication finding and protect the saved-A/failed-B transition with a regression. The intended rule is to prepare the replacement completely, publish it only while it still owns the operation, and otherwise leave the previous deck coherent.

+ +

Memory loader

+

MemoryDeckLoader.reload() is empty while runtime reload enters loading state. Fix the contract mismatch directly. Add replay handoff tests so subscription cannot lose, duplicate, or reorder current state.

+ +

PDF lifecycle

+

Keep focused cleanup, cancellation, and stale-callback work. Narrow the architecture claim: the public PDF plugin interface is already small. A private capture-host extraction is conditional internal consolidation, not a required new application-facing abstraction.

+ +

Corrections to the plan

+
    +
  1. Describe the method accurately. The earlier work was informed by improve-codebase-architecture and codebase-design, but did not execute the complete tool-driven sub-agent and grilling process. Behavioral verification remains pending.
  2. +
  3. Do not make proposed modules mandatory. A new owner is justified only when it removes a proven caller obligation and concentrates a behavior rule.
  4. +
  5. Remove soft scheduling dependencies. Publication handoff, refresh ownership, stale PDF callbacks, and writer serialization can be fixed independently unless implementation evidence shows a real prerequisite.
  6. +
  7. Tighten acceptance tests. Observe publication during notifications, test replay during subscriber attachment, and test old-asset lifetime while replacement cleanup runs.
  8. +
  9. Use scope-sensitive merge gates. Checks required by changed behavior must pass, or the maintainer must explicitly accept a documented limited exception. Existing green CI is baseline evidence only.
  10. +
  11. Separate fixes, structural proposals, and product decisions. Do not treat one backlog as one mandatory refactor program.
  12. +
+ +

Work tracks

+ + + + + + + +
TrackExamplesRule
Focused regressions and fixesPublication handoff, reload completion, refresh ownership, asset identity, PDF cleanup/callbacks, writer exclusivityWrite the regression, then make the smallest passing correction.
Conditional structural changesNeutral publication owner, broader render revision, private PDF capture hostProceed only when focused fixes still leave material caller obligations.
Platform and release decisionsmacOS/Xcode support, export limits, interruption guarantees, DCM, client credential policyRequire an explicit maintainer decision; do not silently broaden scope.
+ +

Revised first milestone

+
    +
  1. Establish the pinned baseline.
  2. +
  3. Add saved-A/failed-B publication regression and minimal fix.
  4. +
  5. Add memory-reload terminal-result regression and minimal fix.
  6. +
  7. Add PDF terminal-cleanup and stale-callback regressions and minimal fixes.
  8. +
  9. Reassess whether remaining coordination justifies larger module shapes.
  10. +
  11. Record structural decisions with the changes that implement them.
  12. +
+ +

Artifact-structure check

+

The supplied artifacts contain 36 work items, 9 proposed PR groups, 30 regression rows, and 44 source-register entries. The structural check found no duplicate task/regression IDs, unknown task references, dependency cycles, unassigned regression rows, duplicate HTML element IDs, or broken internal fragment links in the two checked HTML reports.

+

These checks do not prove runtime correctness, test coverage, dependency necessity, or priority correctness. See superdeck_architecture_audit_checks.json.

+ +

Evidence and source paths

+
    +
  • Matt Pocock: improve-codebase-architecture and codebase-design skills.
  • +
  • Repository guidance: AGENTS.md.
  • +
  • PDF interface and lifecycle: packages/plugins/pdf/lib/superdeck_pdf.dart, pdf_plugin.dart, and pdf_controller.dart.
  • +
  • Publication handoff: packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart.
  • +
  • Document and loader state: deck_document_store.dart, memory_deck_loader.dart, and deck_session_state.dart.
  • +
  • Recorded decisions: docs/maintainers/renderer-and-assembly-evaluation.md.
  • +
+ +

Final disposition

+

The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Require executed evidence for each behavior an implementation claims and prefer the smallest change that removes a proven caller obligation.

+
+ From 882a3209491f7162880718c61889d6c1d1b156d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 12:45:45 +0000 Subject: [PATCH 04/11] docs: add second-pass architecture findings and change sizing Run the improve-codebase-architecture process against the reviewed baseline and record the result in the audit: four candidates with deletion-test verdicts, the size of each focused slice, findings the first pass missed, and corrections to the first pass's own wording. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01P2GhoLCcxJsj66fwofnxw9 --- ...uperdeck_architecture_adversarial_audit.md | 98 ++++++++++++++++--- 1 file changed, 84 insertions(+), 14 deletions(-) diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md index 31027a54..28b13bde 100644 --- a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md @@ -4,7 +4,7 @@ **Repository:** `conceptadev/superdeck` **Reviewed baseline:** `main@d160766c862a4ea7670a41c98f085bfae33a24f7` -**Date:** 21 September 2026 +**Date:** 21 September 2026; second pass 22 September 2026 **Status:** source and plan audit; new application regressions were not executed. ## Context @@ -13,6 +13,64 @@ This document is an adversarial second pass over the earlier SuperDeck architect It is a correction addendum. It does not turn the earlier 36-item backlog into 36 confirmed defects or mandatory architecture changes. The earlier work used the skills' source guidance and vocabulary, but did not execute the complete tool-driven process described by the skill, including an independent sub-agent exploration pass and the full candidate grilling loop. +**The earlier artifacts are not in this repository.** The review, action plan, and backlog that this document corrects were never committed on any branch. `superdeck_architecture_audit_checks.json` records their SHA-256 hashes, but a maintainer cannot open them. References below to backlog items, PR groups, and task IDs such as `SD-32` cannot be verified from the repository. Either commit those artifacts beside this document or treat the findings below as the standalone record. + +## Second pass: the skill's process, run against the code + +On 22 September 2026 the skill's process was run directly on this baseline: scope by commit history, read the ADR and recorded decisions, explore the code, apply the deletion test to each suspect module, and write the candidate report. The report was written outside the repository, as the skill requires. Two departures from the skill: the exploration was done without a sub-agent, by maintainer instruction, and the grilling loop was not started because it needs the maintainer's choice of candidate. + +Terms below follow the `codebase-design` glossary: module, interface, implementation, depth, seam, adapter, leverage, locality. The repository has no `CONTEXT.md`, so domain terms come from the code's own doc comments: deck, generated deck, saved deck, runtime, document, preview, artwork, Wizard, library. + +### Candidates + +| # | Candidate | Strength | Dependency category | +| --- | --- | --- | --- | +| 1 | Give deck publication one owner | Strong | in-process | +| 2 | Deepen the memory deck loader | Strong | in-process | +| 3 | One PDF export is one run | Worth exploring | local-substitutable | +| 4 | One notion of "is this run still current" | Speculative | in-process | + +**1. Give deck publication one owner.** Two publishers write the same four stores by hand: `GeneratedDeckResultApplier` writes the document, the preview loader, and the theme; `DeckLibraryController.open` writes those three and binds the artwork owner. The rule "unbind the saved deck's artwork only once the generated deck is published" lives in a lambda in `wizard_page.dart`, which runs before the applier can abandon the result. Deletion test: delete the applier and its staging, queue, and eviction logic reappears in the Wizard controller; delete the lambda and the invariant vanishes, which is the saved-A/failed-B defect. Both concentrate, so the module earns its keep now. `DeckLibraryController` already documents itself as the owner of "which deck's artwork the renderer resolves", so it is the natural home. No new class is required. ADR-0001 keeps deck-owned artwork separate from derived thumbnails; the deepened module owns artwork binding only. + +**2. Deepen the memory deck loader.** `MemoryDeckLoader` is about ten lines of implementation behind an interface that carries three hidden facts: subscribe before the first update or lose it (the eager-provider rule in `providers.dart`), `reload()` does nothing while `DeckSessionState.reload()` marks itself loading, and unchanged text never re-emits, so the retry button in `slide_page_content.dart` hangs. Deletion test: delete the loader and the three rules reappear in every publisher and in the app root. The deepening is for the loader to own its current state: late subscribers receive it, `reload()` re-emits it, and the ordering rule leaves the app root. No event bus and no new adapter. + +**3. One PDF export is one run.** `PdfController` is reusable across exports, so `export()` resets run state before its `try` and nothing clears captured images in a `finally`. The dialog must dispose and recreate the controller when slides change, and `_handleExport` re-reads the controller field after its await, so it acts on whichever controller is current. Two `@visibleForTesting` members let tests reach past the interface. Capture already has two adapters, `SlideCaptureService` in production and a fake in tests, so that internal seam is real. The public interface of the plugin is two exports with one caller, so no public export-session seam is justified. + +**4. One notion of "is this run still current".** The generation engine is the hottest area in commit history and is already deep: four entry points on `DeckGeneratorService` with about 4,000 lines behind them in part files. Run ownership is expressed four ways: an operation epoch in the Wizard controller, `isCancelled` closures polled about fifty times in the engine, an `isValid` guard in the applier, and a `revision` counter on `DeckDocumentStore` that nothing reads outside its own test. Not proposed now. Revisit if candidates 1 and 2 leave the guard plumbing painful. + +**Looked at, not proposed.** The Markdown renderer, slide assembly, and Mermaid are decided in `renderer-and-assembly-evaluation.md` and ADR-0001; no friction found that warrants reopening them. `DeckLibrary` has a four-method interface and writes each file through a temporary sibling and a rename; it is deep enough. `deck_generator_pipeline_helpers_test.dart` tests past the engine's interface and should fold into service-level tests when next touched. + +**Top recommendation:** candidate 1. It passes the deletion test today and fixes a confirmed defect. Candidate 2 is the smallest change if a one-file first slice is wanted. + +### Size of the change + +Estimates cover the focused slice of each candidate, with regressions. Lines are net source plus test lines. + +| Candidate | Source files | Test files | Lines | Public interface change | +| --- | --- | --- | --- | --- | +| 1. Publication owner | 4 (`wizard_page.dart`, `generated_deck_result_applier.dart`, `deck_library_controller.dart`, `deck_document_store.dart`) | 3 | about 250 | none outside `packages/playground` | +| 2. Memory loader | 2 (`memory_deck_loader.dart`, `providers.dart` comment) | 2 | about 120 | none; `DeckLoader` unchanged | +| 3. PDF run, focused fix | 2 (`pdf_controller.dart`, `pdf_export_screen.dart`) | 2 | about 80 | none; barrel unchanged | +| 3. PDF run, full restructure | 2 | 2 | about 300 | none | +| 4. Run identity | not estimated | | | | + +Total for the three focused slices: about 450 lines across eight source files and seven test files, all inside `packages/playground` and `packages/plugins/pdf`. No package in `packages/superdeck` or `packages/core` changes its interface. Each slice is independent and can ship as its own PR. + +### Findings the first pass missed + +- `DeckDocumentStore.revision` has no reader outside its own test. It is interface with no leverage. Delete it with candidate 1. +- The subscribe-before-publish rule leaks into the app root. The eager-provider comment in `packages/playground/lib/app/providers.dart` is a loader invariant living in the wrong module. Candidate 2 removes the need for it. +- Two test-only members on `PdfController` expose internal seams to tests. The skill's deepening notes say not to do this. Candidate 3 deletes them. +- `DeckLibraryController.refresh()` lacks the busy guard that `save()` and `open()` have, so it can interleave with them. Candidate 1's owner absorbs this fix. +- The first pass did not say whether it checked the commit-history hot spot. The second pass did: the generation engine is deep and needs no candidate. + +### Corrections to this document's first pass + +- The "Memory loader" section below speaks of replay behavior. Replay to late subscribers is the proposed deepening, not existing behavior. The loader has no replay today. +- "Thumbnail/artwork identity" appears in the fix track below with no finding section that explains it. It stays listed, but it needs a recorded finding before work starts. +- The first pass rated a neutral publication owner as conditional. The deletion test says the owner earns its keep now. The correction is in "Separate three work tracks" below. +- The rendered HTML copy beside this file is a second source of truth and drifts from it. The skill writes its report to a temporary directory so nothing lands in the repository. The HTML has not been updated with the second pass and should be removed. + ## Findings that remain justified ### Active-deck publication @@ -21,11 +79,13 @@ The Wizard releases the open saved deck before invoking the generated-result app The desired invariant is: prepare a replacement completely, publish it only while it still owns the operation, and otherwise leave the previous deck coherent. +Verified on the baseline: `wizard_page.dart` line 123 calls `releaseOpenDeck()` before `apply()`. If the applier abandons or throws, deck A's text stays in the document store, but its artwork now resolves through the in-memory fallback and the "Save another copy" state is lost. + ### Memory loader `MemoryDeckLoader.reload()` is empty while runtime reload sets loading state to true. This is a direct contract mismatch. Fix it without inventing a generic event bus or changing every loader adapter. -Replay behavior needs stronger tests around the subscription handoff: no lost update, no duplicate replay to existing listeners, and no stale snapshot after a newer state. +If the fix adds replay to late subscribers, which the second pass recommends, it needs tests around the subscription handoff: no lost update, no duplicate replay to existing listeners, and no stale snapshot after a newer state. ### PDF lifecycle @@ -38,6 +98,8 @@ Therefore: - treat a private capture-host extraction as conditional; - do not add a public export-session abstraction merely to satisfy deep-module terminology. +The stale-callback defect, made concrete: `_handleExport` in `pdf_export_screen.dart` awaits the old controller's export, then reads status from the controller field, which `didUpdateWidget` may have replaced. The new controller is idle, not failed, so the dialog closes while the new export is starting. No test covers `didUpdateWidget`. + ### Existing architecture decisions Do not reopen the Markdown renderer, shared slide assembly, state-management approach, AI provider/model split, or persistence format as side effects of this work. Existing maintainer decisions already cover several of these areas. @@ -60,7 +122,7 @@ Do not present this self-review as independent corroboration. A deep module is justified when it removes real caller obligations and concentrates a behavior rule. A new class or file is not evidence of increased depth. -Names such as `ActiveDeckPublication` should remain proposed homes for proven ownership rules until the smallest correct implementation is known. +Names such as `ActiveDeckPublication` should remain proposed homes for proven ownership rules until the smallest correct implementation is known. For publication, the second pass found that the smallest correct implementation is the existing `DeckLibraryController`; a new class is not needed. ### 3. Remove soft scheduling dependencies @@ -89,19 +151,20 @@ Record exact command, tested SHA, host, result, and exclusions for each implemen ### 6. Separate three work tracks **Focused regressions and fixes** -- publication handoff; -- reload completion; +- publication handoff, fixed inside the existing owner (`DeckLibraryController`), not in the widget lambda; +- reload completion and replay to late subscribers; - refresh ownership; -- thumbnail/artwork identity; +- thumbnail/artwork identity (needs a recorded finding first); - PDF cleanup and stale callbacks; -- writer exclusivity where supported calls justify it. +- writer exclusivity where supported calls justify it; +- delete the unread `DeckDocumentStore.revision`. **Conditional structural changes** -- neutral publication owner; - broader render-revision plumbing; -- private PDF capture host. +- private PDF capture host, or the single-use export run; +- one run-identity value across generation, Wizard, and application. -Choose these only when the focused fixes still leave material caller obligations. +Choose these only when the focused fixes still leave material caller obligations. The neutral publication owner moved out of this track: it passes the deletion test today. **Platform and release decisions** - supported macOS/Xcode combination; @@ -115,8 +178,8 @@ These require explicit maintainer decisions and should not silently expand into ## Revised first milestone 1. Establish the pinned baseline. -2. Add the saved-A/failed-B regression and the smallest passing fix. -3. Add the memory-reload terminal-result regression and the smallest passing fix. +2. Add the saved-A/failed-B regression and the smallest passing fix, inside `DeckLibraryController`. +3. Add the memory-reload terminal-result and late-subscriber regressions and the smallest passing fix. 4. Add PDF terminal-cleanup and stale-callback regressions and the smallest passing fixes. 5. Reassess whether enough coordination remains to justify the larger module shapes. 6. Record each structural decision alongside the change that implements it. @@ -135,9 +198,10 @@ The supplied review artifacts were checked for internal structure: - no unknown task/regression/source/PR references in task records; - no declared dependency cycles; - no unassigned regression rows; +- three tasks (`SD-32`, `SD-33`, `SD-35`) with no regression IDs; - no duplicate HTML element IDs or broken internal fragment links in the two checked HTML reports. -These checks do **not** prove test coverage, dependency necessity, runtime correctness, or that the proposed priorities are correct. +These checks do **not** prove test coverage, dependency necessity, runtime correctness, or that the proposed priorities are correct. The checked artifacts are not in the repository, so the counts cannot be re-run from it. See `superdeck_architecture_audit_checks.json` for the machine-readable results. @@ -149,12 +213,18 @@ See `superdeck_architecture_audit_checks.json` for the machine-readable results. - PDF public entry point: `packages/plugins/pdf/lib/superdeck_pdf.dart` - PDF plugin: `packages/plugins/pdf/lib/src/pdf_plugin.dart` - PDF controller: `packages/plugins/pdf/lib/src/pdf_controller.dart` +- PDF dialog: `packages/plugins/pdf/lib/src/pdf_export_screen.dart` - Wizard handoff: `packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart` +- Generated-result applier: `packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart` +- Library controller: `packages/playground/lib/features/library/domain/deck_library_controller.dart` - Document store: `packages/playground/lib/core/domain/stores/deck_document_store.dart` - Memory loader: `packages/playground/lib/core/data/data_sources/memory_deck_loader.dart` +- App root providers: `packages/playground/lib/app/providers.dart` - Runtime session state: `packages/superdeck/lib/src/deck/deck_session_state.dart` +- Retry entry point: `packages/superdeck/lib/src/deck/slide_page_content.dart` - Recorded renderer/assembly decisions: `docs/maintainers/renderer-and-assembly-evaluation.md` +- Mermaid decision: `docs/maintainers/adr/0001-runtime-mermaid-rendering.md` ## Final disposition -The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Apply the corrections above, require executed evidence for each behavior an implementation claims, and prefer the smallest change that removes a proven caller obligation. +The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Apply the corrections above, require executed evidence for each behavior an implementation claims, and prefer the smallest change that removes a proven caller obligation. The second pass adds one change of stance: deck publication already has a proven owner, so fix it there first. From d885cab6595a3188a4ba00e6ae35c00226a4cc82 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 12:18:15 -0500 Subject: [PATCH 05/11] docs: correct the adversarial architecture audit Make the Markdown the standalone record, remove the drifting HTML copy and the structural-check JSON, and narrow the publication, loader, PDF, and engine conclusions to what the reviewed source supports. --- ...erdeck_architecture_adversarial_audit.html | 87 ----- ...uperdeck_architecture_adversarial_audit.md | 300 +++++++++--------- .../superdeck_architecture_audit_checks.json | 39 --- 3 files changed, 153 insertions(+), 273 deletions(-) delete mode 100644 docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html delete mode 100644 docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html deleted file mode 100644 index fec3e5b2..00000000 --- a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.html +++ /dev/null @@ -1,87 +0,0 @@ - - - - - -SuperDeck — Adversarial architecture audit - - - -

SuperDeck / Architecture review / Audit addendum

-
-

SuperDeck architecture plan: adversarial audit

-

Decision: keep the main direction, but revise the execution plan before treating it as implementation-ready. The strongest work remains publication correctness, memory-loader completion, asset identity, and PDF lifecycle safety.

-

Repository: conceptadev/superdeck
-Reviewed baseline: main@d160766c862a4ea7670a41c98f085bfae33a24f7
-Status: source and plan audit; new application regressions were not executed.

- -
Purpose. This is a correction addendum to the earlier architecture review, action plan, and backlog. It checks the plan against the deep-module guidance and repository conventions. It does not turn the 36 planning items into 36 confirmed defects.
- -

What remains justified

-

Active-deck publication

-

The Wizard releases the saved deck before invoking the generated-result applier. Keep the publication finding and protect the saved-A/failed-B transition with a regression. The intended rule is to prepare the replacement completely, publish it only while it still owns the operation, and otherwise leave the previous deck coherent.

- -

Memory loader

-

MemoryDeckLoader.reload() is empty while runtime reload enters loading state. Fix the contract mismatch directly. Add replay handoff tests so subscription cannot lose, duplicate, or reorder current state.

- -

PDF lifecycle

-

Keep focused cleanup, cancellation, and stale-callback work. Narrow the architecture claim: the public PDF plugin interface is already small. A private capture-host extraction is conditional internal consolidation, not a required new application-facing abstraction.

- -

Corrections to the plan

-
    -
  1. Describe the method accurately. The earlier work was informed by improve-codebase-architecture and codebase-design, but did not execute the complete tool-driven sub-agent and grilling process. Behavioral verification remains pending.
  2. -
  3. Do not make proposed modules mandatory. A new owner is justified only when it removes a proven caller obligation and concentrates a behavior rule.
  4. -
  5. Remove soft scheduling dependencies. Publication handoff, refresh ownership, stale PDF callbacks, and writer serialization can be fixed independently unless implementation evidence shows a real prerequisite.
  6. -
  7. Tighten acceptance tests. Observe publication during notifications, test replay during subscriber attachment, and test old-asset lifetime while replacement cleanup runs.
  8. -
  9. Use scope-sensitive merge gates. Checks required by changed behavior must pass, or the maintainer must explicitly accept a documented limited exception. Existing green CI is baseline evidence only.
  10. -
  11. Separate fixes, structural proposals, and product decisions. Do not treat one backlog as one mandatory refactor program.
  12. -
- -

Work tracks

- - - - - - - -
TrackExamplesRule
Focused regressions and fixesPublication handoff, reload completion, refresh ownership, asset identity, PDF cleanup/callbacks, writer exclusivityWrite the regression, then make the smallest passing correction.
Conditional structural changesNeutral publication owner, broader render revision, private PDF capture hostProceed only when focused fixes still leave material caller obligations.
Platform and release decisionsmacOS/Xcode support, export limits, interruption guarantees, DCM, client credential policyRequire an explicit maintainer decision; do not silently broaden scope.
- -

Revised first milestone

-
    -
  1. Establish the pinned baseline.
  2. -
  3. Add saved-A/failed-B publication regression and minimal fix.
  4. -
  5. Add memory-reload terminal-result regression and minimal fix.
  6. -
  7. Add PDF terminal-cleanup and stale-callback regressions and minimal fixes.
  8. -
  9. Reassess whether remaining coordination justifies larger module shapes.
  10. -
  11. Record structural decisions with the changes that implement them.
  12. -
- -

Artifact-structure check

-

The supplied artifacts contain 36 work items, 9 proposed PR groups, 30 regression rows, and 44 source-register entries. The structural check found no duplicate task/regression IDs, unknown task references, dependency cycles, unassigned regression rows, duplicate HTML element IDs, or broken internal fragment links in the two checked HTML reports.

-

These checks do not prove runtime correctness, test coverage, dependency necessity, or priority correctness. See superdeck_architecture_audit_checks.json.

- -

Evidence and source paths

-
    -
  • Matt Pocock: improve-codebase-architecture and codebase-design skills.
  • -
  • Repository guidance: AGENTS.md.
  • -
  • PDF interface and lifecycle: packages/plugins/pdf/lib/superdeck_pdf.dart, pdf_plugin.dart, and pdf_controller.dart.
  • -
  • Publication handoff: packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart.
  • -
  • Document and loader state: deck_document_store.dart, memory_deck_loader.dart, and deck_session_state.dart.
  • -
  • Recorded decisions: docs/maintainers/renderer-and-assembly-evaluation.md.
  • -
- -

Final disposition

-

The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Require executed evidence for each behavior an implementation claims and prefer the smallest change that removes a proven caller obligation.

-
- diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md index 28b13bde..59ff6156 100644 --- a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md @@ -1,230 +1,236 @@ -# SuperDeck architecture plan: adversarial audit +# SuperDeck architecture audit -**Decision:** keep the main direction, but revise the execution plan before treating it as implementation-ready. +**Decision:** Investigate publication and saving first, then memory reload, then PDF terminal cleanup. Shared-module placement and late-subscriber replay stay open. This branch records those limits. It does not authorize a class merger. **Repository:** `conceptadev/superdeck` -**Reviewed baseline:** `main@d160766c862a4ea7670a41c98f085bfae33a24f7` -**Date:** 21 September 2026; second pass 22 September 2026 -**Status:** source and plan audit; new application regressions were not executed. +**Reviewed code:** `882a3209491f7162880718c61889d6c1d1b156d1` +**Code baseline:** `d160766c862a4ea7670a41c98f085bfae33a24f7` (the non-documentation tree of the reviewed commit) +**Date:** 22 September 2026 +**Status:** Standalone source review. The regressions named below were not executed in Flutter. -## Context +## Standalone record -This document is an adversarial second pass over the earlier SuperDeck architecture review, action plan, and backlog. It checks those artifacts against Matt Pocock's `improve-codebase-architecture` and `codebase-design` guidance, the repository's `AGENTS.md`, and selected implementation files. +This Markdown file is the standalone record of the audit. The earlier architecture review, the action plan, and the backlog are not in the repository. A temporary candidate report was written outside the repository and is not available here either. That absence limits how far a reader can reconstruct the skill's presentation steps. This document does not claim that a missing temporary report proves every skill step ran. -It is a correction addendum. It does not turn the earlier 36-item backlog into 36 confirmed defects or mandatory architecture changes. The earlier work used the skills' source guidance and vocabulary, but did not execute the complete tool-driven process described by the skill, including an independent sub-agent exploration pass and the full candidate grilling loop. +What follows is checked against the source at the reviewed commit: callers, implementation, and existing tests. Flutter, the application, and the test runner were not run. -**The earlier artifacts are not in this repository.** The review, action plan, and backlog that this document corrects were never committed on any branch. `superdeck_architecture_audit_checks.json` records their SHA-256 hashes, but a maintainer cannot open them. References below to backlog items, PR groups, and task IDs such as `SD-32` cannot be verified from the repository. Either commit those artifacts beside this document or treat the findings below as the standalone record. +## Domain -## Second pass: the skill's process, run against the code +Three decks are easy to treat as one object. They are not. -On 22 September 2026 the skill's process was run directly on this baseline: scope by commit history, read the ADR and recorded decisions, explore the code, apply the deletion test to each suspect module, and write the candidate report. The report was written outside the repository, as the skill requires. Two departures from the skill: the exploration was done without a sub-agent, by maintainer instruction, and the grilling loop was not started because it needs the maintainer's choice of candidate. +- A **generated result** is the `DeckGenerationResult` retained by `WizardGenerationController` after a generation attempt the controller kept. It holds that attempt's slides, artwork bytes, and theme. +- The **active runtime deck** is what the app is presenting: Markdown in `DeckDocumentStore`, slides in the deck session, theme in `DeckCustomizationStore`, and the artwork binding on `DeckLibraryAssetStore`. +- A **saved deck** is a named deck on disk, identified by `SavedDeckRef`: its Markdown file, its artwork files, and its theme manifest. -Terms below follow the `codebase-design` glossary: module, interface, implementation, depth, seam, adapter, leverage, locality. The repository has no `CONTEXT.md`, so domain terms come from the code's own doc comments: deck, generated deck, saved deck, runtime, document, preview, artwork, Wizard, library. +A correct save writes one of those as a single snapshot. A correct open or publication replaces the active runtime deck with one coherent deck. The defects below are places where those owners diverge. -### Candidates +The repository has no `CONTEXT.md`. These three names are the domain distinction this review needed. They are not a claim that a full glossary was produced. -| # | Candidate | Strength | Dependency category | -| --- | --- | --- | --- | -| 1 | Give deck publication one owner | Strong | in-process | -| 2 | Deepen the memory deck loader | Strong | in-process | -| 3 | One PDF export is one run | Worth exploring | local-substitutable | -| 4 | One notion of "is this run still current" | Speculative | in-process | +## How terms are used -**1. Give deck publication one owner.** Two publishers write the same four stores by hand: `GeneratedDeckResultApplier` writes the document, the preview loader, and the theme; `DeckLibraryController.open` writes those three and binds the artwork owner. The rule "unbind the saved deck's artwork only once the generated deck is published" lives in a lambda in `wizard_page.dart`, which runs before the applier can abandon the result. Deletion test: delete the applier and its staging, queue, and eviction logic reappears in the Wizard controller; delete the lambda and the invariant vanishes, which is the saved-A/failed-B defect. Both concentrate, so the module earns its keep now. `DeckLibraryController` already documents itself as the owner of "which deck's artwork the renderer resolves", so it is the natural home. No new class is required. ADR-0001 keeps deck-owned artwork separate from derived thumbnails; the deepened module owns artwork binding only. +Module, interface, depth, seam, and locality follow the `codebase-design` glossary, with these applications: -**2. Deepen the memory deck loader.** `MemoryDeckLoader` is about ten lines of implementation behind an interface that carries three hidden facts: subscribe before the first update or lose it (the eager-provider rule in `providers.dart`), `reload()` does nothing while `DeckSessionState.reload()` marks itself loading, and unchanged text never re-emits, so the retry button in `slide_page_content.dart` hangs. Deletion test: delete the loader and the three rules reappear in every publisher and in the app root. The deepening is for the loader to own its current state: late subscribers receive it, `reload()` re-emits it, and the ordering rule leaves the app root. No event bus and no new adapter. +- Depth is the leverage a caller gets, including ordering and failure behavior. A short list of method names in front of a long file is not a depth measurement. +- An interface is everything a caller must know. Leaving a declaration unchanged can still change observable behavior. +- A comment that describes current ownership, and a set of shared dependencies, record the current design. They do not by themselves choose the next home for a behavior. +- A function is a module. A helper-level test is not automatically the wrong seam. Removing one requires a replacement that still observes the guarantee. -**3. One PDF export is one run.** `PdfController` is reusable across exports, so `export()` resets run state before its `try` and nothing clears captured images in a `finally`. The dialog must dispose and recreate the controller when slides change, and `_handleExport` re-reads the controller field after its await, so it acts on whichever controller is current. Two `@visibleForTesting` members let tests reach past the interface. Capture already has two adapters, `SlideCaptureService` in production and a fake in tests, so that internal seam is real. The public interface of the plugin is two exports with one caller, so no public export-session seam is justified. +## 1. Publication and saving -**4. One notion of "is this run still current".** The generation engine is the hottest area in commit history and is already deep: four entry points on `DeckGeneratorService` with about 4,000 lines behind them in part files. Run ownership is expressed four ways: an operation epoch in the Wizard controller, `isCancelled` closures polled about fifty times in the engine, an `isValid` guard in the applier, and a `revision` counter on `DeckDocumentStore` that nothing reads outside its own test. Not proposed now. Revisit if candidates 1 and 2 leave the guard plumbing painful. +### Ordering defect, limited to result application -**Looked at, not proposed.** The Markdown renderer, slide assembly, and Mermaid are decided in `renderer-and-assembly-evaluation.md` and ADR-0001; no friction found that warrants reopening them. `DeckLibrary` has a four-method interface and writes each file through a temporary sibling and a rename; it is deep enough. `deck_generator_pipeline_helpers_test.dart` tests past the engine's interface and should fold into service-level tests when next touched. +`WizardGenerationController`'s result callback calls `DeckLibraryController.releaseOpenDeck()` and then `GeneratedDeckResultApplier.apply()` (`wizard_page.dart` 119–126). `releaseOpenDeck()` clears the open saved deck, the last-save notice, and the artwork binding (`deck_library_controller.dart` 181–186). It runs before the applier can accept, abandon, or throw. -**Top recommendation:** candidate 1. It passes the deletion test today and fixes a confirmed defect. Candidate 2 is the smallest change if a one-file first slice is wanted. +An ordinary planning or composition failure returns before that callback. `generateSlides()` checks cancellation, success or partial success, and non-empty slides, and only then calls `_completeComposition()` (`wizard_generation_controller.dart` 267–310). `_completeComposition()` is what enters the callback (`wizard_generation_controller.dart` 155–186). `retryFailedSlides()` uses the same completion path. The premature unbind is reached only when result application runs. From there the applier can reject the result, an asset write can fail, or `apply` can stop before publication. The applier does not restore the previous library binding. -### Size of the change +Consequences, at the reviewed source: -Estimates cover the focused slice of each candidate, with regressions. Lines are net source plus test lines. +- `releaseOpenDeck()` does not delete saved artwork files. It only forgets the binding. +- `DeckLibraryAssetStore.resolve()` consults the bound saved deck and otherwise the in-memory fallback (`deck_library_asset_store.dart` 34–42). Artwork still in memory can mask the missing binding. +- `ResolvedAssetImage` resolves again when its asset key or store changes (`resolved_asset_image.dart` 57–63). An image that already resolved can keep that URI. The store object itself stays stable across `bindTo`. +- Opening the saved deck again calls `open()`, which binds artwork once more. That is a later user operation. It is not rollback by the failed applier. -| Candidate | Source files | Test files | Lines | Public interface change | -| --- | --- | --- | --- | --- | -| 1. Publication owner | 4 (`wizard_page.dart`, `generated_deck_result_applier.dart`, `deck_library_controller.dart`, `deck_document_store.dart`) | 3 | about 250 | none outside `packages/playground` | -| 2. Memory loader | 2 (`memory_deck_loader.dart`, `providers.dart` comment) | 2 | about 120 | none; `DeckLoader` unchanged | -| 3. PDF run, focused fix | 2 (`pdf_controller.dart`, `pdf_export_screen.dart`) | 2 | about 80 | none; barrel unchanged | -| 3. PDF run, full restructure | 2 | 2 | about 300 | none | -| 4. Run identity | not estimated | | | | +User-visible impact is therefore conditional. A same-run generated deck, or an image that already resolved, can conceal the defect. The regression that shows it uses a saved deck whose artwork exists **only on disk**, then a failed or abandoned application, then a fresh asset resolution. -Total for the three focused slices: about 450 lines across eight source files and seven test files, all inside `packages/playground` and `packages/plugins/pdf`. No package in `packages/superdeck` or `packages/core` changes its interface. Each slice is independent and can ship as its own PR. +### The deletion test does not choose a class -### Findings the first pass missed +Deleting the applier would spread staging, sequencing, and cleanup into its caller. That is evidence that the applier is doing real work. It is not evidence that those responsibilities belong on `DeckLibraryController`. -- `DeckDocumentStore.revision` has no reader outside its own test. It is interface with no leverage. Delete it with candidate 1. -- The subscribe-before-publish rule leaks into the app root. The eager-provider comment in `packages/playground/lib/app/providers.dart` is a loader invariant living in the wrong module. Candidate 2 removes the need for it. -- Two test-only members on `PdfController` expose internal seams to tests. The skill's deepening notes say not to do this. Candidate 3 deletes them. -- `DeckLibraryController.refresh()` lacks the busy guard that `save()` and `open()` have, so it can interleave with them. Candidate 1's owner absorbs this fix. -- The first pass did not say whether it checked the commit-history hot spot. The second pass did: the generation engine is deep and needs no candidate. +Deleting the callback's `releaseOpenDeck()` call would stop the premature unbind. On a successful publication the previous saved-deck binding could remain active, so the old deck would still answer artwork lookups. That is a different failure from the failed-application case. The comparison shows an ordering obligation: change the saved-deck binding at an accepted publication point. It does not name a unique class to own that obligation. -### Corrections to this document's first pass +`DeckLibraryController` currently combines library operations and runtime publication. Its class comment describes that current design. Adding generation-specific staging, result interpretation, cancellation, and eviction would couple library management to generation further. The comment is not a reason to do that. -- The "Memory loader" section below speaks of replay behavior. Replay to late subscribers is the proposed deepening, not existing behavior. The loader has no replay today. -- "Thumbnail/artwork identity" appears in the fix track below with no finding section that explains it. It stays listed, but it needs a recorded finding before work starts. -- The first pass rated a neutral publication owner as conditional. The deletion test says the owner earns its keep now. The correction is in "Separate three work tracks" below. -- The rendered HTML copy beside this file is a second source of truth and drifts from it. The skill writes its report to a temporary directory so nothing lands in the repository. The HTML has not been updated with the second pass and should be removed. +Shared-module placement stays unresolved. Compare a small shared in-process publication operation with a prepared-deck operation on the existing controller. A new class is not required by the evidence. A new class is not forbidden by the evidence. -## Findings that remain justified +### Acceptance scenarios for one coherent deck -### Active-deck publication +These three scenarios are the ownership contract. Moving the same setters into another class does not satisfy them. -The Wizard releases the open saved deck before invoking the generated-result applier. That leaves a correctness-sensitive ownership rule in the caller. Keep the publication finding and add a regression for saved deck A followed by a failed or invalidated generated replacement B. +**A saved result must not be mixed with another document.** `_WizardExperienceState._save()` takes images and theme from the retained `controller.result` (`wizard_page.dart` 161–182). `DeckLibraryController.save()` takes Markdown from the current global document (`deck_library_controller.dart` 102–123). Nothing checks that those inputs describe the same deck. The completed Wizard can push `/decks` (`wizard_page.dart` 249). The saved-decks button does the same while any stage is showing (`wizard_page.dart` 284–291). `open()` replaces the global document. The Wizard route stays under that push, so returning to it keeps the generated result. A save can then write deck B's Markdown with result A's artwork and theme. The storage writer persists the images it is given. It does not check that they match every reference in the Markdown. -The desired invariant is: prepare a replacement completely, publish it only while it still owns the operation, and otherwise leave the previous deck coherent. +Validation: generate A, open saved B, return to the Wizard, and save. The Markdown, artwork references, bytes, and theme must all belong to the deck the save action claims to save. This navigation path is established from the route and call structure. It was not executed in a running app. -Verified on the baseline: `wizard_page.dart` line 123 calls `releaseOpenDeck()` before `apply()`. If the applier abandons or throws, deck A's text stays in the document store, but its artwork now resolves through the in-memory fallback and the "Save another copy" state is lost. +**A newer saved-deck selection must survive an older generation.** The saved-decks button stays available during generation and does not cancel the Wizard operation (`wizard_page.dart` 284–291). The completion guard checks the Wizard operation epoch, cancellation, and disposal (`wizard_generation_controller.dart` 155–186). It does not check library selection or active-deck identity. Starting generation B, opening saved A, and then letting B finish can publish B over A. -### Memory loader +Validation: hold generation at a deterministic gate, open A, release B, and assert that B does not silently replace A. Define what supersedes publication: either selecting another deck removes the earlier run's right to replace the runtime, or the result stays available for an explicit later acceptance. -`MemoryDeckLoader.reload()` is empty while runtime reload sets loading state to true. This is a direct contract mismatch. Fix it without inventing a generic event bus or changing every loader adapter. +**A malformed open must not mix the previous deck with the new one.** `open()` sets the open deck, binds artwork, and replaces Markdown before `updateMarkdown()` (`deck_library_controller.dart` 162–169). `MemoryDeckLoader.updateMarkdown()` catches a decode failure and emits an error (`memory_deck_loader.dart` 18–27). `open()` still returns success. When the session already has slides, `DeckSessionState` keeps those slides and records the error as a build failure (`deck_session_state.dart` 63–74). The visible slides can remain deck A's while the document and artwork binding are deck B's. This path does not need a generated result. -If the fix adds replay to late subscribers, which the second pass recommends, it needs tests around the subscription handoff: no lost update, no duplicate replay to existing listeners, and no stale snapshot after a newer state. +Validation: open valid A, attempt malformed B, and inspect the returned outcome together with the document, the artwork binding, the theme, and the visible slides. Failure must not leave a mixed deck. Last-good preview behavior has to keep the document identity, asset view, and theme that belong with those slides. -### PDF lifecycle +The ordering regression belongs on the same contract: after a saved deck is showing, a failed or abandoned application must leave that deck's document, artwork binding, and slides together, including when the artwork exists only on disk and is resolved again. -Keep the focused PDF lifecycle work. The inspected export path has no terminal `finally` cleanup for captured PNG references and mutates shared run state before its `try` block. +### `refresh()` busy guard, independent of publication -However, narrow the architecture claim. The package's supported application-facing interface is already small: the public barrel exports the plugin and options, and the plugin opens the internal dialog. Application callers do not assemble the PageView, capture keys, and readiness scopes. +`save()` returns immediately when `_isBusy` is set (`deck_library_controller.dart` 107). `open()` does the same (`deck_library_controller.dart` 157). `refresh()` does not (`deck_library_controller.dart` 137–150), and it still sets and clears that flag. A refresh can overlap an open or a save, finish first, and mark the controller idle while the other operation is still running. A later operation can then start, and an older list or error can overwrite newer state. -Therefore: -- keep focused cleanup, cancellation, and stale-callback fixes; -- treat a private capture-host extraction as conditional; -- do not add a public export-session abstraction merely to satisfy deep-module terminology. +Severity is medium. Confidence is high from the source. A guard, or explicit operation ownership, is a focused correction. It does not depend on moving generated publication into this controller. Test controlled completion ordering on its own. -The stale-callback defect, made concrete: `_handleExport` in `pdf_export_screen.dart` awaits the old controller's export, then reads status from the controller field, which `didUpdateWidget` may have replaced. The new controller is idle, not failed, so the dialog closes while the new export is starting. No test covers `didUpdateWidget`. +### `DeckDocumentStore.revision` is unused, and it is the wrong identity -### Existing architecture decisions +The counter and its getter remain at `deck_document_store.dart` 9–26. The comment says a long-running operation captures the value before publishing. The only reader in this tree is `deck_document_store_test.dart`. The publication path does not read it. -Do not reopen the Markdown renderer, shared slide assembly, state-management approach, AI provider/model split, or persistence format as side effects of this work. Existing maintainer decisions already cover several of these areas. +The counter is unused today. That does not mean publication needs no identity guard. The cross-deck cases above are the reason to settle identity before deleting anything. `replaceMarkdown()` also ignores identical text, so the counter would not move for two decks that share Markdown and differ in artwork or theme. Identical Markdown is not enough to identify those decks. -## Corrections to the execution plan +Remove the unused counter later, as its own cleanup, or replace it with the identity the operation actually needs. Do not prescribe automatic deletion from the lack of a reader, and do not infer safety from its absence. -### 1. Describe the method accurately +### First implementation slice -Call the earlier work a **source-reviewed architecture plan informed by `improve-codebase-architecture` and `codebase-design`; implementation and behavioral verification pending**. +Start with coherent active-deck publication and saving. The first regressions are the three scenarios above, plus failed or abandoned application of a generated result. The aim is that a publish or a save names one deck and writes or shows only that deck. -Each claim should distinguish: -- inspected source; -- inferred consequence; -- executed check; -- proposed design. +Keep the generation pipeline, asset staging, the library persistence adapter, the renderer, and the PDF plugin interface unless one of those regressions requires a change. Fix memory reload on its own next. Address PDF terminal cleanup without treating controller replacement as the reason for a larger redesign. -Do not present this self-review as independent corroboration. +## 2. Memory loader -### 2. Do not make every proposed module mandatory +### Retry reaches a reload that never finishes -A deep module is justified when it removes real caller obligations and concentrates a behavior rule. A new class or file is not evidence of increased depth. +`MemoryDeckLoader.updateMarkdown()` records the input before decoding. A decode failure emits `SlidesErrorEvent` (`memory_deck_loader.dart` 18–27). `load()` returns that broadcast stream. `reload()` returns immediately and emits nothing (`memory_deck_loader.dart` 31–34). -Names such as `ActiveDeckPublication` should remain proposed homes for proven ownership rules until the smallest correct implementation is known. For publication, the second pass found that the smallest correct implementation is the existing `DeckLibraryController`; a new class is not needed. +`DeckSessionState.reload()` clears the error, sets loading, and awaits the loader (`deck_session_state.dart` 78–87). `SlidePageContent` connects the fatal-error Retry action to `DeckController.reloadDeck()` (`slide_page_content.dart` 34–38), which calls that session reload. The empty future completes. The loading signal stays set until some other publication emits an event. -### 3. Remove soft scheduling dependencies +That path is reachable without an editor. `MacOsDeckLibrary.open()` reads saved Markdown as text and does not parse it (`mac_os_deck_library.dart` 409–428). `DeckLibraryController.open()` passes that text to the memory loader and returns success even when decoding emits an error. `SavedDecksPage._open()` then enters presentation (`saved_decks_page.dart` 33–38). `DeckPresenter` builds `SlidePageContent` (`deck_presenter.dart` 51–61) and does not bypass the error screen. -Several backlog dependencies describe a preferred order rather than a hard prerequisite. In particular: -- the saved-to-generated handoff regression need not wait for full memory replay work; -- refresh operation ownership can be fixed independently; -- stale PDF callbacks can be guarded in the current dialog before a session redesign; -- writer serialization can be tested and fixed inside the current writer; -- ADRs and domain terms should be written with the decision they record, not postponed until the end; -- obsolete coordination should be removed in the PR that proves its replacement. +A malformed **first** saved deck can reach Retry, because fatal error requires that no slides have loaded (`deck_session_state.dart` 36–38 and 63–74). After a successful load, a later decode error is a build failure and the last slides stay. Severity is medium. Confidence is high from the call path. -### 4. Tighten acceptance tests +### Required fix: reload completion -**Publication:** observe state during publication, not only after the overall future completes. A method that calls several existing setters is not automatically atomic. +An explicit reload must produce a terminal result when the Markdown is unchanged, and it must define the no-input case, where no Markdown has been supplied. Ordinary `updateMarkdown()` deduplication can stay: identical text still does not need a new preview event on the update path. -**Replay:** test an update that occurs while a subscriber attaches. Also specify the behavior for valid deck A followed by an invalid update and then a new subscriber. +Re-emitting the last error does not reread a corrected file from disk. Refreshing a saved deck that changed on disk remains a library-open concern. -**Asset lifetime:** block a capture of an old revision, publish a replacement, run cleanup, then prove either that the old capture keeps its own asset view or that it is explicitly cancelled before saving. Verify eventual release. +### Late-subscriber replay is optional -### 5. Make merge gates scope-sensitive +The production app creates `DeckController` eagerly, before publication (`providers.dart` 17–23, `lazy: false` at 64–77). Replay could remove the "subscribe before the first event" obligation. It would also decide latest error versus last-good content, subscription handoff order, duplicate delivery, and what a new session observes after a valid deck followed by an invalid update. Those decisions need their own consumer and tests. -Checks required for behavior changed by a PR must pass. A relevant blocked check remains an unverified guarantee unless the maintainer explicitly accepts a documented, limited exception. +Replay is not required to fix the retry defect. It is not a decision this review treats as proved. -Record exact command, tested SHA, host, result, and exclusions for each implemented change. Existing green CI is baseline evidence, not proof of newly proposed race scenarios. +The eager-provider comment gives two reasons: the loader subscription, and seeding the initial `DeckOptions`. Replay addresses the subscription reason. It does not by itself remove eager `DeckOptions` seeding. -### 6. Separate three work tracks +`BundledDeckLoader` and `FileDeckLoader` use their own controllers and reload cycles. A change confined to `MemoryDeckLoader` can leave them alone. The `DeckLoader` type promises `load()`, `reload()`, and `dispose()`. It does not promise replay or broadcast delivery. Imposing either on every loader would be a separate contract change. -**Focused regressions and fixes** -- publication handoff, fixed inside the existing owner (`DeckLibraryController`), not in the widget lambda; -- reload completion and replay to late subscribers; -- refresh ownership; -- thumbnail/artwork identity (needs a recorded finding first); -- PDF cleanup and stale callbacks; -- writer exclusivity where supported calls justify it; -- delete the unread `DeckDocumentStore.revision`. +## 3. PDF lifecycle -**Conditional structural changes** -- broader render-revision plumbing; -- private PDF capture host, or the single-use export run; -- one run-identity value across generation, Wizard, and application. +### Entry contract and terminal image release -Choose these only when the focused fixes still leave material caller obligations. The neutral publication owner moved out of this track: it passes the deletion test today. +`PdfController.export()` clears cancellation, counters, the error, the captured-image list, and status before `try` (`pdf_controller.dart` 264–271). That method has no guard against use after disposal or against overlapping exports. An explicit lifecycle contract is warranted: either the controller is single-use, or entry refuses reuse and overlap. Moving the assignments inside `try` does not by itself establish safe run ownership. -**Platform and release decisions** -- supported macOS/Xcode combination; -- export limits; -- interruption guarantees; -- DCM setup and debt work; -- distributed-client credential policy. +There is no `finally` that clears `_images`. A failure after some slides are captured, and before PDF assembly, retains those PNG references while the failure dialog stays open. `dispose()` does not clear the list either (`pdf_controller.dart` 363–375). -These require explicit maintainer decisions and should not silently expand into unrelated rewrites. +Successful assembly does clear the list. `_buildPdfForCurrentPlatform()` copies or transfers the images and then clears `_images` on both the web and native paths (`pdf_controller.dart` 334–347). Captured images are released on the success path. This is not an unbounded leak after the controller becomes unreachable. -## Revised first milestone +The smallest fix is the entry contract above, plus releasing per-run image references on every terminal path, including failure and cancellation. The regression captures slide one, fails or cancels before the complete PDF is assembled, and checks that the controller no longer retains those bytes. Existing tests cover the normal handoff of images into PDF assembly and disposal during a pending save. Those guarantees stay. -1. Establish the pinned baseline. -2. Add the saved-A/failed-B regression and the smallest passing fix, inside `DeckLibraryController`. -3. Add the memory-reload terminal-result and late-subscriber regressions and the smallest passing fix. -4. Add PDF terminal-cleanup and stale-callback regressions and the smallest passing fixes. -5. Reassess whether enough coordination remains to justify the larger module shapes. -6. Record each structural decision alongside the change that implements it. +### Controller replacement is a latent hazard -These slices can progress independently unless implementation evidence shows a real shared prerequisite. +`_handleExport()` awaits `_exportController.export()` and then reads `_exportController` again (`pdf_export_screen.dart` 102–109). `didUpdateWidget()` disposes that controller and creates another when the widget's slide list changes (`pdf_export_screen.dart` 125–131). A caller that rebuilds the dialog with a different list could observe the new controller, which has not failed, and close the dialog while a new export is starting. -## Structural artifact check +`PdfExportDialogScreen.show()` reads the slide list once (`pdf_export_screen.dart` 34–68). Both the shell-modal builder and the dialog builder close over that same list. Later deck updates do not pass a new list into the screen. The production plugin calls `show()` (`pdf_plugin.dart` 27–28). The supported invocation path does not produce the replacement. Treat the callback as a **latent replacement hazard**, not as a demonstrated application defect. -The supplied review artifacts were checked for internal structure: +A local check that the completing controller is still the controller the callback started is small defensive work. Making the dialog explicitly snapshot-based, and removing replacement behavior the supported path does not use, is the other small option. Neither option requires a new public export-session module. The public plugin surface stays the plugin and its options. -- 36 work items; -- 9 proposed PR groups; -- 30 regression rows; -- 44 source-register entries; -- no duplicate task or regression IDs; -- no unknown task/regression/source/PR references in task records; -- no declared dependency cycles; -- no unassigned regression rows; -- three tasks (`SD-32`, `SD-33`, `SD-35`) with no regression IDs; -- no duplicate HTML element IDs or broken internal fragment links in the two checked HTML reports. +### Testing hooks -These checks do **not** prove test coverage, dependency necessity, runtime correctness, or that the proposed priorities are correct. The checked artifacts are not in the repository, so the counts cannot be re-run from it. +`capturedImageCountForTesting` (`pdf_controller.dart` 101–102) is how tests see whether captured bytes are still held. The cleanup requirement above needs that observation, or an equivalent one. Do not delete this hook without a replacement retention check. -See `superdeck_architecture_audit_checks.json` for the machine-readable results. +`waitForRenderBoundaryPaint` (`pdf_controller.dart` 136–138) is a different seam. Paint-wait tests that count fake context and render-object reads are coupled to the wait loop. Export-level waiting and timeout tests are a better observation of the same user-facing behavior. Replace that hook when the export-level tests exist. The presence of a larger export module is not a reason to drop either check first. + +## 4. Generation engine + +### Size at the reviewed tree + +Physical line counts, including comments and blanks, for the files named here: + +| File | Lines | +| --- | ---: | +| `deck_generator_service.dart` | 818 | +| `deck_generator_pipeline.dart` | 973 | +| `deck_generator_images.dart` | 285 | +| `deck_plan_repair.dart` | 246 | +| `deck_generator_workflow.dart` | 239 | +| **Total** | **2,561** | + +A larger total would have to name more files. This review does not. + +### What callers actually depend on + +`plan`, `generateFromPlan`, `retryFailedSlides`, and `generate` are the four principal methods on `DeckGeneratorService`. Callers also depend on model choice, catalogs, budgets, timeouts, injected providers, partial-result semantics, and cancellation. Part files declare further public surface. Those obligations are part of the interface. Line count and the number of principal methods do not measure depth, and they do not justify a split. + +`_withModelSession()` centralizes client setup and disposal. Generation hides schema preparation, provider requests, repair, and finalization. That work stays. The files being long is not a reason to split the engine. + +### Approved-plan editing is worth exploring + +`WizardGenerationController.updateSlide()` rebuilds a plan and calls `validateDeckPlanIssues()` with the typography, image-style, and theme catalogs taken from the generation service (`wizard_generation_controller.dart` 230–267). The Wizard therefore knows how to assemble the engine's validation context. Concentrating approved-plan editing and validation, so the caller does not repeat that catalog knowledge, is worth exploring. It is narrower than restructuring the engine or inventing a run-identity type for every operation. + +Keeping the caller obligation is acceptable until that extraction has a concrete consumer. This review does not require it for the publication slice. + +### Change history is not a hotspot ranking + +`deck_generator_service.dart` changed on 18 July 2026, 7 September 2026, and 8 September 2026, among other dates. Those commits show continued work in the engine. This review did not define a history window, a path set, or a counting method, so it does not rank the engine as the hottest area. A keep decision for the pipeline does not need that ranking. The weak argument is using file size, or an undefined hotspot, as the reason to rewrite it. + +## 5. Conditional size forecast + +The figures below are a rough forecast of a **narrow** reading of each slice. They add as 250 + 120 + 80 = 450. That addition is not a measure of implementation effort, verification scope, or proof that a design is already the smallest correct one. The earlier estimate's eight source files and seven test files were not a locked file list. The test files were unnamed. They are not repeated here as scope. + +| Slice | Forecast | Included | Excluded | +| --- | ---: | --- | --- | +| Publication | about 250 | A narrow change that moves the saved-deck binding to an accepted publication point, with a regression for failed application | Coherent snapshots, cross-publisher invalidation, and observation of publication during notifications. `WizardGenerationController` and the asset-store integration are outside the old four-file list and may be required by the scenarios in section 1 | +| Loader | about 120 | Reload completion, including unchanged Markdown and the no-input case, with tests | Late-subscriber replay. Folding replay into this number treats an optional contract change as part of the retry fix | +| PDF | about 80 | An entry guard, terminal release of per-run images, a local callback guard, and their regressions | A new run module, a public export-session type, and deletion of the captured-image hook | + +A reasonable constraint is that these focused fixes do not require Dart type changes outside `packages/playground` and `packages/plugins/pdf`. Unchanged declarations are a different claim from an unchanged interface. Reload completion can stay inside `MemoryDeckLoader` while `DeckLoader`'s declarations stay as they are. Replay would change what a new subscriber observes even if those declarations stayed the same. The publication scenarios can require the Wizard controller to learn about the active deck without a new public type, and they can equally wait on a small shared operation. The forecast does not choose. + +## 6. Left in place + +This review does not reopen the Markdown renderer, shared slide assembly, the state-management approach, the AI provider and model split, or the persistence format. `docs/maintainers/renderer-and-assembly-evaluation.md` and `docs/maintainers/adr/0001-runtime-mermaid-rendering.md` already record the renderer and Mermaid decisions. + +The library persistence adapter, asset staging, and the PDF plugin's public barrel stay as they are unless a regression in sections 1–3 requires a change. ## Evidence - Matt Pocock, `improve-codebase-architecture`: https://github.com/mattpocock/skills/blob/main/skills/engineering/improve-codebase-architecture/SKILL.md - Matt Pocock, `codebase-design`: https://github.com/mattpocock/skills/blob/main/skills/engineering/codebase-design/SKILL.md -- SuperDeck repository guidance: `AGENTS.md` -- PDF public entry point: `packages/plugins/pdf/lib/superdeck_pdf.dart` -- PDF plugin: `packages/plugins/pdf/lib/src/pdf_plugin.dart` -- PDF controller: `packages/plugins/pdf/lib/src/pdf_controller.dart` -- PDF dialog: `packages/plugins/pdf/lib/src/pdf_export_screen.dart` -- Wizard handoff: `packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart` +- Repository guidance: `AGENTS.md` +- Wizard handoff and save: `packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart` +- Wizard completion and outline edit: `packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart` - Generated-result applier: `packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart` - Library controller: `packages/playground/lib/features/library/domain/deck_library_controller.dart` +- Saved-deck open page: `packages/playground/lib/features/library/presentation/saved_decks_page.dart` +- macOS library read: `packages/playground/lib/features/library/data/mac_os_deck_library.dart` +- Artwork binding: `packages/playground/lib/core/data/data_sources/deck_library_asset_store.dart` - Document store: `packages/playground/lib/core/domain/stores/deck_document_store.dart` - Memory loader: `packages/playground/lib/core/data/data_sources/memory_deck_loader.dart` -- App root providers: `packages/playground/lib/app/providers.dart` -- Runtime session state: `packages/superdeck/lib/src/deck/deck_session_state.dart` -- Retry entry point: `packages/superdeck/lib/src/deck/slide_page_content.dart` -- Recorded renderer/assembly decisions: `docs/maintainers/renderer-and-assembly-evaluation.md` -- Mermaid decision: `docs/maintainers/adr/0001-runtime-mermaid-rendering.md` +- App providers: `packages/playground/lib/app/providers.dart` +- Session state: `packages/superdeck/lib/src/deck/deck_session_state.dart` +- Retry screen: `packages/superdeck/lib/src/deck/slide_page_content.dart` +- Presenter: `packages/superdeck/lib/src/ui/deck_presenter.dart` +- Resolved images: `packages/superdeck/lib/src/ui/widgets/resolved_asset_image.dart` +- PDF plugin: `packages/plugins/pdf/lib/src/pdf_plugin.dart` +- PDF dialog: `packages/plugins/pdf/lib/src/pdf_export_screen.dart` +- PDF controller: `packages/plugins/pdf/lib/src/pdf_controller.dart` +- Generation service and parts: `packages/playground/lib/features/ai/quick_agent/core/engine/services/` + +## Disposition -## Final disposition +Publication ordering is a real defect with a conditional user-visible effect, and the three cross-deck scenarios are the stronger reason to start there. The first code change should make a publish and a save refer to one deck. It should not start from a merger into `DeckLibraryController`. -The architecture direction survives the adversarial review. Blanket approval of the original 36-item execution plan does not. Apply the corrections above, require executed evidence for each behavior an implementation claims, and prefer the smallest change that removes a proven caller obligation. The second pass adds one change of stance: deck publication already has a proven owner, so fix it there first. +Memory reload must finish, including when the input did not change and when there is no input. Replay waits for its own need. PDF export needs an entry contract and terminal release of captured images. The replacement callback stays a latent hazard of an unsupported rebuild. The generation engine stays intact. Approved-plan validation is the only engine extraction this review marks as worth exploring. diff --git a/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json b/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json deleted file mode 100644 index 63111394..00000000 --- a/docs/maintainers/architecture-review/superdeck_architecture_audit_checks.json +++ /dev/null @@ -1,39 +0,0 @@ -{ - "check_scope": "Artifact structure only. Not Dart analysis, Flutter tests, link HTTP checks, or UI validation.", - "repository": "conceptadev/superdeck", - "reviewed_sha": "d160766c862a4ea7670a41c98f085bfae33a24f7", - "input_sha256": { - "superdeck_architecture_review.md": "1765d7de23ef6f04082ff0768999229fd39dfb4502c3d1657b21c2a6a49149a7", - "superdeck_architecture_review.html": "484da210666ad845477d3f35c257609a1299bfde2e54c57cbb7dc698fb597fc8", - "superdeck_architecture_action_plan.md": "2cb561662165608281a7d4e3e7097927392907a038cd732c81165d2a80ee3bb9", - "superdeck_architecture_action_plan.html": "b672ba6ff75e2056d0eb6b77d576fefd6c46e24b1c989b39b1943f38c1a6cd58", - "superdeck_architecture_backlog.json": "2accbee68528eacbd7e30e988396ac2eccb9ea163aed8c708c29100fa8fd3cec" - }, - "counts": { - "tasks": 36, - "proposed_pr_groups": 9, - "regression_rows": 30, - "source_entries": 44 - }, - "duplicate_task_ids": [], - "duplicate_regression_ids": [], - "unknown_references": [], - "dependency_cycles": [], - "unassigned_regressions": [], - "tasks_without_regression_ids": [ - "SD-32", - "SD-33", - "SD-35" - ], - "html_checks": { - "superdeck_architecture_review.html": { - "duplicate_ids": [], - "broken_fragment_links": [] - }, - "superdeck_architecture_action_plan.html": { - "duplicate_ids": [], - "broken_fragment_links": [] - } - }, - "semantic_caveat": "Acyclic references do not establish necessary dependencies, correct priorities, or correct behavior." -} From d0f7bcce7fe8b472fcba201f8d99d30a6ecc23d8 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 12:20:03 -0500 Subject: [PATCH 06/11] docs: name ownership guarantees as the first audit slice State that the publication work starts from those guarantees rather than a class merger. --- .../superdeck_architecture_adversarial_audit.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md index 59ff6156..7fd13c41 100644 --- a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md @@ -96,7 +96,7 @@ Remove the unused counter later, as its own cleanup, or replace it with the iden ### First implementation slice -Start with coherent active-deck publication and saving. The first regressions are the three scenarios above, plus failed or abandoned application of a generated result. The aim is that a publish or a save names one deck and writes or shows only that deck. +Start with coherent active-deck publication and saving. The recommended first slice is those ownership guarantees, not a class merger. The first regressions are the three scenarios above, plus failed or abandoned application of a generated result. The aim is that a publish or a save names one deck and writes or shows only that deck. Keep the generation pipeline, asset staging, the library persistence adapter, the renderer, and the PDF plugin interface unless one of those regressions requires a change. Fix memory reload on its own next. Address PDF terminal cleanup without treating controller replacement as the reason for a larger redesign. From 1e56b434ca9d039f5dd68505817cb10ab9d0038a Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 20:22:16 -0500 Subject: [PATCH 07/11] fix(playground): keep saved-deck ownership consistent across generate, save, and open Opening a saved deck now withdraws an in-flight generation's automatic publication; the retained result can still be shown or saved explicitly, and a save writes that result's Markdown, artwork, and theme together. The open saved deck is released only when a generated deck is accepted. Overlapping save, open, and refresh calls can no longer overwrite newer state, an open switches only after its Markdown decodes, and MemoryDeckLoader.reload re-emits the last terminal result. --- .../data/data_sources/memory_deck_loader.dart | 50 +- .../domain/generated_deck_result_applier.dart | 26 +- .../wizard_generation_controller.dart | 64 +- .../wizard_generation_status.dart | 11 + .../ai/wizard/presentation/wizard_page.dart | 64 +- .../domain/deck_library_controller.dart | 133 +++- .../data_sources/memory_deck_loader_test.dart | 171 ++++ .../library/domain/deck_ownership_test.dart | 740 ++++++++++++++++++ .../test/helpers/fake_deck_library.dart | 14 + 9 files changed, 1209 insertions(+), 64 deletions(-) create mode 100644 packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart create mode 100644 packages/playground/test/features/library/domain/deck_ownership_test.dart diff --git a/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart b/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart index 2fa7b515..4b7bc427 100644 --- a/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart +++ b/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart @@ -12,27 +12,61 @@ class MemoryDeckLoader extends DeckLoader { final _controller = StreamController.broadcast(); bool _disposed = false; + bool _hasMarkdown = false; String? _lastLoadedMarkdown; + List? _loadedSlides; + Object? _loadError; + + void _emitDecoded(String markdown) { + try { + final slides = _codec.decode(markdown); + _loadedSlides = slides; + _loadError = null; + _controller.add(SlidesLoadedEvent(slides)); + } catch (error) { + _loadedSlides = null; + _loadError = error; + _controller.add(SlidesErrorEvent('$error', error: error)); + } + } /// Parses the given markdown and emits a [SlidesLoadedEvent]. void updateMarkdown(String markdown) { if (_disposed || markdown == _lastLoadedMarkdown) return; _lastLoadedMarkdown = markdown; + _hasMarkdown = true; + _emitDecoded(markdown); + } - try { - final slides = _codec.decode(markdown); - _controller.add(SlidesLoadedEvent(slides)); - } catch (e) { - _controller.add(SlidesErrorEvent('$e', error: e)); + /// Re-emits the last terminal preview result. + /// + /// Identical text on [updateMarkdown] stays quiet. Reload does not read a + /// saved deck from disk; a file that changed on disk is opened again through + /// the library. + @override + Future reload() async { + if (_disposed) return; + if (!_hasMarkdown) { + _controller.add(SlidesErrorEvent('No markdown has been supplied.')); + await Future.delayed(Duration.zero); + + return; } + final error = _loadError; + if (error != null) { + _controller.add(SlidesErrorEvent('$error', error: error)); + } else { + _controller.add(SlidesLoadedEvent(_loadedSlides ?? const [])); + } + // Broadcast delivery is a later microtask. Stay incomplete until that + // event has reached listeners, including a session waiting to leave + // loading. + await Future.delayed(Duration.zero); } @override Stream load() => _controller.stream; - @override - Future reload() async {} - @override Future dispose() async { _disposed = true; diff --git a/packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart b/packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart index d415b813..3de18fed 100644 --- a/packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart +++ b/packages/playground/lib/features/ai/quick_agent/domain/generated_deck_result_applier.dart @@ -70,8 +70,9 @@ final class GeneratedDeckResultApplier { Future _apply( DeckGenerationResult result, - GeneratedDeckApplicationGuard isValid, - ) async { + GeneratedDeckApplicationGuard isValid, { + void Function()? onAccepted, + }) async { const abandoned = GeneratedDeckApplication(published: false); if (!isValid()) return abandoned; @@ -107,6 +108,11 @@ final class GeneratedDeckResultApplier { return abandoned; } + // The saved-deck binding changes only once this attempt is accepted. + // A rejection, an abandoned write, or a throw leaves the previous deck + // answering for its own artwork. + onAccepted?.call(); + final markdown = const SlideSerializer().serialize(result.slides); _documentStore.replaceMarkdown(markdown); _deckLoader?.updateMarkdown(markdown); @@ -149,13 +155,16 @@ final class GeneratedDeckResultApplier { Future apply( DeckGenerationResult result, { required GeneratedDeckApplicationGuard isValid, + void Function()? onAccepted, }) { final application = Completer(); // The queue only sequences the work. Failures reach the caller through // the completer, so one failed application cannot block the next one. _queue = _queue.then((_) async { try { - application.complete(await _apply(result, isValid)); + application.complete( + await _apply(result, isValid, onAccepted: onAccepted), + ); } catch (error, stackTrace) { application.completeError(error, stackTrace); } @@ -164,3 +173,14 @@ final class GeneratedDeckResultApplier { return application.future; } } + +/// Publishes [result] and releases the open saved deck only if that +/// publication is accepted. +Future applyGeneratedDeckResult({ + required GeneratedDeckResultApplier applier, + required DeckGenerationResult result, + required GeneratedDeckApplicationGuard isValid, + required void Function() releaseSavedDeck, +}) { + return applier.apply(result, isValid: isValid, onAccepted: releaseSavedDeck); +} diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart index fde0d939..b857eb5e 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart @@ -38,6 +38,7 @@ final class WizardGenerationController extends ChangeNotifier { final DeckGeneratorService _service; final ApplyWizardDeckResult _applyResult; + final int Function() _deckSelectionEpoch; WizardGenerationStage _stage = .setup; @@ -57,8 +58,10 @@ final class WizardGenerationController extends ChangeNotifier { WizardGenerationController({ required DeckGeneratorService service, required ApplyWizardDeckResult applyResult, + int Function()? deckSelectionEpoch, }) : _service = service, - _applyResult = applyResult; + _applyResult = applyResult, + _deckSelectionEpoch = deckSelectionEpoch ?? (() => 0); Future _createOutline( DeckGenerationRequest request, { @@ -152,15 +155,22 @@ final class WizardGenerationController extends ChangeNotifier { _notify(); } + bool _mayAutoPublish(int selectionEpoch) => + _deckSelectionEpoch() == selectionEpoch; + Future _completeComposition( int operation, DeckGenerationResult generated, + int selectionEpoch, ) async { final GeneratedDeckApplication application; try { application = await _applyResult( generated, - isValid: () => !_isOperationCancelled(operation) && !_disposed, + isValid: () => + !_isOperationCancelled(operation) && + !_disposed && + _mayAutoPublish(selectionEpoch), ); } catch (error) { if (_isOperationCancelled(operation) || _disposed) return; @@ -172,9 +182,18 @@ final class WizardGenerationController extends ChangeNotifier { return; } if (_isOperationCancelled(operation) || _disposed) return; - // The applier stops when a newer run replaced this one, so the run that - // owns the state keeps it. - if (!application.published) return; + // Opening a saved deck withdraws automatic publication. The result stays + // available for an explicit acceptance that publishes that generation alone. + if (!application.published) { + if (!_mayAutoPublish(selectionEpoch)) { + _result = generated; + _stage = .completed; + _progress = const GenerationProgress(.idle); + _notify(); + } + + return; + } if (application.cleanupError != null) { _applyNotice = 'Some artwork from the previous deck could not be removed.'; @@ -275,6 +294,7 @@ final class WizardGenerationController extends ChangeNotifier { } final operation = ++_operationEpoch; + final selectionEpoch = _deckSelectionEpoch(); _result = null; _errorMessage = null; _failedPhase = null; @@ -308,7 +328,7 @@ final class WizardGenerationController extends ChangeNotifier { return; } - await _completeComposition(operation, generated); + await _completeComposition(operation, generated, selectionEpoch); } Future retryFailedSlides() async { @@ -323,6 +343,7 @@ final class WizardGenerationController extends ChangeNotifier { } final operation = ++_operationEpoch; + final selectionEpoch = _deckSelectionEpoch(); _errorMessage = null; _failedPhase = null; _applyNotice = null; @@ -358,7 +379,36 @@ final class WizardGenerationController extends ChangeNotifier { return; } - await _completeComposition(operation, generated); + await _completeComposition(operation, generated, selectionEpoch); + } + + /// Publishes the retained generation as the only active deck. + /// + /// Selecting a saved deck removes an in-flight run's right to publish on + /// completion. This is the later, explicit acceptance of that same result. + Future acceptRetainedGeneration() async { + final generated = _result; + if (generated == null || isBusy || _disposed) return; + final GeneratedDeckApplication application; + try { + application = await _applyResult(generated, isValid: () => !_disposed); + } catch (error) { + if (_disposed) return; + _fail( + .composition, + 'Slides were generated but could not be loaded: $error', + ); + + return; + } + if (_disposed || !application.published) return; + if (application.cleanupError != null) { + _applyNotice = + 'Some artwork from the previous deck could not be removed.'; + } + _stage = .completed; + _progress = const GenerationProgress(.idle); + _notify(); } Future retry() async { diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart index 464294d6..4d28a402 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart @@ -32,6 +32,7 @@ class WizardGenerationStatus extends StatelessWidget { this.onStartOver, this.onSave, this.onOpenSavedDecks, + this.onShowGeneratedDeck, this.saveLabel = 'Save deck', this.isSaving = false, }); @@ -62,6 +63,9 @@ class WizardGenerationStatus extends StatelessWidget { /// Opens the list of decks already saved. final VoidCallback? onOpenSavedDecks; + /// Publishes the retained generation over the saved deck currently showing. + final VoidCallback? onShowGeneratedDeck; + /// Reflects whether this deck has been saved yet, so a second save reads as /// another copy rather than an undo. final String saveLabel; @@ -239,6 +243,13 @@ class WizardGenerationStatus extends StatelessWidget { icon: LucideIcons.save, variant: .outline, ), + if (onShowGeneratedDeck != null) + SdButton( + label: 'Show generated deck', + onPressed: onShowGeneratedDeck, + icon: LucideIcons.presentation, + variant: .outline, + ), SdButton( label: 'Present deck', onPressed: onPresent, diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart index 4ddfe808..a2d80221 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart @@ -5,6 +5,7 @@ import 'package:go_router/go_router.dart'; import 'package:hero_ui/hero_ui.dart'; import 'package:provider/provider.dart'; +import '../../../../core/data/mappers/deck_markdown_codec.dart'; import '../../../../core/domain/design/presentation_image_style_catalog.dart'; import '../../../../core/domain/stores/deck_customization_store.dart'; import '../../../../core/data/data_sources/deck_library_asset_store.dart'; @@ -14,8 +15,8 @@ import '../../../library/domain/deck_library_controller.dart'; import '../../../library/domain/saved_deck.dart'; import '../../../library/presentation/save_deck_dialog.dart'; import '../../quick_agent/core/engine/services/deck_generator_service.dart'; -import '../../quick_agent/core/env_config.dart'; import '../../quick_agent/domain/generated_deck_result_applier.dart'; +import '../../quick_agent/core/env_config.dart'; import '../../image_generation/image_generator.dart'; import '../../image_generation/image_style_preview_coordinator.dart'; import '../chat/chat_conversation_profile.dart'; @@ -26,6 +27,31 @@ import 'wizard_generation_status.dart'; import 'wizard_outline_review.dart'; import 'wizard_view.dart'; +/// Saves [result] as its own deck: its Markdown, artwork, and theme. +/// +/// The runtime document is not the source. Opening another saved deck leaves +/// the Wizard holding this result, and the save still writes this result alone. +Future saveRetainedWizardDeck({ + required DeckLibraryController library, + required DeckGenerationResult result, + required String name, +}) { + final theme = result.theme; + + return library.save( + name: name, + markdown: const DeckMarkdownCodec().encode(result.slides), + images: result.generatedImages, + theme: theme == null + ? null + : SavedDeckTheme( + id: theme.descriptor.id, + version: theme.descriptor.version, + density: theme.density, + ), + ); +} + /// Host for the conversational Wizard, the app's only authoring flow. /// /// Generated Markdown is kept in memory so opening this screen never requests a @@ -117,13 +143,14 @@ class WizardPage extends StatelessWidget { imageGenerator: finalImageGenerator, ), applyResult: (result, {required isValid}) { - // A generated deck owns the runtime from here. The saved deck - // that was open must stop answering for artwork, or a new - // image could resolve to the old deck's file of the same name. - library.releaseOpenDeck(); - - return resultApplier.apply(result, isValid: isValid); + return applyGeneratedDeckResult( + applier: resultApplier, + result: result, + isValid: isValid, + releaseSavedDeck: library.releaseOpenDeck, + ); }, + deckSelectionEpoch: () => library.deckSelectionEpoch, ); }, ), @@ -168,24 +195,17 @@ class _WizardExperienceState extends State<_WizardExperience> { ); if (name == null || !mounted) return; - await library.save( - name: name, - images: result.generatedImages, - theme: switch (result.theme) { - final theme? => SavedDeckTheme( - id: theme.descriptor.id, - version: theme.descriptor.version, - density: theme.density, - ), - _ => null, - }, - ); + await saveRetainedWizardDeck(library: library, result: result, name: name); } @override Widget build(BuildContext context) { final controller = context.watch(); final library = context.watch(); + final document = context.watch(); + final generatedMarkdown = controller.result == null + ? null + : const DeckMarkdownCodec().encode(controller.result!.slides); final overlay = switch (controller.stage) { .setup => null, .planning || .composing => _CenteredScrollable( @@ -247,6 +267,12 @@ class _WizardExperienceState extends State<_WizardExperience> { }, onSave: library.canSave ? () => unawaited(_save(controller)) : null, onOpenSavedDecks: () => context.push('/decks'), + onShowGeneratedDeck: + library.openDeck != null && + generatedMarkdown != null && + document.markdown != generatedMarkdown + ? () => unawaited(controller.acceptRetainedGeneration()) + : null, saveLabel: library.lastSave == null ? 'Save deck' : 'Save another copy', diff --git a/packages/playground/lib/features/library/domain/deck_library_controller.dart b/packages/playground/lib/features/library/domain/deck_library_controller.dart index d77663b1..5b0b2896 100644 --- a/packages/playground/lib/features/library/domain/deck_library_controller.dart +++ b/packages/playground/lib/features/library/domain/deck_library_controller.dart @@ -2,6 +2,7 @@ import 'package:flutter/foundation.dart'; import '../../../core/data/data_sources/deck_library_asset_store.dart'; import '../../../core/data/data_sources/memory_deck_loader.dart'; +import '../../../core/data/mappers/deck_markdown_codec.dart'; import '../../../core/domain/design/generated_deck_style_mapper.dart'; import '../../../core/domain/design/presentation_theme_catalog.dart'; import '../../../core/domain/design/presentation_typography_catalog.dart'; @@ -26,11 +27,18 @@ class DeckLibraryController extends ChangeNotifier { final PresentationThemeCatalog _themeCatalog; final PresentationTypographyCatalog _typographyCatalog; + static const _codec = DeckMarkdownCodec(); + List _decks = const []; SavedDeckRef? _openDeck; DeckSaveOutcome? _lastSave; String? _errorMessage; - bool _isBusy = false; + int _mutationGeneration = 0; + int _listGeneration = 0; + int _completedMutations = 0; + int _inFlight = 0; + int _deckSelectionEpoch = 0; + bool _mutationInFlight = false; DeckLibraryController({ required DeckLibrary library, @@ -49,11 +57,26 @@ class DeckLibraryController extends ChangeNotifier { _typographyCatalog = typographyCatalog ?? PresentationTypographyCatalog.withDefaults(); - void _setBusy({required bool busy}) { - _isBusy = busy; + void _beginOperation() { + _inFlight++; + notifyListeners(); + } + + void _endOperation() { + _inFlight--; notifyListeners(); } + bool _isLatestMutation(int generation) => generation == _mutationGeneration; + + bool _isLatestList(int generation) => generation == _listGeneration; + + bool _refreshIsStale(int generation, int seenMutations) { + return !_isLatestList(generation) || + _mutationInFlight || + seenMutations != _completedMutations; + } + /// Restores the appearance a deck was saved with. /// /// A selection the catalog no longer offers leaves the current theme alone @@ -83,97 +106,153 @@ class DeckLibraryController extends ChangeNotifier { /// The saved deck being presented, or `null` for a generated one. SavedDeckRef? get openDeck => _openDeck; + /// Advances when a saved deck is committed to the runtime. + /// + /// A generation captures this before it can publish. Opening another deck + /// withdraws that automatic publication; the retained result can still be + /// accepted explicitly. + int get deckSelectionEpoch => _deckSelectionEpoch; + /// What the last save wrote, for the notice the Wizard shows. DeckSaveOutcome? get lastSave => _lastSave; /// A user-facing reason the last save or open did not finish. String? get errorMessage => _errorMessage; - /// Whether a save or open is running. - bool get isBusy => _isBusy; + /// Whether a save, open, or refresh is running. + bool get isBusy => _inFlight > 0; /// Whether decks can be stored at all on this platform. bool get canSave => _library.canSave; - /// Writes the current document as a new deck named [name]. + /// Writes one deck named [name]. /// - /// [images] carries the artwork of the generation that produced the - /// document, and [theme] the selection it was generated with. + /// [markdown] is the document to persist. When it is omitted, the current + /// runtime document is saved. [images] and [theme] belong to that same deck. + /// An explicit [markdown] also becomes the deck on screen, so a save cannot + /// store one deck's text beside another's artwork. Future save({ required String name, + String? markdown, List images = const [], SavedDeckTheme? theme, }) async { - if (_isBusy) return false; - _errorMessage = null; - _setBusy(busy: true); + if (_mutationInFlight) return false; + _mutationInFlight = true; + final mutation = ++_mutationGeneration; + // A refresh that started earlier, or that finishes while this save is + // still writing, must not replace the deck this save commits. + _listGeneration++; + final persisted = markdown ?? _documentStore.markdown; + _beginOperation(); try { final result = await _library.save( name: name, - markdown: _documentStore.markdown, + markdown: persisted, images: images, theme: theme, ); + final current = _isLatestMutation(mutation); switch (result) { case Ok(:final value): - _lastSave = value; - // The saved copy, not the run's memory, now owns this artwork. - _openDeck = value.ref; - _assetStore.bindTo(value.ref); - _decks = [value.ref, ..._decks.where((ref) => ref != value.ref)]; + if (current) { + _lastSave = value; + // The saved copy, not the run's memory, now owns this artwork. + _openDeck = value.ref; + _assetStore.bindTo(value.ref); + _decks = [value.ref, ..._decks.where((ref) => ref != value.ref)]; + _errorMessage = null; + if (markdown != null) { + _documentStore.replaceMarkdown(persisted); + _deckLoader.updateMarkdown(persisted); + if (theme != null) _errorMessage = _applyTheme(theme); + } + _completedMutations++; + } return true; case Failure(:final error): - _errorMessage = '$error'; + if (current) { + _errorMessage = '$error'; + _completedMutations++; + } return false; } } finally { - _setBusy(busy: false); + _mutationInFlight = false; + _endOperation(); } } /// Reloads the list of saved decks. + /// + /// Refresh may overlap a save or an open. It does not clear [isBusy] while + /// that other operation is still running, and a stale completion does not + /// replace a newer deck list or error. Future refresh() async { - _errorMessage = null; - _setBusy(busy: true); + final generation = ++_listGeneration; + final seenMutations = _completedMutations; + _beginOperation(); try { switch (await _library.list()) { case Ok(:final value): + if (_refreshIsStale(generation, seenMutations)) return; _decks = value; + _errorMessage = null; case Failure(:final error): + if (_refreshIsStale(generation, seenMutations)) return; _errorMessage = '$error'; } } finally { - _setBusy(busy: false); + _endOperation(); } } /// Publishes the saved deck at [ref] for presentation. /// /// The deck is read-only: it is loaded into the preview and the renderer, - /// with its own artwork and the theme it was saved with. + /// with its own artwork and the theme it was saved with. Document, artwork + /// binding, theme, and slides change together, and only after the Markdown + /// decodes. Future open(SavedDeckRef ref) async { - if (_isBusy) return false; - _errorMessage = null; - _setBusy(busy: true); + if (_mutationInFlight) return false; + _mutationInFlight = true; + final mutation = ++_mutationGeneration; + _listGeneration++; + _beginOperation(); try { switch (await _library.open(ref)) { case Ok(:final value): + if (!_isLatestMutation(mutation)) return false; + try { + _codec.decode(value.markdown); + } catch (error) { + _errorMessage = '$error'; + _completedMutations++; + + return false; + } + if (!_isLatestMutation(mutation)) return false; _openDeck = value.ref; _assetStore.bindTo(value.ref); _documentStore.replaceMarkdown(value.markdown); _deckLoader.updateMarkdown(value.markdown); _errorMessage = _applyTheme(value.theme); + _deckSelectionEpoch++; + _completedMutations++; return true; case Failure(:final error): + if (!_isLatestMutation(mutation)) return false; _errorMessage = '$error'; + _completedMutations++; return false; } } finally { - _setBusy(busy: false); + _mutationInFlight = false; + _endOperation(); } } diff --git a/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart b/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart new file mode 100644 index 00000000..881a9f49 --- /dev/null +++ b/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart @@ -0,0 +1,171 @@ +import 'package:flutter/material.dart' show Colors; +import 'package:flutter_test/flutter_test.dart'; +import 'package:google_fonts/google_fonts.dart'; +import 'package:playground/core/data/data_sources/deck_library_asset_store.dart'; +import 'package:playground/core/data/data_sources/memory_asset_cache_store.dart'; +import 'package:playground/core/data/data_sources/memory_deck_loader.dart'; +import 'package:playground/core/domain/stores/deck_customization_store.dart'; +import 'package:playground/core/domain/stores/deck_document_store.dart'; +import 'package:playground/features/library/domain/deck_library_controller.dart'; +import 'package:superdeck/superdeck.dart'; +import 'package:superdeck_core/superdeck_core.dart'; + +import '../../../helpers/fake_deck_library.dart'; + +const _markdown = ''' +--- +title: Loaded +--- + +A deck that already decoded. +'''; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + setUpAll(() => GoogleFonts.config.allowRuntimeFetching = false); + + test('reload re-emits unchanged slides before it completes', () async { + final harness = _harness(); + harness.loader.updateMarkdown(_markdown); + await Future.delayed(Duration.zero); + expect(harness.deck.session.isLoading.value, isFalse); + final title = + harness.deck.session.loadedSlides.value?.single.options?.title; + expect(title, 'Loaded'); + + final order = await _reload(harness.loader, harness.deck); + + expect(order.eventBeforeComplete, isTrue); + expect(order.event, isA()); + expect(harness.deck.session.isLoading.value, isFalse); + expect( + harness.deck.session.loadedSlides.value?.single.options?.title, + title, + ); + expect(harness.deck.session.error.value, isNull); + }); + + test( + 'reload re-emits a terminal error for markdown that failed to decode', + () async { + final harness = _harness(); + harness.loader.updateMarkdown('@column\n'); + await Future.delayed(Duration.zero); + expect(harness.deck.session.loadedSlides.value, isNull); + expect(harness.deck.session.isLoading.value, isFalse); + expect(harness.deck.session.error.value, isNotNull); + + final order = await _reload(harness.loader, harness.deck); + + expect(order.eventBeforeComplete, isTrue); + expect(order.event, isA()); + expect(harness.deck.session.isLoading.value, isFalse); + expect(harness.deck.session.loadedSlides.value, isNull); + expect(harness.deck.session.error.value, isNotNull); + }, + ); + + test( + 'reload without markdown emits a terminal error and clears loading', + () async { + final harness = _harness(); + expect(harness.deck.session.loadedSlides.value, isNull); + + final order = await _reload(harness.loader, harness.deck); + + expect(order.eventBeforeComplete, isTrue); + expect(order.event, isA()); + expect(harness.deck.session.isLoading.value, isFalse); + expect(harness.deck.session.loadedSlides.value, isNull); + expect(harness.deck.session.error.value, isNotNull); + }, + ); + + test('an ordinary identical update emits no new preview event', () async { + final harness = _harness(); + harness.loader.updateMarkdown(_markdown); + await Future.delayed(Duration.zero); + final events = []; + final subscription = harness.loader.load().listen(events.add); + addTearDown(subscription.cancel); + + harness.loader.updateMarkdown(_markdown); + await Future.delayed(Duration.zero); + + expect(events, isEmpty); + }); + + test('reload does not reread a saved deck from the library', () async { + final library = FakeDeckLibrary(); + final loader = MemoryDeckLoader(); + final document = DeckDocumentStore(markdown: _markdown); + final assets = DeckLibraryAssetStore( + library: library, + fallback: MemoryAssetCacheStore(), + ); + final deck = DeckController( + deckLoader: loader, + options: DeckOptions(), + assetCacheStore: assets, + ); + final customization = DeckCustomizationStore( + deck, + background: Colors.black, + foreground: Colors.white, + ); + final controller = DeckLibraryController( + library: library, + documentStore: document, + deckLoader: loader, + customizationStore: customization, + assetStore: assets, + ); + addTearDown(() { + controller.dispose(); + customization.dispose(); + deck.dispose(); + document.dispose(); + }); + + final saved = await controller.save(name: 'Saved'); + expect(saved, isTrue); + final opened = await controller.open(controller.openDeck!); + expect(opened, isTrue); + final opensBeforeReload = library.openCount; + + await deck.session.reload(); + + expect(library.openCount, opensBeforeReload); + expect(deck.session.isLoading.value, isFalse); + }); +} + +({MemoryDeckLoader loader, DeckController deck}) _harness() { + final loader = MemoryDeckLoader(); + final deck = DeckController( + deckLoader: loader, + options: DeckOptions(), + assetCacheStore: MemoryAssetCacheStore(), + ); + addTearDown(deck.dispose); + + return (loader: loader, deck: deck); +} + +Future<({bool eventBeforeComplete, SlidesEvent? event})> _reload( + MemoryDeckLoader loader, + DeckController deck, +) async { + var completed = false; + SlidesEvent? event; + var eventBeforeComplete = false; + final subscription = loader.load().listen((next) { + event = next; + eventBeforeComplete = !completed; + }); + addTearDown(subscription.cancel); + + await deck.session.reload().whenComplete(() => completed = true); + + return (eventBeforeComplete: eventBeforeComplete, event: event); +} diff --git a/packages/playground/test/features/library/domain/deck_ownership_test.dart b/packages/playground/test/features/library/domain/deck_ownership_test.dart new file mode 100644 index 00000000..c07a1f4d --- /dev/null +++ b/packages/playground/test/features/library/domain/deck_ownership_test.dart @@ -0,0 +1,740 @@ +import 'dart:async'; + +import 'package:flutter/material.dart' show Color, Colors; +import 'package:flutter_test/flutter_test.dart'; +import 'package:google_fonts/google_fonts.dart'; +import 'package:playground/core/data/data_sources/deck_file.dart'; +import 'package:playground/core/data/data_sources/deck_library_asset_store.dart'; +import 'package:playground/core/data/data_sources/memory_asset_cache_store.dart'; +import 'package:playground/core/data/data_sources/memory_deck_loader.dart'; +import 'package:playground/core/domain/design/presentation_theme_catalog.dart'; +import 'package:playground/core/domain/design/presentation_typography_catalog.dart'; +import 'package:playground/core/domain/generated_image_asset.dart'; +import 'package:playground/core/result.dart'; +import 'package:playground/core/domain/stores/deck_customization_store.dart'; +import 'package:playground/core/domain/stores/deck_document_store.dart'; +import 'package:playground/features/ai/quick_agent/core/engine/schemas/outline_schema.dart'; +import 'package:playground/features/ai/quick_agent/core/engine/services/deck_generation_request.dart'; +import 'package:playground/features/ai/quick_agent/core/engine/services/deck_generator_service.dart'; +import 'package:playground/features/ai/quick_agent/domain/generated_deck_result_applier.dart'; +import 'package:playground/features/ai/wizard/presentation/wizard_generation_controller.dart'; +import 'package:playground/features/ai/wizard/presentation/wizard_page.dart'; +import 'package:playground/features/library/domain/deck_library_controller.dart'; +import 'package:playground/features/library/domain/saved_deck.dart'; +import 'package:superdeck/superdeck.dart'; +import 'package:superdeck_core/superdeck_core.dart' hide Ok; + +import '../../../helpers/fake_deck_library.dart'; + +const _savedMarkdown = ''' +--- +title: Saved A +--- + +Saved deck A stays on screen. +'''; + +const _otherMarkdown = ''' +--- +title: Saved B +--- + +Saved deck B stays on screen. +'''; + +const _boldTheme = SavedDeckTheme( + id: 'bold-product', + version: 1, + density: 'balanced', +); + +const _paperTheme = SavedDeckTheme( + id: 'technical-paper', + version: 1, + density: 'balanced', +); + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + setUpAll(() => GoogleFonts.config.allowRuntimeFetching = false); + + group('generated result application', () { + test('a rejected application leaves the saved deck intact', () async { + final scope = await _showingSavedDeck(); + + final application = await applyGeneratedDeckResult( + applier: scope.applier, + result: _generatedResult(), + isValid: () => false, + releaseSavedDeck: scope.libraryController.releaseOpenDeck, + ); + + expect(application.published, isFalse); + _expectSavedDeckIntact(scope); + }); + + test('an abandoned application leaves the saved deck intact', () async { + final scope = await _showingSavedDeck(); + var accepted = true; + + final application = await applyGeneratedDeckResult( + applier: scope.applier, + result: _generatedResult(), + isValid: () { + final current = accepted; + accepted = false; + return current; + }, + releaseSavedDeck: scope.libraryController.releaseOpenDeck, + ); + + expect(application.published, isFalse); + _expectSavedDeckIntact(scope); + }); + + test('a thrown application leaves the saved deck intact', () async { + final scope = await _showingSavedDeck(fallback: _ThrowingAssetCache()); + + await expectLater( + applyGeneratedDeckResult( + applier: scope.applier, + result: _generatedResult(), + isValid: () => true, + releaseSavedDeck: scope.libraryController.releaseOpenDeck, + ), + throwsA(isA()), + ); + + _expectSavedDeckIntact(scope); + }); + }); + + group('one deck per save and publication', () { + test( + 'saving a retained generation writes that generation after another deck was opened', + () async { + final scope = await _showingSavedDeck(markdown: _otherMarkdown); + final wizard = _wizard( + scope, + result: _generatedResult(assetKey: 'generated-a.png'), + ); + + await wizard.controller.createOutline(_request); + await wizard.controller.generateSlides(); + expect(wizard.controller.result, isNotNull); + + final reopened = await scope.libraryController.open(scope.savedRef); + expect(reopened, isTrue); + await _flush(); + expect(scope.document.markdown, _otherMarkdown); + + final saved = await saveRetainedWizardDeck( + library: scope.libraryController, + result: wizard.controller.result!, + name: 'Generated copy', + ); + + expect(saved, isTrue); + final path = scope.libraryController.openDeck!.reference.path; + final stored = scope.library.markdown[path]!; + expect(stored, contains('Only the generated deck.')); + expect(stored, contains('generated-a.png')); + expect(stored, isNot(contains('Saved deck B stays on screen.'))); + expect(scope.library.assets[path]?['generated-a.png'], [1, 2, 3]); + expect(scope.library.themes[path], _paperTheme); + expect(scope.document.markdown, stored); + await _flush(); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + 'Generated A', + ); + }, + ); + + test( + 'a saved deck opened during generation is not replaced, and accepting the generation publishes only that generation', + () async { + final scope = _newScope(); + await scope.library.save( + name: 'Saved A', + markdown: _savedMarkdown, + images: [ + GeneratedImageAsset.success( + assetKey: 'saved-a.png', + bytes: [4, 5, 6], + ), + ], + theme: _boldTheme, + ); + final savedRef = scope.library.saved.single; + final gate = Completer(); + final wizard = _wizard(scope, pendingResult: gate); + + await wizard.controller.createOutline(_request); + final composition = wizard.controller.generateSlides(); + await Future.delayed(Duration.zero); + expect(wizard.controller.stage, WizardGenerationStage.composing); + + final opened = await scope.libraryController.open(savedRef); + expect(opened, isTrue); + await _flush(); + final saved = _snapshot(scope); + final artwork = await scope.assets.resolve('saved-a.png'); + expect(artwork, isNotNull); + + gate.complete(_generatedResult()); + await composition; + + expect(wizard.controller.stage, WizardGenerationStage.completed); + expect(wizard.controller.result, isNotNull); + _expectSnapshot(scope, saved); + expect(await scope.assets.resolve('saved-a.png'), artwork); + + await wizard.controller.acceptRetainedGeneration(); + await _flush(); + + expect(scope.libraryController.openDeck, isNull); + expect(scope.document.markdown, contains('Only the generated deck.')); + expect( + scope.document.markdown, + isNot(contains('Saved deck A stays on screen.')), + ); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + 'Generated A', + ); + expect(scope.customization.background, isNot(saved.background)); + final generatedUri = await scope.assets.resolve('generated-a.png'); + final savedUri = await scope.assets.resolve('saved-a.png'); + expect(generatedUri, isNotNull); + expect(savedUri, isNull); + }, + ); + }); + + group('library open and refresh', () { + test( + 'a malformed open reports failure and keeps the previous deck', + () async { + final scope = await _showingSavedDeck(); + final saved = _snapshot(scope); + final artwork = await scope.assets.resolve('saved-a.png'); + final broken = await scope.library.save( + name: 'Broken', + markdown: '@column\n', + ); + final brokenRef = switch (broken) { + Ok(:final value) => value.ref, + Failure(:final error) => fail( + 'could not stage a broken deck: $error', + ), + }; + + final opened = await scope.libraryController.open(brokenRef); + + expect(opened, isFalse); + expect(scope.libraryController.errorMessage, isNotNull); + _expectSnapshot(scope, saved); + expect(await scope.assets.resolve('saved-a.png'), artwork); + }, + ); + + test( + 'an overlapping refresh does not mark the library idle while a save is still running', + () async { + final scope = _newScope(); + final saveEntered = Completer(); + final releaseSave = Completer(); + scope.library.beforeSave = () async { + if (!saveEntered.isCompleted) saveEntered.complete(); + await releaseSave.future; + }; + final saving = scope.libraryController.save( + name: 'Current', + markdown: _savedMarkdown, + images: [ + GeneratedImageAsset.success( + assetKey: 'saved-a.png', + bytes: [4, 5, 6], + ), + ], + theme: _boldTheme, + ); + await saveEntered.future; + + final listEntered = Completer(); + final releaseList = Completer(); + scope.library.beforeList = () async { + if (!listEntered.isCompleted) listEntered.complete(); + await releaseList.future; + }; + scope.library.listError = Exception('stale list'); + final refreshing = scope.libraryController.refresh(); + await listEntered.future; + releaseList.complete(); + await refreshing; + + expect(scope.libraryController.isBusy, isTrue); + + releaseSave.complete(); + final saved = await saving; + await _flush(); + + expect(saved, isTrue); + expect(scope.libraryController.isBusy, isFalse); + expect(scope.libraryController.decks.single.name, 'Current'); + expect(scope.libraryController.openDeck?.name, 'Current'); + expect(scope.libraryController.lastSave, isNotNull); + expect(scope.libraryController.errorMessage, isNull); + expect(scope.document.markdown, _savedMarkdown); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + 'Saved A', + ); + expect(await scope.assets.resolve('saved-a.png'), isNotNull); + expect(scope.customization.background, isNot(Colors.black)); + }, + ); + + test( + 'a refresh started during a save does not erase that save after it commits', + () async { + final scope = _newScope(); + final saveEntered = Completer(); + final releaseSave = Completer(); + scope.library.beforeSave = () async { + if (!saveEntered.isCompleted) saveEntered.complete(); + await releaseSave.future; + }; + final listEntered = Completer(); + final releaseList = Completer(); + scope.library.beforeList = () async { + if (!listEntered.isCompleted) listEntered.complete(); + await releaseList.future; + }; + + final saving = scope.libraryController.save(name: 'Current'); + await saveEntered.future; + final refreshing = scope.libraryController.refresh(); + await listEntered.future; + + releaseSave.complete(); + final saved = await saving; + expect(saved, isTrue); + expect(scope.libraryController.decks.single.name, 'Current'); + expect(scope.libraryController.openDeck?.name, 'Current'); + expect(scope.libraryController.isBusy, isTrue); + + scope.library.listError = Exception('late stale list'); + releaseList.complete(); + await refreshing; + + expect(scope.libraryController.decks.single.name, 'Current'); + expect(scope.libraryController.openDeck?.name, 'Current'); + expect(scope.libraryController.lastSave, isNotNull); + expect(scope.libraryController.errorMessage, isNull); + expect(scope.libraryController.isBusy, isFalse); + }, + ); + + test('an overlapping refresh does not drop a successful open', () async { + final scope = _newScope(); + scope.document.replaceMarkdown(_savedMarkdown); + final saved = await scope.libraryController.save(name: 'Saved A'); + expect(saved, isTrue); + final ref = scope.libraryController.openDeck!; + scope.libraryController.releaseOpenDeck(); + scope.document.replaceMarkdown('# Draft\n'); + + final openEntered = Completer(); + final releaseOpen = Completer(); + scope.library.beforeOpen = () async { + if (!openEntered.isCompleted) openEntered.complete(); + await releaseOpen.future; + }; + final opening = scope.libraryController.open(ref); + await openEntered.future; + + final listEntered = Completer(); + final releaseList = Completer(); + scope.library.beforeList = () async { + if (!listEntered.isCompleted) listEntered.complete(); + await releaseList.future; + }; + scope.library.listError = Exception('stale list'); + final refreshing = scope.libraryController.refresh(); + await listEntered.future; + releaseList.complete(); + await refreshing; + + expect(scope.libraryController.isBusy, isTrue); + expect(scope.document.markdown, '# Draft\n'); + + releaseOpen.complete(); + final opened = await opening; + await _flush(); + + expect(opened, isTrue); + expect(scope.libraryController.isBusy, isFalse); + expect(scope.libraryController.openDeck, ref); + expect(scope.document.markdown, _savedMarkdown); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + 'Saved A', + ); + expect(scope.libraryController.errorMessage, isNull); + }); + + test( + 'a slower refresh does not replace the deck list or error from a newer save', + () async { + final scope = _newScope(); + final listEntered = Completer(); + final releaseList = Completer(); + scope.library.beforeList = () async { + if (!listEntered.isCompleted) listEntered.complete(); + await releaseList.future; + }; + scope.library.listError = Exception('older list'); + final refreshing = scope.libraryController.refresh(); + await listEntered.future; + + final saved = await scope.libraryController.save(name: 'Newer'); + + expect(saved, isTrue); + expect(scope.libraryController.isBusy, isTrue); + expect(scope.libraryController.decks.single.name, 'Newer'); + expect(scope.libraryController.errorMessage, isNull); + + releaseList.complete(); + await refreshing; + + expect(scope.libraryController.decks.single.name, 'Newer'); + expect(scope.libraryController.errorMessage, isNull); + expect(scope.libraryController.isBusy, isFalse); + }, + ); + + test( + 'a slower refresh does not replace the error from a newer open', + () async { + final scope = await _showingSavedDeck(); + final listEntered = Completer(); + final releaseList = Completer(); + scope.library.beforeList = () async { + if (!listEntered.isCompleted) listEntered.complete(); + await releaseList.future; + }; + scope.library.listError = Exception('older list'); + final refreshing = scope.libraryController.refresh(); + await listEntered.future; + scope.library.openError = Exception('missing deck'); + + final opened = await scope.libraryController.open(scope.savedRef); + + expect(opened, isFalse); + expect(scope.libraryController.isBusy, isTrue); + expect(scope.libraryController.errorMessage, contains('missing deck')); + + releaseList.complete(); + await refreshing; + + expect(scope.libraryController.errorMessage, contains('missing deck')); + expect(scope.libraryController.decks, isNotEmpty); + }, + ); + }); +} + +Future _flush() => Future.delayed(Duration.zero); + +const _request = DeckGenerationRequest( + userIntent: 'Ownership', + slideCount: 1, + themeId: 'technical-paper', +); + +typedef _OwnedScope = ({ + DeckLibraryController libraryController, + FakeDeckLibrary library, + DeckDocumentStore document, + MemoryDeckLoader loader, + DeckCustomizationStore customization, + DeckLibraryAssetStore assets, + DeckController deck, + GeneratedDeckResultApplier applier, + SavedDeckRef savedRef, + Uri? artworkUri, + Color background, + String? slideTitle, +}); + +_OwnedScope _newScope({AssetCacheStore? fallback}) { + final library = FakeDeckLibrary(); + final memory = fallback ?? MemoryAssetCacheStore(); + final assets = DeckLibraryAssetStore(library: library, fallback: memory); + final document = DeckDocumentStore(markdown: '# Draft\n'); + final loader = MemoryDeckLoader(); + final deck = DeckController( + deckLoader: loader, + options: DeckOptions(), + assetCacheStore: assets, + ); + final customization = DeckCustomizationStore( + deck, + background: Colors.black, + foreground: Colors.white, + ); + final libraryController = DeckLibraryController( + library: library, + documentStore: document, + deckLoader: loader, + customizationStore: customization, + assetStore: assets, + ); + final applier = GeneratedDeckResultApplier( + documentStore: document, + deckLoader: loader, + assetCacheStore: assets, + customizationStore: customization, + ); + addTearDown(() { + libraryController.dispose(); + customization.dispose(); + deck.dispose(); + document.dispose(); + }); + + return ( + libraryController: libraryController, + library: library, + document: document, + loader: loader, + customization: customization, + assets: assets, + deck: deck, + applier: applier, + savedRef: SavedDeckRef( + reference: DeckFileReference(path: '/unset'), + name: 'unset', + savedAt: DateTime.utc(2026), + ), + artworkUri: null, + background: Colors.black, + slideTitle: null, + ); +} + +Future<_OwnedScope> _showingSavedDeck({ + AssetCacheStore? fallback, + String markdown = _savedMarkdown, + String name = 'Saved A', + String assetKey = 'saved-a.png', + SavedDeckTheme theme = _boldTheme, +}) async { + final scope = _newScope(fallback: fallback); + scope.document.replaceMarkdown(markdown); + final saved = await scope.libraryController.save( + name: name, + images: [ + GeneratedImageAsset.success(assetKey: assetKey, bytes: [4, 5, 6]), + ], + theme: theme, + ); + expect(saved, isTrue); + final ref = scope.libraryController.openDeck!; + final opened = await scope.libraryController.open(ref); + expect(opened, isTrue); + final uri = await scope.assets.resolve(assetKey); + expect(uri, isNotNull); + expect(scope.deck.session.loadedSlides.value, isNotNull); + + return ( + libraryController: scope.libraryController, + library: scope.library, + document: scope.document, + loader: scope.loader, + customization: scope.customization, + assets: scope.assets, + deck: scope.deck, + applier: scope.applier, + savedRef: ref, + artworkUri: uri, + background: scope.customization.background, + slideTitle: scope.deck.session.loadedSlides.value?.single.options?.title, + ); +} + +typedef _Snapshot = ({ + SavedDeckRef? openDeck, + String markdown, + Uri? artworkUri, + Color background, + String? slideTitle, +}); + +_Snapshot _snapshot(_OwnedScope scope) => ( + openDeck: scope.libraryController.openDeck, + markdown: scope.document.markdown, + artworkUri: scope.artworkUri, + background: scope.customization.background, + slideTitle: scope.deck.session.loadedSlides.value?.single.options?.title, +); + +void _expectSnapshot(_OwnedScope scope, _Snapshot saved) { + expect(scope.libraryController.openDeck, saved.openDeck); + expect(scope.document.markdown, saved.markdown); + expect(scope.customization.background, saved.background); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + saved.slideTitle, + ); +} + +Future _expectSavedDeckIntact(_OwnedScope scope) async { + expect(scope.libraryController.openDeck, scope.savedRef); + expect(scope.document.markdown, _savedMarkdown); + expect(await scope.assets.resolve('saved-a.png'), scope.artworkUri); + expect(scope.customization.background, scope.background); + expect( + scope.deck.session.loadedSlides.value?.single.options?.title, + scope.slideTitle, + ); +} + +DeckGenerationResult _generatedResult({String assetKey = 'generated-a.png'}) { + final themes = PresentationThemeCatalog.withDefaults(); + final typography = PresentationTypographyCatalog.withDefaults(); + + return DeckGenerationResult.success( + slides: [_generatedSlide(assetKey)], + plan: _plan(), + theme: themes.resolve( + id: 'technical-paper', + version: 1, + density: 'balanced', + typographyCatalog: typography, + ), + generatedImages: [ + GeneratedImageAsset.success(assetKey: assetKey, bytes: [1, 2, 3]), + ], + ); +} + +Slide _generatedSlide(String assetKey) => Slide.parse({ + 'key': 'opening', + 'options': {'title': 'Generated A', 'style': 'visual'}, + 'sections': [ + { + 'type': 'section', + 'blocks': [ + { + 'type': 'block', + 'content': '## Generated A\n\nOnly the generated deck.', + }, + { + 'type': 'widget', + 'name': 'image', + 'args': {'src': assetKey, 'fit': 'cover'}, + }, + ], + }, + ], +}); + +DeckPlan _plan() => DeckPlan.parse({ + 'topic': 'Ownership', + 'story': 'One deck is saved and shown.', + 'theme': {'id': 'technical-paper', 'version': 1, 'density': 'balanced'}, + 'sections': [ + { + 'key': 'main', + 'title': 'Main', + 'purpose': 'Keep one deck together.', + 'transition': 'Close clearly.', + 'slideKeys': ['opening'], + }, + ], + 'slides': [ + { + 'key': 'opening', + 'title': 'Generated A', + 'purpose': 'Show the generated deck.', + 'sectionKey': 'main', + 'assertion': 'The generated deck stands alone.', + 'contentUnits': ['Only the generated deck.'], + 'narrativeRole': 'opening', + 'contentBrief': 'Open with the generated deck.', + 'continuity': 'Lead into the story.', + 'composition': 'content', + 'treatment': 'visual', + 'density': 'balanced', + 'elements': [], + }, + ], +}); + +({WizardGenerationController controller}) _wizard( + _OwnedScope scope, { + DeckGenerationResult? result, + Completer? pendingResult, +}) { + final service = _ScriptedGenerator( + result: result ?? _generatedResult(), + pendingResult: pendingResult, + ); + final controller = WizardGenerationController( + service: service, + applyResult: (generated, {required isValid}) { + return applyGeneratedDeckResult( + applier: scope.applier, + result: generated, + isValid: isValid, + releaseSavedDeck: scope.libraryController.releaseOpenDeck, + ); + }, + deckSelectionEpoch: () => scope.libraryController.deckSelectionEpoch, + ); + addTearDown(controller.dispose); + + return (controller: controller); +} + +final class _ScriptedGenerator extends DeckGeneratorService { + _ScriptedGenerator({required this.result, this.pendingResult}) + : super(apiKey: 'test-key'); + + final DeckGenerationResult result; + final Completer? pendingResult; + + @override + Future plan( + DeckGenerationRequest request, { + onProgress, + onTrace, + isCancelled, + }) async { + return DeckPlanningResult.success(_plan()); + } + + @override + Future generateFromPlan( + DeckGenerationRequest request, + DeckPlan approvedPlan, { + onProgress, + onTrace, + isCancelled, + }) { + final pending = pendingResult; + if (pending != null) return pending.future; + + return Future.value(result); + } +} + +final class _ThrowingAssetCache extends MemoryAssetCacheStore { + @override + Future write(String assetKey, List bytes) { + throw StateError('cannot store artwork'); + } +} diff --git a/packages/playground/test/helpers/fake_deck_library.dart b/packages/playground/test/helpers/fake_deck_library.dart index 73100a70..6512ea6b 100644 --- a/packages/playground/test/helpers/fake_deck_library.dart +++ b/packages/playground/test/helpers/fake_deck_library.dart @@ -18,6 +18,13 @@ class FakeDeckLibrary implements DeckLibrary { Exception? openError; Exception? listError; int saveCount = 0; + int openCount = 0; + + /// Held by tests that need [save], [open], or [list] to finish in an + /// order the controller does not choose. + Future Function()? beforeSave; + Future Function()? beforeOpen; + Future Function()? beforeList; @override Future> save({ @@ -27,6 +34,8 @@ class FakeDeckLibrary implements DeckLibrary { SavedDeckTheme? theme, }) async { saveCount++; + final saveHook = beforeSave; + if (saveHook != null) await saveHook(); final error = saveError; if (error != null) return Result.error(error); @@ -65,6 +74,8 @@ class FakeDeckLibrary implements DeckLibrary { @override Future>> list() async { + final listHook = beforeList; + if (listHook != null) await listHook(); final error = listError; return error != null ? Result.error(error) : Result.ok(List.of(saved)); @@ -72,6 +83,9 @@ class FakeDeckLibrary implements DeckLibrary { @override Future> open(SavedDeckRef ref) async { + openCount++; + final openHook = beforeOpen; + if (openHook != null) await openHook(); final error = openError; if (error != null) return Result.error(error); final content = markdown[ref.reference.path]; From c30272be4ba5773990fb65f2f23f0f4abdff194b Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 20:22:16 -0500 Subject: [PATCH 08/11] fix(pdf): release captured images and ignore stale export completions Only one export runs at a time. Captured image bytes are released when an export fails, is cancelled, or the controller is disposed, and the dialog ignores a completion from a controller it has replaced or disposed. The dialog accepts a capture service for tests. --- .../plugins/pdf/lib/src/pdf_controller.dart | 12 + .../pdf/lib/src/pdf_export_screen.dart | 18 +- .../test/src/pdf_export_lifecycle_test.dart | 366 ++++++++++++++++++ 3 files changed, 392 insertions(+), 4 deletions(-) create mode 100644 packages/plugins/pdf/test/src/pdf_export_lifecycle_test.dart diff --git a/packages/plugins/pdf/lib/src/pdf_controller.dart b/packages/plugins/pdf/lib/src/pdf_controller.dart index 5448b275..1a35800a 100644 --- a/packages/plugins/pdf/lib/src/pdf_controller.dart +++ b/packages/plugins/pdf/lib/src/pdf_controller.dart @@ -75,6 +75,7 @@ class PdfController { final List _images = []; bool _disposed = false; bool _cancelled = false; + bool _exportInFlight = false; final _exportStatus = signal(PdfExportStatus.idle); final _capturedCount = signal(0); @@ -261,7 +262,13 @@ class PdfController { } /// Captures each slide and writes the resulting PDF. + /// + /// An export that is already running, or a controller that has been + /// disposed, is left untouched. Captured image bytes are released when the + /// run fails, is cancelled, or the controller is disposed. Future export() async { + if (_disposed || _exportInFlight) return; + _exportInFlight = true; _cancelled = false; _capturedCount.value = 0; _exportError.value = null; @@ -315,14 +322,18 @@ class PdfController { if (!_disposed) _exportStatus.value = PdfExportStatus.complete; } on _ExportCancelledException catch (e) { + _images.clear(); if (!_disposed) _exportStatus.value = PdfExportStatus.idle; log(e.toString()); } catch (e) { + _images.clear(); if (!_disposed) { _exportError.value = 'Export failed: $e'; _exportStatus.value = PdfExportStatus.failed; } log('Export failed: $e'); + } finally { + _exportInFlight = false; } } @@ -363,6 +374,7 @@ class PdfController { /// Releases page-controller and signal resources owned by this controller. void dispose() { cancel(); + _images.clear(); _disposed = true; _pageController.dispose(); diff --git a/packages/plugins/pdf/lib/src/pdf_export_screen.dart b/packages/plugins/pdf/lib/src/pdf_export_screen.dart index b8b5a579..7844fc95 100644 --- a/packages/plugins/pdf/lib/src/pdf_export_screen.dart +++ b/packages/plugins/pdf/lib/src/pdf_export_screen.dart @@ -13,6 +13,7 @@ class PdfExportDialogScreen extends StatefulWidget { required this.slides, this.options = const PdfExportOptions(), this.onClose, + this.slideCaptureService, }); /// Slides to capture into the PDF. @@ -24,6 +25,10 @@ class PdfExportDialogScreen extends StatefulWidget { /// Callback used when the export surface is hosted by a shell modal. final VoidCallback? onClose; + /// Capture service for this dialog. The production path uses the default + /// slide capture service. + final SlideCaptureService? slideCaptureService; + @override State createState() => _PdfExportDialogScreenState(); @@ -81,7 +86,7 @@ class _PdfExportDialogScreenState extends State { void _setupExportController() { _exportController = PdfController( slides: widget.slides, - slideCaptureService: SlideCaptureService(), + slideCaptureService: widget.slideCaptureService ?? SlideCaptureService(), options: widget.options, ); @@ -100,10 +105,15 @@ class _PdfExportDialogScreenState extends State { } Future _handleExport() async { - await _exportController.export(); - if (!mounted) return; + final started = _exportController; + await started.export(); + if (!mounted || + !identical(started, _exportController) || + started.disposed) { + return; + } - if (_exportController.exportStatus.value != PdfExportStatus.failed) { + if (started.exportStatus.value != PdfExportStatus.failed) { _close(); } } diff --git a/packages/plugins/pdf/test/src/pdf_export_lifecycle_test.dart b/packages/plugins/pdf/test/src/pdf_export_lifecycle_test.dart new file mode 100644 index 00000000..b53290a4 --- /dev/null +++ b/packages/plugins/pdf/test/src/pdf_export_lifecycle_test.dart @@ -0,0 +1,366 @@ +import 'dart:async'; +import 'dart:convert'; +import 'dart:typed_data'; + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:superdeck/superdeck.dart'; +import 'package:superdeck_pdf/src/pdf_controller.dart'; +import 'package:superdeck_pdf/src/pdf_export_options.dart'; +import 'package:superdeck_pdf/src/pdf_export_screen.dart'; + +import '../helpers/fake_slide_capture_service.dart'; +import '../helpers/test_helpers.dart'; + +final _png = base64Decode( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVR4nGP4DwQACfsD/fteaysAAAAASUVORK5CYII=', +); + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + group('PdfController lifecycle', () { + test('export after disposal does not capture or write signals', () async { + final capture = FakeSlideCaptureService(_png); + final controller = PdfController( + slides: createTestSlides(1), + slideCaptureService: capture, + waitDuration: Duration.zero, + ); + controller.dispose(); + + await controller.export(); + + expect(capture.captureFromKeyCalls, 0); + }); + + testWidgets('an overlapping export does not reset the in-flight run', ( + tester, + ) async { + final capture = _GatedCapture(_png); + final controller = PdfController( + slides: createTestSlides(2), + slideCaptureService: capture, + waitDuration: Duration.zero, + options: PdfExportOptions(pdfSaver: (_) async => true), + ); + addTearDown(() { + for (final gate in capture.gates) { + if (!gate.isCompleted) gate.complete(); + } + controller.dispose(); + }); + + await tester.pumpWidget(_harness(controller)); + await tester.pump(); + final export = controller.export(); + await _until(tester, () => capture.gates.isNotEmpty); + capture.gates.first.complete(); + await _until( + tester, + () => + controller.capturedImageCountForTesting == 1 && + capture.gates.length >= 2, + ); + + final status = controller.exportStatus.value; + final retained = controller.capturedImageCountForTesting; + await controller.export(); + + expect(controller.exportStatus.value, status); + expect(controller.capturedImageCountForTesting, retained); + expect(status, PdfExportStatus.capturing); + + controller.cancel(); + capture.gates[1].complete(); + await _finish(tester, export); + + expect(controller.exportStatus.value, PdfExportStatus.idle); + expect(controller.capturedImageCountForTesting, 0); + }); + + testWidgets('failure after a captured slide releases that image', ( + tester, + ) async { + final capture = _FailAfterFirst(_png); + final controller = PdfController( + slides: createTestSlides(2), + slideCaptureService: capture, + waitDuration: Duration.zero, + options: PdfExportOptions(pdfSaver: (_) async => true), + ); + addTearDown(controller.dispose); + + await tester.pumpWidget(_harness(controller)); + await tester.pump(); + await _finish(tester, controller.export()); + + expect(controller.exportStatus.value, PdfExportStatus.failed); + expect(controller.capturedImageCountForTesting, 0); + expect(capture.captureFromKeyCalls, greaterThan(1)); + }); + + testWidgets('cancellation after a captured slide releases that image', ( + tester, + ) async { + final capture = _GatedCapture(_png); + final controller = PdfController( + slides: createTestSlides(2), + slideCaptureService: capture, + waitDuration: Duration.zero, + options: PdfExportOptions(pdfSaver: (_) async => true), + ); + addTearDown(() { + for (final gate in capture.gates) { + if (!gate.isCompleted) gate.complete(); + } + controller.dispose(); + }); + + await tester.pumpWidget(_harness(controller)); + await tester.pump(); + final export = controller.export(); + await _until(tester, () => capture.gates.isNotEmpty); + capture.gates.first.complete(); + await _until( + tester, + () => + controller.capturedImageCountForTesting == 1 && + capture.gates.length >= 2, + ); + + controller.cancel(); + capture.gates[1].complete(); + await _finish(tester, export); + + expect(controller.exportStatus.value, PdfExportStatus.idle); + expect(controller.capturedImageCountForTesting, 0); + }); + + testWidgets('disposal after a captured slide releases that image', ( + tester, + ) async { + final capture = _GatedCapture(_png); + final controller = PdfController( + slides: createTestSlides(2), + slideCaptureService: capture, + waitDuration: Duration.zero, + options: PdfExportOptions(pdfSaver: (_) async => true), + ); + addTearDown(() { + for (final gate in capture.gates) { + if (!gate.isCompleted) gate.complete(); + } + }); + + await tester.pumpWidget(_harness(controller)); + await tester.pump(); + final export = controller.export(); + await _until(tester, () => capture.gates.isNotEmpty); + capture.gates.first.complete(); + await _until( + tester, + () => + controller.capturedImageCountForTesting == 1 && + capture.gates.length >= 2, + ); + + controller.dispose(); + expect(controller.capturedImageCountForTesting, 0); + capture.gates[1].complete(); + await _finish(tester, export); + + expect(controller.capturedImageCountForTesting, 0); + expect(tester.takeException(), isNull); + }); + + testWidgets('a later export can run after the previous one finishes', ( + tester, + ) async { + final capture = FakeSlideCaptureService(_png); + final controller = PdfController( + slides: createTestSlides(1), + slideCaptureService: capture, + waitDuration: Duration.zero, + options: PdfExportOptions(pdfSaver: (_) async => true), + ); + addTearDown(controller.dispose); + + await tester.pumpWidget(_harness(controller)); + await tester.pump(); + await _finish(tester, controller.export()); + expect(controller.exportStatus.value, PdfExportStatus.complete); + + await _finish(tester, controller.export()); + + expect(controller.exportStatus.value, PdfExportStatus.complete); + expect(controller.capturedImageCountForTesting, 0); + expect(capture.captureFromKeyCalls, 2); + }); + }); + + testWidgets( + 'a replaced export controller does not close the dialog from its own status', + (tester) async { + addTearDown(() { + tester.view.resetPhysicalSize(); + tester.view.resetDevicePixelRatio(); + }); + tester.view + ..physicalSize = superDeckSlideSize + ..devicePixelRatio = 1; + + final firstRelease = Completer(); + final secondRelease = Completer(); + final capture = _ScriptedCapture((call) async { + if (call == 1) { + await firstRelease.future; + return _png; + } + await secondRelease.future; + throw StateError('replacement export is still the one running'); + }); + addTearDown(() { + if (!firstRelease.isCompleted) firstRelease.complete(); + if (!secondRelease.isCompleted) { + secondRelease.complete(); + } + }); + + var closed = 0; + Widget dialog(List slides) { + return MaterialApp( + home: PdfExportDialogScreen( + slides: slides, + slideCaptureService: capture, + onClose: () => closed++, + options: PdfExportOptions(pdfSaver: (_) async => true), + ), + ); + } + + await tester.pumpWidget(dialog(createTestSlides(1))); + await _drive(tester, () => capture.calls >= 1); + + await tester.pumpWidget(dialog(createTestSlides(1))); + await _drive(tester, () => capture.calls >= 2); + + firstRelease.complete(); + await _settle(tester); + + expect(closed, 0); + expect(find.byType(PdfExportDialogScreen), findsOneWidget); + + secondRelease.complete(); + await _settle(tester); + + expect(closed, 0); + expect(tester.takeException(), isNull); + }, + ); +} + +Widget _harness(PdfController controller) { + return MaterialApp( + home: Scaffold( + body: PageView( + controller: controller.pageController, + children: [ + for (final slide in controller.slides) + RepaintBoundary( + key: controller.getSlideKey(slide), + child: controller + .getSlideReadiness(slide) + .bind(const SizedBox.expand()), + ), + ], + ), + ), + ); +} + +Future _until(WidgetTester tester, bool Function() done) async { + await tester.runAsync(() async { + for (var i = 0; i < 100 && !done(); i++) { + await tester.pump(const Duration(milliseconds: 20)); + await Future.delayed(const Duration(milliseconds: 10)); + } + }); + expect(done(), isTrue); +} + +Future _settle(WidgetTester tester) async { + await tester.runAsync(() async { + for (var i = 0; i < 15; i++) { + await tester.pump(const Duration(milliseconds: 30)); + await Future.delayed(const Duration(milliseconds: 10)); + } + }); +} + +Future _drive(WidgetTester tester, bool Function() done) async { + await tester.runAsync(() async { + for (var i = 0; i < 80 && !done(); i++) { + await tester.pump(const Duration(milliseconds: 20)); + await Future.delayed(const Duration(milliseconds: 10)); + } + }); + expect(done(), isTrue); +} + +Future _finish(WidgetTester tester, Future export) async { + var completed = false; + final done = export.whenComplete(() => completed = true); + await _until(tester, () => completed); + await done; +} + +final class _GatedCapture extends FakeSlideCaptureService { + _GatedCapture(super.bytes); + + final gates = >[]; + + @override + Future captureFromKey({ + required GlobalKey key, + required SlideCaptureQuality quality, + }) async { + final gate = Completer(); + gates.add(gate); + await gate.future; + + return super.captureFromKey(key: key, quality: quality); + } +} + +final class _FailAfterFirst extends FakeSlideCaptureService { + _FailAfterFirst(super.bytes); + + @override + Future captureFromKey({ + required GlobalKey key, + required SlideCaptureQuality quality, + }) async { + final image = await super.captureFromKey(key: key, quality: quality); + if (captureFromKeyCalls > 1) throw StateError('capture failed'); + + return image; + } +} + +final class _ScriptedCapture extends FakeSlideCaptureService { + _ScriptedCapture(this._onCall) : super(_png); + + final Future Function(int call) _onCall; + var calls = 0; + + @override + Future captureFromKey({ + required GlobalKey key, + required SlideCaptureQuality quality, + }) { + calls++; + + return _onCall(calls); + } +} From 4052d80d478d3d95bf5bdc6bcef054bcc7b85af4 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 20:44:03 -0500 Subject: [PATCH 09/11] revert(playground): drop the saved-deck ownership fix The saved-deck library is being replaced by a one-shot export, which removes the concurrent saved and generated decks this fix coordinated. The MemoryDeckLoader.reload fix moves to that change. --- .../data/data_sources/memory_deck_loader.dart | 50 +- .../domain/generated_deck_result_applier.dart | 26 +- .../wizard_generation_controller.dart | 64 +- .../wizard_generation_status.dart | 11 - .../ai/wizard/presentation/wizard_page.dart | 62 +- .../domain/deck_library_controller.dart | 138 +--- .../data_sources/memory_deck_loader_test.dart | 171 ---- .../library/domain/deck_ownership_test.dart | 740 ------------------ .../test/helpers/fake_deck_library.dart | 14 - 9 files changed, 62 insertions(+), 1214 deletions(-) delete mode 100644 packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart delete mode 100644 packages/playground/test/features/library/domain/deck_ownership_test.dart diff --git a/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart b/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart index 55ccad8a..0a556d5e 100644 --- a/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart +++ b/packages/playground/lib/core/data/data_sources/memory_deck_loader.dart @@ -11,61 +11,27 @@ class MemoryDeckLoader extends DeckLoader { final _controller = StreamController.broadcast(); bool _disposed = false; - bool _hasMarkdown = false; String? _lastLoadedMarkdown; - List? _loadedSlides; - Object? _loadError; - - void _emitDecoded(String markdown) { - try { - final slides = _codec.decode(markdown); - _loadedSlides = slides; - _loadError = null; - _controller.add(SlidesLoadedEvent(slides)); - } catch (error) { - _loadedSlides = null; - _loadError = error; - _controller.add(SlidesErrorEvent('$error', error: error)); - } - } /// Parses the given markdown and emits a [SlidesLoadedEvent]. void updateMarkdown(String markdown) { if (_disposed || markdown == _lastLoadedMarkdown) return; _lastLoadedMarkdown = markdown; - _hasMarkdown = true; - _emitDecoded(markdown); - } - /// Re-emits the last terminal preview result. - /// - /// Identical text on [updateMarkdown] stays quiet. Reload does not read a - /// saved deck from disk; a file that changed on disk is opened again through - /// the library. - @override - Future reload() async { - if (_disposed) return; - if (!_hasMarkdown) { - _controller.add(SlidesErrorEvent('No markdown has been supplied.')); - await Future.delayed(Duration.zero); - - return; - } - final error = _loadError; - if (error != null) { - _controller.add(SlidesErrorEvent('$error', error: error)); - } else { - _controller.add(SlidesLoadedEvent(_loadedSlides ?? const [])); + try { + final slides = _codec.decode(markdown); + _controller.add(SlidesLoadedEvent(slides)); + } catch (e) { + _controller.add(SlidesErrorEvent('$e', error: e)); } - // Broadcast delivery is a later microtask. Stay incomplete until that - // event has reached listeners, including a session waiting to leave - // loading. - await Future.delayed(Duration.zero); } @override Stream load() => _controller.stream; + @override + Future reload() async {} + @override Future dispose() async { _disposed = true; diff --git a/packages/playground/lib/features/ai/generation/domain/generated_deck_result_applier.dart b/packages/playground/lib/features/ai/generation/domain/generated_deck_result_applier.dart index 5eed7bed..af634b8a 100644 --- a/packages/playground/lib/features/ai/generation/domain/generated_deck_result_applier.dart +++ b/packages/playground/lib/features/ai/generation/domain/generated_deck_result_applier.dart @@ -69,9 +69,8 @@ final class GeneratedDeckResultApplier { Future _apply( DeckGenerationResult result, - GeneratedDeckApplicationGuard isValid, { - void Function()? onAccepted, - }) async { + GeneratedDeckApplicationGuard isValid, + ) async { const abandoned = GeneratedDeckApplication(published: false); if (!isValid()) return abandoned; @@ -101,11 +100,6 @@ final class GeneratedDeckResultApplier { return abandoned; } - // The saved-deck binding changes only once this attempt is accepted. - // A rejection, an abandoned write, or a throw leaves the previous deck - // answering for its own artwork. - onAccepted?.call(); - final markdown = const SlideSerializer().serialize(result.slides); _documentStore.replaceMarkdown(markdown); _deckLoader.updateMarkdown(markdown); @@ -146,16 +140,13 @@ final class GeneratedDeckResultApplier { Future apply( DeckGenerationResult result, { required GeneratedDeckApplicationGuard isValid, - void Function()? onAccepted, }) { final application = Completer(); // The queue only sequences the work. Failures reach the caller through // the completer, so one failed application cannot block the next one. _queue = _queue.then((_) async { try { - application.complete( - await _apply(result, isValid, onAccepted: onAccepted), - ); + application.complete(await _apply(result, isValid)); } catch (error, stackTrace) { application.completeError(error, stackTrace); } @@ -164,14 +155,3 @@ final class GeneratedDeckResultApplier { return application.future; } } - -/// Publishes [result] and releases the open saved deck only if that -/// publication is accepted. -Future applyGeneratedDeckResult({ - required GeneratedDeckResultApplier applier, - required DeckGenerationResult result, - required GeneratedDeckApplicationGuard isValid, - required void Function() releaseSavedDeck, -}) { - return applier.apply(result, isValid: isValid, onAccepted: releaseSavedDeck); -} diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart index 47aea5a4..5e886081 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_controller.dart @@ -38,7 +38,6 @@ final class WizardGenerationController extends ChangeNotifier { final DeckGeneratorService _service; final ApplyWizardDeckResult _applyResult; - final int Function() _deckSelectionEpoch; WizardGenerationStage _stage = .setup; @@ -58,10 +57,8 @@ final class WizardGenerationController extends ChangeNotifier { WizardGenerationController({ required DeckGeneratorService service, required ApplyWizardDeckResult applyResult, - int Function()? deckSelectionEpoch, }) : _service = service, - _applyResult = applyResult, - _deckSelectionEpoch = deckSelectionEpoch ?? (() => 0); + _applyResult = applyResult; Future _createOutline( DeckGenerationRequest request, { @@ -155,22 +152,15 @@ final class WizardGenerationController extends ChangeNotifier { _notify(); } - bool _mayAutoPublish(int selectionEpoch) => - _deckSelectionEpoch() == selectionEpoch; - Future _completeComposition( int operation, DeckGenerationResult generated, - int selectionEpoch, ) async { final GeneratedDeckApplication application; try { application = await _applyResult( generated, - isValid: () => - !_isOperationCancelled(operation) && - !_disposed && - _mayAutoPublish(selectionEpoch), + isValid: () => !_isOperationCancelled(operation) && !_disposed, ); } catch (error) { if (_isOperationCancelled(operation) || _disposed) return; @@ -182,18 +172,9 @@ final class WizardGenerationController extends ChangeNotifier { return; } if (_isOperationCancelled(operation) || _disposed) return; - // Opening a saved deck withdraws automatic publication. The result stays - // available for an explicit acceptance that publishes that generation alone. - if (!application.published) { - if (!_mayAutoPublish(selectionEpoch)) { - _result = generated; - _stage = .completed; - _progress = const GenerationProgress(.idle); - _notify(); - } - - return; - } + // The applier stops when a newer run replaced this one, so the run that + // owns the state keeps it. + if (!application.published) return; if (application.cleanupError != null) { _applyNotice = 'Some artwork from the previous deck could not be removed.'; @@ -292,7 +273,6 @@ final class WizardGenerationController extends ChangeNotifier { } final operation = ++_operationEpoch; - final selectionEpoch = _deckSelectionEpoch(); _result = null; _errorMessage = null; _failedPhase = null; @@ -326,7 +306,7 @@ final class WizardGenerationController extends ChangeNotifier { return; } - await _completeComposition(operation, generated, selectionEpoch); + await _completeComposition(operation, generated); } Future retryFailedSlides() async { @@ -341,7 +321,6 @@ final class WizardGenerationController extends ChangeNotifier { } final operation = ++_operationEpoch; - final selectionEpoch = _deckSelectionEpoch(); _errorMessage = null; _failedPhase = null; _applyNotice = null; @@ -377,36 +356,7 @@ final class WizardGenerationController extends ChangeNotifier { return; } - await _completeComposition(operation, generated, selectionEpoch); - } - - /// Publishes the retained generation as the only active deck. - /// - /// Selecting a saved deck removes an in-flight run's right to publish on - /// completion. This is the later, explicit acceptance of that same result. - Future acceptRetainedGeneration() async { - final generated = _result; - if (generated == null || isBusy || _disposed) return; - final GeneratedDeckApplication application; - try { - application = await _applyResult(generated, isValid: () => !_disposed); - } catch (error) { - if (_disposed) return; - _fail( - .composition, - 'Slides were generated but could not be loaded: $error', - ); - - return; - } - if (_disposed || !application.published) return; - if (application.cleanupError != null) { - _applyNotice = - 'Some artwork from the previous deck could not be removed.'; - } - _stage = .completed; - _progress = const GenerationProgress(.idle); - _notify(); + await _completeComposition(operation, generated); } Future retry() async { diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart index 9054eb57..0b16d97d 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_generation_status.dart @@ -32,7 +32,6 @@ class WizardGenerationStatus extends StatelessWidget { this.onStartOver, this.onSave, this.onOpenSavedDecks, - this.onShowGeneratedDeck, this.saveLabel = 'Save deck', this.isSaving = false, }); @@ -63,9 +62,6 @@ class WizardGenerationStatus extends StatelessWidget { /// Opens the list of decks already saved. final VoidCallback? onOpenSavedDecks; - /// Publishes the retained generation over the saved deck currently showing. - final VoidCallback? onShowGeneratedDeck; - /// Reflects whether this deck has been saved yet, so a second save reads as /// another copy rather than an undo. final String saveLabel; @@ -243,13 +239,6 @@ class WizardGenerationStatus extends StatelessWidget { icon: LucideIcons.save, variant: .outline, ), - if (onShowGeneratedDeck != null) - SdButton( - label: 'Show generated deck', - onPressed: onShowGeneratedDeck, - icon: LucideIcons.presentation, - variant: .outline, - ), SdButton( label: 'Present deck', onPressed: onPresent, diff --git a/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart b/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart index 33b5a95c..b79619c9 100644 --- a/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart +++ b/packages/playground/lib/features/ai/wizard/presentation/wizard_page.dart @@ -4,7 +4,6 @@ import 'package:flutter/material.dart'; import 'package:go_router/go_router.dart'; import 'package:hero_ui/hero_ui.dart'; import 'package:provider/provider.dart'; -import 'package:superdeck_builder/superdeck_builder.dart'; import '../../../../core/domain/design/presentation_image_style_catalog.dart'; import '../../../../core/domain/stores/deck_customization_store.dart'; @@ -27,31 +26,6 @@ import 'wizard_generation_status.dart'; import 'wizard_outline_review.dart'; import 'wizard_view.dart'; -/// Saves [result] as its own deck: its Markdown, artwork, and theme. -/// -/// The runtime document is not the source. Opening another saved deck leaves -/// the Wizard holding this result, and the save still writes this result alone. -Future saveRetainedWizardDeck({ - required DeckLibraryController library, - required DeckGenerationResult result, - required String name, -}) { - final theme = result.theme; - - return library.save( - name: name, - markdown: const SlideSerializer().serialize(result.slides), - images: result.generatedImages, - theme: theme == null - ? null - : SavedDeckTheme( - id: theme.descriptor.id, - version: theme.descriptor.version, - density: theme.density, - ), - ); -} - /// Host for the conversational Wizard, the app's only authoring flow. /// /// Generated Markdown is kept in memory: results are applied straight to the @@ -143,14 +117,13 @@ class WizardPage extends StatelessWidget { imageGenerator: finalImageGenerator, ), applyResult: (result, {required isValid}) { - return applyGeneratedDeckResult( - applier: resultApplier, - result: result, - isValid: isValid, - releaseSavedDeck: library.releaseOpenDeck, - ); + // A generated deck owns the runtime from here. The saved deck + // that was open must stop answering for artwork, or a new + // image could resolve to the old deck's file of the same name. + library.releaseOpenDeck(); + + return resultApplier.apply(result, isValid: isValid); }, - deckSelectionEpoch: () => library.deckSelectionEpoch, ); }, ), @@ -195,17 +168,24 @@ class _WizardExperienceState extends State<_WizardExperience> { ); if (name == null || !mounted) return; - await saveRetainedWizardDeck(library: library, result: result, name: name); + await library.save( + name: name, + images: result.generatedImages, + theme: switch (result.theme) { + final theme? => SavedDeckTheme( + id: theme.descriptor.id, + version: theme.descriptor.version, + density: theme.density, + ), + _ => null, + }, + ); } @override Widget build(BuildContext context) { final controller = context.watch(); final library = context.watch(); - final document = context.watch(); - final generatedMarkdown = controller.result == null - ? null - : const SlideSerializer().serialize(controller.result!.slides); final overlay = switch (controller.stage) { .setup => null, .planning || .composing => _CenteredScrollable( @@ -267,12 +247,6 @@ class _WizardExperienceState extends State<_WizardExperience> { }, onSave: library.canSave ? () => unawaited(_save(controller)) : null, onOpenSavedDecks: () => context.push('/decks'), - onShowGeneratedDeck: - library.openDeck != null && - generatedMarkdown != null && - document.markdown != generatedMarkdown - ? () => unawaited(controller.acceptRetainedGeneration()) - : null, saveLabel: library.lastSave == null ? 'Save deck' : 'Save another copy', diff --git a/packages/playground/lib/features/library/domain/deck_library_controller.dart b/packages/playground/lib/features/library/domain/deck_library_controller.dart index 5b0b2896..096474f6 100644 --- a/packages/playground/lib/features/library/domain/deck_library_controller.dart +++ b/packages/playground/lib/features/library/domain/deck_library_controller.dart @@ -2,7 +2,6 @@ import 'package:flutter/foundation.dart'; import '../../../core/data/data_sources/deck_library_asset_store.dart'; import '../../../core/data/data_sources/memory_deck_loader.dart'; -import '../../../core/data/mappers/deck_markdown_codec.dart'; import '../../../core/domain/design/generated_deck_style_mapper.dart'; import '../../../core/domain/design/presentation_theme_catalog.dart'; import '../../../core/domain/design/presentation_typography_catalog.dart'; @@ -27,18 +26,10 @@ class DeckLibraryController extends ChangeNotifier { final PresentationThemeCatalog _themeCatalog; final PresentationTypographyCatalog _typographyCatalog; - static const _codec = DeckMarkdownCodec(); - List _decks = const []; - SavedDeckRef? _openDeck; DeckSaveOutcome? _lastSave; String? _errorMessage; - int _mutationGeneration = 0; - int _listGeneration = 0; - int _completedMutations = 0; - int _inFlight = 0; - int _deckSelectionEpoch = 0; - bool _mutationInFlight = false; + bool _isBusy = false; DeckLibraryController({ required DeckLibrary library, @@ -57,26 +48,11 @@ class DeckLibraryController extends ChangeNotifier { _typographyCatalog = typographyCatalog ?? PresentationTypographyCatalog.withDefaults(); - void _beginOperation() { - _inFlight++; - notifyListeners(); - } - - void _endOperation() { - _inFlight--; + void _setBusy({required bool busy}) { + _isBusy = busy; notifyListeners(); } - bool _isLatestMutation(int generation) => generation == _mutationGeneration; - - bool _isLatestList(int generation) => generation == _listGeneration; - - bool _refreshIsStale(int generation, int seenMutations) { - return !_isLatestList(generation) || - _mutationInFlight || - seenMutations != _completedMutations; - } - /// Restores the appearance a deck was saved with. /// /// A selection the catalog no longer offers leaves the current theme alone @@ -103,162 +79,100 @@ class DeckLibraryController extends ChangeNotifier { /// Saved decks, newest first. List get decks => _decks; - /// The saved deck being presented, or `null` for a generated one. - SavedDeckRef? get openDeck => _openDeck; - - /// Advances when a saved deck is committed to the runtime. - /// - /// A generation captures this before it can publish. Opening another deck - /// withdraws that automatic publication; the retained result can still be - /// accepted explicitly. - int get deckSelectionEpoch => _deckSelectionEpoch; - /// What the last save wrote, for the notice the Wizard shows. DeckSaveOutcome? get lastSave => _lastSave; /// A user-facing reason the last save or open did not finish. String? get errorMessage => _errorMessage; - /// Whether a save, open, or refresh is running. - bool get isBusy => _inFlight > 0; + /// Whether a save or open is running. + bool get isBusy => _isBusy; /// Whether decks can be stored at all on this platform. bool get canSave => _library.canSave; - /// Writes one deck named [name]. + /// Writes the current document as a new deck named [name]. /// - /// [markdown] is the document to persist. When it is omitted, the current - /// runtime document is saved. [images] and [theme] belong to that same deck. - /// An explicit [markdown] also becomes the deck on screen, so a save cannot - /// store one deck's text beside another's artwork. + /// [images] carries the artwork of the generation that produced the + /// document, and [theme] the selection it was generated with. Future save({ required String name, - String? markdown, List images = const [], SavedDeckTheme? theme, }) async { - if (_mutationInFlight) return false; - _mutationInFlight = true; - final mutation = ++_mutationGeneration; - // A refresh that started earlier, or that finishes while this save is - // still writing, must not replace the deck this save commits. - _listGeneration++; - final persisted = markdown ?? _documentStore.markdown; - _beginOperation(); + if (_isBusy) return false; + _errorMessage = null; + _setBusy(busy: true); try { final result = await _library.save( name: name, - markdown: persisted, + markdown: _documentStore.markdown, images: images, theme: theme, ); - final current = _isLatestMutation(mutation); switch (result) { case Ok(:final value): - if (current) { - _lastSave = value; - // The saved copy, not the run's memory, now owns this artwork. - _openDeck = value.ref; - _assetStore.bindTo(value.ref); - _decks = [value.ref, ..._decks.where((ref) => ref != value.ref)]; - _errorMessage = null; - if (markdown != null) { - _documentStore.replaceMarkdown(persisted); - _deckLoader.updateMarkdown(persisted); - if (theme != null) _errorMessage = _applyTheme(theme); - } - _completedMutations++; - } + _lastSave = value; + // The saved copy, not the run's memory, now owns this artwork. + _assetStore.bindTo(value.ref); + _decks = [value.ref, ..._decks.where((ref) => ref != value.ref)]; return true; case Failure(:final error): - if (current) { - _errorMessage = '$error'; - _completedMutations++; - } + _errorMessage = '$error'; return false; } } finally { - _mutationInFlight = false; - _endOperation(); + _setBusy(busy: false); } } /// Reloads the list of saved decks. - /// - /// Refresh may overlap a save or an open. It does not clear [isBusy] while - /// that other operation is still running, and a stale completion does not - /// replace a newer deck list or error. Future refresh() async { - final generation = ++_listGeneration; - final seenMutations = _completedMutations; - _beginOperation(); + _errorMessage = null; + _setBusy(busy: true); try { switch (await _library.list()) { case Ok(:final value): - if (_refreshIsStale(generation, seenMutations)) return; _decks = value; - _errorMessage = null; case Failure(:final error): - if (_refreshIsStale(generation, seenMutations)) return; _errorMessage = '$error'; } } finally { - _endOperation(); + _setBusy(busy: false); } } /// Publishes the saved deck at [ref] for presentation. /// /// The deck is read-only: it is loaded into the preview and the renderer, - /// with its own artwork and the theme it was saved with. Document, artwork - /// binding, theme, and slides change together, and only after the Markdown - /// decodes. + /// with its own artwork and the theme it was saved with. Future open(SavedDeckRef ref) async { - if (_mutationInFlight) return false; - _mutationInFlight = true; - final mutation = ++_mutationGeneration; - _listGeneration++; - _beginOperation(); + if (_isBusy) return false; + _errorMessage = null; + _setBusy(busy: true); try { switch (await _library.open(ref)) { case Ok(:final value): - if (!_isLatestMutation(mutation)) return false; - try { - _codec.decode(value.markdown); - } catch (error) { - _errorMessage = '$error'; - _completedMutations++; - - return false; - } - if (!_isLatestMutation(mutation)) return false; - _openDeck = value.ref; _assetStore.bindTo(value.ref); _documentStore.replaceMarkdown(value.markdown); _deckLoader.updateMarkdown(value.markdown); _errorMessage = _applyTheme(value.theme); - _deckSelectionEpoch++; - _completedMutations++; return true; case Failure(:final error): - if (!_isLatestMutation(mutation)) return false; _errorMessage = '$error'; - _completedMutations++; return false; } } finally { - _mutationInFlight = false; - _endOperation(); + _setBusy(busy: false); } } /// Forgets the open saved deck, so a new generation owns the runtime again. void releaseOpenDeck() { - _openDeck = null; _lastSave = null; _assetStore.bindTo(null); notifyListeners(); diff --git a/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart b/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart deleted file mode 100644 index 881a9f49..00000000 --- a/packages/playground/test/core/data/data_sources/memory_deck_loader_test.dart +++ /dev/null @@ -1,171 +0,0 @@ -import 'package:flutter/material.dart' show Colors; -import 'package:flutter_test/flutter_test.dart'; -import 'package:google_fonts/google_fonts.dart'; -import 'package:playground/core/data/data_sources/deck_library_asset_store.dart'; -import 'package:playground/core/data/data_sources/memory_asset_cache_store.dart'; -import 'package:playground/core/data/data_sources/memory_deck_loader.dart'; -import 'package:playground/core/domain/stores/deck_customization_store.dart'; -import 'package:playground/core/domain/stores/deck_document_store.dart'; -import 'package:playground/features/library/domain/deck_library_controller.dart'; -import 'package:superdeck/superdeck.dart'; -import 'package:superdeck_core/superdeck_core.dart'; - -import '../../../helpers/fake_deck_library.dart'; - -const _markdown = ''' ---- -title: Loaded ---- - -A deck that already decoded. -'''; - -void main() { - TestWidgetsFlutterBinding.ensureInitialized(); - setUpAll(() => GoogleFonts.config.allowRuntimeFetching = false); - - test('reload re-emits unchanged slides before it completes', () async { - final harness = _harness(); - harness.loader.updateMarkdown(_markdown); - await Future.delayed(Duration.zero); - expect(harness.deck.session.isLoading.value, isFalse); - final title = - harness.deck.session.loadedSlides.value?.single.options?.title; - expect(title, 'Loaded'); - - final order = await _reload(harness.loader, harness.deck); - - expect(order.eventBeforeComplete, isTrue); - expect(order.event, isA()); - expect(harness.deck.session.isLoading.value, isFalse); - expect( - harness.deck.session.loadedSlides.value?.single.options?.title, - title, - ); - expect(harness.deck.session.error.value, isNull); - }); - - test( - 'reload re-emits a terminal error for markdown that failed to decode', - () async { - final harness = _harness(); - harness.loader.updateMarkdown('@column\n'); - await Future.delayed(Duration.zero); - expect(harness.deck.session.loadedSlides.value, isNull); - expect(harness.deck.session.isLoading.value, isFalse); - expect(harness.deck.session.error.value, isNotNull); - - final order = await _reload(harness.loader, harness.deck); - - expect(order.eventBeforeComplete, isTrue); - expect(order.event, isA()); - expect(harness.deck.session.isLoading.value, isFalse); - expect(harness.deck.session.loadedSlides.value, isNull); - expect(harness.deck.session.error.value, isNotNull); - }, - ); - - test( - 'reload without markdown emits a terminal error and clears loading', - () async { - final harness = _harness(); - expect(harness.deck.session.loadedSlides.value, isNull); - - final order = await _reload(harness.loader, harness.deck); - - expect(order.eventBeforeComplete, isTrue); - expect(order.event, isA()); - expect(harness.deck.session.isLoading.value, isFalse); - expect(harness.deck.session.loadedSlides.value, isNull); - expect(harness.deck.session.error.value, isNotNull); - }, - ); - - test('an ordinary identical update emits no new preview event', () async { - final harness = _harness(); - harness.loader.updateMarkdown(_markdown); - await Future.delayed(Duration.zero); - final events = []; - final subscription = harness.loader.load().listen(events.add); - addTearDown(subscription.cancel); - - harness.loader.updateMarkdown(_markdown); - await Future.delayed(Duration.zero); - - expect(events, isEmpty); - }); - - test('reload does not reread a saved deck from the library', () async { - final library = FakeDeckLibrary(); - final loader = MemoryDeckLoader(); - final document = DeckDocumentStore(markdown: _markdown); - final assets = DeckLibraryAssetStore( - library: library, - fallback: MemoryAssetCacheStore(), - ); - final deck = DeckController( - deckLoader: loader, - options: DeckOptions(), - assetCacheStore: assets, - ); - final customization = DeckCustomizationStore( - deck, - background: Colors.black, - foreground: Colors.white, - ); - final controller = DeckLibraryController( - library: library, - documentStore: document, - deckLoader: loader, - customizationStore: customization, - assetStore: assets, - ); - addTearDown(() { - controller.dispose(); - customization.dispose(); - deck.dispose(); - document.dispose(); - }); - - final saved = await controller.save(name: 'Saved'); - expect(saved, isTrue); - final opened = await controller.open(controller.openDeck!); - expect(opened, isTrue); - final opensBeforeReload = library.openCount; - - await deck.session.reload(); - - expect(library.openCount, opensBeforeReload); - expect(deck.session.isLoading.value, isFalse); - }); -} - -({MemoryDeckLoader loader, DeckController deck}) _harness() { - final loader = MemoryDeckLoader(); - final deck = DeckController( - deckLoader: loader, - options: DeckOptions(), - assetCacheStore: MemoryAssetCacheStore(), - ); - addTearDown(deck.dispose); - - return (loader: loader, deck: deck); -} - -Future<({bool eventBeforeComplete, SlidesEvent? event})> _reload( - MemoryDeckLoader loader, - DeckController deck, -) async { - var completed = false; - SlidesEvent? event; - var eventBeforeComplete = false; - final subscription = loader.load().listen((next) { - event = next; - eventBeforeComplete = !completed; - }); - addTearDown(subscription.cancel); - - await deck.session.reload().whenComplete(() => completed = true); - - return (eventBeforeComplete: eventBeforeComplete, event: event); -} diff --git a/packages/playground/test/features/library/domain/deck_ownership_test.dart b/packages/playground/test/features/library/domain/deck_ownership_test.dart deleted file mode 100644 index 4b60bdbb..00000000 --- a/packages/playground/test/features/library/domain/deck_ownership_test.dart +++ /dev/null @@ -1,740 +0,0 @@ -import 'dart:async'; - -import 'package:flutter/material.dart' show Color, Colors; -import 'package:flutter_test/flutter_test.dart'; -import 'package:google_fonts/google_fonts.dart'; -import 'package:playground/core/data/data_sources/deck_file.dart'; -import 'package:playground/core/data/data_sources/deck_library_asset_store.dart'; -import 'package:playground/core/data/data_sources/memory_asset_cache_store.dart'; -import 'package:playground/core/data/data_sources/memory_deck_loader.dart'; -import 'package:playground/core/domain/design/presentation_theme_catalog.dart'; -import 'package:playground/core/domain/design/presentation_typography_catalog.dart'; -import 'package:playground/core/domain/generated_image_asset.dart'; -import 'package:playground/core/result.dart'; -import 'package:playground/core/domain/stores/deck_customization_store.dart'; -import 'package:playground/core/domain/stores/deck_document_store.dart'; -import 'package:playground/features/ai/generation/core/engine/schemas/outline_schema.dart'; -import 'package:playground/features/ai/generation/core/engine/services/deck_generation_request.dart'; -import 'package:playground/features/ai/generation/core/engine/services/deck_generator_service.dart'; -import 'package:playground/features/ai/generation/domain/generated_deck_result_applier.dart'; -import 'package:playground/features/ai/wizard/presentation/wizard_generation_controller.dart'; -import 'package:playground/features/ai/wizard/presentation/wizard_page.dart'; -import 'package:playground/features/library/domain/deck_library_controller.dart'; -import 'package:playground/features/library/domain/saved_deck.dart'; -import 'package:superdeck/superdeck.dart'; -import 'package:superdeck_core/superdeck_core.dart' hide Ok; - -import '../../../helpers/fake_deck_library.dart'; - -const _savedMarkdown = ''' ---- -title: Saved A ---- - -Saved deck A stays on screen. -'''; - -const _otherMarkdown = ''' ---- -title: Saved B ---- - -Saved deck B stays on screen. -'''; - -const _boldTheme = SavedDeckTheme( - id: 'bold-product', - version: 1, - density: 'balanced', -); - -const _paperTheme = SavedDeckTheme( - id: 'technical-paper', - version: 1, - density: 'balanced', -); - -void main() { - TestWidgetsFlutterBinding.ensureInitialized(); - setUpAll(() => GoogleFonts.config.allowRuntimeFetching = false); - - group('generated result application', () { - test('a rejected application leaves the saved deck intact', () async { - final scope = await _showingSavedDeck(); - - final application = await applyGeneratedDeckResult( - applier: scope.applier, - result: _generatedResult(), - isValid: () => false, - releaseSavedDeck: scope.libraryController.releaseOpenDeck, - ); - - expect(application.published, isFalse); - _expectSavedDeckIntact(scope); - }); - - test('an abandoned application leaves the saved deck intact', () async { - final scope = await _showingSavedDeck(); - var accepted = true; - - final application = await applyGeneratedDeckResult( - applier: scope.applier, - result: _generatedResult(), - isValid: () { - final current = accepted; - accepted = false; - return current; - }, - releaseSavedDeck: scope.libraryController.releaseOpenDeck, - ); - - expect(application.published, isFalse); - _expectSavedDeckIntact(scope); - }); - - test('a thrown application leaves the saved deck intact', () async { - final scope = await _showingSavedDeck(fallback: _ThrowingAssetCache()); - - await expectLater( - applyGeneratedDeckResult( - applier: scope.applier, - result: _generatedResult(), - isValid: () => true, - releaseSavedDeck: scope.libraryController.releaseOpenDeck, - ), - throwsA(isA()), - ); - - _expectSavedDeckIntact(scope); - }); - }); - - group('one deck per save and publication', () { - test( - 'saving a retained generation writes that generation after another deck was opened', - () async { - final scope = await _showingSavedDeck(markdown: _otherMarkdown); - final wizard = _wizard( - scope, - result: _generatedResult(assetKey: 'generated-a.png'), - ); - - await wizard.controller.createOutline(_request); - await wizard.controller.generateSlides(); - expect(wizard.controller.result, isNotNull); - - final reopened = await scope.libraryController.open(scope.savedRef); - expect(reopened, isTrue); - await _flush(); - expect(scope.document.markdown, _otherMarkdown); - - final saved = await saveRetainedWizardDeck( - library: scope.libraryController, - result: wizard.controller.result!, - name: 'Generated copy', - ); - - expect(saved, isTrue); - final path = scope.libraryController.openDeck!.reference.path; - final stored = scope.library.markdown[path]!; - expect(stored, contains('Only the generated deck.')); - expect(stored, contains('generated-a.png')); - expect(stored, isNot(contains('Saved deck B stays on screen.'))); - expect(scope.library.assets[path]?['generated-a.png'], [1, 2, 3]); - expect(scope.library.themes[path], _paperTheme); - expect(scope.document.markdown, stored); - await _flush(); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - 'Generated A', - ); - }, - ); - - test( - 'a saved deck opened during generation is not replaced, and accepting the generation publishes only that generation', - () async { - final scope = _newScope(); - await scope.library.save( - name: 'Saved A', - markdown: _savedMarkdown, - images: [ - GeneratedImageAsset.success( - assetKey: 'saved-a.png', - bytes: [4, 5, 6], - ), - ], - theme: _boldTheme, - ); - final savedRef = scope.library.saved.single; - final gate = Completer(); - final wizard = _wizard(scope, pendingResult: gate); - - await wizard.controller.createOutline(_request); - final composition = wizard.controller.generateSlides(); - await Future.delayed(Duration.zero); - expect(wizard.controller.stage, WizardGenerationStage.composing); - - final opened = await scope.libraryController.open(savedRef); - expect(opened, isTrue); - await _flush(); - final saved = _snapshot(scope); - final artwork = await scope.assets.resolve('saved-a.png'); - expect(artwork, isNotNull); - - gate.complete(_generatedResult()); - await composition; - - expect(wizard.controller.stage, WizardGenerationStage.completed); - expect(wizard.controller.result, isNotNull); - _expectSnapshot(scope, saved); - expect(await scope.assets.resolve('saved-a.png'), artwork); - - await wizard.controller.acceptRetainedGeneration(); - await _flush(); - - expect(scope.libraryController.openDeck, isNull); - expect(scope.document.markdown, contains('Only the generated deck.')); - expect( - scope.document.markdown, - isNot(contains('Saved deck A stays on screen.')), - ); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - 'Generated A', - ); - expect(scope.customization.background, isNot(saved.background)); - final generatedUri = await scope.assets.resolve('generated-a.png'); - final savedUri = await scope.assets.resolve('saved-a.png'); - expect(generatedUri, isNotNull); - expect(savedUri, isNull); - }, - ); - }); - - group('library open and refresh', () { - test( - 'a malformed open reports failure and keeps the previous deck', - () async { - final scope = await _showingSavedDeck(); - final saved = _snapshot(scope); - final artwork = await scope.assets.resolve('saved-a.png'); - final broken = await scope.library.save( - name: 'Broken', - markdown: '@column\n', - ); - final brokenRef = switch (broken) { - Ok(:final value) => value.ref, - Failure(:final error) => fail( - 'could not stage a broken deck: $error', - ), - }; - - final opened = await scope.libraryController.open(brokenRef); - - expect(opened, isFalse); - expect(scope.libraryController.errorMessage, isNotNull); - _expectSnapshot(scope, saved); - expect(await scope.assets.resolve('saved-a.png'), artwork); - }, - ); - - test( - 'an overlapping refresh does not mark the library idle while a save is still running', - () async { - final scope = _newScope(); - final saveEntered = Completer(); - final releaseSave = Completer(); - scope.library.beforeSave = () async { - if (!saveEntered.isCompleted) saveEntered.complete(); - await releaseSave.future; - }; - final saving = scope.libraryController.save( - name: 'Current', - markdown: _savedMarkdown, - images: [ - GeneratedImageAsset.success( - assetKey: 'saved-a.png', - bytes: [4, 5, 6], - ), - ], - theme: _boldTheme, - ); - await saveEntered.future; - - final listEntered = Completer(); - final releaseList = Completer(); - scope.library.beforeList = () async { - if (!listEntered.isCompleted) listEntered.complete(); - await releaseList.future; - }; - scope.library.listError = Exception('stale list'); - final refreshing = scope.libraryController.refresh(); - await listEntered.future; - releaseList.complete(); - await refreshing; - - expect(scope.libraryController.isBusy, isTrue); - - releaseSave.complete(); - final saved = await saving; - await _flush(); - - expect(saved, isTrue); - expect(scope.libraryController.isBusy, isFalse); - expect(scope.libraryController.decks.single.name, 'Current'); - expect(scope.libraryController.openDeck?.name, 'Current'); - expect(scope.libraryController.lastSave, isNotNull); - expect(scope.libraryController.errorMessage, isNull); - expect(scope.document.markdown, _savedMarkdown); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - 'Saved A', - ); - expect(await scope.assets.resolve('saved-a.png'), isNotNull); - expect(scope.customization.background, isNot(Colors.black)); - }, - ); - - test( - 'a refresh started during a save does not erase that save after it commits', - () async { - final scope = _newScope(); - final saveEntered = Completer(); - final releaseSave = Completer(); - scope.library.beforeSave = () async { - if (!saveEntered.isCompleted) saveEntered.complete(); - await releaseSave.future; - }; - final listEntered = Completer(); - final releaseList = Completer(); - scope.library.beforeList = () async { - if (!listEntered.isCompleted) listEntered.complete(); - await releaseList.future; - }; - - final saving = scope.libraryController.save(name: 'Current'); - await saveEntered.future; - final refreshing = scope.libraryController.refresh(); - await listEntered.future; - - releaseSave.complete(); - final saved = await saving; - expect(saved, isTrue); - expect(scope.libraryController.decks.single.name, 'Current'); - expect(scope.libraryController.openDeck?.name, 'Current'); - expect(scope.libraryController.isBusy, isTrue); - - scope.library.listError = Exception('late stale list'); - releaseList.complete(); - await refreshing; - - expect(scope.libraryController.decks.single.name, 'Current'); - expect(scope.libraryController.openDeck?.name, 'Current'); - expect(scope.libraryController.lastSave, isNotNull); - expect(scope.libraryController.errorMessage, isNull); - expect(scope.libraryController.isBusy, isFalse); - }, - ); - - test('an overlapping refresh does not drop a successful open', () async { - final scope = _newScope(); - scope.document.replaceMarkdown(_savedMarkdown); - final saved = await scope.libraryController.save(name: 'Saved A'); - expect(saved, isTrue); - final ref = scope.libraryController.openDeck!; - scope.libraryController.releaseOpenDeck(); - scope.document.replaceMarkdown('# Draft\n'); - - final openEntered = Completer(); - final releaseOpen = Completer(); - scope.library.beforeOpen = () async { - if (!openEntered.isCompleted) openEntered.complete(); - await releaseOpen.future; - }; - final opening = scope.libraryController.open(ref); - await openEntered.future; - - final listEntered = Completer(); - final releaseList = Completer(); - scope.library.beforeList = () async { - if (!listEntered.isCompleted) listEntered.complete(); - await releaseList.future; - }; - scope.library.listError = Exception('stale list'); - final refreshing = scope.libraryController.refresh(); - await listEntered.future; - releaseList.complete(); - await refreshing; - - expect(scope.libraryController.isBusy, isTrue); - expect(scope.document.markdown, '# Draft\n'); - - releaseOpen.complete(); - final opened = await opening; - await _flush(); - - expect(opened, isTrue); - expect(scope.libraryController.isBusy, isFalse); - expect(scope.libraryController.openDeck, ref); - expect(scope.document.markdown, _savedMarkdown); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - 'Saved A', - ); - expect(scope.libraryController.errorMessage, isNull); - }); - - test( - 'a slower refresh does not replace the deck list or error from a newer save', - () async { - final scope = _newScope(); - final listEntered = Completer(); - final releaseList = Completer(); - scope.library.beforeList = () async { - if (!listEntered.isCompleted) listEntered.complete(); - await releaseList.future; - }; - scope.library.listError = Exception('older list'); - final refreshing = scope.libraryController.refresh(); - await listEntered.future; - - final saved = await scope.libraryController.save(name: 'Newer'); - - expect(saved, isTrue); - expect(scope.libraryController.isBusy, isTrue); - expect(scope.libraryController.decks.single.name, 'Newer'); - expect(scope.libraryController.errorMessage, isNull); - - releaseList.complete(); - await refreshing; - - expect(scope.libraryController.decks.single.name, 'Newer'); - expect(scope.libraryController.errorMessage, isNull); - expect(scope.libraryController.isBusy, isFalse); - }, - ); - - test( - 'a slower refresh does not replace the error from a newer open', - () async { - final scope = await _showingSavedDeck(); - final listEntered = Completer(); - final releaseList = Completer(); - scope.library.beforeList = () async { - if (!listEntered.isCompleted) listEntered.complete(); - await releaseList.future; - }; - scope.library.listError = Exception('older list'); - final refreshing = scope.libraryController.refresh(); - await listEntered.future; - scope.library.openError = Exception('missing deck'); - - final opened = await scope.libraryController.open(scope.savedRef); - - expect(opened, isFalse); - expect(scope.libraryController.isBusy, isTrue); - expect(scope.libraryController.errorMessage, contains('missing deck')); - - releaseList.complete(); - await refreshing; - - expect(scope.libraryController.errorMessage, contains('missing deck')); - expect(scope.libraryController.decks, isNotEmpty); - }, - ); - }); -} - -Future _flush() => Future.delayed(Duration.zero); - -const _request = DeckGenerationRequest( - userIntent: 'Ownership', - slideCount: 1, - themeId: 'technical-paper', -); - -typedef _OwnedScope = ({ - DeckLibraryController libraryController, - FakeDeckLibrary library, - DeckDocumentStore document, - MemoryDeckLoader loader, - DeckCustomizationStore customization, - DeckLibraryAssetStore assets, - DeckController deck, - GeneratedDeckResultApplier applier, - SavedDeckRef savedRef, - Uri? artworkUri, - Color background, - String? slideTitle, -}); - -_OwnedScope _newScope({AssetCacheStore? fallback}) { - final library = FakeDeckLibrary(); - final memory = fallback ?? MemoryAssetCacheStore(); - final assets = DeckLibraryAssetStore(library: library, fallback: memory); - final document = DeckDocumentStore(markdown: '# Draft\n'); - final loader = MemoryDeckLoader(); - final deck = DeckController( - deckLoader: loader, - options: DeckOptions(), - assetCacheStore: assets, - ); - final customization = DeckCustomizationStore( - deck, - background: Colors.black, - foreground: Colors.white, - ); - final libraryController = DeckLibraryController( - library: library, - documentStore: document, - deckLoader: loader, - customizationStore: customization, - assetStore: assets, - ); - final applier = GeneratedDeckResultApplier( - documentStore: document, - deckLoader: loader, - assetCacheStore: assets, - customizationStore: customization, - ); - addTearDown(() { - libraryController.dispose(); - customization.dispose(); - deck.dispose(); - document.dispose(); - }); - - return ( - libraryController: libraryController, - library: library, - document: document, - loader: loader, - customization: customization, - assets: assets, - deck: deck, - applier: applier, - savedRef: SavedDeckRef( - reference: DeckFileReference(path: '/unset'), - name: 'unset', - savedAt: DateTime.utc(2026), - ), - artworkUri: null, - background: Colors.black, - slideTitle: null, - ); -} - -Future<_OwnedScope> _showingSavedDeck({ - AssetCacheStore? fallback, - String markdown = _savedMarkdown, - String name = 'Saved A', - String assetKey = 'saved-a.png', - SavedDeckTheme theme = _boldTheme, -}) async { - final scope = _newScope(fallback: fallback); - scope.document.replaceMarkdown(markdown); - final saved = await scope.libraryController.save( - name: name, - images: [ - GeneratedImageAsset.success(assetKey: assetKey, bytes: [4, 5, 6]), - ], - theme: theme, - ); - expect(saved, isTrue); - final ref = scope.libraryController.openDeck!; - final opened = await scope.libraryController.open(ref); - expect(opened, isTrue); - final uri = await scope.assets.resolve(assetKey); - expect(uri, isNotNull); - expect(scope.deck.session.loadedSlides.value, isNotNull); - - return ( - libraryController: scope.libraryController, - library: scope.library, - document: scope.document, - loader: scope.loader, - customization: scope.customization, - assets: scope.assets, - deck: scope.deck, - applier: scope.applier, - savedRef: ref, - artworkUri: uri, - background: scope.customization.background, - slideTitle: scope.deck.session.loadedSlides.value?.single.options?.title, - ); -} - -typedef _Snapshot = ({ - SavedDeckRef? openDeck, - String markdown, - Uri? artworkUri, - Color background, - String? slideTitle, -}); - -_Snapshot _snapshot(_OwnedScope scope) => ( - openDeck: scope.libraryController.openDeck, - markdown: scope.document.markdown, - artworkUri: scope.artworkUri, - background: scope.customization.background, - slideTitle: scope.deck.session.loadedSlides.value?.single.options?.title, -); - -void _expectSnapshot(_OwnedScope scope, _Snapshot saved) { - expect(scope.libraryController.openDeck, saved.openDeck); - expect(scope.document.markdown, saved.markdown); - expect(scope.customization.background, saved.background); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - saved.slideTitle, - ); -} - -Future _expectSavedDeckIntact(_OwnedScope scope) async { - expect(scope.libraryController.openDeck, scope.savedRef); - expect(scope.document.markdown, _savedMarkdown); - expect(await scope.assets.resolve('saved-a.png'), scope.artworkUri); - expect(scope.customization.background, scope.background); - expect( - scope.deck.session.loadedSlides.value?.single.options?.title, - scope.slideTitle, - ); -} - -DeckGenerationResult _generatedResult({String assetKey = 'generated-a.png'}) { - final themes = PresentationThemeCatalog.withDefaults(); - final typography = PresentationTypographyCatalog.withDefaults(); - - return DeckGenerationResult.success( - slides: [_generatedSlide(assetKey)], - plan: _plan(), - theme: themes.resolve( - id: 'technical-paper', - version: 1, - density: 'balanced', - typographyCatalog: typography, - ), - generatedImages: [ - GeneratedImageAsset.success(assetKey: assetKey, bytes: [1, 2, 3]), - ], - ); -} - -Slide _generatedSlide(String assetKey) => Slide.parse({ - 'key': 'opening', - 'options': {'title': 'Generated A', 'style': 'visual'}, - 'sections': [ - { - 'type': 'section', - 'blocks': [ - { - 'type': 'block', - 'content': '## Generated A\n\nOnly the generated deck.', - }, - { - 'type': 'widget', - 'name': 'image', - 'args': {'src': assetKey, 'fit': 'cover'}, - }, - ], - }, - ], -}); - -DeckPlan _plan() => DeckPlan.parse({ - 'topic': 'Ownership', - 'story': 'One deck is saved and shown.', - 'theme': {'id': 'technical-paper', 'version': 1, 'density': 'balanced'}, - 'sections': [ - { - 'key': 'main', - 'title': 'Main', - 'purpose': 'Keep one deck together.', - 'transition': 'Close clearly.', - 'slideKeys': ['opening'], - }, - ], - 'slides': [ - { - 'key': 'opening', - 'title': 'Generated A', - 'purpose': 'Show the generated deck.', - 'sectionKey': 'main', - 'assertion': 'The generated deck stands alone.', - 'contentUnits': ['Only the generated deck.'], - 'narrativeRole': 'opening', - 'contentBrief': 'Open with the generated deck.', - 'continuity': 'Lead into the story.', - 'composition': 'content', - 'treatment': 'visual', - 'density': 'balanced', - 'elements': [], - }, - ], -}); - -({WizardGenerationController controller}) _wizard( - _OwnedScope scope, { - DeckGenerationResult? result, - Completer? pendingResult, -}) { - final service = _ScriptedGenerator( - result: result ?? _generatedResult(), - pendingResult: pendingResult, - ); - final controller = WizardGenerationController( - service: service, - applyResult: (generated, {required isValid}) { - return applyGeneratedDeckResult( - applier: scope.applier, - result: generated, - isValid: isValid, - releaseSavedDeck: scope.libraryController.releaseOpenDeck, - ); - }, - deckSelectionEpoch: () => scope.libraryController.deckSelectionEpoch, - ); - addTearDown(controller.dispose); - - return (controller: controller); -} - -final class _ScriptedGenerator extends DeckGeneratorService { - _ScriptedGenerator({required this.result, this.pendingResult}) - : super(apiKey: 'test-key'); - - final DeckGenerationResult result; - final Completer? pendingResult; - - @override - Future plan( - DeckGenerationRequest request, { - onProgress, - onTrace, - isCancelled, - }) async { - return DeckPlanningResult.success(_plan()); - } - - @override - Future generateFromPlan( - DeckGenerationRequest request, - DeckPlan approvedPlan, { - onProgress, - onTrace, - isCancelled, - }) { - final pending = pendingResult; - if (pending != null) return pending.future; - - return Future.value(result); - } -} - -final class _ThrowingAssetCache extends MemoryAssetCacheStore { - @override - Future write(String assetKey, List bytes) { - throw StateError('cannot store artwork'); - } -} diff --git a/packages/playground/test/helpers/fake_deck_library.dart b/packages/playground/test/helpers/fake_deck_library.dart index 6512ea6b..73100a70 100644 --- a/packages/playground/test/helpers/fake_deck_library.dart +++ b/packages/playground/test/helpers/fake_deck_library.dart @@ -18,13 +18,6 @@ class FakeDeckLibrary implements DeckLibrary { Exception? openError; Exception? listError; int saveCount = 0; - int openCount = 0; - - /// Held by tests that need [save], [open], or [list] to finish in an - /// order the controller does not choose. - Future Function()? beforeSave; - Future Function()? beforeOpen; - Future Function()? beforeList; @override Future> save({ @@ -34,8 +27,6 @@ class FakeDeckLibrary implements DeckLibrary { SavedDeckTheme? theme, }) async { saveCount++; - final saveHook = beforeSave; - if (saveHook != null) await saveHook(); final error = saveError; if (error != null) return Result.error(error); @@ -74,8 +65,6 @@ class FakeDeckLibrary implements DeckLibrary { @override Future>> list() async { - final listHook = beforeList; - if (listHook != null) await listHook(); final error = listError; return error != null ? Result.error(error) : Result.ok(List.of(saved)); @@ -83,9 +72,6 @@ class FakeDeckLibrary implements DeckLibrary { @override Future> open(SavedDeckRef ref) async { - openCount++; - final openHook = beforeOpen; - if (openHook != null) await openHook(); final error = openError; if (error != null) return Result.error(error); final content = markdown[ref.reference.path]; From e3b572043b7abb985ac7c81eddd7fa2c5cfe2bf9 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 20:54:25 -0500 Subject: [PATCH 10/11] docs: note that deck export supersedes the audited library --- .../superdeck_architecture_adversarial_audit.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md index 374dd2c3..5cd9081c 100644 --- a/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md +++ b/docs/maintainers/architecture-review/superdeck_architecture_adversarial_audit.md @@ -1,5 +1,7 @@ # SuperDeck architecture audit +> **Status:** The saved-deck library this audit examines is replaced by deck export in [#124](https://github.com/conceptadev/superdeck/pull/124). Its publication, saving, and ownership findings no longer apply. The memory-reload fix moved to #124, and the PDF terminal cleanup ships with this branch. + **Decision:** Investigate publication and saving first, then memory reload, then PDF terminal cleanup. Shared-module placement and late-subscriber replay stay open. This branch records those limits. It does not authorize a class merger. **Repository:** `conceptadev/superdeck` From ff533c2234030335d358eaaacb5b1b9be5f231de Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 22 Sep 2026 20:57:25 -0500 Subject: [PATCH 11/11] docs(pdf): note single-run exports and released captures --- packages/plugins/pdf/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/plugins/pdf/CHANGELOG.md b/packages/plugins/pdf/CHANGELOG.md index 9dea8e28..b5193cb3 100644 --- a/packages/plugins/pdf/CHANGELOG.md +++ b/packages/plugins/pdf/CHANGELOG.md @@ -4,6 +4,8 @@ coordinated Ack 1.2 migration. - Capture PDF slide images with good quality on all platforms. +- Run one PDF export at a time, and release captured slide images when an + export fails, is cancelled, or the export dialog closes. - Use `FileSaver.saveFile` for default PDF saves on web and Linux, and surface unexpected save failures as export failures.