Skip to content

Commit 905103c

Browse files
authored
feat: When asset is deleted or uploaded - refresh asset resource (#5568)
## Description When user is using asset resource and binding it on canvas, for example video file bound to video tag to the src attribute and then deletes the asset, it is expected that canvas reflects the deleted asset as well as the variable. ## Steps for reproduction 1. click button 2. expect xyz ## Code Review - [ ] hi @kof, I need you to do - conceptual review (architecture, feature-correctness) - detailed review (read every line) - test it on preview ## Before requesting a review - [ ] made a self-review - [ ] added inline comments where things may be not obvious (the "why", not "what") ## Before merging - [ ] tested locally and on preview environment (preview dev login: 0000) - [ ] updated [test cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md) document - [ ] added tests - [ ] if any new env variables are added, added them to `.env` file
1 parent 33f720f commit 905103c

File tree

9 files changed

+508
-27
lines changed

9 files changed

+508
-27
lines changed

.github/copilot-instructions.md

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,21 +39,93 @@
3939

4040
## Testing
4141

42-
- Test utilities, not components
4342
- Every bug fix must have a test to prevent regression
43+
- Write tests for pure functions - avoid mocking whenever possible
44+
- Extract testable pure logic from side-effect code (I/O, network, DOM, React hooks)
45+
- **CRITICAL: Never define functions in test files that belong in implementation code**
46+
- If a test requires a pure function, that function must be exported from the implementation and imported in the test
4447

45-
### Exporting Utilities for Testing
48+
### When to Test
4649

47-
When testing internal utilities, export via `__testing__`:
50+
**DO test:**
51+
52+
- Complex business logic with edge cases
53+
- Pure functions with non-trivial computation (algorithms, parsing, calculations)
54+
- Functions with multiple branches or conditions
55+
- Bug fixes (regression prevention)
56+
57+
**DON'T test:**
58+
59+
- Trivial wrappers around library/language features (e.g., `map.delete()`)
60+
- Simple data transformations with no logic
61+
- Coordination code that only orchestrates side effects
62+
- Direct framework integrations (DOM/React/hooks)
63+
64+
### Writing Testable Code
65+
66+
**WRONG** - Never define implementation logic in tests:
4867

4968
```typescript
50-
export const __testing__ = { internalUtility };
69+
// ❌ BAD: Function defined in test file
70+
const computeKey = (props: { assetId?: string }) => {
71+
return props.assetId ?? "default";
72+
};
73+
74+
test("computes key", () => {
75+
expect(computeKey({ assetId: "123" })).toBe("123");
76+
});
5177
```
5278

53-
Import in tests:
79+
**CORRECT** - Extract to implementation, export, and test:
5480

5581
```typescript
56-
const { internalUtility } = __testing__;
82+
// implementation.ts
83+
export const computeKey = (props: {
84+
assetId?: string;
85+
src?: string;
86+
defaultValue?: unknown;
87+
}) => {
88+
return (
89+
props.assetId?.toString() ??
90+
(props.defaultValue != null ? String(props.defaultValue) : undefined) ??
91+
(props.src != null ? String(props.src) : undefined)
92+
);
93+
};
94+
95+
// implementation.test.ts
96+
import { computeKey } from "./implementation";
97+
98+
test("computes key", () => {
99+
expect(computeKey({ assetId: "123" })).toBe("123");
100+
});
101+
```
102+
103+
This ensures:
104+
105+
- Implementation code is reusable and in the right place
106+
- Tests verify actual production code, not duplicates
107+
- No logic duplication between implementation and tests
108+
109+
### Exporting for Tests
110+
111+
**If the function is ALREADY used in production code:**
112+
113+
- Use regular export: `export const utilityFn = ...`
114+
- The function has real production value beyond testing
115+
116+
**If the function is ONLY needed for tests (internal utility not used elsewhere):**
117+
118+
- Use `__testing__` export to signal it's for testing only:
119+
120+
```typescript
121+
// implementation.ts
122+
const internalHelper = (x: number) => x * 2;
123+
124+
export const __testing__ = { internalHelper };
125+
126+
// implementation.test.ts
127+
import { __testing__ } from "./implementation";
128+
const { internalHelper } = __testing__;
57129
```
58130

59131
Don't create separate utility files just for testing.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
11
import type { Asset } from "@webstudio-is/sdk";
22
import { $assets } from "~/shared/nano-states";
33
import { serverSyncStore } from "~/shared/sync/sync-stores";
4+
import { onNextTransactionComplete } from "~/shared/sync/project-queue";
5+
import { invalidateAssets } from "~/shared/resources";
46

57
export const deleteAssets = (assetIds: Asset["id"][]) => {
68
serverSyncStore.createTransaction([$assets], (assets) => {
79
for (const assetId of assetIds) {
810
assets.delete(assetId);
911
}
1012
});
13+
14+
// Wait for server to confirm transaction, then invalidate cache
15+
onNextTransactionComplete(() => {
16+
invalidateAssets();
17+
});
1118
};

apps/builder/app/builder/shared/assets/upload-assets.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import {
1616
type UploadingFileData,
1717
} from "~/shared/nano-states";
1818
import { serverSyncStore } from "~/shared/sync/sync-stores";
19+
import { onNextTransactionComplete } from "~/shared/sync/project-queue";
20+
import { invalidateAssets } from "~/shared/resources";
1921
import {
2022
formatAssetName,
2123
getFileName,
@@ -38,6 +40,10 @@ const safeDeleteAssets = (assetIds: Asset["id"][], projectId: string) => {
3840
assets.delete(assetId);
3941
}
4042
});
43+
44+
onNextTransactionComplete(() => {
45+
invalidateAssets();
46+
});
4147
};
4248

4349
const safeSetAsset = (asset: Asset, projectId: string) => {
@@ -52,6 +58,10 @@ const safeSetAsset = (asset: Asset, projectId: string) => {
5258
serverSyncStore.createTransaction([$assets], (assets) => {
5359
assets.set(asset.id, asset);
5460
});
61+
62+
onNextTransactionComplete(() => {
63+
invalidateAssets();
64+
});
5565
};
5666

5767
const getFilesData = async <T extends File | URL>(
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { describe, test, expect } from "vitest";
5+
import { __testing__ } from "./webstudio-component";
6+
7+
const { computeComponentKey } = __testing__;
8+
9+
describe("computeComponentKey - key generation logic", () => {
10+
test("prioritizes assetId over other props", () => {
11+
expect(
12+
computeComponentKey({
13+
$webstudio$canvasOnly$assetId: "asset-123",
14+
src: "/image.jpg",
15+
defaultValue: "default",
16+
})
17+
).toBe("asset-123");
18+
});
19+
20+
test("falls back to defaultValue when no assetId", () => {
21+
expect(
22+
computeComponentKey({
23+
src: "/image.jpg",
24+
defaultValue: "default-value",
25+
})
26+
).toBe("default-value");
27+
});
28+
29+
test("uses src when no assetId or defaultValue", () => {
30+
expect(
31+
computeComponentKey({
32+
src: "/path/to/video.mp4",
33+
})
34+
).toBe("/path/to/video.mp4");
35+
});
36+
37+
test("returns undefined when no relevant props", () => {
38+
expect(computeComponentKey({})).toBeUndefined();
39+
});
40+
41+
test("handles null and undefined values", () => {
42+
expect(
43+
computeComponentKey({
44+
src: null,
45+
defaultValue: undefined,
46+
})
47+
).toBeUndefined();
48+
});
49+
50+
test("coerces defaultValue to string", () => {
51+
expect(computeComponentKey({ defaultValue: 42 })).toBe("42");
52+
expect(computeComponentKey({ defaultValue: true })).toBe("true");
53+
expect(computeComponentKey({ defaultValue: 0 })).toBe("0");
54+
});
55+
56+
test("coerces src to string", () => {
57+
expect(computeComponentKey({ src: "string-src" })).toBe("string-src");
58+
expect(computeComponentKey({ src: 123 })).toBe("123");
59+
expect(computeComponentKey({ src: undefined })).toBeUndefined();
60+
});
61+
62+
test("different assetIds produce different keys", () => {
63+
const key1 = computeComponentKey({
64+
$webstudio$canvasOnly$assetId: "asset-123",
65+
src: "/image.jpg",
66+
});
67+
const key2 = computeComponentKey({
68+
$webstudio$canvasOnly$assetId: "asset-456",
69+
src: "/image.jpg",
70+
});
71+
72+
expect(key1).not.toBe(key2);
73+
});
74+
75+
test("different src values produce different keys", () => {
76+
const key1 = computeComponentKey({ src: "/assets/video1.mp4" });
77+
const key2 = computeComponentKey({ src: "/assets/video2.mp4" });
78+
79+
expect(key1).not.toBe(key2);
80+
});
81+
});

apps/builder/app/canvas/features/webstudio-component/webstudio-component.tsx

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,21 @@ import {
4141
} from "@webstudio-is/react-sdk";
4242
import { rawTheme } from "@webstudio-is/design-system";
4343
import { Input, Select, Textarea } from "@webstudio-is/sdk-components-react";
44+
45+
const computeComponentKey = (props: Record<string, unknown>) => {
46+
const assetId = props.$webstudio$canvasOnly$assetId;
47+
const src = props.src;
48+
const defaultValue = props.defaultValue;
49+
50+
return (
51+
(typeof assetId === "string" ? assetId : undefined) ??
52+
(defaultValue != null ? String(defaultValue) : undefined) ??
53+
(src != null ? String(src) : undefined)
54+
);
55+
};
56+
57+
export const __testing__ = { computeComponentKey };
58+
4459
import {
4560
$propValuesByInstanceSelectorWithMemoryProps,
4661
getIndexedInstanceId,
@@ -557,8 +572,9 @@ export const WebstudioComponentCanvas = forwardRef<
557572

558573
// React ignores defaultValue changes after first render.
559574
// Key prop forces re-creation to reflect updates on canvas.
560-
const key =
561-
props.defaultValue != null ? props.defaultValue.toString() : undefined;
575+
// Also use assetId to recreate component when asset changes (e.g., deleted, replaced)
576+
// For expressions that resolve to asset URLs (via assets resource), use the src value itself
577+
const key = computeComponentKey(props);
562578

563579
const instanceElement = (
564580
<>
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import hash from "@emotion/hash";
2+
import type { ResourceRequest } from "@webstudio-is/sdk";
3+
4+
export const getResourceKey = (resource: ResourceRequest) => {
5+
try {
6+
return hash(
7+
JSON.stringify([
8+
// explicitly list all fields to keep hash stable
9+
resource.name,
10+
resource.method,
11+
resource.url,
12+
resource.searchParams,
13+
resource.headers,
14+
resource.body,
15+
])
16+
);
17+
} catch {
18+
// guard from invalid resources
19+
return "";
20+
}
21+
};

0 commit comments

Comments
 (0)