test(ui): cover fontAssets service (API, hooks, FontFace loading)#439
Conversation
Expand ui/src/services/fontAssets.test.ts beyond the existing
fontFamilyForAsset checks to cover the rest of the public surface:
- listFontAssets / uploadFontAsset / deleteFontAsset: URL + method +
credentials + body assertions, 4xx error propagation, network reject,
409 conflict reuse-by-sanitized-id path, and 409-without-match throw.
- loadFontAssets: FontFace family/URL/weight wiring, percent-encoding
of paths with spaces, per-asset failures swallowed via allSettled,
and per-path caching across repeat calls.
- React Query hooks: useFontAssets loading/success/error/disabled, and
useUploadFontAsset / useDeleteFontAsset invalidate ['fontAssets'] on
success.
Mirrors the vi.mock('./base') + global.fetch = vi.fn() style already
used by converter.test.ts / marketplace.test.ts / permissions.test.ts.
Phase 2 coverage sprint.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
==========================================
+ Coverage 62.90% 63.05% +0.14%
==========================================
Files 215 215
Lines 55435 55435
Branches 1597 1597
==========================================
+ Hits 34874 34955 +81
+ Misses 20555 20474 -81
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| }); | ||
| }); | ||
|
|
||
| // ── loadFontAssets ────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🟡 Added section-divider comment violates repository comment rules
AGENTS.md explicitly forbids section-divider comments and says to use blank lines or code structure instead. The added loadFontAssets section header is a pure divider/comment label; the surrounding describe('loadFontAssets', ...) and test names already provide the structure, so this violates the repository's mandatory comment guidelines.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch — fixed in 36a326a. Removed both // ── loadFontAssets ── and // ── React Query hooks ── dividers; the surrounding describe blocks already provide the structure.
| }); | ||
| }); | ||
|
|
||
| // ── React Query hooks ─────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🟡 Added React Query section divider violates repository comment rules
AGENTS.md explicitly forbids section-divider comments and says to use blank lines or code structure instead. The added React Query hooks header is a pure divider/comment label; the following wrapper and describe('useFontAssets / useUploadFontAsset / useDeleteFontAsset', ...) already provide the structure, so this violates the repository's mandatory comment guidelines.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| it('on 409 conflict, refetches the list and reuses the asset matching the sanitized filename', async () => { | ||
| const existing: FontAsset = { | ||
| ...SYSTEM_ASSET, | ||
| id: 'My_Font.ttf', | ||
| name: 'My Font', | ||
| path: 'samples/fonts/user/My_Font.ttf', | ||
| is_system: false, | ||
| }; | ||
| fetchMock() | ||
| .mockResolvedValueOnce(mockResponse({ ok: false, status: 409 })) | ||
| .mockResolvedValueOnce(mockResponse({ ok: true, status: 200, json: [existing] })); | ||
|
|
||
| const file = new File(['data'], 'My Font.ttf', { type: 'font/ttf' }); | ||
| const result = await uploadFontAsset(file); | ||
|
|
||
| expect(result).toEqual(existing); | ||
| expect(fetchMock()).toHaveBeenCalledTimes(2); | ||
| expect(fetchMock().mock.calls[1][0]).toBe('http://localhost:4545/api/v1/assets/fonts'); | ||
| }); |
There was a problem hiding this comment.
📝 Info: 409 conflict test documents intentional client-side reuse behavior
The new test around upload conflicts verifies an existing non-obvious behavior in uploadFontAsset: a 409 response causes the client to refetch listFontAssets() and return the asset whose id equals the locally sanitized filename (ui/src/services/fontAssets.ts:92-100). I did not flag this as a bug because the test matches the implementation and the affected UI caller treats the mutation result as a successful upload before invalidating ['fontAssets'], so the behavior appears intentional rather than an accidental swallowing of conflicts.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Agreed — the 409 reuse-by-sanitized-id path is intentional behavior of uploadFontAsset (fontAssets.ts:92-100) and the test pins the public contract. Not a bug; just locking it down so a future refactor can't silently break the reuse fallback.
| it('caches loaded fonts so repeated calls for the same path do not re-register', async () => { | ||
| const load = vi.fn().mockResolvedValue(undefined); | ||
| installFontFaceStub(load); | ||
|
|
||
| const asset: FontAsset = { | ||
| ...SYSTEM_ASSET, | ||
| id: 'load4-Once.ttf', | ||
| path: 'load4/system/Once.ttf', | ||
| }; | ||
| await loadFontAssets([asset]); | ||
| expect(load).toHaveBeenCalledTimes(1); | ||
|
|
||
| await loadFontAssets([asset]); | ||
| expect(load).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
📝 Info: FontFace cache test relies on module-level state persisting across tests
The new caching test exercises the module-level loadedFonts set in fontAssets.ts; that set is not exported or reset between tests, so the test suite avoids cross-test interference by using unique synthetic path prefixes (load1, load2, etc.) for each loadFontAssets case. This is worth preserving if more tests are added, because reusing a previously loaded path would skip FontFace registration and make assertions depend on test order.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Confirmed — loadedFonts is a module-private Set<string> with no reset hook, so the suite uses load1/ … load4/ path prefixes to keep each loadFontAssets test independent of order. Called this out in the PR body's "Notes" section so future additions follow the same convention.
| beforeEach(() => { | ||
| global.fetch = vi.fn() as never; | ||
| queryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { retry: false, gcTime: 0 }, | ||
| mutations: { retry: false }, | ||
| }, | ||
| }); | ||
| wrapper = makeWrapper(queryClient); |
There was a problem hiding this comment.
📝 Info: React Query hook tests use a real QueryClient with retries disabled
The hook tests create a dedicated QueryClient with queries.retry and mutations.retry disabled before rendering hooks, which is important because otherwise failed fetch assertions could observe retry-driven extra calls or delayed error states. This matches the service hooks' actual query key and invalidation contract in fontAssets.ts:146-179, so I did not identify a cache-key or invalidation bug in the added tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Devin Review flagged the // \u2500\u2500 loadFontAssets \u2500\u2500 and // \u2500\u2500 React Query hooks \u2500\u2500 lines as violations of the repo comment guidelines (AGENTS.md: 'Section dividers \u2014 `// --- Public Modules ---`, `// State`, `// Handlers`. Use blank lines or code structure instead.'). The surrounding describe blocks and test names already provide structure. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Phase 2 coverage sprint — expand
ui/src/services/fontAssets.test.tsbeyond the existing fivefontFamilyForAssetcases to cover the rest of the public surface ofui/src/services/fontAssets.ts(previously 59 uncovered lines, 7.8% covered, no#[cfg(test)]-equivalent block for the API / hooks /loadFontAssets).Behaviors covered (file → describe block):
fontFamilyForAsset— derivessk-…family from a path, strips only the final extension, preserves-Boldsuffixes, handles bare filenames, paths with no extension, and multi-dot filenames.listFontAssets— GETs/api/v1/assets/fontswithcredentials: 'include', returns the parsed JSON array, throws includingstatusTexton 4xx, and surfaces network errors (fetchrejection).uploadFontAsset— POSTs aFormDatawith the file under thefilekey to the correct URL, throws including the server error text on 413, on 409 re-fetches the list and reuses the asset whoseidmatches the sanitized filename (e.g.My Font.ttf→My_Font.ttf), and on 409 with no match throwsFont asset already exists: ….deleteFontAsset— DELETEs/api/v1/assets/fonts/<encoded id>(asserts percent-encoding viaMy Font.ttf→My%20Font.ttf) withcredentials: 'include', and throws including the server error text on 404.loadFontAssets— registers each asset via theFontFaceconstructor with the rightsk-…family, the righturl(/api/v1/assets/fonts/file/<scope>/<filename>)source (with percent-encoding of spaces in the path), and the right weight ('400'vs'700'for-Boldids); callsdocument.fonts.add(face)afterload()resolves; swallows per-assetload()failures viaPromise.allSettledso the overall load still resolves; and caches byasset.pathso repeat calls do not re-register the same path.useFontAssets/useUploadFontAsset/useDeleteFontAsset—useFontAssetsresolves to the listed assets, surfaces fetch errors as an error state, and staysidle(nofetchcall) whenenabled=false; both mutation hooks call their underlying function and invalidate the['fontAssets']query key on success (asserted via avi.spyOn(queryClient, 'invalidateQueries')).Style mirrors the existing service tests (
converter.test.ts,marketplace.test.ts,permissions.test.ts):vi.mock('./base', …)forwardingfetchApitoglobal.fetch = vi.fn(), plus a smallmockResponse({...})helper for theok/status/statusText/json/textshape.loadFontAssetsis exercised with aFontFaceclass stub +Object.defineProperty(document, 'fonts', …)(happy-dom has no nativeFontFace/document.fonts).Files inspected for style:
ui/src/services/converter.test.tsui/src/services/marketplace.test.tsui/src/services/permissions.test.tsui/src/services/base.tsui/src/hooks/useEdgeAlertSubscription.test.ts(renderHook +@testing-library/reactpatterns)ui/src/test/setup.tsCommands run:
just lint-ui— passes (only pre-existing warnings inAssetLibrary.tsx,ConfigurableNode.tsx,InspectorPane.tsx,DesignView.tsx,StreamView.tsx; no new warnings or errors).bun run test:run src/services/fontAssets.test.ts— 24/24 pass.bun run test:run(full UI suite) — 662/662 pass across 44 files.Review & Testing Checklist for Human
Risk: green — tests-only PR, no production code touched.
bun run test:run src/services/fontAssets.test.tspasses locally (24 tests).just lint-uiintroduces no new warnings.file.name.replace(/[^a-zA-Z0-9._-]/g, '_')must match an existingasset.id).Notes
fontServeUrlis exercised indirectly throughloadFontAssets(it is module-private and not exported); the assertions on theFontFacesourceargument verify the URL shape and percent-encoding without reaching into the private symbol.loadedFonts: Set<string>is shared across tests; eachloadFontAssetstest uses a unique path prefix (load1/,load2/,load3/,load4/) so test order does not matter.Follow-ups / observations (no real bugs found):
loadFontFaceusesasset.id.includes('-Bold') || asset.id.includes('Bold')— the second branch is a strict superset of the first, so the first is dead. Behaviour is unchanged (anything containingBoldis bold), so I did not test the two branches separately; flagging here for the coordinator in case it's worth simplifying.enabledboolean;useFontAssets(false)correctly leavesfetchStatus === 'idle'and never callsfetch. No bug.Link to Devin session: https://staging.itsdev.in/sessions/f21c1037d23e40bea6dc08bfcdca2b30
Requested by: @streamer45
Devin Review
b1c7fb9(HEAD is36a326a)