Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions apps/code/src/main/services/updates/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
mockAppMeta,
mockMainWindow,
mockLifecycleService,
mockUpdatesStore,
updaterHandlers,
} = vi.hoisted(() => {
const updaterHandlers: {
Expand Down Expand Up @@ -80,6 +81,13 @@ const {
shutdownWithoutContainer: vi.fn(() => Promise.resolve()),
setQuittingForUpdate: vi.fn(),
},
mockUpdatesStore: {
_value: null as string | null,
get: vi.fn((_key: string) => mockUpdatesStore._value),
set: vi.fn((_key: string, value: string) => {
mockUpdatesStore._value = value;
}),
},
};
});

Expand All @@ -98,6 +106,10 @@ vi.mock("../../utils/env.js", () => ({
isDevBuild: () => !mockAppMeta.isProduction,
}));

vi.mock("../../utils/store.js", () => ({
updatesStore: mockUpdatesStore,
}));

// Import the service after mocks are set up
import { UpdatesService } from "./service";

Expand Down Expand Up @@ -145,6 +157,9 @@ describe("UpdatesService", () => {
// Clear env flag
delete process.env.ELECTRON_DISABLE_AUTO_UPDATE;

// Reset persisted updates store
mockUpdatesStore._value = null;

service = new UpdatesService();
injectPorts(service);
});
Expand Down Expand Up @@ -270,6 +285,67 @@ describe("UpdatesService", () => {
});
});

describe("initial check on startup", () => {
it.each([
{
desc: "runs initial check on first ever launch (no stored version)",
storedVersion: null,
appVersion: "1.0.0",
expectCheck: true,
expectSet: "1.0.0" as string | null,
},
{
desc: "runs initial check when relaunching the same version",
storedVersion: "1.0.0",
appVersion: "1.0.0",
expectCheck: true,
expectSet: null,
},
{
desc: "skips initial check after a version change (post-update restart)",
storedVersion: "1.0.0",
appVersion: "1.0.1",
expectCheck: false,
expectSet: "1.0.1" as string | null,
},
])(
"$desc",
async ({ storedVersion, appVersion, expectCheck, expectSet }) => {
mockUpdatesStore._value = storedVersion;
mockAppMeta.version = appVersion;

await initializeService(service);

if (expectCheck) {
expect(mockUpdater.check).toHaveBeenCalled();
} else {
expect(mockUpdater.check).not.toHaveBeenCalled();
}

if (expectSet !== null) {
expect(mockUpdatesStore.set).toHaveBeenCalledWith(
"lastLaunchedVersion",
expectSet,
);
} else {
expect(mockUpdatesStore.set).not.toHaveBeenCalled();
}
},
);

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 () => {
Comment on lines 285 to 350
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

Object.defineProperty(process, "arch", {
Expand Down
24 changes: 22 additions & 2 deletions apps/code/src/main/services/updates/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { inject, injectable, postConstruct, preDestroy } from "inversify";
import { MAIN_TOKENS } from "../../di/tokens";
import { isDevBuild } from "../../utils/env";
import { logger } from "../../utils/logger";
import { updatesStore } from "../../utils/store";
import { TypedEventEmitter } from "../../utils/typed-event-emitter";
import type { AppLifecycleService } from "../app-lifecycle/service";
import {
Expand Down Expand Up @@ -191,8 +192,27 @@ export class UpdatesService extends TypedEventEmitter<UpdatesEvents> {
),
);

// Perform initial check (periodic source — not user-initiated)
this.checkForUpdates("periodic");
// Skip the initial check if the app version changed since last launch —
// that means we just restarted to apply an update, and re-checking
// immediately tends to find another release that shipped in the meantime.
const lastLaunchedVersion = updatesStore.get("lastLaunchedVersion");
const currentVersion = this.appMeta.version;
const isPostUpdateRestart =
lastLaunchedVersion !== null && lastLaunchedVersion !== currentVersion;

if (isPostUpdateRestart) {
log.info("Skipping initial update check after version change", {
previousVersion: lastLaunchedVersion,
currentVersion,
});
} else {
// Perform initial check (periodic source — not user-initiated)
this.checkForUpdates("periodic");
}

if (lastLaunchedVersion !== currentVersion) {
updatesStore.set("lastLaunchedVersion", currentVersion);
}

// Set up periodic checks
this.checkIntervalId = setInterval(
Expand Down
12 changes: 12 additions & 0 deletions apps/code/src/main/utils/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export interface WindowStateSchema {
isMaximized: boolean;
}

interface UpdatesStoreSchema {
lastLaunchedVersion: string | null;
}

const userDataDir = getUserDataDir();

export const rendererStore = new Store<RendererStoreSchema>({
Expand All @@ -52,3 +56,11 @@ export const windowStateStore = new Store<WindowStateSchema>({
isMaximized: true,
},
});

export const updatesStore = new Store<UpdatesStoreSchema>({
name: "updates",
cwd: userDataDir,
defaults: {
lastLaunchedVersion: null,
},
});
Loading