feat(code): do not check for updates after update restart#1932
feat(code): do not check for updates after update restart#1932adboio wants to merge 1 commit intographite-base/1932from
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/main/services/updates/service.test.ts
Line: 285-340
Comment:
**Prefer parameterised tests**
The three scenario tests in `"initial check on startup"` share an identical structure — set `_value`, set `version`, call `initializeService`, assert on `check` and `set` — and should be collapsed into a single `it.each` table per the team's testing conventions.
```typescript
it.each([
{
desc: "first ever launch (no stored version)",
storedVersion: null,
appVersion: "1.0.0",
expectCheck: true,
expectSet: "1.0.0",
},
{
desc: "relaunching the same version",
storedVersion: "1.0.0",
appVersion: "1.0.0",
expectCheck: true,
expectSet: null,
},
{
desc: "post-update restart (version changed)",
storedVersion: "1.0.0",
appVersion: "1.0.1",
expectCheck: false,
expectSet: "1.0.1",
},
])("$desc", async ({ storedVersion, appVersion, expectCheck, expectSet }) => {
mockUpdatesStore._value = storedVersion;
mockAppMeta.version = appVersion;
await initializeService(service);
expectCheck
? expect(mockUpdater.check).toHaveBeenCalled()
: expect(mockUpdater.check).not.toHaveBeenCalled();
if (expectSet) {
expect(mockUpdatesStore.set).toHaveBeenCalledWith("lastLaunchedVersion", expectSet);
} else {
expect(mockUpdatesStore.set).not.toHaveBeenCalled();
}
});
```
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(code): do not check for updates aft..." | Re-trigger Greptile |
| }); | ||
| }); | ||
|
|
||
| describe("initial check on startup", () => { | ||
| it("runs initial check on first ever launch (no stored version)", async () => { | ||
| mockUpdatesStore._value = null; | ||
| mockAppMeta.version = "1.0.0"; | ||
|
|
||
| await initializeService(service); | ||
|
|
||
| expect(mockUpdater.check).toHaveBeenCalled(); | ||
| expect(mockUpdatesStore.set).toHaveBeenCalledWith( | ||
| "lastLaunchedVersion", | ||
| "1.0.0", | ||
| ); | ||
| }); | ||
|
|
||
| it("runs initial check when relaunching the same version", async () => { | ||
| mockUpdatesStore._value = "1.0.0"; | ||
| mockAppMeta.version = "1.0.0"; | ||
|
|
||
| await initializeService(service); | ||
|
|
||
| expect(mockUpdater.check).toHaveBeenCalled(); | ||
| // No need to rewrite the same value | ||
| expect(mockUpdatesStore.set).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("skips initial check after a version change (post-update restart)", async () => { | ||
| mockUpdatesStore._value = "1.0.0"; | ||
| mockAppMeta.version = "1.0.1"; | ||
|
|
||
| await initializeService(service); | ||
|
|
||
| expect(mockUpdater.check).not.toHaveBeenCalled(); | ||
| expect(mockUpdatesStore.set).toHaveBeenCalledWith( | ||
| "lastLaunchedVersion", | ||
| "1.0.1", | ||
| ); | ||
| }); | ||
|
|
||
| it("still schedules the periodic check after skipping the initial one", async () => { | ||
| mockUpdatesStore._value = "1.0.0"; | ||
| mockAppMeta.version = "1.0.1"; | ||
|
|
||
| await initializeService(service); | ||
| expect(mockUpdater.check).not.toHaveBeenCalled(); | ||
|
|
||
| // Advance past the 24h interval | ||
| await vi.advanceTimersByTimeAsync(24 * 60 * 60 * 1000); | ||
| expect(mockUpdater.check).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("feedUrl", () => { | ||
| it("constructs correct feed URL with platform, arch, and version", async () => { |
There was a problem hiding this comment.
The three scenario tests in "initial check on startup" share an identical structure — set _value, set version, call initializeService, assert on check and set — and should be collapsed into a single it.each table per the team's testing conventions.
it.each([
{
desc: "first ever launch (no stored version)",
storedVersion: null,
appVersion: "1.0.0",
expectCheck: true,
expectSet: "1.0.0",
},
{
desc: "relaunching the same version",
storedVersion: "1.0.0",
appVersion: "1.0.0",
expectCheck: true,
expectSet: null,
},
{
desc: "post-update restart (version changed)",
storedVersion: "1.0.0",
appVersion: "1.0.1",
expectCheck: false,
expectSet: "1.0.1",
},
])("$desc", async ({ storedVersion, appVersion, expectCheck, expectSet }) => {
mockUpdatesStore._value = storedVersion;
mockAppMeta.version = appVersion;
await initializeService(service);
expectCheck
? expect(mockUpdater.check).toHaveBeenCalled()
: expect(mockUpdater.check).not.toHaveBeenCalled();
if (expectSet) {
expect(mockUpdatesStore.set).toHaveBeenCalledWith("lastLaunchedVersion", expectSet);
} else {
expect(mockUpdatesStore.set).not.toHaveBeenCalled();
}
});Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/updates/service.test.ts
Line: 285-340
Comment:
**Prefer parameterised tests**
The three scenario tests in `"initial check on startup"` share an identical structure — set `_value`, set `version`, call `initializeService`, assert on `check` and `set` — and should be collapsed into a single `it.each` table per the team's testing conventions.
```typescript
it.each([
{
desc: "first ever launch (no stored version)",
storedVersion: null,
appVersion: "1.0.0",
expectCheck: true,
expectSet: "1.0.0",
},
{
desc: "relaunching the same version",
storedVersion: "1.0.0",
appVersion: "1.0.0",
expectCheck: true,
expectSet: null,
},
{
desc: "post-update restart (version changed)",
storedVersion: "1.0.0",
appVersion: "1.0.1",
expectCheck: false,
expectSet: "1.0.1",
},
])("$desc", async ({ storedVersion, appVersion, expectCheck, expectSet }) => {
mockUpdatesStore._value = storedVersion;
mockAppMeta.version = appVersion;
await initializeService(service);
expectCheck
? expect(mockUpdater.check).toHaveBeenCalled()
: expect(mockUpdater.check).not.toHaveBeenCalled();
if (expectSet) {
expect(mockUpdatesStore.set).toHaveBeenCalledWith("lastLaunchedVersion", expectSet);
} else {
expect(mockUpdatesStore.set).not.toHaveBeenCalled();
}
});
```
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
071912e to
7a4c9bd
Compare
|
Closing in favor of #1938 |

Problem
we ship too fast 🤪
when i click 'restart' on an update, it's almost guaranteed that there is already another update ready as soon as the app restarts
Changes
tracks last-launched version and does not check for updates if they differ, so that restarting after an update doesn't immediately give you another one, and it just falls back to the 24hr cadence
How did you test this?
unit tests