Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36803 +/- ##
===========================================
- Coverage 66.21% 66.19% -0.02%
===========================================
Files 3384 3385 +1
Lines 115025 115027 +2
Branches 21066 21173 +107
===========================================
- Hits 76165 76144 -21
- Misses 36255 36284 +29
+ Partials 2605 2599 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
chore/refactor-emtpy-runtime
branch
from
850cf7d to
973142b
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
WalkthroughReplaces Deno-specific runtime usage with a generic IRuntimeController. Introduces EmptyRuntime for compiler-error cases. Updates ProxiedApp and multiple managers to use getRuntimeController(). Deno runtime now implements IRuntimeController. Tests refactored to use TestData.getMockApp and a new getMockRuntimeController, removing direct Deno runtime dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Manager as AppManager
participant App as ProxiedApp
participant RT as IRuntimeController
note over Manager,App: Normal run-time request flow
Client->>App: Invoke feature (e.g., API/command)
App->>RT: sendRequest({ method, params })
RT-->>App: Promise<result>
App-->>Client: result
sequenceDiagram
autonumber
participant Manager as AppManager
participant App as ProxiedApp
participant Empty as EmptyRuntime
note over Manager,App: Compile error path
Manager->>Empty: new EmptyRuntime(appId)
Manager->>App: new ProxiedApp(..., Empty)
App->>Empty: sendRequest({ method, params })
Empty-->>App: throw Error("runtime cannot handle requests")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.ts (1)
31-35: Fix setup order: ProxiedApp is created with an undefined managerYou’re constructing
this.mockAppbeforethis.mockManagerexists, and capturing a staleappingetOneById. Reorder to create the manager first and referencethis.mockAppdynamically.Apply this diff:
@@ - this.mockApp = TestData.getMockApp({ id: 'testing', name: 'testing' }, this.mockManager); @@ - const bri = this.mockBridges; - const app = this.mockApp; + const bri = this.mockBridges; @@ - getOneById(appId: string): ProxiedApp { - return appId === 'failMePlease' ? undefined : app; - }, + getOneById(appId: string): ProxiedApp { + return appId === 'failMePlease' ? undefined : this.mockApp; + }, @@ this.mockAccessors = new AppAccessorManager(this.mockManager); const ac = this.mockAccessors; this.mockManager.getAccessorManager = function _getAccessorManager(): AppAccessorManager { return ac; }; + + this.mockApp = TestData.getMockApp({ id: 'testing', name: 'testing' }, this.mockManager);Also applies to: 48-50, 65-70
packages/apps-engine/src/server/managers/AppVideoConfProvider.ts (1)
93-99: Ensure JSON-RPC "method not found" parity (-32601)AppsEngineDenoRuntime exports JSONRPC_METHOD_NOT_FOUND = -32601 and multiple call sites check e.code === JSONRPC_METHOD_NOT_FOUND. packages/apps-engine/src/server/runtime/EmptyRuntime.ts currently throws a plain Error in sendRequest (no .code), so those checks will not match — set error.code = JSONRPC_METHOD_NOT_FOUND (or throw a JSON-RPC-style error) in EmptyRuntime.sendRequest and audit any other IRuntimeController implementations.
packages/apps-engine/src/server/managers/AppRuntimeManager.ts (1)
39-47: Force-start leaks previous runtimeWith { force: true }, the existing runtime is overwritten without being stopped, leaking a subprocess and event handlers.
- if (appId in this.subprocesses && !options.force) { - throw new Error('App already has an associated runtime'); - } - - this.subprocesses[appId] = this.runtimeFactory(this.manager, appPackage, storageItem); + if (appId in this.subprocesses) { + if (!options.force) { + throw new Error('App already has an associated runtime'); + } + await this.stopRuntime(this.subprocesses[appId]).catch(() => {}); + } + this.subprocesses[appId] = this.runtimeFactory(this.manager, appPackage, storageItem);
🧹 Nitpick comments (19)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
5-7: Make RuntimeRequestOptions extensibleAllow partial options so callers can override only what they need (e.g., timeout) without forcing all fields.
-export type RuntimeRequestOptions = { - timeout: number; -}; +export type RuntimeRequestOptions = { + timeout?: number; +};packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts (2)
3-3: Unify EventEmitter import with IRuntimeControllerImport EventEmitter from
events(same as the interface) to avoid subtle type mismatches and improve clarity.-import { type Readable, EventEmitter } from 'stream'; +import { type Readable } from 'stream'; +import { EventEmitter } from 'events';
20-21: Tighten sendRequest typing to match the interfaceUse the shared
RuntimeRequestOptionstype so defaults and call sites stay consistent.-import type { IRuntimeController } from '../IRuntimeController'; +import type { IRuntimeController, RuntimeRequestOptions } from '../IRuntimeController'; @@ - public async sendRequest(message: Pick<jsonrpc.RequestObject, 'method' | 'params'>, options = this.options): Promise<unknown> { + public async sendRequest( + message: { method: string; params: any[] }, + options: RuntimeRequestOptions = this.options, + ): Promise<unknown> {Also applies to: 317-335
packages/apps-engine/tests/test-data/utilities.ts (1)
513-538: EventEmitter stub is minimal; consider a real emitter if tests need eventsIf any test relies on emitted events (ready, processExit), back this mock with an actual EventEmitter for fidelity. Otherwise, current stub is fine.
Example approach (not required now):
- Extend Node’s EventEmitter and implement the IRuntimeController methods around it.
packages/apps-engine/src/server/managers/AppOutboundCommunicationProvider.ts (1)
35-46: Harden error propagation when wrapping unknown errors.If a non-Error is thrown, e?.message may be undefined. Wrap safely.
- throw new AppOutboundProcessError(e.message, method); + const msg = e instanceof Error ? e.message : String(e); + throw new AppOutboundProcessError(msg, method);packages/apps-engine/tests/server/managers/AppSlashCommand.spec.ts (1)
14-15: Avoid {} as AppManager unless guaranteed unused.Passing an empty manager into TestData.getMockApp is brittle if ProxiedApp starts using it in future. Consider providing a minimal stub (bridges/accessors) or a helper TestData.getMockManager().
Confirm no ProxiedApp method invoked in this spec touches manager.
packages/apps-engine/tests/server/managers/AppApiManager.spec.ts (1)
41-41: Initialization order bug: getMockApp called before mockManager is created.this.mockManager is undefined when passed to getMockApp, so the ProxiedApp is constructed with an undefined manager. Reorder to construct the manager first (capturing a mutable app ref), then create the app.
@@ public setupFixture() { this.mockBridges = new TestsAppBridges(); - - this.mockApp = TestData.getMockApp({ id: 'testing', name: 'TestApp' }, this.mockManager); - - const bri = this.mockBridges; - const app = this.mockApp; + const bri = this.mockBridges; + let app: ProxiedApp; this.mockManager = { @@ getOneById(appId: string): ProxiedApp { return appId === 'failMePlease' ? undefined : app; }, @@ } as AppManager; + + this.mockApp = TestData.getMockApp({ id: 'testing', name: 'TestApp' }, this.mockManager); + app = this.mockApp;packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts (2)
42-42: Same init-order issue as above: manager must exist before constructing mock app.Create the manager first (with a mutable app ref), then instantiate the app.
@@ public setupFixture() { this.mockBridges = new TestsAppBridges(); - - this.mockApp = TestData.getMockApp({ id: 'testing', name: 'TestApp' }, this.mockManager); - - const bri = this.mockBridges; - const app = this.mockApp; + const bri = this.mockBridges; + let app: ProxiedApp; this.mockManager = { @@ getOneById(appId: string): ProxiedApp { return appId === 'failMePlease' ? undefined : app; }, @@ } as AppManager; + + this.mockApp = TestData.getMockApp({ id: 'testing', name: 'TestApp' }, this.mockManager); + app = this.mockApp;
458-459: Remove leftover debug logging of a synthetic stack.This logs a new Error().stack (not the thrown error) and adds noise. Either assert the throw or log the actual error.
- await ascm.executePreview('nope', previewItem, context).catch(() => console.log(new Error().stack)); - + // Swallow potential error to continue flow (already covered by not.toThrowAsync above) + await ascm.executePreview('nope', previewItem, context).catch(() => undefined);packages/apps-engine/src/server/ProxiedApp.ts (1)
62-83: Fix JSON-RPC error-range checkThe OR makes the condition true for almost any number. Use AND and correct range (-32099..-32000).
- if (e.code >= -32999 || e.code <= -32000) { + if (typeof e.code === 'number' && e.code >= -32099 && e.code <= -32000) { // we really need to receive a logger from rocket.chat console.error('JSON-RPC error received: ', e); }packages/apps-engine/src/server/runtime/EmptyRuntime.ts (2)
24-26: Throw JSON-RPC–shaped errorProvide a code to integrate with existing handlers and logging.
- public async sendRequest(message: { method: string; params: any[] }, options?: RuntimeRequestOptions): Promise<unknown> { - throw new Error(`EmptyRuntime cannot handle requests. Method: ${message.method}`); - } + public async sendRequest(message: { method: string; params: any[] }, options?: RuntimeRequestOptions): Promise<unknown> { + const err: any = new Error(`EmptyRuntime cannot handle requests. Method: ${message.method}`); + // JSON-RPC server error range; "runtime unavailable" + err.code = -32000; + err.data = { appId: this.appId, method: message.method }; + throw err; + }
39-42: Emit processExit on stop for cache coherenceKeeps ProxiedApp.getStatus cache consistent across all controllers.
public async stopApp(): Promise<void> { - // Nothing to stop in an empty runtime - return Promise.resolve(); + // Nothing to stop in an empty runtime; still notify listeners + this.emit('processExit'); + return Promise.resolve(); }packages/apps-engine/src/server/AppManager.ts (1)
280-283: Persist compiler-error statusWhen compilation fails, persist COMPILER_ERROR_DISABLED so DB reflects the real state.
- const prl = new ProxiedApp(this, item, new EmptyRuntime(item.id)); + item.status = AppStatus.COMPILER_ERROR_DISABLED; + try { + await this.appMetadataStorage.update(item); + } catch { + // ignore persistence errors here; runtime stays disabled anyway + } + const prl = new ProxiedApp(this, item, new EmptyRuntime(item.id));packages/apps-engine/src/server/managers/AppRuntimeManager.ts (3)
45-55: Auto-prune on processExit (optional but useful)Registering a listener keeps the registry accurate if the subprocess exits independently.
try { - await this.subprocesses[appId].setupApp(); + const controller = this.subprocesses[appId]; + controller.on('processExit', () => { + if (this.subprocesses[appId] === controller) { + delete this.subprocesses[appId]; + } + }); + await controller.setupApp();
57-65: Pass options through to the runtimeCurrently ignores timeout. Map ExecRequestOptions to RuntimeRequestOptions.
- return subprocess.sendRequest(execRequest); + return subprocess.sendRequest(execRequest, options?.timeout ? { timeout: options.timeout } : undefined);
67-75: Ensure map cleanup even if stop failsUse try/finally; compute appId up-front.
- public async stopRuntime(controller: IRuntimeController): Promise<void> { - await controller.stopApp(); - - const appId = controller.getAppId(); - if (appId in this.subprocesses) { - delete this.subprocesses[appId]; - } - } + public async stopRuntime(controller: IRuntimeController): Promise<void> { + const appId = controller.getAppId(); + try { + await controller.stopApp(); + } finally { + if (appId in this.subprocesses) { + delete this.subprocesses[appId]; + } + } + }packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts (3)
133-143: Add assertions for force semanticsValidate that the previous runtime is stopped when force=true and that the registry contains the new instance.
I can add a test that spies on stopRuntime/stopApp and asserts it’s called before replacing the entry.
145-153: Also assert subprocess stop on setup failureThe manager stops the runtime on setup error; assert stopApp() is invoked.
I can extend this test with an Expect on stopApp being called once after setupApp rejection.
115-122: Add stopRuntime coverageAdd a test that starts a runtime, calls stopRuntime, and asserts the entry is removed from subprocesses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
packages/apps-engine/src/server/AppManager.ts(7 hunks)packages/apps-engine/src/server/ProxiedApp.ts(3 hunks)packages/apps-engine/src/server/managers/AppApi.ts(1 hunks)packages/apps-engine/src/server/managers/AppOutboundCommunicationProvider.ts(1 hunks)packages/apps-engine/src/server/managers/AppRuntimeManager.ts(4 hunks)packages/apps-engine/src/server/managers/AppSchedulerManager.ts(1 hunks)packages/apps-engine/src/server/managers/AppSlashCommand.ts(1 hunks)packages/apps-engine/src/server/managers/AppVideoConfProvider.ts(1 hunks)packages/apps-engine/src/server/runtime/EmptyRuntime.ts(1 hunks)packages/apps-engine/src/server/runtime/IRuntimeController.ts(1 hunks)packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts(2 hunks)packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts(2 hunks)packages/apps-engine/tests/server/managers/AppApiManager.spec.ts(1 hunks)packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts(3 hunks)packages/apps-engine/tests/server/managers/AppSlashCommand.spec.ts(1 hunks)packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts(2 hunks)packages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.ts(1 hunks)packages/apps-engine/tests/test-data/utilities.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
packages/apps-engine/tests/server/managers/AppSlashCommand.spec.tspackages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.tspackages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.tspackages/apps-engine/tests/server/managers/AppApiManager.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
packages/apps-engine/tests/server/managers/AppSlashCommand.spec.tspackages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.tspackages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.tspackages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.tspackages/apps-engine/tests/server/managers/AppApiManager.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
packages/apps-engine/tests/server/managers/AppSlashCommand.spec.tspackages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.tspackages/apps-engine/tests/server/managers/AppApiManager.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.tspackages/apps-engine/tests/server/managers/AppApiManager.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
packages/apps-engine/tests/server/managers/AppApiManager.spec.ts
🧬 Code graph analysis (11)
packages/apps-engine/tests/server/managers/AppSlashCommand.spec.ts (2)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)packages/apps-engine/tests/test-data/utilities.ts (1)
TestData(178-547)
packages/apps-engine/src/server/runtime/EmptyRuntime.ts (1)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (2)
IRuntimeController(9-34)RuntimeRequestOptions(5-7)
packages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.ts (1)
packages/apps-engine/tests/test-data/utilities.ts (1)
TestData(178-547)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (1)
packages/apps-engine/tests/test-data/utilities.ts (1)
TestData(178-547)
packages/apps-engine/src/server/managers/AppRuntimeManager.ts (1)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
IRuntimeController(9-34)
packages/apps-engine/src/server/AppManager.ts (2)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)packages/apps-engine/src/server/runtime/EmptyRuntime.ts (1)
EmptyRuntime(6-50)
packages/apps-engine/tests/test-data/utilities.ts (2)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
IRuntimeController(9-34)packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)
packages/apps-engine/src/server/ProxiedApp.ts (1)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
IRuntimeController(9-34)
packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts (1)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
IRuntimeController(9-34)
packages/apps-engine/tests/server/managers/AppApiManager.spec.ts (1)
packages/apps-engine/tests/test-data/utilities.ts (1)
TestData(178-547)
packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts (1)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
IRuntimeController(9-34)
🔇 Additional comments (12)
packages/apps-engine/src/server/runtime/IRuntimeController.ts (1)
9-34: LGTM: clear, minimal runtime surfaceInterface shape and docs look good and align with usage in ProxiedApp.
If any implementer imports a different EventEmitter type than this interface (events vs stream), TS may lose strictness. Consider standardizing on
eventsacross implementers.packages/apps-engine/tests/test-data/utilities.ts (3)
70-77: Good: startRuntimeForApp now returns an IRuntimeController mockAligns tests with the new runtime abstraction.
97-99: Good: getOneById returns a ProxiedApp built via TestData.getMockAppKeeps tests consistent and decoupled from Deno specifics.
540-545: LGTM: getMockApp constructs ProxiedApp with mock runtime (call sites verified)
rg shows all usages pass the new signature ({ id, name }, manager); Deno coupling removed.packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (1)
10-11: LGTM: switch to TestData.getMockRuntimeControllerTest no longer depends on Deno types and matches the new interface.
Also applies to: 44-48
packages/apps-engine/src/server/managers/AppSlashCommand.ts (1)
68-71: Runtime abstraction migration looks good; confirm request/response compatibility.The switch to getRuntimeController().sendRequest is correct and consistent with the new abstraction.
Please confirm all IRuntimeController implementations preserve the same request shape and return types for:
- slashcommand:${command}:${method} → void | ISlashCommandPreview
packages/apps-engine/src/server/managers/AppApi.ts (1)
68-71: Good migration to getRuntimeController.Call structure and params match the prior behavior.
packages/apps-engine/src/server/managers/AppSchedulerManager.ts (1)
61-64: Verify scheduler id used to call the runtime (processor.id vs. created processorId).registerProcessors registers processors under createProcessorId(processor.id, appId) (host-level id) but wrapProcessor calls the runtime with method
scheduler:${processor.id}(the original id). Only occurrence found: packages/apps-engine/src/server/managers/AppSchedulerManager.ts:61-64. Confirm the runtime expects the original id; if not, change the sendRequest call to use the created processorId.packages/apps-engine/src/server/ProxiedApp.ts (3)
12-13: Decouple from Deno-specific constantAvoid importing JSONRPC constants from the Deno runtime to keep ProxiedApp runtime-agnostic. Inline the standard JSON-RPC value or move constants to a shared module.
[ suggest_recommended_refactor ]
Apply:
-import type { IRuntimeController } from './runtime/IRuntimeController'; -import { JSONRPC_METHOD_NOT_FOUND } from './runtime/deno/AppsEngineDenoRuntime'; +import type { IRuntimeController } from './runtime/IRuntimeController'; +// JSON-RPC "Method not found" +const JSONRPC_METHOD_NOT_FOUND_CODE = -32601;And:
- if (e.code === JSONRPC_METHOD_NOT_FOUND) { + if (e.code === JSONRPC_METHOD_NOT_FOUND_CODE) { throw e; }
35-37: Getter rename looks goodAccessor now returns the generic IRuntimeController, aligning with the new abstraction.
28-29: Ensure processExit is emitted by all controllersProxiedApp clears the status cache on 'processExit'. EmptyRuntime.stopApp currently doesn’t emit it; propose emitting there (see EmptyRuntime comment).
packages/apps-engine/src/server/AppManager.ts (1)
599-601: Approve — verify no remaining direct stopApp()/stop callsProvided rg produced no output; re-run locally to confirm no direct getRuntimeController().stopApp()/stop usages remain (e.g. rg -nP 'getRuntimeController()).(stopApp|stop)' -C1).
2 participants
Proposed changes (including videos or screenshots)
Refactor
ProxiedAppinstantiation when an app failes to compile by creating an EmptyRuntime class instead of creating an ad-hoc object ment to mimic a runtime controllerIssue(s)
Steps to test or reproduce
Further comments
Refactor based on changes in #36802
Summary by CodeRabbit