diff --git a/CHANGELOG.md b/CHANGELOG.md index 1683aa1f1c..ccffb45567 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ and this project adheres to ### Fixed +- Fix flickering of active collaborator icons between states(active, inactive, + unavailable) [#3931](https://github.com/OpenFn/lightning/issues/3931) + ## [2.15.0-pre] - 2025-11-20 ### Added diff --git a/assets/js/collaborative-editor/stores/createAwarenessStore.ts b/assets/js/collaborative-editor/stores/createAwarenessStore.ts index 81f0c07241..1dd5a8534e 100644 --- a/assets/js/collaborative-editor/stores/createAwarenessStore.ts +++ b/assets/js/collaborative-editor/stores/createAwarenessStore.ts @@ -120,6 +120,7 @@ export const createAwarenessStore = (): AwarenessStore => { rawAwareness: null, isConnected: false, lastUpdated: null, + userCache: new Map(), } as AwarenessState, // No initial transformations needed draft => draft @@ -128,11 +129,15 @@ export const createAwarenessStore = (): AwarenessStore => { const listeners = new Set<() => void>(); let awarenessInstance: Awareness | null = null; let lastSeenTimer: NodeJS.Timeout | null = null; + let cacheCleanupTimer: NodeJS.Timeout | null = null; + + // Cache configuration + const CACHE_TTL = 60 * 1000; // 1 minute in milliseconds // Redux DevTools integration const devtools = wrapStoreWithDevTools({ name: 'AwarenessStore', - excludeKeys: ['rawAwareness'], // Exclude Y.js Awareness object + excludeKeys: ['rawAwareness', 'userCache'], // Exclude Y.js Awareness object and Map cache maxAge: 200, // Higher limit since awareness changes are frequent }); @@ -195,6 +200,53 @@ export const createAwarenessStore = (): AwarenessStore => { return users; }; + /** + * Update cache with current users + */ + const updateCache = (users: AwarenessUser[]) => { + const now = Date.now(); + const newCache = new Map(state.userCache); + + // Add/update users in cache + users.forEach(user => { + newCache.set(user.user.id, { + user, + cachedAt: now, + }); + }); + + // Clean up expired entries (older than 1 minute) + newCache.forEach((cachedUser, userId) => { + if (now - cachedUser.cachedAt > CACHE_TTL) { + newCache.delete(userId); + } + }); + + return newCache; + }; + + /** + * Merge live users with cached users + * Cached users are used as fallback when they're missing from live awareness + */ + const mergeUsersWithCache = (liveUsers: AwarenessUser[]): AwarenessUser[] => { + const liveUserIds = new Set(liveUsers.map(u => u.user.id)); + const mergedUsers = [...liveUsers]; + + // Add cached users that aren't in the live set + state.userCache.forEach((cachedUser, userId) => { + if (!liveUserIds.has(userId)) { + // Only add if cache is still valid + const now = Date.now(); + if (now - cachedUser.cachedAt <= CACHE_TTL) { + mergedUsers.push(cachedUser.user); + } + } + }); + + return mergedUsers; + }; + /** * Handle awareness state changes - core collaborative data update pattern */ @@ -204,10 +256,13 @@ export const createAwarenessStore = (): AwarenessStore => { return; } - const users = extractUsersFromAwareness(awarenessInstance); + const liveUsers = extractUsersFromAwareness(awarenessInstance); + const mergedUsers = mergeUsersWithCache(liveUsers); + const updatedCache = updateCache(liveUsers); state = produce(state, draft => { - draft.users = users; + draft.users = mergedUsers; + draft.userCache = updatedCache; draft.lastUpdated = Date.now(); }); notify('awarenessChange'); @@ -217,6 +272,36 @@ export const createAwarenessStore = (): AwarenessStore => { // PATTERN 2: Direct Immer → Notify + Awareness Update (Local Commands) // ============================================================================= + /** + * Set up periodic cache cleanup + */ + const setupCacheCleanup = () => { + if (cacheCleanupTimer) { + clearInterval(cacheCleanupTimer); + } + + // Clean up expired cache entries every 30 seconds + cacheCleanupTimer = setInterval(() => { + const now = Date.now(); + const newCache = new Map(state.userCache); + let hasChanges = false; + + newCache.forEach((cachedUser, userId) => { + if (now - cachedUser.cachedAt > CACHE_TTL) { + newCache.delete(userId); + hasChanges = true; + } + }); + + if (hasChanges) { + state = produce(state, draft => { + draft.userCache = newCache; + }); + notify('cacheCleanup'); + } + }, 30000); // Check every 30 seconds + }; + /** * Initialize awareness instance and set up observers */ @@ -235,6 +320,9 @@ export const createAwarenessStore = (): AwarenessStore => { // Set up awareness observer for Pattern 1 updates awareness.on('change', handleAwarenessChange); + // Set up cache cleanup + setupCacheCleanup(); + // Update local state state = produce(state, draft => { draft.localUser = userData; @@ -267,6 +355,11 @@ export const createAwarenessStore = (): AwarenessStore => { lastSeenTimer = null; } + if (cacheCleanupTimer) { + clearInterval(cacheCleanupTimer); + cacheCleanupTimer = null; + } + devtools.disconnect(); state = produce(state, draft => { @@ -276,6 +369,7 @@ export const createAwarenessStore = (): AwarenessStore => { draft.isInitialized = false; draft.isConnected = false; draft.lastUpdated = Date.now(); + draft.userCache = new Map(); }); notify('destroyAwareness'); }; @@ -367,13 +461,14 @@ export const createAwarenessStore = (): AwarenessStore => { /** * Update last seen timestamp + * @param forceTimestamp - Optional timestamp to use instead of Date.now() */ - const updateLastSeen = () => { + const updateLastSeen = (forceTimestamp?: number) => { if (!awarenessInstance) { return; } - const timestamp = Date.now(); + const timestamp = forceTimestamp ?? Date.now(); awarenessInstance.setLocalStateField('lastSeen', timestamp); // Note: We don't update local state here as awareness observer will handle it @@ -383,19 +478,96 @@ export const createAwarenessStore = (): AwarenessStore => { * Set up automatic last seen updates */ const setupLastSeenTimer = () => { - if (lastSeenTimer) { - clearInterval(lastSeenTimer); + let frozenTimestamp: number | null = null; + + const startTimer = () => { + if (lastSeenTimer) { + clearInterval(lastSeenTimer); + } + lastSeenTimer = setInterval(() => { + // If page is hidden, use frozen timestamp, otherwise use current time + if (frozenTimestamp) frozenTimestamp++; // This is to make sure that state is updated and data gets transmitted + updateLastSeen(frozenTimestamp ?? undefined); + }, 10000); // Update every 10 seconds + }; + + const getVisibilityProps = () => { + if (typeof document.hidden !== 'undefined') { + return { hidden: 'hidden', visibilityChange: 'visibilitychange' }; + } + + if ( + // @ts-expect-error webkitHidden not defined + typeof (document as unknown as Document).webkitHidden !== 'undefined' + ) { + return { + hidden: 'webkitHidden', + visibilityChange: 'webkitvisibilitychange', + }; + } + // @ts-expect-error mozHidden not defined + if (typeof (document as unknown as Document).mozHidden !== 'undefined') { + return { hidden: 'mozHidden', visibilityChange: 'mozvisibilitychange' }; + } + // @ts-expect-error msHidden not defined + if (typeof (document as unknown as Document).msHidden !== 'undefined') { + return { hidden: 'msHidden', visibilityChange: 'msvisibilitychange' }; + } + return null; + }; + + const visibilityProps = getVisibilityProps(); + + const handleVisibilityChange = () => { + if (!visibilityProps) return; + + const isHidden = (document as unknown as Document)[ + visibilityProps.hidden as keyof Document + ]; + + if (isHidden) { + // Page is hidden, freeze the current timestamp + frozenTimestamp = Date.now(); + } else { + // Page is visible, unfreeze and update immediately + frozenTimestamp = null; + updateLastSeen(); + } + }; + + // Set up visibility change listener if supported + if (visibilityProps) { + document.addEventListener( + visibilityProps.visibilityChange, + handleVisibilityChange + ); + + // Check initial visibility state + const isHidden = (document as unknown as Document)[ + visibilityProps.hidden as keyof Document + ]; + if (isHidden) { + // Start with frozen timestamp if already hidden + frozenTimestamp = Date.now(); + } } - lastSeenTimer = setInterval(() => { - updateLastSeen(); - }, 10000); // Update every 10 seconds + // Always start the timer (whether visible or hidden) + startTimer(); + // cleanup return () => { if (lastSeenTimer) { clearInterval(lastSeenTimer); lastSeenTimer = null; } + + if (visibilityProps) { + document.removeEventListener( + visibilityProps.visibilityChange, + handleVisibilityChange + ); + } }; }; diff --git a/assets/js/collaborative-editor/types/awareness.ts b/assets/js/collaborative-editor/types/awareness.ts index a9bc7f692c..e955277fde 100644 --- a/assets/js/collaborative-editor/types/awareness.ts +++ b/assets/js/collaborative-editor/types/awareness.ts @@ -37,6 +37,14 @@ export interface LocalUserData { color: string; } +/** + * Cached user entry for fallback when awareness is throttled + */ +export interface CachedUser { + user: AwarenessUser; + cachedAt: number; +} + /** * Awareness store state */ @@ -52,6 +60,9 @@ export interface AwarenessState { // Connection state isConnected: boolean; lastUpdated: number | null; + + // Fallback cache for throttled awareness updates (1 minute TTL) + userCache: Map; } /** diff --git a/assets/test/collaborative-editor/components/ActiveCollaborators.test.tsx b/assets/test/collaborative-editor/components/ActiveCollaborators.test.tsx index a694c83b1b..6d713f1446 100644 --- a/assets/test/collaborative-editor/components/ActiveCollaborators.test.tsx +++ b/assets/test/collaborative-editor/components/ActiveCollaborators.test.tsx @@ -160,9 +160,9 @@ describe('ActiveCollaborators - Activity Indicator', () => { mockRemoteUsers = []; }); - test('shows green border for users active within last 2 minutes', () => { + test('shows green border for users active within last 12 seconds (0.2 minutes)', () => { const now = Date.now(); - const oneMinuteAgo = now - 60 * 1000; // 1 minute ago + const fiveSecondsAgo = now - 5 * 1000; // 5 seconds ago mockRemoteUsers = [ createMockAwarenessUser({ @@ -172,7 +172,7 @@ describe('ActiveCollaborators - Activity Indicator', () => { email: 'john@example.com', color: '#ff0000', }, - lastSeen: oneMinuteAgo, + lastSeen: fiveSecondsAgo, }), ]; @@ -182,9 +182,9 @@ describe('ActiveCollaborators - Activity Indicator', () => { expect(borderDiv).toBeInTheDocument(); }); - test('shows gray border for users inactive for more than 2 minutes', () => { + test('shows gray border for users inactive for more than 12 seconds (0.2 minutes)', () => { const now = Date.now(); - const threeMinutesAgo = now - 3 * 60 * 1000; // 3 minutes ago + const thirtySecondsAgo = now - 30 * 1000; // 30 seconds ago mockRemoteUsers = [ createMockAwarenessUser({ @@ -194,7 +194,7 @@ describe('ActiveCollaborators - Activity Indicator', () => { email: 'john@example.com', color: '#ff0000', }, - lastSeen: threeMinutesAgo, + lastSeen: thirtySecondsAgo, }), ]; @@ -223,12 +223,12 @@ describe('ActiveCollaborators - Activity Indicator', () => { expect(borderDiv).toBeInTheDocument(); }); - test('correctly implements the 2-minute threshold (120,000ms)', () => { + test('correctly implements the 12-second threshold (0.2 minutes = 12,000ms)', () => { const now = Date.now(); - const justUnderTwoMinutes = now - (2 * 60 * 1000 - 1000); // 1 second before threshold - const justOverTwoMinutes = now - (2 * 60 * 1000 + 1000); // 1 second after threshold + const justUnderThreshold = now - (0.2 * 60 * 1000 - 1000); // 1 second before threshold (11 seconds ago) + const justOverThreshold = now - (0.2 * 60 * 1000 + 1000); // 1 second after threshold (13 seconds ago) - // User just under 2 minutes should have green border + // User just under 12 seconds should have green border mockRemoteUsers = [ createMockAwarenessUser({ clientId: 1, @@ -238,7 +238,7 @@ describe('ActiveCollaborators - Activity Indicator', () => { email: 'active@example.com', color: '#ff0000', }, - lastSeen: justUnderTwoMinutes, + lastSeen: justUnderThreshold, }), createMockAwarenessUser({ clientId: 2, @@ -248,7 +248,7 @@ describe('ActiveCollaborators - Activity Indicator', () => { email: 'inactive@example.com', color: '#00ff00', }, - lastSeen: justOverTwoMinutes, + lastSeen: justOverThreshold, }), ]; @@ -261,7 +261,7 @@ describe('ActiveCollaborators - Activity Indicator', () => { expect(grayBorder).toBeInTheDocument(); }); - test('border color updates when user crosses the 2-minute threshold', () => { + test('border color updates when user crosses the 12-second threshold', () => { vi.useFakeTimers(); const now = Date.now(); @@ -282,8 +282,8 @@ describe('ActiveCollaborators - Activity Indicator', () => { // Initially should have green border expect(container.querySelector('.border-green-500')).toBeInTheDocument(); - // Advance time by 3 minutes - vi.advanceTimersByTime(3 * 60 * 1000); + // Advance time by 15 seconds (beyond the 12-second threshold) + vi.advanceTimersByTime(15 * 1000); // Re-render to trigger the component to re-evaluate rerender(); @@ -379,6 +379,98 @@ describe('ActiveCollaborators - Store Integration', () => { }); }); +describe('ActiveCollaborators - Cache Behavior', () => { + beforeEach(() => { + mockRemoteUsers = []; + }); + + test('continues showing users from cache when awareness is throttled', () => { + const now = Date.now(); + + // Initial user list + mockRemoteUsers = [ + createMockAwarenessUser({ + user: { + id: 'user-1', + name: 'John Doe', + email: 'john@example.com', + color: '#ff0000', + }, + lastSeen: now, + }), + ]; + + const { rerender } = render(); + expect(screen.getByText('JD')).toBeInTheDocument(); + + // Simulate throttling - user still in cache (via merged users from store) + // The cache keeps users for 1 minute, so they should still appear + mockRemoteUsers = [ + createMockAwarenessUser({ + user: { + id: 'user-1', + name: 'John Doe', + email: 'john@example.com', + color: '#ff0000', + }, + lastSeen: now, + }), + ]; + + rerender(); + expect(screen.getByText('JD')).toBeInTheDocument(); + }); + + test('cached users appear with their last known state', () => { + const now = Date.now(); + const fiveSecondsAgo = now - 5 * 1000; + + // User active 5 seconds ago + mockRemoteUsers = [ + createMockAwarenessUser({ + user: { + id: 'user-1', + name: 'Jane Smith', + email: 'jane@example.com', + color: '#00ff00', + }, + lastSeen: fiveSecondsAgo, + }), + ]; + + render(); + + // Should still show as active (green border) because < 12 seconds + expect(screen.getByText('JS')).toBeInTheDocument(); + expect(screen.getByText('JS').closest('div')?.parentElement).toHaveClass( + 'border-green-500' + ); + }); + + test('cached users eventually expire after threshold', () => { + const now = Date.now(); + const thirtySecondsAgo = now - 30 * 1000; // Well over 12 seconds + + mockRemoteUsers = [ + createMockAwarenessUser({ + user: { + id: 'user-1', + name: 'Old User', + email: 'old@example.com', + color: '#ff0000', + }, + lastSeen: thirtySecondsAgo, + }), + ]; + + const { container } = render(); + + // Should show with gray border (inactive) because > 12 seconds + expect(screen.getByText('OU')).toBeInTheDocument(); + expect(container.querySelector('.border-gray-500')).toBeInTheDocument(); + }); +}); + describe('ActiveCollaborators - Edge Cases', () => { beforeEach(() => { mockRemoteUsers = [];