diff --git a/CHANGELOG.md b/CHANGELOG.md index d84c0337200..afa7d72fa76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ and this project adheres to ### Fixed +- Fix issue where back button must be pressed 3 times to go back once from the + Workflow canvas [#4812](https://github.com/OpenFn/lightning/issues/4812) - Reduce `run:log` channel timeouts under heavy log volume by moving `log_lines` search indexing off the insert path. The full-text search vector is now backfilled by a background worker rather than computed synchronously on every @@ -38,7 +40,8 @@ and this project adheres to the whole run. The search vector is now built off the insert path by a background `Lightning.Invocation.DataclipSearchVectorWorker` (sharing the `search_indexing` queue with the log-lines worker), making dataclip search - eventually consistent. [#4800](https://github.com/OpenFn/lightning/issues/4800) + eventually consistent. + [#4800](https://github.com/OpenFn/lightning/issues/4800) - Channel join crashes when multiple users open the same workflow concurrently [#4802](https://github.com/OpenFn/lightning/issues/4802) - Fix `purge_deleted` Oban job crashing when a soft-deleted project has diff --git a/assets/js/react/lib/use-url-state.ts b/assets/js/react/lib/use-url-state.ts index a7a915e266b..f088d47f538 100644 --- a/assets/js/react/lib/use-url-state.ts +++ b/assets/js/react/lib/use-url-state.ts @@ -82,6 +82,29 @@ class URLStore { */ getSnapshot = () => this.state; + // A fresh, mutable copy of the current URL. Writers seed from this and + // override only the dimension they own (search or hash); everything else + // carries over untouched. + private currentURL = () => new URL(window.location.href); + + private urlsAreEquivalent = (a: URL, b: URL) => { + if (a.pathname !== b.pathname || a.hash !== b.hash) return false; + const ap = new URLSearchParams(a.search); + ap.sort(); + const bp = new URLSearchParams(b.search); + bp.sort(); + return ap.toString() === bp.toString(); + }; + + // Skip no-op writes so mount-time normalization doesn't stack duplicate + // browser history entries (a no-op pushState never notifies subscribers + // anyway, due to the guard in updateParams). Param order is ignored so a + // reorder-only write is also treated as a no-op. + private pushIfChanged = (newURL: URL) => { + if (this.urlsAreEquivalent(newURL, this.currentURL())) return; + history.pushState({}, '', newURL); + }; + /** * Update URL search params (merges with existing params). * Accepts strings, numbers, booleans; null removes param. @@ -89,20 +112,17 @@ class URLStore { updateSearchParams = ( updates: Record ) => { - const newParams = new URLSearchParams(window.location.search); + const newURL = this.currentURL(); Object.entries(updates).forEach(([key, value]) => { if (value === null) { - newParams.delete(key); + newURL.searchParams.delete(key); } else { - newParams.set(key, String(value)); + newURL.searchParams.set(key, String(value)); } }); - const newURL = new URL(window.location.pathname, window.location.origin); - newURL.search = newParams.toString(); - newURL.hash = window.location.hash; - history.pushState({}, '', newURL); + this.pushIfChanged(newURL); }; /** @@ -112,15 +132,14 @@ class URLStore { replaceSearchParams = ( newParams: Record ) => { - const newURL = new URL(window.location.pathname, window.location.origin); - // Only set params with non-null values (clears all existing params) + const newURL = this.currentURL(); + newURL.search = ''; Object.entries(newParams).forEach(([key, value]) => { if (value !== null) { newURL.searchParams.set(key, String(value)); } }); - newURL.hash = window.location.hash; - history.pushState({}, '', newURL); + this.pushIfChanged(newURL); }; /** @@ -128,11 +147,9 @@ class URLStore { * Pass null to remove hash. */ updateHash = (hash: string | null) => { - const newURL = - window.location.pathname + - window.location.search + - (hash ? `#${hash}` : ''); - history.pushState({}, '', newURL); + const newURL = this.currentURL(); + newURL.hash = hash ? `#${hash}` : ''; + this.pushIfChanged(newURL); }; } diff --git a/assets/test/react/lib/use-url-state.test.ts b/assets/test/react/lib/use-url-state.test.ts index 2464c4d7ec4..da1ee6a316c 100644 --- a/assets/test/react/lib/use-url-state.test.ts +++ b/assets/test/react/lib/use-url-state.test.ts @@ -11,7 +11,7 @@ */ import { renderHook, act } from '@testing-library/react'; -import { describe, test, expect, beforeEach } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest'; import { useURLState } from '../../../js/react/lib/use-url-state'; @@ -201,6 +201,151 @@ describe('useURLState', () => { }); }); + describe('updateSearchParams - no-op writes', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test('does not push a history entry when params are unchanged', () => { + history.replaceState({}, '', '/workflow?panel=run&job=abc'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + // Same values that are already in the URL -> a no-op + result.current.updateSearchParams({ panel: 'run', job: 'abc' }); + }); + + expect(pushSpy).not.toHaveBeenCalled(); + expect(window.location.search).toBe('?panel=run&job=abc'); + }); + + test('still pushes a history entry when a param actually changes', () => { + history.replaceState({}, '', '/workflow?panel=run'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.updateSearchParams({ panel: 'inspector' }); + }); + + expect(pushSpy).toHaveBeenCalledTimes(1); + expect(result.current.params.panel).toBe('inspector'); + }); + }); + + describe('replaceSearchParams - no-op writes', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test('does not push a history entry when params are unchanged', () => { + history.replaceState({}, '', '/workflow?panel=run'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.replaceSearchParams({ panel: 'run' }); + }); + + expect(pushSpy).not.toHaveBeenCalled(); + expect(window.location.search).toBe('?panel=run'); + }); + + test('still pushes a history entry when a param actually changes', () => { + history.replaceState({}, '', '/workflow?panel=run'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.replaceSearchParams({ panel: 'inspector' }); + }); + + expect(pushSpy).toHaveBeenCalledTimes(1); + expect(result.current.params.panel).toBe('inspector'); + }); + + test('does not push when params are reordered but unchanged', () => { + history.replaceState({}, '', '/workflow?job=abc&panel=run'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + // Same params, different object-key order — semantically identical URL + result.current.replaceSearchParams({ panel: 'run', job: 'abc' }); + }); + + expect(pushSpy).not.toHaveBeenCalled(); + expect(window.location.search).toBe('?job=abc&panel=run'); + }); + }); + + describe('updateHash - no-op writes', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test('does not push a history entry when hash is unchanged', () => { + history.replaceState({}, '', '/workflow#step-2'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.updateHash('step-2'); + }); + + expect(pushSpy).not.toHaveBeenCalled(); + expect(window.location.hash).toBe('#step-2'); + }); + + test('does not push when clearing an already-absent hash', () => { + history.replaceState({}, '', '/workflow'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.updateHash(null); + }); + + expect(pushSpy).not.toHaveBeenCalled(); + }); + + test('still pushes when adding a hash', () => { + history.replaceState({}, '', '/workflow'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.updateHash('step-5'); + }); + + expect(pushSpy).toHaveBeenCalledTimes(1); + expect(window.location.hash).toBe('#step-5'); + }); + + test('still pushes when removing an existing hash', () => { + history.replaceState({}, '', '/workflow#a'); + + const { result } = renderHook(() => useURLState()); + const pushSpy = vi.spyOn(history, 'pushState'); + + act(() => { + result.current.updateHash(null); + }); + + expect(pushSpy).toHaveBeenCalledTimes(1); + expect(window.location.hash).toBe(''); + }); + }); + describe('replaceSearchParams', () => { test('replaces all params with new ones (clears unspecified params)', () => { history.replaceState({}, '', '/workflow?panel=run&job=abc&step=5');