Merged
Conversation
📝 WalkthroughWalkthroughSplit repository typecheck into library and test steps, add per-package test tsconfigs and a test-typecheck runner, introduce runtime validation and stricter adapter typing across auth packages, refactor authorization actor inference to registry-derived types, and update many tests and runtime helpers for safer shapes and manifest-typed Flux APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as run-test-typecheck.mjs
participant FS as Filesystem
participant TSC as Bunx/tsc
Runner->>FS: discover packages with tests + tsconfig
alt package has tsconfig.tests.json
Runner->>TSC: spawn bunx tsc -p package/tsconfig.tests.json --noEmit
else
Runner->>FS: write temp tsconfig.json (extends package tsconfig, includes src/tests)
Runner->>TSC: spawn bunx tsc -p temp/tsconfig.json --noEmit
TSC-->>FS: (reads temp config)
Runner->>FS: cleanup temp config
end
TSC-->>Runner: exit code
alt non-zero
Runner->>Runner: throw "Test typecheck failed for <package>"
Runner->>process: exit(1)
else
Runner->>Runner: continue to next package
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/authorization/src/contracts.ts (1)
159-185:⚠️ Potential issue | 🟠 MajorMatch the optional
actorregistry field when inferring actor types.
actoris optional on bothAuthorizationPolicyRegistryEntry(line 131) andAuthorizationAbilityRegistryEntry(line 143), but the new conditionals at lines 175-176 and 181-182 require a mandatory{ actor: ... }property. Registry entries that omit the actor field therefore fall back toobject, losing proper actor type inference for policies and abilities.🛠️ Proposed fix
-type FallbackRegistryActor<TActor> = [TActor] extends [never] ? object : TActor +type FallbackRegistryActor<TActor> = [Exclude<TActor, undefined>] extends [never] + ? object + : Exclude<TActor, undefined> @@ -export type PolicyActorForName<TPolicyName extends string> = RegisteredAuthorizationPolicyEntry<TPolicyName> extends { - actor: infer TActor +export type PolicyActorForName<TPolicyName extends string> = RegisteredAuthorizationPolicyEntry<TPolicyName> extends { + readonly actor?: infer TActor } ? FallbackRegistryActor<TActor> : object -export type AbilityActorForName<TAbilityName extends string> = RegisteredAuthorizationAbilityEntry<TAbilityName> extends { - actor: infer TActor +export type AbilityActorForName<TAbilityName extends string> = RegisteredAuthorizationAbilityEntry<TAbilityName> extends { + readonly actor?: infer TActor } ? FallbackRegistryActor<TActor> : object🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authorization/src/contracts.ts` around lines 159 - 185, The conditional type checks for PolicyActorForName and AbilityActorForName require the registry entry to have a mandatory actor property, which misses entries where actor is optional; change the pattern from extends { actor: infer TActor } to extends { actor?: infer TActor } in both PolicyActorForName and AbilityActorForName so optional actor fields are matched and their inferred types are passed through to FallbackRegistryActor<TActor> instead of falling back to object.packages/auth-social/src/index.ts (1)
234-254:⚠️ Potential issue | 🟠 MajorValidate adapter user ids before persisting social identities.
adapter.getId(user)can still yield an invalid value, and the laterserialized.id!path can save an invaliduserId. Fail before serialization/persistence.🛡️ Proposed id validation
+function requireUserId(id: unknown): string | number { + if (typeof id === 'string' && id.trim().length > 0) { + return id + } + if (typeof id === 'number' && Number.isFinite(id)) { + return id + } + + throw new Error('[`@holo-js/auth-social`] Auth provider users must expose a valid string or number id.') +} + function serializeLocalUser( adapter: RuntimeAuthProviderAdapter, user: Record<string, unknown>, providerName: string, ): AuthUserLike { - const id = adapter.getId(user) + const id = requireUserId(adapter.getId(user)) const serialized = adapter.serialize ? adapter.serialize(user) : { ...(user as Record<string, unknown>) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-social/src/index.ts` around lines 234 - 254, In serializeLocalUser, validate the id returned by adapter.getId(user) before using it: call adapter.getId(user), ensure it is a non-empty string (or otherwise meets the project's id shape), and throw a descriptive error if invalid so you fail early; then proceed to build serialized (using adapter.serialize if present), set result.id to the validated id, attach AUTH_PROVIDER_MARKER as before, and return the frozen result (this prevents persisting an invalid userId later).packages/auth/src/runtime.ts (1)
2078-2098:⚠️ Potential issue | 🟡 MinorPassword reset consume:
adapter.getId(user)result isn't validated withrequireUserId.Every other adapter id extraction in this file now routes through
requireUserIdfor a uniform, clear error. Inpasswords.consume(), the id fromadapter.getId(user)is passed straight intoupdateUserRecord(and laterfindById), so a misbehaving adapter returning a non–string/number here would surface as a downstreamno longer existserror or an adapter-specific failure instead of the normalized runtime error. Cheap to tighten:🛡 Proposed fix
const password = await getRuntimeBindings().passwordHasher.hash(input.password) - const updated = await updateUserRecord(record.provider, adapter.getId(user), { + const updated = await updateUserRecord(record.provider, requireUserId( + adapter, + user, + '[`@holo-js/auth`] Password reset requires a user with a serializable id.', + ), { password, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/runtime.ts` around lines 2078 - 2098, In passwords.consume, validate the id returned by adapter.getId(user) using requireUserId before passing it to updateUserRecord and subsequent lookups to ensure a normalized runtime error for bad adapter behavior; locate the adapter.getId(user) call in passwords.consume, call requireUserId(adapter.getId(user)) and use that returned id variable in updateUserRecord (and any later findById/use), leaving the rest of the flow (password hashing, store.delete, store.deleteByEmail, return updated) unchanged.
🧹 Nitpick comments (7)
packages/storage-s3/src/index.ts (1)
218-223: Drop the redundantUint8Arraycopy inside theBlob.
payloadBytesis already aUint8Arrayreturned fromtoBodyBytes, sonew Uint8Array(payloadBytes)just allocates an extra copy for no benefit.Blobaccepts the originalUint8Arraydirectly. (Also worth noting: an empty-string payload produces a zero-lengthUint8Arraywhich is truthy, so this branch correctly still sends an empty Blob — consistent with the empty-string hash used during signing.)♻️ Proposed simplification
- body: payloadBytes ? new Blob([new Uint8Array(payloadBytes)]) : undefined, + body: payloadBytes ? new Blob([payloadBytes]) : undefined,Optionally, if there's no specific reason the body must be a
Blob(e.g., to dodge a particular undici streaming quirk), passingpayloadBytesdirectly as aUint8Arrayis also a validBodyInitand avoids the wrapper entirely. If theBlobform was chosen deliberately to work around a runtime issue, a short comment noting it would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage-s3/src/index.ts` around lines 218 - 223, The Request body construction is copying payloadBytes into a new Uint8Array unnecessarily; inside the function that returns new Request(...) use payloadBytes directly (either pass payloadBytes into new Blob([payloadBytes]) or pass payloadBytes as the BodyInit without wrapping in a Blob) instead of new Uint8Array(payloadBytes); keep the conditional that allows an empty Uint8Array to produce an empty Blob and, if the Blob wrapper was chosen for a runtime reason, add a short comment near the Request creation explaining that choice for future maintainers.packages/storage/tests/s3Driver.test.ts (1)
7-9: Let TypeScript validate the dynamic import shape.The explicit type assertion on line 8 can mask export signature drift. The declared return type already provides TypeScript with a contract to enforce assignability without requiring the assertion.
♻️ Proposed refactor
async function loadDriver(): Promise<typeof createS3Driver> { - const module = await import('../src/runtime/drivers/s3') as { default: typeof createS3Driver } + const module = await import('../src/runtime/drivers/s3') return module.default }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/tests/s3Driver.test.ts` around lines 7 - 9, Remove the explicit type assertion on the dynamic import in loadDriver so TypeScript can validate the module shape against the declared return type: change "const module = await import('../src/runtime/drivers/s3') as { default: typeof createS3Driver }" to a plain dynamic import (e.g., "const module = await import('../src/runtime/drivers/s3')") and keep the function signature async function loadDriver(): Promise<typeof createS3Driver> so the compiler enforces that module.default is assignable to createS3Driver; reference the loadDriver function and the createS3Driver export in the s3 module when making this change.packages/mail/tests/runtime.test.ts (1)
32-55: Nice type-narrowing helpers — minor note onBuiltInDriverNameduplication.
getMailerConfigusing thedriverdiscriminant +getBuiltInDriverthat throws on missing keys is a clean improvement over the previous spread-and-hope pattern.Minor:
BuiltInDriverNameis a hand-maintained union ('preview' | 'fake' | 'log' | 'smtp') that must stay in sync withmailRuntimeInternals.builtInDrivers. If the source ever exposes a typed key list (orkeyof typeof builtInDrivers), deriving from it would avoid a silent skew.♻️ Optional derivation
-type BuiltInDriverName = 'preview' | 'fake' | 'log' | 'smtp' +type BuiltInDriverName = keyof typeof mailRuntimeInternals.builtInDrivers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mail/tests/runtime.test.ts` around lines 32 - 55, Replace the hand-maintained BuiltInDriverName union with a derived type from the actual builtInDrivers object so it cannot drift out of sync: change the type definition to use keyof typeof mailRuntimeInternals.builtInDrivers and update any usages (e.g., the getBuiltInDriver(name: BuiltInDriverName) parameter) to use that derived type so TypeScript will enforce keys match mailRuntimeInternals.builtInDrivers at compile time.packages/cli/tests/authorization-registry.test.ts (1)
159-159: Assertactor: objecton both typed authorization entries.This broad
toContaincan pass if only one generated entry emits the fallback actor type. Pin it to the policy and ability blocks so the test catches regressions in either path.🧪 Proposed test tightening
- expect(types).toContain('actor: object') + expect(types).toMatch(/"posts": \{\s+actor: object/) + expect(types).toMatch(/"reports\.export": \{\s+actor: object/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/authorization-registry.test.ts` at line 159, The current assertion expect(types).toContain('actor: object') is too broad—update the test to assert that both the policy-generated entry and the ability-generated entry include 'actor: object'; locate the array variable types and replace the single toContain check with two targeted checks that verify one entry associated with the policy block contains 'actor: object' and one entry associated with the ability block contains 'actor: object' (e.g., by filtering types for strings that reference "policy" and "ability" or matching their identifying substrings and asserting each filtered result includes 'actor: object').packages/authorization/src/runtime.ts (1)
48-58: Registry-derived actor inference looks correct; one edge case to be aware of.
FallbackAuthorizationActor<TActor>degrades toneverwhen the registry-mapped actor type doesn't intersect withobject(e.g. someone registers a primitive). That will make the handler parameter effectively unusable at the call site rather than defaulting toobject. That's arguably the right behavior (forces correct registry typing) but surprising; a short TSDoc on these helpers — especially the[Extract<..., keyof registry>] extends [never] ? object : …fallback path — would save a reader of the type an investigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authorization/src/runtime.ts` around lines 48 - 58, The helper types FallbackAuthorizationActor, PolicyActorForDefinition, and AbilityActorForDefinition can produce surprising behavior when a registry maps to a primitive (the FallbackAuthorizationActor may evaluate to never instead of object), so add concise TSDoc above each of these type aliases explaining the edge case: describe that FallbackAuthorizationActor<TActor> will only yield object for non-never object-like types and will become never if the registry-mapped actor is a primitive, and note the intended consequence (forcing stricter typing) and the fallback logic in the conditional extract checks that reference PolicyActorForName, AuthorizationPolicyRegistry, AbilityActorForName, and AuthorizationAbilityRegistry. Ensure the comments mention that this is deliberate and guide readers on expected behavior at call sites.packages/auth-clerk/src/index.ts (1)
535-558: Validation helpers mirror the auth-workos implementation consistently.The structure matches auth-workos, which helps auditability. Consider (optionally, in a follow-up) lifting
requireUserId/requireUserRecordinto a shared internal util in@holo-js/authso the three provider packages (clerk, workos, social) don't each maintain their own copies — right now any future tweak has to be applied in three places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-clerk/src/index.ts` around lines 535 - 558, The two validation helpers requireUserId and requireUserRecord are duplicated across provider packages; extract them into a single shared internal util in the `@holo-js/auth` package (e.g., export functions from an internal module like auth/internal/validation), keep the exact signatures (requireUserId(adapter: RuntimeAuthProviderAdapter, user: unknown, message: string): string | number and requireUserRecord(user: unknown, message: string): Record<string, unknown>), update the provider packages (clerk, workos, social) to import these helpers instead of defining them locally, and remove the local definitions (ensure type imports for RuntimeAuthProviderAdapter are preserved or re-exported from the shared module).scripts/run-test-typecheck.mjs (1)
47-63: Consider batching type-tests per package into a singletscinvocation.Today every
*.type.test.tsfile produces its own temptsconfig.jsonand its ownbunx tsc --noEmitprocess. For packages with many type-tests this multiplies process startup + full program load cost. A single generated config that includes all the package's type-tests (like the "main" test config does) would typecheck them in one pass and dramatically cut CI time, at the cost of losing per-file isolation.If per-file isolation is intentional (e.g., because different
.type.test.tsfiles assert conflicting module augmentations or declaration merges), it's worth a short code comment explaining why so future contributors don't "optimize" it away.Also applies to: 138-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-test-typecheck.mjs` around lines 47 - 63, resolveTestTsconfigs currently creates one generated tsconfig per type-test file by calling createGeneratedTypeTestConfig for each file returned by collectTypeTestFiles, which causes many separate tsc invocations; change it to batch all type-test files for a package into a single generated tsconfig (e.g., call a new createGeneratedTypeTestsBatchConfig(packageDir, typeTestFiles, generatedConfigDirs) or reuse createGeneratedMainTestConfig with the list of files) so resolveTestTsconfigs pushes one combined configPath instead of many, and add a short comment on why per-file isolation would be required if you decide to keep the current behavior (mention resolveTestTsconfigs, collectTypeTestFiles, createGeneratedTypeTestConfig, createGeneratedMainTestConfig).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-social/src/index.ts`:
- Around line 95-100: The validation treats any falsy adapter return as "not
found"; change requireUserRecord to only consider null or undefined as "not
found" and to throw if the adapter returns a non-null/non-undefined non-object
(e.g., false, 0, "") — update the function signature/returns as needed so
callers can handle nullish results from findByCredentials, and adjust the other
identical adapter-result validation later in the file (the other adapter return
check) to follow the same logic; ensure error messages still use the existing
message parameter and keep throwing for invalid non-object values.
In `@packages/flux-react/tests/package.type.test.ts`:
- Around line 15-42: The manifest object is annotated with
GeneratedBroadcastManifest which widens literal event names and channel patterns
to string; change the manifest declaration to preserve literal types by using
"as const satisfies GeneratedBroadcastManifest" on the manifest variable so
TypeScript keeps literal types while still validating shape; update the manifest
binding (the variable named manifest used when calling createFluxClient) to use
this pattern so hook type inference for createFluxClient and any functions
referencing event names/channels remains precise.
In `@packages/flux-svelte/tests/package.type.test.ts`:
- Around line 16-43: The manifest object is being widened by the explicit type
annotation (const manifest: GeneratedBroadcastManifest) which loses literal
types before createFluxClient infers TManifest; change the declaration to
preserve literals by using a const assertion combined with a satisfier (i.e.
replace the explicit type annotation on manifest with "as const satisfies
GeneratedBroadcastManifest") so the manifest values remain literal while still
validating the shape, ensuring createFluxClient (and
fluxInternals.createPusherConnector usage) receives the precise literal types
for correct downstream type inference.
In `@packages/flux-vue/tests/package.type.test.ts`:
- Around line 15-42: The manifest constant is being widened by the explicit
GeneratedBroadcastManifest annotation; change the declaration to preserve
literal types by using the "as const satisfies GeneratedBroadcastManifest"
pattern on the manifest object (referencing the manifest symbol and
GeneratedBroadcastManifest type) so TypeScript keeps event/channel literal
inference while still validating the shape; apply the same change to the
corresponding manifest literals in the flux-react and flux-svelte test files.
In `@packages/queue-db/tests/failed.test.ts`:
- Around line 37-49: The inline stubbed getDialect() in the test still returns
old capability keys (jsonOperations, lateralJoins, pessimisticLocking,
vectorColumns) which no longer exist; update the mocked getDialect() return
value in failed.test.ts (the mocked getDialect / connection cast as never) to
match the current dialect capability shape used elsewhere (use keys like
jsonValueQuery, jsonContains, jsonLength, schemaQualifiedIdentifiers,
nativeUpsert, ddlAlterSupport, introspection, returning, lockForUpdate,
sharedLock, concurrentQueries, workerThreadExecution, savepoints, etc.),
removing the deprecated keys so the test exercises the real capability flags.
In `@scripts/run-test-typecheck.mjs`:
- Around line 116-127: collectTypeTestFiles uses Dirent.parentPath
(entry.parentPath) which exists only in Node ≥20.12.0 and will be undefined on
20.11.x; change the mapping to compute the parent path without relying on
parentPath by falling back to a stable alternative: use path.dirname(entry.path)
when parentPath is undefined (and as a final fallback use the testsDir), then
call join(parent, entry.name). Update collectTypeTestFiles to import/use dirname
from 'path' and replace join(entry.parentPath, entry.name) with
join(parentPathFallback, entry.name) where parentPathFallback = entry.parentPath
?? dirname(entry.path) ?? testsDir.
---
Outside diff comments:
In `@packages/auth-social/src/index.ts`:
- Around line 234-254: In serializeLocalUser, validate the id returned by
adapter.getId(user) before using it: call adapter.getId(user), ensure it is a
non-empty string (or otherwise meets the project's id shape), and throw a
descriptive error if invalid so you fail early; then proceed to build serialized
(using adapter.serialize if present), set result.id to the validated id, attach
AUTH_PROVIDER_MARKER as before, and return the frozen result (this prevents
persisting an invalid userId later).
In `@packages/auth/src/runtime.ts`:
- Around line 2078-2098: In passwords.consume, validate the id returned by
adapter.getId(user) using requireUserId before passing it to updateUserRecord
and subsequent lookups to ensure a normalized runtime error for bad adapter
behavior; locate the adapter.getId(user) call in passwords.consume, call
requireUserId(adapter.getId(user)) and use that returned id variable in
updateUserRecord (and any later findById/use), leaving the rest of the flow
(password hashing, store.delete, store.deleteByEmail, return updated) unchanged.
In `@packages/authorization/src/contracts.ts`:
- Around line 159-185: The conditional type checks for PolicyActorForName and
AbilityActorForName require the registry entry to have a mandatory actor
property, which misses entries where actor is optional; change the pattern from
extends { actor: infer TActor } to extends { actor?: infer TActor } in both
PolicyActorForName and AbilityActorForName so optional actor fields are matched
and their inferred types are passed through to FallbackRegistryActor<TActor>
instead of falling back to object.
---
Nitpick comments:
In `@packages/auth-clerk/src/index.ts`:
- Around line 535-558: The two validation helpers requireUserId and
requireUserRecord are duplicated across provider packages; extract them into a
single shared internal util in the `@holo-js/auth` package (e.g., export functions
from an internal module like auth/internal/validation), keep the exact
signatures (requireUserId(adapter: RuntimeAuthProviderAdapter, user: unknown,
message: string): string | number and requireUserRecord(user: unknown, message:
string): Record<string, unknown>), update the provider packages (clerk, workos,
social) to import these helpers instead of defining them locally, and remove the
local definitions (ensure type imports for RuntimeAuthProviderAdapter are
preserved or re-exported from the shared module).
In `@packages/authorization/src/runtime.ts`:
- Around line 48-58: The helper types FallbackAuthorizationActor,
PolicyActorForDefinition, and AbilityActorForDefinition can produce surprising
behavior when a registry maps to a primitive (the FallbackAuthorizationActor may
evaluate to never instead of object), so add concise TSDoc above each of these
type aliases explaining the edge case: describe that
FallbackAuthorizationActor<TActor> will only yield object for non-never
object-like types and will become never if the registry-mapped actor is a
primitive, and note the intended consequence (forcing stricter typing) and the
fallback logic in the conditional extract checks that reference
PolicyActorForName, AuthorizationPolicyRegistry, AbilityActorForName, and
AuthorizationAbilityRegistry. Ensure the comments mention that this is
deliberate and guide readers on expected behavior at call sites.
In `@packages/cli/tests/authorization-registry.test.ts`:
- Line 159: The current assertion expect(types).toContain('actor: object') is
too broad—update the test to assert that both the policy-generated entry and the
ability-generated entry include 'actor: object'; locate the array variable types
and replace the single toContain check with two targeted checks that verify one
entry associated with the policy block contains 'actor: object' and one entry
associated with the ability block contains 'actor: object' (e.g., by filtering
types for strings that reference "policy" and "ability" or matching their
identifying substrings and asserting each filtered result includes 'actor:
object').
In `@packages/mail/tests/runtime.test.ts`:
- Around line 32-55: Replace the hand-maintained BuiltInDriverName union with a
derived type from the actual builtInDrivers object so it cannot drift out of
sync: change the type definition to use keyof typeof
mailRuntimeInternals.builtInDrivers and update any usages (e.g., the
getBuiltInDriver(name: BuiltInDriverName) parameter) to use that derived type so
TypeScript will enforce keys match mailRuntimeInternals.builtInDrivers at
compile time.
In `@packages/storage-s3/src/index.ts`:
- Around line 218-223: The Request body construction is copying payloadBytes
into a new Uint8Array unnecessarily; inside the function that returns new
Request(...) use payloadBytes directly (either pass payloadBytes into new
Blob([payloadBytes]) or pass payloadBytes as the BodyInit without wrapping in a
Blob) instead of new Uint8Array(payloadBytes); keep the conditional that allows
an empty Uint8Array to produce an empty Blob and, if the Blob wrapper was chosen
for a runtime reason, add a short comment near the Request creation explaining
that choice for future maintainers.
In `@packages/storage/tests/s3Driver.test.ts`:
- Around line 7-9: Remove the explicit type assertion on the dynamic import in
loadDriver so TypeScript can validate the module shape against the declared
return type: change "const module = await import('../src/runtime/drivers/s3') as
{ default: typeof createS3Driver }" to a plain dynamic import (e.g., "const
module = await import('../src/runtime/drivers/s3')") and keep the function
signature async function loadDriver(): Promise<typeof createS3Driver> so the
compiler enforces that module.default is assignable to createS3Driver; reference
the loadDriver function and the createS3Driver export in the s3 module when
making this change.
In `@scripts/run-test-typecheck.mjs`:
- Around line 47-63: resolveTestTsconfigs currently creates one generated
tsconfig per type-test file by calling createGeneratedTypeTestConfig for each
file returned by collectTypeTestFiles, which causes many separate tsc
invocations; change it to batch all type-test files for a package into a single
generated tsconfig (e.g., call a new
createGeneratedTypeTestsBatchConfig(packageDir, typeTestFiles,
generatedConfigDirs) or reuse createGeneratedMainTestConfig with the list of
files) so resolveTestTsconfigs pushes one combined configPath instead of many,
and add a short comment on why per-file isolation would be required if you
decide to keep the current behavior (mention resolveTestTsconfigs,
collectTypeTestFiles, createGeneratedTypeTestConfig,
createGeneratedMainTestConfig).
🪄 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: 6360d728-3181-4930-b426-59cb447fa718
📒 Files selected for processing (51)
package.jsonpackages/adapter-nuxt/tests/setup.test.tspackages/adapter-nuxt/tsconfig.tests.jsonpackages/auth-clerk/src/index.tspackages/auth-social/src/index.tspackages/auth-workos/src/index.tspackages/auth/src/contracts.tspackages/auth/src/runtime.tspackages/auth/tsconfig.tests.jsonpackages/auth/tsconfig.type-tests.jsonpackages/authorization/src/contracts.tspackages/authorization/src/runtime.tspackages/authorization/tests/contracts.type.test.tspackages/authorization/tsconfig.tests.jsonpackages/authorization/tsconfig.type-tests.jsonpackages/broadcast/src/worker.tspackages/broadcast/tests/contracts.test.tspackages/cli/src/project/registry.tspackages/cli/tests/authorization-registry.test.tspackages/config/tests/broadcast-config.type.test.tspackages/config/tests/security-config.type.test.tspackages/events/src/registry.tspackages/events/tests/afterCommit.test.tspackages/events/tests/boundaries.test.tspackages/events/tests/contracts.test.tspackages/events/tests/contracts.type.test.tspackages/events/tests/registry.test.tspackages/flux-react/tests/package.test.tspackages/flux-react/tests/package.type.test.tspackages/flux-svelte/tests/package.type.test.tspackages/flux-vue/tests/package.type.test.tspackages/flux/tests/package.test.tspackages/forms/tests/client.test.tspackages/forms/tests/security.test.tspackages/forms/tsconfig.tests.jsonpackages/mail/tests/contracts.test.tspackages/mail/tests/index.type.test.tspackages/mail/tests/runtime.test.tspackages/media/tests/media.test.tspackages/notifications/tests/contracts.test.tspackages/notifications/tests/contracts.type.test.tspackages/notifications/tests/index.type.test.tspackages/notifications/tests/runtime.test.tspackages/queue-db/tests/database-driver.test.tspackages/queue-db/tests/failed.test.tspackages/storage-s3/src/index.tspackages/storage/src/runtime/composables/index.tspackages/storage/tests/facade.test.tspackages/storage/tests/s3Driver.test.tspackages/validation/tsconfig.tests.jsonscripts/run-test-typecheck.mjs
Comment on lines
+37
to
+49
| returning: false, | ||
| lockForUpdate: false, | ||
| sharedLock: false, | ||
| concurrentQueries: false, | ||
| jsonOperations: true, | ||
| lateralJoins: false, | ||
| workerThreadExecution: false, | ||
| pessimisticLocking: false, | ||
| savepoints: true, | ||
| vectorColumns: false, | ||
| jsonValueQuery: true, | ||
| jsonContains: true, | ||
| jsonLength: true, | ||
| schemaQualifiedIdentifiers: true, | ||
| nativeUpsert: false, | ||
| ddlAlterSupport: false, | ||
| introspection: false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\b(jsonOperations|lateralJoins|pessimisticLocking|vectorColumns)\b' packages/queue-db/tests/failed.test.tsRepository: cobraprojects/holo-js
Length of output: 512
Update the remaining inline dialect stub to the new capability shape.
The mocked getDialect() at lines 223-240 returns removed capability keys (jsonOperations, lateralJoins, pessimisticLocking, vectorColumns) that have been removed from the dialect capabilities. Since the connection is cast as never, TypeScript won't catch this mismatch and the test can exercise undefined capability flags.
🛠️ Proposed fix
getDialect() {
- return {
- name: 'sqlite',
- capabilities: {
- concurrentQueries: false,
- jsonOperations: true,
- lateralJoins: false,
- workerThreadExecution: false,
- pessimisticLocking: false,
- savepoints: true,
- vectorColumns: false,
- },
- quoteIdentifier(identifier: string) {
- return `"${identifier}"`
- },
- createPlaceholder() {
- return '?'
- },
- }
+ return createDialect('sqlite', '?')
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/queue-db/tests/failed.test.ts` around lines 37 - 49, The inline
stubbed getDialect() in the test still returns old capability keys
(jsonOperations, lateralJoins, pessimisticLocking, vectorColumns) which no
longer exist; update the mocked getDialect() return value in failed.test.ts (the
mocked getDialect / connection cast as never) to match the current dialect
capability shape used elsewhere (use keys like jsonValueQuery, jsonContains,
jsonLength, schemaQualifiedIdentifiers, nativeUpsert, ddlAlterSupport,
introspection, returning, lockForUpdate, sharedLock, concurrentQueries,
workerThreadExecution, savepoints, etc.), removing the deprecated keys so the
test exercises the real capability flags.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/notifications/src/index.ts (1)
89-91: Add explicit return type for API stability.The wrapper omits the return type annotation, unlike the base
defineNotificationsConfiginpackages/config/src/loader.ts(line 548) which declaresDefineConfigValue<TConfig>. Relying on inference here means the public API's return type is implicit and can silently drift if the base implementation changes. Mirror the base signature for consistency with the otherdefine*Confighelpers.♻️ Proposed change
-export function defineNotificationsConfig<const TConfig extends HoloNotificationsConfig>(config: TConfig) { - return defineBaseNotificationsConfig(config) -} +export function defineNotificationsConfig<const TConfig extends HoloNotificationsConfig>( + config: TConfig, +): DefineConfigValue<TConfig> { + return defineBaseNotificationsConfig(config) +}This also requires importing
DefineConfigValuefrom@holo-js/config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/notifications/src/index.ts` around lines 89 - 91, The wrapper defineNotificationsConfig currently omits an explicit return type which can allow the public API to drift; update the function signature for defineNotificationsConfig to explicitly return DefineConfigValue<TConfig> (matching the base defineNotificationsConfig in packages/config/src/loader.ts) and add an import for DefineConfigValue from '@holo-js/config', keeping the implementation returning defineBaseNotificationsConfig(config) unchanged so callers get the stable, declared API type.packages/notifications/src/runtime.ts (1)
1008-1017: Optional: reuse theAnonymousRoutesWithChannelhelper fromcontracts.tsto avoid duplicating the type expression.The exact same
Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in TChannel]: NotificationRouteFor<TChannel> }>pattern is now defined asAnonymousRoutesWithChannelinpackages/notifications/src/contracts.ts(lines 223-228). Consider exporting that helper and using it here to keep the runtime signature and thePendingAnonymousNotification.channelcontract in lock-step.♻️ Proposed refactor
In
packages/notifications/src/contracts.ts, export the helper:-type AnonymousRoutesWithChannel< +export type AnonymousRoutesWithChannel< TRoutes extends Partial<{ readonly [TChannel in NotificationChannelName]: NotificationRouteFor<TChannel> }>, TChannel extends NotificationChannelName, > = Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in TChannel]: NotificationRouteFor<TChannel> }>Then in
runtime.ts:+ type AnonymousRoutesWithChannel, ... channel<TChannel extends NotificationChannelName>( channel: TChannel, route: NotificationRouteFor<TChannel>, - ): PendingAnonymousNotification<Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in TChannel]: NotificationRouteFor<TChannel> }>> { + ): PendingAnonymousNotification<AnonymousRoutesWithChannel<TRoutes, TChannel>> { const normalizedChannel = normalizeOptionalString(channel, 'Notification channel') return new AnonymousNotificationBuilder({ ...this.target.routes, [normalizedChannel]: route, - } as Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in TChannel]: NotificationRouteFor<TChannel> }>) + } as AnonymousRoutesWithChannel<TRoutes, TChannel>) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/notifications/src/runtime.ts` around lines 1008 - 1017, Export the existing type helper AnonymousRoutesWithChannel from packages/notifications/src/contracts.ts and then update the channel method in runtime.ts to use AnonymousRoutesWithChannel instead of the inline Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in TChannel]: NotificationRouteFor<TChannel> }>; i.e., import AnonymousRoutesWithChannel and replace the long type annotation on the PendingAnonymousNotification return and the AnonymousNotificationBuilder constructor argument with AnonymousRoutesWithChannel<TRoutes, TChannel> so the runtime signature and PendingAnonymousNotification.channel remain consistent.packages/cli/tests/authorization-registry.test.ts (1)
159-162: Optional: assert regex matches before checking contents.If
renderGeneratedAuthorizationTypesever changes the ordering (e.g., emitsrecordActionsbeforeactor, or drops theinput:marker) the regex will silently returnundefinedandexpect(undefined).toContain('actor: object')will throw a less obviousreceived value must be a stringerror. Consider assertingpolicyEntry/abilityEntryare defined first to make regressions easier to diagnose.♻️ Suggested tweak
- const policyEntry = /"posts": \{[\s\S]*?recordActions:/.exec(types)?.[0] - const abilityEntry = /"reports\.export": \{[\s\S]*?input:/.exec(types)?.[0] - expect(policyEntry).toContain('actor: object') - expect(abilityEntry).toContain('actor: object') + const policyEntry = /"posts": \{[\s\S]*?recordActions:/.exec(types)?.[0] + const abilityEntry = /"reports\.export": \{[\s\S]*?input:/.exec(types)?.[0] + expect(policyEntry).toBeDefined() + expect(abilityEntry).toBeDefined() + expect(policyEntry).toContain('actor: object') + expect(abilityEntry).toContain('actor: object')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/authorization-registry.test.ts` around lines 159 - 162, The test currently uses regex extraction (policyEntry and abilityEntry) and immediately calls expect(...).toContain('actor: object'), which will throw a confusing "received value must be a string" error if the regex returns undefined; update the test around renderGeneratedAuthorizationTypes so you first assert the regex matches (e.g., expect(policyEntry).toBeDefined() and expect(abilityEntry).toBeDefined()) before asserting their contents, referencing the existing policyEntry and abilityEntry variables to make failures clear and diagnosable.packages/storage/tests/facade.test.ts (1)
337-354: Starttry/finallybefore mutating globals.If
Object.definePropertyorresetStorageRuntime()throws during setup, the currentfinallyblocks will not run, which can leak partially stubbedglobalThisstate into later tests.🧪 Proposed test-isolation fix
- Object.defineProperty(globalThis, 'useRuntimeConfig', { - configurable: true, - writable: true, - value: () => runtimeConfig, - }) - Object.defineProperty(globalThis, 'useStorage', { - configurable: true, - writable: true, - value: () => backend, - }) - resetStorageRuntime() - try { + Object.defineProperty(globalThis, 'useRuntimeConfig', { + configurable: true, + writable: true, + value: () => runtimeConfig, + }) + Object.defineProperty(globalThis, 'useStorage', { + configurable: true, + writable: true, + value: () => backend, + }) + resetStorageRuntime() + await expect(useStorage('public').exists('avatars/user-1.txt')).resolves.toBe(false) expect(useStorage('public').url('avatars/user-1.txt')).toBe('https://app.test/storage/avatars/user-1.txt') } finally { restoreGlobalProperty('useRuntimeConfig', previousUseRuntimeConfig) restoreGlobalProperty('useStorage', previousUseStorage)- Reflect.deleteProperty(globalThis, 'useRuntimeConfig') - Reflect.deleteProperty(globalThis, 'useStorage') - resetStorageRuntime() - try { + Reflect.deleteProperty(globalThis, 'useRuntimeConfig') + Reflect.deleteProperty(globalThis, 'useStorage') + resetStorageRuntime() + expect(() => useStorage('local')).toThrow('Storage runtime is not configured') } finally { restoreGlobalProperty('useRuntimeConfig', previousUseRuntimeConfig) restoreGlobalProperty('useStorage', previousUseStorage)Also applies to: 362-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/tests/facade.test.ts` around lines 337 - 354, Start the try block before you mutate globals so restores always run if setup throws: save previousUseRuntimeConfig and previousUseStorage, then open try { ... } immediately, inside the try call Object.defineProperty on globalThis for 'useRuntimeConfig' and 'useStorage' and call resetStorageRuntime(), run the assertions (useStorage(...).exists and .url), and in the finally call restoreGlobalProperty('useRuntimeConfig', previousUseRuntimeConfig) and restoreGlobalProperty('useStorage', previousUseStorage); apply the same change to the other test block around lines 362-370 to ensure test isolation.packages/storage-s3/src/index.ts (1)
218-229: Consider simplifying Blob construction from the typed-array payload.The IIFE manually unwraps the
ArrayBuffer, handlesbyteOffset/byteLengthviews, and falls back toslice().bufferfor non-ArrayBufferbacking. Node.js v18+ (and undici via undici#1601) correctly handlebyteOffsetandbyteLengthdirectly in theBlobconstructor when passed aUint8ArrayorBufferview, respecting the view's offset and length per spec. Since this package targets modern Node versions,new Blob([payloadBytes])produces identical results without the defensive unwrapping. This removes noticeable complexity from the code path:- const requestBody = payloadBytes - ? (() => { - const { buffer, byteOffset, byteLength } = payloadBytes - const arrayBuffer = buffer instanceof ArrayBuffer - ? (byteOffset === 0 && byteLength === buffer.byteLength - ? buffer - : buffer.slice(byteOffset, byteOffset + byteLength)) - : payloadBytes.slice().buffer - - return new Blob([arrayBuffer]) - })() - : undefined + const requestBody = payloadBytes ? new Blob([payloadBytes]) : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage-s3/src/index.ts` around lines 218 - 229, The current IIFE that unwraps payloadBytes into an ArrayBuffer before constructing requestBody is overly defensive; simplify by passing the typed-array/view directly to the Blob constructor. Replace the IIFE that builds requestBody (the block referencing payloadBytes, arrayBuffer, byteOffset, byteLength and new Blob([...])) with a direct construction like new Blob([payloadBytes]) when payloadBytes is present so Blob correctly respects view offsets/lengths on modern Node.js; ensure the variable name requestBody and the conditional (payloadBytes ? ... : undefined) remain unchanged.packages/auth/src/runtime.ts (1)
249-255: Consolidate duplicaterequireRecordValue/requireUserRecordhelpers.
requireUserRecordat lines 764-770 has an identical body torequireRecordValuehere. Since they're used interchangeably (e.g., theserializeUserserialize-fallback usesrequireRecordValuewhilecreateEmailVerificationFacade.createusesrequireUserRecordfor the same kind of check), keep a single helper to reduce drift.♻️ Proposed consolidation
-function requireUserRecord(user: unknown, message: string): Record<string, unknown> { - if (!user || typeof user !== 'object') { - throw new Error(message) - } - - return user as Record<string, unknown> -} - function getGuardProviderAdapter(Then replace the
requireUserRecord(...)call at line 1916 withrequireRecordValue(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/runtime.ts` around lines 249 - 255, There are duplicate helper functions requireRecordValue and requireUserRecord with identical logic; keep a single helper (preferably requireRecordValue) and remove the duplicate requireUserRecord; update all call sites that currently call requireUserRecord (for example in createEmailVerificationFacade.create) and any other places (including the serializeUser serialize-fallback) to call requireRecordValue instead, ensuring imports/exports are adjusted so the single helper is available everywhere.packages/auth-social/src/index.ts (1)
339-346: Redundant null guard afterresolveUserRecord.
resolveUserRecordalready returnsnullonly when the adapter result isnull/undefined(otherwise it throws). Since the subsequentif (!linkedUser)branch also throws with the same message, collapse to a singlerequireUserRecordcall.♻️ Proposed simplification
if (existingIdentity) { - const linkedUser = resolveUserRecord( - await adapter.findById(existingIdentity.userId), - `[`@holo-js/auth-social`] Linked social identity "${provider}:${profile.id}" references a missing local user.`, - ) - if (!linkedUser) { - throw new Error(`[`@holo-js/auth-social`] Linked social identity "${provider}:${profile.id}" references a missing local user.`) - } + const linkedUser = requireUserRecord( + await adapter.findById(existingIdentity.userId), + `[`@holo-js/auth-social`] Linked social identity "${provider}:${profile.id}" references a missing local user.`, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-social/src/index.ts` around lines 339 - 346, Replace the resolveUserRecord + manual null check with a single required lookup: call requireUserRecord(await adapter.findById(existingIdentity.userId), `[`@holo-js/auth-social`] Linked social identity "${provider}:${profile.id}" references a missing local user.`) instead of assigning linkedUser from resolveUserRecord and then throwing on !linkedUser; this removes the redundant null guard around resolveUserRecord/linkedUser and centralizes the error behavior in requireUserRecord.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth/src/runtime.ts`:
- Around line 1364-1375: requireUserId currently treats any string or number as
valid, allowing empty/whitespace-only strings and non-finite numbers; update
requireUserId to reject string ids that are empty or only whitespace and numeric
ids that are not finite (NaN/Infinity) by throwing the provided message when
those cases occur, and also adjust extractUserId to apply the same
non-empty/finite checks so fallbacks to user.id can't reintroduce the gap;
reference the requireUserId and extractUserId functions and ensure the
validation logic mirrors the auth-social behavior (trim string and check length,
and use Number.isFinite for numbers) before returning the id.
In `@packages/events/src/contracts.ts`:
- Around line 338-341: The exported function normalizeListenerDefinition
currently accepts only ListenerDefinitionInput<TInput, TResult> but needs an
overload to accept unknown for structural compatibility with
EventsModule.normalizeListenerDefinition(value: unknown); add an overload
signature normalizeListenerDefinition(value: unknown):
ListenerDefinition<unknown, unknown> (or similar) above the existing generic
declaration, keep the existing generic implementation unchanged, and rely on the
runtime isListenerDefinition() check inside normalizeListenerDefinition to
validate the unknown input while preserving the strongly-typed generic signature
for authored listeners (referencing normalizeListenerDefinition,
ListenerDefinitionInput, ListenerDefinition, and isListenerDefinition).
In `@packages/flux-svelte/tests/package.type.test.ts`:
- Line 5: The test currently imports only GeneratedBroadcastManifest and asserts
payloads with toExtend<Record<string, unknown>>() which is too loose; add
BroadcastJsonObject to the type import (alongside GeneratedBroadcastManifest)
and replace each payload assertion that calls toExtend<Record<string,
unknown>>() with toEqualTypeOf<BroadcastJsonObject>() (the four assertions
referenced in the comment around the payload checks for
GeneratedBroadcastManifest) so the inferred payload types are strictly compared
to the BroadcastJsonObject fallback.
In `@packages/flux-vue/tests/package.type.test.ts`:
- Around line 1-4: The type tests use toExtend which only checks structural
assignability; update the assertions in the test file to assert exact expected
types: replace payload assertions to explicitly equal BroadcastJsonObject (use
expectTypeOf(...).toEqualTypeOf<BroadcastJsonObject>() or equivalent) and
replace the status assertion to assert the full
Readonly<Ref<FluxConnectionStatus>> return type (use
expectTypeOf(fluxInternals.status).toEqualTypeOf<Readonly<Ref<FluxConnectionStatus>>>()),
referencing createFluxClient, fluxInternals, GeneratedBroadcastManifest where
the values are derived so the tests validate exact types rather than loose
shapes.
---
Nitpick comments:
In `@packages/auth-social/src/index.ts`:
- Around line 339-346: Replace the resolveUserRecord + manual null check with a
single required lookup: call requireUserRecord(await
adapter.findById(existingIdentity.userId), `[`@holo-js/auth-social`] Linked social
identity "${provider}:${profile.id}" references a missing local user.`) instead
of assigning linkedUser from resolveUserRecord and then throwing on !linkedUser;
this removes the redundant null guard around resolveUserRecord/linkedUser and
centralizes the error behavior in requireUserRecord.
In `@packages/auth/src/runtime.ts`:
- Around line 249-255: There are duplicate helper functions requireRecordValue
and requireUserRecord with identical logic; keep a single helper (preferably
requireRecordValue) and remove the duplicate requireUserRecord; update all call
sites that currently call requireUserRecord (for example in
createEmailVerificationFacade.create) and any other places (including the
serializeUser serialize-fallback) to call requireRecordValue instead, ensuring
imports/exports are adjusted so the single helper is available everywhere.
In `@packages/cli/tests/authorization-registry.test.ts`:
- Around line 159-162: The test currently uses regex extraction (policyEntry and
abilityEntry) and immediately calls expect(...).toContain('actor: object'),
which will throw a confusing "received value must be a string" error if the
regex returns undefined; update the test around
renderGeneratedAuthorizationTypes so you first assert the regex matches (e.g.,
expect(policyEntry).toBeDefined() and expect(abilityEntry).toBeDefined()) before
asserting their contents, referencing the existing policyEntry and abilityEntry
variables to make failures clear and diagnosable.
In `@packages/notifications/src/index.ts`:
- Around line 89-91: The wrapper defineNotificationsConfig currently omits an
explicit return type which can allow the public API to drift; update the
function signature for defineNotificationsConfig to explicitly return
DefineConfigValue<TConfig> (matching the base defineNotificationsConfig in
packages/config/src/loader.ts) and add an import for DefineConfigValue from
'@holo-js/config', keeping the implementation returning
defineBaseNotificationsConfig(config) unchanged so callers get the stable,
declared API type.
In `@packages/notifications/src/runtime.ts`:
- Around line 1008-1017: Export the existing type helper
AnonymousRoutesWithChannel from packages/notifications/src/contracts.ts and then
update the channel method in runtime.ts to use AnonymousRoutesWithChannel
instead of the inline Readonly<Omit<TRoutes, TChannel> & { readonly [TKey in
TChannel]: NotificationRouteFor<TChannel> }>; i.e., import
AnonymousRoutesWithChannel and replace the long type annotation on the
PendingAnonymousNotification return and the AnonymousNotificationBuilder
constructor argument with AnonymousRoutesWithChannel<TRoutes, TChannel> so the
runtime signature and PendingAnonymousNotification.channel remain consistent.
In `@packages/storage-s3/src/index.ts`:
- Around line 218-229: The current IIFE that unwraps payloadBytes into an
ArrayBuffer before constructing requestBody is overly defensive; simplify by
passing the typed-array/view directly to the Blob constructor. Replace the IIFE
that builds requestBody (the block referencing payloadBytes, arrayBuffer,
byteOffset, byteLength and new Blob([...])) with a direct construction like new
Blob([payloadBytes]) when payloadBytes is present so Blob correctly respects
view offsets/lengths on modern Node.js; ensure the variable name requestBody and
the conditional (payloadBytes ? ... : undefined) remain unchanged.
In `@packages/storage/tests/facade.test.ts`:
- Around line 337-354: Start the try block before you mutate globals so restores
always run if setup throws: save previousUseRuntimeConfig and
previousUseStorage, then open try { ... } immediately, inside the try call
Object.defineProperty on globalThis for 'useRuntimeConfig' and 'useStorage' and
call resetStorageRuntime(), run the assertions (useStorage(...).exists and
.url), and in the finally call restoreGlobalProperty('useRuntimeConfig',
previousUseRuntimeConfig) and restoreGlobalProperty('useStorage',
previousUseStorage); apply the same change to the other test block around lines
362-370 to ensure test isolation.
🪄 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: edc2278b-3db8-4884-bf08-6ef1cdd109b0
📒 Files selected for processing (24)
packages/auth-social/src/index.tspackages/auth/src/runtime.tspackages/authorization/src/contracts.tspackages/cli/tests/authorization-registry.test.tspackages/config/src/loader.tspackages/events/src/contracts.tspackages/events/src/index.tspackages/events/tests/contracts.test.tspackages/events/tests/contracts.type.test.tspackages/flux-react/src/index.tspackages/flux-react/tests/package.type.test.tspackages/flux-svelte/src/index.tspackages/flux-svelte/tests/package.type.test.tspackages/flux-vue/src/index.tspackages/flux-vue/tests/package.type.test.tspackages/mail/tests/runtime.test.tspackages/notifications/src/contracts.tspackages/notifications/src/index.tspackages/notifications/src/runtime.tspackages/storage-s3/src/index.tspackages/storage-s3/tests/storage-s3.test.tspackages/storage/tests/facade.test.tspackages/storage/tests/s3Driver.test.tsscripts/run-test-typecheck.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/events/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/flux-react/tests/package.type.test.ts
- packages/events/tests/contracts.type.test.ts
- scripts/run-test-typecheck.mjs
- packages/events/tests/contracts.test.ts
- packages/storage/tests/s3Driver.test.ts
Comment on lines
+1364
to
+1375
| function requireUserId( | ||
| adapter: ErasedAuthProviderAdapter, | ||
| user: unknown, | ||
| message: string, | ||
| ): string | number { | ||
| const userId = extractUserId(adapter, user) | ||
| if (typeof userId !== 'string' && typeof userId !== 'number') { | ||
| throw new Error(message) | ||
| } | ||
|
|
||
| return userId | ||
| } |
There was a problem hiding this comment.
Tighten requireUserId to reject empty strings and non-finite numbers for parity with auth-social.
packages/auth-social/src/index.ts (lines 115-137) rejects empty/whitespace-only string ids and non-finite numbers, but this counterpart accepts '', ' ', NaN, and Infinity as valid user ids. That means a misbehaving adapter can produce a PAT record with userId: '' (or NaN), which then flows into tokenStore.create, updateUserRecord, and the password-reset consume path.
🛡️ Proposed fix to align with auth-social
function requireUserId(
adapter: ErasedAuthProviderAdapter,
user: unknown,
message: string,
): string | number {
const userId = extractUserId(adapter, user)
- if (typeof userId !== 'string' && typeof userId !== 'number') {
- throw new Error(message)
+ if (typeof userId === 'string') {
+ const normalized = userId.trim()
+ if (!normalized) {
+ throw new Error(message)
+ }
+ return normalized
+ }
+
+ if (typeof userId === 'number' && Number.isFinite(userId)) {
+ return userId
}
- return userId
+ throw new Error(message)
}You may also want extractUserId to apply the same finite/non-empty filter so fallbacks to user.id don't reintroduce the gap.
📝 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.
Suggested change
| function requireUserId( | |
| adapter: ErasedAuthProviderAdapter, | |
| user: unknown, | |
| message: string, | |
| ): string | number { | |
| const userId = extractUserId(adapter, user) | |
| if (typeof userId !== 'string' && typeof userId !== 'number') { | |
| throw new Error(message) | |
| } | |
| return userId | |
| } | |
| function requireUserId( | |
| adapter: ErasedAuthProviderAdapter, | |
| user: unknown, | |
| message: string, | |
| ): string | number { | |
| const userId = extractUserId(adapter, user) | |
| if (typeof userId === 'string') { | |
| const normalized = userId.trim() | |
| if (!normalized) { | |
| throw new Error(message) | |
| } | |
| return normalized | |
| } | |
| if (typeof userId === 'number' && Number.isFinite(userId)) { | |
| return userId | |
| } | |
| throw new Error(message) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth/src/runtime.ts` around lines 1364 - 1375, requireUserId
currently treats any string or number as valid, allowing empty/whitespace-only
strings and non-finite numbers; update requireUserId to reject string ids that
are empty or only whitespace and numeric ids that are not finite (NaN/Infinity)
by throwing the provided message when those cases occur, and also adjust
extractUserId to apply the same non-empty/finite checks so fallbacks to user.id
can't reintroduce the gap; reference the requireUserId and extractUserId
functions and ensure the validation logic mirrors the auth-social behavior (trim
string and check length, and use Number.isFinite for numbers) before returning
the id.
Comment on lines
338
to
+341
| export function normalizeListenerDefinition< | ||
| TInput extends EventReferenceInput, | ||
| TListener extends ListenerDefinition<TInput>, | ||
| >(listener: TListener): TListener { | ||
| TResult, | ||
| >(listener: ListenerDefinitionInput<TInput, TResult>): ListenerDefinition<TInput, TResult> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify normalizeListenerDefinition remains assignable to the core EventsModule contract.
# Expect: Any assignment or `satisfies EventsModule` check involving the events module should not fail
# because normalizeListenerDefinition requires ListenerDefinitionInput instead of accepting unknown.
rg -n -C3 'normalizeListenerDefinition|EventsModule' --type=tsRepository: cobraprojects/holo-js
Length of output: 14811
🏁 Script executed:
# Check the type definitions
rg -n 'type NormalizedListenerDefinition|type ListenerDefinition' packages/ --type=ts -A 3Repository: cobraprojects/holo-js
Length of output: 1026
🏁 Script executed:
# Check the actual function implementation to understand runtime validation
sed -n '338,365p' packages/events/src/contracts.tsRepository: cobraprojects/holo-js
Length of output: 1522
🏁 Script executed:
# Verify how listener is passed in holo.ts - check the type of 'listener' before the normalizeListenerDefinition call
sed -n '3465,3480p' packages/core/src/portable/holo.tsRepository: cobraprojects/holo-js
Length of output: 829
🏁 Script executed:
# Get the full ListenerDefinition type definition
rg -n 'export type ListenerDefinition' packages/events/src/contracts.ts -A 15Repository: cobraprojects/holo-js
Length of output: 618
🏁 Script executed:
# Check if there's a TypeScript error when the eventsModule is assigned
# Look for any type assertions or satisfies clauses
rg -n 'satisfies.*EventsModule|as.*EventsModule' packages/ --type=tsRepository: cobraprojects/holo-js
Length of output: 191
🏁 Script executed:
# Verify if tests check this contract compatibility
cat packages/events/tests/contracts.type.test.tsRepository: cobraprojects/holo-js
Length of output: 3925
🏁 Script executed:
# Look for the full ListenerDefinition type definition
rg -n 'export type ListenerDefinition[^I]' packages/events/src/contracts.ts -A 20Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if the code actually builds/compiles without errors by looking at the test results
# Also check if there are any type assertion workarounds in place
rg -n 'as ListenerDefinitionInput|as unknown' packages/core/src/portable/holo.tsRepository: cobraprojects/holo-js
Length of output: 169
🏁 Script executed:
# Check the resolveListenerExport function to understand what type is returned
sed -n '1522,1550p' packages/core/src/portable/holo.tsRepository: cobraprojects/holo-js
Length of output: 1139
🏁 Script executed:
# Check the loadEventsModule function to see if there's a type assertion
sed -n '1349,1365p' packages/core/src/portable/holo.tsRepository: cobraprojects/holo-js
Length of output: 905
🏁 Script executed:
# Check if there's a TypeScript build that reports errors
# Look for tsconfig and check compilation
fd tsconfig.json | head -5Repository: cobraprojects/holo-js
Length of output: 239
🏁 Script executed:
# Check if the function can actually accept unknown or if it causes a type error
# by looking at how it's called with the listener (which is unknown from resolveListenerExport)
rg -n 'const listener = resolveListenerExport' packages/core/src/portable/holo.ts -A 5Repository: cobraprojects/holo-js
Length of output: 384
🏁 Script executed:
# Check the CLI discovery module to see how it expects the function signature
sed -n '165,180p' packages/cli/src/project/shared.tsRepository: cobraprojects/holo-js
Length of output: 652
🏁 Script executed:
# Check if there's a way the module exports are typed to satisfy the EventsModule interface
# Look for how the events module is exported from the package
cat packages/events/src/index.tsRepository: cobraprojects/holo-js
Length of output: 1653
🏁 Script executed:
# Run a TypeScript type check to see if there's actually an error
# Check the actual ListenerDefinition full type
rg -n '^export type ListenerDefinition' packages/events/src/contracts.ts -A 10Repository: cobraprojects/holo-js
Length of output: 443
🏁 Script executed:
# Check if there are already function overloads for normalizeListenerDefinition
sed -n '330,390p' packages/events/src/contracts.ts | grep -A 60 'export function normalizeListenerDefinition'Repository: cobraprojects/holo-js
Length of output: 2219
🏁 Script executed:
# Check if there's a CLI module that might wrap this with different typing
rg -n 'normalizeListenerDefinition' packages/cli/src/project/discovery.ts -B 3 -A 3Repository: cobraprojects/holo-js
Length of output: 517
🏁 Script executed:
# Verify what the actual return type should be - check if ListenerDefinition structurally matches NormalizedListenerDefinition
rg -n 'type ListenerDefinition[^I]' packages/events/src/contracts.ts -A 25 | head -50Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if there's a build configuration that might suppress type errors
fd package.json packages/events -x cat {} | grep -A 10 '"scripts"'Repository: cobraprojects/holo-js
Length of output: 310
🏁 Script executed:
# Verify the actual ListenerDefinition type definition (we got interrupted before)
rg -n 'export type ListenerDefinition[^I]' packages/events/src/contracts.ts -A 30Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if the module export has any type assertion to satisfy EventsModule
rg -n 'EventsModule|satisfies' packages/events/src/ --type=tsRepository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Search for ListenerDefinition type definition more broadly
rg 'export (type|interface) ListenerDefinition' packages/events/src/contracts.ts -A 30Repository: cobraprojects/holo-js
Length of output: 1328
🏁 Script executed:
# Check if the module is actually used and if there are TypeScript errors by looking at the package build
cd packages/events && npm run typecheck 2>&1 | head -50Repository: cobraprojects/holo-js
Length of output: 759
🏁 Script executed:
# Look at the actual file content around the ListenerDefinition type
sed -n '140,200p' packages/events/src/contracts.tsRepository: cobraprojects/holo-js
Length of output: 1972
🏁 Script executed:
# Check if there's any type assertion or casting in the imports/exports path
rg -n 'importOptionalModule|as EventsModule' packages/core/src/portable/holo.ts -B 2 -A 2Repository: cobraprojects/holo-js
Length of output: 7008
🏁 Script executed:
# Verify the actual issue by checking how TypeScript would resolve the module type
# Look at the complete EventsModule definition
sed -n '309,330p' packages/core/src/portable/holo.tsRepository: cobraprojects/holo-js
Length of output: 930
Add an overload to accept unknown inputs for structural EventsModule compatibility.
The exported normalizeListenerDefinition signature requires ListenerDefinitionInput<TInput, TResult>, but packages/core/src/portable/holo.ts expects it to match EventsModule.normalizeListenerDefinition(value: unknown). The function performs runtime validation via isListenerDefinition(), so accepting unknown is safe. Add an overload to accept unknown while preserving the typed signature for authored listeners:
Proposed overload
export function normalizeListenerDefinition<
TInput extends EventReferenceInput,
TResult,
>(listener: ListenerDefinitionInput<TInput, TResult>): ListenerDefinition<TInput, TResult>
+export function normalizeListenerDefinition(listener: unknown): ListenerDefinition
export function normalizeListenerDefinition(listener: unknown): ListenerDefinition {
if (!isListenerDefinition(listener)) {
throw new Error('[Holo Events] Listeners must define "listensTo" and a "handle" function.')
}
+ const definition = listener as ListenerDefinitionInput<EventReferenceInput, unknown>
const queue = normalizeOptionalBoolean(listener.queue, 'Listener queue')
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/events/src/contracts.ts` around lines 338 - 341, The exported
function normalizeListenerDefinition currently accepts only
ListenerDefinitionInput<TInput, TResult> but needs an overload to accept unknown
for structural compatibility with
EventsModule.normalizeListenerDefinition(value: unknown); add an overload
signature normalizeListenerDefinition(value: unknown):
ListenerDefinition<unknown, unknown> (or similar) above the existing generic
declaration, keep the existing generic implementation unchanged, and rely on the
runtime isListenerDefinition() check inside normalizeListenerDefinition to
validate the unknown input while preserving the strongly-typed generic signature
for authored listeners (referencing normalizeListenerDefinition,
ListenerDefinitionInput, ListenerDefinition, and isListenerDefinition).
| import type { Readable } from 'svelte/store' | ||
| import type { FluxConnectionStatus } from '@holo-js/flux' | ||
| import { createFluxClient, fluxInternals } from '@holo-js/flux' | ||
| import type { GeneratedBroadcastManifest } from '@holo-js/broadcast' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect payload fallback typing and loose Svelte type-test assertions.
rg -n -C3 'type BroadcastPayloadFor|type BroadcastJsonObject|toExtend<Record<string, unknown>>' packages/broadcast/src/contracts.ts packages/flux-svelte/tests/package.type.test.tsRepository: cobraprojects/holo-js
Length of output: 2824
🏁 Script executed:
# Get the complete BroadcastPayloadFor type definition
sed -n '329,340p' packages/broadcast/src/contracts.tsRepository: cobraprojects/holo-js
Length of output: 610
🏁 Script executed:
# Check the index/exports of the broadcast package
cat packages/broadcast/src/index.tsRepository: cobraprojects/holo-js
Length of output: 3983
🏁 Script executed:
# View the complete package.type.test.ts file to understand context
wc -l packages/flux-svelte/tests/package.type.test.tsRepository: cobraprojects/holo-js
Length of output: 115
🏁 Script executed:
# Check the current imports in the test file
head -20 packages/flux-svelte/tests/package.type.test.tsRepository: cobraprojects/holo-js
Length of output: 732
Tighten payload type assertions to match fallback type.
The payload assertions use toExtend<Record<string, unknown>>(), which only checks broad assignability. This can miss regressions where the payload type narrows from the expected BroadcastJsonObject fallback to something incompatible.
Add BroadcastJsonObject to the imports and change the assertions to toEqualTypeOf<BroadcastJsonObject>() on lines 47, 50, 53, and 56:
Change
-import type { GeneratedBroadcastManifest } from '@holo-js/broadcast'
+import type { BroadcastJsonObject, GeneratedBroadcastManifest } from '@holo-js/broadcast'Then on each payload assertion:
-expectTypeOf(payload).toExtend<Record<string, unknown>>()
+expectTypeOf(payload).toEqualTypeOf<BroadcastJsonObject>()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flux-svelte/tests/package.type.test.ts` at line 5, The test
currently imports only GeneratedBroadcastManifest and asserts payloads with
toExtend<Record<string, unknown>>() which is too loose; add BroadcastJsonObject
to the type import (alongside GeneratedBroadcastManifest) and replace each
payload assertion that calls toExtend<Record<string, unknown>>() with
toEqualTypeOf<BroadcastJsonObject>() (the four assertions referenced in the
comment around the payload checks for GeneratedBroadcastManifest) so the
inferred payload types are strictly compared to the BroadcastJsonObject
fallback.
Comment on lines
1
to
+4
| import { describe, expectTypeOf, it } from 'vitest' | ||
| import type { Ref } from 'vue' | ||
| import type { FluxConnectionStatus } from '@holo-js/flux' | ||
| import { createFluxClient, fluxInternals } from '@holo-js/flux' | ||
| import type { GeneratedBroadcastManifest } from '@holo-js/broadcast' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Vue composable return type and broad type-test assertions.
rg -n -C3 'useFluxConnectionStatus|toExtend<Record<string, unknown>>|toExtend<\{ readonly value: FluxConnectionStatus \}>' packages/flux-vue/src/index.ts packages/flux-vue/tests/package.type.test.tsRepository: cobraprojects/holo-js
Length of output: 3392
🏁 Script executed:
rg -n 'BroadcastJsonObject' --type tsRepository: cobraprojects/holo-js
Length of output: 10829
🏁 Script executed:
fd -e ts -e d.ts | xargs grep -l 'BroadcastJsonObject' 2>/dev/null | head -20Repository: cobraprojects/holo-js
Length of output: 432
Strengthen Vue type assertions to catch regressions.
The toExtend assertions only check structural assignability and miss type regressions. The payload assertions should explicitly match BroadcastJsonObject, and the status assertion should verify the full Readonly<Ref<FluxConnectionStatus>> return type rather than just a shape with a value property.
Proposed type test improvements
import { describe, expectTypeOf, it } from 'vitest'
+import type { Ref } from 'vue'
import type { FluxConnectionStatus } from '@holo-js/flux'
import { createFluxClient, fluxInternals } from '@holo-js/flux'
-import type { GeneratedBroadcastManifest } from '@holo-js/broadcast'
+import type { BroadcastJsonObject, GeneratedBroadcastManifest } from '@holo-js/broadcast'
@@
const generic = useFlux('orders.1', 'orders.updated', payload => {
- expectTypeOf(payload).toExtend<Record<string, unknown>>()
+ expectTypeOf(payload).toEqualTypeOf<BroadcastJsonObject>()
}, { client })
const genericMany = useFlux('orders.1', ['orders.updated', 'orders.shipped'], payload => {
- expectTypeOf(payload).toExtend<Record<string, unknown>>()
+ expectTypeOf(payload).toEqualTypeOf<BroadcastJsonObject>()
}, { client })
const pub = useFluxPublic('feed.1', 'orders.updated', payload => {
- expectTypeOf(payload).toExtend<Record<string, unknown>>()
+ expectTypeOf(payload).toEqualTypeOf<BroadcastJsonObject>()
}, { client })
const priv = useFluxPrivate('orders.1', 'orders.shipped', payload => {
- expectTypeOf(payload).toExtend<Record<string, unknown>>()
+ expectTypeOf(payload).toEqualTypeOf<BroadcastJsonObject>()
}, { client })
@@
- expectTypeOf(status).toExtend<{ readonly value: FluxConnectionStatus }>()
+ expectTypeOf(status).toEqualTypeOf<Readonly<Ref<FluxConnectionStatus>>>()📝 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.
Suggested change
| import { describe, expectTypeOf, it } from 'vitest' | |
| import type { Ref } from 'vue' | |
| import type { FluxConnectionStatus } from '@holo-js/flux' | |
| import { createFluxClient, fluxInternals } from '@holo-js/flux' | |
| import type { GeneratedBroadcastManifest } from '@holo-js/broadcast' | |
| import { describe, expectTypeOf, it } from 'vitest' | |
| import type { Ref } from 'vue' | |
| import type { FluxConnectionStatus } from '@holo-js/flux' | |
| import { createFluxClient, fluxInternals } from '@holo-js/flux' | |
| import type { BroadcastJsonObject, GeneratedBroadcastManifest } from '@holo-js/broadcast' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flux-vue/tests/package.type.test.ts` around lines 1 - 4, The type
tests use toExtend which only checks structural assignability; update the
assertions in the test file to assert exact expected types: replace payload
assertions to explicitly equal BroadcastJsonObject (use
expectTypeOf(...).toEqualTypeOf<BroadcastJsonObject>() or equivalent) and
replace the status assertion to assert the full
Readonly<Ref<FluxConnectionStatus>> return type (use
expectTypeOf(fluxInternals.status).toEqualTypeOf<Readonly<Ref<FluxConnectionStatus>>>()),
referencing createFluxClient, fluxInternals, GeneratedBroadcastManifest where
the values are derived so the tests validate exact types rather than loose
shapes.
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
New Features
Bug Fixes
Chores