refactor(datatrak): decompose sync status listener hooks #6578

jaskfla · 2025-12-08T23:06:59Z

Previously, we looked at the event type to determine what SetStateActions needed to be fired

This PR flips that around: look at what status we care about, and listen for the relevant event(s)

Bonus: Also exposes each property through its own hook

gemini-code-assist · 2025-12-08T23:07:21Z

Summary of Changes

Hello @jaskfla, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the application's sync status management by transitioning from a single, event-driven state update mechanism to a more granular, hook-based approach. This change significantly enhances the modularity and reusability of sync status information, allowing components to subscribe only to the specific status updates they require, thereby simplifying component logic and improving maintainability.

Highlights

Decomposition of Sync Status Hooks: The monolithic event listener and state management logic for sync status in SyncPage.tsx has been broken down into several specialized, reusable custom hooks.
New syncStatus.ts Module: A new file, syncStatus.ts, has been introduced to centralize and export dedicated hooks for various sync properties, including useIsRequestingSync, useIsInSyncQueue, useIsSyncing, useSyncStage, useSyncProgress, useLastSyncTime, and useSyncError.
Simplified Component Logic: Components like SyncPage.tsx now consume sync status information via these new, focused hooks, significantly reducing their internal state management complexity and improving readability.
Centralized Date Formatting: The logic for formatting the last successful sync time has been moved from SyncPage.tsx into the LastSyncDate.tsx component itself, making the component more self-contained.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

jaskfla · 2025-12-08T23:08:02Z

packages/datatrak-web/src/sync/syncStatus.ts

A bit conflicted about how repetitive this file is; but on balance I think still a bit easier to code trace

gemini-code-assist

Code Review

This pull request refactors the application's sync status management by introducing a new syncStatus.ts file containing several custom React hooks (e.g., useIsRequestingSync, useIsSyncing, useLastSyncTime, useSyncError). These hooks encapsulate the logic for tracking various sync states and events, allowing the SyncPage component to simplify its internal state management and event listeners. The useIsSyncing hook was moved to this new file, and date formatting for the last successful sync time was centralized within LastSyncDate.tsx. Review comments indicate that several of the new hooks suffer from stale closures due to missing dependencies in useCallback, inefficiently subscribe to all sync manager events instead of specific ones, and contain incorrect logic for clearing sync error messages.

packages/datatrak-web/src/sync/syncStatus.ts

packages/datatrak-web/src/views/Sync/SyncPage.tsx

packages/datatrak-web/src/sync/syncStatus.ts

jaskfla · 2025-12-10T21:48:06Z

packages/datatrak-web/src/sync/syncStatus.ts

+  useEffect(() => {
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_REQUESTING, clear);
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_IN_QUEUE, clear);
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_STARTED, clear);
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_ERROR, update);
+    return () => {
+      syncManager?.emitter?.off(SYNC_EVENT_ACTIONS.SYNC_REQUESTING, clear);
+      syncManager?.emitter?.off(SYNC_EVENT_ACTIONS.SYNC_IN_QUEUE, clear);
+      syncManager?.emitter?.off(SYNC_EVENT_ACTIONS.SYNC_STARTED, clear);
+      syncManager?.emitter?.off(SYNC_EVENT_ACTIONS.SYNC_ERROR, update);
+    };
+  }, [clear, syncManager?.emitter, update]);


Similar to useIsSyncing (see #6574 (comment)), I’m a little spooked that the source of truth for message is actually the useState in this hook. Shouldn’t it read from syncManager every time?

jaskfla · 2025-12-10T21:49:38Z

packages/datatrak-web/src/sync/useIsSyncing.ts

The entirety of this file is now included in syncStatus.ts, with one change: renamed clientSyncManager to syncManager

jaskfla · 2025-12-10T21:54:18Z

packages/datatrak-web/src/views/Sync/SyncPage.tsx

-                  formattedLastSuccessfulSyncTime={formattedLastSuccessfulSyncTime}
-                  lastSyncDate={syncManager.lastSuccessfulSyncTime}


These two props are actually the same thing; one is derived from the other

The component (which already has a heading hard-coded into it and isn’t really intended to be reused) can now just grab the last successful sync time itself from the new useLastSyncTime hook

cursor · 2025-12-10T22:10:18Z

packages/datatrak-web/src/views/Sync/LastSyncDate.tsx

+      () => void setFormattedDate(formatLastSuccessfulSyncTime(lastSyncTime)),
+      1_000,
+    );
+    return () => void clearInterval(interval);


Bug: Missing initial value for formatted date display

The formattedDate state is initialized without a value and the useEffect only calls setFormattedDate via setInterval after the first 1-second delay. On initial render and for the first second, formattedDate will be undefined, causing the <time> element to display nothing when lastSyncTime exists. The formatted value needs to be set immediately when the component mounts or when lastSyncTime changes.

cursor · 2025-12-10T22:10:18Z

packages/datatrak-web/src/sync/syncStatus.ts

+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_STARTED, update);
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_STATE_CHANGED, update);
+    syncManager?.emitter?.on(SYNC_EVENT_ACTIONS.SYNC_ENDED, update);
+    return () => void syncManager?.emitter?.off('*', update);


Bug: Incorrect mitt cleanup fails to remove event listeners

In useSyncProgress, the cleanup function uses emitter?.off('*', update) but the handlers were registered on specific event types like SYNC_REQUESTING, SYNC_IN_QUEUE, etc. In mitt, calling off('*', handler) only removes a handler that was registered with on('*', handler), not handlers registered on specific event types. This cleanup doesn't actually unsubscribe the handlers, causing a memory leak and potential duplicate handler invocations.

cursor · 2025-12-10T22:10:18Z

packages/datatrak-web/src/sync/syncStatus.ts

+
+export function useSyncError(): string | null {
+  const syncManager = useSyncContext()?.clientSyncManager;
+  const [message, setMessage] = useState<string | null>(null);


Bug: Error message not initialized from syncManager state

The useSyncError hook initializes message state to null rather than reading from syncManager.errorMessage. The original code in SyncPage initialized with useState<string | null>(syncManager.errorMessage). If a user navigates to a page using this hook after an error occurred but before a new sync action clears it, they won't see the existing error - a behavioral regression from the previous implementation.

jaskfla · 2025-12-11T04:41:19Z

Closing in favour of #6585

jaskfla commented Dec 8, 2025

View reviewed changes

gemini-code-assist bot reviewed Dec 8, 2025

View reviewed changes

cursor bot reviewed Dec 8, 2025

View reviewed changes

packages/datatrak-web/src/sync/syncStatus.ts Outdated Show resolved Hide resolved

packages/datatrak-web/src/views/Sync/SyncPage.tsx Outdated Show resolved Hide resolved

packages/datatrak-web/src/sync/syncStatus.ts Outdated Show resolved Hide resolved

jaskfla marked this pull request as draft December 8, 2025 23:15

jaskfla force-pushed the extract-sync-status branch from a375546 to 1f0c5fa Compare December 8, 2025 23:15

jaskfla force-pushed the sync-animation branch from 4f9a81e to 9530f0a Compare December 9, 2025 02:53

jaskfla force-pushed the extract-sync-status branch from 441e203 to 88c2240 Compare December 9, 2025 02:53

jaskfla commented Dec 10, 2025

View reviewed changes

jaskfla force-pushed the extract-sync-status branch from 175f27d to 7e88fcf Compare December 10, 2025 21:50

jaskfla commented Dec 10, 2025

View reviewed changes

jaskfla marked this pull request as ready for review December 10, 2025 22:00

jaskfla force-pushed the extract-sync-status branch 2 times, most recently from a5e67a8 to ad2b028 Compare December 10, 2025 22:04

cursor bot reviewed Dec 10, 2025

View reviewed changes

jaskfla force-pushed the extract-sync-status branch 3 times, most recently from 1936d3e to 5c3268e Compare December 10, 2025 22:23

jaskfla force-pushed the sync-animation branch from 9530f0a to ba7a20d Compare December 11, 2025 01:46

jaskfla added 7 commits December 11, 2025 14:46

extract

aaa86fc

convenience wrapper

b120025

type emitter

e82e21b

relevant events only

be684ac

add missing dependency

f7c7a3f

restore relative date refresh

4b00bc7

use new hook

5a0befd

jaskfla force-pushed the extract-sync-status branch from 5c3268e to 5a0befd Compare December 11, 2025 01:46

jaskfla mentioned this pull request Dec 11, 2025

feat(sync): RN-1717: Sync bugs #6446

Open

jaskfla closed this Dec 11, 2025

		formattedLastSuccessfulSyncTime={formattedLastSuccessfulSyncTime}
		lastSyncDate={syncManager.lastSuccessfulSyncTime}

refactor(datatrak): decompose sync status listener hooks #6578

refactor(datatrak): decompose sync status listener hooks #6578

Uh oh!

Conversation

jaskfla commented Dec 8, 2025

Uh oh!

gemini-code-assist bot commented Dec 8, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

jaskfla Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaskfla Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

jaskfla Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

jaskfla Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Dec 10, 2025

Choose a reason for hiding this comment

Bug: Missing initial value for formatted date display

Uh oh!

cursor bot Dec 10, 2025

Choose a reason for hiding this comment

Bug: Incorrect mitt cleanup fails to remove event listeners

Uh oh!

cursor bot Dec 10, 2025

Choose a reason for hiding this comment

Bug: Error message not initialized from syncManager state

Uh oh!

jaskfla commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jaskfla Dec 10, 2025 •

edited

Loading