regression(desktop-app): Moved settings module #36914
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughReplaces the Desktop startup stub: removes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Win as window
participant DI as desktopInjection
participant Req as window.require (wrapped)
participant Orig as Original require
rect rgba(200,220,255,0.18)
note over Win,DI: Desktop context only
Win->>DI: load desktopInjection
DI->>Win: save original require
DI->>Win: replace window.require with wrapper
end
Win->>Req: require('meteor/rocketchat:user-presence')
alt id matches stub
Req-->>Win: return fakeUserPresenceModule
else other id
Req->>Orig: delegate to original require
Orig-->>Req: module
Req-->>Win: module
end
sequenceDiagram
autonumber
participant App as AppLayout
participant F as useDesktopFavicon
participant T as useDesktopTitle
participant RCD as RocketChatDesktop
App->>F: invoke hook
F->>F: resolve absolute favicon URL
alt Desktop present
F->>RCD: setFavicon(url)
end
App->>T: invoke hook
T->>T: read Site_Name setting
alt Desktop present and title set
T->>RCD: setTitle(title)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36914 +/- ##
===========================================
+ Coverage 66.58% 66.59% +0.01%
===========================================
Files 3344 3346 +2
Lines 114639 114661 +22
Branches 21092 21103 +11
===========================================
+ Hits 76329 76356 +27
+ Misses 35623 35619 -4
+ Partials 2687 2686 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tassoevan
force-pushed
the
regression/settings-desktop
branch
from
c88b6cc to
79240f3
Compare
![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
🧹 Nitpick comments (3)
apps/meteor/client/startup/desktopInjection.ts (2)
20-31: Preserve require signature, add guard, and keep compatibility with variant IDs.Wrap with a typeof guard, forward all args, and accept common path variants for the settings module.
- window.require = ((fn) => - Object.assign((id: string) => { - if (id === 'meteor/rocketchat:user-presence') { - return fakeUserPresenceModule; - } - - if (id === '/app/settings/client/index.ts') { - return fakeSettingsModule; - } - - return fn(id); - }, fn))(window.require); + if (typeof window.require === 'function') { + const originalRequire = window.require; + window.require = Object.assign( + (id: string, ...args: unknown[]) => { + if (id === 'meteor/rocketchat:user-presence') { + return fakeUserPresenceModule; + } + if ( + id === '/app/settings/client/index.ts' || + id === '/app/settings/client/index.js' || + id === '/app/settings/client' + ) { + return fakeSettingsModule; + } + // @ts-expect-error forward extra args if present + return originalRequire(id, ...args); + }, + originalRequire, + ); + }
6-18: Make stubs immutable to avoid accidental mutation by consumers.Freeze the stub objects; safer in case downstream code tries to mutate them.
- const fakeUserPresenceModule = { + const fakeUserPresenceModule = Object.freeze({ UserPresence: { awayTime: undefined, start: () => undefined, }, - }; + }); @@ - const fakeSettingsModule = { + const fakeSettingsModule = Object.freeze({ settings: { get: (settingId: string) => watch(PublicSettings.use, (state) => state.get(settingId)?.value), }, - }; + });apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (1)
4-12: Same: movewindowaccess into the hook to avoid module-scope globals.Keeps it import-safe and consistent with
useDesktopTitle.-const { RocketChatDesktop } = window; - export const useDesktopFavicon = () => { const faviconUrl = useAssetPath('favicon'); useEffect(() => { if (!faviconUrl) return; - RocketChatDesktop?.setFavicon(faviconUrl); + (window as any).RocketChatDesktop?.setFavicon(faviconUrl); }, [faviconUrl]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/meteor/client/main.ts(1 hunks)apps/meteor/client/startup/desktopInjection.ts(1 hunks)apps/meteor/client/startup/fakeUserPresence.ts(0 hunks)apps/meteor/client/views/root/AppLayout.tsx(2 hunks)apps/meteor/client/views/root/hooks/useDesktopFavicon.ts(1 hunks)apps/meteor/client/views/root/hooks/useDesktopTitle.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/startup/fakeUserPresence.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/views/root/hooks/useDesktopTitle.ts (1)
packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
apps/meteor/client/views/root/AppLayout.tsx (2)
apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (1)
useDesktopFavicon(6-13)apps/meteor/client/views/root/hooks/useDesktopTitle.ts (1)
useDesktopTitle(6-13)
apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (1)
packages/ui-contexts/src/index.ts (1)
useAssetPath(22-22)
🔇 Additional comments (4)
apps/meteor/client/startup/desktopInjection.ts (2)
4-4: Scope guard looks good.Patching only when
window.RocketChatDesktopexists keeps web builds untouched.
14-18: Confirm Desktop settings.get contract — manual verification requiredAutomated search returned only server-side uses of settings.get and no Desktop/client consumers; cannot confirm whether Desktop expects a reactive accessor, an observable/subscribe API, or a plain value — verify the Desktop/Electron callers of settings.get and confirm the exact return shape they use (callable getter vs. immediate value vs. subscription).
Location: apps/meteor/client/startup/desktopInjection.ts (fakeSettingsModule.settings.get uses watch(PublicSettings.use, state => state.get(settingId)?.value)).apps/meteor/client/main.ts (1)
3-3: Switch to new desktop startup is correct and early enough.Importing
./startup/desktopInjectionbefore the dynamic chain ensures the require hook is in place.apps/meteor/client/views/root/AppLayout.tsx (1)
15-16: Desktop hooks integration looks safe.Hooks are side-effect only and no-ops when Desktop API is absent.
Also applies to: 70-71
tassoevan
force-pushed
the
regression/settings-desktop
branch
from
79240f3 to
e9c68c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (1)
9-11: Consider cleanup for setUrlResolver (HMR/unmount hygiene).If the Desktop API supports resetting/restoring the resolver, add a cleanup to avoid leaking a stale resolver across hot reloads or unmounts.
Can you confirm whether
setUrlResolverreturns the previous resolver or acceptsundefined/nullto clear? If yes, we can add a cleanup in this effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/meteor/client/main.ts(1 hunks)apps/meteor/client/startup/desktopInjection.ts(1 hunks)apps/meteor/client/startup/fakeUserPresence.ts(0 hunks)apps/meteor/client/views/root/AppLayout.tsx(2 hunks)apps/meteor/client/views/root/hooks/useDesktopFavicon.ts(1 hunks)apps/meteor/client/views/root/hooks/useDesktopTitle.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/startup/fakeUserPresence.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/views/root/hooks/useDesktopTitle.ts
- apps/meteor/client/main.ts
- apps/meteor/client/startup/desktopInjection.ts
- apps/meteor/client/views/root/AppLayout.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (1)
packages/ui-contexts/src/index.ts (2)
useAbsoluteUrl(20-20)useAssetPath(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/client/views/root/hooks/useDesktopFavicon.ts (2)
9-18: Good effect ordering.Registering the URL resolver before setting the favicon ensures resolver availability on mount. Nice.
1-2: No action required — RocketChatDesktop already declaredAmbient declaration exists at apps/meteor/client/definitions/global.d.ts (imports IRocketChatDesktop from @rocket.chat/desktop-api); do not add a new types file.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
It introduces a fake module mimicking
/app/settings/client/index.tssince the desktop app references it directly. Moreover, it handles some desktop's observation of server settings (if available) by itself instead of relying on unsafe injected code.Issue(s)
ARCH-1806
Caused by #36844
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Refactor