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 significantly enhances the synchronization capabilities by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 comprehensive suite of tests for the sync functionality, significantly improving the robustness of the CentralSyncManager. The new tests cover pull, push, session management, and the sync lookup table logic, which is a valuable addition. The changes also include minor refactorings and additions to support this testing effort, such as a test hook for pausing snapshots and enabling the avoidRepull feature by default. My review focuses on enhancing type safety within the new test files by replacing the use of any with more specific types, which will improve maintainability and code clarity.
| import { waitForPushComplete, waitForSession } from '../testUtilities/waitForSync'; | ||
|
|
||
| describe('CentralSyncManager.push', () => { | ||
| let models: any; |
There was a problem hiding this comment.
To improve type safety, please avoid using any. You can type models as TestSyncServerModelRegistry by importing it from ../types.
| let models: any; | |
| let models: TestSyncServerModelRegistry; |
jaskfla
left a comment
jaskfla
left a comment
There was a problem hiding this comment.
Buncha small optional details, but nothing to complain about. Looking great overall—happy for you to pick and choose which comments need addressing and merge without re-review ✅
Thanks for sorting this out 🙏
| * @param {number} tick | ||
| */ | ||
| async markAsStartedAt(id, tick) { | ||
| await this.updateById(id, { started_at_tick: tick }); |
There was a problem hiding this comment.
Small stylistic thing, but could be nice to be explicit that we’re throwing away the return value
| await this.updateById(id, { started_at_tick: tick }); | |
| void (await this.updateById(id, { started_at_tick: tick })); |
|
|
||
| describe('startSession', () => { | ||
| it('creates a new session', async () => { | ||
| const centralSyncManager = new CentralSyncManager(models); |
There was a problem hiding this comment.
Optional: could declare let centralSyncManager: CentralSyncManager in a higher scope and do centralSyncManager = new CentralSyncManager(models) in the beforeEach phase
| lookupTable: { | ||
| perModelUpdateTimeoutMs: 1_000_000, | ||
| avoidRepull: false, | ||
| avoidRepull: true, |
There was a problem hiding this comment.
Double checking this was an intentional change?
| // @ts-expect-error - central-server createApp is a JS file without types | ||
| import { createApp } from '../../../central-server/src/createApp'; | ||
|
|
||
| const models = getTestModels() as ModelRegistry & { user: any }; |
There was a problem hiding this comment.
| const models = getTestModels() as ModelRegistry & { user: any }; | |
| const models = getTestModels() as SyncServerModelRegistry; |
(Remember to update imports)
jaskfla
force-pushed
the
rn-1717-test-four
branch
from
55e4843 to
2600161
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| record_type: 'user_account', | ||
| updated_at_sync_tick: expectedTick.toString(), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test assertion never validates record data
The forEach callback doesn't receive any parameters (uses () instead of (record)), and expect.objectContaining({...}) by itself creates a matcher but never actually asserts anything. The matcher needs to be used with expect(record).toEqual(...) or similar. This test will always pass regardless of the actual data in userAccountLookupData, effectively making it useless for verifying the updated_at_sync_tick values.
|
|
||
| await centralSyncManager.connectToSession(sessionId); | ||
|
|
||
| expect(() => centralSyncManager.connectToSession(sessionId)).not.toThrow(); |
There was a problem hiding this comment.
Test uses synchronous assertion on async function
The test uses expect(() => connectToSession(...)).not.toThrow() on an async function, but .toThrow() only catches synchronous throws, not Promise rejections. Since connectToSession is async, this assertion will always pass even if the function returns a rejected Promise. The correct pattern is await expect(...).resolves.toBeDefined() as used elsewhere (e.g., line 189 uses the async-aware .rejects.toThrow()).
| lookupTable: { | ||
| perModelUpdateTimeoutMs: 1_000_000, | ||
| avoidRepull: false, | ||
| avoidRepull: true, |
There was a problem hiding this comment.
Default config change in test-focused PR
The default value for avoidRepull in DEFAULT_CONFIG was changed from false to true. This changes production behavior in a PR that's titled as "Sync tests" and should only be adding tests. A reviewer in the discussion explicitly asked "Double checking this was an intentional change?" suggesting this may have been accidentally included. This behavioral change affects how sync data is handled for devices and could impact production sync behavior if not intentional.
This reverts commit b7377af.
* type emitter * extract into separate hooks * convenience wrapper * use new hook * read `isSyncing` from `syncManager` every time * refactor * preserve existing behaviour
* type emitter * extract into separate hooks * convenience wrapper * use new hook * read `isSyncing` from `syncManager` every time * refactor * preserve existing behaviour * fix oops * memoise event handler * organise imports * extract hook
2 participants
Changes: