diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f682f6c23..756ffd2811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to ### Changed +- Improve version pinning behavior in collaborative editor + [#4121](https://github.com/OpenFn/lightning/issues/4121) - Unify disabled button states across collaborative editor for consistent styling and behaviour [#4179](https://github.com/OpenFn/lightning/issues/4179) diff --git a/assets/js/collaborative-editor/components/VersionDropdown.tsx b/assets/js/collaborative-editor/components/VersionDropdown.tsx index 68d8f63a1b..f7b1101153 100644 --- a/assets/js/collaborative-editor/components/VersionDropdown.tsx +++ b/assets/js/collaborative-editor/components/VersionDropdown.tsx @@ -1,5 +1,7 @@ import { useEffect, useRef, useState } from 'react'; +import { useURLState } from '#/react/lib/use-url-state'; + import { cn } from '../../utils/cn'; import { useRequestVersions, @@ -30,11 +32,16 @@ export function VersionDropdown({ const versionsError = useVersionsError(); const requestVersions = useRequestVersions(); + // Check if version is pinned via URL parameter + const { params } = useURLState(); + const isPinnedVersion = params['v'] !== undefined && params['v'] !== null; + // Show placeholder while loading version information const isLoadingVersion = currentVersion === null || latestVersion === null; - // Determine if viewing latest version (only when we have both values) - const isLatestVersion = !isLoadingVersion && currentVersion === latestVersion; + // Determine if viewing latest version (only when we have both values AND no pinned version) + const isLatestVersion = + !isLoadingVersion && currentVersion === latestVersion && !isPinnedVersion; // Format version display const currentVersionDisplay = isLoadingVersion @@ -95,8 +102,8 @@ export function VersionDropdown({ } }, [versionsError]); - const handleVersionClick = (version: Version) => { - if (version.is_latest) { + const handleVersionClick = (version: Version | 'latest') => { + if (version === 'latest') { onVersionSelect('latest'); } else { onVersionSelect(version.lock_version); @@ -146,44 +153,66 @@ export function VersionDropdown({ No versions available ) : ( - versions.map((version, index) => { - const isSelected = version.is_latest - ? isLatestVersion - : version.lock_version === currentVersion; - - // Show "latest" for the latest version, otherwise show version number - // For the first item (which is latest), show "latest" - // For subsequent items, show version number even if they have is_latest=true - const displayText = - index === 0 && version.is_latest - ? 'latest' - : `v${String(version.lock_version).substring(0, 7)}`; - - return ( + <> + {/* First, show "latest" option that removes version parameter */} + {versions.length > 0 && versions[0].is_latest && ( - ); - }) + )} + + {/* Then show all versions with version numbers (including latest) */} + {versions.map(version => { + const isSelected = + !isLatestVersion && version.lock_version === currentVersion; + + const displayText = `v${String(version.lock_version).substring(0, 7)}`; + + return ( + + ); + })} + )} diff --git a/assets/js/collaborative-editor/components/diagram/CollaborativeWorkflowDiagram.tsx b/assets/js/collaborative-editor/components/diagram/CollaborativeWorkflowDiagram.tsx index 063f416099..3b5d95f302 100644 --- a/assets/js/collaborative-editor/components/diagram/CollaborativeWorkflowDiagram.tsx +++ b/assets/js/collaborative-editor/components/diagram/CollaborativeWorkflowDiagram.tsx @@ -19,14 +19,16 @@ import { useHistoryLoading, useRunSteps, } from '../../hooks/useHistory'; -import { useIsNewWorkflow } from '../../hooks/useSessionContext'; +import { + useIsNewWorkflow, + useLatestSnapshotLockVersion, +} from '../../hooks/useSessionContext'; import { useVersionMismatch } from '../../hooks/useVersionMismatch'; import { useNodeSelection } from '../../hooks/useWorkflow'; import { useKeyboardShortcut } from '../../keyboard'; import type { RunSummary } from '../../types/history'; import MiniHistory from './MiniHistory'; -import { VersionMismatchBanner } from './VersionMismatchBanner'; import CollaborativeWorkflowDiagramImpl from './WorkflowDiagram'; interface CollaborativeWorkflowDiagramProps { @@ -42,6 +44,7 @@ export function CollaborativeWorkflowDiagram({ const isNewWorkflow = useIsNewWorkflow(); const isHistoryChannelConnected = useHistoryChannelConnected(); const { params, updateSearchParams } = useURLState(); + const latestSnapshotLockVersion = useLatestSnapshotLockVersion(); // Get history data and commands const history = useHistory(); @@ -85,14 +88,22 @@ export function CollaborativeWorkflowDiagram({ // Find the workorder that contains this run const workorder = history.find(wo => wo.runs.some(r => r.id === run.id)); + // Only include version parameter if the run's version differs from latest + // This prevents pinning to read-only mode when viewing latest version runs + const runVersion = workorder?.version; + const shouldPinVersion = + runVersion !== null && + runVersion !== undefined && + runVersion !== latestSnapshotLockVersion; + // Single atomic update - both version and run in one call // This prevents race conditions between two separate updateSearchParams calls updateSearchParams({ - v: workorder ? String(workorder.version) : null, + v: shouldPinVersion ? String(runVersion) : null, run: run.id, }); }, - [history, updateSearchParams] + [history, latestSnapshotLockVersion, updateSearchParams] ); // Clear URL parameter when deselecting run @@ -155,14 +166,6 @@ export function CollaborativeWorkflowDiagram({ return (
- {versionMismatch && ( - - )} - )} diff --git a/assets/js/collaborative-editor/components/diagram/MiniHistory.tsx b/assets/js/collaborative-editor/components/diagram/MiniHistory.tsx index 21792438b9..f47085a62b 100644 --- a/assets/js/collaborative-editor/components/diagram/MiniHistory.tsx +++ b/assets/js/collaborative-editor/components/diagram/MiniHistory.tsx @@ -30,6 +30,7 @@ import { relativeLocale } from '../../../hooks'; import { duration } from '../../../utils/duration'; import truncateUid from '../../../utils/truncateUID'; import { useProject } from '../../hooks/useSessionContext'; +import { useVersionSelect } from '../../hooks/useVersionSelect'; import { useWorkflowState } from '../../hooks/useWorkflow'; import type { RunSummary, WorkOrder } from '../../types/history'; import { @@ -41,6 +42,8 @@ import { RunBadge } from '../common/RunBadge'; import { ShortcutKeys } from '../ShortcutKeys'; import { Tooltip } from '../Tooltip'; +import { VersionMismatchBanner } from './VersionMismatchBanner'; + // Extended types with selection state for UI type RunWithSelection = RunSummary & { selected?: boolean }; type WorkOrderWithSelection = Omit & { @@ -284,6 +287,11 @@ interface MiniHistoryProps { // New props for panel variant variant?: 'floating' | 'panel'; onBack?: () => void; + // Version mismatch detection + versionMismatch?: { + runVersion: number; + currentVersion: number; + } | null; } export default function MiniHistory({ @@ -298,6 +306,7 @@ export default function MiniHistory({ onRetry, variant = 'floating', onBack, + versionMismatch, }: MiniHistoryProps) { const [expandedWorder, setExpandedWorder] = useState(''); const now = new Date(); @@ -305,6 +314,14 @@ export default function MiniHistory({ // Get project and workflow IDs from state for navigation const project = useProject(); const workflow = useWorkflowState(state => state.workflow); + const handleVersionSelect = useVersionSelect(); + + // Handler to navigate to the run's version + const handleGoToVersion = () => { + if (versionMismatch) { + handleVersionSelect(versionMismatch.runVersion); + } + }; // Clear expanded work order when panel collapses React.useEffect(() => { @@ -532,6 +549,16 @@ export default function MiniHistory({
)} + {/* Version mismatch banner when collapsed */} + {collapsed && versionMismatch && ( + + )} +
)}
+ + {/* Version mismatch banner at bottom of panel */} + {!collapsed && versionMismatch && ( + + )} ); } diff --git a/assets/js/collaborative-editor/components/diagram/VersionMismatchBanner.tsx b/assets/js/collaborative-editor/components/diagram/VersionMismatchBanner.tsx index 4eb89b1bde..810664330b 100644 --- a/assets/js/collaborative-editor/components/diagram/VersionMismatchBanner.tsx +++ b/assets/js/collaborative-editor/components/diagram/VersionMismatchBanner.tsx @@ -9,58 +9,57 @@ * as the workflow structure may have changed between versions. * * Features: - * - Compact two-line design with version information - * - Dismissible via X button - * - Positioned at top-center of canvas + * - Compact design with version information + * - Action button to navigate to the correct version + * - Positioned at bottom of MiniHistory panel */ -import { useState } from 'react'; - import { cn } from '#/utils/cn'; +import { Tooltip } from '../Tooltip'; + interface VersionMismatchBannerProps { runVersion: number; currentVersion: number; + onGoToVersion: () => void; className?: string; + compact?: boolean; } export function VersionMismatchBanner({ runVersion, currentVersion, + onGoToVersion, className, + compact = false, }: VersionMismatchBannerProps) { - const [dismissed, setDismissed] = useState(false); - - if (dismissed) { - return null; - } + const tooltipText = `This run was executed on v${runVersion}, but you're visualizing it on v${currentVersion} of the workflow. Big things may have changed.`; return (
-
-
diff --git a/assets/js/collaborative-editor/hooks/useWorkflow.ts b/assets/js/collaborative-editor/hooks/useWorkflow.ts index 5efb77ddb4..7d0cba86b9 100644 --- a/assets/js/collaborative-editor/hooks/useWorkflow.ts +++ b/assets/js/collaborative-editor/hooks/useWorkflow.ts @@ -632,35 +632,30 @@ export const useWorkflowActions = () => { * - hasPermission: User has can_edit_workflow permission * - isConnected: Session is synced with backend * - isDeleted: Workflow has been deleted - * - isOldSnapshot: Viewing an old snapshot (not latest version) + * - isPinnedVersion: Viewing a pinned version (any ?v parameter in URL) * * @internal This is shared logic between useCanSave and useCanRun */ const useWorkflowConditions = () => { const { isSynced } = useSession(); const permissions = usePermissions(); - const latestSnapshotLockVersion = useLatestSnapshotLockVersion(); const workflow = useWorkflowState(state => state.workflow); + const { params } = useURLState(); const hasEditPermission = permissions?.can_edit_workflow ?? false; const hasRunPermission = permissions?.can_run_workflow ?? false; const isConnected = isSynced; const isDeleted = workflow !== null && workflow.deleted_at !== null; - // Only consider it an old snapshot if workflow is loaded, latest - // snapshot lock version is available AND different from workflow - // lock version - const isOldSnapshot = - workflow !== null && - latestSnapshotLockVersion !== null && - workflow.lock_version !== latestSnapshotLockVersion; + // Check if version is pinned via URL parameter + const isPinnedVersion = params['v'] !== undefined && params['v'] !== null; return { hasEditPermission, hasRunPermission, isConnected, isDeleted, - isOldSnapshot, + isPinnedVersion, }; }; @@ -674,11 +669,11 @@ const useWorkflowConditions = () => { * Checks: * 1. User permissions (can_edit_workflow) * 2. Connection state (isSynced) - * 3. Lock version (viewing latest snapshot) + * 3. Version pinning (any ?v parameter in URL) * 4. Workflow deletion state (deleted_at) */ export const useCanSave = (): { canSave: boolean; tooltipMessage: string } => { - const { hasEditPermission, isConnected, isDeleted, isOldSnapshot } = + const { hasEditPermission, isConnected, isDeleted, isPinnedVersion } = useWorkflowConditions(); // Determine tooltip message (check in priority order) @@ -694,9 +689,9 @@ export const useCanSave = (): { canSave: boolean; tooltipMessage: string } => { } else if (isDeleted) { canSave = false; tooltipMessage = 'Workflow has been deleted'; - } else if (isOldSnapshot) { + } else if (isPinnedVersion) { canSave = false; - tooltipMessage = 'You cannot edit an old snapshot of a workflow'; + tooltipMessage = 'You are viewing a pinned version of this workflow'; } return { canSave, tooltipMessage }; @@ -712,7 +707,7 @@ export const useCanSave = (): { canSave: boolean; tooltipMessage: string } => { * Checks: * 1. User permissions (can_edit_workflow or can_run_workflow) * 2. Connection state (isSynced) - * 3. Lock version (viewing latest snapshot) + * 3. Version pinning (any ?v parameter in URL) * 4. Workflow deletion state (deleted_at) * 5. Run limits (from session context) */ @@ -722,7 +717,7 @@ export const useCanRun = (): { canRun: boolean; tooltipMessage: string } => { hasRunPermission, isConnected, isDeleted, - isOldSnapshot, + isPinnedVersion, } = useWorkflowConditions(); // Get run limits from session context (defaults to allowed if missing) @@ -745,9 +740,9 @@ export const useCanRun = (): { canRun: boolean; tooltipMessage: string } => { } else if (isDeleted) { canRun = false; tooltipMessage = 'Workflow has been deleted'; - } else if (isOldSnapshot) { + } else if (isPinnedVersion) { canRun = false; - tooltipMessage = 'You cannot run an old snapshot of a workflow'; + tooltipMessage = 'You are viewing a pinned version of this workflow'; } else if (!runLimits.allowed && runLimits.message) { canRun = false; tooltipMessage = runLimits.message; @@ -766,7 +761,7 @@ export const useCanRun = (): { canRun: boolean; tooltipMessage: string } => { * Checks (in priority order): * 1. Workflow deletion state (deleted_at) * 2. User permissions (can_edit_workflow) - * 3. Snapshot version (viewing old snapshot) + * 3. Version pinning (any ?v parameter in URL) * * Note: Connection state does not affect read-only status. Offline editing * is fully supported - Y.Doc buffers transactions locally and syncs when @@ -778,8 +773,11 @@ export const useWorkflowReadOnly = (): { } => { // Get permissions and workflow state const permissions = usePermissions(); - const latestSnapshotLockVersion = useLatestSnapshotLockVersion(); const workflow = useWorkflowState(state => state.workflow); + const { params } = useURLState(); + + // Check if version is pinned via URL parameter + const isPinnedVersion = params['v'] !== undefined && params['v'] !== null; // Don't show read-only state until permissions are loaded // This prevents flickering during initial load @@ -790,12 +788,8 @@ export const useWorkflowReadOnly = (): { // Compute read-only conditions const hasPermission = permissions.can_edit_workflow; const isDeleted = workflow !== null && workflow.deleted_at !== null; - const isOldSnapshot = - workflow !== null && - latestSnapshotLockVersion !== null && - workflow.lock_version !== latestSnapshotLockVersion; - // Priority order: deleted > permissions > snapshot + // Priority order: deleted > permissions > pinned version if (isDeleted) { return { isReadOnly: true, @@ -808,10 +802,10 @@ export const useWorkflowReadOnly = (): { tooltipMessage: 'You do not have permission to edit this workflow', }; } - if (isOldSnapshot) { + if (isPinnedVersion) { return { isReadOnly: true, - tooltipMessage: 'You cannot edit or run an old snapshot of a workflow', + tooltipMessage: 'You are viewing a pinned version of this workflow', }; } diff --git a/assets/test/collaborative-editor/components/Header.keyboard.test.tsx b/assets/test/collaborative-editor/components/Header.keyboard.test.tsx index 15a952ca86..3093c5286a 100644 --- a/assets/test/collaborative-editor/components/Header.keyboard.test.tsx +++ b/assets/test/collaborative-editor/components/Header.keyboard.test.tsx @@ -23,12 +23,23 @@ import { StoreContext } from '../../../js/collaborative-editor/contexts/StorePro import { KeyboardProvider } from '../../../js/collaborative-editor/keyboard'; import type { CreateSessionContextOptions } from '../__helpers__/sessionContextFactory'; import { simulateStoreProviderWithConnection } from '../__helpers__/storeProviderHelpers'; +import { + createMockURLState, + getURLStateMockValue, +} from '../__helpers__/urlStateMocks'; import { createMinimalWorkflowYDoc } from '../__helpers__/workflowStoreHelpers'; // ============================================================================= // TEST MOCKS // ============================================================================= +// Mock useURLState +const urlState = createMockURLState(); + +vi.mock('../../../js/react/lib/use-url-state', () => ({ + useURLState: () => getURLStateMockValue(urlState), +})); + // Mock useAdaptorIcons to prevent async fetch warnings vi.mock('../../../js/workflow-diagram/useAdaptorIcons', () => ({ default: () => ({}), @@ -177,6 +188,9 @@ async function renderAndWaitForReady( // ============================================================================= describe('Header - Save Workflow (Cmd+S / Ctrl+S)', () => { + beforeEach(() => { + urlState.reset(); + }); beforeEach(() => { vi.clearAllMocks(); }); @@ -328,13 +342,15 @@ describe('Header - Save Workflow (Cmd+S / Ctrl+S)', () => { cleanup(); }); - test('Cmd+S does NOT call saveWorkflow when viewing old snapshot', async () => { + test('Cmd+S does NOT call saveWorkflow when viewing pinned version', async () => { const user = userEvent.setup(); + + // Set pinned version in URL + urlState.setParam('v', '1'); + const { wrapper, emitSessionContext, saveWorkflowSpy, cleanup } = await createTestSetup({ permissions: { can_edit_workflow: true, can_run_workflow: true }, - workflowLockVersion: 1, - latestSnapshotLockVersion: 2, }); const { unmount } = await renderAndWaitForReady( @@ -515,6 +531,10 @@ describe('Header - Save Workflow (Cmd+S / Ctrl+S)', () => { // ============================================================================= describe('Header - Save & Sync to GitHub (Cmd+Shift+S / Ctrl+Shift+S)', () => { + beforeEach(() => { + urlState.reset(); + }); + beforeEach(() => { vi.clearAllMocks(); }); @@ -645,14 +665,16 @@ describe('Header - Save & Sync to GitHub (Cmd+Shift+S / Ctrl+Shift+S)', () => { cleanup(); }); - test('Cmd+Shift+S does NOT open modal when viewing old snapshot', async () => { + test('Cmd+Shift+S does NOT open modal when viewing pinned version', async () => { const user = userEvent.setup(); + + // Set pinned version in URL + urlState.setParam('v', '1'); + const { wrapper, emitSessionContext, openGitHubSyncModalSpy, cleanup } = await createTestSetup({ permissions: { can_edit_workflow: true, can_run_workflow: true }, hasGithubConnection: true, - workflowLockVersion: 1, - latestSnapshotLockVersion: 2, }); const { unmount } = await renderAndWaitForReady( diff --git a/assets/test/collaborative-editor/components/ReadOnlyWarning.test.tsx b/assets/test/collaborative-editor/components/ReadOnlyWarning.test.tsx index 06ebba4e83..eac3c0092b 100644 --- a/assets/test/collaborative-editor/components/ReadOnlyWarning.test.tsx +++ b/assets/test/collaborative-editor/components/ReadOnlyWarning.test.tsx @@ -11,7 +11,7 @@ import { act, render, screen, waitFor } from '@testing-library/react'; import type React from 'react'; -import { describe, expect, test } from 'vitest'; +import { beforeEach, describe, expect, test, vi } from 'vitest'; import * as Y from 'yjs'; import { ReadOnlyWarning } from '../../../js/collaborative-editor/components/ReadOnlyWarning'; @@ -23,12 +23,23 @@ import { createSessionStore } from '../../../js/collaborative-editor/stores/crea import { createWorkflowStore } from '../../../js/collaborative-editor/stores/createWorkflowStore'; import type { Session } from '../../../js/collaborative-editor/types/session'; import { createSessionContext } from '../__helpers__/sessionContextFactory'; +import { + createMockURLState, + getURLStateMockValue, +} from '../__helpers__/urlStateMocks'; import { createMockPhoenixChannel, createMockPhoenixChannelProvider, } from '../mocks/phoenixChannel'; import { createMockSocket } from '../mocks/phoenixSocket'; +// Mock useURLState +const urlState = createMockURLState(); + +vi.mock('../../../js/react/lib/use-url-state', () => ({ + useURLState: () => getURLStateMockValue(urlState), +})); + // ============================================================================= // TEST HELPERS // ============================================================================= @@ -298,6 +309,10 @@ describe('ReadOnlyWarning - Styling', () => { // ============================================================================= describe('ReadOnlyWarning - Read-Only States', () => { + beforeEach(() => { + urlState.reset(); + }); + test('renders for deleted workflow', async () => { const { wrapper, emitSessionContext } = createTestSetup({ workflowDeletedAt: new Date().toISOString(), @@ -330,11 +345,11 @@ describe('ReadOnlyWarning - Read-Only States', () => { }); }); - test('renders for old snapshots', async () => { - const { wrapper, emitSessionContext } = createTestSetup({ - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, - }); + test('renders for pinned versions', async () => { + // Set pinned version in URL + urlState.setParam('v', '1'); + + const { wrapper, emitSessionContext } = createTestSetup({}); render(, { wrapper }); @@ -353,6 +368,10 @@ describe('ReadOnlyWarning - Read-Only States', () => { // ============================================================================= describe('ReadOnlyWarning - Dynamic Changes', () => { + beforeEach(() => { + urlState.reset(); + }); + test('appears when workflow becomes deleted', async () => { const { wrapper, emitSessionContext, ydoc } = createTestSetup({ permissions: { can_edit_workflow: true, can_run_workflow: true }, diff --git a/assets/test/collaborative-editor/components/VersionDropdown.test.tsx b/assets/test/collaborative-editor/components/VersionDropdown.test.tsx index 4dd65859fa..0fc85a542d 100644 --- a/assets/test/collaborative-editor/components/VersionDropdown.test.tsx +++ b/assets/test/collaborative-editor/components/VersionDropdown.test.tsx @@ -455,9 +455,10 @@ describe('VersionDropdown', () => { // Open dropdown await user.click(button); - // Should show formatted timestamp + // Should show formatted timestamp (appears twice: once for "latest" and once for "v2") const timestamp = new Date('2024-01-15T10:30:00Z').toLocaleString(); - expect(screen.getByText(timestamp)).toBeInTheDocument(); + const timestampElements = screen.getAllByText(timestamp); + expect(timestampElements.length).toBe(2); }); test('highlights currently selected version', async () => { @@ -540,8 +541,11 @@ describe('VersionDropdown', () => { const versionButtons = screen.getAllByRole('menuitem'); expect(versionButtons[0]).toHaveTextContent('latest'); - // Second item should show version number - expect(versionButtons[1]).toHaveTextContent('v4'); + // Second item should show version number (v5) + expect(versionButtons[1]).toHaveTextContent('v5'); + + // Third item should show version number (v4) + expect(versionButtons[2]).toHaveTextContent('v4'); }); }); @@ -827,9 +831,9 @@ describe('VersionDropdown', () => { // Open dropdown await user.click(button); - // All version items should have menuitem role + // All version items should have menuitem role (1 "latest" + 2 versions) const menuItems = screen.getAllByRole('menuitem'); - expect(menuItems).toHaveLength(2); + expect(menuItems).toHaveLength(3); }); }); }); diff --git a/assets/test/collaborative-editor/components/diagram/VersionMismatchBanner.test.tsx b/assets/test/collaborative-editor/components/diagram/VersionMismatchBanner.test.tsx index 7532cd595e..f84e886c1f 100644 --- a/assets/test/collaborative-editor/components/diagram/VersionMismatchBanner.test.tsx +++ b/assets/test/collaborative-editor/components/diagram/VersionMismatchBanner.test.tsx @@ -1,56 +1,67 @@ /** * VersionMismatchBanner Component Tests * - * Tests the dismissible version mismatch banner that appears when + * Tests the version mismatch banner that appears when * the canvas version differs from the selected run's version. */ import { fireEvent, render, screen } from '@testing-library/react'; -import { describe, expect, test } from 'vitest'; +import { describe, expect, test, vi } from 'vitest'; import { VersionMismatchBanner } from '../../../../js/collaborative-editor/components/diagram/VersionMismatchBanner'; describe('VersionMismatchBanner', () => { - test('displays version information and dismiss button', () => { - render(); + test('displays version information and action button when not compact', () => { + const onGoToVersion = vi.fn(); + render( + + ); // Check version info is displayed expect( - screen.getByText(/Canvas shows v155 \(Selected run: v159\)/) - ).toBeInTheDocument(); - expect( - screen.getByText(/Canvas layout may differ from actual run/) + screen.getByText(/This run took place on version 15/) ).toBeInTheDocument(); - // Check dismiss button is present - const dismissButton = screen.getByLabelText( - 'Dismiss version mismatch warning' - ); - expect(dismissButton).toBeInTheDocument(); + // Check action button is present + const actionButton = screen.getByRole('button', { + name: /View as executed/, + }); + expect(actionButton).toBeInTheDocument(); }); - test('dismisses banner when X button is clicked', () => { - const { container } = render( - + test('calls onGoToVersion when action button is clicked', () => { + const onGoToVersion = vi.fn(); + render( + ); - // Banner should be visible initially - expect( - screen.getByText(/Canvas shows v155 \(Selected run: v159\)/) - ).toBeInTheDocument(); - - // Click dismiss button - const dismissButton = screen.getByLabelText( - 'Dismiss version mismatch warning' - ); - fireEvent.click(dismissButton); + // Click action button + const actionButton = screen.getByRole('button', { + name: /View as executed/, + }); + fireEvent.click(actionButton); - // Banner should be removed from DOM - expect(container.firstChild).toBeNull(); + // Handler should be called + expect(onGoToVersion).toHaveBeenCalledTimes(1); }); - test('shows information icon instead of warning icon', () => { - render(); + test('shows information icon', () => { + const onGoToVersion = vi.fn(); + render( + + ); // Check for info icon (hero-information-circle) const icon = document.querySelector('.hero-information-circle'); @@ -58,10 +69,12 @@ describe('VersionMismatchBanner', () => { }); test('applies custom className', () => { + const onGoToVersion = vi.fn(); const { container } = render( ); @@ -69,4 +82,44 @@ describe('VersionMismatchBanner', () => { const banner = container.firstChild as HTMLElement; expect(banner).toHaveClass('custom-class'); }); + + test('hides version text when compact', () => { + const onGoToVersion = vi.fn(); + render( + + ); + + // Version text should not be present in compact mode + expect( + screen.queryByText(/This run took place on version 15/) + ).not.toBeInTheDocument(); + + // But button should still be present + const actionButton = screen.getByRole('button', { + name: /View as executed/, + }); + expect(actionButton).toBeInTheDocument(); + }); + + test('shows version text when not compact', () => { + const onGoToVersion = vi.fn(); + render( + + ); + + // Version text should be visible + expect( + screen.getByText(/This run took place on version 15/) + ).toBeInTheDocument(); + }); }); diff --git a/assets/test/collaborative-editor/hooks/useWorkflowReadOnly.test.tsx b/assets/test/collaborative-editor/hooks/useWorkflowReadOnly.test.tsx index e0343c115a..0c95b68dd4 100644 --- a/assets/test/collaborative-editor/hooks/useWorkflowReadOnly.test.tsx +++ b/assets/test/collaborative-editor/hooks/useWorkflowReadOnly.test.tsx @@ -2,12 +2,12 @@ * useWorkflowReadOnly Hook Tests * * Tests for the read-only state hook that determines if a workflow - * should be editable based on deletion state, permissions, and snapshot version. + * should be editable based on deletion state, permissions, and version pinning. */ import { act, renderHook, waitFor } from '@testing-library/react'; import type React from 'react'; -import { describe, expect, test } from 'vitest'; +import { beforeEach, describe, expect, test, vi } from 'vitest'; import * as Y from 'yjs'; import { SessionContext } from '../../../js/collaborative-editor/contexts/SessionProvider'; @@ -24,12 +24,23 @@ import { createSessionContext, mockPermissions, } from '../__helpers__/sessionContextFactory'; +import { + createMockURLState, + getURLStateMockValue, +} from '../__helpers__/urlStateMocks'; import { createMockPhoenixChannel, createMockPhoenixChannelProvider, } from '../mocks/phoenixChannel'; import { createMockSocket } from '../mocks/phoenixSocket'; +// Mock useURLState +const urlState = createMockURLState(); + +vi.mock('../../../js/react/lib/use-url-state', () => ({ + useURLState: () => getURLStateMockValue(urlState), +})); + // ============================================================================= // TEST HELPERS // ============================================================================= @@ -144,6 +155,14 @@ function createWrapper(options: WrapperOptions = {}): [ ]; } +// ============================================================================= +// SETUP +// ============================================================================= + +beforeEach(() => { + urlState.reset(); +}); + // ============================================================================= // DELETED WORKFLOW TESTS // ============================================================================= @@ -189,11 +208,12 @@ describe('useWorkflowReadOnly - Deleted Workflow', () => { }); }); - test('deleted state takes priority over old snapshot', async () => { + test('deleted state takes priority over pinned version', async () => { + // Set pinned version in URL + urlState.setParam('v', '1'); + const [wrapper, { emitSessionContext }] = createWrapper({ permissions: { can_edit_workflow: true, can_run_workflow: true }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: new Date().toISOString(), }); @@ -237,11 +257,12 @@ describe('useWorkflowReadOnly - Permissions', () => { }); }); - test('permission restriction takes priority over old snapshot', async () => { + test('permission restriction takes priority over pinned version', async () => { + // Set pinned version in URL + urlState.setParam('v', '1'); + const [wrapper, { emitSessionContext }] = createWrapper({ permissions: { can_edit_workflow: false, can_run_workflow: false }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: null, }); @@ -261,15 +282,16 @@ describe('useWorkflowReadOnly - Permissions', () => { }); // ============================================================================= -// SNAPSHOT VERSION TESTS +// VERSION PINNING TESTS // ============================================================================= -describe('useWorkflowReadOnly - Snapshot Version', () => { - test('returns read-only true with snapshot message for old snapshot', async () => { +describe('useWorkflowReadOnly - Version Pinning', () => { + test('returns read-only true with pinned version message when ?v parameter is present', async () => { + // Set pinned version in URL + urlState.setParam('v', '1'); + const [wrapper, { emitSessionContext }] = createWrapper({ permissions: { can_edit_workflow: true, can_run_workflow: true }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: null, }); @@ -282,16 +304,15 @@ describe('useWorkflowReadOnly - Snapshot Version', () => { await waitFor(() => { expect(result.current.isReadOnly).toBe(true); expect(result.current.tooltipMessage).toBe( - 'You cannot edit or run an old snapshot of a workflow' + 'You are viewing a pinned version of this workflow' ); }); }); - test('returns not read-only when viewing latest snapshot', async () => { + test('returns not read-only when no ?v parameter is present', async () => { + // No version parameter in URL (editing latest) const [wrapper, { emitSessionContext }] = createWrapper({ permissions: { can_edit_workflow: true, can_run_workflow: true }, - latestSnapshotLockVersion: 1, - workflowLockVersion: 1, workflowDeletedAt: null, }); @@ -485,12 +506,12 @@ describe('useWorkflowReadOnly - Edge Cases', () => { // ============================================================================= describe('useWorkflowReadOnly - Priority Order', () => { - test('verifies complete priority order: deleted > permissions > snapshot', async () => { + test('verifies complete priority order: deleted > permissions > pinned version', async () => { // Test 1: All three conditions apply - deleted takes priority + urlState.setParam('v', '1'); + const [wrapper1, { emitSessionContext: emit1 }] = createWrapper({ permissions: { can_edit_workflow: false, can_run_workflow: false }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: new Date().toISOString(), }); @@ -508,11 +529,12 @@ describe('useWorkflowReadOnly - Priority Order', () => { ); }); - // Test 2: Permission and snapshot conditions apply - permission takes priority + // Test 2: Permission and pinned version conditions apply - permission takes priority + urlState.reset(); + urlState.setParam('v', '1'); + const [wrapper2, { emitSessionContext: emit2 }] = createWrapper({ permissions: { can_edit_workflow: false, can_run_workflow: false }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: null, }); @@ -530,11 +552,12 @@ describe('useWorkflowReadOnly - Priority Order', () => { ); }); - // Test 3: Only snapshot condition applies + // Test 3: Only pinned version condition applies + urlState.reset(); + urlState.setParam('v', '1'); + const [wrapper3, { emitSessionContext: emit3 }] = createWrapper({ permissions: { can_edit_workflow: true, can_run_workflow: true }, - latestSnapshotLockVersion: 2, - workflowLockVersion: 1, workflowDeletedAt: null, }); @@ -548,7 +571,7 @@ describe('useWorkflowReadOnly - Priority Order', () => { await waitFor(() => { expect(result3.current.tooltipMessage).toBe( - 'You cannot edit or run an old snapshot of a workflow' + 'You are viewing a pinned version of this workflow' ); }); }); diff --git a/lib/lightning_web/live/run_live/show.ex b/lib/lightning_web/live/run_live/show.ex index 01cb54979b..a89a2ad346 100644 --- a/lib/lightning_web/live/run_live/show.ex +++ b/lib/lightning_web/live/run_live/show.ex @@ -99,7 +99,12 @@ defmodule LightningWeb.RunLive.Show do <:value> <.link navigate={ - ~p"/projects/#{@project}/w/#{@workflow.id}?a=#{run.id}&v=#{run.snapshot.lock_version}" + # Only include version param if snapshot differs from current workflow version + if run.snapshot.lock_version == @workflow.lock_version do + ~p"/projects/#{@project}/w/#{@workflow.id}?a=#{run.id}" + else + ~p"/projects/#{@project}/w/#{@workflow.id}?a=#{run.id}&v=#{run.snapshot.lock_version}" + end } class="link text-ellipsis" > diff --git a/lib/lightning_web/live/run_live/workorder_component.ex b/lib/lightning_web/live/run_live/workorder_component.ex index dc41046485..ffe08a4102 100644 --- a/lib/lightning_web/live/run_live/workorder_component.ex +++ b/lib/lightning_web/live/run_live/workorder_component.ex @@ -180,7 +180,13 @@ defmodule LightningWeb.RunLive.WorkOrderComponent do <%= if @last_run do %> <.link navigate={ - ~p"/projects/#{@project}/w/#{@work_order.workflow.id}?a=#{@last_run.id}&v=#{@work_order.snapshot.lock_version}" + # Only include version param if snapshot differs from current workflow version + if @work_order.workflow.lock_version == + @work_order.snapshot.lock_version do + ~p"/projects/#{@project}/w/#{@work_order.workflow.id}?a=#{@last_run.id}" + else + ~p"/projects/#{@project}/w/#{@work_order.workflow.id}?a=#{@last_run.id}&v=#{@work_order.snapshot.lock_version}" + end } class="inline-block" > diff --git a/test/lightning_web/live/run_live/show_test.exs b/test/lightning_web/live/run_live/show_test.exs index 558b6a8118..f97de72b39 100644 --- a/test/lightning_web/live/run_live/show_test.exs +++ b/test/lightning_web/live/run_live/show_test.exs @@ -232,6 +232,70 @@ defmodule LightningWeb.RunLive.ShowTest do |> render_async() end + describe "workflow link" do + setup :register_and_log_in_user + setup :create_project_for_current_user + + test "excludes version param when run snapshot matches current workflow", %{ + conn: conn, + project: project + } do + workflow = insert(:simple_workflow, project: project) |> with_snapshot() + %{triggers: [%{id: webhook_trigger_id}]} = workflow + + # Post to webhook to create a run + assert %{"work_order_id" => wo_id} = + post(conn, "/i/#{webhook_trigger_id}", %{"x" => 1}) + |> json_response(200) + + %{runs: [%{id: run_id}]} = WorkOrders.get(wo_id, include: [:runs]) + + {:ok, view, _html} = live(conn, ~p"/projects/#{project.id}/runs/#{run_id}") + + html = view |> element("#run-detail-#{run_id}") |> render_async() + + # Find the workflow link - should NOT include version param + assert html =~ + ~r/href="\/projects\/#{project.id}\/w\/#{workflow.id}\?a=#{run_id}"/ + + refute html =~ ~r/&v=/ + end + + test "includes version param when run snapshot differs from current workflow", + %{ + conn: conn, + project: project + } do + # Create workflow with initial snapshot + workflow = + insert(:simple_workflow, project: project, lock_version: 1) + |> with_snapshot() + + %{triggers: [%{id: webhook_trigger_id}]} = workflow + + # Post to webhook to create a run on v1 + assert %{"work_order_id" => wo_id} = + post(conn, "/i/#{webhook_trigger_id}", %{"x" => 1}) + |> json_response(200) + + %{runs: [%{id: run_id}], snapshot: snapshot} = + WorkOrders.get(wo_id, include: [:runs, :snapshot]) + + # Update workflow to v2 (simulating a change after the run was created) + workflow = + Lightning.Repo.update!(Ecto.Changeset.change(workflow, lock_version: 2)) + + {:ok, view, _html} = live(conn, ~p"/projects/#{project.id}/runs/#{run_id}") + + html = view |> element("#run-detail-#{run_id}") |> render_async() + + # Find the workflow link - should include version param + # Note: & is HTML-escaped as & in rendered output + assert html =~ + ~r/href="\/projects\/#{project.id}\/w\/#{workflow.id}\?a=#{run_id}&v=#{snapshot.lock_version}"/ + end + end + defp select_step(view, run, job_name) do view |> element("#step-list-#{run.id} a[data-phx-link]", job_name)