refactor: split provider-inventory into 2 interfaces#3197
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughLaunches provider-inventory in worker threads for REST and providers-sync interfaces; adds bootstrapEntry to load providers before app bootstrap; inlines Hono app construction per interface; guards APP_INITIALIZER execution; updates drizzle schema usage and tsup build entrypoints. ChangesMulti-interface worker-thread architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3197 +/- ##
==========================================
- Coverage 64.03% 63.23% -0.81%
==========================================
Files 1096 1057 -39
Lines 26685 25669 -1016
Branches 6474 6307 -167
==========================================
- Hits 17089 16233 -856
+ Misses 8394 8246 -148
+ Partials 1202 1190 -12
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/split-pi
branch
from
14ebb7d to
d970d07
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/provider-inventory/src/services/start-server/start-server.spec.ts (1)
169-171: ⚡ Quick winAdd coverage for the unregistered-initializer branch.
Hard-coding
isRegistered()totruemeans the new guard never gets exercised in this file. A regression where startup should skipresolveAll(APP_INITIALIZER)would still pass here.🤖 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 `@apps/provider-inventory/src/services/start-server/start-server.spec.ts` around lines 169 - 171, The test currently hard-codes the DependencyContainer mock's isRegistered to always return true so the branch where APP_INITIALIZER is not registered is never exercised; change the mock for isRegistered to be configurable (e.g., use vi.fn().mockImplementation((token) => input?.containerRegistered ?? true) or similar) and add a test case where isRegistered returns false while resolveAll would have returned initializers; assert that resolveAll(APP_INITIALIZER) is not called and the startup path that skips initializers runs (locate the DependencyContainer mock, its isRegistered and resolveAll methods, and the test calling the start-server routine in start-server.spec.ts to implement this).apps/provider-inventory/tsup.config.ts (1)
24-24: ⚡ Quick winAppend to
NODE_OPTIONSinstead of replacing it.The current command overwrites
NODE_OPTIONS, dropping any flags already set in the environment. Watch mode could then behave differently from the normal dev/prod runtime if other Node options are configured.Use parameter expansion to preserve existing flags:
- onSuccess: overrideOptions.watch && !isProduction ? "NODE_OPTIONS='--allow-worker' npm run prod" : undefined, + onSuccess: overrideOptions.watch && !isProduction ? "NODE_OPTIONS=\"${NODE_OPTIONS:+$NODE_OPTIONS }--allow-worker\" npm run prod" : undefined,🤖 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 `@apps/provider-inventory/tsup.config.ts` at line 24, The onSuccess command currently replaces NODE_OPTIONS and loses any existing flags; update the onSuccess branch in tsup.config.ts (the expression using overrideOptions.watch and isProduction) to append --allow-worker to the existing NODE_OPTIONS using shell parameter expansion rather than assigning a new value so existing options are preserved when running the "npm run prod" command in watch mode.
🤖 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 `@apps/provider-inventory/src/bootstrap-entry.ts`:
- Line 7: The dynamic import currently uses a hardcoded TypeScript extension
("await import(\"./providers/index.ts\")") which will fail at runtime after tsup
compilation; change the import specifier to be extensionless (e.g., import
"./providers/index" or "./providers") so Node's resolver can load the compiled
.js; apply the same change to the other dynamic import occurrences noted in
server.ts (the import expressions at lines referenced in the review) to avoid
runtime module resolution errors.
In `@apps/provider-inventory/src/server.ts`:
- Around line 87-90: The current Promise named `exited` treats both "exit" and
"error" events on `ref` as a clean resolution, hiding worker failures; change it
so the "error" event causes the Promise to reject (or triggers process
termination with non-zero exit) while only the "exit" event resolves
successfully. Locate the `exited` Promise and update its handlers on `ref` to
call resolve on "exit" and call reject (or call process.exit(1) / propagate the
error) on "error", ensuring the parent process observes and fails on worker
errors instead of silently continuing.
In `@apps/provider-inventory/src/services/start-server/start-server.ts`:
- Around line 47-49: The guard using container.isRegistered(APP_INITIALIZER) is
non-recursive and can miss parent-registered initializers when startServer() is
invoked with a child container; change the check to
container.isRegistered(APP_INITIALIZER, true) so it matches
resolveAll(APP_INITIALIZER) behavior, then continue to await
Promise.all(container.resolveAll(APP_INITIALIZER).map(initializer =>
initializer[ON_APP_START]())); ensure you reference APP_INITIALIZER,
container.isRegistered, container.resolveAll, ON_APP_START and startServer when
making the change.
---
Nitpick comments:
In `@apps/provider-inventory/src/services/start-server/start-server.spec.ts`:
- Around line 169-171: The test currently hard-codes the DependencyContainer
mock's isRegistered to always return true so the branch where APP_INITIALIZER is
not registered is never exercised; change the mock for isRegistered to be
configurable (e.g., use vi.fn().mockImplementation((token) =>
input?.containerRegistered ?? true) or similar) and add a test case where
isRegistered returns false while resolveAll would have returned initializers;
assert that resolveAll(APP_INITIALIZER) is not called and the startup path that
skips initializers runs (locate the DependencyContainer mock, its isRegistered
and resolveAll methods, and the test calling the start-server routine in
start-server.spec.ts to implement this).
In `@apps/provider-inventory/tsup.config.ts`:
- Line 24: The onSuccess command currently replaces NODE_OPTIONS and loses any
existing flags; update the onSuccess branch in tsup.config.ts (the expression
using overrideOptions.watch and isProduction) to append --allow-worker to the
existing NODE_OPTIONS using shell parameter expansion rather than assigning a
new value so existing options are preserved when running the "npm run prod"
command in watch mode.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 515991e7-281d-4ed3-8d4a-88e0c486f36c
📒 Files selected for processing (9)
apps/provider-inventory/src/bootstrap-entry.tsapps/provider-inventory/src/providers-sync-app.tsapps/provider-inventory/src/providers/index.tsapps/provider-inventory/src/providers/stream-boostrap.provider.tsapps/provider-inventory/src/rest-app.tsapps/provider-inventory/src/server.tsapps/provider-inventory/src/services/start-server/start-server.spec.tsapps/provider-inventory/src/services/start-server/start-server.tsapps/provider-inventory/tsup.config.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/providers/stream-boostrap.provider.ts
- apps/provider-inventory/src/providers/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/provider-inventory/src/providers/drizzle.provider.ts`:
- Line 7: Replace the relative import of the model schemas with the project path
alias: change the import that brings in modelsSchema (currently `import * as
modelsSchema from "../model-schemas"`) to use the `@src/*` alias (e.g., `import
* as modelsSchema from "`@src/model-schemas`"`), update any build/tsconfig alias
config if needed, and ensure the symbol `modelsSchema` is still referenced
correctly in drizzle.provider.ts (and any export/consume sites).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65b11cbf-bdd5-4599-aa9c-514eb15256fc
📒 Files selected for processing (10)
apps/provider-inventory/src/bootstrap-entry.tsapps/provider-inventory/src/providers-sync-app.tsapps/provider-inventory/src/providers/drizzle.provider.tsapps/provider-inventory/src/providers/index.tsapps/provider-inventory/src/providers/stream-boostrap.provider.tsapps/provider-inventory/src/rest-app.tsapps/provider-inventory/src/server.tsapps/provider-inventory/src/services/start-server/start-server.spec.tsapps/provider-inventory/src/services/start-server/start-server.tsapps/provider-inventory/tsup.config.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/providers/index.ts
- apps/provider-inventory/src/providers/stream-boostrap.provider.ts
✅ Files skipped from review due to trivial changes (1)
- apps/provider-inventory/src/services/start-server/start-server.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/provider-inventory/src/rest-app.ts
- apps/provider-inventory/src/providers-sync-app.ts
- apps/provider-inventory/tsup.config.ts
- apps/provider-inventory/src/bootstrap-entry.ts
- apps/provider-inventory/src/server.ts
- apps/provider-inventory/src/services/start-server/start-server.ts
baktun14
approved these changes
May 19, 2026
stalniy
force-pushed
the
refactor/split-pi
branch
from
d970d07 to
ed5351d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/provider-inventory/src/services/start-server/start-server.spec.ts (1)
169-171: ⚡ Quick winCover the unregistered-initializer path.
This mock always returns
true, so the new guard's false branch is never exercised. Please add one case withisRegisteredreturningfalseand assertresolveAllandON_APP_STARTare skipped.🤖 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 `@apps/provider-inventory/src/services/start-server/start-server.spec.ts` around lines 169 - 171, Add a test case in start-server.spec.ts that covers the branch where the DependencyContainer reports not registered: create a mocked DependencyContainer where isRegistered returns false and verify that resolveAll is not called and ON_APP_START handlers are not invoked; update the existing test suite that uses the mock<DependencyContainer> (the instance with isRegistered, resolveAll) to include this scenario and assert resolveAll was not called and any ON_APP_START side-effects were skipped when isRegistered returns false.
🤖 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 `@apps/provider-inventory/src/providers-sync-app.ts`:
- Around line 29-31: Wrap the scheduler startup in an awaited try-catch: when
server is truthy, call await on
container.resolve(DiscoverySchedulerService).start() inside a try block; in the
catch log the error (including details) via the app logger/processLogger and
handle failure (e.g., stop the server or exit process with non-zero code or set
a health/failure flag) so the app does not continue accepting requests without a
running scheduler. Ensure you reference DiscoverySchedulerService.start() and
the container.resolve(...) call when making the change.
---
Nitpick comments:
In `@apps/provider-inventory/src/services/start-server/start-server.spec.ts`:
- Around line 169-171: Add a test case in start-server.spec.ts that covers the
branch where the DependencyContainer reports not registered: create a mocked
DependencyContainer where isRegistered returns false and verify that resolveAll
is not called and ON_APP_START handlers are not invoked; update the existing
test suite that uses the mock<DependencyContainer> (the instance with
isRegistered, resolveAll) to include this scenario and assert resolveAll was not
called and any ON_APP_START side-effects were skipped when isRegistered returns
false.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47ae0327-b166-4639-b26a-1f6645626f6c
📒 Files selected for processing (10)
apps/provider-inventory/src/bootstrap-entry.tsapps/provider-inventory/src/providers-sync-app.tsapps/provider-inventory/src/providers/drizzle.provider.tsapps/provider-inventory/src/providers/index.tsapps/provider-inventory/src/providers/stream-boostrap.provider.tsapps/provider-inventory/src/rest-app.tsapps/provider-inventory/src/server.tsapps/provider-inventory/src/services/start-server/start-server.spec.tsapps/provider-inventory/src/services/start-server/start-server.tsapps/provider-inventory/tsup.config.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/providers/index.ts
- apps/provider-inventory/src/providers/stream-boostrap.provider.ts
stalniy
force-pushed
the
refactor/split-pi
branch
from
ed5351d to
887f094
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/provider-inventory/src/providers-sync-app.ts (1)
29-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait scheduler startup in Line 31 so rollback actually works.
try/catchhere only handles sync throws; ifstart()is async, rejection will escape andserver.close()won’t run.Proposed fix
if (server) { try { - container.resolve(DiscoverySchedulerService).start(); + await container.resolve(DiscoverySchedulerService).start(); } catch (error) { server.close(); throw error; } }#!/bin/bash set -euo pipefail # Verify whether DiscoverySchedulerService.start is async / Promise-returning. fd discovery-scheduler.service.ts --type f | while read -r f; do echo "=== $f ===" rg -n -C3 'class\s+DiscoverySchedulerService|start\s*\(' "$f" done🤖 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 `@apps/provider-inventory/src/providers-sync-app.ts` around lines 29 - 35, The try/catch currently only handles synchronous throws from container.resolve(DiscoverySchedulerService).start(); make the startup awaited so rejections trigger the catch and run server.close(): change the surrounding logic to await the Promise returned by DiscoverySchedulerService.start (i.e., call await container.resolve(DiscoverySchedulerService).start()), ensure the enclosing function is async or otherwise handles the returned Promise, and keep the existing catch block to call server.close() and rethrow the error so rollback actually executes on async failures.
🤖 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 `@apps/provider-inventory/src/server.ts`:
- Around line 12-15: The code uses parseInt(rawAppConfig.PORT) which accepts
malformed inputs like "3092foo" and later when INTERFACE === "all" derives
worker ports that can exceed 65535; update the PORT validation (where
rawAppConfig.PORT is parsed into port) to strictly accept only a numeric string
(e.g. /^\d+$/), convert to a Number, verify it's an integer within 0–65535, and
on failure throw or exit with a clear error; additionally, before calling
bootstrapInWorker in the INTERFACE === "all" branch check that port + index <=
65535 for each SUPPORTED_INTERFACES index (or refuse startup if any would
overflow) so bootstrapInWorker({ PORT: String(port + index), INTERFACE:
interfaceName }) always receives a valid port.
In `@apps/provider-inventory/tsup.config.ts`:
- Line 24: The onSuccess command currently embeds a POSIX-only env var
assignment string ("NODE_OPTIONS='--allow-worker' npm run prod") which will fail
on Windows; change the onSuccess value to use cross-env (e.g., "cross-env
NODE_OPTIONS=--allow-worker npm run prod") so the environment variable is set
portably, and update the same pattern in the other tsup config (the occurrence
that also uses onSuccess / overrideOptions.watch and isProduction).
---
Duplicate comments:
In `@apps/provider-inventory/src/providers-sync-app.ts`:
- Around line 29-35: The try/catch currently only handles synchronous throws
from container.resolve(DiscoverySchedulerService).start(); make the startup
awaited so rejections trigger the catch and run server.close(): change the
surrounding logic to await the Promise returned by
DiscoverySchedulerService.start (i.e., call await
container.resolve(DiscoverySchedulerService).start()), ensure the enclosing
function is async or otherwise handles the returned Promise, and keep the
existing catch block to call server.close() and rethrow the error so rollback
actually executes on async failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac5f272a-618d-4d65-9d2b-f4fdc65748fd
📒 Files selected for processing (10)
apps/provider-inventory/src/bootstrap-entry.tsapps/provider-inventory/src/providers-sync-app.tsapps/provider-inventory/src/providers/drizzle.provider.tsapps/provider-inventory/src/providers/index.tsapps/provider-inventory/src/providers/stream-boostrap.provider.tsapps/provider-inventory/src/rest-app.tsapps/provider-inventory/src/server.tsapps/provider-inventory/src/services/start-server/start-server.spec.tsapps/provider-inventory/src/services/start-server/start-server.tsapps/provider-inventory/tsup.config.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/providers/stream-boostrap.provider.ts
- apps/provider-inventory/src/providers/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/provider-inventory/src/services/start-server/start-server.spec.ts
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
2 participants
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.
Why
Closes CON-358
What
Summary by CodeRabbit
New Features
Refactor
Bug Fixes