Repository navigation
clean leaked framework logic - #16
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request refactors multiple framework blog applications (Next.js, Nuxt, SvelteKit) and the CLI/DB packages to eliminate static schema imports at module load time. Schema initialization is deferred to runtime via dynamic preloading in test runners and CLI processes. Models now use string identifiers instead of imported schema objects, and query builders use direct model methods rather than Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 2 seconds.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: 1
🧹 Nitpick comments (1)
packages/cli/tests/cli.test.ts (1)
642-643: ⚡ Quick winMake the “no
schema.generated” checks less brittle (match static import/require patterns).Right now the test asserts the generated helper files contain no substring
schema.generated. That can fail even when the leak you care about (static/module-load-time import of the schema artifact) is already fixed, because any incidental occurrence (comments/strings/paths unrelated to static imports) will break the test.Prefer matching static import/require usage instead (e.g.,
import ... from '...schema.generated...'orrequire('...schema.generated...')) and asserting that those patterns are absent.🛠️ Proposed adjustment (apply to all 3 occurrences)
- expect(await readFile(join(projectRoot, 'server/holo.ts'), 'utf8')).not.toContain('schema.generated') - expect(await readFile(join(projectRoot, 'app/layout.tsx'), 'utf8')).not.toContain('schema.generated') + const staticSchemaImportPattern = + /(import\s+[^;]+?from\s+['"][^'"]*schema\.generated['"]|require\s*\(\s*['"][^'"]*schema\.generated['"])/m + expect(await readFile(join(projectRoot, 'server/holo.ts'), 'utf8')).not.toMatch(staticSchemaImportPattern) + expect(await readFile(join(projectRoot, 'app/layout.tsx'), 'utf8')).not.toMatch(staticSchemaImportPattern)- expect(await readFile(join(nextRoot, 'server/holo.ts'), 'utf8')).not.toContain('schema.generated') - expect(await readFile(join(nextRoot, 'app/layout.tsx'), 'utf8')).not.toContain('schema.generated') + expect(await readFile(join(nextRoot, 'server/holo.ts'), 'utf8')).not.toMatch(staticSchemaImportPattern) + expect(await readFile(join(nextRoot, 'app/layout.tsx'), 'utf8')).not.toMatch(staticSchemaImportPattern)- expect(await readFile(join(svelteRoot, 'src/lib/server/holo.ts'), 'utf8')).not.toContain('schema.generated') + expect(await readFile(join(svelteRoot, 'src/lib/server/holo.ts'), 'utf8')).not.toMatch(staticSchemaImportPattern)Also applies to: 1432-1434, 1446-1447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/cli.test.ts` around lines 642 - 643, Replace the brittle substring assertions that check server/holo.ts and app/layout.ts for "schema.generated" with regex-based checks that only match static import/require patterns (e.g., /import\s+[^;]+from\s+['"].*schema\.generated.*['"]/ and /require\(\s*['"].*schema\.generated.*['"]\s*\)/); update the two expect calls that read join(projectRoot, 'server/holo.ts') and join(projectRoot, 'app/layout.ts') (and the two other occurrences around the file) to assert that those regexes do not match the file contents instead of using not.toContain('schema.generated'), so incidental occurrences in comments/strings won’t fail the test.
🤖 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/cli/src/project/registry.ts`:
- Around line 80-88: The importProjectModule call in
renderGeneratedSchemaRuntimeArtifact loads the project's schema which registers
tables into the process-global registry and can leak state; to fix this snapshot
and restore the registry around the import (or clear it before import and
restore previous entries after), i.e. before calling importProjectModule take a
copy of the current registry state (using your registry API), clear the
registry, perform importProjectModule(projectRoot, schemaPath) and call
extractGeneratedSchemaTables(moduleValue) to get the tables without relying on
load-time side effects, then restore the registry to the snapped state so no
generated tables remain in the CLI process; update
renderGeneratedSchemaRuntimeArtifact to perform this
save/clear/import/extract/restore sequence.
---
Nitpick comments:
In `@packages/cli/tests/cli.test.ts`:
- Around line 642-643: Replace the brittle substring assertions that check
server/holo.ts and app/layout.ts for "schema.generated" with regex-based checks
that only match static import/require patterns (e.g.,
/import\s+[^;]+from\s+['"].*schema\.generated.*['"]/ and
/require\(\s*['"].*schema\.generated.*['"]\s*\)/); update the two expect calls
that read join(projectRoot, 'server/holo.ts') and join(projectRoot,
'app/layout.ts') (and the two other occurrences around the file) to assert that
those regexes do not match the file contents instead of using
not.toContain('schema.generated'), so incidental occurrences in comments/strings
won’t fail the test.
🪄 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: 76f22dec-91c5-4063-a0e6-2f1861b4f782
📒 Files selected for processing (35)
apps/blog-next/app/layout.tsxapps/blog-next/server/holo.tsapps/blog-next/server/lib/blog.tsapps/blog-next/server/models/Category.tsapps/blog-next/server/models/Post.tsapps/blog-next/server/models/Tag.tsapps/blog-next/server/models/User.tsapps/blog-next/tests/blog-logic.mjsapps/blog-next/tests/run.mjsapps/blog-nuxt/server/lib/blog.tsapps/blog-nuxt/server/models/Category.tsapps/blog-nuxt/server/models/Post.tsapps/blog-nuxt/server/models/Tag.tsapps/blog-nuxt/server/models/User.tsapps/blog-nuxt/server/plugins/generated-schema.tsapps/blog-nuxt/tests/blog-logic.mjsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/server/models/Category.tsapps/blog-sveltekit/server/models/Post.tsapps/blog-sveltekit/server/models/Tag.tsapps/blog-sveltekit/server/models/User.tsapps/blog-sveltekit/src/lib/server/blog.tsapps/blog-sveltekit/src/lib/server/holo.tsapps/blog-sveltekit/tests/blog-logic.mjsapps/blog-sveltekit/tests/run.mjspackages/adapter-next/src/runtime.tspackages/cli/src/project/discovery.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/project/shared.tspackages/cli/tests/cli.test.tspackages/db/src/index.tspackages/db/src/schema/generated.tspackages/db/src/schema/index.tspackages/db/tests/generated-schema.test.ts
💤 Files with no reviewable changes (11)
- apps/blog-next/server/models/Tag.ts
- apps/blog-next/server/holo.ts
- apps/blog-next/server/models/Category.ts
- apps/blog-sveltekit/server/models/User.ts
- apps/blog-sveltekit/src/lib/server/holo.ts
- apps/blog-sveltekit/server/models/Post.ts
- apps/blog-sveltekit/server/models/Category.ts
- apps/blog-next/server/models/User.ts
- apps/blog-nuxt/server/plugins/generated-schema.ts
- apps/blog-sveltekit/server/models/Tag.ts
- apps/blog-next/server/models/Post.ts
Comment on lines
+80
to
+88
| async function renderGeneratedSchemaRuntimeArtifact( | ||
| projectRoot: string, | ||
| schemaEntry: string, | ||
| ): Promise<string> { | ||
| const schemaPath = resolve(projectRoot, schemaEntry) | ||
| const moduleValue = await importProjectModule(projectRoot, schemaPath) | ||
| const tables = extractGeneratedSchemaTables(moduleValue) | ||
| return renderGeneratedSchemaRuntimeModule(tables) | ||
| } |
There was a problem hiding this comment.
Avoid leaking generated tables into the CLI process.
importProjectModule(projectRoot, schemaPath) executes the schema module, and those generated schema modules register tables into the process-global registry on load. That means this artifact-generation step mutates shared DB state, so tables from one project/run can bleed into later work in the same process. Please clear/restore the registry around this import, or extract the exported tables without relying on the module’s load-time side effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/project/registry.ts` around lines 80 - 88, The
importProjectModule call in renderGeneratedSchemaRuntimeArtifact loads the
project's schema which registers tables into the process-global registry and can
leak state; to fix this snapshot and restore the registry around the import (or
clear it before import and restore previous entries after), i.e. before calling
importProjectModule take a copy of the current registry state (using your
registry API), clear the registry, perform importProjectModule(projectRoot,
schemaPath) and call extractGeneratedSchemaTables(moduleValue) to get the tables
without relying on load-time side effects, then restore the registry to the
snapped state so no generated tables remain in the CLI process; update
renderGeneratedSchemaRuntimeArtifact to perform this
save/clear/import/extract/restore sequence.
…t): The importProjectModule call does trigger registerGeneratedTables as a side effect of loading the generated schema module, which writes to globalThis.__holoGeneratedTables__. However, the CLI process never reads from that global map — extractGeneratedSchemaTables reads directly from the module's exports. The leaked state is inert and has no observable effect on CLI behavior. Adding snapshot/restore logic would be unnecessary complexity.
Finding 2 (brittle substring assertions in tests): The .not.toContain('schema.generated') assertions check scaffolded files whose content is fully controlled by the framework's code generators. A regex matching only import/require patterns would actually be weaker — it would miss other forms of leakage (dynamic imports, string paths, etc.). The current assertions are appropriately strict for their purpose.
- No changes needed.
- fix cli test
This was referenced May 5, 2026
Merged
Merged
Merged
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
Release Notes
New Features
Chores