From 90324007e80513274141a19b55b0f5ad679e0f41 Mon Sep 17 00:00:00 2001 From: alexey1312 Date: Thu, 30 Jul 2026 10:59:31 +0500 Subject: [PATCH] fix(images): export light/dark pairs together with granular cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ImagesLoader.loadFromSingleFileWithGranularCache` filtered to changed nodes without pairing, so a designer edit to only the dark variant produced an orphan dark asset and `ImagesProcessor` rejected the whole config: Error: Asset count mismatch: light=0, dark=1 Missing assets in Light: safety-sos-button-color f124526 added pairing to `IconsLoader` only. `ImagesLoader` was never updated, and both of its branches were affected — the PNG branch had no pairing-aware helper at all. This broke `03_daily_assets_update` in Oymyakon-Atoms-iOS on every day the illustrations file changed (4 runs in July). - `loadPNGImagesWithGranularCache` takes `darkModeSuffix: String? = nil` and routes to `fetchImageComponentsWithGranularCacheAndPairing` when set - single-file PNG branch passes `darkSuffix`; SVG branch switches to `loadVectorImagesWithGranularCacheAndPairing` - `loadFromLightAndDarkFileWithGranularCache` keeps `nil` on purpose: light and dark live in separate files and are filtered independently Also fixed, found during the same investigation: - `batch` returned exit code 0 while reporting failures, so CI treated a broken export as green — now throws `ExitCode.failure` when `failureCount > 0` - the failure table truncates both config name and error, making CI logs useless for diagnosis — full errors are now printed underneath it - 5 tests in `SubcommandFaultTolerancePrecedenceTests` crashed on `ParsableArguments.init()`, which leaves every `@Option` undecoded so the first read traps. Switched to `try FaultToleranceOptions.parse([])`, the pattern already used throughout `FaultToleranceOptionsTests`. Under `--parallel` this surfaced as a non-zero exit with no `XCTAssert` failure line, which is why it read as a green suite. Full suite now green: 1996 XCTest + 211 swift-testing, exit 0. --- .claude/rules/cache-granular.md | 24 ++++ .claude/rules/gotchas.md | 25 ++++ .claude/rules/modification-checklists.md | 20 +++ .../ExFigCLI/Loaders/ImageLoaderBase.swift | 20 ++- Sources/ExFigCLI/Loaders/ImagesLoader.swift | 6 +- Sources/ExFigCLI/Subcommands/Batch.swift | 13 ++ ...commandFaultTolerancePrecedenceTests.swift | 15 +- ...magesLoaderGranularCachePairingTests.swift | 136 ++++++++++++++++++ 8 files changed, 249 insertions(+), 10 deletions(-) create mode 100644 Tests/ExFigTests/Loaders/ImagesLoaderGranularCachePairingTests.swift diff --git a/.claude/rules/cache-granular.md b/.claude/rules/cache-granular.md index 922f53b7..9e45c2a4 100644 --- a/.claude/rules/cache-granular.md +++ b/.claude/rules/cache-granular.md @@ -74,6 +74,30 @@ This pattern ensures: - Multiple configs referencing the same Figma file all benefit from granular cache tracking - Single atomic save at the end contains both file versions AND nodeHashes +## Light/Dark Pairing (mandatory in single-file mode) + +In `useSingleFile` + `suffixDarkMode` mode, filtering to changed nodes alone breaks +`ImagesProcessor.process(light:dark:)` — it requires matching pairs and throws +`AssetsValidatorError.countMismatch` (`Asset count mismatch: light=0, dark=1`) when only one side changed. + +Every single-file granular path MUST use the pairing-aware variant so a changed dark node drags its +light sibling into the export: + +```toon +paths[3]{caller,method,pairing}: + IconsLoader.loadFromSingleFileWithGranularCache,loadVectorImagesWithGranularCacheAndPairing,required + ImagesLoader.loadFromSingleFileWithGranularCache (PNG),loadPNGImagesWithGranularCache(darkModeSuffix:),required + ImagesLoader.loadFromSingleFileWithGranularCache (SVG),loadVectorImagesWithGranularCacheAndPairing,required +``` + +`loadFromLightAndDarkFileWithGranularCache` must NOT pair (`darkModeSuffix: nil`) — light and dark live +in separate files and are filtered independently. + +Pairing matches on `baseName(for: component.iconName, darkModeSuffix:)`, so `darkSuffix` must be the +same value later passed to `splitByDarkMode` (both default to `"_dark"` when `suffixDarkMode` is unset). + +Regression tests: `IconsLoaderGranularCachePairingTests`, `ImagesLoaderGranularCachePairingTests`. + ## Known Limitations - Config changes (output path, format, scale) are not detected - use `--force` when config changes diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index 91f69400..a1eef77f 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -202,6 +202,31 @@ final class Collector: Sendable { } ``` +## ParsableArguments: never construct with bare `init()` and then read it + +`ParsableArguments.init()` satisfies the protocol but leaves every `@Option`/`@Flag` **undecoded**. +Assigning is fine; the first *read* traps and kills the whole test process: + +``` +Can't read a value from a parsable argument definition. +``` + +Under `swift test --parallel` this shows up as a suite that logs `started` with no `passed` and a +non-zero exit — no `XCTAssert` failure line, so it is easy to mistake for a green run. + +```swift +// BAD — traps on the first read of .rateLimit +cmd.faultToleranceOptions = FaultToleranceOptions() +_ = cmd.faultToleranceOptions.effectiveRateLimit(configValue: nil) + +// GOOD — "no CLI flag given", all defaults decoded +cmd.faultToleranceOptions = try FaultToleranceOptions.parse([]) +``` + +Applies to `filter`, `strictPathValidation`, and every `@OptionGroup` on manually built subcommands +(the `BatchConfigRunner.make*` pattern). Production is safe — `BatchConfigRunner` receives +already-parsed option groups; only hand-rolled test instances hit this. + ## Test Helpers for Codable Types ```swift diff --git a/.claude/rules/modification-checklists.md b/.claude/rules/modification-checklists.md index 0fae53d0..5e017448 100644 --- a/.claude/rules/modification-checklists.md +++ b/.claude/rules/modification-checklists.md @@ -22,6 +22,26 @@ When adding fields to `FrameSource` (PKL) / `SourceInput` (ExFigCore), also upda 8. `DownloadAll.swift` — pass filter value to both `exportIcons` and `exportImages` 9. Error/warning types with context (`ExFigError`, `ExFigWarning`) — add associated values if needed +## Modifying IconsLoader or ImagesLoader (always both) + +`IconsLoader` and `ImagesLoader` are siblings over `ImageLoaderBase` with near-identical +`loadFromSingleFileWithGranularCache` / `loadFromLightAndDarkFileWithGranularCache` structure. +A fix applied to one is usually a bug left in the other. Put shared logic in `ImageLoaderBase` and +check ALL granular call sites: + +```toon +call_sites[4]{loader,mode,branches}: + IconsLoader,single-file,vector only + IconsLoader,light+dark files,vector only + ImagesLoader,single-file,PNG + SVG (two branches!) + ImagesLoader,light+dark files,PNG + SVG (two branches!) +``` + +`ImagesLoader` has a raster branch that `IconsLoader` does not (`isRasterFormat && !useSVGSource`) — +any helper added for icons needs a PNG counterpart. Example: `f124526` added light/dark pairing to +`IconsLoader` only; `ImagesLoader` kept exporting orphan dark assets until `loadPNGImagesWithGranularCache` +gained `darkModeSuffix:`. See `.claude/rules/cache-granular.md` → Light/Dark Pairing. + ## Adding a New Filter Level (e.g., page filtering) Filter predicate sites that ALL need updating: diff --git a/Sources/ExFigCLI/Loaders/ImageLoaderBase.swift b/Sources/ExFigCLI/Loaders/ImageLoaderBase.swift index 99fb41eb..0ff0b6e7 100644 --- a/Sources/ExFigCLI/Loaders/ImageLoaderBase.swift +++ b/Sources/ExFigCLI/Loaders/ImageLoaderBase.swift @@ -572,20 +572,32 @@ class ImageLoaderBase: @unchecked Sendable { /// 2. Computes content hashes for granular change detection /// 3. Filters to only changed components (if cache exists) /// 4. Returns computed hashes for cache update after export + /// + /// Pass `darkModeSuffix` in `useSingleFile` mode so a changed dark node drags its light sibling + /// into the export — otherwise `ImagesProcessor` validation fails with a count mismatch. + /// Leave it `nil` in light/dark-file mode, where each file is filtered independently. func loadPNGImagesWithGranularCache( fileId: String, frameName: String, pageName: String? = nil, filter: String? = nil, scales: [Double], + darkModeSuffix: String? = nil, rtlProperty: String? = Component.defaultRTLProperty, rtlActiveValues: [String]? = nil, onBatchProgress: @escaping BatchProgressCallback = { _, _ in } ) async throws -> ImagesWithHashesResult { - let filterResult = try await fetchImageComponentsWithGranularCache( - fileId: fileId, frameName: frameName, pageName: pageName, filter: filter, rtlProperty: rtlProperty, - rtlActiveValues: rtlActiveValues - ) + let filterResult = if let darkModeSuffix { + try await fetchImageComponentsWithGranularCacheAndPairing( + fileId: fileId, frameName: frameName, pageName: pageName, filter: filter, + darkModeSuffix: darkModeSuffix, rtlProperty: rtlProperty, rtlActiveValues: rtlActiveValues + ) + } else { + try await fetchImageComponentsWithGranularCache( + fileId: fileId, frameName: frameName, pageName: pageName, filter: filter, rtlProperty: rtlProperty, + rtlActiveValues: rtlActiveValues + ) + } if filterResult.allSkipped { return ImagesWithHashesResult( diff --git a/Sources/ExFigCLI/Loaders/ImagesLoader.swift b/Sources/ExFigCLI/Loaders/ImagesLoader.swift index 511c19c9..002a6d5b 100644 --- a/Sources/ExFigCLI/Loaders/ImagesLoader.swift +++ b/Sources/ExFigCLI/Loaders/ImagesLoader.swift @@ -427,12 +427,14 @@ final class ImagesLoader: ImageLoaderBase, @unchecked Sendable { // swiftlint:di // PNG source: Raster images (PNG/WebP) with granular cache let scales = getScales(customScales: configScales) + // Use pairing-aware method to ensure light/dark pairs are exported together let result = try await loadPNGImagesWithGranularCache( fileId: fileId, frameName: frameName, pageName: pageName, filter: filter, scales: scales, + darkModeSuffix: darkSuffix, rtlProperty: config.rtlProperty, rtlActiveValues: config.rtlActiveValues, onBatchProgress: onBatchProgress @@ -461,12 +463,14 @@ final class ImagesLoader: ImageLoaderBase, @unchecked Sendable { // swiftlint:di ) } else { // SVG source or vector output: fetch SVG with granular cache - let result = try await loadVectorImagesWithGranularCache( + // Use pairing-aware method to ensure light/dark pairs are exported together + let result = try await loadVectorImagesWithGranularCacheAndPairing( fileId: fileId, frameName: frameName, pageName: pageName, params: SVGParams(), filter: filter, + darkModeSuffix: darkSuffix, rtlProperty: config.rtlProperty, rtlActiveValues: config.rtlActiveValues, onBatchProgress: onBatchProgress diff --git a/Sources/ExFigCLI/Subcommands/Batch.swift b/Sources/ExFigCLI/Subcommands/Batch.swift index 6cb7a77b..d2981a41 100644 --- a/Sources/ExFigCLI/Subcommands/Batch.swift +++ b/Sources/ExFigCLI/Subcommands/Batch.swift @@ -150,6 +150,11 @@ extension ExFigCommand { resolved: resolved, ui: ui ) + + // A reported failure must fail the process — otherwise CI treats a broken export as green. + if result.failureCount > 0 { + throw ExitCode.failure + } } // MARK: - Run Helpers @@ -868,6 +873,14 @@ extension ExFigCommand { } NooraUI.shared.table(headers: headers, rows: rows) + + // The table truncates both the config name and the error, which makes CI logs useless + // for identifying what failed. Repeat failures in full underneath it. + guard !result.failures.isEmpty else { return } + ui.info("") + for failure in result.failures { + ui.error("\(failure.config.name): \(failure.error.localizedDescription)") + } } private func displayRateLimitStatus(status: RateLimiterStatus, ui: TerminalUI) { diff --git a/Tests/ExFigTests/Input/SubcommandFaultTolerancePrecedenceTests.swift b/Tests/ExFigTests/Input/SubcommandFaultTolerancePrecedenceTests.swift index 51f5f168..870fb259 100644 --- a/Tests/ExFigTests/Input/SubcommandFaultTolerancePrecedenceTests.swift +++ b/Tests/ExFigTests/Input/SubcommandFaultTolerancePrecedenceTests.swift @@ -11,6 +11,11 @@ import XCTest /// /// Without these, a regression that swaps `??` direction or stops passing `figma?.rateLimit` /// in any subcommand would slip through unit tests on `FaultToleranceOptions` alone. +/// +/// "No CLI flag given" MUST be spelled `try FaultToleranceOptions.parse([])`, never the bare +/// `FaultToleranceOptions()` initializer: `ParsableArguments.init()` leaves every `@Option` +/// undecoded, so the first read of `.rateLimit` / `.timeout` traps with +/// "Can't read a value from a parsable argument definition" and kills the test process. final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { private var tempFiles: [URL] = [] @@ -49,7 +54,7 @@ final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { cmd.globalOptions = GlobalOptions() cmd.options = options cmd.cacheOptions = CacheOptions() - cmd.faultToleranceOptions = FaultToleranceOptions() + cmd.faultToleranceOptions = try FaultToleranceOptions.parse([]) cmd.filter = nil let figma = cmd.options.params.figma @@ -63,7 +68,7 @@ final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { cmd.globalOptions = GlobalOptions() cmd.options = options cmd.cacheOptions = CacheOptions() - cmd.faultToleranceOptions = FaultToleranceOptions() + cmd.faultToleranceOptions = try FaultToleranceOptions.parse([]) cmd.filter = nil let figma = cmd.options.params.figma @@ -98,7 +103,7 @@ final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { cmd.globalOptions = GlobalOptions() cmd.options = options cmd.cacheOptions = CacheOptions() - cmd.faultToleranceOptions = HeavyFaultToleranceOptions() + cmd.faultToleranceOptions = try HeavyFaultToleranceOptions.parse([]) cmd.filter = nil let figma = cmd.options.params.figma @@ -114,7 +119,7 @@ final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { cmd.globalOptions = GlobalOptions() cmd.options = options cmd.cacheOptions = CacheOptions() - cmd.faultToleranceOptions = HeavyFaultToleranceOptions() + cmd.faultToleranceOptions = try HeavyFaultToleranceOptions.parse([]) cmd.filter = nil cmd.strictPathValidation = false @@ -143,7 +148,7 @@ final class SubcommandFaultTolerancePrecedenceTests: XCTestCase { func testPKLTimeoutUsedWhenCLITimeoutAbsent() throws { let options = try makeOptions(figmaBlock: "timeout = 60.0") - let cliOpts = FaultToleranceOptions() + let cliOpts = try FaultToleranceOptions.parse([]) let configTimeout = options.params.figma?.timeout let effective: TimeInterval? = cliOpts.timeout.map { TimeInterval($0) } ?? configTimeout diff --git a/Tests/ExFigTests/Loaders/ImagesLoaderGranularCachePairingTests.swift b/Tests/ExFigTests/Loaders/ImagesLoaderGranularCachePairingTests.swift new file mode 100644 index 00000000..5332b87b --- /dev/null +++ b/Tests/ExFigTests/Loaders/ImagesLoaderGranularCachePairingTests.swift @@ -0,0 +1,136 @@ +@testable import ExFigCLI +import ExFigCore +@testable import FigmaAPI +import Logging +import XCTest + +/// Tests for granular cache light/dark pairing logic in ImagesLoader. +/// +/// Mirrors `IconsLoaderGranularCachePairingTests`: in single-file + `suffixDarkMode` mode a changed +/// dark node must drag its light sibling into the export, otherwise `ImagesProcessor.process(light:dark:)` +/// rejects the unpaired asset with `countMismatch(light:dark:)` and the whole config fails. +final class ImagesLoaderGranularCachePairingTests: XCTestCase { + var mockClient: MockClient! + var logger: Logger! + + override func setUp() { + super.setUp() + mockClient = MockClient() + logger = Logger(label: "test") + } + + override func tearDown() { + mockClient = nil + super.tearDown() + } + + /// iOS illustrations — the PNG branch of `loadFromSingleFileWithGranularCache`. + func testOnlyDarkChanged_pngBranch_includesBothVersions() async throws { + let secondResult = try await runOnlyDarkChangedScenario(platform: .ios) + + XCTAssertFalse(secondResult.allSkipped) + XCTAssertEqual( + secondResult.light.count, 1, + "light sibling must be re-exported together with the changed dark node" + ) + XCTAssertEqual(secondResult.dark?.count, 1) + XCTAssertEqual(secondResult.light.first?.name, "illuHome") + + assertProcessorAccepts(secondResult, platform: .ios) + } + + /// Web illustrations — the SVG/vector branch of `loadFromSingleFileWithGranularCache`. + func testOnlyDarkChanged_vectorBranch_includesBothVersions() async throws { + let secondResult = try await runOnlyDarkChangedScenario(platform: .web) + + XCTAssertFalse(secondResult.allSkipped) + XCTAssertEqual( + secondResult.light.count, 1, + "light sibling must be re-exported together with the changed dark node" + ) + XCTAssertEqual(secondResult.dark?.count, 1) + + assertProcessorAccepts(secondResult, platform: .web) + } + + // MARK: - Scenario + + /// Exports twice: cold cache, then with only the dark node of `illuHome` modified. + private func runOnlyDarkChangedScenario(platform: Platform) async throws -> ImagesLoaderResultWithHashes { + let components = [ + Component.make(nodeId: "1:1", name: "illuHome", frameName: "InDrive"), + Component.make(nodeId: "1:2", name: "illuHome-dark", frameName: "InDrive"), + Component.make(nodeId: "1:3", name: "illuCar", frameName: "InDrive"), + Component.make(nodeId: "1:4", name: "illuCar-dark", frameName: "InDrive"), + ] + + let nodes = makeNodeResponse(for: components) + mockClient.setResponse(nodes, for: NodesEndpoint.self) + mockClient.setResponse(components, for: ComponentsEndpoint.self) + mockClient.setResponse(makeImageURLs(for: components), for: ImageEndpoint.self) + + var cache = ImageTrackingCache() + cache.updateFileVersion(fileId: "file123", version: "v1") + + let params = PKLConfig.make( + lightFileId: "file123", imagesFrameName: "InDrive", + imagesSuffixDarkMode: "-dark" + ) + + let loader = ImagesLoader(client: mockClient, params: params, platform: platform, logger: logger) + loader.granularCacheManager = GranularCacheManager(client: mockClient, cache: cache) + + let firstResult = try await loader.loadWithGranularCache() + XCTAssertFalse(firstResult.allSkipped) + XCTAssertEqual(firstResult.light.count, 2) + XCTAssertEqual(firstResult.dark?.count, 2) + + for (fileId, hashes) in firstResult.computedHashes { + cache.updateNodeHashes(fileId: fileId, hashes: hashes) + } + + // Modify ONLY the dark version of illuHome — a designer tweak to the dark variant. + var modifiedNodes = nodes + modifiedNodes["1:2"] = Node.makeWithFill( + id: "1:2", name: "illuHome-dark", + fillColor: PaintColor(r: 1.0, g: 0.0, b: 0.0, a: 1.0) + ) + mockClient.setResponse(modifiedNodes, for: NodesEndpoint.self) + + let loader2 = ImagesLoader(client: mockClient, params: params, platform: platform, logger: logger) + loader2.granularCacheManager = GranularCacheManager(client: mockClient, cache: cache) + + return try await loader2.loadWithGranularCache() + } + + // MARK: - Helpers + + /// The downstream consequence of missing pairing: `ImagesProcessor` rejects an unpaired dark asset. + private func assertProcessorAccepts(_ result: ImagesLoaderResultWithHashes, platform: Platform) { + let processor = ImagesProcessor(platform: platform, nameStyle: .camelCase) + let processed = processor.process(light: result.light, dark: result.dark) + if case let .failure(error) = processed.result { + XCTFail("ImagesProcessor validation failed: \(error.localizedDescription)") + } + } + + private func makeNodeResponse(for components: [Component]) -> [NodeId: Node] { + var nodes: [NodeId: Node] = [:] + for component in components { + nodes[component.nodeId] = Node.makeWithFill( + id: component.nodeId, + name: component.name, + fillColor: PaintColor(r: 0.5, g: 0.5, b: 0.5, a: 1.0) + ) + } + return nodes + } + + private func makeImageURLs(for components: [Component]) -> [NodeId: ImagePath?] { + var urls: [NodeId: ImagePath?] = [:] + for component in components { + urls[component.nodeId] = "https://figma.com/\(component.name).png" + } + return urls + } +}