Skip to content

Commit e1698b5

Browse files
clauded-gubert
authored andcommitted
refactor(apps): Phase 3 - registration + app-settings surface via AppResourceBridge
Moves the last accessor category off the accessor:* channel. After this phase the base-runtime emits zero accessor:* messages - every accessor call is a bridges:* message, and handleAccessorMessage is dead (removed in Phase 4). Host: - Add AppResourceBridge, a concrete engine-owned bridge (not part of the app-facing AppBridges surface) that delegates to the manager registries (slash commands, APIs, scheduler, UI action buttons, external components, video-conf and outbound providers) and to the app's ProxiedApp storage item + AppSettingsManager for app settings. - BaseRuntimeSubprocessController.handleBridgeMessage resolves the getAppResourceBridge name via a dedicated controller field and suppresses AppResourceBridge.REGISTRATION_METHODS while the subprocess is restarting (replacing the old getConfigurationExtend hijack in handleAccessorMessage, now keyed on an explicit method set). AppManager is handed to the bridge in the controller constructor; no apps/meteor changes required. Permission and conflict semantics are preserved by delegating to the same managers. Runtime: - getConfigurationExtend / getConfigurationModify and the app-settings SettingRead/SettingUpdater/SettingsExtend members now call getAppResourceBridge().do*, keeping the AppObjectRegistry stash-then-forward for slash commands, processors, api endpoints and providers. accessor:api: listApis is replaced by doListApis. registerButton stays a synchronous void. - Removed the now-dead proxify machinery and WithProxy type from mod.ts. RPC-boundary: SettingRead.getValueById treats null/undefined alike as "does not exist" (undefined serializes to null across the boundary). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
1 parent 34aef0b commit e1698b5

9 files changed

Lines changed: 474 additions & 96 deletions

File tree

docs/proposals/apps-accessor-consolidation/README.md

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -599,16 +599,34 @@ through `RemoteBridges` (`Promise<unknown>`) and are cast to their interface ret
599599
`void`-returning methods (`setActiveState`, `endActiveState`, reactions, deletes) `await` instead of
600600
returning the bridge value, matching the interface.
601601

602-
### Phase 3 — Registration surface via `AppResourceBridge`
603-
604-
1. Host: implement `AppResourceBridge` (§4) + internal-bridge lookup in `handleBridgeMessage` +
605-
`restarting` guard for registration methods. Unit-test the guard and throw-vs-silent permission
606-
behaviors.
607-
2. Runtime: rewrite `getConfigurationExtend` / `getConfigurationModify:slashCommands` /
608-
`SettingRead` / `SettingUpdater` / `SettingsExtend` members as local classes calling
609-
`RemoteBridges.getAppResourceBridge()`, preserving the `AppObjectRegistry` stash-then-forward
610-
wrappers; replace `accessor:api:listApis` with `doListApis`.
611-
3. Delete the host `*Extend`/`SlashCommandsModify`/`SettingRead`/`SettingUpdater` accessors; port tests.
602+
### Phase 3 — Registration surface via `AppResourceBridge` — ✅ landed
603+
604+
1. ✅ Host: added the concrete `AppResourceBridge` (`src/server/bridges/AppResourceBridge.ts`)
605+
delegating to the managers (`AppSlashCommandManager`, `AppApiManager`, `AppSchedulerManager`,
606+
`UIActionButtonManager`, `AppExternalComponentManager`, `AppVideoConfProviderManager`,
607+
`AppOutboundCommunicationProviderManager`) and the app's `ProxiedApp` storage item +
608+
`AppSettingsManager` for settings. Wired into `BaseRuntimeSubprocessController.handleBridgeMessage`
609+
via a dedicated `getAppResourceBridge` lookup (resolved from a controller field, not `AppBridges`)
610+
+ the `restarting` guard keyed on `AppResourceBridge.REGISTRATION_METHODS`. Permission/conflict
611+
semantics are unchanged because each method calls the same manager the host accessor used (the
612+
video-conf/outbound `PermissionDeniedError` throw and the UI log-and-refuse both propagate as
613+
before). The `AppManager` instance is passed to the bridge in the controller constructor — no
614+
`apps/meteor` orchestrator changes needed.
615+
2. ✅ Runtime: rewrote `getConfigurationExtend` (ui/settings/externalComponents/api/scheduler/
616+
videoConfProviders/outboundCommunication/slashCommands), `getConfigurationModify`
617+
(slashCommands → modify/enable/disable; scheduler → local `SchedulerModify`), and the app-settings
618+
`SettingRead`/`SettingUpdater`/`SettingsExtend` members to call `getAppResourceBridge().do*`,
619+
preserving the `AppObjectRegistry` stash-then-forward. `accessor:api:listApis` is replaced by
620+
`doListApis`. `registerButton` stays a synchronous `void` (fire-and-forget) per its interface. The
621+
now-dead `proxify` machinery and `WithProxy` type were removed from `mod.ts`**the runtime no
622+
longer emits any `accessor:*` message at all.**
623+
3. Host accessor deletion (`*Extend`/`SlashCommandsModify`/`SettingRead`/`SettingUpdater`) + the rest
624+
of the teardown are deferred to Phase 4, consistent with Phases 1–2. Tests:
625+
`accessors/tests/configuration.test.ts` + updated `AppAccessors.test.ts`; the host
626+
`DenoRuntimeSubprocessController` test validates the controller wiring.
627+
628+
**RPC-boundary note:** `SettingRead.getValueById` treats `null` and `undefined` alike as "does not
629+
exist" (undefined serializes to null across the boundary), same adaptation as `ServerSettingRead`.
612630

613631
### Phase 4 — Teardown
614632

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import type { ISettingRead } from '@rocket.chat/apps-engine/definition/accessors';
2+
import type { ISetting } from '@rocket.chat/apps-engine/definition/settings';
3+
4+
import type { RemoteBridges } from '../../bridges/RemoteBridges';
5+
6+
// App settings are host-persisted metadata (ProxiedApp storage item), fronted by the internal
7+
// AppResourceBridge. The value fallback that used to run host-side now runs locally.
8+
export class SettingRead implements ISettingRead {
9+
constructor(private readonly bridges: RemoteBridges) {}
10+
11+
public getById(id: string): Promise<ISetting> {
12+
return this.bridges.getAppResourceBridge().doGetSettingById(id, 'APP_ID') as Promise<ISetting>;
13+
}
14+
15+
public async getValueById(id: string): Promise<any> {
16+
const set = (await this.getById(id)) as ISetting;
17+
18+
// The host accessor checks `typeof set === 'undefined'`; across the RPC boundary an absent
19+
// host return arrives as null, so both are treated as "does not exist".
20+
if (set === undefined || set === null) {
21+
throw new Error(`Setting "${id}" does not exist.`);
22+
}
23+
24+
if (set.value === undefined || set.value === null) {
25+
return set.packageValue;
26+
}
27+
28+
return set.value;
29+
}
30+
}
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import type { ISettingUpdater } from '@rocket.chat/apps-engine/definition/accessors/ISettingUpdater';
2+
import type { ISetting } from '@rocket.chat/apps-engine/definition/settings';
3+
4+
import type { RemoteBridges } from '../../bridges/RemoteBridges';
5+
6+
// The "not found" guard and the AppSettingsManager persistence run host-side in the
7+
// AppResourceBridge (they depend on the ProxiedApp storage item and the settings manager); the
8+
// runtime accessor is a thin forwarder.
9+
export class SettingUpdater implements ISettingUpdater {
10+
constructor(private readonly bridges: RemoteBridges) {}
11+
12+
public async updateValue(id: ISetting['id'], value: ISetting['value']): Promise<void> {
13+
await this.bridges.getAppResourceBridge().doUpdateSettingValue(id, value, 'APP_ID');
14+
}
15+
16+
public async updateSelectOptions(id: ISetting['id'], values: ISetting['values']): Promise<void> {
17+
await this.bridges.getAppResourceBridge().doUpdateSettingSelectOptions(id, values, 'APP_ID');
18+
}
19+
}

packages/apps/base-runtime/src/lib/accessors/mod.ts

Lines changed: 57 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,18 @@ import type { IConfigurationExtend } from '@rocket.chat/apps-engine/definition/a
44
import type { IConfigurationModify } from '@rocket.chat/apps-engine/definition/accessors/IConfigurationModify';
55
import type { IEnvironmentRead } from '@rocket.chat/apps-engine/definition/accessors/IEnvironmentRead';
66
import type { IEnvironmentWrite } from '@rocket.chat/apps-engine/definition/accessors/IEnvironmentWrite';
7+
import type { IExternalComponentsExtend } from '@rocket.chat/apps-engine/definition/accessors/IExternalComponentsExtend';
78
import type { IHttp, IHttpExtend } from '@rocket.chat/apps-engine/definition/accessors/IHttp';
89
import type { IModify } from '@rocket.chat/apps-engine/definition/accessors/IModify';
910
import type { INotifier } from '@rocket.chat/apps-engine/definition/accessors/INotifier';
1011
import type { IOutboundCommunicationProviderExtend } from '@rocket.chat/apps-engine/definition/accessors/IOutboundCommunicationProviderExtend';
1112
import type { IPersistence } from '@rocket.chat/apps-engine/definition/accessors/IPersistence';
1213
import type { IRead } from '@rocket.chat/apps-engine/definition/accessors/IRead';
1314
import type { ISchedulerExtend } from '@rocket.chat/apps-engine/definition/accessors/ISchedulerExtend';
15+
import type { ISettingsExtend } from '@rocket.chat/apps-engine/definition/accessors/ISettingsExtend';
1416
import type { ISlashCommandsExtend } from '@rocket.chat/apps-engine/definition/accessors/ISlashCommandsExtend';
1517
import type { ISlashCommandsModify } from '@rocket.chat/apps-engine/definition/accessors/ISlashCommandsModify';
18+
import type { IUIExtend } from '@rocket.chat/apps-engine/definition/accessors/IUIExtend';
1619
import type { IVideoConfProvidersExtend } from '@rocket.chat/apps-engine/definition/accessors/IVideoConfProvidersExtend';
1720
import type { IApi } from '@rocket.chat/apps-engine/definition/api/IApi';
1821
import type { IApiEndpointMetadata } from '@rocket.chat/apps-engine/definition/api/IApiEndpointMetadata';
@@ -26,7 +29,6 @@ import type { IVideoConfProvider } from '@rocket.chat/apps-engine/definition/vid
2629

2730
import { Persistence } from './Persistence';
2831
import { HttpExtend } from './extenders/HttpExtender';
29-
import { formatErrorResponse } from './formatResponseErrorHandler';
3032
import { Http } from './http';
3133
import { AppObjectRegistry } from '../../AppObjectRegistry';
3234
import { RemoteBridges } from '../bridges/RemoteBridges';
@@ -37,6 +39,8 @@ import { EnvironmentalVariableRead } from './environment/EnvironmentalVariableRe
3739
import { ServerSettingRead } from './environment/ServerSettingRead';
3840
import { ServerSettingUpdater } from './environment/ServerSettingUpdater';
3941
import { ServerSettingsModify } from './environment/ServerSettingsModify';
42+
import { SettingRead } from './environment/SettingRead';
43+
import { SettingUpdater } from './environment/SettingUpdater';
4044
import { ModerationModify } from './modify/ModerationModify';
4145
import { ModifyCreator } from './modify/ModifyCreator';
4246
import { ModifyDeleter } from './modify/ModifyDeleter';
@@ -61,9 +65,6 @@ import { UploadRead } from './read/UploadRead';
6165
import { UserRead } from './read/UserRead';
6266
import { VideoConferenceRead } from './read/VideoConferenceRead';
6367

64-
/** Helper: extends T with an internal _proxy property used for delegation. */
65-
type WithProxy<T> = T & { _proxy: T };
66-
6768
const httpMethods = ['get', 'post', 'put', 'delete', 'head', 'options', 'patch'] as const;
6869

6970
// We need to create this object first thing, as we'll handle references to it later on
@@ -102,40 +103,9 @@ export class AppAccessors {
102103

103104
private readonly bridges: RemoteBridges;
104105

105-
private proxify: <T>(namespace: string, overrides?: Record<string, (...args: unknown[]) => unknown>) => T;
106-
107106
constructor(private readonly senderFn: typeof Messenger.sendRequest) {
108107
this.bridges = new RemoteBridges(senderFn);
109108

110-
this.proxify = <T>(namespace: string, overrides: Record<string, (...args: unknown[]) => unknown> = {}): T =>
111-
new Proxy(
112-
{ __kind: `accessor:${namespace}` },
113-
{
114-
get:
115-
(_target: unknown, prop: string) =>
116-
(...params: unknown[]) => {
117-
// We don't want to send a request for this prop
118-
if (prop === 'toJSON') {
119-
return {};
120-
}
121-
122-
// If the prop is inteded to be overriden by the caller
123-
if (prop in overrides) {
124-
return overrides[prop].apply(undefined, params);
125-
}
126-
127-
return senderFn({
128-
method: `accessor:${namespace}:${prop}`,
129-
params,
130-
})
131-
.then((response) => response.result)
132-
.catch((err) => {
133-
throw formatErrorResponse(err);
134-
});
135-
},
136-
},
137-
) as T;
138-
139109
this.http = new Http(this.getReader(), this.getPersistence(), this.httpExtend, this.getSenderFn());
140110
this.notifier = new Notifier(this.getSenderFn());
141111
}
@@ -146,11 +116,10 @@ export class AppAccessors {
146116

147117
public getEnvironmentRead(): IEnvironmentRead {
148118
if (!this.environmentRead) {
149-
// App settings (`getSettings`) remain proxied to the host until Phase 3 (they are
150-
// backed by the host ProxiedApp storage item); server settings and environment
151-
// variables now run locally against their bridges.
119+
// App settings, server settings and environment variables all run locally now; app
120+
// settings reach the host ProxiedApp storage item through the internal AppResourceBridge.
152121
this.environmentRead = new EnvironmentRead(
153-
this.proxify('getEnvironmentRead:getSettings'),
122+
new SettingRead(this.bridges),
154123
new ServerSettingRead(this.bridges),
155124
new EnvironmentalVariableRead(this.bridges),
156125
);
@@ -161,36 +130,33 @@ export class AppAccessors {
161130

162131
public getEnvironmentWrite() {
163132
if (!this.environmentWriter) {
164-
// App-settings updates (`getSettings`) remain proxied to the host until Phase 3.
165-
this.environmentWriter = new EnvironmentWrite(
166-
this.proxify('getEnvironmentWrite:getSettings'),
167-
new ServerSettingUpdater(this.bridges),
168-
);
133+
this.environmentWriter = new EnvironmentWrite(new SettingUpdater(this.bridges), new ServerSettingUpdater(this.bridges));
169134
}
170135

171136
return this.environmentWriter;
172137
}
173138

174139
public getConfigurationModify() {
175140
if (!this.configModifier) {
176-
const slashCommandsModify: WithProxy<ISlashCommandsModify> = {
177-
_proxy: this.proxify('getConfigurationModify:slashCommands'),
141+
const resourceBridge = this.bridges.getAppResourceBridge();
142+
143+
const slashCommandsModify: ISlashCommandsModify = {
178144
modifySlashCommand(slashcommand: ISlashCommand) {
179145
// Store the slashcommand instance to use when the Apps-Engine calls the slashcommand
180146
AppObjectRegistry.set(`slashcommand:${slashcommand.command}`, slashcommand);
181147

182-
return this._proxy.modifySlashCommand(slashcommand);
148+
return resourceBridge.doModifySlashCommand(slashcommand, 'APP_ID') as Promise<void>;
183149
},
184150
disableSlashCommand(command: string) {
185-
return this._proxy.disableSlashCommand(command);
151+
return resourceBridge.doDisableSlashCommand(command, 'APP_ID') as Promise<void>;
186152
},
187153
enableSlashCommand(command: string) {
188-
return this._proxy.enableSlashCommand(command);
154+
return resourceBridge.doEnableSlashCommand(command, 'APP_ID') as Promise<void>;
189155
},
190156
};
191157

192158
this.configModifier = {
193-
scheduler: this.proxify('getConfigurationModify:scheduler'),
159+
scheduler: new SchedulerModify(this.bridges),
194160
slashCommands: slashCommandsModify,
195161
serverSettings: new ServerSettingsModify(this.bridges),
196162
};
@@ -201,10 +167,9 @@ export class AppAccessors {
201167

202168
public getConfigurationExtend() {
203169
if (!this.configExtender) {
204-
const { senderFn } = this;
170+
const resourceBridge = this.bridges.getAppResourceBridge();
205171

206-
const apiExtend: WithProxy<IApiExtend> = {
207-
_proxy: this.proxify('getConfigurationExtend:api'),
172+
const apiExtend: IApiExtend = {
208173
async provideApi(api: IApi) {
209174
const apiEndpoints = AppObjectRegistry.get<IApiEndpointMetadata[]>('apiEndpoints')!;
210175

@@ -215,67 +180,83 @@ export class AppAccessors {
215180
AppObjectRegistry.set(`api:${endpoint.path}`, endpoint);
216181
});
217182

218-
const result = await this._proxy.provideApi(api);
183+
await resourceBridge.doProvideApi(api, 'APP_ID');
219184

220185
// Let's call the listApis method to cache the info from the endpoints
221186
// Also, since this is a side-effect, we do it async so we can return to the caller
222-
senderFn({ method: 'accessor:api:listApis' })
223-
.then((response) => apiEndpoints.push(...(response.result as IApiEndpointMetadata[])))
224-
.catch((err) => err.error);
225-
226-
return result;
187+
resourceBridge
188+
.doListApis('APP_ID')
189+
.then((endpoints) => apiEndpoints.push(...(endpoints as IApiEndpointMetadata[])))
190+
.catch(() => undefined);
227191
},
228192
};
229193

230-
const schedulerExtend: WithProxy<ISchedulerExtend> = {
231-
_proxy: this.proxify('getConfigurationExtend:scheduler'),
194+
const schedulerExtend: ISchedulerExtend = {
232195
registerProcessors(processors: IProcessor[]) {
233196
// Store the processor instance to use when the Apps-Engine calls the processor
234197
processors.forEach((processor) => {
235198
AppObjectRegistry.set(`scheduler:${processor.id}`, processor);
236199
});
237200

238-
return this._proxy.registerProcessors(processors);
201+
return resourceBridge.doRegisterProcessors(processors, 'APP_ID') as Promise<void | Array<string>>;
239202
},
240203
};
241204

242-
const videoConfProviders: WithProxy<IVideoConfProvidersExtend> = {
243-
_proxy: this.proxify('getConfigurationExtend:videoConfProviders'),
205+
const videoConfProviders: IVideoConfProvidersExtend = {
244206
provideVideoConfProvider(provider: IVideoConfProvider) {
245207
// Store the videoConfProvider instance to use when the Apps-Engine calls the videoConfProvider
246208
AppObjectRegistry.set(`videoConfProvider:${provider.name}`, provider);
247209

248-
return this._proxy.provideVideoConfProvider(provider);
210+
return resourceBridge.doProvideVideoConfProvider(provider, 'APP_ID') as Promise<void>;
249211
},
250212
};
251213

252-
const outboundCommunication: WithProxy<IOutboundCommunicationProviderExtend> = {
253-
_proxy: this.proxify('getConfigurationExtend:outboundCommunication'),
214+
const outboundCommunication: IOutboundCommunicationProviderExtend = {
254215
registerEmailProvider(provider: IOutboundEmailMessageProvider) {
255216
AppObjectRegistry.set(`outboundCommunication:${provider.name}-${provider.type}`, provider);
256-
return this._proxy.registerEmailProvider(provider);
217+
return resourceBridge.doRegisterOutboundProvider(provider, 'APP_ID') as Promise<void>;
257218
},
258219
registerPhoneProvider(provider: IOutboundPhoneMessageProvider) {
259220
AppObjectRegistry.set(`outboundCommunication:${provider.name}-${provider.type}`, provider);
260-
return this._proxy.registerPhoneProvider(provider);
221+
return resourceBridge.doRegisterOutboundProvider(provider, 'APP_ID') as Promise<void>;
261222
},
262223
};
263224

264-
const slashCommandsExtend: WithProxy<ISlashCommandsExtend> = {
265-
_proxy: this.proxify('getConfigurationExtend:slashCommands'),
225+
const slashCommandsExtend: ISlashCommandsExtend = {
266226
provideSlashCommand(slashcommand: ISlashCommand) {
267227
// Store the slashcommand instance to use when the Apps-Engine calls the slashcommand
268228
AppObjectRegistry.set(`slashcommand:${slashcommand.command}`, slashcommand);
269229

270-
return this._proxy.provideSlashCommand(slashcommand);
230+
return resourceBridge.doProvideSlashCommand(slashcommand, 'APP_ID') as Promise<void>;
231+
},
232+
};
233+
234+
const ui: IUIExtend = {
235+
// `registerButton` is a synchronous `void` in the interface, but the host registration
236+
// is async over the bridge; fire-and-forget matches the contract. The host manager
237+
// logs-and-refuses rather than throwing, so there is no rejection to surface here.
238+
registerButton(button) {
239+
void resourceBridge.doRegisterActionButton(button, 'APP_ID').catch(() => undefined);
240+
},
241+
};
242+
243+
const settings: ISettingsExtend = {
244+
provideSetting(setting) {
245+
return resourceBridge.doProvideSetting(setting, 'APP_ID') as Promise<void>;
246+
},
247+
};
248+
249+
const externalComponents: IExternalComponentsExtend = {
250+
register(externalComponent) {
251+
return resourceBridge.doRegisterExternalComponent(externalComponent, 'APP_ID') as Promise<void>;
271252
},
272253
};
273254

274255
this.configExtender = {
275-
ui: this.proxify('getConfigurationExtend:ui'),
256+
ui,
276257
http: this.httpExtend,
277-
settings: this.proxify('getConfigurationExtend:settings'),
278-
externalComponents: this.proxify('getConfigurationExtend:externalComponents'),
258+
settings,
259+
externalComponents,
279260
api: apiExtend,
280261
scheduler: schedulerExtend,
281262
videoConfProviders,
@@ -303,11 +284,8 @@ export class AppAccessors {
303284

304285
public getReader() {
305286
if (!this.reader) {
306-
// The environment sub-reader keeps its own `getSettings` proxy namespace
307-
// (`getReader:getEnvironmentReader:getSettings`) so the app-settings path stays
308-
// byte-for-byte until Phase 3; server settings and env vars run locally.
309287
const environmentReader = new EnvironmentRead(
310-
this.proxify('getReader:getEnvironmentReader:getSettings'),
288+
new SettingRead(this.bridges),
311289
new ServerSettingRead(this.bridges),
312290
new EnvironmentalVariableRead(this.bridges),
313291
);

0 commit comments

Comments
 (0)