feat(sync): RN-1545: Fix bugs #6492
Conversation
Summary of ChangesHello @chris-bes, 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 addresses various synchronization-related bugs and introduces several enhancements to improve the reliability, performance, and maintainability of the sync process across both the client and server. Key changes include optimizing how sync snapshots are generated for new versus existing projects, bolstering error handling for database batch operations, and refining client-side sync management. Additionally, it prevents concurrent execution of scheduled tasks and improves database dependency ordering for more robust data handling. 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 several fixes and refactorings related to the sync mechanism. Key changes include:
- Implementing a more robust sync strategy that performs a full sync for new projects and a delta sync for existing ones.
- Improving error handling in database save operations to provide more specific error messages, which will greatly aid in debugging.
- Ensuring database changes are saved in an order that respects foreign key constraints by sorting models based on their dependencies.
- Preventing concurrent execution of scheduled tasks.
- Adding cleanup for database connections to prevent resource leaks.
The changes are well-implemented and significantly improve the robustness and debuggability of the sync process. I have a couple of suggestions for further improvement: one to simplify an async call and another to make date filtering logic more robust.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
chris-bes
changed the base branch from
rn-1545-remove-user-id
to
rn-1545-allow-switching-project
| await queryClient.invalidateQueries(['getUser']); | ||
| // If the user changes their project, we need to invalidate the entity descendants query so that recent entities are updated if they change back to the previous project without refreshing the page | ||
| if (variables.projectId) { | ||
| queryClient.invalidateQueries(['entityDescendants']); | ||
| queryClient.invalidateQueries(['tasks']); | ||
|
|
||
| await queryClient.invalidateQueries(['entityDescendants']); | ||
| await queryClient.invalidateQueries(['tasks']); |
There was a problem hiding this comment.
Did these cause issues? 👀
From memory TkDodo (TanStack contributor) had a blog post suggesting that we can “fire and forget” these and let it update in the background
|
|
||
| const importSurveyResponsePermissionsChecker = async accessPolicy => { | ||
| await models.surveyResponse.assertCanImport(accessPolicy, entitiesBySurveyCode); | ||
| await models.surveyResponse.assertCanImport(models, accessPolicy, entitiesBySurveyCode); |
There was a problem hiding this comment.
Keep out of scope for me to do; but if we need to pass models in, it’ll be cleaner to make this a static method SurveyResponseModel.assertCanImport()
There was a problem hiding this comment.
Bit late in the project for me to say this, but next time don’t hesitate to make me fix my own regressions 😅🙏
Thanks so much for fixing this
| /** | ||
| * This is the Jest-sanctioned workaround | ||
| * @see https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom | ||
| */ | ||
| Object.defineProperty(window, 'matchMedia', { |
There was a problem hiding this comment.
Nice, centralising this is much cleaner 🙏
… into rn-1545-fix-bugs # Conflicts: # packages/central-server/src/apiV2/import/importStriveLabResults/importStriveLabResults.js # packages/central-server/src/permissions/assertions.js # packages/central-server/src/tests/apiV2/entities/EditEntity.test.js # packages/database/src/core/modelClasses/SurveyResponse/SurveyResponse.js # packages/tsutils/src/task/formatFilters.ts # packages/types/src/types/models.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… into rn-1545-fix-bugs # Conflicts: # packages/central-server/src/apiV2/meditrakApp/postChanges.js
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… into rn-1545-fix-bugs # Conflicts: # packages/central-server/src/tests/apiV2/import/importSurveyResponses/assertCanImportSurveyResponses.test.js # packages/database/src/__tests__/modelClasses/User/addRecentEntities.test.js
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
# Conflicts: # packages/datatrak-web/src/api/mutations/useEditUser.ts
# Conflicts: # packages/types/src/schemas/schemas.ts
# Conflicts: # packages/datatrak-web/jest.setup.js
chris-bes
merged commit 1fa6e63
into
rn-1545-epic-datatrak-offline
2 participants
Changes:
Fix various sync bugs
Note
Strengthens sync persistence (dependency-ordered writes, granular error surfacing), updates survey response permission APIs, improves Datatrak sync flow/tests, enables streaming in nginx, prevents overlapping scheduled tasks, and updates schemas/SQL.
sortModelsByDependencyOrderand use insaveIncomingSnapshotChanges.saveCreates/Updates/Deletesresilient with per-record fallbacks and clearer error messages; useupdateById.getDependencyOrder.surveyResponse.assertCanImport(models, ...)andassertCanSubmit(models, ...)across imports/submissions/resubmissions.SurveyResponseModelfrom@tupaia/database.clientSyncManager.triggerSync/triggerUrgentSync, remove projects-in-sync hook; add project to sync on login/route; close DB connections on unmount.matchMedia, DB, contexts) and adjust tests (user id/accessPolicy).sync-apianddatatrak-web-apito support streaming.ScheduledTask: prevent overlapping runs viaisRunningflag.addRecentEntities: validate entities individually and write via SQL; update tests to use real models.buildSyncLookupSelect: simplify params (removeuserIds).updated_at_sync_tickto several schemas (e.g.,Country) and new task/raw result schemas; adjust enums/req-res schemas.@tupaia/superset-api: add@tupaia/utilsdependency.api-clientservice subdomains includesync-apiconsistently.Written by Cursor Bugbot for commit 46f4730. This will update automatically on new commits. Configure here.