Conversation
- Introduce Gallery to manage app shots and readiness state - Add AppShot class with types, headlines, badges, and trust marks - Define GalleryTemplate and ScreenTemplate for layout structure - Implement GalleryPalette for color scheme management - Provide comprehensive tests for gallery components and helpers
- Make ScreenTemplate, TextSlot, DeviceSlot, Decoration, and GalleryPalette Codable - Implement custom Codable for GalleryTemplate to encode screens as dictionary keyed by raw values - Default GalleryTemplate screens parameter to empty dictionary for convenience
- Implement custom Decoder init for ScreenTemplate - Add comprehensive JSON round-trip tests for gallery types - Verify encoding and decoding of nested structures and optional properties
- Implement GalleryHTMLRenderer to produce responsive HTML panels - Add compose method to AppShot for template-based rendering - Extend Gallery with renderAll and renderShot for full and single shot rendering - Include comprehensive tests for composition and rendering behavior
- define GalleryTemplateProvider and GalleryTemplateRepository protocols - implement AggregateGalleryTemplateRepository for combined provider results - add unit tests for repository listing and fetching templates by id
- Add GET /app-shots/gallery-templates route in AppShotsController - Introduce GalleryTemplate Presentable and AffordanceProviding conformances - Inject GalleryTemplateRepository in REST routes setup
- Implement GalleryTemplate.previewHTML for self-contained preview - Add GalleryHTMLRenderer.renderPreviewPage to render full preview page - Encode previewHTML in GalleryTemplate Codable conformance
- Extend GalleryTemplate with description and background properties - Update Codable conformance to include new fields - Enhance GalleryHTMLRenderer to render previews using gallery background and wireframe phone UI - Add light/dark background detection for styling adjustments
- Render gallery preview as horizontal strip with sample content - Use mock gallery and panels for realistic multi-panel display - Simplify HTML output with flexible styled container
- Add tagline, body, and trustMarks properties to AppShot model - Update GalleryHTMLRenderer to render tagline, body text, trust marks, and floating badges with adaptive colors - Revise preview page with realistic sample content including taglines, badges, and body text
- Reduce panel width and border radius for a refined look - Adjust body and container styles for better alignment and scrolling
- Implement JSON encoding and decoding for Gallery and AppShot models - Add tests to verify JSON round-trip and decoding from file formats - Enable previewHTML inclusion in GalleryTemplate JSON encoding - Add gallery preview rendering integration tests for HTML output validation
- Exclude previewHTML from GalleryTemplate encoding and decoding - Move previewHTML property to Gallery for unified HTML preview rendering
- Change GalleryTemplateProvider and GalleryTemplateRepository to use Gallery - Rename related methods to listGalleries and getGallery(templateId:) - Add encoding support for previewHTML in Gallery Codable implementation - Remove outdated documentation comments for clarity
- Replace listGalleryTemplates with listGalleries - Return JSON with pretty-printed, sorted keys wrapped in "data" key
- Replace GalleryTemplateRepositoryTests to return and verify Gallery instances - Adjust mocks and expectations to use galleries linked with templates - Rename test methods to reflect gallery-focused behavior
- Replace mock gallery creation with direct Gallery parameter - Utilize existing template, palette, and appShots content for preview
- Replace inline gallery template setup with makeGallery helper - Add tests for previewHTML content with various backgrounds - Verify previewHTML inclusion in JSON encoding
- Replace mock gallery setup with decoding from gallery-templates.json - Verify multiple galleries and their unique content - Generate and write preview HTML files for all galleries - Refine GalleryComposeTests to check for phone status bar presence
- Rename renderPanel to renderScreen for clarity - Update related variables and comments from panel to screen - Adjust badge positioning and device frame color variables - Improve HTML structure and CSS for gallery preview and device wireframe
- Default to preset device slot if screen template device is missing for hero shots - Improve positioning fallback logic in
- Verify hero shot includes wireframe phone placeholder "9:41" - Replace check for absence of <img> tag with presence of placeholder text
…erer - Add wrapPage to GalleryHTMLRenderer to generate full HTML pages - Replace TemplateHTMLRenderer calls with GalleryHTMLRenderer for previewHTML, apply, and renderFragment - Introduce conversion methods on ScreenshotTemplate for ScreenTemplate, GalleryPalette, and AppShot types - Simplify ScreenDesign.previewHTML by using TemplateContent.apply()
- Replace TemplateContent with AppShot for template application - Simplify ScreenshotTemplate to use ScreenTemplate and GalleryPalette - Remove legacy slots and background enums for streamlined layout - Update all rendering paths to use AppShot for consistent content handling
- Replace TemplateContent with AppShot in template apply and render tests - Verify previewHTML contains template name - Refactor test factories to use screenTemplate and palette instead of legacy slots - Update JSON encoding tests for updated deviceCount behavior
- Refactor deviceCount tests for templates with and without devices - Adjust previewHTML test to check template name instead of generic text - Update screenshot template tableRow test to match default deviceCount of 1
- Replace single device slot with array to allow multi-device layouts - Add backward-compatible decoding for single or multiple devices - Update GalleryHTMLRenderer to render multiple devices side-by-side
- Replace single DeviceSlot with array in ScreenTemplate - Improve AppShot table output and affordances commands - Remove unused makeScreenDesign helper from test helpers - Simplify deviceCount computation in ScreenshotTemplate
- Extract shared CSS building logic into `buildPageStyles` - Use `loadPageWrapperTemplate` to unify template loading - Simplify style injection in HTML by replacing individual style keys with combined `styles` string
- Replace HTML string assembly with template-driven rendering in GalleryHTMLRenderer - Build full context dictionaries for screens, devices, decorations, badges, and trust marks - Externalize HTML structure and styles to template files, enabling easier customization - Update resources to formatted, multiline HTML templates with semantic elements - Maintain backward-compatible rendering methods for text slots (tagline, headline, subheading)
…ilding - Extract reusable context builders for text, badges, decorations, and devices - Replace inline HTML string construction with template rendering - Add new HTML templates for subcomponents and page wrapper - Refactor page style building and preview rendering for flexibility and clarity
- Introduce derived color properties in GalleryPalette for consistent theming - Update GalleryHTMLRenderer to use palette colors instead of inline logic - Move decoration shape display characters and animation CSS into domain models - Simplify device context rendering with palette-based styling and wireframe colors
…contexts - Replace hardcoded colors with CSS custom properties in HTML templates - Remove GalleryPalette from context builders, passing minimal data only - Add data-theme and themeVars for light/dark mode styling in GalleryHTMLRenderer - Simplify decoration, badge, device, and trust mark contexts for cleaner theming - Update wireframe and device templates to use CSS variables and modern styles
- Remove explicit HTML element render functions, relying on templates only - Update device rendering to use context flags for phone frames and wireframes - Simplify headline, tagline, badges, trust marks, and decorations rendering logic - Consolidate test helpers and improve test coverage for layout and palette handling - Inline container styling and data-theme attribute in templates for theme consistency
- Replace CSS string building in Swift with context dictionary - Update HTML template to handle dynamic styles and viewport fill - Share page wrapper context with ThemedPage for consistency
- Enhance block matching to correctly parse nested if tags - Add tests covering various nested if scenarios - Inline and optimize gallery HTML templates for performance - Inject theme variables in preview page and screen templates - Refactor The
- Add thread-safe in-memory cache for loaded templates
…ation - Replace manual template parsing with MustacheTemplate compilation - Load and cache .mustache templates from bundle resources in MustacheLibrary - Update GalleryHTMLRenderer to render using Mustache templates by name - Rename HTML templates from .html to .mustache and adjust markup for Mustache syntax - Simplify context values to use booleans instead of string flags - Update tests to reflect new section/tag syntax and rendering behavior
- Move HTML structure from Swift to 13 external `.html` files - Introduce `HTMLComposer` lightweight template engine with variable and control syntax - Add `HTMLTemplateRepository` protocol for template management and plugin overrides - Preserve existing public API of `GalleryHTMLRenderer`
- Cache rendered previewHTML for AppShotTemplate and Gallery to improve performance - Introduce thread-safe PreviewCache in GalleryHTMLRenderer for reuse and invalidation
- Introduce ThemeDesign JSON for batching AI theme generation - Add ThemeDesignApplier to apply cached designs without AI calls - Use cqi units for consistent sizing across previews and exports - Enhance GalleryHTMLRenderer to merge designs into rendering pipeline - Extend CLI with --design-only and --apply-design options for workflows
- extract all HTML, CSS colors, and animations into 7 precompiled `.mustache` files - move theme detection and text color logic to GalleryPalette - add Decoration optional style fields and displayCharacter computed property - enable immediate theme application without auto-compose step - implement Blitz plugin `design()` for fast design→apply flow
- Introduce `themes design` to generate ThemeDesign JSON via AI - Add `themes apply-design` to apply cached ThemeDesign to templates deterministically without AI - Simplify `themes apply` by removing design-only and apply-design flags and logic - Update tests to cover new subcommands and workflows
- Introduce gallery-templates subcommand with list and get features - Support output formats and HTML preview for gallery templates - Add AggregateGalleryTemplateRepository factory method - Include comprehensive tests for listing and retrieving gallery templates
- Rename test to focus on background gradient presence - Replace template name check with DOCTYPE presence validation
- Introduce `gallery-templates` commands: list and get templates with preview support - Add `themes` commands: design generation, apply cached designs with previews - Update command structure with new source files for templates, gallery templates, and themes
- Update folder structure for better clarity and consistency - Adjust related test file paths to match new folder name - Correct MARK comments to reflect updated naming conventions
- Update source, infrastructure, and test folder paths for clarity - Adjust related documentation to reflect new directory names
📝 WalkthroughWalkthroughRefactors screenshot rendering into a gallery-centric Mustache pipeline, replaces ScreenshotTemplate/ScreenDesign with AppShot/Gallery models, adds ThemeDesign and deterministic application, introduces new CLI/REST gallery & theme endpoints, and adds Mustache resources plus extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as AppShotsController
participant GalleryRepo as GalleryTemplateRepository
participant ScreenRenderer as GalleryHTMLRenderer
participant Composer as HTMLComposer
participant Templates as .mustache Files
rect rgba(100, 150, 255, 0.5)
Note over Client,Templates: Gallery Composition Flow
Client->>API: POST /app-shots/gallery/compose<br/>(templateId, screenshots[])
API->>GalleryRepo: getGallery(templateId)
GalleryRepo-->>API: Gallery with template & palette
loop For each screenshot in input
API->>ScreenRenderer: renderScreen(AppShot,<br/>ScreenLayout, GalleryPalette)
ScreenRenderer->>Composer: render(template: "screen",<br/>context: {...})
Composer->>Templates: screen.mustache
Templates-->>Composer: rendered fragment
Composer-->>ScreenRenderer: fragment
ScreenRenderer-->>API: themed HTML
end
API-->>Client: JSON with composed screens[]
end
rect rgba(100, 200, 100, 0.5)
Note over Client,Templates: Theme Design Workflow (Two Steps)
Client->>API: POST /app-shots/themes/design<br/>(themeId)
API->>GalleryRepo: design(themeId) → ThemeDesign
GalleryRepo-->>API: palette + decorations
API-->>Client: ThemeDesign JSON
Client->>API: POST /app-shots/themes/apply-design<br/>(design, templateId, screenshot)
API->>GalleryRepo: getGallery(templateId)
GalleryRepo-->>API: Gallery with ScreenLayout
API->>ScreenRenderer: renderScreen(AppShot,<br/>screenLayout.withDecorations(design.decorations),<br/>design.palette)
ScreenRenderer->>Composer: render templates with theme context
Composer->>Templates: screen.mustache, theme-vars.mustache,<br/>keyframes.mustache
Templates-->>Composer: styled + decorated HTML
Composer-->>ScreenRenderer: complete themed fragment
ScreenRenderer-->>API: HTML
API-->>Client: themed HTML or PNG export
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/features/app-shots.md (1)
13-472:⚠️ Potential issue | 🟠 MajorFeature doc is missing required section structure.
This update still does not provide all required sections as explicit headings (
CLI Usage,Typical Workflow,API Reference,Extendingare missing).As per coding guidelines: “Update
docs/features/<feature>.mdwith 8 sections: CLI Usage, Typical Workflow, Architecture, Domain Models, File Map, API Reference, Testing, Extending”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots.md` around lines 13 - 472, The feature doc docs/features/app-shots.md is missing the required top-level headings; update the file to include the eight explicit sections: "CLI Usage", "Typical Workflow", "Architecture", "Domain Models", "File Map", "API Reference", "Testing", and "Extending" (you can retain "Quick Start" as a subsection under "CLI Usage"); reorganize existing content so CLI flags and examples go under "CLI Usage", Quick Start and workflows under "Typical Workflow", architecture diagram under "Architecture", all model/type descriptions under "Domain Models", the sources/tests lists under "File Map"/"Testing", and add an "API Reference" and "Extending" section with brief placeholders if needed to satisfy the guideline.docs/features/app-shots-themes.md (1)
28-45:⚠️ Potential issue | 🟡 MinorReorganize this feature doc to the required section contract.
This page still uses custom headings (
User Journey,CLI / Agent,CLI Reference,Plugin Integration Guide) instead of the required feature-doc sections. Please rename/split these so readers can reliably findCLI Usage,Typical Workflow,API Reference, andExtending.As per coding guidelines,
docs/features/<feature>.mdmust be updated with 8 sections: CLI Usage, Typical Workflow, Architecture, Domain Models, File Map, API Reference, Testing, Extending.Also applies to: 64-123, 476-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots-themes.md` around lines 28 - 45, Rework the document to the required 8-section contract: replace the custom headings ("User Journey", "Plugin UI", "CLI / Agent", "CLI Reference", "Plugin Integration Guide") with the mandated sections: CLI Usage, Typical Workflow, Architecture, Domain Models, File Map, API Reference, Testing, and Extending; move the "Capture → Design → Pick Theme → Export" flow and plugin steps (including the POST /themes/design call and the ThemeDesign / ThemeDesignApplier.apply items) into Typical Workflow and API Reference as appropriate; extract implementation details about deterministic re-rendering and ThemeDesign JSON into Domain Models and Architecture; add a File Map that lists related files referenced by the doc and a Testing section describing how to test theme application, and put integration instructions under Extending. Ensure headings exactly match the required section names and split/relocate content from the old headings accordingly.
🟠 Major comments (27)
Sources/ASCCommand/Commands/AppShots/AppShotsExport.swift-47-50 (1)
47-50:⚠️ Potential issue | 🟠 MajorMake preview-container replacement resilient to non-default aspect ratios.
At Line 49, the replacement only matches
aspect-ratio:1320/2868. If the preview HTML uses any other ratio (or slightly different formatting), the 320px container is not replaced, and export can render as a scaled preview instead of full-frame.Proposed fix
- htmlContent = htmlContent - .replacingOccurrences(of: "width:320px;aspect-ratio:1320/2868;container-type:inline-size", with: "width:100%;height:100%;container-type:inline-size") + htmlContent = htmlContent + .replacingOccurrences( + of: #"width:\s*320px\s*;\s*aspect-ratio:[^;]+\s*;\s*container-type:inline-size"#, + with: "width:100%;height:100%;container-type:inline-size", + options: .regularExpression + ) .replacingOccurrences(of: "display:flex;justify-content:center;align-items:center;min-height:100vh;background:`#111`", with: "margin:0;overflow:hidden")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ASCCommand/Commands/AppShots/AppShotsExport.swift` around lines 47 - 50, The current htmlContent replacement is too specific (matches "aspect-ratio:1320/2868") so the width:320px branch in AppShotsExport.swift (the htmlContent.contains("width:320px") check and the replacingOccurrences(of:...) calls) can miss other aspect ratios; update the first replacingOccurrences to use a regular-expression pattern that matches any aspect-ratio value and tolerates optional whitespace/order (e.g. a regex like "width:320px;\\s*aspect-ratio:[^;]+;\\s*container-type:inline-size" or a more permissive pattern) and call replacingOccurrences(with:options:.regularExpression) so any aspect-ratio is replaced with "width:100%;height:100%;container-type:inline-size"; keep the second replacement (display:flex...) but ensure patterns tolerate minor spacing differences as well by switching to regex where appropriate.docs/features/app-shots.md-212-212 (1)
212-212:⚠️ Potential issue | 🟠 MajorDependency flow diagram conflicts with enforced architecture direction.
Line 212 documents
ASCCommand → Domain ← Infrastructure, but the project rule isASCCommand → Infrastructure → Domain.As per coding guidelines: “Architecture must maintain three strict layers with unidirectional dependency flow:
ASCCommand → Infrastructure → Domain”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots.md` at line 212, The documented dependency arrow is inverted; update the diagram text that currently reads "ASCCommand → Domain ← Infrastructure" so it reflects the enforced unidirectional architecture "ASCCommand → Infrastructure → Domain" by replacing the middle and right-hand arrows accordingly (reference symbols: ASCCommand, Infrastructure, Domain).Sources/Domain/Screenshots/Gallery/GalleryPalette.swift-7-33 (1)
7-33:⚠️ Potential issue | 🟠 MajorAdd
AffordanceProvidingsupport to this Domain model.
GalleryPaletteis a Domain model but currently lacksAffordanceProvidingconformance and anaffordancesfield.As per coding guidelines: “All domain models must implement
AffordanceProvidingprotocol with anaffordancesfield containing ready-to-run CLI commands”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/GalleryPalette.swift` around lines 7 - 33, GalleryPalette must conform to AffordanceProviding and include an affordances field: add AffordanceProviding to the type declaration, add a public let affordances: [Affordance] (or the project's affording type) with a default empty value to the primary init (keep existing parameters but add affordances: [Affordance] = []), update the custom init(from:) to decodeIfPresent(.affordances) and fall back to [] and add .affordances to the CodingKeys enum so Codable encoding/decoding continues to work; ensure the new affordances property name and the AffordanceProviding protocol are referenced exactly (GalleryPalette, affordances, AffordanceProviding, init(from:), CodingKeys).Sources/Domain/Screenshots/Gallery/Resources/preview-screen.mustache-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorHard-coded aspect ratio will distort previews for non-portrait templates.
This template always renders
1320/2868. For landscape/square/portrait43 previews, cards will be scaled incorrectly.Suggested fix
-<div style="width:120px;aspect-ratio:1320/2868;border-radius:6px;overflow:hidden;flex-shrink:0;box-shadow:0 1px 4px rgba(0,0,0,0.06),0 4px 12px rgba(0,0,0,0.04)">{{{screen}}}</div> +<div style="width:120px;aspect-ratio:{{aspectRatio}};border-radius:6px;overflow:hidden;flex-shrink:0;box-shadow:0 1px 4px rgba(0,0,0,0.06),0 4px 12px rgba(0,0,0,0.04)">{{{screen}}}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/Resources/preview-screen.mustache` at line 1, The preview template hard-codes aspect-ratio:1320/2868 which distorts non-portrait templates; update preview-screen.mustache to accept a dynamic aspect ratio (e.g., a template variable like {{aspectRatio}} or computed width/height) and use that variable in place of the fixed 1320/2868 so landscape/square/portrait43 previews scale correctly; ensure the producer code that renders this template supplies the correct aspect ratio (derived from the screen's width/height or template type) and consider using CSS fallbacks (e.g., removing the fixed aspect-ratio or adding object-fit rules for the {{{screen}}} content) so rendering remains correct across all template orientations.Sources/Domain/Screenshots/Gallery/GalleryPalette.swift-40-44 (1)
40-44:⚠️ Potential issue | 🟠 Major
isLightcurrently misclassifies palettes and can force incorrect dark-mode behavior.At Line 41, returning
falsewhenevertextColorexists couples text override to theme detection. Also, the substring heuristic at Line 42-43 is too loose for many valid CSS colors/gradients.Suggested fix
extension GalleryPalette { + private var firstHexColor: String? { + let pattern = #"#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})"# + guard let regex = try? NSRegularExpression(pattern: pattern) else { return nil } + let range = NSRange(background.startIndex..<background.endIndex, in: background) + guard let match = regex.firstMatch(in: background, range: range), + let swiftRange = Range(match.range, in: background) else { return nil } + return String(background[swiftRange]) + } + + private func luminance(for hex: String) -> Double? { + var value = hex + if value.count == 4 { // `#RGB` -> `#RRGGBB` + value = "#" + value.dropFirst().map { "\($0)\($0)" }.joined() + } + guard value.count == 7 else { return nil } + let r = Double(Int(value.dropFirst(1).prefix(2), radix: 16) ?? 0) / 255.0 + let g = Double(Int(value.dropFirst(3).prefix(2), radix: 16) ?? 0) / 255.0 + let b = Double(Int(value.dropFirst(5).prefix(2), radix: 16) ?? 0) / 255.0 + return 0.2126 * r + 0.7152 * g + 0.0722 * b + } + /// Whether this palette has a light background (heuristic based on hex values). public var isLight: Bool { - guard textColor == nil else { return false } - let lightHex = ["#f", "#F", "#e", "#E", "#d", "#D", "#c", "#C", "#b", "#B", "#a8", "#A8", "#9"] - return lightHex.contains(where: { background.contains($0) }) + guard let hex = firstHexColor, let luma = luminance(for: hex) else { return false } + return luma >= 0.6 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/GalleryPalette.swift` around lines 40 - 44, The isLight computed property should not early-return based on textColor and should use a precise color detection and luminance check instead of loose substring matching: in GalleryPalette.isLight remove the guard that returns false when textColor != nil, detect an actual hex color at the start of background (or extract the first color token from a gradient) using a case-insensitive regex that matches valid 3/4/6/8-digit hex forms, expand shorthand hex to full RGB, convert sRGB -> linear and compute relative luminance from the RGB channels, then return true when that luminance exceeds a chosen threshold (e.g. 0.5) to indicate a light background; fallback to false if no parseable color is found.Sources/Domain/Screenshots/Gallery/Resources/preview-page.mustache-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorHarden unescaped HTML/CSS injection points.
{{{themeVars}}}and{{{screenDivs}}}are raw inserts. If any upstream value is AI/user/plugin-controlled, preview HTML is vulnerable to injection. Add CSP defense-in-depth and ensure upstream sanitization/allowlisting.🔒 Suggested hardening (defense in depth)
-<!DOCTYPE html><html><head><meta charset="utf-8"> +<!DOCTYPE html><html><head><meta charset="utf-8"> +<meta http-equiv="Content-Security-Policy" content="default-src 'none'; img-src data: blob: https: file:; style-src 'unsafe-inline'; font-src data: https:">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/Resources/preview-page.mustache` around lines 6 - 7, The template injects raw HTML via {{{themeVars}}} and {{{screenDivs}}}, enabling HTML/CSS injection if upstream content is untrusted; fix by ensuring values passed into themeVars and screenDivs are sanitized/allowlisted before rendering (escape or strip scripts/styles and disallow dangerous attributes), and add defense-in-depth by emitting a strict Content-Security-Policy header/meta (e.g., disallow inline scripts/styles, restrict script/src/style-src to trusted origins) from the preview rendering path so even if unsanitized markup appears it cannot execute.Sources/Domain/Screenshots/ThemeDesign.swift-12-22 (1)
12-22:⚠️ Potential issue | 🟠 Major
ThemeDesignis missing the required domain affordances contract.This new Domain model does not conform to
AffordanceProvidingand does not expose anaffordancesfield, which breaks the repo’s domain-model convention for agent navigation output.As per coding guidelines, "All domain models must implement
AffordanceProvidingprotocol with anaffordancesfield containing ready-to-run CLI commands."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/ThemeDesign.swift` around lines 12 - 22, ThemeDesign currently lacks the required AffordanceProviding conformance and an affordances property; update the struct ThemeDesign to adopt the AffordanceProviding protocol and add a public let affordances: [Affordance] property (or computed var if preferred) populated with the domain-specific CLI commands for this model, ensure init(palette:decorations:) is updated to accept or synthesize affordances (or provide a secondary init) and implement any required methods from AffordanceProviding so ThemeDesign (alongside GalleryPalette and Decoration types) conforms to the repo convention.Tests/ASCCommandTests/Commands/AppShots/AppShotsThemesTests.swift-198-217 (1)
198-217:⚠️ Potential issue | 🟠 MajorUse a unique temp filename and guaranteed cleanup to avoid flaky parallel tests.
Using a fixed filename at Line 198 can collide across runs/suites, and cleanup at the end is skipped if the test exits early.
💡 Proposed reliability fix
- let designPath = NSTemporaryDirectory() + "test-design.json" - let data = try JSONEncoder().encode(design) - try data.write(to: URL(fileURLWithPath: designPath)) + let designPath = (NSTemporaryDirectory() as NSString) + .appendingPathComponent("test-design-\(UUID().uuidString).json") + let designURL = URL(fileURLWithPath: designPath) + let data = try JSONEncoder().encode(design) + try data.write(to: designURL) + defer { try? FileManager.default.removeItem(at: designURL) } @@ - try? FileManager.default.removeItem(atPath: designPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ASCCommandTests/Commands/AppShots/AppShotsThemesTests.swift` around lines 198 - 217, The test uses a fixed designPath which can collide in parallel runs and only deletes it at the end, so change the test that constructs designPath (used when calling AppShotsThemesApplyDesign.parse and writing the encoded design) to create a unique temporary file (e.g., by appending a UUID or using a temp file API) and ensure deterministic cleanup by removing the file in a defer block or a guaranteed teardown path; keep the rest of the setup (MockTemplateRepository.getTemplate stub and cmd.execute call) unchanged but reference the new unique designPath when invoking parse/execute.Sources/ASCCommand/Commands/AppShots/AppShotsTemplates.swift-174-178 (1)
174-178:⚠️ Potential issue | 🟠 MajorPreserve the provided screenshot path for HTML preview.
Converting to
lastPathComponentdrops directory context and can break preview rendering for valid user paths.Proposed fix
- let shotForHTML = AppShot(screenshot: URL(fileURLWithPath: screenshot).lastPathComponent, type: .feature) + let shotForHTML = AppShot(screenshot: screenshot, type: .feature)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ASCCommand/Commands/AppShots/AppShotsTemplates.swift` around lines 174 - 178, The code currently strips directory context by passing URL(fileURLWithPath: screenshot).lastPathComponent into AppShot; change it to preserve the full provided screenshot path when constructing the AppShot used for HTML preview (i.e., pass the original screenshot path or its full file URL string instead of lastPathComponent) so AppShot creation in this block (shotForHTML / AppShot(...)) retains directory context and prevents broken previews.Sources/Domain/Screenshots/ThemedPage.swift-5-27 (1)
5-27:⚠️ Potential issue | 🟠 MajorAlign
ThemedPagewith the Domain model contract.This new Domain model is missing required conformance/fields from repository standards:
Codable, plusAffordanceProvidingwith anaffordancesfield.As per coding guidelines:
Sources/Domain/**/*.swift: “All Domain layer models arepublic struct+Sendable+Equatable+Codable” and “All domain models must implementAffordanceProvidingprotocol with anaffordancesfield containing ready-to-run CLI commands.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/ThemedPage.swift` around lines 5 - 27, The ThemedPage struct currently lacks required Domain contract conformance; update ThemedPage to also conform to Codable and AffordanceProviding, add a public let affordances: [Affordance] (or appropriate type from your domain) stored property with a sensible default (e.g., empty array) and include it in the initializer, and ensure conformance signatures include Codable and AffordanceProviding alongside Sendable and Equatable; keep the existing html computed property and initializer logic but add affordances to initializer parameters (with default) so serialization and affordance provisioning work as expected.Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swift-14-16 (1)
14-16:⚠️ Potential issue | 🟠 MajorRestore global renderer state after mutation.
This test mutates a global static and never resets it, which can contaminate later tests.
Proposed fix
- if let frameData = try? Data(contentsOf: framePath) { + let previousPhoneFrameDataURL = GalleryHTMLRenderer.phoneFrameDataURL + defer { GalleryHTMLRenderer.phoneFrameDataURL = previousPhoneFrameDataURL } + if let frameData = try? Data(contentsOf: framePath) { GalleryHTMLRenderer.phoneFrameDataURL = "data:image/png;base64," + frameData.base64EncodedString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swift` around lines 14 - 16, The test mutates the global static GalleryHTMLRenderer.phoneFrameDataURL and does not restore it, risking test contamination; modify the test in GalleryPreviewOutputTests.swift to save the original value of GalleryHTMLRenderer.phoneFrameDataURL before assigning the base64 data and restore it after the test (use defer or a teardown) so the global renderer state is reset regardless of test outcome.Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swift-34-45 (1)
34-45:⚠️ Potential issue | 🟠 MajorAvoid writing preview artifacts into repository state during tests.
Writing under
.asc/...in the project root makes the test non-hermetic and can pollute CI/local worktrees.Proposed fix
- let outputDir = projectRoot.appendingPathComponent(".asc/app-shots/gallery-previews") + let outputDir = FileManager.default.temporaryDirectory + .appendingPathComponent("gallery-previews-\(UUID().uuidString)", isDirectory: true) + defer { try? FileManager.default.removeItem(at: outputDir) } try FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swift` around lines 34 - 45, The test currently writes preview files into the repo via the outputDir variable (projectRoot.appendingPathComponent(".asc/app-shots/gallery-previews")), which can pollute CI/worktrees; change the test to create and use a temporary directory instead (e.g., FileManager.default.temporaryDirectory with a unique suffix or UUID) for outputDir, keep the same loop over galleries and use gallery.previewHTML as before, and ensure the temp directory is created with FileManager.default.createDirectory(...); also remove or update the printed path so it doesn't reference repo-relative locations. This modifies how outputDir is constructed and prevents writing into the repository state.Sources/ASCCommand/Commands/AppShots/AppShotsTemplates.swift-168-171 (1)
168-171:⚠️ Potential issue | 🟠 MajorFail fast when image preview is requested without a renderer.
Current control flow falls back to non-image output if
preview == .imageandrendereris nil, which is a wrong mode/result mismatch.Proposed fix
- if preview == .image, let renderer { - let html = template.apply(shot: shot, fillViewport: true) - return try await renderToImage(html: html, renderer: renderer) - } + if preview == .image { + guard let renderer else { + throw ValidationError("Preview format 'image' requires an HTML renderer.") + } + let html = template.apply(shot: shot, fillViewport: true) + return try await renderToImage(html: html, renderer: renderer) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ASCCommand/Commands/AppShots/AppShotsTemplates.swift` around lines 168 - 171, The code currently falls through to non-image output when preview == .image but renderer is nil; change this to fail fast: before the existing if-block that reads `if preview == .image, let renderer { ... }` add an explicit guard that checks `if preview == .image && renderer == nil` and then return/throw a clear error (e.g., throw a new PreviewRendererMissingError or return a failure result) so the function does not silently produce the wrong output; ensure the error type is defined and used where this function (the AppShots template rendering path that calls `template.apply(shot:fillViewport:)` and `renderToImage(html:renderer:)`) is allowed to propagate the failure.Sources/Infrastructure/ScreenshotPlans/AggregateGalleryTemplateRepository.swift-25-27 (1)
25-27:⚠️ Potential issue | 🟠 MajorAvoid full fan-out in
getGallerywhen a match can be found early.Current implementation always loads all providers via
listGalleries(). This adds unnecessary latency/work for single-template lookup.♻️ Suggested optimization
public func getGallery(templateId: String) async throws -> Gallery? { - let all = try await listGalleries() - return all.first { $0.template?.id == templateId } + for provider in providers { + let galleries = try await provider.galleries() + if let match = galleries.first(where: { $0.template?.id == templateId }) { + return match + } + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Infrastructure/ScreenshotPlans/AggregateGalleryTemplateRepository.swift` around lines 25 - 27, getGallery currently calls listGalleries() which fan-outs to every provider; change it to short-circuit lookup by iterating providers and asking each provider for the specific template (instead of fetching all galleries). In AggregateGalleryTemplateRepository.getGallery(templateId:) call a provider-level lookup (e.g., provider.getGallery(templateId:) or implement provider.fetchGallery(templateId:)) for each provider and return immediately when a non-nil Gallery is found; if you need concurrency, use a TaskGroup that cancels remaining tasks when the first result is returned so work is not wasted.Sources/Infrastructure/ScreenshotPlans/AggregateGalleryTemplateRepository.swift-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorMake provider registration idempotent on the shared repository.
register(provider:)blindly appends, so repeated bootstrap paths can produce duplicated providers and duplicated gallery rows.🐛 Possible fix
public final actor AggregateGalleryTemplateRepository: GalleryTemplateRepository { public static let shared = AggregateGalleryTemplateRepository() private var providers: [any GalleryTemplateProvider] = [] + private var providerKeys = Set<String>() @@ public func register(provider: any GalleryTemplateProvider) { + let key = String(reflecting: type(of: provider)) + guard providerKeys.insert(key).inserted else { return } providers.append(provider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Infrastructure/ScreenshotPlans/AggregateGalleryTemplateRepository.swift` around lines 12 - 14, The register(provider:) method on AggregateGalleryTemplateRepository currently appends providers blindly causing duplicates; change it to be idempotent by checking the providers collection before appending: in the register(provider: any GalleryTemplateProvider) implementation, compare the incoming provider against existing entries in providers (by identity or a unique provider identifier exposed by GalleryTemplateProvider) and only append if not already present, ensuring repeated bootstraps do not add duplicate providers or duplicated gallery rows.Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift-9-23 (1)
9-23:⚠️ Potential issue | 🟠 MajorThis test is non-hermetic and has filesystem side effects
The test depends on repo-relative files and writes artifacts under
.asc/..., making it environment-sensitive and CI-fragile. Prefer fixture resources in the test bundle and write outputs to a temp directory only when explicitly enabled (e.g., env flag).Also applies to: 71-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift` around lines 9 - 23, The test TemplatePreviewPageTests should stop depending on repo-relative paths and writing into the repo; update the setup in TemplatePreviewPageTests.swift to load the iPhone frame and templates.json from bundled test fixtures (use the test bundle/Bundle.module or Bundle(for: ...)) instead of computing root via testFile/deletingLastPathComponent and use a temporary directory (FileManager.default.temporaryDirectory or NSTemporaryDirectory()) for any output artifacts, only writing there when an explicit env flag (e.g., WRITE_TEST_ARTIFACTS) is set; replace usages of framePath/jsonPath and direct Data(contentsOf:) with bundle resource lookup and guard the write paths so GalleryHTMLRenderer.phoneFrameDataURL and any output writes respect the temp directory + env flag.Sources/Domain/Screenshots/Gallery/HTMLComposer.swift-32-34 (1)
32-34:⚠️ Potential issue | 🟠 MajorTemplate-loading/render failures are silently swallowed
Returning
""or an emptyMustacheLibraryon failure hides resource/template regressions and makes failures hard to diagnose. At minimum, emit diagnostics/assertions; ideally propagate a throwing error path.🧭 Minimal diagnostic hardening
public static func render(template name: String, with context: Any) -> String { - library.render(context, withTemplate: name) ?? "" + guard let output = library.render(context, withTemplate: name) else { + assertionFailure("Failed to render template '\(name)'") + return "" + } + return output }Also applies to: 44-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/HTMLComposer.swift` around lines 32 - 34, The render(template:with:) method (and the other render overloads using library.render / MustacheLibrary) currently swallows failures by returning an empty string; change these paths to surface errors by either making render(template:with:) throwing and propagating the underlying error from library.render (or converting a nil result into a descriptive thrown error), or at minimum log/assert the failure including the template name and library error information before returning; update the function signature(s) (render(template:with:), any other render overloads that call library.render) and callers accordingly so template-loading/render failures are not silently ignored and include diagnostic details.Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift-15-17 (1)
15-17:⚠️ Potential issue | 🟠 MajorGlobal renderer state is mutated without restoration
GalleryHTMLRenderer.phoneFrameDataURLis set and never reset, so test order can change outcomes across the suite. Save previous value and restore viadefer.♻️ Suggested isolation fix
if let frameData = try? Data(contentsOf: framePath) { + let previous = GalleryHTMLRenderer.phoneFrameDataURL + defer { GalleryHTMLRenderer.phoneFrameDataURL = previous } GalleryHTMLRenderer.phoneFrameDataURL = "data:image/png;base64," + frameData.base64EncodedString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift` around lines 15 - 17, The test mutates global state by setting GalleryHTMLRenderer.phoneFrameDataURL without restoring it; capture the current value into a local (e.g., let previous = GalleryHTMLRenderer.phoneFrameDataURL) before assigning the test value, then restore it in a defer block (defer { GalleryHTMLRenderer.phoneFrameDataURL = previous }) so the original global is always returned after the test completes; apply this change around the code that reads framePath and assigns the base64 string.Sources/ASCCommand/Commands/Web/Controllers/AppShotsController.swift-54-64 (1)
54-64:⚠️ Potential issue | 🟠 MajorMake screenshot staging fail-fast and self-cleaning.
These loops silently skip invalid base64 items and never delete the temp PNGs they do write. That means bad payloads can produce partial/misaligned output, and repeated requests will leak disk space on the server. Decode the whole batch eagerly, return
400on the first invalid item, anddefercleanup of every staged file after rendering.Also applies to: 117-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ASCCommand/Commands/Web/Controllers/AppShotsController.swift` around lines 54 - 64, The current loop over screenshotsB64 silently skips invalid base64 and never removes temporary files; change it to eagerly validate and decode every item (iterate screenshotsB64, attempt Data(base64Encoded:) for each and if any decode fails immediately return a 400 response), only write files if all decodes succeed, collect file paths into paths and dataURLs as before, and ensure you schedule cleanup with defer to remove every staged temp PNG after rendering (use FileManager.removeItem on each path). Apply the same eager validation, fail-fast 400 behavior, and deferred cleanup to the second staging block referenced around lines 117-127 as well.Sources/ASCCommand/Commands/AppShots/AppShotsThemes.swift-103-109 (1)
103-109:⚠️ Potential issue | 🟠 MajorRoute
designoutput throughOutputFormatter.This hardcodes pretty JSON and bypasses the standard formatter, so
--output/--prettyare ignored and the new command never injects CLI affordances into its response.As per coding guidelines, "All CLI commands must include an
affordancesfield in responses with ready-to-run CLI commands for agent navigation" and "UseOutputFormatter.formatAgentItems()to merge affordances into JSON output".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ASCCommand/Commands/AppShots/AppShotsThemes.swift` around lines 103 - 109, The execute(themeRepo: any ThemeRepository) method currently encodes the design with a hardcoded JSONEncoder; replace that with the CLI OutputFormatter so --output/--pretty flags are respected and CLI affordances are injected: call ThemeRepository.design(id:), build the affordances object required by our CLI, then pass the design and affordances into OutputFormatter.formatAgentItems(...) (or the equivalent OutputFormatter API used elsewhere) and return its result string instead of manually encoding with JSONEncoder; update references in this function (execute and the design result handling) to use OutputFormatter.formatAgentItems and ensure the returned JSON includes the affordances field.Sources/Domain/Screenshots/AppShotTemplate.swift-74-78 (1)
74-78:⚠️ Potential issue | 🟠 MajorUse full-value equality here.
supportedSizesanddescriptionare omitted, so two distinct templates can compare equal. That will break any diffing, caching, or tests that rely onAppShotTemplatebeing a real value type.♻️ Proposed fix
-extension AppShotTemplate: Equatable { - public static func == (lhs: AppShotTemplate, rhs: AppShotTemplate) -> Bool { - lhs.id == rhs.id && lhs.name == rhs.name && lhs.category == rhs.category - && lhs.screenLayout == rhs.screenLayout && lhs.palette == rhs.palette - } -} +extension AppShotTemplate: Equatable {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/AppShotTemplate.swift` around lines 74 - 78, The custom Equatable implementation for AppShotTemplate omits supportedSizes and description, causing different instances to compare equal; update the == operator in the AppShotTemplate extension to compare all stored properties (e.g., include lhs.supportedSizes == rhs.supportedSizes and lhs.description == rhs.description) so equality is full-value, or remove the custom implementation so the compiler can synthesize Equatable if all properties are Equatable.Sources/Domain/Screenshots/Gallery/Gallery.swift-125-131 (1)
125-131:⚠️ Potential issue | 🟠 Major
previewHTMLwill go stale after the first render.The cache key is only
gallery-\(id), buttemplate,palette, and everyAppShot’s content can still change afterward. Once this property is read once, later edits will keep returning the old preview.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/Gallery.swift` around lines 125 - 131, The previewHTML property currently uses a static cache key ("gallery-\(id)") so it becomes stale; change the key passed to GalleryHTMLRenderer.cachedPreview to include the gallery's mutable state (for example include template, palette and each AppShot's content or a combined hash/timestamp) so that GalleryHTMLRenderer.cachedPreview("gallery-\(id)-\(stateHash)") will invalidate when template, palette, or any AppShot changes; update the key construction where previewHTML calls GalleryHTMLRenderer.cachedPreview and keep calling GalleryHTMLRenderer.renderPreviewPage(self) for generation.Sources/Domain/Screenshots/Gallery/Gallery.swift-16-23 (1)
16-23:⚠️ Potential issue | 🟠 Major
Galleryhas the same mutable@unchecked Sendableproblem.This is another public reference type with mutable state inside Domain, so cross-task safety now depends on caller discipline instead of the type system. That departs from the repository’s Domain-model contract.
As per coding guidelines,
All Domain layer models are public struct + Sendable + Equatable + Codable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/Gallery.swift` around lines 16 - 23, Gallery is a public reference type with mutable state and uses `@unchecked` Sendable; change it to a value type that satisfies the Domain contract: convert the public final class Gallery to a public struct Gallery, remove `@unchecked` Sendable, and add Sendable, Equatable and Codable conformances; keep id and appName as immutable lets, keep appShots/template/palette as stored properties (use private(set) where mutability should be restricted), add/update an initializer that sets those properties, and ensure referenced types (AppShot, GalleryTemplate, GalleryPalette) conform to Sendable/Equatable/Codable if necessary and update any call sites that relied on reference semantics (e.g., identity or inout expectations).Sources/Domain/Screenshots/Gallery/AppShot.swift-14-24 (1)
14-24:⚠️ Potential issue | 🟠 Major
AppShotbreaks the Domain value-model contract.Making this a mutable reference type with
@unchecked Sendablesidesteps the thread-safety guarantee the Domain layer is supposed to provide. It also leaves the model without parent context, so its affordances cannot be reliably emitted as ready-to-run commands.As per coding guidelines,
All Domain layer models are public struct + Sendable + Equatable + CodableandEvery Domain model must carry its parent ID (e.g. AppStoreVersion.appId, AppScreenshot.setId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/AppShot.swift` around lines 14 - 24, Replace the mutable reference class AppShot with an immutable value type: change public final class AppShot: `@unchecked` Sendable, Identifiable to public struct AppShot: Sendable, Identifiable, Equatable, Codable and make all fields immutable (use let for id, screenshots, type, tagline, headline, body, badges, trustMarks) so the model is thread-safe; remove `@unchecked` Sendable and ensure synthesized Equatable/Codable work. Add an explicit parent identifier property (e.g., let appId: String or let setId: String) to carry the parent context required by Domain affordances, and update any initializer(s) and uses of AppShot to pass that parent ID. Ensure ScreenType remains compatible with Codable/Sendable or adjust accordingly.Sources/Domain/Screenshots/Gallery/ScreenLayout.swift-223-228 (1)
223-228:⚠️ Potential issue | 🟠 MajorFail fast on malformed
DecorationShapeobjects.If the object form decodes but does not contain
label, this silently becomes.gem. That hides badThemeDesignJSON and renders the wrong decoration instead of surfacing the schema error.Suggested fix
let dict = try container.decode([String: String].self) - if let text = dict["label"] { - self = .label(text) - } else { - self = .gem - } + guard let text = dict["label"] else { + throw DecodingError.dataCorruptedError( + in: container, + debugDescription: "Expected a decoration shape string or an object with a `label` field." + ) + } + self = .label(text) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/ScreenLayout.swift` around lines 223 - 228, The decoding currently treats a decoded dictionary without a "label" key as .gem, hiding malformed ThemeDesign JSON; update DecorationShape's Decodable init (the init(from:) that decodes [String: String]) to validate the dict and throw a DecodingError (e.g., .dataCorrupted or .keyNotFound with a clear context) when "label" is missing instead of defaulting to .gem, so invalid input surfaces as a decode failure; include the key name "label" and the decoded dictionary in the error context to aid debugging.Sources/Domain/Screenshots/Gallery/AppShot.swift-64-100 (1)
64-100:⚠️ Potential issue | 🟠 MajorThis
Codableimplementation does not round-trip cleanly.
idis never encoded or decoded, so every deserializedAppShotgets a fresh UUID. Also, ifscreenshotsis empty, Line 93 serializesscreenshotas"", which turns “no screenshots” into bogus data.Suggested fix
extension AppShot: Codable { private enum CodingKeys: String, CodingKey { - case screenshot, screenshots, type, tagline, headline, body, badges, trustMarks + case id, screenshot, screenshots, type, tagline, headline, body, badges, trustMarks } public convenience init(from decoder: any Decoder) throws { let c = try decoder.container(keyedBy: CodingKeys.self) + let id = try c.decodeIfPresent(String.self, forKey: .id) ?? UUID().uuidString // Support both "screenshot" (single) and "screenshots" (array) let shots: [String] if let arr = try? c.decode([String].self, forKey: .screenshots) { shots = arr } else if let single = try? c.decode(String.self, forKey: .screenshot) { shots = [single] } else { shots = [] } let type = try c.decodeIfPresent(ScreenType.self, forKey: .type) ?? .feature - self.init(screenshots: shots, type: type) + self.init(id: id, screenshots: shots, type: type) self.tagline = try c.decodeIfPresent(String.self, forKey: .tagline) self.headline = try c.decodeIfPresent(String.self, forKey: .headline) self.body = try c.decodeIfPresent(String.self, forKey: .body) self.badges = try c.decodeIfPresent([String].self, forKey: .badges) ?? [] self.trustMarks = try c.decodeIfPresent([String].self, forKey: .trustMarks) } public func encode(to encoder: any Encoder) throws { var c = encoder.container(keyedBy: CodingKeys.self) + try c.encode(id, forKey: .id) if screenshots.count > 1 { try c.encode(screenshots, forKey: .screenshots) - } else { - try c.encode(screenshot, forKey: .screenshot) + } else if let first = screenshots.first { + try c.encode(first, forKey: .screenshot) + } else { + try c.encode([], forKey: .screenshots) } try c.encode(type, forKey: .type) try c.encodeIfPresent(tagline, forKey: .tagline) try c.encodeIfPresent(headline, forKey: .headline) try c.encodeIfPresent(body, forKey: .body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/AppShot.swift` around lines 64 - 100, The Codable implementation for AppShot should preserve the id and avoid emitting a bogus empty "screenshot" string: add "id" to CodingKeys and decode/encode the id in init(from:) and encode(to:), so deserialized AppShot keeps its original UUID; update init(from:) to treat an empty decoded single screenshot string as no screenshots (i.e., if decoding .screenshot yields "" then set shots = []), and update encode(to:) to only emit .screenshot when screenshots.count == 1 and the single value is non-empty (omit both .screenshot and .screenshots when screenshots.isEmpty) so no empty-string placeholder is written.Sources/Domain/Screenshots/Gallery/Gallery.swift-145-168 (1)
145-168:⚠️ Potential issue | 🟠 Major
Gallerypersistence drops identity and stores a derived cache.
idis not serialized, so a decoded gallery becomes a different entity. At the same time,previewHTMLis encoded even though it is derived and ignored on decode, which makes the payload stale and non-round-trippable.Suggested fix
extension Gallery: Codable { private enum CodingKeys: String, CodingKey { - case appName, appShots, template, palette, previewHTML + case id, appName, appShots, template, palette } public convenience init(from decoder: any Decoder) throws { let c = try decoder.container(keyedBy: CodingKeys.self) + let id = try c.decodeIfPresent(String.self, forKey: .id) ?? UUID().uuidString let appName = try c.decode(String.self, forKey: .appName) // Decode appShots directly — they carry their own screenshot/type/content let shots = try c.decode([AppShot].self, forKey: .appShots) - self.init(appName: appName, screenshots: []) + self.init(id: id, appName: appName, screenshots: []) self.appShots = shots self.template = try c.decodeIfPresent(GalleryTemplate.self, forKey: .template) self.palette = try c.decodeIfPresent(GalleryPalette.self, forKey: .palette) } public func encode(to encoder: any Encoder) throws { var c = encoder.container(keyedBy: CodingKeys.self) + try c.encode(id, forKey: .id) try c.encode(appName, forKey: .appName) try c.encode(appShots, forKey: .appShots) try c.encodeIfPresent(template, forKey: .template) try c.encodeIfPresent(palette, forKey: .palette) - try c.encode(previewHTML, forKey: .previewHTML) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Screenshots/Gallery/Gallery.swift` around lines 145 - 168, The Gallery Codable implementation currently omits the primary identity and encodes a derived field; update the CodingKeys to include id, decode id in init(from:) and encode id in encode(to:) so decoded Gallery preserves identity (referencing Gallery, CodingKeys, init(from:), encode(to:)), and stop encoding previewHTML (remove previewHTML from encode or mark it as transient) so derived previewHTML is not persisted and the payload is round-trippable.
🟡 Minor comments (4)
docs/features/app-shots.md-32-32 (1)
32-32:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
Several changed fences are missing explicit languages and will keep markdownlint warnings active.
Suggested fixes
-``` +```bash asc app-shots templates list ... -``` +``` -``` +```text ASCCommand Domain Infrastructure ... -``` +``` -``` +```text 1cqi = 1% of the container's inline size -``` +```Also applies to: 165-165, 221-221, 236-236, 258-258, 424-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots.md` at line 32, The markdown file docs/features/app-shots.md contains several fenced code blocks missing language identifiers (MD040); update each affected triple-backtick fence to include an appropriate language tag (e.g., use ```bash for the command block containing "asc app-shots templates list", and ```text for the ASCII table block starting with "ASCCommand Domain Infrastructure" and for the explanatory line "1cqi = 1% of the container's inline size") so all code fences have explicit languages and the markdownlint warnings are resolved.docs/features/app-shots-themes.md-38-40 (1)
38-40:⚠️ Potential issue | 🟡 MinorUse one REST route prefix consistently.
This example calls
POST /themes/design, but later in the same doc the endpoints are documented under/app-shots/themes/.... One of these is stale, and the current mismatch makes the copy-paste flow unreliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots-themes.md` around lines 38 - 40, The doc uses two different route prefixes for the same action: `POST /themes/design` vs the `/app-shots/themes/...` family; pick one prefix and update the example to match the rest of the document (e.g., change `POST /themes/design {themeId}` to `POST /app-shots/themes/design {themeId}`) and ensure any references to the deterministic re-render call (`ThemeDesignApplier.apply(design, shot, layout)`) remain consistent with the chosen endpoint naming.docs/features/app-shots-themes.md-347-350 (1)
347-350:⚠️ Potential issue | 🟡 Minor
buildContext()is described as returning HTML, not a prompt.The method description says
buildContext()“returns full HTML”, but the rest of the page describes it as the prompt builder for the AI provider. That wording will mislead plugin implementers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/app-shots-themes.md` around lines 347 - 350, Update the docs entry for buildContext() to correctly describe it as producing the theme prompt for the AI provider rather than returning full HTML: change the Returns/Description text so buildContext() is described as returning a String prompt (the theme prompt used by the AI provider) and ensure it contrasts with buildDesignContext() which returns a ThemeDesign JSON prompt; reference the buildContext() symbol in the edit so implementers understand it produces the prompt content rather than rendered HTML.CHANGELOG.md-10-30 (1)
10-30:⚠️ Potential issue | 🟡 MinorAdd the missing
Fixedsubsection under[Unreleased].This entry only has
AddedandChanged, but the repository changelog contract requiresAdded,Changed, andFixedunder[Unreleased].As per coding guidelines,
CHANGELOG.mdmust add changelog entries under[Unreleased]using Keep a Changelog format with categories: Added, Changed, Fixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 10 - 30, The [Unreleased] changelog block is missing the required "Fixed" subsection; update CHANGELOG.md by adding a "### Fixed" header under the [Unreleased] section (beside the existing "Added" and "Changed" headings) and include at least one bullet (can be a placeholder like "- <describe fix here>" or a real fix) so the file conforms to the Keep a Changelog contract; ensure the new "### Fixed" sits alongside the existing entries (e.g., after "Changed") and follows the same formatting style as the other subsections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0c90b74-8d6b-4852-b40e-1a5007832d9c
📒 Files selected for processing (77)
CHANGELOG.mdPackage.resolvedPackage.swiftSources/ASCCommand/ClientProvider.swiftSources/ASCCommand/Commands/AppShots/AppShotsCommand.swiftSources/ASCCommand/Commands/AppShots/AppShotsExport.swiftSources/ASCCommand/Commands/AppShots/AppShotsGalleryTemplates.swiftSources/ASCCommand/Commands/AppShots/AppShotsTemplates.swiftSources/ASCCommand/Commands/AppShots/AppShotsThemes.swiftSources/ASCCommand/Commands/Web/Controllers/AppShotsController.swiftSources/ASCCommand/Commands/Web/RESTRoutes.swiftSources/Domain/ScreenshotPlans/LayoutMode.swiftSources/Domain/ScreenshotPlans/ScreenDesign.swiftSources/Domain/ScreenshotPlans/ScreenshotTemplate.swiftSources/Domain/ScreenshotPlans/TemplateContent.swiftSources/Domain/ScreenshotPlans/TemplateHTMLRenderer.swiftSources/Domain/ScreenshotPlans/ThemedPage.swiftSources/Domain/Screenshots/AppShotTemplate.swiftSources/Domain/Screenshots/AppShotsConfig.swiftSources/Domain/Screenshots/AppShotsConfigStorage.swiftSources/Domain/Screenshots/Gallery/AppShot.swiftSources/Domain/Screenshots/Gallery/Gallery.swiftSources/Domain/Screenshots/Gallery/GalleryHTMLRenderer.swiftSources/Domain/Screenshots/Gallery/GalleryPalette.swiftSources/Domain/Screenshots/Gallery/GalleryTemplate.swiftSources/Domain/Screenshots/Gallery/GalleryTemplateRepository.swiftSources/Domain/Screenshots/Gallery/HTMLComposer.swiftSources/Domain/Screenshots/Gallery/Resources/keyframes.mustacheSources/Domain/Screenshots/Gallery/Resources/page-wrapper.mustacheSources/Domain/Screenshots/Gallery/Resources/preview-page.mustacheSources/Domain/Screenshots/Gallery/Resources/preview-screen.mustacheSources/Domain/Screenshots/Gallery/Resources/screen.mustacheSources/Domain/Screenshots/Gallery/Resources/theme-vars.mustacheSources/Domain/Screenshots/Gallery/Resources/wireframe.mustacheSources/Domain/Screenshots/Gallery/ScreenLayout.swiftSources/Domain/Screenshots/HTMLRenderer.swiftSources/Domain/Screenshots/ScreenTheme.swiftSources/Domain/Screenshots/ScreenshotPlans+RESTRoutes.swiftSources/Domain/Screenshots/TemplateRepository.swiftSources/Domain/Screenshots/ThemeDesign.swiftSources/Domain/Screenshots/ThemeDesignApplier.swiftSources/Domain/Screenshots/ThemedPage.swiftSources/Infrastructure/ScreenshotPlans/AggregateGalleryTemplateRepository.swiftSources/Infrastructure/ScreenshotPlans/AggregateTemplateRepository.swiftSources/Infrastructure/ScreenshotPlans/AggregateThemeRepository.swiftTests/ASCCommandTests/Commands/AppShots/AppShotsGalleryTemplatesTests.swiftTests/ASCCommandTests/Commands/AppShots/AppShotsTemplatesTests.swiftTests/ASCCommandTests/Commands/AppShots/AppShotsThemesTests.swiftTests/ASCCommandTests/Commands/Web/AppShotsControllerTests.swiftTests/DomainTests/ScreenshotPlans/ScreenDesignTests.swiftTests/DomainTests/ScreenshotPlans/TemplateApplyTests.swiftTests/DomainTests/Screenshots/AppShotTemplateTests.swiftTests/DomainTests/Screenshots/AppShotsConfigTests.swiftTests/DomainTests/Screenshots/Gallery/AppShotTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryApplyTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryCodableTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryComposeTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryHTMLRendererTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryPreviewTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryTemplateRepositoryTests.swiftTests/DomainTests/Screenshots/Gallery/GalleryTests.swiftTests/DomainTests/Screenshots/Gallery/HTMLComposerTests.swiftTests/DomainTests/Screenshots/Gallery/ScreenLayoutTests.swiftTests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swiftTests/DomainTests/Screenshots/Gallery/TextSlotPreviewTests.swiftTests/DomainTests/Screenshots/ScreenThemeTests.swiftTests/DomainTests/Screenshots/TemplateApplyTests.swiftTests/DomainTests/Screenshots/TemplateRenderTests.swiftTests/DomainTests/Screenshots/ThemeDesignApplierTests.swiftTests/DomainTests/Screenshots/ThemeDesignTests.swiftTests/DomainTests/Shared/PresentableTests.swiftTests/DomainTests/TestHelpers/MockRepositoryFactory.swiftTests/InfrastructureTests/ScreenshotPlans/AggregateTemplateRepositoryTests.swiftapps/asc-web/command-center/css/layout.cssdocs/features/app-shots-themes.mddocs/features/app-shots.md
💤 Files with no reviewable changes (8)
- Sources/Domain/ScreenshotPlans/LayoutMode.swift
- Tests/DomainTests/ScreenshotPlans/TemplateApplyTests.swift
- Sources/Domain/ScreenshotPlans/ThemedPage.swift
- Sources/Domain/ScreenshotPlans/TemplateContent.swift
- Tests/DomainTests/ScreenshotPlans/ScreenDesignTests.swift
- Sources/Domain/ScreenshotPlans/ScreenDesign.swift
- Sources/Domain/ScreenshotPlans/TemplateHTMLRenderer.swift
- Sources/Domain/ScreenshotPlans/ScreenshotTemplate.swift
Comment on lines
+16
to
+29
| nonisolated(unsafe) private static var _library: MustacheLibrary? | ||
|
|
||
| /// The template library. Lazily loads from bundle Resources on first access. | ||
| public static var library: MustacheLibrary { | ||
| if let lib = _library { return lib } | ||
| let lib = loadLibrary() | ||
| _library = lib | ||
| return lib | ||
| } | ||
|
|
||
| /// Replace the library (e.g. for plugins providing custom templates). | ||
| public static func setLibrary(_ lib: MustacheLibrary) { | ||
| _library = lib | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Shared mutable state declarations/usages:"
rg -n --type swift 'private static var _library|public static var library|setLibrary\('
echo
echo "Call sites of HTMLComposer accessors:"
rg -n --type swift 'HTMLComposer\.(library|setLibrary|render)\('Repository: tddworks/asc-cli
Length of output: 4811
🏁 Script executed:
cat -n Sources/Domain/Screenshots/Gallery/HTMLComposer.swiftRepository: tddworks/asc-cli
Length of output: 3043
🏁 Script executed:
# Check the GalleryHTMLRenderer context to understand if these are called concurrently
head -80 Sources/Domain/Screenshots/Gallery/GalleryHTMLRenderer.swift | cat -nRepository: tddworks/asc-cli
Length of output: 3227
🏁 Script executed:
# Check if GalleryHTMLRenderer methods are async or if they call HTMLComposer from concurrent contexts
rg -n --type swift -A 3 'public static func.*render' Sources/Domain/Screenshots/Gallery/GalleryHTMLRenderer.swiftRepository: tddworks/asc-cli
Length of output: 505
🏁 Script executed:
# Check for async/await, Task, DispatchQueue usage in GalleryHTMLRenderer
rg -n --type swift 'async|await|Task|DispatchQueue|@MainActor' Sources/Domain/Screenshots/Gallery/GalleryHTMLRenderer.swiftRepository: tddworks/asc-cli
Length of output: 42
🏁 Script executed:
# Check where GalleryHTMLRenderer methods are called from
rg -n --type swift 'GalleryHTMLRenderer\.(renderScreen|renderGallery|renderPreviewPage)' Sources/ Tests/Repository: tddworks/asc-cli
Length of output: 2098
🏁 Script executed:
# Check if there are any async contexts in the call chain
rg -n --type swift 'async func|func.*async' Sources/Domain/Screenshots/ | head -20Repository: tddworks/asc-cli
Length of output: 1886
🏁 Script executed:
# Check AppShotsController context to see if these are handling concurrent requests
sed -n '140,210p' Sources/ASCCommand/Commands/Web/Controllers/AppShotsController.swift | cat -nRepository: tddworks/asc-cli
Length of output: 4485
🏁 Script executed:
# Check if there's any actor isolation or concurrency safety in the codebase architecture
rg -n --type swift '@Actor|nonisolated|Sendable' Sources/Domain/ | head -15Repository: tddworks/asc-cli
Length of output: 1879
Unsynchronized mutable static state introduces a race in template library initialization
The _library property is read and written from concurrent web request handlers without synchronization. Requests arriving concurrently can race on both the lazy initialization (line 20–23) and setLibrary() (line 28), producing nondeterministic behavior. This is particularly critical since AppShotsController handles HTTP requests asynchronously, and each can trigger the getter. The nonisolated(unsafe) annotation explicitly opts out of actor isolation without providing thread safety — unlike PreviewCache in the same module, which correctly uses NSLock().
Additionally, rendering failures are silently swallowed: missing templates return empty strings (line 33), parse errors return the template unparsed (line 38), and resource loading errors return empty libraries (lines 46, 51).
🔒 Suggested fix
public enum HTMLComposer {
- nonisolated(unsafe) private static var _library: MustacheLibrary?
+ private static var _library: MustacheLibrary?
+ private static let libraryLock = NSLock()
public static var library: MustacheLibrary {
+ libraryLock.lock()
+ defer { libraryLock.unlock() }
if let lib = _library { return lib }
let lib = loadLibrary()
_library = lib
return lib
}
public static func setLibrary(_ lib: MustacheLibrary) {
+ libraryLock.lock()
+ defer { libraryLock.unlock() }
_library = lib
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| nonisolated(unsafe) private static var _library: MustacheLibrary? | |
| /// The template library. Lazily loads from bundle Resources on first access. | |
| public static var library: MustacheLibrary { | |
| if let lib = _library { return lib } | |
| let lib = loadLibrary() | |
| _library = lib | |
| return lib | |
| } | |
| /// Replace the library (e.g. for plugins providing custom templates). | |
| public static func setLibrary(_ lib: MustacheLibrary) { | |
| _library = lib | |
| } | |
| private static var _library: MustacheLibrary? | |
| private static let libraryLock = NSLock() | |
| /// The template library. Lazily loads from bundle Resources on first access. | |
| public static var library: MustacheLibrary { | |
| libraryLock.lock() | |
| defer { libraryLock.unlock() } | |
| if let lib = _library { return lib } | |
| let lib = loadLibrary() | |
| _library = lib | |
| return lib | |
| } | |
| /// Replace the library (e.g. for plugins providing custom templates). | |
| public static func setLibrary(_ lib: MustacheLibrary) { | |
| libraryLock.lock() | |
| defer { libraryLock.unlock() } | |
| _library = lib | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Domain/Screenshots/Gallery/HTMLComposer.swift` around lines 16 - 29,
The _library static is unsafely accessed concurrently: protect reads/writes to
nonisolated(unsafe) private static var _library by introducing a synchronization
primitive (e.g. an NSLock or DispatchQueue) used inside the public static var
library getter and public static func setLibrary(_:) so lazy initialization
(library getter calling loadLibrary()) and replacement via setLibrary(_) are
atomic and race-free; also update loadLibrary() and the template rendering paths
it uses to surface or log errors instead of swallowing them (do not return empty
strings or silent fallbacks)—have loadLibrary propagate or log resource/parse
errors and ensure the library getter logs/propagates load failures so callers
(e.g. AppShotsController) can handle them deterministically.
| @@ -0,0 +1 @@ | |||
| {{{themeVars}}}<div data-theme="{{theme}}" style="background:{{background}};position:relative;overflow:hidden;container-type:inline-size;width:100%;height:100%">{{#tagline}}<div style="position:absolute;top:{{top}}%;left:{{pad}}%;right:{{pad}}%;z-index:4;font-weight:{{weight}};font-size:{{fontSize}}cqi;color:{{color}};letter-spacing:0.1em;text-transform:uppercase;text-align:{{align}};white-space:pre-line">{{{content}}}</div>{{/tagline}}{{#headline}}<div style="position:absolute;top:{{top}}%;left:{{pad}}%;right:{{pad}}%;z-index:4;font-weight:{{weight}};font-size:{{fontSize}}cqi;color:{{color}};line-height:0.92;letter-spacing:-0.03em;text-align:{{align}};white-space:pre-line">{{{content}}}</div>{{/headline}}{{#subheading}}<div style="position:absolute;top:{{top}}%;left:{{pad}}%;right:{{padRight}}%;z-index:4;font-weight:{{weight}};font-size:{{fontSize}}cqi;color:var(--body-color);line-height:1.4;text-align:{{align}}">{{{content}}}</div>{{/subheading}}{{#trustMarks}}<div style="position:absolute;top:{{top}}%;left:{{pad}}%;z-index:4;display:flex;gap:4px;flex-wrap:wrap">{{#items}}<span style="background:var(--badge-bg);border-radius:5px;padding:0.3cqi 0.8cqi;font-size:{{fontSize}}cqi;font-weight:700;color:var(--trust-color);letter-spacing:0.04em">{{text}}</span>{{/items}}</div>{{/trustMarks}}{{#badges}}<div style="position:absolute;left:{{left}}%;top:{{top}}%;z-index:5;background:var(--badge-bg);border:1px solid var(--badge-border);border-radius:100px;padding:0.3cqi 0.8cqi;font-size:{{fontSize}}cqi;font-weight:700;color:var(--badge-color);backdrop-filter:blur(8px);-webkit-backdrop-filter:blur(8px);white-space:nowrap">{{text}}</div>{{/badges}}{{#devices}}{{#hasScreenshot}}<div style="position:absolute;left:{{left}}%;top:{{top}}%;width:{{width}}%;z-index:2"><img src="{{screenshot}}" style="width:100%;display:block" alt=""></div>{{/hasScreenshot}}{{#hasWireframe}}<div style="position:absolute;left:{{left}}%;top:{{top}}%;width:{{width}}%;z-index:2">{{#hasPhoneFrame}}<div style="aspect-ratio:1470/3000;position:relative;filter:drop-shadow(0 4px 20px rgba(0,0,0,var(--shadow-opacity)))">{{{wireframeHTML}}}<img src="{{phoneFrameURL}}" style="position:absolute;inset:0;width:100%;height:100%;z-index:2;pointer-events:none" alt=""></div>{{/hasPhoneFrame}}{{#noPhoneFrame}}<div style="aspect-ratio:1470/3000;position:relative;filter:drop-shadow(0 4px 20px rgba(0,0,0,var(--shadow-opacity)));background:var(--frame-bg);border-radius:12%/5.5%;border:1.5px solid var(--frame-border);overflow:hidden">{{{wireframeHTML}}}</div>{{/noPhoneFrame}}</div>{{/hasWireframe}}{{/devices}}{{#decorations}}<div style="position:absolute;left:{{left}}%;top:{{top}}%;z-index:3;font-size:{{fontSize}}cqi;opacity:{{opacity}};background:{{background}};color:{{#useDefaultColor}}var(--deco-color){{/useDefaultColor}}{{color}};border-radius:{{borderRadius}};padding:0.3cqi 0.8cqi;pointer-events:none;white-space:nowrap;{{animStyle}}">{{{content}}}</div>{{/decorations}}{{#hasAnimations}}{{{keyframesHTML}}}{{/hasAnimations}}</div> No newline at end of file | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In swift-mustache, does {{{name}}}render unescaped HTML while{{name}} HTML-escapes output?
💡 Result:
Yes. In swift-mustache (Hummingbird’s Swift Mustache implementation), {{name}} renders HTML-escaped output by default, while {{{name}}} renders the value unescaped (raw HTML). [1][2]
🏁 Script executed:
#!/bin/bash
# Find references to screen.mustache and understand context
echo "=== Finding references to screen.mustache ==="
rg "screen\.mustache" --type-list | head -20
rg "screen\.mustache" -l
echo -e "\n=== Exploring directory structure ==="
find Sources/Domain/Screenshots -type f -name "*.swift" -o -name "*.ts" -o -name "*.js" | head -20Repository: tddworks/asc-cli
Length of output: 1521
🏁 Script executed:
#!/bin/bash
# Look for how content/tagline/headline are populated and if they're sanitized
echo "=== Searching for content field assignments ==="
rg "content" Sources/Domain/Screenshots -t swift -A 2 -B 2 | head -80
echo -e "\n=== Searching for sanitization patterns ==="
rg "sanitize|escape|html" Sources/Domain/Screenshots -t swift | head -20Repository: tddworks/asc-cli
Length of output: 8668
🏁 Script executed:
#!/bin/bash
# Look for the structure that builds the template context
echo "=== Searching for wireframeHTML, keyframesHTML (trusted generated HTML) ==="
rg "wireframeHTML|keyframesHTML" Sources/Domain/Screenshots -t swift -A 2 -B 2 | head -50Repository: tddworks/asc-cli
Length of output: 1295
Escape user-provided text before rendering to prevent XSS injection.
The template injects user content from AppShot (tagline, headline, body) directly into the page using unescaped {{{content}}} variables. A user providing <img src=x onerror="alert()"> as a headline would execute arbitrary JavaScript. This affects tagline, headline, subheading, and decoration text slots.
The codebase intentionally replaces newlines with <br> tags before passing content to the template. To fix this securely, HTML-escape the user text first, then insert the intentional <br> tags (or use a dedicated template variable for safe HTML fragments), then render with {{{safecontent}}}. Alternatively, apply escaping in the template itself using {{content}} while handling line breaks separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Domain/Screenshots/Gallery/Resources/screen.mustache` at line 1, The
template renders unescaped user text via {{{content}}} for tagline, headline,
subheading, and decorations causing XSS; fix by HTML-escaping user-provided
strings on the server (escape &, <, >, ", ') then re-insert the intended <br>
line breaks (or build a separate safeHtml fragment) and expose that escaped
fragment as e.g. safeContent (or safeTagline/safeHeadline/etc.), then update the
mustache usage to render the pre-sanitized HTML fragment (e.g.
{{{safeContent}}}) or switch to the escaped variable {{content}} and handle
breaks separately; update all occurrences where {{{content}}} is used (tagline,
headline, subheading, decorations) so only escaped/sanitized content is
injected.
hanrw
changed the title
- Simplify GalleryPreviewOutputTests with inline sample data - Add tests for empty and dark-themed gallery previews - Refactor TemplatePreviewPageTests to use hardcoded templates - Cover multi-device layouts and gradient backgrounds in previews
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift (1)
31-33: Prefer user-visible assertions over template internalsThese checks are tightly coupled to implementation tokens (
container-type, raw hex snippets,<!DOCTYPE html>,9:41). Consider asserting what a user would perceive in preview output (e.g., expected headline content, presence of preview sections/devices via stable semantic markers) instead of CSS/HTML internals.♻️ Suggested direction
- `#expect`(html.contains("<!DOCTYPE html>")) - `#expect`(html.contains("container-type")) + `#expect`(html.contains("SHIP FASTER")) + `#expect`(html.contains("Clean Design")) - `#expect`(html.contains("#ff6b6b")) - `#expect`(html.contains("#feca57")) + `#expect`(html.contains("linear-gradient(")) // higher-level background intent - `#expect`(html.contains("<!DOCTYPE html>")) - `#expect`(html.contains("9:41")) + `#expect`(html.contains("Compare"))As per coding guidelines
Tests/**/*.swift: Test cases must reflect the user's mental model — describe what the user sees and expects, not internal implementation details.Also applies to: 43-44, 60-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift` around lines 31 - 33, Replace the fragile html.contains assertions that check implementation tokens (e.g., html.contains("<!DOCTYPE html>") and html.contains("container-type")) with user-facing assertions that validate visible preview content and semantic markers; update the assertions in TemplatePreviewPageTests (where html is inspected) to assert for expected headline text, presence of preview sections/devices via stable semantic markers or ARIA/role attributes, and any sample content a user would see (instead of raw hex/snippets or "9:41"), so tests use html.contains(...) only for user-visible strings or stable semantic attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift`:
- Around line 47-63: Update the test `multi-device template preview renders all
device slots` to assert the number of rendered device slots rather than a single
match: after obtaining tmpl.previewHTML in the test, count occurrences of the
wireframe marker (e.g., the "9:41" status bar string) in html and assert the
count equals the number of DeviceSlot entries (2 for the current tmpl). Modify
the assertion that currently uses html.contains("9:41") to a count-based check
(for example using html.components(separatedBy: "9:41").count - 1 or a regex
match count) so the test verifies both DeviceSlot instances are rendered.
---
Nitpick comments:
In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift`:
- Around line 31-33: Replace the fragile html.contains assertions that check
implementation tokens (e.g., html.contains("<!DOCTYPE html>") and
html.contains("container-type")) with user-facing assertions that validate
visible preview content and semantic markers; update the assertions in
TemplatePreviewPageTests (where html is inspected) to assert for expected
headline text, presence of preview sections/devices via stable semantic markers
or ARIA/role attributes, and any sample content a user would see (instead of raw
hex/snippets or "9:41"), so tests use html.contains(...) only for user-visible
strings or stable semantic attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cbe0b68-ea58-46cf-b971-89c78507b062
📒 Files selected for processing (2)
Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swiftTests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/DomainTests/Screenshots/Gallery/GalleryPreviewOutputTests.swift
Comment on lines
+47
to
+63
| @Test func `multi-device template preview renders all device slots`() { | ||
| let tmpl = AppShotTemplate( | ||
| id: "duo", name: "Duo", | ||
| screenLayout: ScreenLayout( | ||
| headline: TextSlot(y: 0.04, size: 0.08, preview: "Compare"), | ||
| devices: [ | ||
| DeviceSlot(x: 0.35, y: 0.2, width: 0.6), | ||
| DeviceSlot(x: 0.65, y: 0.24, width: 0.6), | ||
| ] | ||
| ), | ||
| palette: GalleryPalette(id: "d", name: "D", background: "#1a1a2e") | ||
| ) | ||
| let html = tmpl.previewHTML | ||
| #expect(html.contains("<!DOCTYPE html>")) | ||
| // Both devices should be rendered (as wireframes since no screenshot) | ||
| #expect(html.contains("9:41")) // wireframe status bar | ||
| } |
There was a problem hiding this comment.
“All device slots” test currently validates only one rendered slot
The test name promises all device slots are rendered, but html.contains("9:41") only proves at least one match. Please assert the expected count.
✅ Minimal fix
- `#expect`(html.contains("9:41")) // wireframe status bar
+ let renderedWireframes = html.components(separatedBy: "9:41").count - 1
+ `#expect`(renderedWireframes == 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/DomainTests/Screenshots/Gallery/TemplatePreviewPageTests.swift` around
lines 47 - 63, Update the test `multi-device template preview renders all device
slots` to assert the number of rendered device slots rather than a single match:
after obtaining tmpl.previewHTML in the test, count occurrences of the wireframe
marker (e.g., the "9:41" status bar string) in html and assert the count equals
the number of DeviceSlot entries (2 for the current tmpl). Modify the assertion
that currently uses html.contains("9:41") to a count-based check (for example
using html.components(separatedBy: "9:41").count - 1 or a regex match count) so
the test verifies both DeviceSlot instances are rendered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gallery-templates list,gallery-templates get --preview)ThemeDesign(palette + decorations) with 1 AI call (themes design), then apply deterministically to any number of screenshots (themes apply-design). No repeated AI calls..mustachetemplate files using swift-mustache.GalleryHTMLRendereronly builds context dicts; all HTML, CSS custom properties (light/dark), and keyframe animations live in templates. Templates are pre-compiled at startup.New CLI Commands
asc app-shots gallery-templates listasc app-shots gallery-templates get --id <ID> [--preview]asc app-shots themes design --id <ID>asc app-shots themes apply-design --design <FILE> --template <ID> ...asc app-shots themes apply --theme <ID> --template <ID> ...New REST Endpoints
GET /api/v1/app-shots/gallery-templatesPOST /api/v1/app-shots/gallery/composePOST /api/v1/app-shots/themes/designPOST /api/v1/app-shots/themes/apply-designPOST /api/v1/app-shots/themes/applyArchitecture Changes
GalleryHTMLRenderer→ context builder only (zero HTML in Swift)HTMLComposer→ wrapsMustacheLibrarywith compiled template cachingGalleryPalette→ ownsisLightdetection andheadlineColorderivationDecorationShape.displayCharacter→ model owns its display dataThemeDesignApplier→ re-renders through standard pipeline with overridden palette + merged decorationsGalleryHTMLRenderer.cachedPreview()for fast API responsestheme-vars.mustachefor light/dark theming (no color values in Swift)Web UI
design()implemented via compose bridgemode: "design"Test plan
swift test— 1698 tests pass (5 new for gallery-templates, 2 new for themes design/apply-design)examples/blitz-screenshots/docs/E2E-TEST.md🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
gallery-templates list/get) and REST endpoints (GET /app-shots/gallery-templates).--design-only/--apply-design; REST:POST /app-shots/themes/designandPOST /app-shots/themes/apply-design).Documentation