Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions apps/meteor/client/hooks/useReactiveValue.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@ import { AuthenticationContext, useSetting } from '@rocket.chat/ui-contexts';
import { Accounts } from 'meteor/accounts-base';
import { Meteor } from 'meteor/meteor';
import type { ContextType, ReactElement, ReactNode } from 'react';
import { useMemo } from 'react';
import { useMemo, useSyncExternalStore } from 'react';

import { useLDAPAndCrowdCollisionWarning } from './hooks/useLDAPAndCrowdCollisionWarning';
import { capitalize as capitalizeService } from '../../../lib/utils/stringUtils';
import { useReactiveValue } from '../../hooks/useReactiveValue';
import { loginServices } from '../../lib/loginServices';
import { getDdpSdk } from '../../lib/sdk/ddpSdk';
import { STORAGE_KEYS, getStoredItem, removeStoredItem } from '../../lib/sdk/storage';
Expand All @@ -29,7 +28,34 @@ const callLoginMethod = (
});
};

const getLoggingIn = () => Accounts.loggingIn();
// Bridge Accounts.loggingIn() — Meteor's Tracker-reactive flag — into a
// non-reactive subscribe/getSnapshot pair for useSyncExternalStore. We hook
// `_setLoggingIn` (Meteor's internal flip, also accessed in
// apps/meteor/client/meteor/overrides/killMeteorStream.ts) to fan out
// transitions without entering a Tracker computation.
const loggingInListeners = new Set<() => void>();
let loggingInBridgeInstalled = false;
const installLoggingInBridge = (): void => {
if (loggingInBridgeInstalled) return;
loggingInBridgeInstalled = true;
const wrap = Accounts as unknown as { _setLoggingIn?: (v: boolean) => void };
const original = wrap._setLoggingIn;
if (typeof original !== 'function') return;
wrap._setLoggingIn = function (this: typeof Accounts, v: boolean) {
original.call(this, v);
loggingInListeners.forEach((cb) => cb());
};
};

const subscribeLoggingIn = (cb: () => void): (() => void) => {
installLoggingInBridge();
loggingInListeners.add(cb);
return () => {
loggingInListeners.delete(cb);
};
};

const getLoggingInSnapshot = (): boolean => Accounts.loggingIn();

const AuthenticationProvider = ({ children }: AuthenticationProviderProps): ReactElement => {
const isLdapEnabled = useSetting('LDAP_Enable', false);
Expand All @@ -39,7 +65,7 @@ const AuthenticationProvider = ({ children }: AuthenticationProviderProps): Reac

useLDAPAndCrowdCollisionWarning();

const isLoggingIn = useReactiveValue(getLoggingIn);
const isLoggingIn = useSyncExternalStore(subscribeLoggingIn, getLoggingInSnapshot);

const contextValue = useMemo(
(): ContextType<typeof AuthenticationContext> => ({
Expand Down
63 changes: 39 additions & 24 deletions apps/meteor/client/providers/ServerProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ import type { Method, PathFor, OperationParams, OperationResult, UrlParams, Path
import type { UploadResult, ServerContextValue } from '@rocket.chat/ui-contexts';
import { ServerContext } from '@rocket.chat/ui-contexts';
import { Meteor } from 'meteor/meteor';
import { Tracker } from 'meteor/tracker';
import { compile } from 'path-to-regexp';
import { useMemo, type ReactNode } from 'react';
import { useMemo, useSyncExternalStore, type ReactNode } from 'react';

import { sdk } from '../../app/utils/client/lib/SDKClient';
import { Info as info } from '../../app/utils/rocketchat.info';
import { useReactiveValue } from '../hooks/useReactiveValue';
import { absoluteUrl } from '../lib/absoluteUrl';
import { ensureConnectedAndAuthenticated, getDdpSdk } from '../lib/sdk/ddpSdk';
import { isSdkTransportEnabled } from '../lib/sdk/sdkTransportEnabled';
Expand Down Expand Up @@ -99,16 +97,6 @@ const reconnect = sdkTransportEnabled
}
: () => Meteor.reconnect();

// With SDK transport on, combine Meteor's DDP status with DDPSDK's so the
// ConnectionStatusBar / idle-connection hooks reflect the worst-case of both
// transports. With the flag off, route status straight through Meteor —
// Meteor.status() is already Tracker-reactive, so adding a second dependency
// via the meteor-backed proxy's emitter would just double-fire the autorun.
const ddpSdkStatusDep = sdkTransportEnabled ? new Tracker.Dependency() : undefined;
if (sdkTransportEnabled) {
getDdpSdk().connection.on('connection', () => ddpSdkStatusDep!.changed());
}

type CombinedStatus = ReturnType<typeof Meteor.status>;

const sdkStatusToMeteor = (sdkStatus: string, meteor: CombinedStatus): CombinedStatus => {
Expand All @@ -132,21 +120,48 @@ const sdkStatusToMeteor = (sdkStatus: string, meteor: CombinedStatus): CombinedS
}
};

const getStatus = sdkTransportEnabled
? () => {
ddpSdkStatusDep!.depend();
return sdkStatusToMeteor(getDdpSdk().connection.status, Meteor.status());
}
: // useReactiveValue stores the snapshot in useSyncExternalStore, which
// compares by identity. Meteor.status() reuses the same internal object
// (mutated in place), so without spreading the snapshot reference never
// changes and ConnectionStatusBar stops re-rendering on connect/drop.
() => ({ ...Meteor.status() });
// With SDK transport on, combine Meteor's DDP status with DDPSDK's so the
// ConnectionStatusBar / idle-connection hooks reflect the worst-case of both
// transports. With the flag off, route status straight through Meteor —
// `meteorBackedSdk` already bridges Meteor's `_stream` events into
// `sdk.connection.on('connection')`, so the same subscription works in both
// modes.
const computeStatus: () => CombinedStatus = sdkTransportEnabled
? () => sdkStatusToMeteor(getDdpSdk().connection.status, Meteor.status())
: () => ({ ...Meteor.status() });

const isStatusEqual = (a: CombinedStatus, b: CombinedStatus): boolean =>
a.status === b.status && a.connected === b.connected && a.retryCount === b.retryCount && a.retryTime === b.retryTime;

let cachedStatus: CombinedStatus = computeStatus();
const statusListeners = new Set<() => void>();
let statusBridgeStarted = false;

const ensureStatusBridge = (): void => {
if (statusBridgeStarted) return;
statusBridgeStarted = true;
getDdpSdk().connection.on('connection', () => {
const next = computeStatus();
if (isStatusEqual(cachedStatus, next)) return;
cachedStatus = next;
statusListeners.forEach((cb) => cb());
});
};

const subscribeStatus = (cb: () => void): (() => void) => {
ensureStatusBridge();
statusListeners.add(cb);
return () => {
statusListeners.delete(cb);
};
};
Comment on lines +136 to +157

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stale cachedStatus between module load and first subscribe can leave the status bar wedged.

cachedStatus is computed once at module-evaluation time (Line 136), and the SDK 'connection' listener is only installed when the first component subscribes (ensureStatusBridge, Lines 140-149). Any 'connection' transition that fires in the window between module load and the first subscribe is dropped — no listener yet, and cachedStatus is never refreshed. getStatusSnapshot then keeps handing React the stale snapshot, and because useSyncExternalStore's post-subscribe re-check also calls getStatusSnapshot (still stale), no re-render is scheduled. On a stable connection that produces no further 'connection' emits, the ConnectionStatusBar can stay stuck on the initial value (e.g. connecting). The previous Tracker.autorun path re-read on every invalidation, so this is a new regression.

Refresh the cache when the bridge is installed so the first subscriber sees current state:

🛠️ Proposed fix
 const ensureStatusBridge = (): void => {
 	if (statusBridgeStarted) return;
 	statusBridgeStarted = true;
 	getDdpSdk().connection.on('connection', () => {
 		const next = computeStatus();
 		if (isStatusEqual(cachedStatus, next)) return;
 		cachedStatus = next;
 		statusListeners.forEach((cb) => cb());
 	});
+	cachedStatus = computeStatus();
 };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/client/providers/ServerProvider.tsx` around lines 136 - 157, The
cachedStatus is computed only at module load and can be stale until the first
subscriber installs the bridge; modify ensureStatusBridge so after setting
statusBridgeStarted = true and before returning it refreshes cachedStatus by
calling computeStatus() (and if you want to preserve existing behavior also
compare with the old cachedStatus and notify statusListeners if it changed) —
update the function ensureStatusBridge to recompute cachedStatus immediately
(using computeStatus) when installing the
getDdpSdk().connection.on('connection', ...) bridge so getStatusSnapshot/readers
see the current state.


const getStatusSnapshot = (): CombinedStatus => cachedStatus;

type ServerProviderProps = { children?: ReactNode };

const ServerProvider = ({ children }: ServerProviderProps) => {
const { connected, status, retryCount, retryTime } = useReactiveValue(getStatus);
const { connected, status, retryCount, retryTime } = useSyncExternalStore(subscribeStatus, getStatusSnapshot);

const value = useMemo(
(): ServerContextValue => ({
Expand Down
Loading