Skip to content

Commit eb02975

Browse files
zsviczianclaude
andcommitted
Fix stale-image retry no-op and ExcalidrawDataScene any-collapse
getFiles() called Object.values() on a Map, which always returns [] - the only caller (ViewSceneFileManager's stale-image retry loop) has likely never actually retried a failed-to-load file. Fixed with Array.from(...values()). ExcalidrawDataScene intersected a base type that already declared elements/appState, so TypeScript merged rather than overrode them, collapsing this.scene.elements to `any` on every direct .filter() call and cascading into dozens of no-unsafe-* findings. Fixed by Omitting the colliding keys from the base type before intersecting; type-only, no runtime behavior change. Two previously-invisible narrowing gaps this uncovered were fixed with the same `as Mutable<ExcalidrawTextElement>[]` idiom already used elsewhere in the file. Confirmed via a git-stash ESLint diff that this introduces zero new findings anywhere in the repo (470 -> 413, with ExcalidrawAutomate.ts and ExcalidrawView.ts also improving as a side effect). Both fixes manually tested and confirmed working. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 6b2b259 commit eb02975

3 files changed

Lines changed: 116 additions & 11 deletions

File tree

RefactorPlan.md

Lines changed: 99 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,14 @@ backlog is 470 `@typescript-eslint/no-unsafe-*` findings (was 514 before the
11571157
fix below), concentrated in `ExcalidrawView.ts` (145), `ExcalidrawData.ts`
11581158
(64, was 108), `ExcalidrawAutomate.ts` (77), and `AIUtils.ts` (42).
11591159

1160+
**Risk policy for this effort:** the general rule for *all* `any`-replacement
1161+
work in this repo — not restated here, read it there — is `AGENTS.md`'s
1162+
"CRITICAL: Behavioral Change Detection When Replacing `any`" section. It
1163+
covers falsy/truthy checks, existence checks, and optional-chaining
1164+
fallbacks changing meaning once a real type replaces `any`, with a mandatory
1165+
pre-change verification checklist. Every fix logged below was screened
1166+
against that checklist before being applied.
1167+
11601168
**Done:** fixed `ExcalidrawData.ts`'s cluster at its root cause. Traced it to
11611169
8 `let parts;` / `let res;` declarations with no initializer and no type
11621170
annotation (`getDecompressedScene`, `getJSON`, the text-element/element-link
@@ -1202,17 +1210,103 @@ same-shape follow-up to the fix above:
12021210
a mechanical fix.
12031211
- Conclusion: doesn't meet the same low-risk bar. Left entirely untouched.
12041212

1213+
**Done (2026-08-14): fixed the `ExcalidrawDataScene.elements`/`appState`
1214+
intersection-collapse root cause.** Triaged the remaining 64 findings across
1215+
`ExcalidrawData.ts` (63) and the newly-extracted `EmbeddedDataRegistries.ts`
1216+
(1) into three buckets before touching anything:
1217+
1218+
1. **A real bug, not a typing artifact.** `EmbeddedDataRegistries.getFiles()`
1219+
did `Object.values(this.host.files)` where `files` is a `Map`.
1220+
`Object.values()` on a `Map` instance always returns `[]` (verified with
1221+
a plain Node check — this is standard JS, not a TS quirk: a `Map`'s data
1222+
lives in internal slots, not enumerable own properties). The only real
1223+
caller, `ViewSceneFileManager.ts`'s stale-image retry loop ("in case one
1224+
or more files have not loaded retry later"), has therefore likely never
1225+
actually retried anything since it was written. Fixed with
1226+
`Array.from(this.host.files.values())` — a genuine, intentional runtime
1227+
behavior change (the point of the fix), not a type-only edit. Searched
1228+
every `Map`-typed field in `src/` (`files`/`filesMaster`/`equations`/
1229+
`equationsMaster`/`markdownImages`/`markdownImagesMaster`/`mermaids`/
1230+
`mermaidsMaster`/`elementLinks`/`buttons`/`colorsCache`/`packageMap`/
1231+
`pageDimensionsByPage`/`pdfDocsMap`) against every `Object.keys/values/
1232+
entries` call site in the codebase — this was the only instance of the
1233+
pattern; every other `files`-named argument passed to `Object.values`
1234+
elsewhere is genuinely `BinaryFiles` (Excalidraw's own `Record` type),
1235+
confirmed per call site, not a `Map`.
1236+
2. **One dominant root cause behind most of the rest — type-definition-only,
1237+
no runtime behavior change.** `ExcalidrawDataScene` was declared as
1238+
`SceneDataWithFiles & { elements: Mutable<ExcalidrawElement>[]; appState:
1239+
...; ... }`. `SceneDataWithFiles` (via the upstream `SceneData` type)
1240+
*already* declares `elements` and `appState`. TypeScript doesn't let the
1241+
second declaration override the first inside an intersection — it
1242+
intersects both property types. Verified empirically with a throwaway
1243+
`@ts-expect-error`-shaped probe (added and reverted): the real type of
1244+
`this.scene.elements` was `readonly ExcalidrawElement[] & Mutable<ExcalidrawElement>[]`.
1245+
Calling `.filter()`/similar directly on that (no `?.`, no trailing `as`)
1246+
made TypeScript's overload resolution collapse the call to `any`,
1247+
cascading into every downstream property access. Fixed by wrapping the
1248+
base type in `Omit<SceneDataWithFiles, "elements" | "appState">` so the
1249+
local, stricter declarations actually replace instead of intersect.
1250+
Sampled `ExcalidrawView.ts`/`ExcalidrawAutomate.ts` beforehand and found
1251+
only one non-`.filter()` touch point (`excalidrawData.scene.elements.length`),
1252+
so the blast radius looked contained to `ExcalidrawData.ts` itself before
1253+
attempting the fix — confirmed after the fact via a clean `npm run build`
1254+
(zero new hard type errors anywhere in the dependency graph) plus a
1255+
`git stash`/`pop` full-repo ESLint diff showing zero new findings in any
1256+
file. Two genuine (and expected) knock-on compile errors surfaced in
1257+
`ExcalidrawData.ts` itself — `updateTextElementsFromScene()` and
1258+
`generateMDBase()` both filter `scene.elements` down to elements assumed
1259+
(by the surrounding logic, not the type) to always be text elements, then
1260+
read `.rawText`/`.originalText`/`.text`, which don't exist on the general
1261+
`ExcalidrawElement` union. These were previously invisible because the
1262+
filter result was silently `any`; fixed with the same
1263+
`as Mutable<ExcalidrawTextElement>[]` cast idiom the file already uses in
1264+
`updateSceneTextElements()` and `syncFiles()` for the identical situation
1265+
— no behavior change, just making the existing runtime assumption
1266+
type-visible. Also removed one `as Mutable<ExcalidrawElement>[]` cast in
1267+
`loadData()` that the fix made genuinely redundant
1268+
(`no-unnecessary-type-assertion`).
1269+
3. **Genuine external boundary — confirmed, left untouched.**
1270+
`fileCache.frontmatter[key]` accesses (`getOnLoadScript`/`setLinkPrefix`/
1271+
`setUrlPrefix`/`setAutoexportPreferences`/`setembeddableThemePreference`/
1272+
`getLinkOpacity`, ~13 findings) and `JSON.parse(data)` in
1273+
`loadLegacyData()` (1 finding). Obsidian's own published type is
1274+
`FrontMatterCache { [key: string]: any }` — arbitrary user-authored YAML;
1275+
same category as the `AIUtils.ts` cluster already declined above.
1276+
1277+
**Outcome:** `ExcalidrawData.ts` findings dropped from 63 to 17;
1278+
`EmbeddedDataRegistries.ts` from 1 to 0. The type-definition fix also had
1279+
beneficial ripple effects with zero negative side effects, confirmed via a
1280+
`git stash`/`pop` per-file ESLint diff: `excalidrawAutomateUtils.ts` dropped
1281+
21 → 12 and `ExcalidrawView.ts` dropped 145 → 144, both previously relying
1282+
on the same `any` leak through call sites this session didn't touch
1283+
directly. Whole-repo count: 470 → 413 (57 fewer), matching the sum of all
1284+
per-file deltas exactly — confirming zero new findings anywhere. `npm run
1285+
build`, `npm run lib`, `node --check dist/main.js`, and `git diff --check`
1286+
all passed; the 33-warning circular-dependency baseline is unchanged;
1287+
`dist/main.js` is 4,716,860 bytes (6 bytes above the structural-extraction
1288+
checkpoint, noise). Manual testing pending, prioritizing the two intentional
1289+
behavior changes: the stale-image retry loop (open a drawing with an image
1290+
still mid-sync and confirm it eventually loads without a manual reopen) and
1291+
general text-element/link/back-of-card parsing sanity (the two
1292+
`as Mutable<ExcalidrawTextElement>[]` sites).
1293+
12051294
**If resumed:** look for more `ExcalidrawData.ts`-shaped cases elsewhere in
12061295
the `no-unsafe-*` backlog — specifically, a local variable whose real type
12071296
is already known and already used correctly elsewhere in the same file or
12081297
codebase, just missing on one declaration — rather than external-boundary
12091298
`any` (network responses, `JSON.parse` of untrusted content, etc.), which
12101299
needs the same scrutiny `AIUtils.ts` got before touching. `ExcalidrawView.ts`
1211-
(145 findings) and `ExcalidrawAutomate.ts` (77 findings) are the two
1212-
remaining large clusters and haven't been individually triaged yet — either
1213-
could contain more of the safe kind, but that needs checking file-by-file
1214-
the same way this session did for `ExcalidrawData.ts` and `AIUtils.ts`, not
1215-
assumed from the finding count alone.
1300+
(144 findings) and `ExcalidrawAutomate.ts` (12 findings, down from 77 after
1301+
today's fix) are the two remaining large-ish clusters and haven't been
1302+
individually triaged yet — either could contain more of the safe kind, but
1303+
that needs checking file-by-file the same way this session did for
1304+
`ExcalidrawData.ts` and `AIUtils.ts`, not assumed from the finding count
1305+
alone. `ExcalidrawView.ts`'s cluster is now by far the largest remaining one.
1306+
1307+
**Closed the 2026-08-14 validation checkpoint:** user confirmed both
1308+
targeted manual tests — the stale-image retry loop and general text-
1309+
element/link/back-of-card parsing — succeeded with no issues; committed.
12161310

12171311
## Related, separate effort: `ExcalidrawData.ts` structural extraction
12181312

src/shared/EmbeddedDataRegistries.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export class EmbeddedDataRegistries {
6464
}
6565

6666
public getFiles(): EmbeddedFile[] {
67-
return Object.values(this.host.files);
67+
return Array.from(this.host.files.values());
6868
}
6969

7070
public getFile(fileId: FileId): EmbeddedFile {

src/shared/ExcalidrawData.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,19 @@ type LegacyGridColor = NonNullable<
110110
MajorGridFrequency?: number;
111111
};
112112

113-
type ExcalidrawDataScene = SceneDataWithFiles & {
113+
/**
114+
* `SceneDataWithFiles` (via `SceneData`) already declares optional
115+
* `elements`/`appState` fields. Intersecting it directly would make
116+
* TypeScript intersect those declarations with the stricter ones below
117+
* (e.g. `readonly ExcalidrawElement[] & Mutable<ExcalidrawElement>[]`)
118+
* instead of replacing them, which silently collapses every `.filter()`/
119+
* `.find()` call on `scene.elements` to `any`. Omitting them first makes
120+
* this an actual override.
121+
*/
122+
type ExcalidrawDataScene = Omit<
123+
SceneDataWithFiles,
124+
"elements" | "appState"
125+
> & {
114126
type?: string;
115127
version?: number;
116128
source?: string;
@@ -613,8 +625,7 @@ export class ExcalidrawData {
613625
(el: ExcalidrawElement): el is NonDeletedExcalidrawElement =>
614626
!el.isDeleted,
615627
);
616-
this.scene.elements =
617-
nonDeletedSceneElements as Mutable<ExcalidrawElement>[];
628+
this.scene.elements = nonDeletedSceneElements;
618629

619630
//once off migration of legacy scenes
620631
if (
@@ -1123,7 +1134,7 @@ export class ExcalidrawData {
11231134
//find text element in the scene
11241135
const el = this.scene.elements?.filter(
11251136
(el: ExcalidrawElement) => el.type === "text" && el.id === key,
1126-
);
1137+
) as Mutable<ExcalidrawTextElement>[];
11271138
if (el.length === 0) {
11281139
this.textElements.delete(key); //if no longer in the scene, delete the text element
11291140
} else {
@@ -1376,7 +1387,7 @@ export class ExcalidrawData {
13761387
//https://github.com/zsviczian/obsidian-excalidraw-plugin/issues/566
13771388
const element = this.scene.elements.filter(
13781389
(el: ExcalidrawElement) => el.id === key,
1379-
);
1390+
) as Mutable<ExcalidrawTextElement>[];
13801391
const elementString = this.textElements.get(key).raw;
13811392
if (
13821393
element &&

0 commit comments

Comments
 (0)