Skip to content

Commit 18d7cb5

Browse files
ushkinazushkinaz
committed
fix(routing): stabilize memory management and prevent memory leaks in tests
1 parent 424730a commit 18d7cb5

File tree

5 files changed

+163
-106
lines changed

5 files changed

+163
-106
lines changed

‎src/App.svelte‎

Lines changed: 63 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -162,91 +162,92 @@ let prewarmScheduledFor: CBNData | null = null;
162162
// Initialize routing and fetch builds
163163
const appStart = nowTimeStamp();
164164
const p = mark("app-routing-start");
165-
initializeRouting()
166-
.then((result: InitialAppState) => {
165+
void initializeRouting()
166+
.then(async (result: InitialAppState) => {
167167
builds = result.builds;
168168
resolvedVersion = result.resolvedVersion;
169169
latestStableBuild = result.latestStableBuild;
170170
latestNightlyBuild = result.latestNightlyBuild;
171+
if (result.redirected) {
172+
// initializeRouting already called location.replace() for an invalid URL.
173+
// Stop startup here so we don't start data loading right before navigation.
174+
return;
175+
}
171176
const requestedModsFromUrl = [...$page.route.mods];
172177
173-
data
174-
.setVersion(
178+
try {
179+
await data.setVersion(
175180
resolvedVersion,
176181
localeParam,
177182
isBranchAlias ? requestedVersion : undefined,
178183
builds.find((b) => b.build_number === resolvedVersion)?.langs,
179184
requestedModsFromUrl,
180-
)
181-
.then(() => {
182-
if ($data) {
183-
const resolvedMods = $data.active_mods ?? [];
184-
if (!arraysEqual(requestedModsFromUrl, resolvedMods)) {
185-
const removedMods = requestedModsFromUrl.filter(
186-
(modId) => !resolvedMods.includes(modId),
187-
);
188-
updateQueryParamNoReload(
189-
"mods",
190-
resolvedMods.length > 0 ? resolvedMods.join(",") : null,
191-
);
192-
if (removedMods.length > 0) {
193-
notify(
194-
t("Ignored unknown mods from URL: {mods}.", {
195-
mods: removedMods.join(", "),
196-
_context: URL_MODS_CONTEXT,
197-
}),
198-
"warn",
199-
);
200-
}
201-
}
202-
}
185+
);
186+
} catch (e) {
187+
console.error(e);
188+
notify(
189+
t(
190+
"Failed to load data for {version}. Please select a different version from the footer.",
191+
{ version: resolvedVersion },
192+
),
193+
"error",
194+
);
195+
return;
196+
}
203197
204-
if (
205-
$data &&
206-
$data.requested_locale !== $data.effective_locale &&
207-
$data.requested_locale !== "en"
208-
) {
209-
const build_number = $data.build_number;
210-
const from = getLanguageName($data.requested_locale);
211-
const to = getLanguageName($data.effective_locale);
198+
if ($data) {
199+
const resolvedMods = $data.active_mods ?? [];
200+
if (!arraysEqual(requestedModsFromUrl, resolvedMods)) {
201+
const removedMods = requestedModsFromUrl.filter(
202+
(modId) => !resolvedMods.includes(modId),
203+
);
204+
updateQueryParamNoReload(
205+
"mods",
206+
resolvedMods.length > 0 ? resolvedMods.join(",") : null,
207+
);
208+
if (removedMods.length > 0) {
212209
notify(
213-
t(
214-
"Translation for {requested_lang} missing in {build_number}. Using {resolved_lang}.",
215-
{
216-
build_number: build_number,
217-
requested_lang: from,
218-
resolved_lang: to,
219-
},
220-
),
210+
t("Ignored unknown mods from URL: {mods}.", {
211+
mods: removedMods.join(", "),
212+
_context: URL_MODS_CONTEXT,
213+
}),
221214
"warn",
222215
);
223216
}
224-
metrics.distribution(
225-
"data.load.duration_ms",
226-
nowTimeStamp() - appStart,
217+
}
218+
}
219+
220+
if (
221+
$data &&
222+
$data.requested_locale !== $data.effective_locale &&
223+
$data.requested_locale !== "en"
224+
) {
225+
const build_number = $data.build_number;
226+
const from = getLanguageName($data.requested_locale);
227+
const to = getLanguageName($data.effective_locale);
228+
notify(
229+
t(
230+
"Translation for {requested_lang} missing in {build_number}. Using {resolved_lang}.",
227231
{
228-
unit: "millisecond",
232+
build_number: build_number,
233+
requested_lang: from,
234+
resolved_lang: to,
229235
},
230-
);
231-
})
232-
.catch((e) => {
233-
console.error(e);
234-
notify(
235-
t(
236-
"Failed to load data for {version}. Please select a different version from the footer.",
237-
{ version: resolvedVersion },
238-
),
239-
"error",
240-
);
241-
})
242-
.finally(() => {
243-
p.finish();
244-
});
236+
),
237+
"warn",
238+
);
239+
}
240+
metrics.distribution("data.load.duration_ms", nowTimeStamp() - appStart, {
241+
unit: "millisecond",
242+
});
245243
})
246244
.catch((e) => {
247245
Sentry.captureException(e);
248246
console.error(e);
249247
//TODO: Notify user, we failed to load our app.
248+
})
249+
.finally(() => {
250+
p.finish();
250251
});
251252
252253
const urlConfig = getUrlConfig();

‎src/data.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2897,6 +2897,13 @@ export const data = {
28972897
_currentData = instance;
28982898
set(instance);
28992899
},
2900+
// Test helper: reset singleton store state between app mounts in routing tests.
2901+
_reset() {
2902+
_hasSetVersion = false;
2903+
_currentData = null;
2904+
_ensureModsLoadedPromise = null;
2905+
set(null);
2906+
},
29002907
async ensureModsLoaded() {
29012908
const startData = _currentData;
29022909
if (!startData || startData.mods !== null) return;

‎src/routing.test.ts‎

Lines changed: 55 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import {
2222
} from "vitest";
2323
import * as fs from "fs";
2424
import App from "./App.svelte";
25+
import { data } from "./data";
26+
import { tileData } from "./tile-data";
2527
import { dismiss, notifications } from "./Notification.svelte";
2628

2729
vi.hoisted(() => {
@@ -86,7 +88,9 @@ const testModsData = {
8688

8789
describe("Routing E2E Tests", () => {
8890
vi.setConfig({ testTimeout: 120_000 });
91+
const ROUTING_TEARDOWN_SETTLE_MS = 150;
8992
let originalFetch: typeof global.fetch;
93+
let originalImage: typeof global.Image;
9094
let defaultFetchMock: typeof fetch;
9195
let container: HTMLElement;
9296

@@ -141,8 +145,30 @@ describe("Routing E2E Tests", () => {
141145

142146
beforeAll(() => {
143147
originalFetch = global.fetch;
148+
originalImage = global.Image;
144149
defaultFetchMock = createFetchMock();
145150
global.fetch = defaultFetchMock;
151+
152+
class MockImage {
153+
onload: ((this: GlobalEventHandlers, ev: Event) => any) | null = null;
154+
onerror: OnErrorEventHandler = null;
155+
width = 32;
156+
height = 32;
157+
#src = "";
158+
159+
get src() {
160+
return this.#src;
161+
}
162+
163+
set src(value: string) {
164+
this.#src = value;
165+
queueMicrotask(() => {
166+
this.onload?.call(window, new Event("load"));
167+
});
168+
}
169+
}
170+
171+
global.Image = MockImage as unknown as typeof global.Image;
146172
});
147173

148174
beforeEach(() => {
@@ -180,56 +206,41 @@ describe("Routing E2E Tests", () => {
180206
if (container && container.parentNode) {
181207
container.parentNode.removeChild(container);
182208
}
209+
data._reset();
210+
tileData.reset();
183211
global.fetch = defaultFetchMock;
184212
vi.clearAllMocks();
185213
});
186214

187-
afterAll(() => {
215+
afterAll(async () => {
216+
// Let pending Svelte window-binding timeouts settle before async-leak collection.
217+
await new Promise((resolve) =>
218+
setTimeout(resolve, ROUTING_TEARDOWN_SETTLE_MS),
219+
);
188220
global.fetch = originalFetch;
221+
global.Image = originalImage;
189222
});
190223

191224
async function waitForDataLoad(expectedText?: string | RegExp) {
192-
const start = Date.now();
193-
const timeout = 10000;
225+
await waitFor(() => expect(get(data)).not.toBeNull(), { timeout: 10_000 });
226+
await waitFor(
227+
() =>
228+
expect(
229+
document.querySelector(".loading-container.full-screen"),
230+
).toBeNull(),
231+
{ timeout: 10_000 },
232+
);
194233

195-
while (Date.now() - start < timeout) {
196-
const text = document.body.textContent || "";
197-
const lowerText = text.toLowerCase();
234+
if (!expectedText) return;
198235

199-
if (expectedText) {
200-
if (typeof expectedText === "string") {
201-
if (lowerText.includes(expectedText.toLowerCase())) return;
202-
} else if (expectedText.test(text)) {
203-
return;
204-
}
205-
} else {
206-
const isLoading =
207-
lowerText.includes("loading") &&
208-
(lowerText.includes("game data") || lowerText.includes("builds"));
209-
if (!isLoading) {
210-
if (
211-
lowerText.includes("hitchhiker") ||
212-
lowerText.includes("description") ||
213-
lowerText.includes("results") ||
214-
lowerText.includes("catalog") ||
215-
lowerText.includes("items") ||
216-
lowerText.includes("location") ||
217-
lowerText.includes("monsters") ||
218-
lowerText.includes("mutations") ||
219-
lowerText.includes("rock")
220-
) {
221-
return;
222-
}
223-
}
236+
await waitFor(() => {
237+
const text = document.body.textContent || "";
238+
if (typeof expectedText === "string") {
239+
expect(text.toLowerCase()).toContain(expectedText.toLowerCase());
240+
return;
224241
}
225-
await new Promise((resolve) => setTimeout(resolve, 50));
226-
}
227-
228-
const currentLoc = window.location.pathname;
229-
const bodySlice = document.body.textContent?.slice(0, 1000) || "";
230-
throw new Error(
231-
`Timed out waiting for data load (expected: ${expectedText || "any trigger"}).\nURL: ${currentLoc}\nBody snippet: ${bodySlice}`,
232-
);
242+
expect(expectedText.test(text)).toBe(true);
243+
});
233244
}
234245

235246
async function waitForNavigation() {
@@ -349,6 +360,7 @@ describe("Routing E2E Tests", () => {
349360

350361
// Check query param persists
351362
expect(window.location.search).toContain("lang=ru_RU");
363+
await waitForDataLoad();
352364
});
353365

354366
test("navigateTo preserves mods query param", async () => {
@@ -386,6 +398,7 @@ describe("Routing E2E Tests", () => {
386398
expect(new URL(window.location.href).searchParams.get("mods")).toBe(
387399
"aftershock,magiclysm",
388400
);
401+
await waitForDataLoad();
389402
});
390403

391404
test("loads compatible mod tileset chunk URLs for active mods", async () => {
@@ -770,6 +783,7 @@ describe("Routing E2E Tests", () => {
770783
describe("Version Handling", () => {
771784
test("navigates to correct version with incorrect typed-in URL", async () => {
772785
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
786+
const setVersionSpy = vi.spyOn(data, "setVersion");
773787
const originalReplace = window.location.replace;
774788
let replaceCalled = false;
775789
delete (window.location as any).replace;
@@ -793,8 +807,10 @@ describe("Routing E2E Tests", () => {
793807
// Should have called location.replace to prepend /stable/
794808
expect(replaceCalled).toBe(true);
795809
expect(window.location.pathname).toContain("stable/invalid-version-999");
810+
expect(setVersionSpy).not.toHaveBeenCalled();
796811

797812
warnSpy.mockRestore();
813+
setVersionSpy.mockRestore();
798814
window.location.replace = originalReplace;
799815
});
800816

‎src/routing.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ export type UrlConfig = {
6363
export type InitialAppState = {
6464
builds: BuildInfo[];
6565
resolvedVersion: string;
66+
/**
67+
* True when initializeRouting already triggered location.replace().
68+
* App startup must stop in this case to avoid kicking off redundant data loads
69+
* while the browser is navigating to the corrected URL.
70+
*/
71+
redirected: boolean;
6672
latestStableBuild?: BuildInfo;
6773
latestNightlyBuild?: BuildInfo;
6874
};
@@ -709,6 +715,7 @@ export async function initializeRouting(): Promise<InitialAppState> {
709715
return {
710716
builds,
711717
resolvedVersion,
718+
redirected: true,
712719
latestStableBuild,
713720
latestNightlyBuild,
714721
};
@@ -726,6 +733,7 @@ export async function initializeRouting(): Promise<InitialAppState> {
726733
return {
727734
builds,
728735
resolvedVersion,
736+
redirected: false,
729737
latestStableBuild,
730738
latestNightlyBuild,
731739
};

0 commit comments

Comments
 (0)