diff --git a/e2e/playwright/test-network-and-connection-issues.spec.ts b/e2e/playwright/test-network-and-connection-issues.spec.ts index 157607817..40fde6537 100644 --- a/e2e/playwright/test-network-and-connection-issues.spec.ts +++ b/e2e/playwright/test-network-and-connection-issues.spec.ts @@ -61,7 +61,7 @@ test.describe('Test network related behaviors', () => { }) // Expect the network to be down - await expect(networkToggle).toContainText('Network health (Offline)') + await expect(networkToggle).toContainText('Problem') // Click the network widget await networkWidget.click() @@ -156,8 +156,7 @@ test.describe('Test network related behaviors', () => { // Expect the network to be down await networkToggle.hover() - - await expect(networkToggle).toContainText('Network health (Offline)') + await expect(networkToggle).toContainText('Problem') // Ensure we are not in sketch mode await expect( diff --git a/e2e/playwright/test-utils.ts b/e2e/playwright/test-utils.ts index 294367bc5..aa1e9ca08 100644 --- a/e2e/playwright/test-utils.ts +++ b/e2e/playwright/test-utils.ts @@ -364,6 +364,11 @@ export async function getUtils(page: Page, test_?: typeof test) { ) } + // Chrome devtools protocol session only works in Chromium + const browserType = page.context().browser()?.browserType().name() + const cdpSession = + browserType !== 'chromium' ? null : await page.context().newCDPSession(page) + const util = { waitForAuthSkipAppStart: () => waitForAuthAndLsp(page), waitForPageLoad: () => waitForPageLoad(page), @@ -484,9 +489,15 @@ export async function getUtils(page: Page, test_?: typeof test) { emulateNetworkConditions: async ( networkOptions: Protocol.Network.emulateNetworkConditionsParameters ) => { - return networkOptions.offline - ? page.evaluate('window.engineCommandManager.offline()') - : page.evaluate('window.engineCommandManager.online()') + if (cdpSession === null) { + // Use a fail safe if we can't simulate disconnect (on Safari) + return page.evaluate('window.engineCommandManager.tearDown()') + } + + return cdpSession?.send( + 'Network.emulateNetworkConditions', + networkOptions + ) }, toNormalizedCode(text: string) { diff --git a/src/App.tsx b/src/App.tsx index 710cd213a..e853746d0 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -114,9 +114,6 @@ export function App() { // by the Projects view. billingActor.send({ type: BillingTransition.Update, apiToken: authToken }) - // Tell engineStream to wait for dependencies to start streaming. - engineStreamActor.send({ type: EngineStreamTransition.WaitForDependencies }) - // When leaving the modeling scene, cut the engine stream. return () => { // When leaving the modeling scene, cut the engine stream. diff --git a/src/clientSideScene/sceneEntities.ts b/src/clientSideScene/sceneEntities.ts index db47e6b29..28e0a457b 100644 --- a/src/clientSideScene/sceneEntities.ts +++ b/src/clientSideScene/sceneEntities.ts @@ -3588,8 +3588,7 @@ export class SceneEntities { }) if (!resp) { - console.warn('No response') - return {} as Models['GetSketchModePlane_type'] + return Promise.reject('no response') } if (isArray(resp)) { diff --git a/src/components/EngineStream.tsx b/src/components/EngineStream.tsx index d7be1b700..3b6a1154a 100644 --- a/src/components/EngineStream.tsx +++ b/src/components/EngineStream.tsx @@ -1,3 +1,4 @@ +import { isPlaywright } from '@src/lib/isPlaywright' import { useAppState } from '@src/AppState' import { ClientSideScene } from '@src/clientSideScene/ClientSideSceneComp' import { ViewControlContextMenu } from '@src/components/ViewControlMenu' @@ -51,10 +52,8 @@ export const EngineStream = (props: { const last = useRef(Date.now()) const [firstPlay, setFirstPlay] = useState(true) - const [goRestart, setGoRestart] = useState(false) - const [timeoutId, setTimeoutId] = useState< - ReturnType | undefined - >(undefined) + const [isRestartRequestStarting, setIsRestartRequestStarting] = + useState(false) const [attemptTimes, setAttemptTimes] = useState<[number, number]>([ 0, TIME_1_SECOND, @@ -86,21 +85,18 @@ export const EngineStream = (props: { const streamIdleMode = settings.app.streamIdleMode.current useEffect(() => { - // Will cause a useEffect loop if not checked for. - if (engineStreamState.context.videoRef.current !== null) return engineStreamActor.send({ type: EngineStreamTransition.SetVideoRef, videoRef: { current: videoRef.current }, }) - }, [videoRef.current, engineStreamState]) + }, [videoRef.current]) useEffect(() => { - if (engineStreamState.context.canvasRef.current !== null) return engineStreamActor.send({ type: EngineStreamTransition.SetCanvasRef, canvasRef: { current: canvasRef.current }, }) - }, [canvasRef.current, engineStreamState]) + }, [canvasRef.current]) useEffect(() => { engineStreamActor.send({ @@ -135,6 +131,24 @@ export const EngineStream = (props: { }) } + useEffect(() => { + // Only try to start the stream if we're stopped or think we're done + // waiting for dependencies. + if ( + !( + engineStreamState.value === EngineStreamState.WaitingForDependencies || + engineStreamState.value === EngineStreamState.Stopped + ) + ) + return + + // Don't bother trying to connect if the auth token is empty. + // We have the checks in the machine but this can cause a hot loop. + if (!engineStreamState.context.authToken) return + + startOrReconfigureEngine() + }, [engineStreamState, setAppState]) + // I would inline this but it needs to be a function for removeEventListener. const play = () => { engineStreamActor.send({ @@ -160,13 +174,12 @@ export const EngineStream = (props: { console.log('scene is ready, execute kcl') const kmp = kclManager.executeCode().catch(trap) - // Reset the restart timeouts - setAttemptTimes([0, TIME_1_SECOND]) - - console.log(firstPlay) if (!firstPlay) return setFirstPlay(false) + // Reset the restart timeouts + setAttemptTimes([0, TIME_1_SECOND]) + console.log('firstPlay true, zoom to fit') kmp .then(async () => { @@ -198,76 +211,51 @@ export const EngineStream = (props: { // We do a back-off restart, using a fibonacci sequence, since it // has a nice retry time curve (somewhat quick then exponential) const attemptRestartIfNecessary = () => { - // Timeout already set. - if (timeoutId) return - - setTimeoutId( - setTimeout(() => { - engineStreamState.context.videoRef.current?.pause() - engineCommandManager.tearDown() - startOrReconfigureEngine() - setFirstPlay(true) - - setTimeoutId(undefined) - setGoRestart(false) - }, attemptTimes[0] + attemptTimes[1]) - ) + if (isRestartRequestStarting) return + setIsRestartRequestStarting(true) + setTimeout(() => { + engineStreamState.context.videoRef.current?.pause() + engineCommandManager.tearDown() + startOrReconfigureEngine() + setFirstPlay(false) + setIsRestartRequestStarting(false) + }, attemptTimes[0] + attemptTimes[1]) setAttemptTimes([attemptTimes[1], attemptTimes[0] + attemptTimes[1]]) } - const onOffline = () => { - if ( - !( - EngineConnectionStateType.Disconnected === - engineCommandManager.engineConnection?.state.type || - EngineConnectionStateType.Disconnecting === - engineCommandManager.engineConnection?.state.type - ) - ) { + // Poll that we're connected. If not, send a reset signal. + // Do not restart if we're in idle mode. + const connectionCheckIntervalId = setInterval(() => { + // SKIP DURING TESTS BECAUSE IT WILL MESS WITH REUSING THE + // ELECTRON INSTANCE. + if (isPlaywright()) { return } - engineStreamActor.send({ type: EngineStreamTransition.Stop }) - attemptRestartIfNecessary() - } - if ( - !goRestart && - engineStreamState.value === EngineStreamState.WaitingForDependencies - ) { - setGoRestart(true) - } + // Don't try try to restart if we're already connected! + const hasEngineConnectionInst = engineCommandManager.engineConnection + const isDisconnected = + engineCommandManager.engineConnection?.state.type === + EngineConnectionStateType.Disconnected + const inIdleMode = engineStreamState.value === EngineStreamState.Paused + if ((hasEngineConnectionInst && !isDisconnected) || inIdleMode) return - if (goRestart && !timeoutId) { attemptRestartIfNecessary() - } + }, TIME_1_SECOND) engineCommandManager.addEventListener( EngineCommandManagerEvents.EngineRestartRequest, attemptRestartIfNecessary ) - - engineCommandManager.addEventListener( - EngineCommandManagerEvents.Offline, - onOffline - ) - return () => { + clearInterval(connectionCheckIntervalId) + engineCommandManager.removeEventListener( EngineCommandManagerEvents.EngineRestartRequest, attemptRestartIfNecessary ) - engineCommandManager.removeEventListener( - EngineCommandManagerEvents.Offline, - onOffline - ) } - }, [ - engineStreamState, - attemptTimes, - goRestart, - timeoutId, - engineCommandManager.engineConnection?.state.type, - ]) + }, [engineStreamState, attemptTimes, isRestartRequestStarting]) useEffect(() => { // If engineStreamMachine is already reconfiguring, bail. @@ -281,7 +269,7 @@ export const EngineStream = (props: { const canvas = engineStreamState.context.canvasRef?.current if (!canvas) return - const observer = new ResizeObserver(() => { + new ResizeObserver(() => { // Prevents: // `Uncaught ResizeObserver loop completed with undelivered notifications` window.requestAnimationFrame(() => { @@ -292,19 +280,13 @@ export const EngineStream = (props: { if ( (Math.abs(video.width - window.innerWidth) > 4 || Math.abs(video.height - window.innerHeight) > 4) && - engineStreamState.matches(EngineStreamState.Playing) + !engineStreamState.matches(EngineStreamState.WaitingToPlay) ) { timeoutStart.current = Date.now() startOrReconfigureEngine() } }) - }) - - observer.observe(document.body) - - return () => { - observer.disconnect() - } + }).observe(document.body) }, [engineStreamState.value]) /** @@ -363,21 +345,8 @@ export const EngineStream = (props: { timeoutStart.current = null } else if (timeoutStart.current) { const elapsed = Date.now() - timeoutStart.current - // Don't pause if we're already disconnected. - if ( - // It's unnecessary to once again setup an event listener for - // offline/online to capture this state, when this state already - // exists on the window.navigator object. In hindsight it makes - // me (lee) regret we set React state variables such as - // isInternetConnected in other files when we could check this - // object instead. - engineCommandManager.engineConnection?.state.type === - EngineConnectionStateType.ConnectionEstablished && - elapsed >= IDLE_TIME_MS && - engineStreamState.value === EngineStreamState.Playing - ) { + if (elapsed >= IDLE_TIME_MS) { timeoutStart.current = null - console.log('PAUSING') engineStreamActor.send({ type: EngineStreamTransition.Pause }) } } @@ -388,7 +357,7 @@ export const EngineStream = (props: { return () => { window.cancelAnimationFrame(frameId) } - }, [modelingMachineState, engineStreamState.value]) + }, [modelingMachineState]) useEffect(() => { if (!streamIdleMode) return @@ -401,18 +370,9 @@ export const EngineStream = (props: { return } - engineStreamActor.send({ - type: EngineStreamTransition.Resume, - modelingMachineActorSend, - settings: settingsEngine, - setAppState, - onMediaStream(mediaStream: MediaStream) { - engineStreamActor.send({ - type: EngineStreamTransition.SetMediaStream, - mediaStream, - }) - }, - }) + if (engineStreamState.value === EngineStreamState.Paused) { + startOrReconfigureEngine() + } timeoutStart.current = Date.now() } @@ -511,11 +471,7 @@ export const EngineStream = (props: { } sendSelectEventToEngine(e) - .then((result) => { - if (!result) { - return - } - const { entity_id } = result + .then(({ entity_id }) => { if (!entity_id) { // No entity selected. This is benign return diff --git a/src/hooks/useNetworkStatus.tsx b/src/hooks/useNetworkStatus.tsx index a6e8219e7..581d3276e 100644 --- a/src/hooks/useNetworkStatus.tsx +++ b/src/hooks/useNetworkStatus.tsx @@ -78,19 +78,18 @@ export function useNetworkStatus() { }, [hasIssues, internetConnected, ping]) useEffect(() => { + const onlineCallback = () => { + setInternetConnected(true) + } const offlineCallback = () => { setInternetConnected(false) setSteps(structuredClone(initialConnectingTypeGroupState)) } - engineCommandManager.addEventListener( - EngineCommandManagerEvents.Offline, - offlineCallback - ) + window.addEventListener('online', onlineCallback) + window.addEventListener('offline', offlineCallback) return () => { - engineCommandManager.removeEventListener( - EngineCommandManagerEvents.Offline, - offlineCallback - ) + window.removeEventListener('online', onlineCallback) + window.removeEventListener('offline', offlineCallback) } }, []) @@ -140,8 +139,6 @@ export function useNetworkStatus() { if ( engineConnectionState.type === EngineConnectionStateType.Connecting ) { - setInternetConnected(true) - const groups = Object.values(nextSteps) for (let group of groups) { for (let step of group) { @@ -171,10 +168,6 @@ export function useNetworkStatus() { if (engineConnectionState.value.type === DisconnectingType.Error) { setError(engineConnectionState.value.value) - } else if ( - engineConnectionState.value.type === DisconnectingType.Quit - ) { - return structuredClone(initialConnectingTypeGroupState) } } } diff --git a/src/lang/std/engineConnection.ts b/src/lang/std/engineConnection.ts index 0b9b2e05a..df07c02aa 100644 --- a/src/lang/std/engineConnection.ts +++ b/src/lang/std/engineConnection.ts @@ -1,3 +1,4 @@ +import { TEST } from '@src/env' import type { Models } from '@kittycad/lib' import { VITE_KC_API_WS_MODELING_URL, VITE_KC_DEV_TOKEN } from '@src/env' import { jsAppSettings } from '@src/lib/settings/settingsUtils' @@ -82,9 +83,6 @@ export enum ConnectionError { TooManyConnections, Outage, - // Observed to happen on a local network outage. - PeerConnectionRemoteDisconnected, - // An unknown error is the most severe because it has not been classified // or encountered before. Unknown, @@ -109,8 +107,6 @@ export const CONNECTION_ERROR_TEXT: Record = { 'There are too many open engine connections associated with your account.', [ConnectionError.Outage]: 'We seem to be experiencing an outage. Please visit [status.zoo.dev](https://status.zoo.dev) for updates.', - [ConnectionError.PeerConnectionRemoteDisconnected]: - 'The remote end has disconnected.', [ConnectionError.Unknown]: 'An unexpected error occurred. Please report this to us.', } @@ -230,9 +226,6 @@ export enum EngineConnectionEvents { Opened = 'opened', // (engineConnection: EngineConnection) => void Closed = 'closed', // (engineConnection: EngineConnection) => void NewTrack = 'new-track', // (track: NewTrackArgs) => void - - // A general offline state. - Offline = 'offline', } function toRTCSessionDescriptionInit( @@ -681,20 +674,9 @@ class EngineConnection extends EventTarget { // The remote end broke up with us! :( case 'disconnected': - this.state = { - type: EngineConnectionStateType.Disconnecting, - value: { - type: DisconnectingType.Error, - value: { - error: ConnectionError.PeerConnectionRemoteDisconnected, - context: event, - }, - }, - } this.dispatchEvent( - new CustomEvent(EngineConnectionEvents.Offline, {}) + new CustomEvent(EngineConnectionEvents.RestartRequest, {}) ) - this.disconnectAll() break case 'closed': this.pc?.removeEventListener('icecandidate', this.onIceCandidate) @@ -865,6 +847,7 @@ class EngineConnection extends EventTarget { 'message', this.onDataChannelMessage ) + this.disconnectAll() } this.unreliableDataChannel?.addEventListener( @@ -883,6 +866,7 @@ class EngineConnection extends EventTarget { }, }, } + this.disconnectAll() } this.unreliableDataChannel?.addEventListener( 'error', @@ -972,9 +956,6 @@ class EngineConnection extends EventTarget { this.onNetworkStatusReady ) - this.dispatchEvent( - new CustomEvent(EngineConnectionEvents.Offline, {}) - ) this.disconnectAll() } this.websocket.addEventListener('close', this.onWebSocketClose) @@ -993,6 +974,8 @@ class EngineConnection extends EventTarget { }, } } + + this.disconnectAll() } this.websocket.addEventListener('error', this.onWebSocketError) @@ -1348,9 +1331,6 @@ export enum EngineCommandManagerEvents { // the whole scene is ready (settings loaded) SceneReady = 'scene-ready', - - // we're offline - Offline = 'offline', } /** @@ -1400,7 +1380,6 @@ export class EngineCommandManager extends EventTarget { * This is compared to the {@link outSequence} number to determine if we should ignore * any out-of-order late responses in the unreliable channel. */ - keepForcefulOffline = false inSequence = 1 engineConnection?: EngineConnection commandLogs: CommandLog[] = [] @@ -1474,8 +1453,13 @@ export class EngineCommandManager extends EventTarget { ) } - private onEngineOffline = () => { - this.dispatchEvent(new CustomEvent(EngineCommandManagerEvents.Offline, {})) + private onOffline = () => { + console.log('Browser reported network is offline') + if (TEST) { + console.warn('DURING TESTS ENGINECONNECTION.ONOFFLINE WILL DO NOTHING.') + return + } + this.onEngineConnectionRestartRequest() } idleMode: boolean = false @@ -1510,11 +1494,6 @@ export class EngineCommandManager extends EventTarget { if (settings) { this.settings = settings } - - if (this.keepForcefulOffline) { - return - } - if (width === 0 || height === 0) { return } @@ -1530,6 +1509,8 @@ export class EngineCommandManager extends EventTarget { return } + window.addEventListener('offline', this.onOffline) + let additionalSettings = this.settings.enableSSAO ? '&post_effect=ssao' : '' additionalSettings += '&show_grid=' + (this.settings.showScaleGrid ? 'true' : 'false') @@ -1556,11 +1537,6 @@ export class EngineCommandManager extends EventTarget { this.onEngineConnectionRestartRequest as EventListener ) - this.engineConnection.addEventListener( - EngineConnectionEvents.Offline, - this.onEngineOffline as EventListener - ) - // eslint-disable-next-line @typescript-eslint/no-misused-promises this.onEngineConnectionOpened = async () => { console.log('onEngineConnectionOpened') @@ -1576,9 +1552,9 @@ export class EngineCommandManager extends EventTarget { // Let's restart. console.warn("shit's gone south") console.warn(e) - // this.engineConnection?.dispatchEvent( - // new CustomEvent(EngineConnectionEvents.RestartRequest, {}) - // ) + this.engineConnection?.dispatchEvent( + new CustomEvent(EngineConnectionEvents.RestartRequest, {}) + ) return } @@ -1621,7 +1597,23 @@ export class EngineCommandManager extends EventTarget { console.log('camControlsCameraChange') this._camControlsCameraChange() - await this.sceneInfra?.camControls.restoreRemoteCameraStateAndTriggerSync() + // We should eventually only have 1 restoral call. + if (this.idleMode) { + await this.sceneInfra?.camControls.restoreRemoteCameraStateAndTriggerSync() + } else { + // NOTE: This code is old. It uses the old hack to restore camera. + console.log('call default_camera_get_settings') + // eslint-disable-next-line @typescript-eslint/no-floating-promises + await this.sendSceneCommand({ + // CameraControls subscribes to default_camera_get_settings response events + // firing this at connection ensure the camera's are synced initially + type: 'modeling_cmd_req', + cmd_id: uuidv4(), + cmd: { + type: 'default_camera_get_settings', + }, + }) + } setIsStreamReady(true) @@ -1885,6 +1877,8 @@ export class EngineCommandManager extends EventTarget { tearDown(opts?: { idleMode: boolean }) { this.idleMode = opts?.idleMode ?? false + window.removeEventListener('offline', this.onOffline) + if (this.engineConnection) { for (const [cmdId, pending] of Object.entries(this.pendingCommands)) { pending.reject([ @@ -1934,26 +1928,7 @@ export class EngineCommandManager extends EventTarget { this.engineCommandManager.engineConnection = null } this.engineConnection = undefined - - // It is possible all connections never even started, but we still want - // to signal to the whole application we are "offline". - this.dispatchEvent(new CustomEvent(EngineCommandManagerEvents.Offline, {})) } - - offline() { - this.keepForcefulOffline = true - this.tearDown() - console.log('offline') - } - - online() { - this.keepForcefulOffline = false - this.dispatchEvent( - new CustomEvent(EngineCommandManagerEvents.EngineRestartRequest, {}) - ) - console.log('online') - } - async startNewSession() { this.responseMap = {} } diff --git a/src/lib/selections.ts b/src/lib/selections.ts index c9a17b27c..4d94c5569 100644 --- a/src/lib/selections.ts +++ b/src/lib/selections.ts @@ -703,8 +703,7 @@ export async function sendSelectEventToEngine( cmd_id: uuidv4(), }) if (!res) { - console.warn('No response') - return undefined + return Promise.reject('no response') } if (isArray(res)) { diff --git a/src/machines/engineStreamMachine.ts b/src/machines/engineStreamMachine.ts index d23564db6..013887c4d 100644 --- a/src/machines/engineStreamMachine.ts +++ b/src/machines/engineStreamMachine.ts @@ -70,6 +70,7 @@ export async function holdOntoVideoFrameInCanvas( video: HTMLVideoElement, canvas: HTMLCanvasElement ) { + video.pause() canvas.width = video.videoWidth canvas.height = video.videoHeight canvas.style.width = video.videoWidth + 'px' @@ -219,14 +220,11 @@ export const engineStreamMachine = setup({ if (context.videoRef.current && context.canvasRef.current) { await context.videoRef.current.pause() - // It's possible we've already frozen the frame due to a disconnect. - if (context.videoRef.current.style.display !== 'none') { - await holdOntoVideoFrameInCanvas( - context.videoRef.current, - context.canvasRef.current - ) - context.videoRef.current.style.display = 'none' - } + await holdOntoVideoFrameInCanvas( + context.videoRef.current, + context.canvasRef.current + ) + context.videoRef.current.style.display = 'none' } await rootContext.sceneInfra.camControls.saveRemoteCameraState() @@ -367,12 +365,9 @@ export const engineStreamMachine = setup({ }), }, on: { - [EngineStreamTransition.Resume]: { + [EngineStreamTransition.StartOrReconfigureEngine]: { target: EngineStreamState.Resuming, }, - [EngineStreamTransition.Stop]: { - target: EngineStreamState.Stopped, - }, }, }, [EngineStreamState.Stopped]: { @@ -403,23 +398,12 @@ export const engineStreamMachine = setup({ rootContext: args.self.system.get('root').getSnapshot().context, event: args.event, }), - // Usually only fails if there was a disconnection mid-way. - onError: [ - { - target: EngineStreamState.WaitingForDependencies, - reenter: true, - }, - ], }, on: { // The stream can be paused as it's resuming. [EngineStreamTransition.Pause]: { target: EngineStreamState.Paused, }, - // The stream can be stopped as it's resuming. - [EngineStreamTransition.Stop]: { - target: EngineStreamState.Stopped, - }, [EngineStreamTransition.SetMediaStream]: { target: EngineStreamState.Playing, actions: [