fix: validate and correct export-report proposal against codebase - #61
Merged
Conversation
Key corrections after codebase validation: - Decision 4: TerminalUI does NOT collect warnings (only prints them); replaced with new WarningCollector actor following SharedThemeAttributesCollector pattern - Decision 7: SHA256 → FNV-1a (no CryptoKit/swift-crypto in project; FNV1aHasher already available in codebase) - Decision 1: ExportStats is NOT Codable and has batch-only fields; create new ReportStats: Encodable instead of reusing ExportStats - Added Decision 9 (version field) and Decision 10 (run() result capture) - Fixed task file locations (Batch/ → Report/), added missing tasks - Added spec requirements: version field, zero-file export edge case - Updated all SHA256 references to FNV-1a 16-char hex throughout https://claude.ai/code/session_01M6Cw6Pcsy5L1KVkPZ1esu8
Summary of ChangesHello @alexey1312, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of validating and refining the export-report feature design against the existing codebase. The corrections to use a new ReportStats struct, introduce a WarningCollector actor, and adopt the existing FNV-1a hasher are all well-reasoned and significantly improve the proposal's feasibility. The addition of a version field and ensuring result capture are also great improvements for robustness. My review focuses on aligning the updated documentation with the repository's TOON format convention for tables and lists, as specified in the style guide.
Comment on lines
+90
to
95
| | Option | Pros | Cons | | ||
| | ----------------------------- | ---------------------------------------- | --------------------------------------- | | ||
| | Add collection to TerminalUI | Single point of interception | Adds state to stateless class | | ||
| | Separate WarningCollector | Clean separation, follows actor pattern | Must wire into export commands | | ||
| | Intercept at queueLogMessage | No TerminalUI changes | Only works in batch mode | | ||
|
|
There was a problem hiding this comment.
The "Options considered" table is in Markdown format. According to the repository style guide, all tabular data should use the TOON format to reduce token usage and maintain consistency.
Suggested change
| | Option | Pros | Cons | | |
| | ----------------------------- | ---------------------------------------- | --------------------------------------- | | |
| | Add collection to TerminalUI | Single point of interception | Adds state to stateless class | | |
| | Separate WarningCollector | Clean separation, follows actor pattern | Must wire into export commands | | |
| | Intercept at queueLogMessage | No TerminalUI changes | Only works in batch mode | | |
| options[3]{option,pros,cons}: | |
| "Add collection to TerminalUI","Single point of interception","Adds state to stateless class" | |
| "Separate WarningCollector","Clean separation, follows actor pattern","Must wire into export commands" | |
| "Intercept at queueLogMessage","No TerminalUI changes","Only works in batch mode" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+137
to
+142
| | Option | Pros | Cons | | ||
| | ----------------------- | --------------------------------------- | --------------------------------------------------------- | | ||
| | SHA256 (CryptoKit) | Industry standard, 64-char hex | macOS-only; Linux needs `swift-crypto` dependency | | ||
| | SHA256 (swift-crypto) | Cross-platform, industry standard | New dependency | | ||
| | FNV-1a (already in use) | No new deps, fast (~2 GB/s), in codebase | Non-cryptographic, 16-char hex, collision-prone at scale | | ||
|
|
There was a problem hiding this comment.
This "Options considered" table for checksums is in Markdown format, which deviates from the repository's TOON format convention for tabular data.
Suggested change
| | Option | Pros | Cons | | |
| | ----------------------- | --------------------------------------- | --------------------------------------------------------- | | |
| | SHA256 (CryptoKit) | Industry standard, 64-char hex | macOS-only; Linux needs `swift-crypto` dependency | | |
| | SHA256 (swift-crypto) | Cross-platform, industry standard | New dependency | | |
| | FNV-1a (already in use) | No new deps, fast (~2 GB/s), in codebase | Non-cryptographic, 16-char hex, collision-prone at scale | | |
| options[3]{option,pros,cons}: | |
| "SHA256 (CryptoKit)","Industry standard, 64-char hex","macOS-only; Linux needs `swift-crypto` dependency" | |
| "SHA256 (swift-crypto)","Cross-platform, industry standard","New dependency" | |
| "FNV-1a (already in use)","No new deps, fast (~2 GB/s), in codebase","Non-cryptographic, 16-char hex, collision-prone at scale" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+178
to
+186
| | Risk | Impact | Mitigation | | ||
| | --------------------------------- | ------ | ----------------------------------------------------- | | ||
| | FileWriter tracking overhead | Low | Only active when `--report` specified | | ||
| | FNV-1a collision risk | Low | Non-cryptographic but sufficient for change detection | | ||
| | exfig-action Phase 3 dependency | Medium | Action requires CLI release with `--report` first | | ||
| | Report format evolution | Low | `version` field in report for forward compat | | ||
| | `deleted` detection needs history | Low | Optional -- requires previous report at same path | | ||
| | Warning collector wiring | Low | Follow SharedThemeAttributesCollector actor pattern | | ||
| | run() refactor for result capture | Low | performExportWithResult() already exists | |
There was a problem hiding this comment.
The "Risks / Trade-offs" table is in Markdown format. To adhere to the repository's style guide, it should be converted to the TOON format.
Suggested change
| | Risk | Impact | Mitigation | | |
| | --------------------------------- | ------ | ----------------------------------------------------- | | |
| | FileWriter tracking overhead | Low | Only active when `--report` specified | | |
| | FNV-1a collision risk | Low | Non-cryptographic but sufficient for change detection | | |
| | exfig-action Phase 3 dependency | Medium | Action requires CLI release with `--report` first | | |
| | Report format evolution | Low | `version` field in report for forward compat | | |
| | `deleted` detection needs history | Low | Optional -- requires previous report at same path | | |
| | Warning collector wiring | Low | Follow SharedThemeAttributesCollector actor pattern | | |
| | run() refactor for result capture | Low | performExportWithResult() already exists | | |
| risks[7]{risk,impact,mitigation}: | |
| "FileWriter tracking overhead","Low","Only active when `--report` specified" | |
| "FNV-1a collision risk","Low","Non-cryptographic but sufficient for change detection" | |
| "exfig-action Phase 3 dependency","Medium","Action requires CLI release with `--report` first" | |
| "Report format evolution","Low","`version` field in report for forward compat" | |
| "`deleted` detection needs history","Low","Optional -- requires previous report at same path" | |
| "Warning collector wiring","Low","Follow SharedThemeAttributesCollector actor pattern" | |
| "run() refactor for result capture","Low","performExportWithResult() already exists" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
9
to
+12
| - Add `--report <path>` option to `ExportColors`, `ExportIcons`, `ExportImages`, `ExportTypography` | ||
| - New `ExportReport` struct (analogous to `BatchReport` but for a single command): command name, config path, timing, success/error, stats, collected warnings | ||
| - Reuse existing `ExportStats` from `BatchResult.swift` and `JSONCodec.encodePrettySorted()` from swift-yyjson | ||
| - New `ExportReport` struct (analogous to `BatchReport` but for a single command): version, command name, config path, timing, success/error, stats, collected warnings | ||
| - New `ReportStats: Encodable` struct with count fields only (colors, icons, images, typography) — analogous to `BatchReport.Stats`, NOT reusing `ExportStats` directly (it contains batch-only fields like `computedNodeHashes`, `granularCacheStats`) | ||
| - Serialize via `JSONCodec.encodePrettySorted()` from swift-yyjson |
There was a problem hiding this comment.
This list of Phase 1 changes is in Markdown list format. The repository style guide specifies that lists of items should use the TOON format.
Suggested change
| - Add `--report <path>` option to `ExportColors`, `ExportIcons`, `ExportImages`, `ExportTypography` | |
| - New `ExportReport` struct (analogous to `BatchReport` but for a single command): command name, config path, timing, success/error, stats, collected warnings | |
| - Reuse existing `ExportStats` from `BatchResult.swift` and `JSONCodec.encodePrettySorted()` from swift-yyjson | |
| - New `ExportReport` struct (analogous to `BatchReport` but for a single command): version, command name, config path, timing, success/error, stats, collected warnings | |
| - New `ReportStats: Encodable` struct with count fields only (colors, icons, images, typography) — analogous to `BatchReport.Stats`, NOT reusing `ExportStats` directly (it contains batch-only fields like `computedNodeHashes`, `granularCacheStats`) | |
| - Serialize via `JSONCodec.encodePrettySorted()` from swift-yyjson | |
| phase1_items[4]{item}: | |
| "Add `--report <path>` option to `ExportColors`, `ExportIcons`, `ExportImages`, `ExportTypography`" | |
| "New `ExportReport` struct (analogous to `BatchReport` but for a single command): version, command name, config path, timing, success/error, stats, collected warnings" | |
| "New `ReportStats: Encodable` struct with count fields only (colors, icons, images, typography) — analogous to `BatchReport.Stats`, NOT reusing `ExportStats` directly (it contains batch-only fields like `computedNodeHashes`, `granularCacheStats`)" | |
| "Serialize via `JSONCodec.encodePrettySorted()` from swift-yyjson" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+16
to
18
| - New `AssetManifest` struct tracking every generated file: path, action (`created`/`modified`/`unchanged`/`deleted`), FNV-1a content checksum, asset type | ||
| - Track file write status in `FileWriter` and attach manifest to `ExportReport` | ||
| - Enables: precise change tracking, PR diff comments, design drift detection |
There was a problem hiding this comment.
The list for Phase 2 is a Markdown list. Please convert it to a TOON table to follow the repository's format convention.
Suggested change
| - New `AssetManifest` struct tracking every generated file: path, action (`created`/`modified`/`unchanged`/`deleted`), FNV-1a content checksum, asset type | |
| - Track file write status in `FileWriter` and attach manifest to `ExportReport` | |
| - Enables: precise change tracking, PR diff comments, design drift detection | |
| phase2_items[3]{item}: | |
| "New `AssetManifest` struct tracking every generated file: path, action (`created`/`modified`/`unchanged`/`deleted`), FNV-1a content checksum, asset type" | |
| "Track file write status in `FileWriter` and attach manifest to `ExportReport`" | |
| "Enables: precise change tracking, PR diff comments, design drift detection" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+222
to
228
| #### Scenario: Written file has FNV-1a checksum | ||
|
|
||
| - **GIVEN** an export writes a file with known content | ||
| - **WHEN** the manifest entry is recorded | ||
| - **THEN** `checksum` SHALL be a 64-character lowercase hexadecimal string | ||
| - **AND** the value SHALL match the SHA256 hash of the written file content | ||
| - **THEN** `checksum` SHALL be a 16-character lowercase hexadecimal string | ||
| - **AND** the value SHALL match the FNV-1a hash of the written file content | ||
|
|
There was a problem hiding this comment.
This scenario is formatted as a Markdown list. Please convert it to a TOON table to adhere to the project's documentation standards.
Suggested change
| #### Scenario: Written file has FNV-1a checksum | |
| - **GIVEN** an export writes a file with known content | |
| - **WHEN** the manifest entry is recorded | |
| - **THEN** `checksum` SHALL be a 64-character lowercase hexadecimal string | |
| - **AND** the value SHALL match the SHA256 hash of the written file content | |
| - **THEN** `checksum` SHALL be a 16-character lowercase hexadecimal string | |
| - **AND** the value SHALL match the FNV-1a hash of the written file content | |
| scenario_checksum[4]{step}: | |
| "**GIVEN** an export writes a file with known content" | |
| "**WHEN** the manifest entry is recorded" | |
| "**THEN** `checksum` SHALL be a 16-character lowercase hexadecimal string" | |
| "**AND** the value SHALL match the FNV-1a hash of the written file content" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
237
to
+239
| - **GIVEN** an export that detects a file is unchanged | ||
| - **WHEN** the manifest entry is recorded | ||
| - **THEN** `checksum` SHALL equal the SHA256 of the existing file content | ||
| - **THEN** `checksum` SHALL equal the FNV-1a hash of the existing file content |
There was a problem hiding this comment.
This scenario is formatted as a Markdown list. Please convert it to a TOON table to adhere to the project's documentation standards.
Suggested change
| - **GIVEN** an export that detects a file is unchanged | |
| - **WHEN** the manifest entry is recorded | |
| - **THEN** `checksum` SHALL equal the SHA256 of the existing file content | |
| - **THEN** `checksum` SHALL equal the FNV-1a hash of the existing file content | |
| scenario_unchanged_checksum[3]{step}: | |
| "**GIVEN** an export that detects a file is unchanged" | |
| "**WHEN** the manifest entry is recorded" | |
| "**THEN** `checksum` SHALL equal the FNV-1a hash of the existing file content" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+247
to
+251
| #### Scenario: Initial report version | ||
|
|
||
| - **GIVEN** any export with `--report` | ||
| - **WHEN** the report is written | ||
| - **THEN** `"version"` SHALL be `1` |
There was a problem hiding this comment.
This new scenario for the report version is a Markdown list. It should be converted to a TOON table to follow the repository's formatting convention.
Suggested change
| #### Scenario: Initial report version | |
| - **GIVEN** any export with `--report` | |
| - **WHEN** the report is written | |
| - **THEN** `"version"` SHALL be `1` | |
| scenario_version[3]{step}: | |
| "**GIVEN** any export with `--report`" | |
| "**WHEN** the report is written" | |
| "**THEN** `\"version\"` SHALL be `1`" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
Comment on lines
+259
to
+264
| #### Scenario: Export produces no files | ||
|
|
||
| - **GIVEN** a valid config with no matching assets in Figma | ||
| - **WHEN** the export completes successfully with `--report` | ||
| - **THEN** `manifest.files` SHALL be an empty array `[]` | ||
| - **AND** `stats` SHALL reflect zero counts for the relevant asset type |
There was a problem hiding this comment.
This new scenario for handling zero-file exports is a Markdown list. Please convert it to a TOON table to align with the project's style guide.
Suggested change
| #### Scenario: Export produces no files | |
| - **GIVEN** a valid config with no matching assets in Figma | |
| - **WHEN** the export completes successfully with `--report` | |
| - **THEN** `manifest.files` SHALL be an empty array `[]` | |
| - **AND** `stats` SHALL reflect zero counts for the relevant asset type | |
| scenario_zero_files[4]{step}: | |
| "**GIVEN** a valid config with no matching assets in Figma" | |
| "**WHEN** the export completes successfully with `--report`" | |
| "**THEN** `manifest.files` SHALL be an empty array `[]`" | |
| "**AND** `stats` SHALL reflect zero counts for the relevant asset type" |
References
- The style guide requires using TOON for all tabular data and lists of items, instead of Markdown tables or lists. This is to reduce token usage and ensure a consistent format across documentation files. (link)
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
2 participants
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.
Key corrections after codebase validation:
replaced with new WarningCollector actor following SharedThemeAttributesCollector pattern
FNV1aHasher already available in codebase)
create new ReportStats: Encodable instead of reusing ExportStats
https://claude.ai/code/session_01M6Cw6Pcsy5L1KVkPZ1esu8