Skip to content

Commit 77d7635

Browse files
authored
"cleanup svg icon usage, types and href generation" once more (#13771)
Restoring #13746 Practically, only [5021ed1](5021ed1) is to be reviewed. #### Original description Fixes #13712 Removed remains of svg embedding, as it turned out to not work reliably when combined with shadow-dom (i.e. in visualizations) and the exact behavior is browser dependent. I ended up just unifying all places that reference icon's href into a single function, and added a "missing icon" visual. Unfortunately that means the playwright test snapshots still attempt a potentially cross-origin fetch request to resolve the icons, which is cancelled before hitting playwright's service-worker fetch handler. Allowing cross-origin requests in svg is often possible, but for <use> tags is not yet implemented anywhere. The only workaround that somewhat works is to run the test reporter on the same port as the app under test used to have. We unfortunately use two, so the icons will either show up in dashboard or in project-view tests. Also deduplicated login logic in playwright tests and added extra agreement modal detection, since that modal often used to be a problem for me when running test suite locally. #### The Fix ~~I don't know why, but adding `?no-inline` suffix made it not working. `?inline` suffix didn't work as well. Leaving now without suffix.~~ Edit: There was a bug in vite (see comments below), so bumped vite version with some others to satisfy peer dependencies + applied some necessary changes. Also restored the old way of documenting `SvgIcon` with information why I did it that way.
1 parent 86053a8 commit 77d7635

File tree

26 files changed

+764
-592
lines changed

26 files changed

+764
-592
lines changed

app/common/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
"@internationalized/date": "3.7.0",
3131
"@tanstack/query-persist-client-core": "5.59.20",
3232
"@tanstack/vue-query": "5.59.20",
33-
"@types/node": "^20.11.21",
33+
"@types/node": "catalog:",
3434
"is-network-error": "^1.1.0",
3535
"lib0": "^0.2.99",
3636
"vitest": "catalog:",

app/gui/integration-test/dashboard/actions/index.ts

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
type LocalTrackedCalls,
1515
} from './localApi'
1616
import LoginPageActions from './LoginPageActions'
17-
import { passAgreementsDialog, TEXT, VALID_PASSWORD, type MockParams } from './utilities'
17+
import { passAgreementsDialog, TEXT, type MockParams } from './utilities'
1818
export * from './utilities'
1919

2020
export const getText = (key: TextId, ...replacements: Replacements[TextId]) => {
@@ -28,35 +28,23 @@ export function getAuthFilePath() {
2828
}
2929

3030
/** Perform a successful login. */
31-
async function login({ page }: MockParams, email = '[email protected]', password = VALID_PASSWORD) {
31+
async function loginIfNeeded(page: Page, actions: LoginPageActions<Context>) {
3232
const authFile = getAuthFilePath()
33-
34-
await waitForLoaded(page)
3533
const isLoggedIn = (await page.getByTestId('before-auth-layout').count()) === 0
36-
3734
if (isLoggedIn) {
3835
test.info().annotations.push({
3936
type: 'skip',
4037
description: 'Already logged in',
4138
})
42-
return
43-
}
44-
45-
return test.step('Login', async () => {
46-
test.info().annotations.push({
47-
type: 'Login',
48-
description: 'Performing login',
49-
})
50-
await page.getByPlaceholder(TEXT.emailPlaceholder).fill(email)
51-
await page.getByPlaceholder(TEXT.passwordPlaceholder).fill(password)
52-
await page.getByRole('button', { name: TEXT.login, exact: true }).getByText(TEXT.login).click()
53-
54-
await expect(page.getByText(TEXT.loadingAppMessage)).not.toBeVisible()
55-
56-
await passAgreementsDialog({ page })
57-
39+
const agreementModalVisible = (await page.locator('#agreements-modal').count()) > 0
40+
if (agreementModalVisible) {
41+
await passAgreementsDialog({ page })
42+
await page.context().storageState({ path: authFile })
43+
}
44+
} else {
45+
await actions.login()
5846
await page.context().storageState({ path: authFile })
59-
})
47+
}
6048
}
6149

6250
/** Wait for the page to load. */
@@ -144,7 +132,7 @@ export function mockAllAndLogin({
144132
const actions = mockAll({ page, setupAPI, setupLocalAPI })
145133

146134
const driveActions = actions
147-
.step('Login', (page) => login({ page }))
135+
.step('Pass login screen', (page, _ctx, actions) => loginIfNeeded(page, actions))
148136
.step('Wait for dashboard to load', waitForDashboardToLoad)
149137
.into(DrivePageActions<Context>)
150138
return goToCloudFirst ? driveActions.goToCategory.cloud() : driveActions

app/gui/integration-test/dashboard/mock/react-stripe.tsx

Lines changed: 0 additions & 110 deletions
This file was deleted.

app/gui/integration-test/dashboard/mock/stripe.ts

Lines changed: 0 additions & 26 deletions
This file was deleted.

app/gui/package.json

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,6 @@
6767
"@react-aria/interactions": "3.23.0",
6868
"@sentry/vite-plugin": "^2.22.7",
6969
"@sentry/vue": "^7.120.2",
70-
"@stripe/react-stripe-js": "^2.9.0",
71-
"@stripe/stripe-js": "^3.5.0",
7270
"@tanstack/react-query": "5.59.20",
7371
"@tanstack/vue-query": "5.59.20",
7472
"@vue/reactivity": "^3.5.13",
@@ -145,7 +143,7 @@
145143
"@types/gunzip-maybe": "^1.4.2",
146144
"@types/hash-sum": "^1.0.2",
147145
"@types/mapbox-gl": "^3.4.1",
148-
"@types/node": "^22.10.4",
146+
"@types/node": "catalog:",
149147
"@types/papaparse": "^5.3.15",
150148
"@types/react": "^18.3.18",
151149
"@types/react-dom": "^18.3.5",
@@ -158,8 +156,8 @@
158156
"@types/ws": "^8.5.13",
159157
"@types/yauzl": "^2.10.3",
160158
"@types/zip-stream": "^7.0.0",
161-
"@vitejs/plugin-react": "^4.3.4",
162-
"@vitejs/plugin-vue": "^5.2.1",
159+
"@vitejs/plugin-react": "^5.0.0",
160+
"@vitejs/plugin-vue": "^6.0.1",
163161
"@vue/devtools-api": "^7.7.7",
164162
"@vue/test-utils": "^2.4.6",
165163
"@vue/tsconfig": "^0.5.1",
@@ -191,8 +189,8 @@
191189
"tar-stream": "^3.1.7",
192190
"typescript": "catalog:",
193191
"vite": "catalog:",
194-
"vite-plugin-vue-devtools": "7.7.7",
195-
"vite-plugin-wasm": "^3.4.1",
192+
"vite-plugin-vue-devtools": "^8.0.0",
193+
"vite-plugin-wasm": "^3.5.0",
196194
"vitest": "catalog:",
197195
"vue-tsc": "^2.2.0",
198196
"yaml": "^2.7.0",

app/gui/playwright.config.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,9 @@ export default defineConfig({
7878
fullyParallel: true,
7979
...(WORKERS ? { workers: WORKERS } : {}),
8080
forbidOnly: isCI,
81-
reporter: isCI ? [['list'], ['blob']] : [['html']],
81+
// Make test preview use the same port as test URL, so that svg icons are properly displayed.
82+
// Unfortunately we can't make it work for both dashboard and project-view at the same time.
83+
reporter: isCI ? [['list'], ['blob']] : [['html', { port: ports.projectView }]],
8284
retries: isCI ? 1 : 0,
8385
use: {
8486
actionTimeout: 5000,

app/gui/src/dashboard/components/Icon/Icon.tsx

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ import type {
1212
TestIdProps,
1313
} from '#/components/types'
1414
import { tv, type VariantProps } from '#/utilities/tailwindVariants'
15-
import icons from '@/assets/icons.svg'
1615
import { isIconName, type Icon as PossibleIcon } from '@/util/iconMetadata/iconName'
16+
import { svgUseHref } from '@/util/icons'
1717
import { memo } from 'react'
1818
import SvgMask from '../SvgMask'
1919

@@ -171,12 +171,7 @@ export function SvgUse(props: SvgUseProps) {
171171
preserveAspectRatio="xMidYMid slice"
172172
aria-label={alt}
173173
>
174-
<use
175-
href={icon.includes(':') ? icon : `${icons}#${icon}`}
176-
className="h-full w-full"
177-
aria-hidden="true"
178-
data-icon={icon}
179-
/>
174+
<use href={svgUseHref(icon)} className="h-full w-full" aria-hidden="true" data-icon={icon} />
180175
</svg>
181176
)
182177
}
Lines changed: 6 additions & 0 deletions
Loading

app/gui/src/project-view/components/GraphEditor/widgets/WidgetIcon.vue

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,7 @@ import NodeWidget from '@/components/GraphEditor/NodeWidget.vue'
33
import GrowingSpinner from '@/components/shared/GrowingSpinner.vue'
44
import SvgIcon from '@/components/SvgIcon.vue'
55
import { Score, defineWidget, widgetProps } from '@/providers/widgetRegistry'
6-
import { type URLString } from '@/util/data/urlString'
7-
import { type Icon } from '@/util/iconMetadata/iconName'
6+
import { type AnyWidgetIcon } from '@/util/icons'
87
import { computed } from 'vue'
98
109
const props = defineProps(widgetProps(widgetDefinition))
@@ -17,7 +16,7 @@ export const DisplayIcon: unique symbol = Symbol.for('WidgetInput:DisplayIcon')
1716
declare module '@/providers/widgetRegistry' {
1817
export interface WidgetInput {
1918
[DisplayIcon]?: {
20-
icon: Icon | URLString | '$evaluating'
19+
icon: AnyWidgetIcon
2120
allowChoice?: boolean
2221
showContents?: boolean
2322
noGap?: boolean

app/gui/src/project-view/components/StandaloneButton.vue

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
<script setup lang="ts">
2-
import type { URLString } from '@/util/data/urlString'
3-
import type { Icon } from '@/util/iconMetadata/iconName'
2+
import { type AnyIcon } from '@/util/icons'
43
import SvgButton from './SvgButton.vue'
54
65
const props = defineProps<{
7-
icon?: Icon | URLString | undefined
6+
icon?: AnyIcon | undefined
87
label?: string | undefined
98
disabled?: boolean
109
title?: string | undefined

0 commit comments

Comments
 (0)