rename internal useForm so it is not exposed to user#30
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR refactors the form client API from a public export to an internal export, renaming the factory from ChangesForm Client API Refactor to Internal Export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/forms/tests/contracts.test.ts (1)
1125-1126: 💤 Low valueLGTM — export surface assertions are correct.
The two new contract assertions precisely encode the intent:
./clientmust no longer be a public export, and./internal/clientmust exist. Because the typed cast already marksexportsas optional, if the field were ever absent frompackage.json, Vitest would throw ontoHavePropertyrather than produce a readable failure. In practice this is fine since the field is always present, but the narrower castexports: Record<string, unknown>(non-optional) would make the intent explicit and surface any regression more clearly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/forms/tests/contracts.test.ts` around lines 1125 - 1126, The test currently treats packageJson.exports as optional which hides absence of the exports field; update the type assertion in packages/forms/tests/contracts.test.ts so that packageJson.exports is typed as a non-optional Record<string, unknown> (e.g., use "exports: Record<string, unknown>") instead of an optional type, then keep the existing assertions on packageJson.exports (expect(...).not.toHaveProperty('./client') and expect(...).toHaveProperty('./internal/client')) so any regression where exports is missing fails clearly.packages/adapter-sveltekit/tests/package.test.ts (1)
21-21: 💤 Low valueConsider adding a negative assertion for the old
@holo-js/forms/clientpath.The positive check on Line 21 is sufficient for current correctness, but a
not.toContain("@holo-js/forms/client")guard (being careful about the substring —@holo-js/forms/clientis not a substring of@holo-js/forms/internal/client) would prevent a future regression where the old public path is accidentally re-introduced alongside the internal one.♻️ Proposed addition
expect(clientEntry).toContain("@holo-js/forms/internal/client") + expect(clientEntry).not.toContain('"@holo-js/forms/client"')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/adapter-sveltekit/tests/package.test.ts` at line 21, Add a negative assertion to the test that ensures the old public import path is not present: after the existing positive assertion on clientEntry, add expect(clientEntry).not.toContain("@holo-js/forms/client") to guard against accidentally reintroducing the old public path; reference the same test variable clientEntry in packages/adapter-sveltekit/tests/package.test.ts and make sure the string is the exact old path (not a substring of "@holo-js/forms/internal/client").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adapter-nuxt/src/runtime/composables/forms.ts`:
- Line 224: The current watchEffect recreates and assigns form.value while also
reading it, causing a reactive dependency loop; instead, inside the watchEffect
create a non-reactive localForm by calling createFormClient(schemaDefinition,
options), pass that localForm to syncValuesFromForm (so syncValuesFromForm does
not read form.value), then assign form.value = localForm once syncing is
complete; update references to use localForm in the effect body and keep
createFormClient only called there (remove reliance on the initially created
shallowRef value) so the effect no longer reads and writes the same reactive
ref.
---
Nitpick comments:
In `@packages/adapter-sveltekit/tests/package.test.ts`:
- Line 21: Add a negative assertion to the test that ensures the old public
import path is not present: after the existing positive assertion on
clientEntry, add expect(clientEntry).not.toContain("@holo-js/forms/client") to
guard against accidentally reintroducing the old public path; reference the same
test variable clientEntry in packages/adapter-sveltekit/tests/package.test.ts
and make sure the string is the exact old path (not a substring of
"@holo-js/forms/internal/client").
In `@packages/forms/tests/contracts.test.ts`:
- Around line 1125-1126: The test currently treats packageJson.exports as
optional which hides absence of the exports field; update the type assertion in
packages/forms/tests/contracts.test.ts so that packageJson.exports is typed as a
non-optional Record<string, unknown> (e.g., use "exports: Record<string,
unknown>") instead of an optional type, then keep the existing assertions on
packageJson.exports (expect(...).not.toHaveProperty('./client') and
expect(...).toHaveProperty('./internal/client')) so any regression where exports
is missing fails clearly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a3fd3d-b322-4c58-970b-c383736e87d6
📒 Files selected for processing (21)
packages/adapter-next/src/client.tspackages/adapter-next/tests/client.test.tspackages/adapter-next/tests/package.test.tspackages/adapter-next/tsconfig.jsonpackages/adapter-next/vitest.config.tspackages/adapter-nuxt/src/runtime/composables/forms.d.tspackages/adapter-nuxt/src/runtime/composables/forms.tspackages/adapter-nuxt/tests/package.test.tspackages/adapter-nuxt/tsconfig.jsonpackages/adapter-nuxt/vitest.config.tspackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/tests/package.test.tspackages/adapter-sveltekit/tsconfig.jsonpackages/adapter-sveltekit/vitest.config.tspackages/forms/package.jsonpackages/forms/src/internal/client.tspackages/forms/tests/client.test.tspackages/forms/tests/client.type.test.tspackages/forms/tests/contracts.test.tspackages/forms/tests/docs-examples.test.tspackages/forms/tsup.config.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Tests