chore(apps): consolidate accessor implementation to runtime (4/4) - #41171
Merged
ggazzo merged 8 commits intoJul 31, 2026
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
|
|
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:
WalkthroughThis change replaces ChangesAccessor consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppAccessors
participant bridgeCall
participant BaseRuntimeSubprocessController
participant AppResourceBridge
AppAccessors->>bridgeCall: send bridge operation and APP_ID
bridgeCall->>BaseRuntimeSubprocessController: send bridges:bridge:method request
BaseRuntimeSubprocessController->>AppResourceBridge: dispatch bridge operation
AppResourceBridge-->>BaseRuntimeSubprocessController: return operation result
BaseRuntimeSubprocessController-->>bridgeCall: return JSON-RPC result
bridgeCall-->>AppAccessors: resolve typed result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41171 +/- ##
===========================================
- Coverage 68.75% 68.75% -0.01%
===========================================
Files 4151 4155 +4
Lines 159513 159664 +151
Branches 27997 28054 +57
===========================================
+ Hits 109681 109781 +100
- Misses 44657 44705 +48
- Partials 5175 5178 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
changed the title
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
4 times, most recently
from
July 14, 2026 21:25
a069de9 to
5b77fa6
Compare
d-gubert
changed the base branch from
develop
to
chore/apps-accessor-consolidation-3
July 14, 2026 21:26
d-gubert
changed the title
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 15, 2026 18:00
99e5b50 to
c8c10fe
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 15, 2026 18:00
5b77fa6 to
bee0e93
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 15, 2026 18:50
c8c10fe to
1658bb9
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 15, 2026 18:50
bee0e93 to
0b188fd
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 15, 2026 19:08
1658bb9 to
8bcb150
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 15, 2026 19:08
0b188fd to
c48f953
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 16, 2026 19:05
8bcb150 to
2e441d7
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 16, 2026 19:05
c48f953 to
c042cf4
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 28, 2026 18:59
2e441d7 to
465c6a2
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 28, 2026 18:59
c042cf4 to
f529a27
Compare
ggazzo
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 29, 2026 18:35
465c6a2 to
f43eafd
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 29, 2026 20:16
f43eafd to
59d3fa4
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
2 times, most recently
from
July 29, 2026 20:36
4bc40e0 to
82cadfb
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-3
branch
from
July 29, 2026 23:22
59d3fa4 to
ac77da9
Compare
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 29, 2026 23:22
82cadfb to
3400b31
Compare
d-gubert
marked this pull request as ready for review
July 30, 2026 15:08
dionisio-bot
Bot
added
the
stat: ready to merge
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 30, 2026 17:10
259fd7e to
7d42ca4
Compare
dionisio-bot
Bot
added
stat: conflict
and removed
stat: ready to merge
Completes the primary objective: handleAccessorMessage and the entire accessor:* JSON-RPC category are gone. BaseRuntimeSubprocessController now dispatches only bridges:* (plus ready/log/error notifications), so there is a single app-originated RPC channel, a single permission model (the bridge do* wrappers), and accessor behavior lives in exactly one place - the base-runtime. - BaseRuntimeSubprocessController: delete handleAccessorMessage, ALLOWED_ACCESSOR_METHODS, isValidOrigin, getAccessorForOrigin, the accessor: branch in handleIncomingMessage, and the now-unused accessors/api fields (and their manager.getAccessorManager()/getApiManager() reads). The restart-time registration guard now lives in handleBridgeMessage via AppResourceBridge.REGISTRATION_METHODS (Phase 3). JSONRPC_METHOD_NOT_FOUND is kept - it is imported by ProxiedApp, AppListenerManager, AppVideoConfProvider. - AppListenerManager.executePostMessageSent: the one load-bearing host-side accessor consumer outside the RPC path now calls manager.getBridges().getUserBridge().doGetAppUser(appId) directly; the class no longer references AppAccessorManager. Physical deletion of the now-unreachable host accessor classes + AppAccessorManager is folded into follow-up #4 (documented), because it requires first un-threading the ignored accessors parameter from the sandbox-execution core (runTheCode) - a separate, behavior-neutral change the plan already carved out. The host accessors remain as dead code reachable by nothing on the subprocess path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
The base-runtime is the single source of truth for accessor behavior and the accessor:* message category is gone (Phase 4), so the host copies of the accessors are unreachable. This removes them and un-threads the vestigial accessor manager from the sandbox-execution core. - Drop the ignored `accessors: AppAccessorManager` parameter from the run*/ runTheCode methods of AppApi, AppSlashCommand, AppVideoConfProvider and AppOutboundCommunicationProvider, and the `accessors` field from their four managers (the app executes in the subprocess via sendRequest; the host accessor manager was never consulted). - Delete AppAccessorManager, its managers/index export, and AppManager's accessorManager field/constructor/getAccessorManager/purifyApp usage (purifyApp only cleared caches that were never populated). - Delete src/server/accessors/ (64 files) and the now-orphaned src/server/misc/UIHelper.ts. - Adapt tests: delete tests/server/accessors/ and the AppAccessorManager test; strip getAccessorManager/.accessors setup and assertions from the affected manager, AppManager and Deno-runtime tests. Behavior-neutral: host tsc error-set unchanged vs baseline; host node suites green (excluding the two deno-spawn integration suites, which require the deno binary and run in CI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
AppResourceBridge is stateless (every method takes the appId and delegates to an AppManager-owned manager), so there is no reason to couple its lifetime to each subprocess controller. Construct it once in AppManager and expose it via getAppResourceBridge(); every BaseRuntimeSubprocessController now consumes the shared instance instead of building its own. This matches the documented design (§4: "instantiated by AppManager") and removes per-controller allocation of an object that is identical for all apps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
The RemoteBridges facade built one Proxy per host bridge (25 getters) so ported
accessors could keep their host call shape (this.bridge.doX(...)). Its RemoteBridge
type was an index signature over unknown, so it provided no compile-time typing -
every call site cast `as Promise<T>` regardless - and the `do*` restriction was only
enforced at runtime by the proxy trap. With the host accessors now deleted, the
"keep the host shape" rationale is spent.
Replace it with a single free helper `bridgeCall<T>(senderFn, bridge, method,
...params)`:
- `bridge` is a `BridgeName` union and `method` is constrained to `do${string}`, so
the bridge-method gate is now checked at compile time instead of at runtime.
- It owns the wire encoding (`bridges:<bridge>:<method>`), result unwrap and
`formatErrorResponse` mapping - the one place that knows the encoding.
- Accessors take `senderFn` and call `bridgeCall(this.senderFn, 'getUserBridge',
'doGetById', id, 'APP_ID')`; the `<T>` generic replaces the old `as Promise<T>`
casts. Public accessor typing still comes from `implements I*`.
No behavior change: `'APP_ID'` is still passed explicitly per accessor (not injected),
params are unchanged, and every emitted `{ method, params }` is identical. Deletes
RemoteBridges.ts (+ its test); adds bridgeCall.ts (+ test). Net -234 lines.
base-runtime: 204/204 tests pass; host + base-runtime tsc clean; lint clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 30, 2026 17:33
7d42ca4 to
b454495
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)
packages/apps/base-runtime/src/lib/bridges/bridgeCall.ts (1)
4-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation comments or move this rationale to the proposal documentation.
packages/apps/base-runtime/src/lib/bridges/bridgeCall.ts#L4-L38: remove the bridge-description and sentinel-policy comments.packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts#L7-L8: remove the AppResourceBridge implementation comments.As per coding guidelines, “Avoid code comments in implementation.”
🤖 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 `@packages/apps/base-runtime/src/lib/bridges/bridgeCall.ts` around lines 4 - 38, Remove the implementation comments describing bridge reachability, bridge-call behavior, and the APP_ID sentinel policy in bridgeCall.ts. Also remove the AppResourceBridge implementation comments in SettingRead.ts; leave the surrounding code and behavior unchanged.Source: Coding guidelines
packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts (1)
15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove newly added implementation comments. Keep this rationale in the proposal or PR description rather than production code.
packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts#L15-L17: remove theappIdforwarding comment.packages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.ts#L31-L32: remove the thunk implementation comment.As per coding guidelines, “Avoid code comments in the implementation.”
🤖 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 `@packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts` around lines 15 - 17, Remove the newly added implementation comments: delete the appId forwarding rationale in ModifyDeleter.ts at lines 15-17 and the thunk implementation comment in ModifyUpdater.ts at lines 31-32, leaving the surrounding behavior unchanged.Source: Coding guidelines
🤖 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 `@packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts`:
- Around line 7-9: Remove the migration rationale comments from
packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts lines
7-9 and the slash-command storage comment from
packages/apps/base-runtime/src/lib/accessors/mod.ts line 141; make no functional
changes to the surrounding accessor implementations.
In `@packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts`:
- Around line 10-11: Update ContactRead.getById to return
Promise<ILivechatContact | null> and change its bridgeCall generic to
ILivechatContact | null, matching the IContactRead contract.
In `@packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts`:
- Around line 10-11: Update getThreadById in ThreadRead to normalize a null RPC
result to undefined before returning it, while preserving the existing
message-array result for present threads and the IThreadRead accessor contract.
---
Nitpick comments:
In `@packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts`:
- Around line 15-17: Remove the newly added implementation comments: delete the
appId forwarding rationale in ModifyDeleter.ts at lines 15-17 and the thunk
implementation comment in ModifyUpdater.ts at lines 31-32, leaving the
surrounding behavior unchanged.
In `@packages/apps/base-runtime/src/lib/bridges/bridgeCall.ts`:
- Around line 4-38: Remove the implementation comments describing bridge
reachability, bridge-call behavior, and the APP_ID sentinel policy in
bridgeCall.ts. Also remove the AppResourceBridge implementation comments in
SettingRead.ts; leave the surrounding code and behavior unchanged.
🪄 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 Plus
Run ID: b97942ba-4dc8-47fc-9e11-c7870ea0201f
📒 Files selected for processing (53)
docs/proposals/apps-accessor-consolidation/README.mdpackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/http.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/notifier.tspackages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/tests/helpers/parityHarness.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/base-runtime/src/lib/bridges/bridgeCall.tspackages/apps/base-runtime/src/lib/bridges/tests/RemoteBridges.test.tspackages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.tspackages/apps/base-runtime/src/lib/roomFactory.tspackages/apps/src/server/AppManager.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/tests/test-data/utilities.ts
💤 Files with no reviewable changes (2)
- packages/apps/base-runtime/src/lib/bridges/tests/RemoteBridges.test.ts
- packages/apps/base-runtime/src/lib/bridges/RemoteBridges.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/apps/tests/test-data/utilities.ts
- packages/apps/base-runtime/src/lib/accessors/tests/helpers/parityHarness.ts
- packages/apps/src/server/bridges/AppResourceBridge.ts
- docs/proposals/apps-accessor-consolidation/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (2)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/roomFactory.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/notifier.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/http.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/bridges/bridgeCall.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/src/server/AppManager.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/mod.ts
🧠 Learnings (5)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/roomFactory.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/notifier.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/http.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/bridges/bridgeCall.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/src/server/AppManager.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/mod.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/roomFactory.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/notifier.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/http.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/bridges/bridgeCall.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/src/server/AppManager.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/mod.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/roomFactory.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/notifier.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/http.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/bridges/bridgeCall.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/src/server/AppManager.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/mod.ts
📚 Learning: 2026-07-29T16:20:05.676Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 41377
File: packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts:15-20
Timestamp: 2026-07-29T16:20:05.676Z
Learning: During Rocket.Chat’s Apps accessor-consolidation ports, reviewers should verify the Apps accessor read behavior stays consistent with the corresponding existing host accessor for all inputs—including any documented/known edge cases. If a correctness fix is intentionally expected to diverge from the host implementation, it must not be mixed into this port; instead require a dedicated follow-up change/PR that isolates the behavioral difference and clearly documents the reason and expected outcome.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/AppManager.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🪛 ast-grep (0.45.0)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (44)
packages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.ts (2)
32-35: 🎯 Functional CorrectnessTest still uses
undefinedinstead ofnullfor the missing-setting case.This was already flagged in a prior review: an absent host return is serialized as
nullacross the RPC boundary, but this fixture only exercisesundefined.
4-4: LGTM!Also applies to: 12-44, 56-57, 71-72, 82-83
packages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.ts (1)
4-4: LGTM!Also applies to: 15-17, 30-61, 68-85, 88-92, 99-105, 110-125, 130-143, 154-158
packages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.ts (1)
5-6: LGTM!Also applies to: 16-21, 22-40, 42-47
packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts (1)
7-11: LGTM!Also applies to: 14-30, 32-49, 58-82
packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts (1)
6-17: LGTM!Also applies to: 27-27
packages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.ts (1)
4-9: LGTM!Also applies to: 10-12, 14-16, 18-19
packages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts (1)
12-31: LGTM!Also applies to: 46-50, 62-78, 96-100
packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts (1)
4-8: LGTM!Also applies to: 10-16, 18-24, 29-35, 37-38
packages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.ts (1)
19-22: LGTM!Also applies to: 24-49, 51-54, 63-99, 113-133, 143-144, 154-155, 165-171
packages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.ts (1)
7-10: LGTM!Also applies to: 12-87
packages/apps/src/server/AppManager.ts (1)
14-14: LGTM!Also applies to: 110-111, 166-168, 228-236, 1092-1110
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (1)
20-41: LGTM!Also applies to: 65-66, 86-98, 104-132, 396-449, 451-483
packages/apps/base-runtime/src/lib/bridges/tests/bridgeCall.test.ts (1)
1-107: LGTM!packages/apps/base-runtime/src/lib/accessors/Persistence.ts (1)
4-51: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.ts (1)
4-35: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.ts (1)
4-15: LGTM!packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.ts (1)
4-11: LGTM!packages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.ts (1)
4-15: LGTM!packages/apps/base-runtime/src/lib/accessors/read/RoleRead.ts (1)
4-15: LGTM!packages/apps/base-runtime/src/lib/accessors/read/UploadRead.ts (1)
4-21: LGTM!packages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.ts (1)
4-11: LGTM!packages/apps/base-runtime/src/lib/roomFactory.ts (1)
5-12: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.ts (1)
3-18: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.ts (1)
4-23: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts (1)
4-6: LGTM!Also applies to: 11-18
packages/apps/base-runtime/src/lib/accessors/http.ts (1)
6-6: LGTM!Also applies to: 20-24, 77-77
packages/apps/base-runtime/src/lib/accessors/mod.ts (1)
34-34: LGTM!Also applies to: 104-107, 118-120, 129-132, 144-158, 166-166, 179-180, 183-186, 196-196, 205-218, 225-226, 234-247, 283-304, 316-321, 330-330
packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts (1)
5-6: LGTM!Also applies to: 12-24
packages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.ts (1)
4-19: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/UIController.ts (1)
21-25: LGTM!Also applies to: 93-100, 119-126, 140-147
packages/apps/base-runtime/src/lib/accessors/notifier.ts (1)
8-8: LGTM!Also applies to: 26-26, 36-36, 47-50, 59-59
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts (1)
3-3: LGTM!Also applies to: 9-9
packages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.ts (1)
4-8: LGTM!Also applies to: 17-21
packages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.ts (1)
4-11: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts (1)
13-35: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.ts (1)
10-38: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.ts (1)
4-15: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.ts (1)
26-26: LGTM!Also applies to: 39-54, 148-148, 162-162, 181-181, 212-219, 245-254, 274-282
packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts (1)
5-14: LGTM!Also applies to: 18-23, 38-38
packages/apps/base-runtime/src/lib/accessors/modify/ModifyExtender.ts (1)
11-11: LGTM!Also applies to: 25-25, 34-42, 52-71
packages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.ts (1)
18-18: LGTM!Also applies to: 30-30, 33-35, 51-51, 61-61, 94-94, 124-124
packages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.ts (1)
7-15: LGTM!Also applies to: 26-26
packages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.ts (1)
5-39: LGTM!
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 `@packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts`:
- Around line 10-13: Update ThreadRead.getThreadById to return
Promise<Array<IMessage> | undefined>, normalize the bridgeCall result so both
null and undefined become undefined, and preserve message arrays—including empty
arrays—unchanged. Align the implementation with the existing IThreadRead
contract and corresponding host accessor behavior.
🪄 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 Plus
Run ID: 22248cfa-51c3-43f0-9422-14e567302bc1
📒 Files selected for processing (2)
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (3)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
📚 Learning: 2026-07-29T16:20:05.676Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 41377
File: packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts:15-20
Timestamp: 2026-07-29T16:20:05.676Z
Learning: During Rocket.Chat’s Apps accessor-consolidation ports, reviewers should verify the Apps accessor read behavior stays consistent with the corresponding existing host accessor for all inputs—including any documented/known edge cases. If a correctness fix is intentionally expected to diverge from the host implementation, it must not be mixed into this port; instead require a dedicated follow-up change/PR that isolates the behavioral difference and clearly documents the reason and expected outcome.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
🔇 Additional comments (1)
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts (1)
10-11: LGTM!
ggazzo
previously approved these changes
Jul 30, 2026
ggazzo
enabled auto-merge
July 30, 2026 19:07
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 30, 2026 19:27
ed05f84 to
def212e
Compare
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 `@docs/adr/0001-app-accessor-logic-in-base-runtime.md`:
- Around line 83-85: Update the deferred type findings in the ADR to remove the
resolved ContactRead.getById and ThreadRead.getThreadById entries, reflecting
their current null and undefined behavior. Retain only findings that remain
verified against the current implementations, including any unresolved
MessageRead optional bridge typing.
🪄 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 Plus
Run ID: e01a62ab-7f35-4dc0-b02f-3b02443ab6f7
📒 Files selected for processing (5)
docs/adr/0001-app-accessor-logic-in-base-runtime.mddocs/proposals/apps-accessor-consolidation/README.mddocs/proposals/apps-accessor-consolidation/base-runtime-app-id-exceptions.mdpackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
💤 Files with no reviewable changes (2)
- docs/proposals/apps-accessor-consolidation/base-runtime-app-id-exceptions.md
- docs/proposals/apps-accessor-consolidation/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
📚 Learning: 2026-07-29T16:20:05.676Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 41377
File: packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts:15-20
Timestamp: 2026-07-29T16:20:05.676Z
Learning: During Rocket.Chat’s Apps accessor-consolidation ports, reviewers should verify the Apps accessor read behavior stays consistent with the corresponding existing host accessor for all inputs—including any documented/known edge cases. If a correctness fix is intentionally expected to diverge from the host implementation, it must not be mixed into this port; instead require a dedicated follow-up change/PR that isolates the behavioral difference and clearly documents the reason and expected outcome.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
🪛 LanguageTool
docs/adr/0001-app-accessor-logic-in-base-runtime.md
[grammar] ~222-~222: Ensure spelling is correct
Context: ...dit rule when adding a do* call:** an appId populated from a method parameter rathe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (2)
packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts (1)
10-11: LGTM!packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts (1)
10-13: LGTM!
d-gubert
force-pushed
the
claude/base-runtime-accessor-consolidation-ecurei
branch
from
July 30, 2026 19:34
def212e to
45cb2be
Compare
ggazzo
approved these changes
Jul 31, 2026
d-gubert
pushed a commit
that referenced
this pull request
Aug 5, 2026
… a Runtime SDK
Documents every message variation exchanged between the runtime controller
(packages/apps/src/server/runtime) and the app subprocess
(packages/apps/base-runtime), in both directions, with payload shapes and
semantics:
- transport/framing (msgpack + extension codecs, stdin/stdout, stderr metrics)
- the JSON-RPC 2.0 dialect and its deviations (raw _zPING/_zPONG control
strings, the { value, logs } result envelope, error-code table)
- host->app: app:* lifecycle, check*/execute* listeners, UIKit, upload,
slashcommand/api/videoconference/outboundCommunication/scheduler
- app->host: bridges:* (canonical) and accessor:* (legacy, removed by #41171),
plus ready/log/unhandledRejection/uncaughtException notifications
Proposes a Runtime SDK that declares each method as an explicit typed contract
(replacing positional param guessing), owns the JSON-RPC formatting without
jsonrpc-lite, and can emit a machine-readable reference. Recommends Zod v4
(already in the monorepo, native z.toJSONSchema) for declarations, with AJV as
an optional hot-path validator and ts-morph to generate the bridges:* surface
from the bridge classes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gHoMZr26gfqzfjVYN2AH5
d-gubert
pushed a commit
that referenced
this pull request
Aug 7, 2026
… a Runtime SDK
Documents every message variation exchanged between the runtime controller
(packages/apps/src/server/runtime) and the app subprocess
(packages/apps/base-runtime), in both directions, with payload shapes and
semantics:
- transport/framing (msgpack + extension codecs, stdin/stdout, stderr metrics)
- the JSON-RPC 2.0 dialect and its deviations (raw _zPING/_zPONG control
strings, the { value, logs } result envelope, error-code table)
- host->app: app:* lifecycle, check*/execute* listeners, UIKit, upload,
slashcommand/api/videoconference/outboundCommunication/scheduler
- app->host: bridges:* (canonical) and accessor:* (legacy, removed by #41171),
plus ready/log/unhandledRejection/uncaughtException notifications
Proposes a Runtime SDK that declares each method as an explicit typed contract
(replacing positional param guessing), owns the JSON-RPC formatting without
jsonrpc-lite, and can emit a machine-readable reference. Recommends Zod v4
(already in the monorepo, native z.toJSONSchema) for declarations, with AJV as
an optional hot-path validator and ts-morph to generate the bridges:* surface
from the bridge classes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gHoMZr26gfqzfjVYN2AH5
d-gubert
pushed a commit
that referenced
this pull request
Aug 10, 2026
… a Runtime SDK
Documents every message variation exchanged between the runtime controller
(packages/apps/src/server/runtime) and the app subprocess
(packages/apps/base-runtime), in both directions, with payload shapes and
semantics:
- transport/framing (msgpack + extension codecs, stdin/stdout, stderr metrics)
- the JSON-RPC 2.0 dialect and its deviations (raw _zPING/_zPONG control
strings, the { value, logs } result envelope, error-code table)
- host->app: app:* lifecycle, check*/execute* listeners, UIKit, upload,
slashcommand/api/videoconference/outboundCommunication/scheduler
- app->host: bridges:* (canonical) and accessor:* (legacy, removed by #41171),
plus ready/log/unhandledRejection/uncaughtException notifications
Proposes a Runtime SDK that declares each method as an explicit typed contract
(replacing positional param guessing), owns the JSON-RPC formatting without
jsonrpc-lite, and can emit a machine-readable reference. Recommends Zod v4
(already in the monorepo, native z.toJSONSchema) for declarations, with AJV as
an optional hot-path validator and ts-morph to generate the bridges:* surface
from the bridge classes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gHoMZr26gfqzfjVYN2AH5
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
4 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.
Proposed changes
This PR adds a comprehensive design document that outlines a phased plan to consolidate accessor logic from
packages/apps/src/server/accessors/into the base-runtime, eliminating the need for theaccessor:*JSON-RPC message category and making the base-runtime the single source of truth for accessor behavior.Key points from the plan:
Current state: Accessor calls from apps take three paths:
accessor:*messages (forwarded to host)Target state: Consolidate all accessor logic into the base-runtime, with side effects flowing through the existing
bridges:*RPC channel.Scope: The document analyzes all ~30 accessor classes and classifies them as:
AppResourceBridgefor registration/settings operationsKey design decisions:
RemoteBridgesfacade for typed bridge access in the runtimeAppResourceBridgeto handle registration operations (slash commands, APIs, settings, etc.) through the existingbridges:*channel'APP_ID'placeholder for anti-impersonationUIHelperinto base-runtime to resolve module-resolution constraintsPhased rollout:
End result: Single RPC category (
bridges:*) with unified dispatcher, permission model, and accessor implementation in base-runtime.Issue(s)
Related Jira task: ARCH-2185: reunite duplicated accessor implementations into a shared location
The design doc in this PR is the specification for that task.
Steps to test or reproduce
N/A — This is a design document, not executable code.
Further comments
This document represents significant architectural work to improve code organization and eliminate technical debt. It includes:
AppResourceBridgeto close gapsThe plan is ready for review and discussion before implementation begins.
https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
Summary by CodeRabbit