Merged
Conversation
…and logging improvements - Updated pnpm-lock.yaml to include 'rolldown' as a new optional dependency and upgraded 'minimatch' to version 10.2.4. - Added a debug log statement in risk-scores.tsx to indicate when the sign-up risk engine is disabled in the public build.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSimplified backend sign-up risk engine loading: made the engine type internal, replaced multi-path resolution with a single filesystem path + dynamic import, added an inline zero-score fallback and centralized error handling, updated tests, bumped Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Loader as RiskEngineLoader
participant FS as FileSystem
participant Importer as DynamicImporter
participant Engine as SignUpRiskEngine
Caller->>Loader: calculateSignUpRiskAssessment(args)
Loader->>FS: resolvePrivateEnginePath() / check existence
FS-->>Loader: path exists / not found
alt path exists
Loader->>Importer: dynamic import(resolvedPath)
Importer-->>Loader: module
alt module has valid engine
Loader->>Engine: engine.calculateRiskAssessment(args)
Engine-->>Caller: assessment
else invalid export
Loader-->>Caller: throw StackAssertionError
end
else path not found
Loader->>Loader: use INLINE_ZERO_SCORE_ENGINE (timestamp = new Date())
Loader-->>Caller: zero-score assessment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a debug log to the fallback sign-up risk engine path (when disabled) and updates the packages/private submodule pointer.
Changes:
- Log a debug message when the sign-up risk engine is disabled and
fallbackEngineis used. - Bump
packages/privatesubmodule commit reference.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/private | Updates the git submodule commit pointer. |
| apps/backend/src/lib/risk-scores.tsx | Adds a debug log when the fallback (disabled) risk engine runs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/private:1
- The PR description mentions pnpm-lock/minimatch/rolldown changes, but the diff shown includes a
packages/privatesubmodule bump. Please update the PR description to explicitly call out why this submodule pointer changed (or split it into a separate PR if unrelated).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR significantly refactors the private sign-up risk engine loader in Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[calculateSignUpRiskAssessment] --> B[getEngine]
B --> C{cachedEnginePromise\nnot null?}
C -- yes --> D[await cached promise]
C -- no --> E[loadEngine]
E --> F{PRIVATE_ENGINE_PATH\n== null?}
F -- yes --> G[console.debug\nreturn ZERO_SCORE_ENGINE]
F -- no --> H[loadEngineFromPath]
H --> I{import succeeds?}
I -- no --> J[captureError\nreturn ZERO_SCORE_ENGINE]
I -- yes --> K{isSignUpRiskEngine\nvalidation}
K -- fails --> L[captureError\nreturn ZERO_SCORE_ENGINE]
K -- passes --> M[console.info\nreturn engine]
E --> N[cache enginePromise]
N --> O{await resolves?}
O -- error --> P[clear cache\nthrow error]
O -- ok --> Q[return engine]
D --> Q
G --> Q
J --> Q
L --> Q
M --> Q
Q --> R[calculateRiskAssessmentWithFallback]
R --> S{engine.calculateRiskAssessment\nthrows?}
S -- yes --> T[captureError\nreturn createZeroRiskAssessment]
S -- no --> U[return assessment]
Last reviewed commit: "Refactor risk-scores..." |
…agement - Removed unused imports and streamlined the risk engine dependencies in risk-scores.tsx. - Updated pnpm-lock.yaml to adjust minimatch version and include new dependencies for balanced-match and brace-expansion. - Enhanced the private engine loading logic for better clarity and error handling.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/backend/src/lib/risk-scores.tsx (1)
103-108: Consider caching the loading promise to prevent redundant loads.If multiple concurrent calls arrive when
cachedEngineis null, each will invokeloadEngine()before any completes and caches. While not a correctness issue (the engine is idempotent), caching the promise avoids redundant I/O.♻️ Optional fix using promise caching
-let cachedEngine: SignUpRiskEngine | null = null; +let cachedEnginePromise: Promise<SignUpRiskEngine> | null = null; async function getEngine(): Promise<SignUpRiskEngine> { - if (cachedEngine == null) { - cachedEngine = await loadEngine(); + if (cachedEnginePromise == null) { + cachedEnginePromise = loadEngine(); } - return cachedEngine; + return cachedEnginePromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/risk-scores.tsx` around lines 103 - 108, getEngine currently checks cachedEngine and calls loadEngine() which can be invoked multiple times if concurrent callers race; change to cache the loading promise: introduce a module-level variable (e.g., cachedEnginePromise) and in getEngine, if cachedEngine is null and cachedEnginePromise is null set cachedEnginePromise = loadEngine(); await cachedEnginePromise, assign its result to cachedEngine, clear cachedEnginePromise, and return cachedEngine; ensure subsequent callers await cachedEnginePromise instead of calling loadEngine() directly so only one loadEngine() runs concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/backend/src/lib/risk-scores.tsx`:
- Around line 103-108: getEngine currently checks cachedEngine and calls
loadEngine() which can be invoked multiple times if concurrent callers race;
change to cache the loading promise: introduce a module-level variable (e.g.,
cachedEnginePromise) and in getEngine, if cachedEngine is null and
cachedEnginePromise is null set cachedEnginePromise = loadEngine(); await
cachedEnginePromise, assign its result to cachedEngine, clear
cachedEnginePromise, and return cachedEngine; ensure subsequent callers await
cachedEnginePromise instead of calling loadEngine() directly so only one
loadEngine() runs concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ebb2fd3-75ba-470c-b502-eab134988aeb
📒 Files selected for processing (2)
apps/backend/src/lib/risk-scores.tsxpackages/private
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/private
…or handling - Introduced a new function, resolvePrivateEnginePath, to streamline the logic for locating the private sign-up risk engine. - Enhanced the loadEngine function to utilize the new resolution method and improved logging for better clarity on engine loading status. - Updated tests to reflect changes in engine loading behavior and ensure proper functionality when the private engine is available or not.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/lib/risk-scores.tsx`:
- Around line 179-187: The tests referencing the private engine should be
skipped when the private package is not present: wrap the two
import.meta.vitest?.test calls to conditionally skip using HAS_PRIVATE_ENGINE
(e.g., use import.meta.vitest?.test.skipIf(!HAS_PRIVATE_ENGINE) for the first
test and likewise for the second), so the "resolvePrivateEnginePath finds the
engine in the monorepo" and "loadEngine loads the real engine when available"
tests only run when HAS_PRIVATE_ENGINE is true; ensure you still call
loadEngine() in the second test when running to verify calculateRiskAssessment
exists, but avoid running it in public/CI environments without the private
package.
- Around line 90-96: Guard against PRIVATE_ENGINE_PATH possibly being null
before dynamically importing: import throwErr from
"@stackframe/stack-shared/dist/utils/errors" (or the named export throwErr) and
add a defensive check where you use PRIVATE_ENGINE_PATH (the block that does
await import(/* webpackIgnore: true */ PRIVATE_ENGINE_PATH) and references
HAS_PRIVATE_ENGINE) — if PRIVATE_ENGINE_PATH is null/undefined call throwErr or
throw a StackAssertionError with context instead of passing null into import;
then proceed to import and validate mod.signUpRiskEngine as currently done so
TypeScript's strict-null checks are satisfied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3dd481b-2fb2-431c-ab5f-dc560cda09ec
📒 Files selected for processing (1)
apps/backend/src/lib/risk-scores.tsx
…or reporting - Updated the private engine resolution logic to streamline path detection and ensure compatibility with the monorepo structure. - Introduced a fallback mechanism in calculateRiskAssessmentWithFallback to handle errors gracefully, capturing relevant context for better debugging. - Refactored the getEngine function to utilize cached engine instances, improving performance and reducing redundant imports. - Updated tests to validate the new error handling and engine loading behavior.
- Introduced a new function, getPrivateEnginePathOrThrow, to ensure the PRIVATE_ENGINE_PATH is defined, improving error reporting during engine loading. - Refactored the getEngine function to utilize the new path retrieval method, enhancing clarity in logging. - Updated tests to skip when PRIVATE_ENGINE_PATH is not available, ensuring more robust test execution and coverage for engine loading scenarios.
Collaborator
Author
|
@greptile-ai review |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/backend/src/lib/risk-scores.tsx (1)
232-232: Prefer defensive pattern over non-null assertion.While the callback only executes when vitest is available, the guideline recommends defensive coding with explicit error messages. As per coding guidelines: "Prefer
?? throwErr(...)over non-null assertions with good error messages."Proposed fix
- const { vi } = import.meta.vitest!; + const { vi } = import.meta.vitest ?? throwErr("import.meta.vitest should be defined inside vitest test callback");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/risk-scores.tsx` at line 232, Replace the non-null assertion on import.meta.vitest with a defensive check that throws a clear error when vitest is unavailable: instead of using import.meta.vitest! to assign vi, use a nullish-coalescing pattern that calls a descriptive throw (e.g., import.meta.vitest ?? throwErr('vitest not available when running risk-scores tests')) so the assignment to vi fails with an actionable message; update the assignment that initializes vi to reference import.meta.vitest and the throw helper or an Error to provide the explicit error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/backend/src/lib/risk-scores.tsx`:
- Line 232: Replace the non-null assertion on import.meta.vitest with a
defensive check that throws a clear error when vitest is unavailable: instead of
using import.meta.vitest! to assign vi, use a nullish-coalescing pattern that
calls a descriptive throw (e.g., import.meta.vitest ?? throwErr('vitest not
available when running risk-scores tests')) so the assignment to vi fails with
an actionable message; update the assignment that initializes vi to reference
import.meta.vitest and the throw helper or an Error to provide the explicit
error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cff96163-b9ac-48c0-b8f1-b116a466b755
📒 Files selected for processing (1)
apps/backend/src/lib/risk-scores.tsx
…isms - Replaced cachedEngine with cachedEnginePromise to improve asynchronous handling of the sign-up risk engine. - Introduced loadEngineFromPath function for better error handling during engine imports, ensuring fallback to zero scores on failure. - Updated getEngine function to utilize the new promise-based caching, enhancing performance and clarity. - Modified tests to reflect changes in engine loading behavior and ensure proper functionality when the private engine is unavailable.
Collaborator
Author
|
@greptile-ai review |
N2D4
approved these changes
Mar 21, 2026
N2D4
approved these changes
Mar 21, 2026
- Introduced getEngineWithLoader function to enhance the loading mechanism of the sign-up risk engine, ensuring that rejected promises clear the cached engine. - Updated the getEngine function to utilize the new loader, maintaining performance while improving error management. - Added a test case for getEngineWithLoader to verify that rejected promises correctly reset the cached engine state.
Collaborator
Author
|
@greptile-ai review |
- Removed the getPrivateEnginePathOrThrow function and replaced it with a type guard, isSignUpRiskEngine, to streamline engine validation. - Updated loadEngine and loadEngineFromPath functions to enhance error handling, ensuring fallback to zero scores when the private engine is invalid. - Simplified the getEngine function by removing the loader wrapper, directly utilizing loadEngine for better clarity. - Modified tests to validate the new engine loading behavior and ensure proper fallback mechanisms are in place.
Collaborator
Author
|
@greptile-ai review n rescore |
This was referenced Mar 21, 2026
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
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
Bug Fixes
Chores