fix(db custom color importer) optimizing and fixing#498
Conversation
…lection reuse - Add checkExistingPrefix function to detect existing variable collections with different prefixes - Implement prefix conflict warning system that alerts users when importing with mismatched prefixes - Update getOrCreateCollections to reuse existing collections instead of creating duplicates - Modify setupDisplayModes to properly configure Light/Dark modes for Display collection - Change Theme collection to use single "Value" mode instead of Light/Dark modes - Add logic to remove extra modes from Theme collection to maintain single-mode structure - Prevent duplicate prefix naming in color family mode names - Update ImportMessage type to include optional customPrefix field - Enhance variable path generation to avoid prefix duplication in family names - Improve collection detection to find existing collections by suffix matching rather than exact name matching
…riable reuse logic - Update collection deletion to support prefixed collection names using endsWith matching - Add error handling with console warnings for collection and variable removal operations - Improve cleanup logic to handle Theme, Mode, and Colors collections with proper error catching - Refactor adaptive color variable creation to reuse existing variables across prefixes - Add pattern-based search for existing adaptive variables to prevent duplication - Implement conditional value setting for db-adaptive mode to avoid overwriting existing values - Add scopes configuration for display mode variables based on variable name patterns - Enhance code formatting and add clarifying comments for collection handling logic
- Add `isNewVariable` flag to track when adaptive color variables are newly created - Update db-adaptive mode assignment to apply to both new variables and existing variables without values - Extract `hasDbAdaptiveValue` check into a separate variable for improved readability - Refactor conditional logic to use `isNewVariable || !hasDbAdaptiveValue` for more comprehensive value assignment - Ensure adaptive variables receive proper mode values regardless of creation state
leape
changed the title
…ation - Add comprehensive documentation for the new prefix confirmation step in README - Document automatic prefix detection and manual adjustment capabilities - Include examples of prefix naming conventions (dibe, custom) - Explain importance of prefix consistency with Theme Builder exports - Remove unused `detectedPrefix` state variable from App component - Add `overflow-visible` class to main container for proper dialog rendering - Add horizontal padding and negative margin to prefix dialog stack for better spacing - Renumber subsequent sections in README (3→4, 4→5, 5→6) to accommodate new section - Improve UI layout and accessibility for prefix confirmation workflow
leape
changed the title
…unit tests after Amazon Q review - Add VariableScope type definition for type-safe Figma variable scopes - Create SCOPES constants (BACKGROUND, FOREGROUND, NONE) to eliminate magic strings - Add getScopesForMapping() helper function to centralize scope logic and remove duplication - Create MODE_NAMES constants for Light Mode, Dark Mode, and db-adaptive mode names - Add MESSAGES constants for success and error notifications - Implement ensurePrefixedFamily() helper to standardize prefix handling across codebase - Create standardized logging system (utils/logger.ts) with debug, info, warn, error, and success levels - Add context tags and visual sections to logging for improved debugging - Implement unit tests with Vitest for color utilities, logger, and configuration - Add vitest.config.ts and test documentation (tests/README.md) - Create IMPROVEMENTS.md documenting all type safety and logging enhancements - Update README.md and package.json with testing instructions and dependencies - Refactor existing code to use new constants and logging system throughout codebase
There was a problem hiding this comment.
Pull request overview
This PR enhances the DB Custom Color Importer plugin by adding prefix detection and confirmation UI, refactoring code for better type safety and maintainability, adding comprehensive unit tests, and implementing performance optimizations through parallel operations.
Changes:
- Added prefix detection and confirmation dialog in UI with automatic extraction from JSON color families and filenames
- Refactored code with centralized configuration constants (SCOPES, MODE_NAMES, MESSAGES), improved type safety, and standardized logging system
- Added 53 unit tests covering color utilities, configuration, and logger functionality, plus comprehensive testing documentation
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db-custom-color-importer/ui/src/App.tsx | Adds prefix detection/confirmation workflow with dialog UI and state management |
| packages/db-custom-color-importer/plugin/utils/variables.ts | Refactors variable creation with prefix support, parallel processing, and logging integration |
| packages/db-custom-color-importer/plugin/utils/logger.ts | New standardized logging system with levels, timestamps, and context tags |
| packages/db-custom-color-importer/plugin/utils/logger.test.ts | Comprehensive unit tests for logger functionality (24 tests) |
| packages/db-custom-color-importer/plugin/utils/color.test.ts | Unit tests for color conversion utilities (16 tests) |
| packages/db-custom-color-importer/plugin/utils/collections.ts | Enhances collection management with prefix support and improved mode setup |
| packages/db-custom-color-importer/plugin/utils/cleanup.ts | Optimizes deletion with parallel operations for variables and collections |
| packages/db-custom-color-importer/plugin/config.ts | Adds centralized constants and helper functions for scopes, modes, and messages |
| packages/db-custom-color-importer/plugin/config.test.ts | Unit tests for configuration helpers (13 tests) |
| packages/db-custom-color-importer/plugin/code.ts | Adds prefix checking message handler for existing collections |
| packages/db-custom-color-importer/plugin/types.ts | Adds VariableScope type and customPrefix to ImportMessage |
| packages/db-custom-color-importer/plugin/vitest.config.ts | Configures Vitest test framework |
| packages/db-custom-color-importer/plugin/package.json | Adds test scripts and vitest dependency |
| packages/db-custom-color-importer/package.json | Adds test scripts at package root level |
| packages/db-custom-color-importer/README.md | Updates documentation with prefix confirmation workflow details |
| packages/db-custom-color-importer/TESTING.md | New comprehensive testing guide with examples and best practices |
| packages/db-custom-color-importer/IMPROVEMENTS.md | New detailed documentation of refactoring and optimization changes |
| packages/db-custom-color-importer/plugin/tests/README.md | Test structure documentation for the plugin directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const lightModeId = displayCol.modes.find( | ||
| (m) => m.name === "Light Mode", | ||
| (m) => m.name === MODE_NAMES.LIGHT, | ||
| )!.modeId; | ||
| const darkModeId = displayCol.modes.find( | ||
| (m) => m.name === "Dark Mode", | ||
| (m) => m.name === MODE_NAMES.DARK, | ||
| )!.modeId; |
There was a problem hiding this comment.
Missing null checks: The non-null assertion operators (!) are used on lines 103 and 106, but if MODE_NAMES.LIGHT or MODE_NAMES.DARK are not found in displayCol.modes, these will throw runtime errors. While the modes are created just above, it's safer to add proper null checks or error handling.
| prefixOriginal = matchWithMiddle[2].trim(); | ||
| } else if (matchDirect && matchDirect[1]) { | ||
| // Direct match with known prefix - ignore it | ||
| prefixOriginal = ""; | ||
| } | ||
|
|
There was a problem hiding this comment.
Regex logic issue: The function returns an empty string when matchDirect succeeds, but this doesn't align with the comment on line 115. If the direct match succeeds (e.g., "DB Theme-figma"), the intent seems to be to ignore the known prefix and return empty. However, this means files like "DB Theme-figma.json" will not extract any prefix, which might not be the desired behavior. Consider clarifying the logic or documenting why direct matches should result in an empty prefix.
| prefixOriginal = matchWithMiddle[2].trim(); | |
| } else if (matchDirect && matchDirect[1]) { | |
| // Direct match with known prefix - ignore it | |
| prefixOriginal = ""; | |
| } | |
| // Example: "DB SomePrefix Theme-figma.json" -> "SomePrefix" | |
| prefixOriginal = matchWithMiddle[2].trim(); | |
| } else if (matchDirect && matchDirect[1]) { | |
| // Example: "DB Theme-figma.json" | |
| // Direct match with known prefix only - intentionally do not extract a custom prefix | |
| return ""; | |
| } | |
| if (!prefixOriginal) { | |
| // No prefix segment found to normalize | |
| return ""; | |
| } |
| const nameWithoutExt = filename.replace(/\.json$/i, ""); | ||
|
|
||
| // Known prefixes at the start that should be ignored | ||
| const knownPrefixes = ["DB", "Whitelabel", "S-Bahn", "sab", "SAB"]; |
There was a problem hiding this comment.
Inconsistent knownPrefixes array: The knownPrefixes array in the UI (line 95) includes "SAB" in addition to "sab", while the plugin's knownPrefixes array (variables.ts:254) only has "sab". This inconsistency could lead to different prefix extraction behavior between the UI preview and the actual plugin import. Consider synchronizing these arrays or extracting them to a shared constant.
| const knownPrefixes = ["DB", "Whitelabel", "S-Bahn", "sab", "SAB"]; | |
| const knownPrefixes = ["DB", "Whitelabel", "S-Bahn", "sab"]; |
| const extractPrefixFromJson = (data: any): string | null => { | ||
| // Extract prefix from color family names | ||
| // Example: "dibe-br-color-01" -> "dibe" | ||
| const colors = data?.colors || {}; | ||
| const colorFamilies = Object.keys(colors); | ||
|
|
||
| if (colorFamilies.length === 0) return null; | ||
|
|
||
| // Extract prefixes from all color family names | ||
| // Expected format: "prefix-something-color-number" or "prefix-color-number" | ||
| const prefixes = colorFamilies | ||
| .map((familyName) => { | ||
| // Extract the first part before the first dash | ||
| const match = familyName.match(/^([a-zA-Z0-9]+)(?:-|$)/); | ||
| return match ? match[1].toLowerCase() : null; | ||
| }) | ||
| .filter((p) => p !== null); | ||
|
|
||
| if (prefixes.length === 0) return null; | ||
|
|
||
| // Check if all prefixes are the same | ||
| const firstPrefix = prefixes[0]; | ||
| const allSame = prefixes.every((p) => p === firstPrefix); | ||
|
|
||
| return allSame ? firstPrefix : null; | ||
| }; | ||
|
|
||
| const extractPrefixFromFilename = (filename: string): string => { | ||
| if (!filename) return ""; | ||
|
|
||
| // Remove .json extension | ||
| const nameWithoutExt = filename.replace(/\.json$/i, ""); | ||
|
|
||
| // Known prefixes at the start that should be ignored | ||
| const knownPrefixes = ["DB", "Whitelabel", "S-Bahn", "sab", "SAB"]; | ||
|
|
||
| // Try to match pattern: [KnownPrefix][-\s][Something]Theme-figma | ||
| // or: [KnownPrefix][-\s]Theme-figma | ||
| const regexWithMiddle = new RegExp( | ||
| `^(${knownPrefixes.join("|")})[-\\s]+(.+?)Theme-figma`, | ||
| "i", | ||
| ); | ||
| const regexDirect = new RegExp( | ||
| `^(${knownPrefixes.join("|")})[-\\s]+Theme-figma`, | ||
| "i", | ||
| ); | ||
|
|
||
| const matchWithMiddle = nameWithoutExt.match(regexWithMiddle); | ||
| const matchDirect = nameWithoutExt.match(regexDirect); | ||
|
|
||
| let prefixOriginal = ""; | ||
| if (matchWithMiddle && matchWithMiddle[2]) { | ||
| prefixOriginal = matchWithMiddle[2].trim(); | ||
| } else if (matchDirect && matchDirect[1]) { | ||
| // Direct match with known prefix - ignore it | ||
| prefixOriginal = ""; | ||
| } | ||
|
|
||
| // Remove special characters and convert to lowercase | ||
| const prefix = prefixOriginal.replace(/[^a-zA-Z0-9]/g, "").toLowerCase(); | ||
| return prefix; | ||
| }; |
There was a problem hiding this comment.
Missing test coverage: The extractPrefixFromJson and extractPrefixFromFilename functions contain complex logic with multiple edge cases (e.g., different filename patterns, prefix consistency checks), but there are no test files for the UI components. Given that the plugin directory has comprehensive test coverage, consider adding tests for these critical prefix extraction functions to ensure they work correctly across different scenarios.
| **Performance Improvements:** | ||
|
|
||
| - **Variable deletion:** ~10x faster for collections with many variables | ||
| - **Adaptive variable creation:** ~3-5x faster (depends on number of mappings) | ||
| - **Overall import time:** Reduced by 40-60% for large color sets |
There was a problem hiding this comment.
Unverified performance claims: The IMPROVEMENTS.md document claims specific performance improvements (10x faster variable deletion, 3-5x faster adaptive variable creation, 40-60% faster overall), but these metrics are not backed by benchmarks or measurements in the codebase. Consider adding performance benchmarks to verify these claims or qualifying them as estimated improvements.
| file. Your new colors with prefix "{prefixInput}" will be added to | ||
| the existing collections. This means both color families will be | ||
| available in the same collections. |
There was a problem hiding this comment.
Misleading warning message: The warning message states that "both color families will be available in the same collections," but this may not accurately describe what happens. Based on the code logic in getOrCreateCollections (collections.ts), if collections already exist, they are reused regardless of prefix. The warning should clarify whether existing variables will be updated/merged or if this could lead to naming conflicts.
| file. Your new colors with prefix "{prefixInput}" will be added to | |
| the existing collections. This means both color families will be | |
| available in the same collections. | |
| file and will be reused. Your new colors with prefix "{prefixInput}" | |
| will be added to these existing collections. This means color | |
| variables from both prefixes will coexist in the same collections | |
| and may update existing variables or lead to naming conflicts. |
| let createdCount = 0; | ||
| let linkedCount = 0; | ||
|
|
||
| // Process all mappings in parallel | ||
| const processingPromises = MAPPINGS.map(async (m) => { | ||
| // First, try to find existing adaptive variable (with any prefix) | ||
| const adaptivePattern = `-adaptive/${m.name}`; | ||
| let v: Variable | undefined; | ||
| let isNewVariable = false; | ||
|
|
||
| // Search for existing variable ending with "-adaptive/..." | ||
| for (const [varName, variable] of varMap.entries()) { | ||
| if (varName.endsWith(adaptivePattern)) { | ||
| v = variable; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If not found, create new one with current prefix | ||
| if (!v) { | ||
| const colorVarPath = `${prefix}-adaptive/${m.name}`; | ||
| v = figma.variables.createVariable(colorVarPath, colorCol, "COLOR"); | ||
| isNewVariable = true; | ||
| createdCount++; | ||
| } |
There was a problem hiding this comment.
Race condition: The createdCount variable is being modified by multiple async operations running in parallel. Since JavaScript doesn't have atomic increments, this can lead to incorrect counts when multiple promises update createdCount simultaneously. The same issue exists for linkedCount on line 187. Consider using a different approach such as filtering the results after Promise.all or using a thread-safe counter pattern.
| if (existingCollections.length > 0) { | ||
| // Extract prefix from first existing collection | ||
| const firstCollection = existingCollections[0]; | ||
| const existingPrefix = firstCollection.name.split("-")[0].toLowerCase(); |
There was a problem hiding this comment.
Potential bug with prefix extraction: This regex will fail to extract a prefix from collection names that contain multiple dashes before the suffix. For example, a collection named "my-custom-Theme" would incorrectly extract "my" as the prefix when it should be "my-custom". Consider splitting from the end using the suffix instead of splitting from the beginning, or using a more robust pattern like c.name.slice(0, c.name.lastIndexOf('-')).
| const existingPrefix = firstCollection.name.split("-")[0].toLowerCase(); | |
| const name = firstCollection.name; | |
| const lastDashIndex = name.lastIndexOf("-"); | |
| const existingPrefix = (lastDashIndex !== -1 ? name.slice(0, lastDashIndex) : name).toLowerCase(); |
| const dbAdaptiveModeId = colorCol.modes.find( | ||
| (m) => m.name === "db-adaptive", | ||
| (m) => m.name === MODE_NAMES.DB_ADAPTIVE, | ||
| )!.modeId; |
There was a problem hiding this comment.
Missing null check: The non-null assertion operator (!) is used here, but if MODE_NAMES.DB_ADAPTIVE is not found in colorCol.modes, this will throw a runtime error. While this scenario may be unlikely due to the mode being renamed just above, it's safer to add error handling or a proper check before accessing modeId.
…selection, and smart variable grouping - Add color precision handling with full RGB precision and alpha rounding to prevent hex value distortion (e.g., #001110 → #00120F) - Implement flexible prefix selection dialog showing all detected prefixes from variable names and filenames - Add automatic variable grouping with nested slash-separated paths (e.g., db-poi-db-services → db-poi/db-services) - Implement smart variable reuse by detecting existing variables and updating modes instead of creating duplicates - Separate collection prefix from variable prefix for cleaner organization - Add processing screen with loading animation to prevent accidental multiple imports - Add comprehensive CHANGELOG.md documenting all changes and technical details - Update README.md with improved prefix handling and grouping documentation - Add color-precision.test.ts with test cases using real RITheme JSON values - Enhance color.test.ts with additional precision test cases - Add RITheme-figma-custom-colors.json sample file for testing and reference - Update types.ts, collections.ts, color.ts, variables.ts, and App.tsx to support new features - Improve user guidance with clearer explanation of variable grouping functionality
Proposed changes
Initial setup DB Icon Studio
Types of changes
Further comments
–