Refactor core packages and simplify shared logic#53
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR extracts shared helpers across adapters, auth, cache, broadcast, and CLI runtime flows, and updates the affected integrations, tests, and workspace wiring to use them. ChangesAdapter helper extraction
Auth runtime consolidation
Cache and CLI runtime refactors
Runtime utility consolidation
Sequence Diagram(s)sequenceDiagram
participant CLI
participant RuntimeProcess
participant RuntimeWorker
participant Database
CLI->>RuntimeProcess: createRuntimeInvocation()
RuntimeProcess->>RuntimeWorker: HOLO_RUNTIME_PAYLOAD
RuntimeWorker->>Database: configureDB() and command flow
RuntimeWorker-->>CLI: stdout with executed items
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 (1)
packages/create-holo-js/tsconfig.json (1)
6-13:⚠️ Potential issue | 🟡 MinorRemove unused
@holo-js/cache-dbpath alias.The TypeScript path alias for
@holo-js/cache-dbis added tocreate-holo-js/tsconfig.json(lines 7-9), but no direct imports of@holo-js/cache-dbexist in thecreate-holo-js/src/directory. This path alias is unused and should be removed to keep the configuration clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/create-holo-js/tsconfig.json` around lines 6 - 13, The `@holo-js/cache-db` path alias is defined in the paths object of the tsconfig.json file but is not used anywhere in the create-holo-js package's source directory. Remove the entire path alias entry for `@holo-js/cache-db` (which maps to ../cache-db/src/index.ts) from the paths configuration, keeping only the `@holo-js/cli` alias which is actually being used.
🧹 Nitpick comments (3)
packages/cache/src/flexible.ts (1)
124-130: 💤 Low valuePotential time inconsistency when using custom
nowprovider.On line 127,
resolveFlexibleEnvelopeState(retried)uses the defaultDate.now()instead of theoptions.now?.()that was used earlier (line 95). If a customnowfunction is provided (e.g., for testing with fake timers), this could cause inconsistent state evaluation between the initial read and the retry path.Suggested fix
const retried = await options.read() if ( isFlexibleEnvelope<TValue>(retried) - && resolveFlexibleEnvelopeState(retried) !== 'expired' + && resolveFlexibleEnvelopeState(retried, options.now?.() ?? Date.now()) !== 'expired' ) { return retried.value }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cache/src/flexible.ts` around lines 124 - 130, The retry path in the flexible cache is using the default system time instead of respecting the custom time provider. On line 127, the call to `resolveFlexibleEnvelopeState(retried)` does not pass the `options.now` parameter, causing it to use `Date.now()` internally instead of the custom time function that was provided in options. Pass the `options.now` function (or equivalent time provider) to the `resolveFlexibleEnvelopeState()` call to ensure consistent time evaluation between the initial read and the retry path.packages/cache-db/src/index.ts (1)
236-244: ⚡ Quick winArray index access is fragile if table order changes.
Lines 237 and 243 access
CACHE_DATABASE_TABLE_DEFINITIONS[0]and[1]directly. If the array order changes, this code silently uses the wrong definition. Consider usingfind()by role for clarity and safety.if (!(await schema.hasTable(tableName))) { + const entriesDefinition = CACHE_DATABASE_TABLE_DEFINITIONS.find(d => d.role === 'entries')! await schema.createTable(tableName, (table) => { - applyCacheDatabaseTableDefinition(table, tableName, CACHE_DATABASE_TABLE_DEFINITIONS[0]) + applyCacheDatabaseTableDefinition(table, tableName, entriesDefinition) }) } if (!(await schema.hasTable(lockTableName))) { + const locksDefinition = CACHE_DATABASE_TABLE_DEFINITIONS.find(d => d.role === 'locks')! await schema.createTable(lockTableName, (table) => { - applyCacheDatabaseTableDefinition(table, lockTableName, CACHE_DATABASE_TABLE_DEFINITIONS[1]) + applyCacheDatabaseTableDefinition(table, lockTableName, locksDefinition) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cache-db/src/index.ts` around lines 236 - 244, Replace the direct array index accesses to CACHE_DATABASE_TABLE_DEFINITIONS[0] and CACHE_DATABASE_TABLE_DEFINITIONS[1] with find() calls that filter by role instead. In the applyCacheDatabaseTableDefinition call for the main table (currently using index 0), use find() to locate the definition with the appropriate role for the cache table. In the applyCacheDatabaseTableDefinition call for the lock table (currently using index 1), use find() to locate the definition with the appropriate role for the lock table. This ensures the correct definition is used even if the array order changes.packages/cli/src/runtime-worker.ts (1)
336-356: 💤 Low valueAdd type validation for
payload.options.modelsbefore iteration.Line 336 assumes
payload.options.modelsis an array if truthy, butArray.isArraycheck is done. However, thefor...ofloop at line 342 iterates without ensuring eachnameis a string first (the check is inside the loop). Consider validating upfront or documenting the expected shape.Actually, reviewing more closely: the check
typeof name !== 'string'at line 343-344 usescontinue, silently skipping non-string entries. This could mask misconfiguration. Consider logging or warning when invalid entries are encountered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/runtime-worker.ts` around lines 336 - 356, The code currently silently skips non-string entries in the requested models array using a `continue` statement when the `typeof name !== 'string'` check fails. Instead of silently skipping these invalid entries, add a warning or error log to alert users when non-string model names are encountered in the payload.options.models array. This will help surface misconfiguration issues rather than masking them with silent failures. Modify the block that handles the `typeof name !== 'string'` check to include appropriate logging before the `continue` statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adapter-sveltekit/src/realtime.ts`:
- Around line 40-66: The isReactiveObject function is too permissive and allows
proxying of branded instances like Map, Set, and class instances, which breaks
method calls due to lost receiver binding. Narrow the isReactiveObject function
to only return true for plain objects and arrays by adding additional instanceof
checks to exclude Map, Set, WeakMap, WeakSet, and other branded types alongside
the existing Date and Blob exclusions. This will ensure that only structural
data types are wrapped when shouldWrapValue is invoked in the
createRealtimeReactiveValue function.
In `@packages/auth-social-apple/src/index.ts`:
- Around line 76-82: The cache parameter in the fetchAppleJwks function is
creating a new Map() on every invocation, causing the cache to be empty and
unused each time. Move the Map() cache creation outside of the fetchAppleJwks
function to module level as a constant (similar to how APPLE_JWKS_URL is
defined), and then reference this module-level cache constant in the cache
property of the options object passed to
authRuntimeInternals.jwt.fetchCachedJwks. This ensures the cache persists across
multiple calls to fetchAppleJwks.
In `@packages/auth-social-linkedin/tests/package.test.ts`:
- Around line 26-29: The scopes array from callbackConfig is being reused by
reference in the config object, which allows mutations to leak between test
contexts if exchangeCode modifies the scopes array. Fix this by creating a
shallow copy of callbackConfig.scopes instead of reusing the same reference.
When assigning the scopes property in the config object (in the section where
you have scopes: overrides.scopes ?? callbackConfig.scopes), ensure that when
using callbackConfig.scopes, you create a new array instance (for example, by
spreading or slicing the original array) so that each test context gets its own
independent copy.
In `@packages/broadcast/src/worker.ts`:
- Around line 418-424: The logger helper functions logSocketMessageError and
logScalingMessageError require Error types, but upstream catch blocks receive
unknown values and cast them unsafely as Error. If a callback throws null or a
non-Error value, accessing error.message will throw inside the logger and bypass
cleanup/close paths. Change both functions to accept unknown instead of Error,
then safely extract the message by checking if the value is an Error instance
before accessing .message, using a fallback string for non-Error values. This
ensures logger failures cannot mask or interfere with error handling in catch
paths.
In `@packages/cli/src/runtime.ts`:
- Around line 365-379: The createRuntimeInvocation function unconditionally uses
the --experimental-strip-types flag when the worker path ends with .ts, but this
flag is only available in Node.js 22.6.0+ while the project supports ^20.19.0.
Add Node.js version detection at the top of the createRuntimeInvocation function
to check the current Node version. When the worker path ends with .ts,
conditionally include the --experimental-strip-types flag only if the Node
version is 22.6.0 or higher. For Node.js 20, provide an alternative approach
such as using the .mjs file variant instead of attempting to process the .ts
file directly with the experimental flag.
In `@packages/config/src/defaults.ts`:
- Around line 533-543: The parseInteger function hardcodes "Holo Queue" as the
scope label when calling parseScopedInteger, which causes misleading error
messages when this shared helper is used by non-queue flows like
session/auth/provider name parsing. Similarly, normalizeConnectionName also
hardcodes "Holo Queue" for the same reason. Remove the hardcoded scope labels
from these shared helper functions by adding a new parameter (e.g., scope or
namespace) to both parseInteger and normalizeConnectionName to accept the scope
label as an argument. Then update all callers of these functions to pass the
appropriate scope label for their use case, such as "Holo Queue" for
queue-related calls and other descriptive labels for session/auth/provider
contexts. This way, the error messages will correctly reflect the actual
subsystem context regardless of which flow is using the helper.
---
Outside diff comments:
In `@packages/create-holo-js/tsconfig.json`:
- Around line 6-13: The `@holo-js/cache-db` path alias is defined in the paths
object of the tsconfig.json file but is not used anywhere in the create-holo-js
package's source directory. Remove the entire path alias entry for
`@holo-js/cache-db` (which maps to ../cache-db/src/index.ts) from the paths
configuration, keeping only the `@holo-js/cli` alias which is actually being
used.
---
Nitpick comments:
In `@packages/cache-db/src/index.ts`:
- Around line 236-244: Replace the direct array index accesses to
CACHE_DATABASE_TABLE_DEFINITIONS[0] and CACHE_DATABASE_TABLE_DEFINITIONS[1] with
find() calls that filter by role instead. In the
applyCacheDatabaseTableDefinition call for the main table (currently using index
0), use find() to locate the definition with the appropriate role for the cache
table. In the applyCacheDatabaseTableDefinition call for the lock table
(currently using index 1), use find() to locate the definition with the
appropriate role for the lock table. This ensures the correct definition is used
even if the array order changes.
In `@packages/cache/src/flexible.ts`:
- Around line 124-130: The retry path in the flexible cache is using the default
system time instead of respecting the custom time provider. On line 127, the
call to `resolveFlexibleEnvelopeState(retried)` does not pass the `options.now`
parameter, causing it to use `Date.now()` internally instead of the custom time
function that was provided in options. Pass the `options.now` function (or
equivalent time provider) to the `resolveFlexibleEnvelopeState()` call to ensure
consistent time evaluation between the initial read and the retry path.
In `@packages/cli/src/runtime-worker.ts`:
- Around line 336-356: The code currently silently skips non-string entries in
the requested models array using a `continue` statement when the `typeof name
!== 'string'` check fails. Instead of silently skipping these invalid entries,
add a warning or error log to alert users when non-string model names are
encountered in the payload.options.models array. This will help surface
misconfiguration issues rather than masking them with silent failures. Modify
the block that handles the `typeof name !== 'string'` check to include
appropriate logging before the `continue` statement.
🪄 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: 782438d7-6ce3-4f4b-b407-d18831894ea6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (81)
packages/adapter-next/src/runtime.tspackages/adapter-next/tests/client.test.tspackages/adapter-nuxt/src/realtime-definition-transform.tspackages/adapter-nuxt/src/runtime/composables/client-errors.tspackages/adapter-nuxt/src/runtime/composables/forms.tspackages/adapter-nuxt/src/runtime/composables/object.tspackages/adapter-nuxt/src/runtime/composables/realtime.tspackages/adapter-nuxt/src/runtime/plugins/realtime.client.tspackages/adapter-nuxt/src/runtime/server/auto-imports/holo.tspackages/adapter-nuxt/src/runtime/server/imports/storage.tspackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/preprocess.tspackages/adapter-sveltekit/src/reactive-view.tspackages/adapter-sveltekit/src/realtime-definition-transform.tspackages/adapter-sveltekit/src/realtime.tspackages/adapter-sveltekit/src/transform-utils.tspackages/auth-clerk/src/index.tspackages/auth-clerk/src/jwt.tspackages/auth-clerk/src/request.tspackages/auth-social-apple/package.jsonpackages/auth-social-apple/src/index.tspackages/auth-social-discord/src/index.tspackages/auth-social-facebook/package.jsonpackages/auth-social-facebook/src/index.tspackages/auth-social-github/src/index.tspackages/auth-social-google/src/index.tspackages/auth-social-linkedin/src/index.tspackages/auth-social-linkedin/tests/package.test.tspackages/auth-social/src/index.tspackages/auth-workos/src/contracts.tspackages/auth-workos/src/index.tspackages/auth/src/next/server.tspackages/auth/src/nuxt/server.tspackages/auth/src/runtime.tspackages/auth/src/runtime/jwt.tspackages/auth/src/runtime/optionalSecurity.tspackages/auth/src/runtime/requestNormalization.tspackages/auth/src/runtime/setCookieParser.tspackages/auth/src/sveltekit/client.tspackages/auth/src/sveltekit/server.tspackages/auth/tests/framework.test.tspackages/auth/tests/package.test.tspackages/authorization/src/runtime.tspackages/broadcast/src/auth.tspackages/broadcast/src/contracts.tspackages/broadcast/src/json.tspackages/broadcast/src/runtime.tspackages/broadcast/src/worker.tspackages/broadcast/tests/worker.test.tspackages/cache-db/src/index.tspackages/cache-db/tests/package.test.tspackages/cache-redis/src/index.tspackages/cache/src/db.tspackages/cache/src/facade.tspackages/cache/src/flexible.tspackages/cache/src/optional-driver.tspackages/cache/src/query-bridge.tspackages/cache/src/redis.tspackages/cli/package.jsonpackages/cli/src/cache-migrations.tspackages/cli/src/cli-internals.tspackages/cli/src/cli.tspackages/cli/src/dev.tspackages/cli/src/generators.tspackages/cli/src/media-migrations.tspackages/cli/src/project/discovery-helpers.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/queue-migrations.tspackages/cli/src/runtime-worker.tspackages/cli/src/runtime.tspackages/cli/src/security.tspackages/cli/tests/cli.test.tspackages/cli/tsconfig.jsonpackages/cli/tsup.config.tspackages/cli/vitest.config.tspackages/config/src/access.tspackages/config/src/defaults.tspackages/core/tests/package.test.tspackages/create-holo-js/tsconfig.jsonpackages/db-postgres/src/index.ts
💤 Files with no reviewable changes (3)
- packages/cli/src/cli-internals.ts
- packages/cache-db/tests/package.test.ts
- packages/cli/src/generators.ts
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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