feat: add more handlers #41
Conversation
|
WalkthroughThis pull request introduces extensive refactoring and restructuring across multiple modules. Core adapter handler functions and older implementations are removed or renamed, with new handlers for base and unsupported cases being added. The CLI has been completely revamped to use a simplified, yargs-parser–based approach with delegated command resolution and execution. In addition, test suites and TypeScript configuration files are expanded and improved, dependencies are updated (e.g., replacing vitest-fetch-mock with msw), and ESLint configurations are simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Application
participant CU as CLI Utils
U->>CLI: Provide command-line arguments
CLI->>CU: resolveCommand(args)
CU-->>CLI: Return command (e.g., "version", "generate", "help")
CLI->>CU: runCommand(command, args)
CU-->>CLI: Execute command logic (dynamic import and execution)
CLI->>U: Output result or error
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/adapters/src/types.ts (1)
1-7:⚠️ Potential issueUnused import detected
There's an unused import
EmojiGroupfrom "@mojis/internal-utils" as identified by the pipeline failure.-import type { - EmojiGroup, - EmojiMetadata, - WriteCacheOptions, -} from "@mojis/internal-utils"; +import type { + EmojiMetadata, + WriteCacheOptions, +} from "@mojis/internal-utils";🧰 Tools
🪛 GitHub Check: build
[failure] 3-3:
'EmojiMetadata' is defined but never used
[failure] 2-2:
'EmojiGroup' is defined but never used🪛 GitHub Actions: CI
[error] 2-3: 'EmojiGroup' is defined but never used unused-imports/no-unused-imports
🧹 Nitpick comments (3)
packages/adapters/src/handlers/base/variation.ts (1)
25-49: Consider using a stricter enumeration for the style field.Right now, the style is derived by removing
"style"and trimming the result. Using a predefined enum or constant would better communicate intended values and reduce risk if extra content slips into the string.packages/adapters/src/utils.ts (1)
111-127: Be mindful of property conflicts when merging contexts.Using
Object.assign({}, ctx, extraContext)will overwrite any matching fields inctxwith those fromextraContext. This is convenient, but ensure it’s the desired behavior. Consider shallow-copying only new fields or preventing collisions if needed.packages/adapters/src/index.ts (1)
1-99: Handle partial failures in parallel data fetching.Currently, if any URL fetch fails in
await Promise.all(dataRequests), the entire handler will throw. If partial success is acceptable, consider implementing a fallback or ignoring failed fetches for a robust experience. Otherwise, ensuring the user is aware that a single 404 or network error can halt the entire workflow is important.Do you want me to propose a resilient approach that handles partial failures gracefully?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/adapters/src/handler.ts(0 hunks)packages/adapters/src/handlers.ts(2 hunks)packages/adapters/src/handlers/base/metadata.ts(1 hunks)packages/adapters/src/handlers/base/variation.ts(1 hunks)packages/adapters/src/handlers/modern/variation.ts(0 hunks)packages/adapters/src/index.ts(1 hunks)packages/adapters/src/types.ts(1 hunks)packages/adapters/src/utils.ts(3 hunks)packages/adapters/test/index.test.ts(0 hunks)packages/adapters/test/utils.test.ts(3 hunks)
💤 Files with no reviewable changes (3)
- packages/adapters/test/index.test.ts
- packages/adapters/src/handlers/modern/variation.ts
- packages/adapters/src/handler.ts
🧰 Additional context used
🪛 GitHub Actions: CI
packages/adapters/src/types.ts
[error] 2-3: 'EmojiGroup' is defined but never used unused-imports/no-unused-imports
🔇 Additional comments (10)
packages/adapters/src/handlers/base/metadata.ts (1)
20-20: Function rename aligns with refactoring strategyThe renaming from
modernMetadataHandlertobaseMetadataHandleris consistent with the refactoring goals mentioned in the PR objectives, moving away from the "modern" prefix to the more appropriate "base" prefix.packages/adapters/src/handlers.ts (1)
1-2: Handler imports and references updated consistentlyThe imports and references have been correctly updated to align with the renaming strategy (from "modern" to "base" handlers). This maintains consistency across the codebase.
Also applies to: 7-7, 16-16
packages/adapters/src/types.ts (1)
7-7: Import path updated correctlyThe import path has been updated from
"./_handlers"to"./handlers", reflecting the reorganization of the module structure.packages/adapters/test/utils.test.ts (3)
139-148: Test case added for array of UrlWithCacheGood addition of a test case for when a UrlBuilder returns an array of UrlWithCache objects. This improves test coverage for the handler URL resolution functionality.
161-209: Comprehensive tests added for new buildContext functionThe tests for the new
buildContextfunction are thorough and cover multiple scenarios:
- Merging context with additional properties
- Overriding existing properties
- Handling empty extra context
This ensures the function works correctly in all expected use cases.
3-3: Import statement updated to include new functionThe import statement has been correctly updated to include the new
buildContextfunction from the utils file.packages/adapters/src/handlers/base/variation.ts (2)
7-9: Validateunicode_versionand handle potential parsing errors.Ensure that
ctx.unicode_versionis always a properly formatted semantic version string. If it’s incorrectly formatted or undefined,semver.gtemight throw an error. Consider adding a validation step or fallback in case the string is invalid.
41-45: Reevaluate attaching a fixed property of ["Emoji"].Every variation object is assigned
property: ["Emoji"]even whentype === "text". Verify that this is intentional and consistent with the rest of the code.packages/adapters/src/utils.ts (2)
32-41: ExportcreateUrlWithCacheif it’s used externally.The function is currently declared but not exported. Confirm it’s intentionally internal. If used by other modules, consider making it publicly available to avoid reimplementation.
62-63: Ensure the return type aligns with runtime behavior.The docstring specifies a
Promise<UrlWithCache[]>, which matches the logic. Just confirm that all cases (including builder functions that return a single object) consistently resolve to an array.
github-actions
bot
added
pkg: internal-utils
…taHandler and add baseMetadataHandler
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/adapters/src/handlers/metadata.ts (2)
20-21: Consider addressing the TODO comment.The TODO comment indicates that
baseMetadataHandlermight need a better name. Since this PR is refactoring handlers, it would be good to resolve this naming issue now rather than leaving it for future work.
129-148: Good addition of fallback handler for unsupported emoji versions.The
notSupportedMetadataHandlerprovides a clean fallback for emoji versions that don't have emoji-test.txt files (versions 1.0, 2.0, and 3.0). This prevents errors when processing these versions and returns empty collections instead.A few suggestions:
- The TODO comment on line 129 should be addressed before merging
- Consider adding a brief comment explaining why this returns empty collections
- // TODO: find a better name + // Handler for emoji versions that don't have emoji-test.txt files export const notSupportedMetadataHandler = defineAdapterHandler({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/adapters/src/handlers.ts(1 hunks)packages/adapters/src/handlers/metadata.ts(3 hunks)packages/adapters/src/handlers/pre-alignment/metadata.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/adapters/src/handlers/pre-alignment/metadata.ts
🔇 Additional comments (4)
packages/adapters/src/handlers/metadata.ts (1)
3-3: Import path updated correctly.The import path for
defineAdapterHandlerhas been changed from a relative path to a path higher in the hierarchy, reflecting the reorganization mentioned in the PR.packages/adapters/src/handlers.ts (3)
1-2: Import paths updated correctly for the refactored handlers.The imports correctly reflect the new organization with base handlers instead of modern handlers, which aligns with the PR's refactoring goals.
6-7: Handler array updated to use the new handler implementations.The
METADATA_HANDLERSarray now uses the renamedbaseMetadataHandlerand adds the newnotSupportedMetadataHandler, which provides better support for older emoji versions.
15-15: Variation handler updated to use base implementation.The
baseVariationHandlerreplaces the previous modern implementation, aligning with the overall refactoring approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/adapters/src/handlers/metadata.ts (3)
129-149: Well-implemented fallback handler for unsupported versions.The implementation of
notSupportedMetadataHandleris clean and provides a consistent interface for versions without emoji-test files. This prevents errors when processing older emoji versions.Consider adding a debug log or warning in the
transformoroutputmethod to indicate when this fallback is used, which could help with troubleshooting.output() { + console.debug(`Using fallback metadata handler for unsupported emoji version`); return { groups: [], emojis: {}, }; },
80-84: Enhance error message with additional context.When parsing fails, providing more context in error messages can help with debugging.
if (baseHexcode == null || trailingLine == null) { - throw new Error(`invalid line: ${line}`); + throw new Error(`Invalid line format (missing semicolon separator): ${line}`); }
86-90: Improve error message clarity.Similar to the previous comment, this error message could be more descriptive.
if (baseQualifier == null || comment == null) { - throw new Error(`invalid line: ${line}`); + throw new Error(`Invalid line format (missing # separator): ${line}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/adapters/src/handlers/metadata.ts(3 hunks)
🔇 Additional comments (4)
packages/adapters/src/handlers/metadata.ts (4)
3-3: Import path adjustment looks good.The path adjustment from "../../define" to "../define" reflects the restructuring mentioned in the PR objectives.
16-18: Better documentation on disallowed emoji versions.The expanded comments clearly explain why these emoji versions are skipped and hint at an alternative solution through HTML page generation.
21-22: Consider resolving the TODO before merging.The TODO comment suggests this name might not be final. Since this appears to be part of your public API, it's best to resolve naming questions before releasing to avoid future breaking changes.
If this is a public API component that other code depends on, consider finalizing the name now rather than renaming it in a future PR.
25-28: Good explanation of fallback mechanism.The added comments clearly explain why certain versions are skipped and reference the fallback handler, which improves code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/adapters/src/handlers/metadata.ts (2)
21-28: Better naming needed for the handlerAs noted in the TODO comment,
baseMetadataHandlercould use a better name. Consider a more descriptive name that indicates its purpose or behavior, such asstandardMetadataHandlerorcompatibleVersionsMetadataHandler.- // TODO: give this a better name - export const baseMetadataHandler = defineAdapterHandler({ + export const standardMetadataHandler = defineAdapterHandler({
139-159: Well-structured fallback handlerThe
notSupportedMetadataHandleris a good addition that gracefully handles disallowed emoji versions instead of failing. This improves robustness.Two suggestions:
- Consider adding a warning or log about using unsupported versions
- Document any limitations of the empty return value for consumers
output() { + // Note: This returns empty structures for unsupported versions. + // Consumers should check if groups/emojis are empty to detect unsupported versions. return { groups: [], emojis: {}, }; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/adapters/src/handlers/metadata.ts(5 hunks)packages/cli/src/cli.ts(1 hunks)packages/internal-utils/src/predicates.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/cli.ts
🔇 Additional comments (4)
packages/internal-utils/src/predicates.ts (1)
32-51: Well-implemented version comparison utilityThe new
isBeforefunction is a clean and well-documented extension to the existing version comparison utilities. It follows the same pattern asisBeforeAlignmentbut with the added flexibility of specifying a target version.The implementation properly:
- Handles null/undefined versions
- Uses semver.coerce to handle non-standard version strings
- Includes comprehensive JSDoc documentation
packages/adapters/src/handlers/metadata.ts (3)
2-3: Clean import updatesThe updated imports correctly reflect the new structure, importing the new
isBeforefunction and updating the path fordefineAdapterHandler.
95-106: Good version-aware parsing logicThe conditional handling based on emoji version is well-implemented. Using the new
isBeforefunction makes the code more readable than direct version comparison would have been.
121-122: Improved metadata extractionThe metadata extraction now correctly handles both emoji and unicode versions, making good use of utility functions.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/adapters/src/index.ts (2)
35-62: Parser implementation could be more maintainableThe parser implementation currently only specifically handles the "generic" parser and throws an error for other built-in parsers. Consider implementing a more scalable approach.
- if (isBuiltinParser(handler.parser)) { - if (handler.parser === "generic") { - return genericParse(data, { - separator: handler.parserOptions?.separator ?? ";", - commentPrefix: handler.parserOptions?.commentPrefix ?? "#", - defaultProperty: handler.parserOptions?.defaultProperty ?? "", - propertyMap: handler.parserOptions?.propertyMap ?? {}, - }); - } - - throw new Error(`Parser "${handler.parser}" is not implemented.`); - } + if (isBuiltinParser(handler.parser)) { + const parsers = { + generic: () => genericParse(data, { + separator: handler.parserOptions?.separator ?? ";", + commentPrefix: handler.parserOptions?.commentPrefix ?? "#", + defaultProperty: handler.parserOptions?.defaultProperty ?? "", + propertyMap: handler.parserOptions?.propertyMap ?? {}, + }) + // Add more parsers here + }; + + const parserFn = parsers[handler.parser]; + if (!parserFn) { + throw new Error(`Parser "${handler.parser}" is not implemented.`); + } + + return parserFn(); + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-48: packages/adapters/src/index.ts#L35-L48
Added lines #L35 - L48 were not covered by tests
[warning] 50-51: packages/adapters/src/index.ts#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 53-62: packages/adapters/src/index.ts#L53-L62
Added lines #L53 - L62 were not covered by tests
100-101: Duplicated error messageThis error message is identical to the one on lines 19-21, but it will only be reached if all handlers were found but none were executed.
- throw new Error(`No handler found for type: ${type}`); + throw new Error(`No suitable handler found for type: ${type}`);This provides a clearer distinction between "no handlers exist" (first error) and "handlers exist but none were applicable" (second error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/adapters/src/index.ts(1 hunks)packages/internal-utils/src/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/internal-utils/src/index.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/adapters/src/index.ts
[warning] 2-5: packages/adapters/src/index.ts#L2-L5
Added lines #L2 - L5 were not covered by tests
[warning] 9-9: packages/adapters/src/index.ts#L9
Added line #L9 was not covered by tests
[warning] 14-17: packages/adapters/src/index.ts#L14-L17
Added lines #L14 - L17 were not covered by tests
[warning] 19-21: packages/adapters/src/index.ts#L19-L21
Added lines #L19 - L21 were not covered by tests
[warning] 23-23: packages/adapters/src/index.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: packages/adapters/src/index.ts#L25
Added line #L25 was not covered by tests
[warning] 27-29: packages/adapters/src/index.ts#L27-L29
Added lines #L27 - L29 were not covered by tests
[warning] 32-32: packages/adapters/src/index.ts#L32
Added line #L32 was not covered by tests
[warning] 35-48: packages/adapters/src/index.ts#L35-L48
Added lines #L35 - L48 were not covered by tests
[warning] 50-51: packages/adapters/src/index.ts#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 53-62: packages/adapters/src/index.ts#L53-L62
Added lines #L53 - L62 were not covered by tests
🔇 Additional comments (4)
packages/adapters/src/index.ts (4)
1-7: Exports are now more selective and focusedThe new exports are more specific by directly exporting only the necessary types (
AdapterContextandAdapterHandlerType) rather than using a wildcard export from the handler file. This is a better practice that provides clearer API boundaries.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2-5: packages/adapters/src/index.ts#L2-L5
Added lines #L2 - L5 were not covered by tests
9-16: Well-structured function signature with proper genericsThe function signature is well-designed with three generic parameters that provide type safety while allowing flexibility:
TAdapterHandlerTypeconstrains the handler typeTContextensures the context extends the base adapter contextTExtraContextallows for additional context propertiesThis enables strong typing throughout the function implementation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-9: packages/adapters/src/index.ts#L9
Added line #L9 was not covered by tests
17-21: Early validation prevents unnecessary processingGood practice to validate the existence of handlers for the requested type early in the function execution, which prevents unnecessary processing and provides clear error messages.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-21: packages/adapters/src/index.ts#L19-L21
Added lines #L19 - L21 were not covered by tests
1-101:❓ Verification inconclusive
Test coverage is missing for this critical function
The static analysis indicates that many parts of this function aren't covered by tests. Since this is a core function for the adapter system, it should have comprehensive test coverage.
🏁 Script executed:
#!/bin/bash # Description: Find existing tests for runAdapterHandler and analyze coverage gaps echo "Looking for tests that cover runAdapterHandler..." rg -l "runAdapterHandler" --type test packages/ echo "Looking for any mock or test implementations of adapter handlers..." rg -l "ALL_HANDLERS" --type test packages/Length of output: 402
Critical: Verify and Enhance Test Coverage for runAdapterHandler
Our initial automated search did not reliably reveal any tests exercising the
runAdapterHandlerfunction or its associated adapter handler logic. Given that this function is a core part of the adapter system, it's essential to ensure all execution paths are covered. Please manually verify if unit tests exist for these scenarios and, if not, add tests that cover:
- The case where no handler is found for the given type.
- Branches where
shouldExecutereturns false.- The different flows for transforming, aggregating, and outputting data.
- Error handling when using both builtin and custom parsers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2-5: packages/adapters/src/index.ts#L2-L5
Added lines #L2 - L5 were not covered by tests
[warning] 9-9: packages/adapters/src/index.ts#L9
Added line #L9 was not covered by tests
[warning] 14-17: packages/adapters/src/index.ts#L14-L17
Added lines #L14 - L17 were not covered by tests
[warning] 19-21: packages/adapters/src/index.ts#L19-L21
Added lines #L19 - L21 were not covered by tests
[warning] 23-23: packages/adapters/src/index.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: packages/adapters/src/index.ts#L25
Added line #L25 was not covered by tests
[warning] 27-29: packages/adapters/src/index.ts#L27-L29
Added lines #L27 - L29 were not covered by tests
[warning] 32-32: packages/adapters/src/index.ts#L32
Added line #L32 was not covered by tests
[warning] 35-48: packages/adapters/src/index.ts#L35-L48
Added lines #L35 - L48 were not covered by tests
[warning] 50-51: packages/adapters/src/index.ts#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 53-62: packages/adapters/src/index.ts#L53-L62
Added lines #L53 - L62 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/adapters/tsconfig.test.json(1 hunks)packages/cli/tsconfig.test.json(1 hunks)packages/internal-utils/tsconfig.test.json(1 hunks)packages/parsers/tsconfig.test.json(1 hunks)vitest.config.ts(5 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/adapters/tsconfig.test.json
- packages/internal-utils/tsconfig.test.json
- packages/cli/tsconfig.test.json
- packages/parsers/tsconfig.test.json
🔇 Additional comments (1)
vitest.config.ts (1)
35-38: Good improvement to test configuration structureThe changes to use
checker: "tsc"and dedicatedtsconfig.test.jsonfiles create a cleaner separation between test and production configurations. This is a good practice that improves maintainability by having dedicated TypeScript configurations for tests.Also applies to: 53-56, 70-73, 87-90
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/internal-utils/test/versions.test.ts (1)
88-115: “toSemverCompatible” utility tests are robust.
Notice that prerelease/build metadata is dropped. If preserving that data is needed for future use cases, consider extending the function to handle it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/internal-utils/test/cache.test.ts(6 hunks)packages/internal-utils/test/versions.test.ts(5 hunks)test/msw-utils/msw.ts(1 hunks)test/setup/msw.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
test/setup/msw.ts
[failure] 4-4: packages/internal-utils/test/cache.test.ts
TypeError: Cannot read properties of undefined (reading 'listen')
❯ test/setup/msw.ts:4:28
[failure] 5-5: packages/internal-utils/test/cache.test.ts
TypeError: Cannot read properties of undefined (reading 'close')
❯ test/setup/msw.ts:5:27
[failure] 4-4: packages/internal-utils/test/hexcode.test.ts
TypeError: Cannot read properties of undefined (reading 'listen')
❯ test/setup/msw.ts:4:28
[failure] 5-5: packages/internal-utils/test/hexcode.test.ts
TypeError: Cannot read properties of undefined (reading 'close')
❯ test/setup/msw.ts:5:27
[failure] 4-4: packages/internal-utils/test/predicates.test.ts
TypeError: Cannot read properties of undefined (reading 'listen')
❯ test/setup/msw.ts:4:28
[failure] 5-5: packages/internal-utils/test/predicates.test.ts
TypeError: Cannot read properties of undefined (reading 'close')
❯ test/setup/msw.ts:5:27
[failure] 4-4: packages/internal-utils/test/schemas.test.ts
TypeError: Cannot read properties of undefined (reading 'listen')
❯ test/setup/msw.ts:4:28
[failure] 5-5: packages/internal-utils/test/schemas.test.ts
TypeError: Cannot read properties of undefined (reading 'close')
❯ test/setup/msw.ts:5:27
[failure] 4-4: packages/internal-utils/test/shortcodes.test.ts
TypeError: Cannot read properties of undefined (reading 'listen')
❯ test/setup/msw.ts:4:28
[failure] 5-5: packages/internal-utils/test/shortcodes.test.ts
TypeError: Cannot read properties of undefined (reading 'close')
❯ test/setup/msw.ts:5:27
🪛 GitHub Actions: CI
test/setup/msw.ts
[error] 4-4: TypeError: Cannot read properties of undefined (reading 'listen')
[error] 5-5: TypeError: Cannot read properties of undefined (reading 'close')
🔇 Additional comments (20)
packages/internal-utils/test/cache.test.ts (6)
1-7: LGTM: Import and type updates for MSW integrationThe changes to imports look good. You've correctly:
- Added the
CacheMetatype import- Updated to import
HttpResponsefrom MSW- Added the
mockFetchimport from the new MSW utils- Added
readCacheMetato the imports from cacheThese changes properly support the transition from fetch-mock to MSW for testing.
208-235: LGTM: Well-structured tests for cache expiration scenariosThese new tests thoroughly verify important edge cases:
- Correctly handles non-existent cache metadata
- Properly handles expired cache entries
- Verifies that expired cache files are deleted
The test structure is clear and includes appropriate setup and verification steps.
238-260: LGTM: Comprehensive testing for the newreadCacheMetafunctionGood implementation of tests for the new
readCacheMetafunction. The tests:
- Verify successful metadata reading
- Check the behavior when the metadata file doesn't exist
This provides complete coverage for the new functionality.
288-290: LGTM: Updated mock implementation with MSWThe update from the previous mocking approach to use
mockFetchandHttpResponsefrom MSW is well-implemented and consistent with modern testing practices.
305-307: LGTM: Consistent usage of MSW for error testingThe error scenario test has been properly updated to use MSW's
HttpResponsefor creating a 404 response. This maintains testing consistency across the file.
323-325: LGTM: Updated parser test with MSWThe custom parser test has been correctly updated to use MSW for response mocking.
test/msw-utils/msw.ts (4)
6-18: LGTM: Well-designed helper functions for HTTP method handlingGood implementation of helper functions that:
- Define appropriate types for HTTP methods
- Normalize method names to lowercase
- Parse endpoint patterns in a format-aware way
The pattern parsing implementation is clean and handles the case where URLs might contain spaces.
20-47: LGTM: Flexible mockFetch implementation with proper type overloadsExcellent implementation of the
mockFetchfunction with:
- Clear function overloads for different use cases
- Support for both single and batch mock registrations
- Proper error handling for invalid arguments
- Good use of type safety with template literal types
The implementation is robust and type-safe.
49-68: LGTM: Comprehensive response helper functionsGreat collection of response helpers that:
- Cover all common HTTP status codes
- Provide sensible defaults for error messages
- Set appropriate Content-Type headers
- Include both success and error scenarios
These will simplify test setup considerably.
70-70: LGTM: Clean re-exportsClean re-export of the necessary MSW components to provide a unified API for tests.
packages/internal-utils/test/versions.test.ts (10)
3-5: Great use of MSW for mock responses.
These imports cleanly pull inHttpResponseandmockFetchto facilitate test mocks.
8-10: New import additions look good.
Bringing inextractUnicodeVersionandgetAllEmojiVersionshelps enhance test coverage.
18-18: Thorough "extractVersionFromReadme" coverage.
Renaming or restructuring this suite improves readability and clarity.
50-74: Solid "extractEmojiVersion" tests.
The tests handle valid formats, invalid formats, and whitespace scenarios effectively.
77-86: Confirm version mapping correctness for “13.1” → “13.0”.
If this is an intentional mapping and aligns with spec requirements, then it’s good. Otherwise, revisit the logic to ensure consistency.
117-205: Excellent coverage for “getLatestEmojiVersion”.
You’ve tested draft exclusion, ordering, and fallback—ensuring real-world scenarios are well-handled.
207-243: Comprehensive “extractUnicodeVersion” tests.
All edge cases, including invalid or small emojiVersions, appear to be correctly handled.
245-297: “isEmojiVersionAllowed” logic is well structured.
It’s clear you’re restricting minor/patch releases for versions 1–5 and disallowing 6–10. Double-check if that’s fully intended.
299-315: Great negative path coverage for “getAllEmojiVersions”.
Testing the failure and invalid-data scenarios ensures resilient production code.
317-356: “draft” suite thoroughly tests the fetching and version matching logic.
The coverage of mismatched, invalid, and successful scenarios is excellent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/adapters/src/handlers/modern/sequence.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/adapters/src/handlers/modern/sequence.ts
[warning] 8-8: packages/adapters/src/handlers/modern/sequence.ts#L8
Added line #L8 was not covered by tests
[warning] 12-13: packages/adapters/src/handlers/modern/sequence.ts#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 17-18: packages/adapters/src/handlers/modern/sequence.ts#L17-L18
Added lines #L17 - L18 were not covered by tests
🔇 Additional comments (4)
packages/adapters/src/handlers/modern/sequence.ts (4)
12-14: Change consistent with parameter destructuring.The removal of
ctx.prefix is consistent with the parameter destructuring introduced in the function signature.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: packages/adapters/src/handlers/modern/sequence.ts#L12-L13
Added lines #L12 - L13 were not covered by tests
17-19: Change consistent with parameter destructuring.The removal of
ctx.prefix is consistent with the parameter destructuring introduced in the function signature.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-18: packages/adapters/src/handlers/modern/sequence.ts#L17-L18
Added lines #L17 - L18 were not covered by tests
8-8:❓ Verification inconclusive
Improved code readability with object destructuring.
The parameter destructuring enhancement makes the code more readable by directly extracting the
emoji_versionproperty from the context object. This leads to cleaner code in the function body.
🏁 Script executed:
#!/bin/bash # Check if other handlers also use similar destructuring pattern for consistency rg -n "urls: \(\{.*\}\) =>" --type ts --include "*.ts" packages/adapters/src/handlers/Length of output: 530
Below is a revised version of your review comment that clarifies the consistency check and requests manual verification since the automated search using
rgwith the original flags encountered an error:
Attention: Verify Consistency of Parameter Destructuring Across Handlers
The change in
packages/adapters/src/handlers/modern/sequence.ts(line 8) improves readability by directly destructuring theemoji_versionproperty from the context object:urls: ({ emoji_version }) => {Please ensure that other handler files follow a similar pattern for consistency. Note that our initial automated search using
rgwith the--includeflag encountered an error. A revised command using the-gflag is recommended:#!/bin/bash # Re-run the search for the destructuring pattern in TypeScript files in the handlers directory. rg -n "urls: \(\{.*\}\) =>" -g "*.ts" packages/adapters/src/handlers/Manually verify that any similar implementations in other handlers are consistent with this approach.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-8: packages/adapters/src/handlers/modern/sequence.ts#L8
Added line #L8 was not covered by tests
6-86:❓ Verification inconclusive
Missing test coverage for modified code.
According to the static analysis, the modified code is not covered by tests. Consider adding test cases that verify the correct URL formation for different emoji versions.
🏁 Script executed:
#!/bin/bash # Check for existing tests for this handler echo "Checking for existing tests for modernSequenceHandler..." rg -n "modernSequenceHandler" --type ts --include "*.spec.ts" --include "*.test.ts" packages/ # Check test patterns for other handlers echo "Checking test patterns for other handlers..." rg -n "Handler.*test|test.*Handler" --type ts --include "*.spec.ts" --include "*.test.ts" packages/Length of output: 1301
Please add tests for modernSequenceHandler URL formation
Our initial check to locate test coverage for the
modernSequenceHandlerdid not produce reliable results—specifically, search errors withrg --includesuggest that existing tests (if any) couldn’t be confirmed. We recommend verifying that the handler is tested, particularly for:
- Proper URL construction based on different
emoji_versionvalues.- Correct assignment of cache keys (e.g.,
v${emoji_version}/sequencesandv${emoji_version}/zwj-sequences).If tests are missing, please add dedicated test cases that verify these behaviors. Otherwise, manually confirm that tests exist and update the search commands accordingly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-8: packages/adapters/src/handlers/modern/sequence.ts#L8
Added line #L8 was not covered by tests
[warning] 12-13: packages/adapters/src/handlers/modern/sequence.ts#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 17-18: packages/adapters/src/handlers/modern/sequence.ts#L17-L18
Added lines #L17 - L18 were not covered by tests
| @@ -5,17 +5,17 @@ | |||
|
|
|||
| export const modernSequenceHandler = defineAdapterHandler({ | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider renaming to align with architecture changes.
According to the PR summary, there's a transition from "modern" handlers to "base" handlers. Should this file be renamed from modernSequenceHandler to baseSequenceHandler for consistency?
🏁 Script executed:
#!/bin/bash
# Check which handlers have been renamed from modern to base
echo "Checking for renamed handlers from 'modern' to 'base'..."
rg -n "base.*Handler" --type ts packages/adapters/src/handlers/base/
# Check if there are any remaining modern handlers
echo "Checking for remaining modern handlers..."
rg -n "modern.*Handler" --type ts packages/adapters/src/handlers/Length of output: 588
Action: Rename modernSequenceHandler to align with the new architecture.
After verifying the codebase, it's clear that while some handlers have already been renamed (e.g., baseVariationHandler in packages/adapters/src/handlers/base/variation.ts), the file packages/adapters/src/handlers/modern/sequence.ts still contains modernSequenceHandler. For consistency with the PR's transition guidelines, please update this handler's name to baseSequenceHandler and consider moving the file to the packages/adapters/src/handlers/base/ directory if it fits the overall structure.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/adapters/src/index.ts (1)
98-101:⚠️ Potential issueRemove duplicate error throwing code
There are two identical error throwing statements at the end of the function.
throw new Error(`No handler found for type: ${type}`); } - throw new Error(`No handler found for type: ${type}`); -}
🧹 Nitpick comments (1)
packages/adapters/src/types.ts (1)
124-124: Enhance documentation for updatedshouldExecutetypeThe type has been expanded to accept either a boolean or a function, which adds flexibility to the API. However, this change requires update to the associated documentation to clearly state both options are now supported.
/** * Whether or not that this handler should handle the request. + * This can be either a boolean value or a function that evaluates to a boolean. */ shouldExecute: boolean | ShouldExecute<TContext>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/adapters/src/index.ts(1 hunks)packages/adapters/src/types.ts(1 hunks)packages/adapters/src/types.ts(0 hunks)packages/adapters/src/types.ts(0 hunks)packages/adapters/src/types.ts(3 hunks)packages/adapters/src/index.ts(1 hunks)packages/adapters/src/index.ts(1 hunks)packages/adapters/src/types.ts(1 hunks)packages/adapters/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/adapters/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/adapters/src/index.ts
- packages/adapters/src/types.ts
- packages/adapters/src/index.ts
- packages/adapters/src/types.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/adapters/src/index.ts
[warning] 25-25: packages/adapters/src/index.ts#L25
Added line #L25 was not covered by tests
[warning] 2-5: packages/adapters/src/index.ts#L2-L5
Added lines #L2 - L5 were not covered by tests
[warning] 9-9: packages/adapters/src/index.ts#L9
Added line #L9 was not covered by tests
[warning] 14-17: packages/adapters/src/index.ts#L14-L17
Added lines #L14 - L17 were not covered by tests
[warning] 19-21: packages/adapters/src/index.ts#L19-L21
Added lines #L19 - L21 were not covered by tests
[warning] 23-23: packages/adapters/src/index.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: packages/adapters/src/index.ts#L25
Added line #L25 was not covered by tests
[warning] 27-29: packages/adapters/src/index.ts#L27-L29
Added lines #L27 - L29 were not covered by tests
[warning] 32-32: packages/adapters/src/index.ts#L32
Added line #L32 was not covered by tests
🔇 Additional comments (3)
packages/adapters/src/index.ts (3)
71-71: Type casting withas anyshould be avoidedUsing
as anybypasses TypeScript's type checking. This can lead to runtime errors that could have been caught at compile time.Consider refining the types or using a more specific type assertion that preserves type safety. If the exact type isn't known at this point, create an interface that captures the expected structure.
79-80: Commented-out type errors indicate unresolved type issuesThe
@ts-expect-errorcomment indicates unresolved type issues. This is technical debt that should be addressed.Consider resolving these type issues by:
- Adding proper type definitions for handler methods
- Using generic constraints to ensure type compatibility
- Creating utility types to handle the specific use cases
85-86: Duplicate@ts-expect-errorcomments suggest systematic type issuesMultiple instances of
@ts-expect-errorindicate a systematic issue with the type system design.Consider refactoring the type system to ensure type safety throughout the handler execution pipeline:
- Define clear input/output types for each handler method
- Create utility types for the context extensions
- Use function overloads or conditional types if necessary
Also applies to: 91-92, 94-95
This is similar to how Astro has built their CLI, which i took inspiration from.
feat: improve cli
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli/src/cmd/versions.ts (2)
12-26: Consider providing a use-case or removing the unused--forceflag in help text.Currently, the
--forceflag is displayed in the help text, but there is no logic in this file usingflags.force. If it's truly not needed, removing it from the flags section would reduce confusion. Otherwise, explaining its purpose would be beneficial.
35-53: Handle potential file I/O errors.When reading or writing the lockfile, consider integrating error handling (e.g., try/catch) to gracefully manage file I/O exceptions. Otherwise, any file system error could crash the CLI unexpectedly.
try { const lockfile = await readLockfile(); lockfile.versions = Array.from(versions); lockfile.latest_version = latestVersion; await writeLockfile(lockfile); } catch (err) { + console.error("Failed to read or write lockfile:", err); + process.exit(1); }packages/cli/src/cmd/generate.ts (2)
22-38: Ensure consistent usage of the--forceflag.Similar to
versions.ts, the--forceflag is declared and read, but there’s no direct logic to differentiate forced versus non-forced execution paths here. If you intend to override certain gating conditions, clarifying logic in this block would help.
40-52: Gracefully handle missing or invalid lockfiles.Exiting immediately is acceptable for a CLI, but consider offering the user a command to generate or fix the lockfile automatically instead of requiring manual intervention.
packages/cli/src/cli-utils.ts (1)
29-43: Parameter type documentation mismatch.The docstring for
resolveCommandindicates@param {string} flags, butflagsis actually an object (of typeyargs.Arguments). Consider updating the JSDoc to reflect this correctly.- * @param {string} flags - The parsed command-line arguments from yargs. + * @param {yargs.Arguments} flags - The parsed command-line arguments from yargs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/cli/package.json(1 hunks)packages/cli/src/cli-utils.ts(1 hunks)packages/cli/src/cli.ts(1 hunks)packages/cli/src/cmd/generate.ts(1 hunks)packages/cli/src/cmd/versions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/package.json
🔇 Additional comments (10)
packages/cli/src/cmd/versions.ts (1)
28-33: Good approach to listing available versions.The listing logic is straightforward, with highlighting for drafts. This makes it easy to visually parse the different statuses in the console.
packages/cli/src/cmd/generate.ts (4)
57-70: Nice approach filtering out unsupported versions.Impressively straightforward check for unsupported versions with a helpful warning for users.
72-119: Validate the fallback version logic for older versions.While the fallback logic is solid, confirm that no infinite loop or unintended fallback to the same version can occur if the given version is already the lowest. The code sorts older versions but defaults to the latest if none is found, which might be surprising if the user intended an older fallback.
126-186: Parallel generation withPromise.allSettledis efficient.This chunk effectively handles multiple generator types (metadata, sequences, variations) in parallel. Good usage of directory creation and file writes, providing a clear structure.
258-268: Great closure of the generation flow.Logging errors from the settled promises helps with debugging. The final success message ensures users get feedback on completion.
packages/cli/src/cli-utils.ts (4)
13-19: Enum-like approach for commands is clear.Defining a union type for the CLI commands makes the code maintainable and reduces typos when referring to commands.
45-109: Dynamic help printing is user-friendly.The
printHelpfunction is well-structured, showing usage details, tables, and a descriptive headline. This is concise and effective for new users.
111-156: Good use of a switching mechanism for command resolution.Using a clean switch statement improves clarity while still enabling lazy imports for each command’s code. This fosters faster startup time for the CLI.
158-178: Catch-all error handling is precise.The
try/catchblock that logs and exits with a non-zero code is appropriate for a CLI and ensures errors do not fail silently.packages/cli/src/cli.ts (1)
1-8: Improved code organization through simplified CLI entry point.The CLI implementation has been refactored to significantly improve code organization by moving the complex command handling logic to a separate
cli-utils.tsfile. This makes the main entry point cleaner and more focused on its core responsibility - initializing the CLI and handling top-level errors. The error handling approach is straightforward and appropriate for a CLI tool.This change aligns well with the single responsibility principle and makes the codebase more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/cli/src/cli-utils.ts (3)
14-18: Consider including “help” and “version” in SUPPORTED_COMMANDS or documenting the rationale
Currently, theCLICommandunion includes"help"and"version", but these aren’t present inSUPPORTED_COMMANDS. This works because the code separately checksflags.versionand falls back to"help". However, it may be helpful to either include these commands inSUPPORTED_COMMANDSor document why they're excluded for clarity.
48-112: Prevent potential output formatting issues in CI or non-TTY environments
Usingprocess.stdout.columns < 60to wrap help text works well in typical terminals, but in some CI or non-interactive environmentsprocess.stdout.columnscan be undefined or zero. Consider a fallback or a check to avoid unexpected help output in such cases.- const split = process.stdout.columns < 60; + const columns = typeof process.stdout.columns === 'number' ? process.stdout.columns : 80; + const split = columns < 60;
114-165: Confirm error handling for dynamic imports
The switch statement dynamically imports modules for"versions"and"generate". If there's an import failure or an unhandled rejection in the imported modules, the process might crash. Consider wrapping each import call in a try/catch if partial failures need to be gracefully handled.packages/cli/src/cli.ts (1)
17-18: Add top-level error handling
These lines rely on top-levelawait, which is fine in ESM. However, ifrunCommandthrows an error, it will bubble up and crash the process. If a more graceful failure is desired, wrap these calls in a try/catch or ensure the process exit code is set accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/cli-utils.ts(1 hunks)packages/cli/src/cli.ts(1 hunks)packages/parsers/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/cli/package.json
- pnpm-workspace.yaml
🔇 Additional comments (3)
packages/parsers/package.json (1)
40-40: Clarify Dependency Version Change for apache-autoindex-parseThe version specifier for the
"apache-autoindex-parse"dependency was updated from a caret-based format (e.g.,"^0.2.0") to"catalog:". This indicates a shift to a catalog-based versioning system. Please ensure that:
- Your package manager (pnpm in this case) supports the
"catalog:"notation.- The catalog-based dependency resolution integrates seamlessly with your CI/CD setup and overall dependency management strategy.
If these considerations have been verified, this change should enhance maintainability by standardizing dependency versions.
packages/cli/src/cli-utils.ts (1)
36-46: Validate the positional argument index
The command is resolved fromflags._[2]. Please ensure the CLI invocation approach is consistent so users are aware that commands exist in this position (e.g.,node cli.js mojis generate). If the project ever changes how commands are passed, you may need to update this logic accordingly.packages/cli/src/cli.ts (1)
5-15: Double-check array flags usage in yargs-parser
Thearrayanddefaultconfigurations for"generators"and"shortcode-providers"look correct for multi-value flags. Verify that these flags parse correctly for all expected CLI invocations (e.g.,--generators metadata emojis).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/cli/src/cmd/generate.ts (4)
43-55: Handle unexpected or undefined generators.When no generators are provided or if the value is undefined, the array might become
[undefined], leading to potential issues or confusion. Consider offering default behavior or validating the generator names to ensure that only recognized generators are used.You could, for instance, filter out undefined values or assign a default list of generators:
-const generators = (Array.isArray(flags.generators) ? flags.generators : [flags.generators]) as string[]; +let generators: string[] = []; +if (Array.isArray(flags.generators)) { + generators = flags.generators.filter(Boolean); +} else if (typeof flags.generators === "string") { + generators = [flags.generators]; +} // Provide default, e.g.: +if (generators.length === 0) { + generators = ["metadata", "sequence", "variations"]; +}
57-70: Validate partial support for versions more explicitly.The logic correctly handles unsupported versions; however, consider clarifying why certain versions might not exist or are unsupported (for instance, providing a user-facing message, or referencing documentation). This helps users debug and resolve mismatches in their input more easily.
126-185: Provide error handling for unrecognized generator names.If a user specifies a generator that is not handled by the conditionals (e.g., "metadata", "sequences", "variations"), the code silently ignores it. A warning or explicit validation would help users know when they've provided a misspelled or unimplemented generator.
+for (const generator of generators) { + if (!["metadata", "sequence", "variations"].includes(generator)) { + console.warn(`Unrecognized generator '${generator}' - ignoring.`); + } +}
187-255: Remove or justify large blocks of commented-out code.Blocks of commented-out code can clutter the file and reduce clarity. If these sections are still in development, consider organizing them into separate feature branches or files, or remove them if they are deprecated.
packages/cli/src/cli-utils.ts (3)
35-45: Alert on unknown commands instead of defaulting to "help".Currently, any unsupported command silently falls back to "help". Users who enter an invalid command may not realize it's invalid. Consider showing an explicit error or warning in addition to or in place of defaulting to "help".
const cmd = flags._[2] as string; if (!cmd) { return "help"; } -if (SUPPORTED_COMMANDS.has(cmd as CLICommand)) { - return cmd as CLICommand; -} -return "help"; +if (!SUPPORTED_COMMANDS.has(cmd as CLICommand)) { + console.error(`Unknown command "${cmd}". Please try one of: ${[...SUPPORTED_COMMANDS].join(", ")}`); + return "help"; +} +return cmd as CLICommand;
78-110: Enhance help text layout on small terminals.The table logic uses
process.stdout.columns < 60to handle narrow consoles. Some environments may not have a validstdout.columns, resulting in an undefined or 0 value. Consider a fallback or additional checks to avoid layout issues whencolumnsis invalid.-const split = process.stdout.columns < 60; +const consoleWidth = typeof process.stdout.columns === "number" && process.stdout.columns > 0 + ? process.stdout.columns + : 80; // fallback width +const split = consoleWidth < 60;
120-164: Validate user-provided flags._ indexing.The code uses
flags._.slice(3)to extract versions for the "generate" command. This assumes the command is at_[2]. If users pass flags in a different sequence, or if the parse logic changes, the indexing might break. Consider using named arguments or more robust argument parsing for clarity and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/cli-utils.ts(1 hunks)packages/cli/src/cmd/generate.ts(1 hunks)
🔇 Additional comments (1)
packages/cli/src/cmd/generate.ts (1)
79-120:❓ Verification inconclusive
Improve fallback logic for unsupported versions.
The code sets a fallback version for unsupported versions by picking the immediately preceding version or the latest version available. This approach works but can become confusing if multiple fallback hops occur (e.g., if the fallback version itself is unsupported). Consider either validating the chosen fallback version or applying a more direct fallback path to a guaranteed stable version.
Could you verify if there are scenarios in
existingEmojiVersionswhere the fallback version is also not official, leading to nested fallbacks? If so, we can implement additional logic to recursively find an ultimately supported fallback.
🏁 Script executed:
#!/bin/bash # Gather any chain of fallback versions that loops or references an unsupported version ast-grep --pattern $'versions.map($_ => { $$$ })' --jsonLength of output: 60
Fallback Logic Improvement for Unsupported Versions
The current implementation selects a fallback by choosing the immediately preceding version (or the latest available version) for unsupported emoji versions. Our verification did not reveal any explicit chain of nested fallbacks via the current mapping logic. However, note that if the fallback version chosen ends up being unsupported, it could lead to a chain of fallbacks that might confuse users or introduce unintended behavior.
- Consider adding a validation step: Explicitly check if the chosen fallback version is within the officially supported list.
- Evaluate recursive fallback resolution: Optionally, implement a mechanism that recursively validates or resolves the fallback until a stable, officially supported version is obtained.
Please review and confirm if these enhancements align with the overall design, and apply them if you foresee potential edge cases.
| const results = await Promise.allSettled(promises); | ||
|
|
||
| for (const result of results) { | ||
| if (result.status === "rejected") { | ||
| console.error(result.reason); | ||
| } | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.info(green("done")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust error logging and exit codes.
Though the code collects and logs all errors via Promise.allSettled(), you might want to exit the process with a non-zero exit code if any promise failed, ensuring CI/CD pipelines can catch the error. This is especially important in automated workflows.
for (const result of results) {
if (result.status === "rejected") {
console.error(result.reason);
+ // eslint-disable-next-line node/prefer-global/process
+ process.exitCode = 1;
}
}📝 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.
| const results = await Promise.allSettled(promises); | |
| for (const result of results) { | |
| if (result.status === "rejected") { | |
| console.error(result.reason); | |
| } | |
| } | |
| // eslint-disable-next-line no-console | |
| console.info(green("done")); | |
| } | |
| const results = await Promise.allSettled(promises); | |
| for (const result of results) { | |
| if (result.status === "rejected") { | |
| console.error(result.reason); | |
| // eslint-disable-next-line node/prefer-global/process | |
| process.exitCode = 1; | |
| } | |
| } | |
| // eslint-disable-next-line no-console | |
| console.info(green("done")); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/cli/test/cli.test.ts (2)
63-69: Enhance error testing specificity.While this test verifies that errors are thrown, it would be more robust to check for specific error messages or types.
- await expect(() => cli(["build"])).rejects.toThrow(); + await expect(() => cli(["build"])).rejects.toThrow("Command not found");
1-70: Add test for--write-lockfileflag and empty args.The test suite is missing coverage for the
--write-lockfileflag and for cases where no command is provided.Consider adding these additional tests:
it("should handle write-lockfile flag correctly", async () => { await cli(["build", "--write-lockfile"]); expect(cliUtils.resolveCommand).toHaveBeenCalledWith( expect.objectContaining({ "_": ["build"], "write-lockfile": true, }), ); }); it("should handle no command gracefully", async () => { await cli([]); expect(cliUtils.resolveCommand).toHaveBeenCalledWith( expect.objectContaining({ "_": [], "generators": ["metadata"], "shortcode-providers": ["github"], "force": false, "write-lockfile": false, }), ); });packages/cli/src/cli.ts (3)
5-28: Enhance error handling with specific messages.The error handling is basic - it just logs the error and exits. Consider providing more specific error messages based on the error type.
try { const flags = yargs(args, { // ... configuration }); const cmd = resolveCommand(flags); await runCommand(cmd, flags); } catch (err) { - console.error(err); + if (err instanceof Error) { + console.error(`Error: ${err.message}`); + } else { + console.error('An unknown error occurred:', err); + } process.exit(1); }
30-30: Consider making automatic CLI invocation optional.The CLI is automatically invoked when the module is imported, which might not be desirable when using the module programmatically.
-cli(process.argv.slice(2)); +// Only run CLI when this file is executed directly +if (require.main === module) { + cli(process.argv.slice(2)); +}
1-31: Add help and version flags.The CLI is missing built-in help text and version information which would improve user experience.
Consider adding help and version flags:
import process from "node:process"; import yargs from "yargs-parser"; import { resolveCommand, runCommand } from "./cli-utils"; +import { version } from "../package.json"; export async function cli(args: string[]): Promise<void> { try { const flags = yargs(args, { configuration: { "parse-positional-numbers": false, "camel-case-expansion": false, }, array: ["generators", "shortcode-providers"], - boolean: ["force", "write-lockfile"], + boolean: ["force", "write-lockfile", "help", "version"], default: { "generators": ["metadata"], "shortcode-providers": ["github"], "force": false, "write-lockfile": false, + "help": false, + "version": false, }, }); + if (flags.help) { + printHelp(); + return; + } + + if (flags.version) { + console.log(version); + return; + } const cmd = resolveCommand(flags); await runCommand(cmd, flags); } catch (err) { console.error(err); process.exit(1); } } +function printHelp() { + console.log(` +Usage: mojis [command] [options] + +Commands: + build Build emoji data + +Options: + --generators Generators to use [array] [default: ["metadata"]] + --shortcode-providers Shortcode providers to use [array] [default: ["github"]] + --force Force rebuild [boolean] [default: false] + --write-lockfile Write lockfile [boolean] [default: false] + --help Show help [boolean] + --version Show version [boolean] +`); +} cli(process.argv.slice(2));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/cli.ts(1 hunks)packages/cli/test/cli.test.ts(1 hunks)
🔇 Additional comments (6)
packages/cli/test/cli.test.ts (4)
10-24: LGTM! Great test for default arguments.This test effectively verifies that the CLI passes the correct default arguments to resolveCommand when invoked with the "build" command.
26-37: LGTM! Good coverage of argument overrides.This test correctly verifies that command-line arguments properly override the default values.
39-49: LGTM! Proper array values handling.This test ensures that multiple values for array-type options are correctly processed.
51-61: LGTM! Good command execution flow testing.This test correctly verifies that the resolved command is passed to the runCommand function.
packages/cli/src/cli.ts (2)
2-3: LGTM! Clean dependency imports.Good choice using yargs-parser instead of the full yargs package, and using separate utility functions for command resolution and execution.
7-20: LGTM! Well-structured configuration.The yargs configuration is cleanly set up with appropriate settings for arrays, booleans, and default values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/adapters/src/handlers/variation.ts (1)
12-24: URL construction logic depends on versionThe conditional URL construction based on semantic versioning is well implemented. However, consider adding a brief comment explaining why the URL pattern changes at version 12.1.0 to help future maintainers understand this decision.
urls: (ctx) => { + // Unicode.org changed their directory structure after version 12.1.0 if (semver.lte(`${ctx.unicode_version}.0`, "12.1.0")) { return { url: `https://unicode.org/Public/emoji/${ctx.emoji_version}/emoji-variation-sequences.txt`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/adapters/src/handlers.ts(1 hunks)packages/adapters/src/handlers/metadata.ts(5 hunks)packages/adapters/src/handlers/variation.ts(1 hunks)packages/cli/test/cli.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/test/cli.test.ts
- packages/adapters/src/handlers.ts
🔇 Additional comments (10)
packages/adapters/src/handlers/variation.ts (5)
1-6: Clear imports and well-defined constantsThe imports are appropriate for the functionality, and defining the
NOT_EXISTINGarray as a constant makes the code's intent clear.
7-9: Good documentation of version limitationThe comment clearly explains why special handling is needed for versions before v5.0, providing important context for future maintainers.
26-51: Robust validation in transform methodThe transform method includes proper validation for hexcodes and styles, with clear error messages when invalid data is encountered. This defensive programming approach helps catch issues early.
59-72: Consistent handler for unsupported versionsThe
notSupportedVariationHandlerprovides a clean way to handle versions without variation files. This is better than having conditional logic scattered throughout the code.One minor observation:
transform()returnsundefinedwhileoutput()returns an empty array. While this works as expected, consider aligning these return values for consistency (though the current implementation is valid sinceoutput()determines the final result).
13-13: Potential fragility in semver comparisonThe code appends
.0toctx.unicode_versionto ensure a three-component version for semver comparison:semver.lte(`${ctx.unicode_version}.0`, "12.1.0")This assumes
unicode_versionis always in "major.minor" format. If it could ever include a patch version already (e.g., "13.1.0"), this would result in an invalid semver string ("13.1.0.0").Consider adding a check to verify the format of
unicode_versionbefore appending.0, or ensure through documentation thatunicode_versionwill always be in "major.minor" format.packages/adapters/src/handlers/metadata.ts (5)
2-3: Import changes for enhanced functionalityThe addition of
isBeforeis a good enhancement that supports the new version comparison logic. The change to the import path fordefineAdapterHandleraligns with the refactoring mentioned in the PR.
17-18: Handler renaming and improved documentationRenaming
modernMetadataHandlertobaseMetadataHandleris more semantically appropriate and clear. The added comments provide excellent context about why certain emoji versions are handled differently and clearly reference the fallback handler.Also applies to: 21-21, 24-27
94-105: Well-implemented version-specific emoji extraction logicThe added logic correctly handles the different formatting of emoji metadata based on version. Using
isBeforeto determine the parsing strategy is a clean approach, and the code is well-commented to explain the different behavior for versions before 6.0.0.
120-122: Improved metadata property handlingThe changes to how
emojiVersionandunicodeVersionare assigned provide better robustness with appropriate fallbacks when versions aren't available. Using the utility function forunicodeVersionensures consistency across the codebase.
138-158: Well-designed fallback handler for unsupported versionsThe new
notSupportedMetadataHandleris a good addition that handles emoji versions without an emoji-test file gracefully. The implementation is clean, follows the established handler pattern, and ensures proper complementary coverage withbaseMetadataHandlerthrough opposite conditions inshouldExecute.
This PR adds more handlers and overall cleans up the mess left by #30
Summary by CodeRabbit
New Features
Refactor
Chores