feat(datatrak): RN-1747: ↻ animation when syncing#6574
feat(datatrak): RN-1747: ↻ animation when syncing#6574
Conversation
Summary of ChangesHello @jaskfla, I'm Gemini Code Assist1! 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 improves the user interface by adding a dynamic visual cue for ongoing data synchronization. When a sync operation is active, the sync button will now display a subtle spinning animation, providing immediate feedback to the user about the system's current state. This enhancement leverages a newly created React hook to monitor sync events and control the animation's visibility. Highlights
Using Gemini Code AssistThe 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
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 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a spinning animation on the sync button to indicate when a sync is in progress. This is achieved by adding a new useIsSyncing hook to track the sync status and applying a CSS animation to the sync icon. My review includes suggestions to improve the implementation in both new and modified files. In SyncButton.tsx, I've suggested a refactoring to better encapsulate styling logic within the styled-component. For the new useIsSyncing.ts hook, I've proposed a change to make it more robust by ensuring the initial sync state is correctly determined. These changes will improve the maintainability and correctness of the new feature.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jaskfla
force-pushed
the
sync-animation
branch
from
328c177 to
4f9a81e
Compare
| emitter.on(SYNC_EVENT_ACTIONS.SYNC_STARTED, set); | ||
| emitter.on(SYNC_EVENT_ACTIONS.SYNC_ENDED, unset); | ||
|
|
||
| return () => { | ||
| emitter.off(SYNC_EVENT_ACTIONS.SYNC_STARTED, set); | ||
| emitter.off(SYNC_EVENT_ACTIONS.SYNC_ENDED, unset); | ||
| }; |
There was a problem hiding this comment.
This logic taken directly from SyncPage’s Effect, but: wouldn’t it make more sense to read the value directly from clientSyncManager.isSyncing?
This way there’s a single source of truth
There was a problem hiding this comment.
Agreed, let's read from clientSyncManager.isSyncing 👍
jaskfla
force-pushed
the
sync-animation
branch
from
4f9a81e to
9530f0a
Compare
jaskfla
force-pushed
the
sync-animation
branch
from
9530f0a to
ba7a20d
Compare
| const update = useCallback( | ||
| () => void setIsSyncing(syncManager?.isSyncing ?? false), | ||
| [syncManager?.isSyncing], | ||
| ); |
There was a problem hiding this comment.
Bug: Stale closure in useCallback due to incorrect dependencies
The useCallback dependency array includes syncManager?.isSyncing, which is a primitive value that gets captured at render time. This causes the callback to be recreated when React notices the value changed (during re-render), but the event listener subscriptions in the useEffect still hold references to older callback versions until the effect re-runs. The dependency should be [syncManager] since the callback needs to read isSyncing fresh from the stable syncManager object reference at invocation time, not depend on the value being captured at callback creation.
There was a problem hiding this comment.
I’ll do some dev testing
| to { | ||
| transform: rotate(1turn); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: CSS keyframe name with double-hyphen prefix may fail
The keyframe animation name --spin-sync-button starts with double hyphens, which follows the CSS custom property naming convention (<dashed-ident>). However, @keyframes expects a <custom-ident> which explicitly excludes identifiers starting with --. This could cause the animation to silently fail or not apply in some browsers, preventing the sync button from spinning during sync operations.
There was a problem hiding this comment.
Absolute BS. The -- prefix is best practice.
jaskfla
force-pushed
the
rn-1717-test-four
branch
3 times, most recently
from
55e4843 to
2600161
Compare
This reverts commit 7dcc1ec.
# Conflicts: # packages/central-server/src/apiV2/import/importSurveyResponses/assertCanImportSurveyResponses.js # packages/central-server/src/dataAccessors/upsertAnswers.js
jaskfla
force-pushed
the
sync-animation
branch
from
4ebb718 to
9fbc5ce
Compare
2 participants
Only tangentially related to RN-1717, but need a ticket number to pass CI check for PR title