Add @formisch/angular package with playground#112
Conversation
dosubot
Bot
added
the
size:M
|
@sonukapoor is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds the Suggested reviewers
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds initial scaffolding for the @formisch/angular package, including build/test/lint tooling and a minimal source layout.
Changes:
- Introduces Vitest (with jsdom + setup), ESLint flat config, and TypeScript config for the Angular package
- Adds
tsdownbuild configuration and package metadata for publishingdist/ - Creates initial source entrypoints (
src/index.tsand empty barrels)
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frameworks/angular/vitest.config.ts | Adds Vitest config incl. jsdom, setup file, and coverage settings |
| frameworks/angular/tsdown.config.ts | Adds tsdown build configuration and DTS resolution hints |
| frameworks/angular/tsconfig.json | Adds TS compiler settings for the package |
| frameworks/angular/src/vitest/setup.ts | Adds Angular TestBed cleanup after each test |
| frameworks/angular/src/types/index.ts | Adds placeholder types barrel |
| frameworks/angular/src/index.ts | Adds package public entry barrel exports |
| frameworks/angular/src/functions/index.ts | Adds placeholder functions barrel |
| frameworks/angular/src/components/index.ts | Adds placeholder components barrel |
| frameworks/angular/package.json | Adds package metadata, scripts, exports, and dependencies |
| frameworks/angular/eslint.config.ts | Adds ESLint flat configuration for TS + security + JSDoc |
| frameworks/angular/CHANGELOG.md | Adds initial changelog file |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frameworks/angular/tsconfig.json (1)
17-17: 💤 Low valueConsider including the tsdown config for type-checking consistency.
The
includearray only lists"src", but the core package includes both"src"and"./tsdown.config.ts". Including the build config ensures TypeScript type-checks your build configuration alongside source code.📝 Suggested addition
- "include": ["src"] + "include": ["src", "./tsdown.config.ts"]🤖 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 `@frameworks/angular/tsconfig.json` at line 17, The tsconfig.json's "include" currently only lists "src", so add the build config file to ensure type-checking of the build config as well: update the "include" array to include "./tsdown.config.ts" (i.e., ensure the "include" key in tsconfig.json contains both "src" and "./tsdown.config.ts") so TypeScript will type-check the tsdown.config.ts alongside your source.frameworks/angular/CHANGELOG.md (1)
3-3: 💤 Low valuePlaceholder date in version header.
The version header contains
(Month DD, YYYY)instead of an actual date. Consider using the PR date (May 24, 2026) or aTBDplaceholder until the package is ready for release.🤖 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 `@frameworks/angular/CHANGELOG.md` at line 3, Replace the placeholder date in the changelog header string "## v0.0.1 (Month DD, YYYY)" with a real date or TBD; update the header to either "## v0.0.1 (May 24, 2026)" or "## v0.0.1 (TBD)" so the version header is not using the literal placeholder.frameworks/angular/eslint.config.ts (1)
6-28: ⚖️ Poor tradeoffConsider using the shared
createSourceConfighelper for consistency.The ESLint configuration manually assembles all config objects, but the core package (shown in context) uses
baseConfigsandcreateSourceConfighelpers from@formisch/eslint-config. Using these shared helpers would:
- Reduce duplication across framework packages
- Ensure consistent linting rules across the monorepo
- Simplify future updates to shared configuration
The current manual approach works but may drift from repository conventions over time.
Example using shared helpers (verify exports first)
-import eslint from '`@eslint/js`'; -import { commonRules, importPlugin, jsdoc } from '`@formisch/eslint-config`'; -import pluginSecurity from 'eslint-plugin-security'; +import { + baseConfigs, + createSourceConfig, + tseslint, +} from '`@formisch/eslint-config`'; -import tseslint from 'typescript-eslint'; export default tseslint.config( - { ignores: ['eslint.config.ts', 'dist', 'coverage'] }, - eslint.configs.recommended, - jsdoc.configs['flat/recommended'], - pluginSecurity.configs.recommended, - ...tseslint.configs.strictTypeChecked, - ...tseslint.configs.stylisticTypeChecked, - { - name: 'app/files-to-lint', - files: ['src/**/*.ts'], - extends: [importPlugin.flatConfigs.recommended], - plugins: { jsdoc }, - languageOptions: { - parserOptions: { - project: true, - tsconfigRootDir: import.meta.dirname, - }, - }, - rules: { - ...commonRules, - }, - } + { ignores: ['eslint.config.ts', 'dist', 'coverage'] }, + ...baseConfigs, + createSourceConfig({ + tsconfigRootDir: import.meta.dirname, + files: ['src/**/*.ts'], + extraRules: { + // Angular-specific rules if needed + }, + }) );Note: Verify that
baseConfigsincludes the security plugin if that's required for Angular.🤖 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 `@frameworks/angular/eslint.config.ts` around lines 6 - 28, The file builds an ESLint source config manually via tseslint.config and duplicates shared settings; replace this block by using the repo helper createSourceConfig (and baseConfigs) from `@formisch/eslint-config` to create the 'app/files-to-lint' source config so the package reuses shared baseConfigs and avoids drift. Specifically, import createSourceConfig (and baseConfigs) and refactor the tseslint.config(...) call that references name 'app/files-to-lint', files ['src/**/*.ts'], extends importPlugin.flatConfigs.recommended, plugins { jsdoc }, languageOptions.parserOptions.project/tsconfigRootDir and rules {...commonRules} into a single createSourceConfig invocation that merges baseConfigs and your overrides (ensure baseConfigs includes the security plugin if needed).
🤖 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.
Nitpick comments:
In `@frameworks/angular/CHANGELOG.md`:
- Line 3: Replace the placeholder date in the changelog header string "## v0.0.1
(Month DD, YYYY)" with a real date or TBD; update the header to either "##
v0.0.1 (May 24, 2026)" or "## v0.0.1 (TBD)" so the version header is not using
the literal placeholder.
In `@frameworks/angular/eslint.config.ts`:
- Around line 6-28: The file builds an ESLint source config manually via
tseslint.config and duplicates shared settings; replace this block by using the
repo helper createSourceConfig (and baseConfigs) from `@formisch/eslint-config` to
create the 'app/files-to-lint' source config so the package reuses shared
baseConfigs and avoids drift. Specifically, import createSourceConfig (and
baseConfigs) and refactor the tseslint.config(...) call that references name
'app/files-to-lint', files ['src/**/*.ts'], extends
importPlugin.flatConfigs.recommended, plugins { jsdoc },
languageOptions.parserOptions.project/tsconfigRootDir and rules {...commonRules}
into a single createSourceConfig invocation that merges baseConfigs and your
overrides (ensure baseConfigs includes the security plugin if needed).
In `@frameworks/angular/tsconfig.json`:
- Line 17: The tsconfig.json's "include" currently only lists "src", so add the
build config file to ensure type-checking of the build config as well: update
the "include" array to include "./tsdown.config.ts" (i.e., ensure the "include"
key in tsconfig.json contains both "src" and "./tsdown.config.ts") so TypeScript
will type-check the tsdown.config.ts alongside your source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b3b8ef8-0cb0-403a-814c-be8634e27dc7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
frameworks/angular/CHANGELOG.mdframeworks/angular/eslint.config.tsframeworks/angular/package.jsonframeworks/angular/src/components/index.tsframeworks/angular/src/functions/index.tsframeworks/angular/src/index.tsframeworks/angular/src/types/index.tsframeworks/angular/src/vitest/setup.tsframeworks/angular/tsconfig.jsonframeworks/angular/tsdown.config.tsframeworks/angular/vitest.config.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
sonukapoor
force-pushed
the
feat/111-angular-package-skeleton
branch
from
f364fa9 to
7f11d22
Compare
sonukapoor
changed the title
sonukapoor
force-pushed
the
feat/111-angular-package-skeleton
branch
from
80f2926 to
4f8f1b1
Compare
dosubot
Bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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
`@frameworks/angular/src/components/FormischFieldArray/FormischFieldArray.test.ts`:
- Around line 36-46: The tests creating TestHost (in FormischFieldArray.test.ts)
call TestBed.createComponent(TestHost) and await fixture.whenStable() but never
run initial change detection, causing flaky DOM assertions; update both tests to
call fixture.detectChanges() immediately after TestBed.createComponent(TestHost)
(before awaiting whenStable() or before querying the DOM) so the template
renders and the assertions on the span (data-testid="count") and its textContent
are deterministic.
In `@frameworks/angular/src/components/FormischFieldArray/FormischFieldArray.ts`:
- Around line 54-57: The two signal inputs in FormischFieldArray (the properties
named of and path) currently bypass type safety by using `@Input`({ isSignal: true
} as any) and eslint disables; remove those `@Input` casts and the no-explicit-any
comments and replace them with type-safe readonly signal-input declarations
using input.required for each property (i.e., declare of as an
InputSignal<FormStore<TSchema>> = input.required<FormStore<TSchema>>() and path
as an InputSignal<ValidArrayPath<v.InferInput<TSchema>, TFieldArrayPath>> =
input.required<ValidArrayPath<v.InferInput<TSchema>, TFieldArrayPath>>()) so the
inputs follow the same pattern used in FormischField and FormischForm.
In `@frameworks/angular/src/functions/injectFieldArray/injectFieldArray.ts`:
- Around line 30-50: The // `@__NO_SIDE_EFFECTS__` annotation is placed before the
implementation but should precede the first overload for tree-shaking; move the
comment so it appears immediately above the first overload declaration of
injectFieldArray (the overloaded export function signature for
injectFieldArray<TSchema, TFieldArrayPath>) rather than below it or before the
implementation, ensuring the pure factory hint applies to the overloaded
declaration.
- Line 41: The inline ts-expect-error in injectFieldArray.ts is suppressing a
TypeScript error without context; replace the bare "// `@ts-expect-error`" with a
short comment immediately above that explains which specific type mismatch or
runtime condition is expected and why it is safe (e.g., incompatible generic,
third‑party API types, or narrow-time assertion), and reference the related
symbol (injectFieldArray function and the variable or expression being
cast/suppressed) so future maintainers understand the justification and can
remove it if the underlying types are fixed.
In `@playgrounds/angular/src/components/text-input.component.ts`:
- Around line 27-39: The aria-errormessage attribute is always set to name() +
'-error' even when the error div (rendered by errors()) does not exist; change
the binding so that [attr.aria-errormessage] evaluates to name() + '-error' only
when errors() is truthy and to null otherwise (use a ternary or conditional
expression referencing name() and errors()) so the attribute is removed when
there are no errors.
In `@playgrounds/angular/src/routes/login/login.component.ts`:
- Around line 76-78: The submit handler handleSubmit currently logs the entire
validated payload (v.InferOutput<typeof LoginSchema>) which contains sensitive
fields like email/password; remove the console.log(output) and avoid emitting
credential data to the console or telemetry. Instead, if you need diagnostics,
log only non-sensitive state (e.g., a success flag or masked user id) or send a
safe event (no credentials) from handleSubmit, keeping the LoginSchema output
private.
In `@playgrounds/angular/src/routes/payment/payment.component.ts`:
- Around line 178-180: The handleSubmit implementation logs the full payment
payload (function handleSubmit and schema PaymentSchema); remove the console.log
and instead either log a non-sensitive success message or build a redacted
object that excludes or masks sensitive fields (card number, CVV, email, owner,
etc.) before logging; update handleSubmit to process the payload securely (send
to backend or tokenization) and only log minimal, non-sensitive metadata (e.g.,
success/transaction id) or the redacted payload.
🪄 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: 89370379-8aea-489a-ab2f-571d460fb9ea
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.gitignoreframeworks/angular/src/components/FormischField/FormischField.test.tsframeworks/angular/src/components/FormischField/FormischField.tsframeworks/angular/src/components/FormischField/index.tsframeworks/angular/src/components/FormischFieldArray/FormischFieldArray.test.tsframeworks/angular/src/components/FormischFieldArray/FormischFieldArray.tsframeworks/angular/src/components/FormischFieldArray/index.tsframeworks/angular/src/components/FormischForm/FormischForm.test.tsframeworks/angular/src/components/FormischForm/FormischForm.tsframeworks/angular/src/components/FormischForm/index.tsframeworks/angular/src/components/index.tsframeworks/angular/src/functions/index.tsframeworks/angular/src/functions/injectField/index.tsframeworks/angular/src/functions/injectField/injectField.test-d.tsframeworks/angular/src/functions/injectField/injectField.test.tsframeworks/angular/src/functions/injectField/injectField.tsframeworks/angular/src/functions/injectFieldArray/index.tsframeworks/angular/src/functions/injectFieldArray/injectFieldArray.test-d.tsframeworks/angular/src/functions/injectFieldArray/injectFieldArray.test.tsframeworks/angular/src/functions/injectFieldArray/injectFieldArray.tsframeworks/angular/src/functions/injectForm/index.tsframeworks/angular/src/functions/injectForm/injectForm.test-d.tsframeworks/angular/src/functions/injectForm/injectForm.test.tsframeworks/angular/src/functions/injectForm/injectForm.tsframeworks/angular/src/index.tsframeworks/angular/src/types/field.tsframeworks/angular/src/types/form.tsframeworks/angular/src/types/index.tsframeworks/angular/src/vitest/setup.tspackages/methods/package.jsonpackages/methods/tsdown.config.tsplaygrounds/angular/index.htmlplaygrounds/angular/package.jsonplaygrounds/angular/src/app.component.tsplaygrounds/angular/src/app.config.tsplaygrounds/angular/src/app.routes.tsplaygrounds/angular/src/components/form-footer.component.tsplaygrounds/angular/src/components/form-header.component.tsplaygrounds/angular/src/components/text-input.component.tsplaygrounds/angular/src/global.cssplaygrounds/angular/src/main.tsplaygrounds/angular/src/routes/login/login.component.tsplaygrounds/angular/src/routes/payment/payment.component.tsplaygrounds/angular/src/routes/todos/todos.component.tsplaygrounds/angular/tsconfig.app.jsonplaygrounds/angular/tsconfig.jsonplaygrounds/angular/vite.config.ts
✅ Files skipped from review due to trivial changes (10)
- frameworks/angular/src/functions/injectForm/index.ts
- .gitignore
- playgrounds/angular/tsconfig.json
- playgrounds/angular/vite.config.ts
- playgrounds/angular/src/main.ts
- frameworks/angular/src/components/FormischFieldArray/index.ts
- playgrounds/angular/src/components/form-header.component.ts
- frameworks/angular/src/functions/injectFieldArray/index.ts
- frameworks/angular/src/components/FormischForm/index.ts
- frameworks/angular/src/functions/injectField/index.ts
| const fixture = TestBed.createComponent(TestHost); | ||
| await fixture.whenStable(); | ||
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | ||
| expect(span).not.toBeNull(); | ||
| }); | ||
|
|
||
| it('passes items signal with initial empty array', async () => { | ||
| const fixture = TestBed.createComponent(TestHost); | ||
| await fixture.whenStable(); | ||
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | ||
| expect(span.textContent.trim()).toBe('0'); |
There was a problem hiding this comment.
Run initial change detection before DOM assertions
Add fixture.detectChanges() right after TestBed.createComponent(TestHost) in both tests; await fixture.whenStable() doesn’t perform the initial template render, so the DOM assertions can be flaky/missing the expected text.
Suggested fix
it('renders the template with the field array store', async () => {
const fixture = TestBed.createComponent(TestHost);
+ fixture.detectChanges();
await fixture.whenStable();
const span = fixture.nativeElement.querySelector('[data-testid="count"]');
expect(span).not.toBeNull();
});
it('passes items signal with initial empty array', async () => {
const fixture = TestBed.createComponent(TestHost);
+ fixture.detectChanges();
await fixture.whenStable();
const span = fixture.nativeElement.querySelector('[data-testid="count"]');
expect(span.textContent.trim()).toBe('0');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fixture = TestBed.createComponent(TestHost); | |
| await fixture.whenStable(); | |
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | |
| expect(span).not.toBeNull(); | |
| }); | |
| it('passes items signal with initial empty array', async () => { | |
| const fixture = TestBed.createComponent(TestHost); | |
| await fixture.whenStable(); | |
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | |
| expect(span.textContent.trim()).toBe('0'); | |
| const fixture = TestBed.createComponent(TestHost); | |
| fixture.detectChanges(); | |
| await fixture.whenStable(); | |
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | |
| expect(span).not.toBeNull(); | |
| }); | |
| it('passes items signal with initial empty array', async () => { | |
| const fixture = TestBed.createComponent(TestHost); | |
| fixture.detectChanges(); | |
| await fixture.whenStable(); | |
| const span = fixture.nativeElement.querySelector('[data-testid="count"]'); | |
| expect(span.textContent.trim()).toBe('0'); |
🤖 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
`@frameworks/angular/src/components/FormischFieldArray/FormischFieldArray.test.ts`
around lines 36 - 46, The tests creating TestHost (in
FormischFieldArray.test.ts) call TestBed.createComponent(TestHost) and await
fixture.whenStable() but never run initial change detection, causing flaky DOM
assertions; update both tests to call fixture.detectChanges() immediately after
TestBed.createComponent(TestHost) (before awaiting whenStable() or before
querying the DOM) so the template renders and the assertions on the span
(data-testid="count") and its textContent are deterministic.
There was a problem hiding this comment.
I would first get the implementation right before looking to closely at the test
There was a problem hiding this comment.
@fabian-hiller, understood! Feel free to revisit the detectChanges() addition once the implementation is solid.
There was a problem hiding this comment.
Agreed - we've now fixed the implementation to use proper signal inputs with input.required() and contentChild() for AOT compatibility, replacing the @Input({ isSignal: true } as any) pattern. We'll revisit the tests once the implementation is solid.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /** | ||
| * Creates a reactive field array store for a specific array field within a form store. | ||
| * Exposes all reactive state as Angular Signals callable in templates. | ||
| * | ||
| * Must be called in an injection context (component constructor or field initializer). | ||
| * | ||
| * @param form The form store instance. | ||
| * @param config The field array configuration. | ||
| * | ||
| * @returns The field array store with reactive Signal properties. | ||
| */ | ||
| // @ts-expect-error | ||
| export function injectFieldArray< | ||
| TSchema extends Schema, | ||
| TFieldArrayPath extends RequiredPath, | ||
| >( | ||
| form: FormStore<TSchema>, | ||
| config: InjectFieldArrayConfig<TSchema, TFieldArrayPath> | ||
| ): FieldArrayStore<TSchema, TFieldArrayPath>; | ||
|
|
||
| // @__NO_SIDE_EFFECTS__ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move // @NO_SIDE_EFFECTS`` comment before the first overload.
Per coding guidelines, the // @NO_SIDE_EFFECTS`` comment must be placed before pure factory functions for tree-shaking optimization. For overloaded functions, it should appear before the first overload declaration, not before the implementation. This ensures bundlers correctly identify the function as side-effect-free.
♻️ Corrected placement
/**
* Creates a reactive field array store for a specific array field within a form store.
* Exposes all reactive state as Angular Signals callable in templates.
*
* Must be called in an injection context (component constructor or field initializer).
*
* `@param` form The form store instance.
* `@param` config The field array configuration.
*
* `@returns` The field array store with reactive Signal properties.
*/
+// `@__NO_SIDE_EFFECTS__`
// `@ts-expect-error`
export function injectFieldArray<
TSchema extends Schema,
TFieldArrayPath extends RequiredPath,
>(
form: FormStore<TSchema>,
config: InjectFieldArrayConfig<TSchema, TFieldArrayPath>
): FieldArrayStore<TSchema, TFieldArrayPath>;
-// `@__NO_SIDE_EFFECTS__`
export function injectFieldArray(As per coding guidelines: "Add // @NO_SIDE_EFFECTS`` comment before pure factory functions for tree-shaking optimization"
🤖 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 `@frameworks/angular/src/functions/injectFieldArray/injectFieldArray.ts` around
lines 30 - 50, The // `@__NO_SIDE_EFFECTS__` annotation is placed before the
implementation but should precede the first overload for tree-shaking; move the
comment so it appears immediately above the first overload declaration of
injectFieldArray (the overloaded export function signature for
injectFieldArray<TSchema, TFieldArrayPath>) rather than below it or before the
implementation, ensuring the pure factory hint applies to the overloaded
declaration.
| * | ||
| * @returns The field array store with reactive Signal properties. | ||
| */ | ||
| // @ts-expect-error |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the reason for suppressing type errors.
The // @ts-expect-error`` comment suppresses TypeScript errors without explanation. Add a comment explaining why the type error is expected and safe to ignore.
📝 Suggested improvement
+// TypeScript cannot infer the return type from the generic overload,
+// but the implementation signature ensures type safety at runtime
// `@ts-expect-error`
export function injectFieldArray<📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-expect-error | |
| // TypeScript cannot infer the return type from the generic overload, | |
| // but the implementation signature ensures type safety at runtime | |
| // `@ts-expect-error` | |
| export function injectFieldArray< |
🤖 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 `@frameworks/angular/src/functions/injectFieldArray/injectFieldArray.ts` at
line 41, The inline ts-expect-error in injectFieldArray.ts is suppressing a
TypeScript error without context; replace the bare "// `@ts-expect-error`" with a
short comment immediately above that explains which specific type mismatch or
runtime condition is expected and why it is safe (e.g., incompatible generic,
third‑party API types, or narrow-time assertion), and reference the related
symbol (injectFieldArray function and the variable or expression being
cast/suppressed) so future maintainers understand the justification and can
remove it if the underlying types are fixed.
There was a problem hiding this comment.
Understood, @fabian-hiller — acknowledged and moving on!
| [attr.aria-errormessage]="name() + '-error'" | ||
| [class]="inputClass()" | ||
| (focus)="fieldFocus.emit($event)" | ||
| (change)="fieldChange.emit($event)" | ||
| (blur)="fieldBlur.emit($event)" | ||
| /> | ||
| @if (errors()) { | ||
| <div | ||
| [id]="name() + '-error'" | ||
| class="pt-4 text-sm text-red-500 md:text-base lg:pt-5 lg:text-lg dark:text-red-400" | ||
| > | ||
| {{ errors()![0] }} | ||
| </div> |
There was a problem hiding this comment.
Make aria-errormessage conditional on actual error presence.
Right now it always points to name() + '-error', but that element only exists when errors() is truthy. Set the attribute to null when there are no errors.
Suggested fix
- [attr.aria-errormessage]="name() + '-error'"
+ [attr.aria-errormessage]="errors() ? name() + '-error' : null"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [attr.aria-errormessage]="name() + '-error'" | |
| [class]="inputClass()" | |
| (focus)="fieldFocus.emit($event)" | |
| (change)="fieldChange.emit($event)" | |
| (blur)="fieldBlur.emit($event)" | |
| /> | |
| @if (errors()) { | |
| <div | |
| [id]="name() + '-error'" | |
| class="pt-4 text-sm text-red-500 md:text-base lg:pt-5 lg:text-lg dark:text-red-400" | |
| > | |
| {{ errors()![0] }} | |
| </div> | |
| [attr.aria-errormessage]="errors() ? name() + '-error' : null" | |
| [class]="inputClass()" | |
| (focus)="fieldFocus.emit($event)" | |
| (change)="fieldChange.emit($event)" | |
| (blur)="fieldBlur.emit($event)" | |
| /> | |
| `@if` (errors()) { | |
| <div | |
| [id]="name() + '-error'" | |
| class="pt-4 text-sm text-red-500 md:text-base lg:pt-5 lg:text-lg dark:text-red-400" | |
| > | |
| {{ errors()![0] }} | |
| </div> |
🤖 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 `@playgrounds/angular/src/components/text-input.component.ts` around lines 27 -
39, The aria-errormessage attribute is always set to name() + '-error' even when
the error div (rendered by errors()) does not exist; change the binding so that
[attr.aria-errormessage] evaluates to name() + '-error' only when errors() is
truthy and to null otherwise (use a ternary or conditional expression
referencing name() and errors()) so the attribute is removed when there are no
errors.
There was a problem hiding this comment.
Such things doesn't matter to much for the playground. But feel free to improve it if you agree.
There was a problem hiding this comment.
@fabian-hiller, that's fair — for a playground the strict ARIA correctness isn't critical. The fix is a genuine (if minor) accessibility improvement, so I'd lean toward applying it since the change is trivial:
- [attr.aria-errormessage]="name() + '-error'"
+ [attr.aria-errormessage]="errors() ? name() + '-error' : null"But totally happy to leave it as-is if you'd rather keep the playground code simple. Your call!
| `, | ||
| }) | ||
| export class LoginComponent { | ||
| readonly form = injectForm({ schema: LoginSchema, validate: 'blur' }); |
There was a problem hiding this comment.
I tried to change the config to revalidate: 'input' but it doesn't work. It only runs the validation on blur events.
There was a problem hiding this comment.
Fixed in e3cad10. The root cause was the Angular template binding to the DOM (change) event instead of (input). The change event only fires on blur for text inputs, so validate: 'input' had no effect - validation only ran on blur regardless of the config. Changed text-input.component.ts and slider.component.ts to bind (input) instead, which fires on every keystroke and matches what the other frameworks do.
|
|
||
| protected readonly template: Signal<TemplateRef<unknown> | undefined> = contentChild(TemplateRef); | ||
|
|
||
| private readonly internalFormStore = computed(() => this.of()[INTERNAL]); |
There was a problem hiding this comment.
Can't FormischField reuse injectField? This is how we do it in all the other frameworks. Same for FormischFieldArray.
There was a problem hiding this comment.
Good point. The challenge in Angular is that injectField must be called during component construction (inside an injection context), but the of and path inputs are InputSignal values that Angular binds after construction. In React, useField re-runs on every render so it naturally picks up prop changes. In Vue, the composition API is reactive by default so useField receives computed refs.
For Angular we see two options: (1) have injectField accept Signal<FormStore> and Signal<ValidPath> in addition to plain values, or (2) call injectField inside a computed() that reads from the input signals reactively. We'd lean toward option 1 to keep the call site clean. Happy to refactor FormischField and FormischFieldArray to use injectField once you confirm which direction you prefer.
There was a problem hiding this comment.
@dosu what do you think about this? What should we do?
There was a problem hiding this comment.
I'd recommend Option 1: have injectField accept Signal-based inputs (i.e., Signal<FormStore> and Signal<ValidPath>). Here's why:
It aligns with how every other framework adapter works. In Vue and Solid, the useField hook already accepts getter functions rather than static values — e.g., () => props.of and () => ({ path: props.path }) [1] [2]. Angular's Signal is conceptually equivalent to those getters: a reactive reference that injectField can read inside a computed() to derive the field store reactively.
Why not Option 2 (calling injectField inside a computed())? Angular's injection context rules make this fragile — inject() calls (and anything relying on them) can only run during construction. Wrapping injectField inside a computed() would need careful restructuring to separate the injection-context-dependent parts from the reactive-derivation parts, which defeats the simplicity goal.
Suggested API shape:
// injectField accepts signals directly
const field = injectField(this.of, { path: this.path });
// where `this.of` is InputSignal<FormStore> and `this.path` is InputSignal<ValidPath>Internally, injectField would use computed() to read from those signals and derive the reactive FieldStore — mirroring how Vue uses computed(() => ...) and Solid uses createMemo(() => ...) in their respective useField implementations [3] [4].
This keeps FormischField and FormischFieldArray as thin wrappers (consistent with the React/Vue/Solid pattern where the component just calls the hook and passes the result to children), and gives standalone users of injectField the same reactive experience without needing to manually manage effects.
To reply, just mention @dosu.
Docs are dead. Just use Dosu.
There was a problem hiding this comment.
Done in 5fae5c9. Added signal-based overloads to both injectField and injectFieldArray so they accept Signal<FormStore> and Signal<ValidPath> directly:
// plain values (existing usage)
injectField(form, { path: ['email'] })
// signal values (used by FormischField)
injectField(this.of, { path: this.path })FormischField and FormischFieldArray are now thin wrappers that delegate entirely to the inject functions — matching the pattern in React, Vue, and Solid. The signal overload also exports InjectFieldConfigSignal and InjectFieldArrayConfigSignal for users who want to compose with signals directly.
|
I've addressed everything by implementing the full Angular playground as a 1:1 translation from the React playground, including:
Also fixed |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playgrounds/angular/src/components/file-input.component.ts (1)
68-76: 💤 Low valueOptional: make
containerClassesacomputed()for consistency.
labelClasses(andfiles/displayText) usecomputed(), butcontainerClassesis a plain arrow that recomputes on every change-detection pass. Aligning it avoids redundantclsxcalls and matches the rest of the class.♻️ Proposed change
- protected readonly containerClasses = () => - clsx('px-8 lg:px-10', this.class()); + protected readonly containerClasses = computed(() => + clsx('px-8 lg:px-10', this.class()) + );🤖 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 `@playgrounds/angular/src/components/file-input.component.ts` around lines 68 - 76, Convert the containerClasses property to a computed value like labelClasses to avoid recomputing clsx on each change-detection pass: replace the arrow function containerClasses = () => clsx('px-8 lg:px-10', this.class()) with a readonly computed(() => clsx('px-8 lg:px-10', this.class())) so it memoizes results consistently with labelClasses and the other computed properties (files/displayText).playgrounds/angular/src/components/select.component.ts (1)
78-80: 💤 Low valueConsider removing
isSelected()if select value binding is used.If you adopt the recommended fix of binding the select's
[value]property, theisSelected()method becomes unused and can be removed.🤖 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 `@playgrounds/angular/src/components/select.component.ts` around lines 78 - 80, The isSelected() method is now redundant if you switch to binding the select option via the element's [value] and Angular's value binding; remove the protected isSelected(value: string): boolean { return this.values().includes(value); } method and delete or replace any calls to isSelected() (search for isSelected in this component and templates) so option selection relies on the bound value instead (ensure values() and the select's [(ngModel)]/formControl remain intact).
🤖 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 `@playgrounds/angular/src/components/select.component.ts`:
- Around line 25-46: The select element is not bound to the component value, so
changes to input() / values() won't reliably update the browser selection;
update the <select> to bind its value based on mode (use [value]=input() when
multiple() is falsy and [value]=values() when multiple() is truthy), remove the
per-option [selected] binding on the <option> elements (currently using
isSelected(option.value)), and keep existing attributes like name(),
selectClasses(), placeholder(), and options() as-is so Angular will manage
selection from the select's value instead.
In `@playgrounds/angular/src/components/unstyled-button.component.ts`:
- Around line 52-59: In handleClick(), ensure isLoading is reset even if
handler() throws by wrapping the await in try/finally: call
this.isLoading.set(true) before awaiting handler(), await handler() inside a try
block, and call this.isLoading.set(false) in the finally block; keep locating
this change in the protected async handleClick() method and preserve the
existing this.onClick(), this.clicked.emit(), and guard for !handler.
---
Nitpick comments:
In `@playgrounds/angular/src/components/file-input.component.ts`:
- Around line 68-76: Convert the containerClasses property to a computed value
like labelClasses to avoid recomputing clsx on each change-detection pass:
replace the arrow function containerClasses = () => clsx('px-8 lg:px-10',
this.class()) with a readonly computed(() => clsx('px-8 lg:px-10',
this.class())) so it memoizes results consistently with labelClasses and the
other computed properties (files/displayText).
In `@playgrounds/angular/src/components/select.component.ts`:
- Around line 78-80: The isSelected() method is now redundant if you switch to
binding the select option via the element's [value] and Angular's value binding;
remove the protected isSelected(value: string): boolean { return
this.values().includes(value); } method and delete or replace any calls to
isSelected() (search for isSelected in this component and templates) so option
selection relies on the bound value instead (ensure values() and the select's
[(ngModel)]/formControl remain intact).
🪄 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: c56a8826-6cc6-4841-b9a9-264d9117b18d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
frameworks/angular/src/components/FormischField/FormischField.test.tsframeworks/angular/src/components/FormischFieldArray/FormischFieldArray.tsframeworks/angular/src/components/FormischForm/FormischForm.test.tsframeworks/angular/src/functions/injectField/injectField.test-d.tsframeworks/angular/src/functions/injectFieldArray/injectFieldArray.test-d.tsframeworks/angular/src/functions/injectForm/injectForm.test-d.tsplaygrounds/angular/package.jsonplaygrounds/angular/src/app.component.tsplaygrounds/angular/src/app.routes.tsplaygrounds/angular/src/components/action-button.component.tsplaygrounds/angular/src/components/auto-animate.directive.tsplaygrounds/angular/src/components/button-group.component.tsplaygrounds/angular/src/components/checkbox.component.tsplaygrounds/angular/src/components/color-button.component.tsplaygrounds/angular/src/components/expandable.component.tsplaygrounds/angular/src/components/file-input.component.tsplaygrounds/angular/src/components/form-error.component.tsplaygrounds/angular/src/components/form-footer.component.tsplaygrounds/angular/src/components/form-header.component.tsplaygrounds/angular/src/components/input-errors.component.tsplaygrounds/angular/src/components/input-label.component.tsplaygrounds/angular/src/components/radio-group.component.tsplaygrounds/angular/src/components/radio.component.tsplaygrounds/angular/src/components/select.component.tsplaygrounds/angular/src/components/slider.component.tsplaygrounds/angular/src/components/spinner.component.tsplaygrounds/angular/src/components/text-input.component.tsplaygrounds/angular/src/components/unstyled-button.component.tsplaygrounds/angular/src/icons/angle-down-icon.component.tsplaygrounds/angular/src/routes/nested/nested.component.tsplaygrounds/angular/src/routes/special/special.component.tsplaygrounds/angular/src/routes/todos/todos.component.tsplaygrounds/angular/src/utils/disable-transitions.ts
✅ Files skipped from review due to trivial changes (2)
- playgrounds/angular/src/components/color-button.component.ts
- frameworks/angular/src/components/FormischField/FormischField.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- frameworks/angular/src/functions/injectForm/injectForm.test-d.ts
- playgrounds/angular/package.json
- playgrounds/angular/src/components/form-footer.component.ts
- frameworks/angular/src/functions/injectFieldArray/injectFieldArray.test-d.ts
- playgrounds/angular/src/components/form-header.component.ts
- frameworks/angular/src/components/FormischForm/FormischForm.test.ts
- playgrounds/angular/src/components/text-input.component.ts
- frameworks/angular/src/components/FormischFieldArray/FormischFieldArray.ts
- frameworks/angular/src/functions/injectField/injectField.test-d.ts
sonukapoor
force-pushed
the
feat/111-angular-package-skeleton
branch
from
688a6d0 to
e3cad10
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
I let codex finalizing this PR. Let's see what it comes up with. |
- Fix FormischFieldArray to use signal inputs and contentChild() for AOT compatibility - Replace @formkit/auto-animate/angular (no Ivy metadata) with a custom AutoAnimateDirective using afterNextRender - Fix (change) → (input) on text-input and slider so validate: 'input' fires on every keystroke - Replace effect + setTimeout pattern in ExpandableComponent with afterRenderEffect - Add nested and special playground routes matching React 1:1 - Add all shared UI components (checkbox, select, radio, slider, file-input, expandable, etc.)
- Bind [value] on <select> element for controlled component behavior instead of [selected] on each option; remove now-unused isSelected() method - Wrap await handler() in try/finally in UnstyledButtonComponent so isLoading resets even when the handler throws
Add signal-based overloads to injectField and injectFieldArray so they can accept InputSignal<FormStore> and InputSignal<path> directly. FormischField and FormischFieldArray become thin wrappers that delegate to the inject functions, matching the pattern used in all other frameworks.
sonukapoor
force-pushed
the
feat/111-angular-package-skeleton
branch
from
5fae5c9 to
66580bc
Compare
|
I got everything working and was about to push my changes. But now I can't because you modified the code. |
|
Not sure what I should do now. A force push would override all your changes. I thought it was clear that I was working on this PR. |
|
I modified many things. The source code, the build setup, the playground and a few other details. Merging it with your changes will probably breaks things again. |
|
I created a new PR. Feel free to take a look at it and let me know what you recommend to do. #124 |
Implements the full Angular framework package for Formisch, including types, injectForm, injectField, injectFieldArray, components (FormischForm, FormischField, FormischFieldArray), and a playground to validate the implementation end-to-end.
Closes #111