test: cover framework adapters #31
Conversation
🦋 Changeset detectedLatest commit: d10fc99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces new test suites for the Angular, React, Solid, and Vue adapters, verifying their reactive utilities and confirming that internal override functions are no longer publicly exposed. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FrameworkAdapter (Angular/React/Solid/Vue)
participant UnSignal/UnComputed
participant InternalOverrides
User->>FrameworkAdapter: Call isUnRef(value)
FrameworkAdapter->>FrameworkAdapter: Check if value is ref, function, UnSignal, or UnComputed
FrameworkAdapter-->>User: Return true/false
User->>FrameworkAdapter: Call toUnSignal(value)
FrameworkAdapter->>UnSignal/UnComputed: Convert value to UnSignal
User->>FrameworkAdapter: Call unResolve(signal, options)
FrameworkAdapter->>UnSignal/UnComputed: Convert UnSignal to framework ref or UnSignal/UnComputed (if framework is null)
User->>FrameworkAdapter: Access override functions
FrameworkAdapter-->>User: Return undefined (not exposed)
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 8000 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 (
|
32cebbd to
b66b405
Compare
|
opened a feature request in vitest-dev/eslint-plugin-vitest#729 |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to b66b405 in 2 minutes and 2 seconds. Click for details.
- Reviewed
1080lines of code in13files - Skipped
1files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/bitter-cameras-give.md:1
- Draft comment:
Consider adding more descriptive changelog messages to clarify the fixes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to improve the changelog messages, which is similar to asking to update the PR description. It doesn't provide a specific code suggestion or point out a potential issue in the code itself.
2. packages/unuse-angular/package.json:33
- Draft comment:
Dev dependency '@angular/platform-browser' has been added; ensure its version aligns with peer Angular versions for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is about a dependency change, specifically about ensuring version alignment. According to the rules, comments on dependency changes or library versions should be ignored. Therefore, this comment should be removed.
3. packages/unuse-angular/src/index.spec.ts:34
- Draft comment:
Angular test cases are comprehensive; verify that the test environment setup (using getTestBed) does not leak side effects over multiple runs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the test environment setup does not leak side effects, which is against the rules as it asks for confir 10000 mation and verification. It does not provide a specific suggestion or point out a specific issue.
4. packages/unuse-angular/src/index.ts:195
- Draft comment:
Hiding internal override functions via exporting UNDEFINED is clever; ensure this pattern remains maintainable and documented for future contributors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment starts with praising the approach as "clever" but then asks to "ensure" maintainability and documentation. This is a request for verification/confirmation rather than a specific issue. The pattern itself seems intentional and the code is fairly self-documenting through its clear structure and naming. The pattern could potentially be confusing to developers not familiar with this technique. Maybe there should be a code comment explaining why this is done. While documentation could help, the comment as written is not actionable - it's just asking for verification. If documentation was needed, the comment should specifically request adding a comment explaining the pattern. Delete the comment as it's asking for verification rather than making a specific, actionable request. It violates the rule about not asking authors to verify or ensure things.
5. packages/unuse-react/package.json:33
- Draft comment:
The new dev dependency '@testing-library/react' is added; confirm its version is compatible with your React version and testing setup. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is about a new dev dependency being added and asks the author to confirm compatibility with their React version and testing setup. This falls under the rule of not asking the author to confirm or double-check things related to dependencies.
6. packages/unuse-react/src/index.ts:51
- Draft comment:
When wrapping React refs, the useReactEffect dependency array uses [value]; consider if adding value.current as a dependency would improve reactivity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. packages/unuse-solid/src/index.ts:197
- Draft comment:
Override functions are hidden using UNDEFINED and @ts-expect-error comments; document this for clarity and consider refining types to reduce such suppressions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. packages/unuse-vue/src/index.spec.ts:26
- Draft comment:
The Vue test suite uses a custom mount and useSetup helper effectively; ensure that component unmounting cleans up all side effects. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that component unmounting cleans up all side effects, which falls under the category of asking the author to ensure behavior is intended or tested. This violates the rules.
9. packages/unuse-vue/src/index.ts:49
- Draft comment:
Vue integration uses vueWatch and effect for two-way binding; review the flush mode (sync) to ensure optimal reactivity performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. vitest.config.ts:10
- Draft comment:
Vitest configuration appears robust with proper exclusions and coverage thresholds; verify that the threshold values match your project goals. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify threshold values, which falls under asking them to ensure behavior is intended or to double-check things. This violates the rules.
11. packages/unuse-angular/src/index.spec.ts:51
- Draft comment:
Typographical error: Consider changing 'a Angular Signal' to 'an Angular Signal' in the test description. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar correction is technically correct, our rules state that we should not make purely informative comments or comments that are obvious/unimportant. Grammar fixes in test descriptions don't affect the functionality or maintainability of the code. This is a very minor stylistic issue that doesn't warrant a PR comment. The grammar error could make the code less professional and might be replicated in other places if not fixed now. Documentation quality is important. While documentation quality matters, this is a test description in a test file, not user-facing documentation. The meaning is perfectly clear despite the minor grammar issue. Delete the comment. Grammar fixes in test descriptions are too minor to warrant a PR comment and don't affect code quality or functionality.
Workflow ID: wflow_yCEl2d9OY8njLEUc
You can customize 
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/unuse-angular/src/index.ts (1)
126-147: Address the HACK in Angular signal synchronization.The current implementation directly modifies Angular signal methods (
setandupdate) to maintain reactivity. This approach is fragile and could break with Angular updates. Consider:
- Opening an issue to track this technical debt
- Investigating alternative approaches for bidirectional synchronization
- Adding tests to detect if Angular's behavior changes
Would you like me to help investigate a more robust solution or create an issue to track this?
♻️ Duplicate comments (1)
packages/unuse-angular/src/index.spec.ts (1)
96-97: Same framework null workaround as in Solid testsThis has the same design limitation as noted in the Solid tests regarding the need to cast
nulltoundefined.The same type-safe solution suggested for the Solid tests would apply here as well.
Also applies to: 107-108
🧹 Nitpick comments (7)
packages/unuse-angular/package.json (1)
30-30: Consider consistent versioning for alien-signals dependency.The
alien-signalsdependency is pinned to exact version2.0.5while other packages use^2.0.5. Consider using consistent versioning across all packages for better maintenance.- "alien-signals": "2.0.5", + "alien-signals": "^2.0.5",.changeset/bitter-cameras-give.md (1)
9-10: Consider more descriptive changeset messages.While the changeset format is correct, the descriptions could be more specific about what was fixed. Consider expanding the descriptions to help users understand the changes better.
-fix: isUnRef -fix!: internals not exposed +fix: improve isUnRef to recognize more reactive types +fix!: remove internal override functions from public APIpackages/unuse-angular/src/index.ts (1)
168-173: Document Angular's scope disposal limitation.The
tryOnScopeDisposefunction always returnsfalsefor Angular, indicating that Angular doesn't support effect scope disposal like Vue or Solid. This limitation should be documented in the API documentation to set proper expectations for users.packages/unuse-vue/src/index.spec.ts (1)
36-46: Move test utilities to a separate file.The
useSetupfunction is being exported from a test file, which violates best practices. Consider moving test utilities likemountanduseSetupto a separate test utilities file (e.g.,test-utils.ts).-// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types -export function useSetup<V>(setup: () => V) { +function useSetup<V>(setup: () => V) {Or create a new file
packages/unuse-vue/src/test-utils.tsand move these utilities there.packages/unuse-solid/src/index.ts (1)
187-194: ```shell
#!/bin/bashLocate the index.ts file in the unuse-solid package
find packages/unuse-solid/src -type f -name "index.ts" -print
If found, show lines around the Array.isArray usage
FILE=$(find packages/unuse-solid/src -type f -name "index.ts" | head -n1)
if [ -n "$FILE" ]; then
rg -n -C3 "Array\.isArray" "$FILE"
else
echo "index.ts not found in packages/unuse-solid/src"
fi</blockquote></details> <details> <summary>packages/unuse-solid/src/index.spec.ts (1)</summary><blockquote> `78-79`: **Document the framework null workaround** The TODO comments indicate a design limitation where `null` must be cast to `undefined` because the default parameter handling doesn't distinguish between `null` and `undefined`. Consider creating a type-safe way to explicitly opt out of framework-specific behavior: ```diff +export interface UnResolveOptionsWithoutFramework<TReadonly extends boolean = false> { + framework?: null; + readonly?: TReadonly; +} +export function unResolve<T, TReadonly extends boolean = false>( + signal: UnSignal<T>, + options: UnResolveOptionsWithoutFramework<TReadonly> +): TReadonly extends true ? UnComputed<T> : UnSignal<T>;Also applies to: 89-90
packages/unuse-angular/src/index.spec.ts (1)
136-136: Fix copy-paste error in commentThe comment incorrectly refers to "Vue effect scope" in an Angular test file.
- it('should return false if not inside a Vue effect scope', () => { + it('should return false if not inside an Angular effect scope', () => {
📜 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 (13)
.changeset/bitter-cameras-give.md(1 hunks)packages/unuse-angular/package.json(1 hunks)packages/unuse-angular/src/index.spec.ts(1 hunks)packages/unuse-angular/src/index.ts(3 hunks)packages/unuse-react/package.json(1 hunks)packages/unuse-react/src/index.spec.ts(1 hunks)packages/unuse-react/src/index.ts(4 hunks)packages/unuse-solid/package.json(1 hunks)packages/unuse-solid/src/index.spec.ts(1 hunks)packages/unuse-solid/src/index.ts(3 hunks)packages/unuse-vue/src/index.spec.ts(1 hunks)packages/unuse-vue/src/index.ts(3 hunks)vitest.config.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/unuse-angular/src/index.ts (5)
packages/unuse/src/unSignal/index.ts (1)
isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (1)
isUnComputed(47-55)packages/unuse-react/src/index.ts (4)
UNDEFINED(215-215)UNDEFINED(216-216)UNDEFINED(217-217)UNDEFINED(218-218)packages/unuse-solid/src/index.ts (4)
UNDEFINED(207-207)UNDEFINED(208-208)UNDEFINED(209-209)UNDEFINED(210-210)packages/unuse-vue/src/index.ts (4)
UNDEFINED(193-193)UNDEFINED(194-194)UNDEFINED(195-195)UNDEFINED(196-196)
packages/unuse-solid/src/index.ts (5)
packages/unuse/src/unSignal/index.ts (1)
isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (1)
isUnComputed(47-55)packages/unuse-react/src/index.ts (4)
UNDEFINED(215-215)UNDEFINED(216-216)UNDEFINED(217-217)UNDEFINED(218-218)packages/unuse-angular/src/index.ts (4)
UNDEFINED(205-205)UNDEFINED(206-206)UNDEFINED(207-207)UNDEFINED(208-208)packages/unuse-vue/src/index.ts (4)
UNDEFINED(193-193)UNDEFINED(194-194)UNDEFINED(195-195)UNDEFINED(196-196)
packages/unuse-vue/src/index.spec.ts (7)
packages/unuse-vue/src/index.ts (4)
toUnSignal(197-197)unResolve(199-199)tryOnScopeDispose(198-198)isUnRef(192-192)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(80-82)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
packages/unuse-vue/src/index.ts (5)
packages/unuse/src/unSignal/index.ts (1)
isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (1)
isUnComputed(47-55)packages/unuse-react/src/index.ts (4)
UNDEFINED(215-215)UNDEFINED(216-216)UNDEFINED(217-217)UNDEFINED(218-218)packages/unuse-angular/src/index.ts (4)
UNDEFINED(205-205)UNDEFINED(206-206)UNDEFINED(207-207)UNDEFINED(208-208)packages/unuse-solid/src/index.ts (4)
UNDEFINED(207-207)UNDEFINED(208-208)UNDEFINED(209-209)UNDEFINED(210-210)
packages/unuse-react/src/index.ts (5)
packages/unuse/src/unSignal/index.ts (1)
isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (1)
isUnComputed(47-55)packages/unuse-angular/src/index.ts (4)
UNDEFINED(205-205)UNDEFINED(206-206)UNDEFINED(207-207)UNDEFINED(208-208)packages/unuse-solid/src/index.ts (4)
UNDEFINED(207-207)UNDEFINED(208-208)UNDEFINED(209-209)UNDEFINED(210-210)packages/unuse-vue/src/index.ts (4)
UNDEFINED(193-193)UNDEFINED(194-194)UNDEFINED(195-195)UNDEFINED(196-196)
🪛 ESLint
packages/unuse-react/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@testing-library/react'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-vue/src/index.spec.ts
[error] 6-6: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 7-7: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 21-21: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-angular/src/index.spec.ts
[error] 8-8: Unable to resolve path to module '@angular/core'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@angular/core/testing'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@angular/platform-browser/testing'.
(import-x/no-unresolved)
[error] 14-14: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 28-28: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-solid/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@solidjs/testing-library'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'solid-js'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
🪛 Biome (1.9.4)
packages/unuse-vue/src/index.spec.ts
[error] 36-46: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/unuse-angular/src/index.spec.ts
[error] 29-32: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🔇 Additional comments (17)
packages/unuse-react/package.json (1)
34-34: Verify @testing-library/react version compatibility.The added
@testing-library/reactversion 16.3.0 should be verified for compatibility with the React peer dependency requirement of>=19. Ensure this testing library version supports React 19.What is the latest version of @testing-library/react and does version 16.3.0 support React 19?packages/unuse-solid/package.json (1)
33-35: Verify @solidjs/testing-library version compatibility.The added
@solidjs/testing-libraryversion 0.8.10 should be verified for compatibility with the solid-js peer dependency requirement of>=1.9. Ensure this testing library version supports the minimum SolidJS version.What is the latest version of @solidjs/testing-library and does version 0.8.10 support solid-js 1.9 and above?vitest.config.ts (2)
10-15: LGTM: Appropriate coverage exclusions added.The exclusion of
**/dist/**anddocs/.vitepress/**directories from coverage analysis is appropriate as these contain generated files and documentation that shouldn't be included in test coverage metrics.
25-28: LGTM: Coverage thresholds appropriately increased.The increased coverage thresholds (75% for lines/statements/functions, 80% for branches) reflect the improved test coverage from the new framework adapter test suites. The gradual approach toward the TODO targets is reasonable.
packages/unuse-angular/package.json (1)
34-34: LGTM: Appropriate Angular testing dependency added.The
@angular/platform-browserdependency is correctly added to support Angular-specific testing requirements.packages/unuse-vue/src/index.ts (2)
174-181: LGTM! Extended type recognition improves framework interoperability.The expanded
isUnReffunction now correctly identifies Vue refs, getter functions, UnSignal, and UnComputed types, aligning with the changes in other framework adapters.
189-200: Breaking change: Internal override functions are no longer exposed.This change improves encapsulation by preventing external access to internal override functions. While this is a good architectural decision, it's a breaking change that should be documented in the changelog.
packages/unuse-react/src/index.spec.ts (1)
31-42: Clarify the status of TODO tests.Several tests are marked with
.todo()but contain full implementations. Are these tests:
- Failing due to implementation issues?
- Awaiting React adapter implementation?
- Temporarily disabled?
Consider adding comments explaining why they're marked as TODO or enable them if the functionality is ready.
Also applies to: 43-54, 70-81, 119-133
packages/unuse-react/src/index.ts (4)
17-18: LGTM: Imports added for expanded isUnRef functionalityThe additional imports for
isUnComputedandisUnSignalare correctly added to support the expandedisUnReffunction logic.
177-177: LGTM: Clear documentation of React limitationsGood documentation explaining that React doesn't have a direct equivalent to
onScopeDispose, which helps clarify why the function returnsfalse.
192-201: LGTM: Comprehensive reactive type detectionThe expanded
isUnReffunction correctly identifies all reactive types:
- React refs (objects with
currentproperty)- React state arrays
- Getter functions
- UnSignal instances
- UnComputed instances
The logic is sound and consistent with the pattern established in other framework adapters.
211-222: LGTM: Proper encapsulation of internal override functionsThe pattern of exporting override functions as
undefinedwhile keeping the actual implementations internal is a good architectural decision. This prevents external access to internal implementation details while maintaining the required API surface.packages/unuse-solid/src/index.spec.ts (3)
1-19: LGTM: Well-structured test setupThe test setup is properly configured with:
- Happy DOM environment for browser simulation
- Appropriate imports from Solid.js and testing utilities
- Comprehensive import of functions to test
The static analysis import errors can be safely ignored as they're related to the sandbox environment limitations.
54-66: TODO test needs implementationThe test for resolving UnSignal to Solid Signal is marked as
todobut contains implementation. This suggests the test might not be working as expected.Please verify if this test should remain as
todoor if it can be enabled. If there are issues preventing it from running, consider documenting the specific problem.
164-169: LGTM: Verifies proper API encapsulationThis test correctly verifies that internal override functions are not exposed publicly, which aligns with the architectural goal of hiding implementation details.
packages/unuse-angular/src/index.spec.ts (2)
122-134: TODO test needs implementationSimilar to the Solid test file, this TODO test contains implementation but is marked as todo, suggesting it may not be working correctly.
Please verify if this test should be enabled or if there are specific issues preventing it from running properly.
184-189: LGTM: Verifies proper API encapsulationThis test correctly verifies that internal override functions are not exposed publicly, maintaining consistency with the other framework adapters.
b66b405 to
15418e5
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 15418e5 in 1 minute and 58 seconds. Click for details.
- Reviewed
1080lines of code in13files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/unuse-react/src/index.ts:56
- Draft comment:
When checking for React state, consider verifying that the array has exactly 2 elements instead of just a non‐empty array to avoid false positives. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/unuse-react/src/index.ts:64
- Draft comment:
Using a function as the state updater (e.g. value[1](() => result.get())) may be unclear if a direct value could be passed. Verify that this functional updater is intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/unuse-angular/src/index.ts:195
- Draft comment:
The use of '@ts-expect-error: just do it' when overriding internal functions is repeated. Ensure this pattern is well documented and reviewed for type safety. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that a pattern is well documented and reviewed, which violates the rule against asking the author to ensure something. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. packages/unuse-angular/src/index.spec.ts:51
- Draft comment:< E7F5 br> Typo: In the description, "should convert a Angular Signal to an UnSignal" should use the correct article: "an Angular Signal".
- Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typo in the description, which is not relevant to the code itself. It does not provide any actionable feedback related to the code changes.
Workflow ID: wflow_3Wtrvfmjk2lYLSR9
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/unuse-vue/src/index.spec.ts (2)
99-102: API design issue already tracked.The confusing
null as unknown as undefinedtype cast is a known issue that will be addressed in a separate PR as indicated by past review comments.
110-114: API design issue already tracked.The confusing
null as unknown as undefinedtype cast is a known issue that will be addressed in a separate PR as indicated by past review comments.packages/unuse-solid/src/index.spec.ts (1)
54-54: Todo test is intentionally incomplete.Based on the past review comments, this test is marked as todo intentionally due to failing tests that are outside the current PR scope.
packages/unuse-react/src/index.spec.ts (1)
26-62: Comprehensive toUnSignal test coverage with appropriate TODO markers.The tests cover both reactive (React state/ref) and non-reactive value conversion scenarios. The TODO tests for React-specific conversions are appropriately marked based on the past review discussion.
🧹 Nitpick comments (3)
packages/unuse-solid/src/index.spec.ts (1)
75-84: Consider improving type handling for null framework parameter.The type assertions
null as unknown as undefinedindicate a potential design issue where the API doesn't properly support null values as intended by the TODO comments.Consider updating the function signature to explicitly allow null values:
- framework: null as unknown as undefined, + framework: null,This would require updating the type definitions to properly handle null as a valid framework parameter.
Also applies to: 86-96
packages/unuse-react/src/index.ts (1)
177-177: Minor improvement: Update comment for clarity.The comment could be more specific about React's lifecycle management approach.
- // React does not have a direct equivalent to onScopeDispose + // React uses useEffect cleanup functions instead of onScopeDisposepackages/unuse-angular/src/index.spec.ts (1)
136-136: Fix the incorrect framework reference in comment.The comment mentions "Vue effect scope" but this is an Angular test file. This appears to be a copy-paste error from another framework adapter.
- it('should return false if not inside a Vue effect scope', () => { + it('should return false if not inside an Angular injection context', () => {
📜 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 (13)
.changeset/bitter-cameras-give.md(1 hunks)packages/unuse-angular/package.json(1 hunks)packages/unuse-angular/src/index.spec.ts(1 hunks)packages/unuse-angular/src/index.ts(3 hunks)packages/unuse-react/package.json(1 hunks)packages/unuse-react/src/index.spec.ts(1 hunks)packages/unuse-react/src/index.ts(4 hunks)packages/unuse-solid/package.json(1 hunks)packages/unuse-solid/src/index.spec.ts(1 hunks)packages/unuse-solid/src/index.ts(3 hunks)packages/unuse-vue/src/index.spec.ts(1 hunks)packages/unuse-vue/src/index.ts(3 hunks)vitest.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/unuse-react/package.json
- .changeset/bitter-cameras-give.md
- packages/unuse-angular/package.json
- packages/unuse-solid/package.json
- vitest.config.ts
- packages/unuse-angular/src/index.ts
- packages/unuse-vue/src/index.ts
- packages/unuse-solid/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/unuse-angular/src/index.spec.ts (8)
packages/unuse/src/_testUtils/angular.ts (1)
NgModule(32-34)packages/unuse-angular/src/index.ts (4)
toUnSignal(209-209)unResolve(211-211)tryOnScopeDispose(210-210)isUnRef(204-204)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(80-82)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
packages/unuse-react/src/index.spec.ts (7)
packages/unuse-react/src/index.ts (4)
toUnSignal(219-219)unResolve(221-221)tryOnScopeDispose(220-220)isUnRef(214-214)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(80-82)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
🪛 ESLint
packages/unuse-angular/src/index.spec.ts
[error] 8-8: Unable to resolve path to module '@angular/core'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@angular/core/testing'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@angular/platform-browser/testing'.
(import-x/no-unresolved)
[error] 14-14: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 28-28: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-react/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@testing-library/react'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-solid/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@solidjs/testing-library'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'solid-js'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-vue/src/index.spec.ts
[error] 6-6: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 7-7: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 21-21: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
🪛 Biome (1.9.4)
packages/unuse-vue/src/index.spec.ts
[error] 36-46: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🔇 Additional comments (19)
packages/unuse-vue/src/index.spec.ts (1)
48-196: Comprehensive test coverage looks excellent.The test suite provides thorough coverage of all framework adapter functions:
- Global framework variable verification
- Bidirectional reactive state synchronization in
toUnSignal- Various resolution scenarios in
unResolve- Lifecycle integration with
tryOnScopeDispose- Type guard functionality in
isUnRef- Security verification that override functions are not exposed
The tests appropriately use Vue's composition API and lifecycle features to validate integration behavior.
packages/unuse-solid/src/index.spec.ts (6)
1-20: Test setup and imports look comprehensive.The test file follows best practices with proper environment setup for DOM testing and comprehensive imports covering all the necessary utilities for testing SolidJS reactive functionality.
22-24: Good verification of framework identifier.This test ensures the global framework identifier is correctly set, which is important for framework-specific behavior.
26-47: Comprehensive toUnSignal testing.The tests properly cover both SolidJS signal conversion and non-reactive value conversion, verifying bidirectional data flow through get/set operations.
104-114: Excellent testing of scope disposal functionality.The test properly verifies that the callback is registered during render and executed during cleanup, which is critical for memory management in SolidJS applications.
125-162: Thorough coverage of reactive type detection.The isUnRef tests comprehensively cover all supported reactive types (SolidJS signals, memos, UnSignal, UnComputed, getters) and properly verify rejection of non-reactive values.
164-169: Important verification of API encapsulation.This test ensures internal override functions remain private, which is crucial for maintaining a clean public API surface and preventing misuse of internal implementation details.
packages/unuse-react/src/index.spec.ts (6)
1-19: Well-structured test setup with appropriate imports and environment configuration.The test file correctly uses the happy-dom environment for React testing and imports all necessary testing utilities and functions from the React adapter. The import structure is clean and comprehensive.
21-24: Good verification of framework identifier.This test properly verifies that the global framework variable is set to 'react', ensuring the framework detection works correctly.
65-111: Thorough unResolve test coverage with edge case handling.The tests properly cover:
- Basic UnSignal to React state conversion (marked as TODO appropriately)
- Readonly mode functionality
- Framework override behavior with null values
- Different return types based on options
The TODO comments on lines 93-94 and 104-105 raise a valid concern about the framework parameter type handling.
// Consider updating the type definition to explicitly allow null framework?: TFramework | null;
114-141: Good coverage of tryOnScopeDispose functionality.The tests appropriately cover both the React-specific behavior (marked as TODO) and the fallback behavior when not in a React effect scope. The test correctly expects
falsereturn value since React doesn't have direct scope disposal equivalents.
144-185: Excellent comprehensive isUnRef type guard testing.The test suite thoroughly validates all supported reactive reference types:
- React-specific types (State, Ref, Memo)
- Universal types (UnSignal, UnComputed)
- Function types (getters)
- Negative cases for non-reactive values
This provides excellent coverage for the expanded isUnRef functionality.
188-193: Critical test ensuring internal functions remain hidden.This test is essential for verifying that the breaking change to hide internal override functions is working correctly. It ensures the public API contract is maintained while keeping implementation details private.
packages/unuse-react/src/index.ts (3)
17-18: Appropriate imports for extended isUnRef functionality.The new imports for
isUnComputedandisUnSignalare necessary for the enhanced type guard functionality and align with the universal reactive type support.
192-202: Excellent enhancement to isUnRef type guard functionality.The updated implementation correctly identifies all supported reactive reference types:
- React refs (
currentproperty)- React state tuples (arrays)
- Function getters
- UnSignal and UnComputed types
This provides comprehensive coverage for determining reactive references across the React ecosystem.
211-222: Smart approach to hiding internal override functions.The use of a
UNDEFINEDconstant and aliased exports effectively hides the internal override function implementations while maintaining the export interface. This successfully implements the breaking change to remove internal implementation details from the public API.packages/unuse-angular/src/index.spec.ts (3)
30-32: Good fix for the export issue.The
ZonelessTestModuleis now correctly defined as a non-exported class, addressing the previous review comment about not exporting from test files.
122-134: Clarify the status of the TODO test.This test is marked as
it.todobut the implementation appears complete. Please clarify if:
- The test doesn't work as expected due to Angular limitations
- There are issues with the scope disposal mechanism
- This is waiting for framework support
If the test is functional, consider removing the
.todoand enabling it.
34-190: Excellent comprehensive test coverage.The test suite provides thorough coverage of all framework adapter functions with proper Angular testing setup:
- ✅ Tests all main functions:
toUnSignal,unResolve,tryOnScopeDispose,isUnRef- ✅ Verifies internal override functions are not exposed
- ✅ Uses proper Angular testing environment with zoneless change detection
- ✅ Covers both positive and negative test cases
- ✅ Tests integration with Angular's reactive primitives (signals, computed)
The structure aligns well with the PR objectives to add comprehensive test coverage for framework adapters.
15418e5 to
ecd5437
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ecd5437 in 2 minutes and 24 seconds. Click for details.
- Reviewed
1079lines of code in13files - Skipped
1files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/bitter-cameras-give.md:9
- Draft comment:
Changeset message correctly lists the fixes. Ensure the description stays in sync with the code behaviour. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and asks the PR author to ensure the description stays in sync with the code behavior, which violates the rules.
2. packages/unuse-angular/src/index.ts:195
- Draft comment:
Internal override functions are being hidden by re-exporting as UNDEFINED. Make sure this design is clearly documented for API consumers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the design is documented for API consumers. This falls under the category of asking the author to ensure something, which is against the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.
3. packages/unuse-react/src/index.ts:47
- Draft comment:
The toUnSignal function correctly distinguishes React refs and state arrays. Verify if the dependency array ([value]) in useReactEffect is sufficient since refs are typically stable. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/unuse-react/src/index.ts:136
- Draft comment:
unResolve uses React hooks (useReactState, useReactEffect). Ensure this API is documented to be used only within a component (hook) context. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. packages/unuse-solid/src/index.ts:197
- Draft comment:
Internal override functions are hidden via UNDEFINED as in other adapters. Consistency is maintained, but consider documenting this pattern. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative, suggesting documentation of a pattern. It doesn't provide a specific code suggestion or identify a potential issue with the code. It doesn't align with the rules for good comments.
6. packages/unuse-vue/src/index.ts:60
- Draft comment:
The toUnSignal implementation for Vue uses vueWatch and effect to synchronize changes. Ensure these reactive interactions are adequately documented for Vue users. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. vitest.config.ts:10
- Draft comment:
Coverage thresholds and exclusion patterns have been updated. Verify that these settings align with project requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify alignment with project requirements, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
8. packages/unuse-angular/src/index.spec.ts:94
- Draft comment:
The unResolve tests use 'framework: null as unknown as undefined' as a workaround. Consider addressing this type hack in a future revision for better type safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is pointing out something that's already documented in a TODO. The type hack is intentional and explained. The comment doesn't provide any specific solution, just suggests "addressing it in a future revision". This feels more like an observation than an actionable review comment. The type hack could indicate a design issue in the API that should be fixed. Maybe the comment is trying to elevate visibility of this issue? While the underlying API design could be improved, this comment doesn't provide concrete suggestions. The TODO already tracks this issue more specifically. Delete the comment since it's redundant with the existing TODO and doesn't provide actionable feedback.
9. packages/unuse-react/src/index.spec.ts:90
- Draft comment:
The test for unResolve in React also uses 'framework: null as unknown as undefined'. Consider revisiting this pattern to achieve a more type‐safe solution. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The type casting pattern is intentional and documented with a TODO. The comment suggests improving type safety but doesn't provide a concrete solution. The pattern is in test code, not production code. The TODO already acknowledges this needs to be revisited. The comment doesn't add significant value beyond what's already in the TODO. The type casting could be a legitimate type safety concern that should be addressed. The comment might help draw attention to technical debt. While type safety is important, the existing TODO comment already captures this issue and provides more context about why it's needed. The PR comment doesn't add actionable value. Delete the comment since the issue is already acknowledged by the TODO comment in the code, and the PR comment doesn't provide additional actionable guidance.
10. packages/unuse-solid/src/index.spec.ts:77
- Draft comment:
The unResolve test uses a similar 'framework: null as unknown as undefined' pattern. Document this workaround and consider a more robust type solution later. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. packages/unuse-vue/src/index.spec.ts:96
- Draft comment:
UnResolve tests in the Vue adapter use 'framework: null as unknown as undefined'. Plan to revisit this pattern to ensure type-safe API design. - Reason this comment was not posted:
Comment looked like it was already resolved.
12. .changeset/bitter-cameras-give.md:9
- Draft comment:
Typo: In the commit message on line 9, "isUnRef recognize also unSignal, unComputed and getter functions" may be missing an 's' after "recognize". Consider changing it to "recognizes" for correct subject-verb agreement. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is pointing out a typo in the commit message, which is not directly related to the code changes. The rules do not mention anything about commenting on commit messages, but the focus is on code changes. Therefore, this comment might not be necessary as it doesn't directly impact the code functionality or quality.
Workflow ID: wflow_cD8rMGOe79YtImTd
You can customize
|
@JounQin could you please do a review now? The AI's covered already many nitpicky stuff. Please not that I set some tests on purpose to Also I found an issue with the AI that the framework default fallback should not be This PR fixes the So IMO we should merge it soon, even with the |
ecd5437 to
423c65f
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 423c65f in 1 minute and 55 seconds. Click for details.
- Reviewed
1101lines of code in14files - Skipped
1files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/bitter-cameras-give.md:1
- Draft comment:
Proper changeset message; clearly indicates fix for isUnRef enhancements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the changeset message is proper, which does not help the PR author in any way.
2. .changeset/dirty-buttons-open.md:1
- Draft comment:
Changeset clearly communicates removal of internal override functions from the public API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states what the changeset does without offering any guidance or questions for the author.
3. packages/unuse-angular/package.json:33
- Draft comment:
Added devDependency '@angular/platform-browser'. Verify that its version (20.0.4) aligns with the Angular core requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is related to a dependency version check, which is not allowed according to the rules. It asks the author to verify the version alignment, which is similar to asking for confirmation or verification, which is also not allowed.
4. packages/unuse-angular/src/index.ts:185
- Draft comment:
The isUnRef function now accepts getters, unSignals, and unComputeds. Ensure that accepting any function as a getter does not lead to false positives with non-reactive functions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the change does not lead to false positives, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
5. packages/unuse-angular/src/index.spec.ts:122
- Draft comment:
Test for tryOnScopeDispose is marked as TODO. Ensure this callback behavior is implemented to verify Angular scope disposal in future updates. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that a test is implemented in the future, which is not specific enough and violates the rule against asking the author to ensure something is tested. It doesn't provide a specific suggestion or ask for a specific test to be written now.
6. packages/unuse-react/src/index.ts:60
- Draft comment:
Consider specifying a dependency array in useReactEffect for the state branch to avoid unnecessary re-renders and ensure controlled updates. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. packages/unuse-vue/src/index.ts:177
- Draft comment:
Vue's isUnRef function now includes checks for Vue refs and getters. This approach should ensure that both reactive refs and computed values are recognized. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change in the Vue's isUnRef function. It doesn't provide any actionable feedback or suggestions for improvement.
8. vitest.config.ts:10
- Draft comment:
Coverage thresholds and exclusion patterns have been updated to better reflect project testing goals. Verify these settings align with your quality standards. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify settings, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_TtjF8UqcESeaexp5
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/unuse-vue/src/index.spec.ts (1)
99-101: Framework parameter API issue tracked separately.The TODO comments about the framework parameter API design have been addressed in previous reviews and are tracked in issue #32 for future implementation.
packages/unuse-solid/src/index.spec.ts (1)
78-80: Framework parameter issue already tracked.The TODO comments about framework parameter API design are consistent with other framework adapters and tracked in a separate issue for future resolution.
packages/unuse-angular/src/index.spec.ts (1)
96-98: Framework parameter API design issue tracked.The TODO comments about the framework parameter API are consistent across all framework adapters and will be addressed in a separate PR as indicated in previous reviews.
🧹 Nitpick comments (1)
.changeset/bitter-cameras-give.md (1)
9-9: Fix grammatical error in changeset description.The verb should agree with the singular subject "isUnRef".
-fix: isUnRef recognize also unSignal, unComputed and getter functions +fix: isUnRef recognizes also unSignal, unComputed and getter functions
📜 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 (14)
.changeset/bitter-cameras-give.md(1 hunks).changeset/dirty-buttons-open.md(1 hunks)packages/unuse-angular/package.json(1 hunks)packages/unuse-angular/src/index.spec.ts(1 hunks)packages/unuse-angular/src/index.ts(3 hunks)packages/unuse-react/package.json(1 hunks)packages/unuse-react/src/index.spec.ts(1 hunks)packages/unuse-react/src/index.ts(4 hunks)packages/unuse-solid/package.json(1 hunks)packages/unuse-solid/src/index.spec.ts(1 hunks)packages/unuse-solid/src/index.ts(3 hunks)packages/unuse-vue/src/index.spec.ts(1 hunks)packages/unuse-vue/src/index.ts(3 hunks)vitest.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dirty-buttons-open.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/unuse-angular/package.json
- packages/unuse-solid/package.json
- vitest.config.ts
- packages/unuse-react/package.json
- packages/unuse-vue/src/index.ts
- packages/unuse-angular/src/index.ts
- packages/unuse-solid/src/index.ts
- packages/unuse-react/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/unuse-react/src/index.spec.ts (7)
packages/unuse-react/src/index.ts (4)
toUnSignal(221-221)unResolve(223-223)tryOnScopeDispose(222-222)isUnRef(216-216)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(80-82)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
packages/unuse-solid/src/index.spec.ts (7)
packages/unuse-solid/src/index.ts (4)
toUnSignal(213-213)unResolve(215-215)tryOnScopeDispose(214-214)isUnRef(208-208)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(80-82)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
🪛 LanguageTool
.changeset/bitter-cameras-give.md
[uncategorized] ~9-~9: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... patch 'unuse': patch --- fix: isUnRef recognize also unSignal, unComputed and getter fu...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 ESLint
packages/unuse-angular/src/index.spec.ts
[error] 8-8: Unable to resolve path to module '@angular/core'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@angular/core/testing'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@angular/platform-browser/testing'.
(import-x/no-unresolved)
[error] 14-14: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 28-28: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-react/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@testing-library/react'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-solid/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@solidjs/testing-library'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'solid-js'.
(import-x/no-unresolved)
[error] 5-5: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 19-19: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
packages/unuse-vue/src/index.spec.ts
[error] 6-6: Parse errors in imported module 'vitest': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
[error] 7-7: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 21-21: Parse errors in imported module '.': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.15.2_@typescript-eslint+utils@8.34.1_eslint@9.29.0_jiti@2.4.2__61f7c466241b03093c88d336110da01e/node_modules/eslint-plugin-import-x/lib/require.js (undefined:undefined)
(import-x/namespace)
🔇 Additional comments (10)
packages/unuse-react/src/index.spec.ts (3)
31-41: Review TODO tests for future implementation.The author has indicated these
it.todotests are intentionally failing and will be addressed in future work. This is an acceptable approach for incremental improvement.
188-193: Correctly verifies override functions are not exposed.This test properly confirms that internal override functions have been removed from the public API, which aligns with the PR's encapsulation improvements.
150-157: ```shell
#!/bin/bash
sed -n '140,170p' packages/unuse-react/src/index.spec.ts</details> <details> <summary>packages/unuse-vue/src/index.spec.ts (2)</summary> `57-64`: **Excellent Vue reactivity integration test.** This test properly verifies bidirectional synchronization between Vue refs and UnSignal objects, ensuring the adapter maintains reactivity correctly. --- `155-179`: **Comprehensive coverage of reactive type detection.** The `isUnRef` tests thoroughly verify recognition of Vue refs, computed refs, UnSignal, UnComputed, and getter functions while correctly rejecting non-reactive values. </details> <details> <summary>packages/unuse-solid/src/index.spec.ts (2)</summary> `31-38`: **Well-implemented Solid signal integration test.** This test correctly verifies that UnSignal can wrap Solid signals and maintain bidirectional reactivity using the tuple-based signal API `solidSignal[0]()` and the setter. --- `130-138`: **Solid-specific reactive type coverage.** The tests properly verify `isUnRef` recognition for Solid signals and memos, which are the framework-specific reactive primitives. </details> <details> <summary>packages/unuse-angular/src/index.spec.ts (3)</summary> `30-32`: **Proper Angular test module configuration.** The `ZonelessTestModule` is correctly defined as internal (not exported) and provides the necessary zoneless change detection for testing reactive utilities. --- `51-61`: **Excellent Angular injection context usage.** This test properly uses `getTestBed().runInInjectionContext()` to ensure Angular signals work correctly within the testing environment, and verifies bidirectional reactivity. --- `150-158`: **Comprehensive Angular reactive type coverage.** The tests correctly verify `isUnRef` recognition for Angular signals and computed values, which are the framework's native reactive primitives. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
423c65f to
d96421b
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d96421b in 1 minute and 56 seconds. Click for details.
- Reviewed
1085lines of code in14files - Skipped
1files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/bitter-cameras-give.md:1
- Draft comment:
Changeset message for 'isUnRef' fix is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .changeset/dirty-buttons-open.md:1
- Draft comment:
Changeset indicates removal of internal override functions from public API; message is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/unuse-angular/package.json:33
- Draft comment:
Added '@angular/platform-browser' in devDependencies appears appropriate for Angular tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/unuse-angular/src/index.spec.ts:1
- Draft comment:
Angular adapter tests comprehensively cover toUnSignal, unResolve, tryOnScopeDispose and isUnRef. The use of 'it.todo' for scope disposal indicates pending work. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/unuse-angular/src/index.ts:185
- Draft comment:
The updated isUnRef now checks for Angular signals, getters, and unSignal/unComputed types. Exporting internal override functions as undefined is a neat API encapsulation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/unuse-react/src/index.spec.ts:30
- Draft comment:
React adapter tests include TODOs for converting React state and ref to UnSignal; consider implementing or clarifying the expected behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. packages/unuse-react/src/index.ts:191
- Draft comment:
The React adapter's isUnRef function correctly checks for properties of refs (using 'current'), state arrays, getters, unSignal and unComputed values. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. packages/unuse-solid/src/index.spec.ts:103
- Draft comment:
In the Solid adapter test for tryOnScopeDispose, the hook result should use 'hook.result.current' instead of 'hook.result' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Looking at the test, it's working as-is withhook.result. The suggestion seems to be based on a convention from other testing libraries like React Testing Library, but there's no clear evidence that this is required for Solid's testing library. The test passes and is readable as written. The comment could be correct if Solid's testing library follows the same patterns as React Testing Library. I might be missing documentation that specifies this convention. Without clear evidence that.currentis required or that the current code is problematic, we should not force a change just for consistency with other libraries. Delete the comment since the code works as written and there's no strong evidence that the suggested change is necessary.
9. packages/unuse-solid/src/index.ts:182
- Draft comment:
Solid adapter implementation is consistent with other frameworks. Verify that the reactive synchronization in unResolve works as intended. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
10. packages/unuse-vue/src/index.spec.ts:56
- Draft comment:
Vue adapter tests comprehensively cover conversion of Vue refs, computed refs, and non-reactive values to UnSignal, as well as unResolve functionality. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. packages/unuse-vue/src/index.ts:48
- Draft comment:
Vue adapter correctly leverages vueWatch and reactivity utilities. Exporting internal override functions as undefined effectively hides implementation details. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. vitest.config.ts:10
- Draft comment:
Test configuration now excludes dist, config files, docs, and examples and has updated coverage thresholds, which improves overall test reporting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_A9LjRbvFPiVJdBUo
You can customize
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/unuse-vue/src/index.spec.ts (1)
36-45: LGTM - Well-structured test utility.The
useSetupfunction provides a clean way to test Vue composition API functions within a component context.Note: The previous comment about exporting from test files still applies, but this is acceptable for test utilities.
packages/unuse-solid/src/index.spec.ts (1)
54-66: LGTM - Intentional .todo test noted.The
.todotest is appropriately marked as per previous discussion about tests that currently fail due to implementation gaps outside this PR's scope.packages/unuse-angular/src/index.spec.ts (1)
118-130: Intentional .todo test noted.The
.todotest is appropriately marked as discussed in previous reviews, indicating functionality that needs to be implemented outside this PR's scope.packages/unuse-react/src/index.spec.ts (1)
31-31: Todo tests are intentionally incomplete.The todo tests are marked as incomplete intentionally due to current implementation limitations that need to be addressed in future work.
🧹 Nitpick comments (1)
.changeset/bitter-cameras-give.md (1)
9-9: Fix grammatical error in changeset description.The verb "recognize" should be "recognizes" to agree with the singular subject "isUnRef".
-fix: isUnRef recognize also unSignal, unComputed and getter functions +fix: isUnRef recognizes also unSignal, unComputed and getter functions
📜 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 (14)
.changeset/bitter-cameras-give.md(1 hunks).changeset/dirty-buttons-open.md(1 hunks)packages/unuse-angular/package.json(1 hunks)packages/unuse-angular/src/index.spec.ts(1 hunks)packages/unuse-angular/src/index.ts(3 hunks)packages/unuse-react/package.json(1 hunks)packages/unuse-react/src/index.spec.ts(1 hunks)packages/unuse-react/src/index.ts(4 hunks)packages/unuse-solid/package.json(1 hunks)packages/unuse-solid/src/index.spec.ts(1 hunks)packages/unuse-solid/src/index.ts(3 hunks)packages/unuse-vue/src/index.spec.ts(1 hunks)packages/unuse-vue/src/index.ts(3 hunks)vitest.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/unuse-react/package.json
- packages/unuse-angular/package.json
- packages/unuse-solid/package.json
- vitest.config.ts
- .changeset/dirty-buttons-open.md
- packages/unuse-vue/src/index.ts
- packages/unuse-solid/src/index.ts
- packages/unuse-angular/src/index.ts
- packages/unuse-react/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/unuse-angular/src/index.spec.ts (8)
packages/unuse/src/_testUtils/angular.ts (1)
NgModule(32-34)packages/unuse-angular/src/index.ts (4)
toUnSignal(211-211)unResolve(213-213)tryOnScopeDispose(212-212)isUnRef(206-206)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(72-74)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
packages/unuse-react/src/index.spec.ts (7)
packages/unuse-react/src/index.ts (4)
toUnSignal(221-221)unResolve(223-223)tryOnScopeDispose(222-222)isUnRef(216-216)packages/unuse/src/unSignal/index.ts (2)
unSignal(45-64)isUnSignal(69-79)packages/unuse/src/unComputed/index.ts (2)
isUnComputed(47-55)unComputed(32-42)packages/unuse/src/toUnSignal/index.ts (1)
overrideToUnSignalFn(17-19)packages/unuse/src/unResolve/index.ts (1)
overrideUnResolveFn(72-74)packages/unuse/src/tryOnScopeDispose/index.ts (1)
overrideTryOnScopeDisposeFn(10-14)packages/unuse/src/unAccess/index.ts (1)
overrideIsUnRefFn(44-46)
🪛 LanguageTool
.changeset/bitter-cameras-give.md
[uncategorized] ~9-~9: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... patch 'unuse': patch --- fix: isUnRef recognize also unSignal, unComputed and getter fu...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 ESLint
packages/unuse-angular/src/index.spec.ts
[error] 8-8: Unable to resolve path to module '@angular/core'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@angular/core/testing'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@angular/platform-browser/testing'.
(import-x/no-unresolved)
packages/unuse-react/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@testing-library/react'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
packages/unuse-solid/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@solidjs/testing-library'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'solid-js'.
(import-x/no-unresolved)
packages/unuse-vue/src/index.spec.ts
[error] 7-7: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
🔇 Additional comments (9)
packages/unuse-vue/src/index.spec.ts (2)
98-98: Good improvement on framework parameter API.The use of
framework: 'none'is much cleaner than the previousnull as unknown as undefinedcasting approach that was discussed in past reviews.Also applies to: 107-107
185-190: Excellent verification of API encapsulation.This test correctly verifies that internal override functions are not exposed in the public API, which aligns with the breaking change documented in the changeset.
packages/unuse-solid/src/index.spec.ts (2)
75-77: Consistent framework parameter API usage.Good use of
framework: 'none'which provides a cleaner API than the previous null casting approach.Also applies to: 85-87
160-165: Proper verification of API encapsulation.Correctly tests that internal override functions are not exposed, maintaining API cleanliness.
packages/unuse-angular/src/index.spec.ts (4)
30-32: Good fix - ZonelessTestModule no longer exported.The previous issue with exporting from a test file has been resolved by making the module internal to the test file.
53-61: Well-structured Angular integration test.Proper use of Angular's injection context to test signal interoperability with UnSignal.
95-95: Consistent framework parameter API.Good use of
framework: 'none'providing a cleaner API than previous null casting approaches.Also applies to: 104-104
180-185: Excellent API encapsulation verification.Properly verifies that internal override functions are not exposed in the public API, maintaining clean separation of concerns.
packages/unuse-react/src/index.spec.ts (1)
21-190: Excellent comprehensive test coverage for React adapter.This test suite provides thorough coverage of the React framework adapter functionality, properly testing all the core reactive utilities and confirming that internal override functions are not exposed publicly. The test structure follows good practices and aligns well with the framework adapter architecture.
d96421b to
d10fc99
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed d10fc99 in 2 minutes and 1 seconds. Click for details.
- Reviewed
1087lines of code in14files - Skipped
1files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/bitter-cameras-give.md:9
- Draft comment:
Changeset message correctly indicates a patch update for isUnRef enhancements. Ensure it aligns with your release strategy. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .changeset/dirty-buttons-open.md:9
- Draft comment:
The changeset indicates removal of internal override functions from the public API. Confirm this is intentional and documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/unuse-angular/src/index.ts:185
- Draft comment:
The isUnRef function now treats any function as a getter along with Angular signals and unSignals. Verify that treating all functions as reactive won’t produce false positives in non-reactive contexts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify that a change won't produce false positives, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
4. packages/unuse-angular/src/index.ts:203
- Draft comment:
Exporting internal override functions as undefined improves encapsulation. Consider adding documentation on this design choice for consumers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/unuse-react/src/index.ts:46
- Draft comment:
The toUnSignal function handles React refs by checking for a 'current' property. Ensure this approach covers any non-standard or custom ref implementations you may encounter. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/unuse-solid/src/index.spec.ts:104
- Draft comment:
Review the Solid test for tryOnScopeDispose. The assignment from hook.result is used directly—confirm that the testing utility returns the expected boolean value. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm the behavior of a test utility, which falls under asking the author to ensure the behavior is intended. This violates the rule against asking for confirmation or verification of behavior.
7. packages/unuse-vue/src/index.ts:48
- Draft comment:
The toUnSignal function for Vue uses vueWatch to sync the ref. Ensure that the two-way binding does not lead to update loops, especially when the ref is not read-only. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. vitest.config.ts:24
- Draft comment:
Coverage thresholds have been raised, which is positive for code quality. Verify that these thresholds align with the project’s standards. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. .changeset/bitter-cameras-give.md:9
- Draft comment:
Typographical note: The commit message says "isUnRef recognize also unSignal, unComputed and getter functions". Consider revising to use the correct verb form, e.g., "isUnRef recognizes...", for clarity and correctness. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and suggests a change to the commit message for clarity. It does not provide a code suggestion or address a potential issue in the code itself. According to the rules, purely informative comments should be removed.
10. packages/unuse-react/src/index.spec.ts:158
- Draft comment:
Typo: In the comment, "cant" should be "can't". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically this is about "changed" code since it's a new file, comments about typos in code comments are extremely minor and don't affect functionality. The rules state we should not make obvious or unimportant comments. A missing apostrophe in a comment is definitely unimportant. The code is new, so technically all changes are fair game. Also, maintaining good documentation quality could be considered important. While documentation quality matters, a missing apostrophe doesn't impact readability or understanding. This is exactly the kind of nitpicky comment that creates noise in code reviews. Delete this comment. It's too minor and doesn't affect code quality or functionality.
Workflow ID: wflow_nWWJugNMS0fF6ok6
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/bitter-cameras-give.md (1)
9-9: Fix grammar in changeset description.The verb should agree with the singular subject "isUnRef".
Apply this diff to fix the grammar:
-fix: isUnRef recognize also unSignal, unComputed and getter functions +fix: isUnRef recognizes also unSignal, unComputed and getter functions
📜 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 (14)
.changeset/bitter-cameras-give.md(1 hunks).changeset/dirty-buttons-open.md(1 hunks)packages/unuse-angular/package.json(1 hunks)packages/unuse-angular/src/index.spec.ts(1 hunks)packages/unuse-angular/src/index.ts(3 hunks)packages/unuse-react/package.json(1 hunks)packages/unuse-react/src/index.spec.ts(1 hunks)packages/unuse-react/src/index.ts(4 hunks)packages/unuse-solid/package.json(1 hunks)packages/unuse-solid/src/index.spec.ts(1 hunks)packages/unuse-solid/src/index.ts(3 hunks)packages/unuse-vue/src/index.spec.ts(1 hunks)packages/unuse-vue/src/index.ts(3 hunks)vitest.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/unuse-react/package.json
- packages/unuse-solid/package.json
- .changeset/dirty-buttons-open.md
- vitest.config.ts
- packages/unuse-angular/package.json
- packages/unuse-react/src/index.ts
- packages/unuse-angular/src/index.ts
- packages/unuse-vue/src/index.ts
- packages/unuse-solid/src/index.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/bitter-cameras-give.md
[uncategorized] ~9-~9: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... patch 'unuse': patch --- fix: isUnRef recognize also unSignal, unComputed and getter fu...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 ESLint
packages/unuse-angular/src/index.spec.ts
[error] 8-8: Unable to resolve path to module '@angular/core'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@angular/core/testing'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@angular/platform-browser/testing'.
(import-x/no-unresolved)
packages/unuse-react/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@testing-library/react'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
packages/unuse-solid/src/index.spec.ts
[error] 3-3: Unable to resolve path to module '@solidjs/testing-library'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module 'solid-js'.
(import-x/no-unresolved)
packages/unuse-vue/src/index.spec.ts
[error] 7-7: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
🔇 Additional comments (7)
packages/unuse-react/src/index.spec.ts (1)
1-193: Comprehensive test suite looks good.The test coverage is thorough, testing all major reactive utilities and confirming that override functions are properly hidden from the public API. The intentional
.todotests are acceptable for incremental improvement.packages/unuse-vue/src/index.spec.ts (2)
36-45: Test utility function is appropriately scoped.The
useSetupfunction is a legitimate Vue testing utility that's correctly implemented for component mounting and testing reactive behavior within Vue's composition API context.
98-98: Good resolution of framework parameter API.Using
framework: 'none'instead of the previousnull as unknown as undefinedcasting provides a much cleaner and more explicit API.Also applies to: 107-107
packages/unuse-solid/src/index.spec.ts (1)
1-167: Well-structured Solid test suite.The tests properly utilize SolidJS testing utilities and comprehensively cover the reactive utilities. The framework parameter API is consistent and the intentional
.todotest is acceptable for incremental development.packages/unuse-angular/src/index.spec.ts (3)
30-32: Good fix for test module encapsulation.The
ZonelessTestModuleis now correctly kept internal to the test file, addressing the previous concern about exports from test files.
95-95: Consistent framework parameter API.Using
framework: 'none'provides a clean and explicit API that's consistent across all framework adapters.Also applies to: 104-104
53-60: Proper Angular injection context usage.The test correctly uses
getTestBed().runInInjectionContext()to ensure Angular signals work properly within the testing environment.
well... I'm glad I start covering these 😅
Important
Add test coverage for framework adapters, improve
isUnRefdetection, and remove internal functions from public API.index.spec.tsfiles to verify reactive utility functions and framework integration.isUnRefnow recognizesunSignal,unComputed, and getter functions inindex.tsfor Angular, React, Solid, and Vue.index.tsfor Angular, React, Solid, and Vue.package.jsonfor Angular, React, and Solid.vitest.config.ts.This description was created by
for d10fc99. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor