From 3bd3f1bee89525ed770877858677282e28fa10c1 Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 00:12:01 +0000 Subject: [PATCH 1/6] fix: BROS-775: Show original urls in task code --- label_studio/tasks/api.py | 10 +- label_studio/tasks/tests/test_api.py | 113 +++++++ .../src/components/Common/Table/Table.jsx | 14 +- .../TaskSourceViewer.test.tsx | 290 ++++++++++++++++++ .../TaskSourceViewer/TaskSourceViewer.tsx | 96 +++++- .../TaskSourceViewer/ViewToggle.test.tsx | 162 ++++++++++ .../Common/TaskSourceViewer/ViewToggle.tsx | 50 ++- 7 files changed, 713 insertions(+), 22 deletions(-) create mode 100644 web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx create mode 100644 web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx diff --git a/label_studio/tasks/api.py b/label_studio/tasks/api.py index b6adbeddf88a..6b7dd9fd5ad6 100644 --- a/label_studio/tasks/api.py +++ b/label_studio/tasks/api.py @@ -303,10 +303,18 @@ def prefetch(self, queryset): ) def get_retrieve_serializer_context(self, request): + """Build serializer context for task retrieval. + + The resolve_uri parameter controls whether storage URLs (e.g., s3://bucket/file.jpg) + are converted to proxy URLs (/tasks//resolve/?fileuri=...). This is useful for: + - resolve_uri=True (default): URLs are proxied through Label Studio for security + - resolve_uri=False: Original storage URLs are preserved, useful for debugging + or when users need to see the actual source paths in task preview + """ fields = ['drafts', 'predictions', 'annotations'] return { - 'resolve_uri': True, + 'resolve_uri': bool_from_request(request.GET, 'resolve_uri', True), 'predictions': 'predictions' in fields, 'annotations': 'annotations' in fields, 'drafts': 'drafts' in fields, diff --git a/label_studio/tasks/tests/test_api.py b/label_studio/tasks/tests/test_api.py index e9689d481827..36401646dc3c 100644 --- a/label_studio/tasks/tests/test_api.py +++ b/label_studio/tasks/tests/test_api.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from organizations.tests.factories import OrganizationFactory from projects.tests.factories import ProjectFactory from rest_framework.test import APITestCase @@ -123,3 +125,114 @@ def test_create_task_with_project_id_succeeds(self): response_data = response.json() assert response_data['project'] == self.project.id assert response_data['data'] == {'text': 'test task'} + + +class TestTaskAPIResolveUri(APITestCase): + """Tests for resolve_uri query parameter in task detail endpoint. + + The resolve_uri parameter controls whether storage URLs (e.g., s3://bucket/file.jpg) + are converted to proxy URLs. This is useful for debugging and viewing original + source paths in task preview. + """ + + @classmethod + def setUpTestData(cls): + cls.organization = OrganizationFactory() + cls.project = ProjectFactory(organization=cls.organization) + cls.user = cls.organization.created_by + + def test_get_task_resolve_uri_default_true(self): + """Test that resolve_uri defaults to True when not specified. + + This test validates: + - Creating a task with a storage-like URL in data + - Fetching the task without resolve_uri parameter + - Verifying that Task.resolve_uri method is called (default behavior) + + Critical validation: By default, URLs should be resolved for security, + preventing direct exposure of storage credentials. + """ + task = TaskFactory(project=self.project, data={'image': 's3://bucket/image.jpg'}) + self.client.force_authenticate(user=self.user) + + # Patch resolve_uri to track if it's called + with patch.object(task.__class__, 'resolve_uri', return_value={'image': '/resolved/url'}) as mock_resolve: + response = self.client.get(f'/api/tasks/{task.id}/') + + assert response.status_code == 200 + # resolve_uri should be called by default + mock_resolve.assert_called_once() + + def test_get_task_resolve_uri_explicit_true(self): + """Test that resolve_uri=true explicitly enables URL resolution. + + This test validates: + - Creating a task with a storage-like URL in data + - Fetching the task with resolve_uri=true + - Verifying that Task.resolve_uri method is called + + Critical validation: Explicit resolve_uri=true should resolve URLs. + """ + task = TaskFactory(project=self.project, data={'image': 's3://bucket/image.jpg'}) + self.client.force_authenticate(user=self.user) + + with patch.object(task.__class__, 'resolve_uri', return_value={'image': '/resolved/url'}) as mock_resolve: + response = self.client.get(f'/api/tasks/{task.id}/?resolve_uri=true') + + assert response.status_code == 200 + mock_resolve.assert_called_once() + + def test_get_task_resolve_uri_false_preserves_original_urls(self): + """Test that resolve_uri=false preserves original storage URLs. + + This test validates: + - Creating a task with a storage-like URL in data + - Fetching the task with resolve_uri=false + - Verifying that Task.resolve_uri method is NOT called + - Original URL is preserved in the response + + Critical validation: When resolve_uri=false, users should see original + storage URLs (e.g., s3://bucket/file.jpg) for debugging purposes. + """ + original_url = 's3://my-bucket/path/to/image.jpg' + task = TaskFactory(project=self.project, data={'image': original_url, 'text': 'test'}) + self.client.force_authenticate(user=self.user) + + with patch.object(task.__class__, 'resolve_uri') as mock_resolve: + response = self.client.get(f'/api/tasks/{task.id}/?resolve_uri=false') + + assert response.status_code == 200 + # resolve_uri should NOT be called when resolve_uri=false + mock_resolve.assert_not_called() + # Original URL should be preserved + assert response.json()['data']['image'] == original_url + assert response.json()['data']['text'] == 'test' + + def test_get_task_resolve_uri_false_with_multiple_url_fields(self): + """Test resolve_uri=false with multiple URL fields in task data. + + This test validates: + - Creating a task with multiple storage URLs + - Fetching with resolve_uri=false + - All original URLs are preserved + + Critical validation: All URL fields should preserve their original values. + """ + task_data = { + 'image_1': 's3://bucket-1/image1.jpg', + 'image_2': 'gs://bucket-2/image2.png', + 'audio': 'azure-blob://container/audio.mp3', + 'text': 'Plain text field', + } + task = TaskFactory(project=self.project, data=task_data) + self.client.force_authenticate(user=self.user) + + response = self.client.get(f'/api/tasks/{task.id}/?resolve_uri=false') + + assert response.status_code == 200 + response_data = response.json()['data'] + # All original URLs should be preserved + assert response_data['image_1'] == 's3://bucket-1/image1.jpg' + assert response_data['image_2'] == 'gs://bucket-2/image2.png' + assert response_data['audio'] == 'azure-blob://container/audio.mp3' + assert response_data['text'] == 'Plain text field' diff --git a/web/libs/datamanager/src/components/Common/Table/Table.jsx b/web/libs/datamanager/src/components/Common/Table/Table.jsx index f85ec0cc5555..e2a9ddbe6337 100644 --- a/web/libs/datamanager/src/components/Common/Table/Table.jsx +++ b/web/libs/datamanager/src/components/Common/Table/Table.jsx @@ -158,11 +158,21 @@ export const Table = observer( predictions: out?.predictions, }; - const onTaskLoad = async () => { + /** + * Load full task data from API. + * @param {Object} options - Load options + * @param {boolean} [options.resolveUri=false] - Whether to resolve storage URLs to proxy URLs. + * When false, original storage URLs (s3://..., gs://...) are preserved for debugging. + * When true, URLs are converted to proxy URLs (/tasks/.../resolve/?fileuri=...). + */ + const onTaskLoad = async (options = {}) => { if (isFF(FF_LOPS_E_3) && type === "DE") { return new Promise((resolve) => resolve(out)); } - const response = await api.task({ taskID: out.id }); + const response = await api.task({ + taskID: out.id, + resolve_uri: options.resolveUri ?? false, + }); return response ?? {}; }; diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx new file mode 100644 index 000000000000..127e9cf0fafd --- /dev/null +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx @@ -0,0 +1,290 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import "@testing-library/jest-dom"; +import { TaskSourceViewer, type TaskLoadOptions } from "./TaskSourceViewer"; + +// Mock feature flags +jest.mock("../../../utils/feature-flags", () => ({ + FF_LOPS_E_3: "ff_lops_e_3", + FF_INTERACTIVE_JSON_VIEWER: "ff_interactive_json_viewer", + isFF: (flag: string) => flag === "ff_interactive_json_viewer", +})); + +// Mock UI components +jest.mock("@humansignal/ui", () => ({ + JsonViewer: ({ data }: any) => ( +
{JSON.stringify(data)}
+ ), + Tabs: ({ children, value, onValueChange }: any) => ( +
{ + const target = e.target as HTMLElement; + if (target.dataset.value) { + onValueChange(target.dataset.value); + } + }}> + {children} +
+ ), + TabsList: ({ children }: any) =>
{children}
, + TabsTrigger: ({ children, value }: any) => ( + + ), + Toggle: ({ label, checked, onChange }: any) => ( + + ), +})); + +// Mock CodeView component +jest.mock("./CodeView", () => ({ + CodeView: ({ data }: any) => ( + 
{JSON.stringify(data, null, 2)}
+ ), +})); + +// Mock styles +jest.mock("./TaskSourceViewer.module.scss", () => ({ + taskSourceView: "taskSourceView", + viewContent: "viewContent", +})); + +describe("TaskSourceViewer Component", () => { + const mockTaskData = { + id: 123, + data: { + image: "s3://bucket/image.jpg", + text: "Sample text", + }, + annotations: [], + predictions: [], + }; + + const defaultProps = { + content: { id: 123, data: {} }, + onTaskLoad: jest.fn().mockResolvedValue(mockTaskData), + storageKey: "test:tasksource", + }; + + beforeEach(() => { + jest.clearAllMocks(); + localStorage.clear(); + }); + + describe("Initial Load", () => { + it("should load task data on mount with default resolveUri=false", async () => { + const mockOnTaskLoad = jest.fn().mockResolvedValue(mockTaskData); + + render(); + + await waitFor(() => { + expect(mockOnTaskLoad).toHaveBeenCalledTimes(1); + expect(mockOnTaskLoad).toHaveBeenCalledWith({ resolveUri: false }); + }); + }); + + it("should display task data after loading", async () => { + render(); + + await waitFor(() => { + expect(screen.getByTestId("code-view")).toHaveTextContent("s3://bucket/image.jpg"); + }); + }); + + it("should respect stored resolveUrls preference from localStorage", async () => { + localStorage.setItem("test:tasksource:resolveUrls", "true"); + const mockOnTaskLoad = jest.fn().mockResolvedValue(mockTaskData); + + render(); + + await waitFor(() => { + expect(mockOnTaskLoad).toHaveBeenCalledWith({ resolveUri: true }); + }); + }); + }); + + describe("Resolve URLs Toggle", () => { + it("should reload task data when resolve URLs toggle changes", async () => { + const user = userEvent.setup(); + const mockOnTaskLoad = jest.fn().mockResolvedValue(mockTaskData); + const mockRenderToggle = jest.fn(); + + render( + + ); + + // Wait for initial load + await waitFor(() => { + expect(mockOnTaskLoad).toHaveBeenCalledWith({ resolveUri: false }); + }); + + // The ViewToggle should be rendered via renderToggle callback + await waitFor(() => { + expect(mockRenderToggle).toHaveBeenCalled(); + }); + + // Get the toggle from the last call to renderToggle + const lastCall = mockRenderToggle.mock.calls[mockRenderToggle.mock.calls.length - 1]; + const toggleComponent = lastCall[0]; + + // Render the toggle to interact with it + const { getByTestId } = render(toggleComponent); + const toggle = getByTestId("resolve-urls-toggle"); + + // Click the toggle to enable URL resolution + await user.click(toggle); + + // Should reload with resolveUri: true + await waitFor(() => { + expect(mockOnTaskLoad).toHaveBeenCalledWith({ resolveUri: true }); + }); + }); + + it("should save resolve URLs preference to localStorage", async () => { + const user = userEvent.setup(); + const mockRenderToggle = jest.fn(); + + render( + + ); + + await waitFor(() => { + expect(mockRenderToggle).toHaveBeenCalled(); + }); + + // Get and render the toggle + const lastCall = mockRenderToggle.mock.calls[mockRenderToggle.mock.calls.length - 1]; + const { getByTestId } = render(lastCall[0]); + + await user.click(getByTestId("resolve-urls-toggle")); + + await waitFor(() => { + expect(localStorage.getItem("test:tasksource:resolveUrls")).toBe("true"); + }); + }); + }); + + describe("View Mode Toggle", () => { + it("should default to code view", async () => { + render(); + + await waitFor(() => { + expect(screen.getByTestId("code-view")).toBeInTheDocument(); + }); + }); + + it("should respect stored view preference from localStorage", async () => { + localStorage.setItem("test:tasksource:view", "interactive"); + + render(); + + await waitFor(() => { + expect(screen.getByTestId("json-viewer")).toBeInTheDocument(); + }); + }); + + it("should save view preference to localStorage when changed", async () => { + const user = userEvent.setup(); + const mockRenderToggle = jest.fn(); + + render( + + ); + + await waitFor(() => { + expect(mockRenderToggle).toHaveBeenCalled(); + }); + + // Get and render the toggle + const lastCall = mockRenderToggle.mock.calls[mockRenderToggle.mock.calls.length - 1]; + const { getByTestId } = render(lastCall[0]); + + await user.click(getByTestId("tab-interactive")); + + await waitFor(() => { + expect(localStorage.getItem("test:tasksource:view")).toBe("interactive"); + }); + }); + }); + + describe("Data Explorer Mode", () => { + it("should not include annotations/predictions for Data Explorer", async () => { + const mockOnTaskLoad = jest.fn().mockResolvedValue({ + ...mockTaskData, + annotations: [{ id: 1 }], + predictions: [{ id: 2 }], + }); + + render( + + ); + + await waitFor(() => { + const codeView = screen.getByTestId("code-view"); + expect(codeView).not.toHaveTextContent("annotations"); + expect(codeView).not.toHaveTextContent("predictions"); + }); + }); + }); + + describe("Error Handling", () => { + it("should handle API errors gracefully", async () => { + const mockOnTaskLoad = jest.fn().mockRejectedValue(new Error("API Error")); + + // Should not throw + expect(() => { + render(); + }).not.toThrow(); + }); + }); + + describe("renderToggle Callback", () => { + it("should call renderToggle with ViewToggle component when interactive viewer is enabled", async () => { + const mockRenderToggle = jest.fn(); + + render( + + ); + + await waitFor(() => { + expect(mockRenderToggle).toHaveBeenCalled(); + // The argument should be a React element (ViewToggle) + const toggleElement = mockRenderToggle.mock.calls[0][0]; + expect(toggleElement).toBeTruthy(); + }); + }); + + it("should not call renderToggle when not provided", async () => { + // This test verifies no errors occur when renderToggle is undefined + expect(() => { + render(); + }).not.toThrow(); + }); + }); +}); diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx index d340f4a129d2..b7cd8cae189f 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx @@ -1,4 +1,4 @@ -import { type FC, useEffect, useState, useCallback } from "react"; +import { type FC, useEffect, useState, useCallback, useRef } from "react"; import { JsonViewer, type FilterConfig } from "@humansignal/ui"; import { FF_LOPS_E_3, FF_INTERACTIVE_JSON_VIEWER, isFF } from "../../../utils/feature-flags"; import { CodeView } from "./CodeView"; @@ -7,11 +7,21 @@ import { ViewToggle, type ViewMode } from "./ViewToggle"; export type { ViewMode }; +/** Options passed to onTaskLoad callback */ +export interface TaskLoadOptions { + /** Whether to resolve storage URLs to proxy URLs (default: false) */ + resolveUri?: boolean; +} + export interface TaskSourceViewerProps { /** Task content data */ content: any; - /** Function to load full task data */ - onTaskLoad: () => Promise; + /** + * Function to load full task data. + * @param options - Options including resolveUri to control URL resolution + * @returns Promise with task data + */ + onTaskLoad: (options?: TaskLoadOptions) => Promise; /** SDK type (e.g., "DE" for Data Explorer) */ sdkType?: string; /** Storage key for localStorage persistence */ @@ -53,6 +63,10 @@ const TASK_SOURCE_FILTERS: FilterConfig[] = [ * * Loads task data and provides either code view or interactive JSON viewer. * Specific to the Data Manager and should not be part of the reusable UI library. + * + * Features: + * - Code/Interactive view toggle for different JSON display modes + * - Resolve URLs toggle to show original storage URLs (s3://...) or resolved proxy URLs */ export const TaskSourceViewer: FC = ({ content, @@ -64,12 +78,21 @@ export const TaskSourceViewer: FC = ({ const isInteractiveViewerEnabled = isFF(FF_INTERACTIVE_JSON_VIEWER); const [taskData, setTaskData] = useState(content); + const [isLoading, setIsLoading] = useState(false); + + // Track if this is the initial load to avoid double-fetching + const isInitialLoadRef = useRef(true); // Manage view state internally const [view, setView] = useState(() => storageKey ? (localStorage.getItem(`${storageKey}:view`) as ViewMode) || "code" : "code", ); + // Manage resolve URLs state - default OFF to show original storage URLs + const [resolveUrls, setResolveUrls] = useState(() => + storageKey ? localStorage.getItem(`${storageKey}:resolveUrls`) === "true" : false, + ); + const handleViewChange = useCallback( (newView: ViewMode) => { setView(newView); @@ -82,9 +105,24 @@ export const TaskSourceViewer: FC = ({ [storageKey], ); - // Load full task data - useEffect(() => { - onTaskLoad().then((response) => { + const handleResolveUrlsChange = useCallback( + (newResolveUrls: boolean) => { + setResolveUrls(newResolveUrls); + + // Save to localStorage + if (storageKey) { + localStorage.setItem(`${storageKey}:resolveUrls`, String(newResolveUrls)); + } + }, + [storageKey], + ); + + /** + * Format raw API response into display format. + * Strips annotations/predictions for Data Explorer mode. + */ + const formatTaskData = useCallback( + (response: any) => { const formatted: any = { id: response.id, data: response.data, @@ -100,16 +138,52 @@ export const TaskSourceViewer: FC = ({ formatted.state = response.state; } - setTaskData(formatted); - }); - }, [onTaskLoad, sdkType]); + return formatted; + }, + [sdkType], + ); + + /** + * Load task data from API with current resolveUrls setting. + */ + const loadTaskData = useCallback(async () => { + setIsLoading(true); + try { + const response = await onTaskLoad({ resolveUri: resolveUrls }); + setTaskData(formatTaskData(response)); + } finally { + setIsLoading(false); + } + }, [onTaskLoad, resolveUrls, formatTaskData]); + + // Initial load + useEffect(() => { + if (isInitialLoadRef.current) { + isInitialLoadRef.current = false; + loadTaskData(); + } + }, [loadTaskData]); + + // Reload when resolveUrls changes (but not on initial load) + useEffect(() => { + if (!isInitialLoadRef.current) { + loadTaskData(); + } + }, [resolveUrls]); // eslint-disable-line react-hooks/exhaustive-deps // Provide toggle to external render location (e.g., modal header) useEffect(() => { if (renderToggle && isInteractiveViewerEnabled) { - renderToggle(); + renderToggle( + , + ); } - }, [renderToggle, view, handleViewChange, isInteractiveViewerEnabled]); + }, [renderToggle, view, handleViewChange, resolveUrls, handleResolveUrlsChange, isInteractiveViewerEnabled]); return (
diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx new file mode 100644 index 000000000000..b14146f5e87e --- /dev/null +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx @@ -0,0 +1,162 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import "@testing-library/jest-dom"; +import { ViewToggle } from "./ViewToggle"; + +// Mock the UI components +jest.mock("@humansignal/ui", () => ({ + Tabs: ({ children, value, onValueChange }: any) => ( +
{ + const target = e.target as HTMLElement; + if (target.dataset.value) { + onValueChange(target.dataset.value); + } + }}> + {children} +
+ ), + TabsList: ({ children, className }: any) => ( +
+ {children} +
+ ), + TabsTrigger: ({ children, value }: any) => ( + + ), + Toggle: ({ label, checked, onChange }: any) => ( + + ), +})); + +describe("ViewToggle Component", () => { + const defaultProps = { + view: "code" as const, + onViewChange: jest.fn(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe("View Mode Toggle", () => { + it("should render code and interactive view tabs", () => { + render(); + + expect(screen.getByTestId("tab-code")).toBeInTheDocument(); + expect(screen.getByTestId("tab-interactive")).toBeInTheDocument(); + expect(screen.getByTestId("tab-code")).toHaveTextContent("Code"); + expect(screen.getByTestId("tab-interactive")).toHaveTextContent("Interactive"); + }); + + it("should call onViewChange when switching view modes", async () => { + const user = userEvent.setup(); + const mockOnViewChange = jest.fn(); + + render(); + + await user.click(screen.getByTestId("tab-interactive")); + + expect(mockOnViewChange).toHaveBeenCalledWith("interactive"); + }); + + it("should reflect current view value in tabs", () => { + render(); + + expect(screen.getByTestId("tabs")).toHaveAttribute("data-value", "interactive"); + }); + }); + + describe("Resolve URLs Toggle", () => { + it("should not render resolve URLs toggle when onResolveUrlsChange is not provided", () => { + render(); + + expect(screen.queryByTestId("resolve-urls-toggle")).not.toBeInTheDocument(); + }); + + it("should render resolve URLs toggle when onResolveUrlsChange is provided", () => { + render( + + ); + + expect(screen.getByTestId("resolve-urls-toggle")).toBeInTheDocument(); + expect(screen.getByText("Resolve URLs")).toBeInTheDocument(); + }); + + it("should reflect resolveUrls state in toggle", () => { + render( + + ); + + expect(screen.getByTestId("resolve-urls-toggle")).toBeChecked(); + }); + + it("should call onResolveUrlsChange when toggle is clicked", async () => { + const user = userEvent.setup(); + const mockOnResolveUrlsChange = jest.fn(); + + render( + + ); + + await user.click(screen.getByTestId("resolve-urls-toggle")); + + expect(mockOnResolveUrlsChange).toHaveBeenCalledWith(true); + }); + + it("should call onResolveUrlsChange with false when toggle is unchecked", async () => { + const user = userEvent.setup(); + const mockOnResolveUrlsChange = jest.fn(); + + render( + + ); + + await user.click(screen.getByTestId("resolve-urls-toggle")); + + expect(mockOnResolveUrlsChange).toHaveBeenCalledWith(false); + }); + }); + + describe("Layout", () => { + it("should render both toggles when all props are provided", () => { + render( + + ); + + // View mode tabs should be present + expect(screen.getByTestId("tabs")).toBeInTheDocument(); + // Resolve URLs toggle should be present + expect(screen.getByTestId("resolve-urls-toggle")).toBeInTheDocument(); + }); + }); +}); diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.tsx index fb0e6cd3739c..4d125ea460f5 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.tsx @@ -1,20 +1,54 @@ -import { Tabs, TabsList, TabsTrigger } from "@humansignal/ui"; +import type { ChangeEvent } from "react"; +import { useCallback } from "react"; +import { Tabs, TabsList, TabsTrigger, Toggle } from "@humansignal/ui"; export type ViewMode = "code" | "interactive"; interface ViewToggleProps { + /** Current view mode (code or interactive) */ view: ViewMode; + /** Callback when view mode changes */ onViewChange: (view: ViewMode) => void; + /** Whether to show resolved URLs (proxy URLs) or original storage URLs */ + resolveUrls?: boolean; + /** Callback when resolve URLs toggle changes */ + onResolveUrlsChange?: (resolve: boolean) => void; + /** Additional CSS class */ className?: string; } -export const ViewToggle = ({ view, onViewChange, className }: ViewToggleProps) => { +/** + * ViewToggle - Controls for task source viewer display options + * + * Provides two controls: + * 1. View mode toggle (Code/Interactive) - switches between JSON code view and interactive tree + * 2. Resolve URLs toggle - when OFF, shows original storage URLs (s3://..., gs://...), + * when ON, shows resolved proxy URLs (/tasks/.../resolve/...) + */ +export const ViewToggle = ({ + view, + onViewChange, + resolveUrls = false, + onResolveUrlsChange, + className, +}: ViewToggleProps) => { + const handleResolveUrlsChange = useCallback( + (e: ChangeEvent) => { + onResolveUrlsChange?.(e.target.checked); + }, + [onResolveUrlsChange], + ); + return ( - void} variant="default"> - - Code - Interactive - - +
+ void} variant="default"> + + Code + Interactive + + + + {onResolveUrlsChange && } +
); }; From 0ca010fd471fd5762017abe4b2e5acfed1b0a5eb Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 00:26:50 +0000 Subject: [PATCH 2/6] Linter --- .../TaskSourceViewer.test.tsx | 65 ++++++------------- .../TaskSourceViewer/ViewToggle.test.tsx | 65 +++++-------------- 2 files changed, 36 insertions(+), 94 deletions(-) diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx index 127e9cf0fafd..53e8f650fa4a 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx @@ -1,7 +1,7 @@ import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import "@testing-library/jest-dom"; -import { TaskSourceViewer, type TaskLoadOptions } from "./TaskSourceViewer"; +import { TaskSourceViewer } from "./TaskSourceViewer"; // Mock feature flags jest.mock("../../../utils/feature-flags", () => ({ @@ -12,22 +12,24 @@ jest.mock("../../../utils/feature-flags", () => ({ // Mock UI components jest.mock("@humansignal/ui", () => ({ - JsonViewer: ({ data }: any) => ( -
{JSON.stringify(data)}
- ), + JsonViewer: ({ data }: any) =>
{JSON.stringify(data)}
, Tabs: ({ children, value, onValueChange }: any) => ( -
{ - const target = e.target as HTMLElement; - if (target.dataset.value) { - onValueChange(target.dataset.value); - } - }}> +
{ + const target = e.target as HTMLElement; + if (target.dataset.value) { + onValueChange(target.dataset.value); + } + }} + > {children}
), TabsList: ({ children }: any) =>
{children}
, TabsTrigger: ({ children, value }: any) => ( - ), @@ -47,9 +49,7 @@ jest.mock("@humansignal/ui", () => ({ // Mock CodeView component jest.mock("./CodeView", () => ({ - CodeView: ({ data }: any) => ( - 
{JSON.stringify(data, null, 2)}
- ), + CodeView: ({ data }: any) => 
{JSON.stringify(data, null, 2)}
, })); // Mock styles @@ -118,13 +118,7 @@ describe("TaskSourceViewer Component", () => { const mockOnTaskLoad = jest.fn().mockResolvedValue(mockTaskData); const mockRenderToggle = jest.fn(); - render( - - ); + render(); // Wait for initial load await waitFor(() => { @@ -157,12 +151,7 @@ describe("TaskSourceViewer Component", () => { const user = userEvent.setup(); const mockRenderToggle = jest.fn(); - render( - - ); + render(); await waitFor(() => { expect(mockRenderToggle).toHaveBeenCalled(); @@ -203,12 +192,7 @@ describe("TaskSourceViewer Component", () => { const user = userEvent.setup(); const mockRenderToggle = jest.fn(); - render( - - ); + render(); await waitFor(() => { expect(mockRenderToggle).toHaveBeenCalled(); @@ -234,13 +218,7 @@ describe("TaskSourceViewer Component", () => { predictions: [{ id: 2 }], }); - render( - - ); + render(); await waitFor(() => { const codeView = screen.getByTestId("code-view"); @@ -265,12 +243,7 @@ describe("TaskSourceViewer Component", () => { it("should call renderToggle with ViewToggle component when interactive viewer is enabled", async () => { const mockRenderToggle = jest.fn(); - render( - - ); + render(); await waitFor(() => { expect(mockRenderToggle).toHaveBeenCalled(); diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx index b14146f5e87e..ae711c40ef76 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/ViewToggle.test.tsx @@ -6,12 +6,16 @@ import { ViewToggle } from "./ViewToggle"; // Mock the UI components jest.mock("@humansignal/ui", () => ({ Tabs: ({ children, value, onValueChange }: any) => ( -
{ - const target = e.target as HTMLElement; - if (target.dataset.value) { - onValueChange(target.dataset.value); - } - }}> +
{ + const target = e.target as HTMLElement; + if (target.dataset.value) { + onValueChange(target.dataset.value); + } + }} + > {children}
), @@ -21,18 +25,13 @@ jest.mock("@humansignal/ui", () => ({
), TabsTrigger: ({ children, value }: any) => ( - ), Toggle: ({ label, checked, onChange }: any) => ( ), @@ -84,26 +83,14 @@ describe("ViewToggle Component", () => { }); it("should render resolve URLs toggle when onResolveUrlsChange is provided", () => { - render( - - ); + render(); expect(screen.getByTestId("resolve-urls-toggle")).toBeInTheDocument(); expect(screen.getByText("Resolve URLs")).toBeInTheDocument(); }); it("should reflect resolveUrls state in toggle", () => { - render( - - ); + render(); expect(screen.getByTestId("resolve-urls-toggle")).toBeChecked(); }); @@ -112,13 +99,7 @@ describe("ViewToggle Component", () => { const user = userEvent.setup(); const mockOnResolveUrlsChange = jest.fn(); - render( - - ); + render(); await user.click(screen.getByTestId("resolve-urls-toggle")); @@ -129,13 +110,7 @@ describe("ViewToggle Component", () => { const user = userEvent.setup(); const mockOnResolveUrlsChange = jest.fn(); - render( - - ); + render(); await user.click(screen.getByTestId("resolve-urls-toggle")); @@ -145,13 +120,7 @@ describe("ViewToggle Component", () => { describe("Layout", () => { it("should render both toggles when all props are provided", () => { - render( - - ); + render(); // View mode tabs should be present expect(screen.getByTestId("tabs")).toBeInTheDocument(); From d25856c756e1d53baf9ab16a30153718862b218d Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 02:29:20 +0000 Subject: [PATCH 3/6] Fixes --- .../src/components/Common/Table/Table.jsx | 18 +- .../TaskSourceViewer.module.scss | 23 ++- .../TaskSourceViewer.test.tsx | 161 ++++++++++++------ .../TaskSourceViewer/TaskSourceViewer.tsx | 85 +++++---- .../TaskSourceViewer/ViewToggle.test.tsx | 59 +------ .../Common/TaskSourceViewer/ViewToggle.tsx | 46 +---- .../ui/src/lib/json-viewer/json-viewer.tsx | 46 ++--- web/libs/ui/src/lib/json-viewer/types.ts | 2 + 8 files changed, 223 insertions(+), 217 deletions(-) diff --git a/web/libs/datamanager/src/components/Common/Table/Table.jsx b/web/libs/datamanager/src/components/Common/Table/Table.jsx index e2a9ddbe6337..77e972feb304 100644 --- a/web/libs/datamanager/src/components/Common/Table/Table.jsx +++ b/web/libs/datamanager/src/components/Common/Table/Table.jsx @@ -161,9 +161,9 @@ export const Table = observer( /** * Load full task data from API. * @param {Object} options - Load options - * @param {boolean} [options.resolveUri=false] - Whether to resolve storage URLs to proxy URLs. - * When false, original storage URLs (s3://..., gs://...) are preserved for debugging. - * When true, URLs are converted to proxy URLs (/tasks/.../resolve/?fileuri=...). + * @param {boolean} [options.resolveUri=false] - Whether to resolve storage URIs to proxy URLs. + * When false, original storage URIs (s3://..., gs://...) are preserved for debugging. + * When true, URIs are converted to proxy URLs (/tasks/.../resolve/?fileuri=...). */ const onTaskLoad = async (options = {}) => { if (isFF(FF_LOPS_E_3) && type === "DE") { @@ -185,18 +185,8 @@ export const Table = observer( const modalInstance = modal({ title: `Source for task ${out?.id}`, style: { width: 900 }, - header: null, // Will be set by renderToggle body: ( - { - // Update modal header with toggle - modalInstance?.update({ header: toggle }); - }} - /> + ), }); }} diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss index a1654031ce4d..9a267501beda 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss @@ -3,9 +3,30 @@ flex-direction: column; gap: var(--spacing-base); width: 100%; + // Prevent modal from resizing when switching views (Code vs Interactive) + // This avoids accidental clicks outside the modal when the content shrinks + min-height: 400px; +} + +.viewToggleContainer { + display: flex; + justify-content: flex-end; } .viewContent { flex: 1; - min-height: 0; + min-height: 300px; + max-height: 60vh; + overflow: auto; +} + +.loadingContainer { + display: flex; + align-items: center; + justify-content: center; + min-height: 300px; +} + +.resolveUriToggle { + margin-left: auto; } diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx index 53e8f650fa4a..a33354a93e73 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx @@ -12,7 +12,12 @@ jest.mock("../../../utils/feature-flags", () => ({ // Mock UI components jest.mock("@humansignal/ui", () => ({ - JsonViewer: ({ data }: any) =>
{JSON.stringify(data)}
, + JsonViewer: ({ data, toolbarExtra }: any) => ( +
+ {toolbarExtra &&
{toolbarExtra}
} +
{JSON.stringify(data)}
+
+ ), Tabs: ({ children, value, onValueChange }: any) => (
({ ), Toggle: ({ label, checked, onChange }: any) => ( -
)} diff --git a/web/libs/ui/src/lib/json-viewer/types.ts b/web/libs/ui/src/lib/json-viewer/types.ts index 4ebef8729b56..be702c556f66 100644 --- a/web/libs/ui/src/lib/json-viewer/types.ts +++ b/web/libs/ui/src/lib/json-viewer/types.ts @@ -28,6 +28,8 @@ export interface JsonViewerProps { readerViewThreshold?: number; /** Storage key for localStorage persistence of filters and search state */ storageKey?: string; + /** Additional elements to render in the toolbar (after filters) */ + toolbarExtra?: React.ReactNode; // Display settings /** Container min height */ From 20e13d4689a7f99645c838f516de458da225b379 Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 13:05:54 +0000 Subject: [PATCH 4/6] Fixes --- .../src/components/Common/Table/Table.jsx | 19 ++- .../TaskSourceViewer/CodeView.module.scss | 4 +- .../TaskSourceViewer.module.scss | 25 +-- .../TaskSourceViewer/TaskSourceViewer.tsx | 151 +++++------------- web/libs/ui/src/lib/modal/ModalPopup.tsx | 14 ++ 5 files changed, 76 insertions(+), 137 deletions(-) diff --git a/web/libs/datamanager/src/components/Common/Table/Table.jsx b/web/libs/datamanager/src/components/Common/Table/Table.jsx index 77e972feb304..b75b36736629 100644 --- a/web/libs/datamanager/src/components/Common/Table/Table.jsx +++ b/web/libs/datamanager/src/components/Common/Table/Table.jsx @@ -158,13 +158,6 @@ export const Table = observer( predictions: out?.predictions, }; - /** - * Load full task data from API. - * @param {Object} options - Load options - * @param {boolean} [options.resolveUri=false] - Whether to resolve storage URIs to proxy URLs. - * When false, original storage URIs (s3://..., gs://...) are preserved for debugging. - * When true, URIs are converted to proxy URLs (/tasks/.../resolve/?fileuri=...). - */ const onTaskLoad = async (options = {}) => { if (isFF(FF_LOPS_E_3) && type === "DE") { return new Promise((resolve) => resolve(out)); @@ -185,8 +178,18 @@ export const Table = observer( const modalInstance = modal({ title: `Source for task ${out?.id}`, style: { width: 900 }, + header: null, // Will be set by renderToggle body: ( - + { + // Update modal header with toggle + modalInstance?.update({ header: toggle }); + }} + /> ), }); }} diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/CodeView.module.scss b/web/libs/datamanager/src/components/Common/TaskSourceViewer/CodeView.module.scss index 445604f7ff33..b6609c671d81 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/CodeView.module.scss +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/CodeView.module.scss @@ -3,7 +3,9 @@ background: var(--color-neutral-surface-inset); border-radius: var(--corner-radius-small); overflow: auto; - max-height: 600px; + + // Fill parent container for consistent modal height + height: 100%; box-shadow: inset 0 2px 8px rgba(var(--color-neutral-shadow-raw) / 12%); border: 1px solid var(--color-neutral-border); } diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss index 9a267501beda..22549e0dbf29 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.module.scss @@ -3,30 +3,13 @@ flex-direction: column; gap: var(--spacing-base); width: 100%; - // Prevent modal from resizing when switching views (Code vs Interactive) - // This avoids accidental clicks outside the modal when the content shrinks - min-height: 400px; -} -.viewToggleContainer { - display: flex; - justify-content: flex-end; + // Fixed height prevents modal from resizing when switching Code/Interactive views + // that leads to accidental clicks outside and unexpecteddd modal closing + height: 600px; } .viewContent { flex: 1; - min-height: 300px; - max-height: 60vh; - overflow: auto; -} - -.loadingContainer { - display: flex; - align-items: center; - justify-content: center; - min-height: 300px; -} - -.resolveUriToggle { - margin-left: auto; + min-height: 0; } diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx index a070976e205a..f8ef4d80be8e 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx @@ -1,5 +1,5 @@ -import { type ChangeEvent, type FC, useEffect, useState, useCallback, useRef, useMemo } from "react"; -import { JsonViewer, type FilterConfig, Toggle, Spinner } from "@humansignal/ui"; +import { type ChangeEvent, type FC, useEffect, useState, useCallback, useRef } from "react"; +import { JsonViewer, type FilterConfig, Toggle } from "@humansignal/ui"; import { FF_LOPS_E_3, FF_INTERACTIVE_JSON_VIEWER, isFF } from "../../../utils/feature-flags"; import { CodeView } from "./CodeView"; import styles from "./TaskSourceViewer.module.scss"; @@ -14,18 +14,16 @@ export interface TaskLoadOptions { } export interface TaskSourceViewerProps { - /** Task content data (not used - data is loaded fresh via onTaskLoad) */ + /** Task content data */ content: any; - /** - * Function to load full task data. - * @param options - Options including resolveUri to control URL resolution - * @returns Promise with task data - */ + /** Function to load full task data */ onTaskLoad: (options?: TaskLoadOptions) => Promise; /** SDK type (e.g., "DE" for Data Explorer) */ sdkType?: string; /** Storage key for localStorage persistence */ storageKey?: string; + /** Render toggle in external location (e.g., modal header) */ + renderToggle?: (toggle: React.ReactNode) => void; } // Define filters outside component to prevent recreation on every render @@ -61,26 +59,23 @@ const TASK_SOURCE_FILTERS: FilterConfig[] = [ * * Loads task data and provides either code view or interactive JSON viewer. * Specific to the Data Manager and should not be part of the reusable UI library. - * - * Features: - * - Code/Interactive view toggle for different JSON display modes - * - Resolve URIs toggle to show original storage URIs (s3://...) or resolved proxy URLs */ -export const TaskSourceViewer: FC = ({ onTaskLoad, sdkType, storageKey = "dm:tasksource" }) => { +export const TaskSourceViewer: FC = ({ + content, + onTaskLoad, + sdkType, + storageKey = "dm:tasksource", + renderToggle, +}) => { const isInteractiveViewerEnabled = isFF(FF_INTERACTIVE_JSON_VIEWER); - // Don't use content prop - load fresh data to ensure correct resolve_uri setting - const [taskData, setTaskData] = useState(null); - const [isLoading, setIsLoading] = useState(true); - - // Track if this is the initial load to avoid double-fetching - const isInitialLoadRef = useRef(true); + const [taskData, setTaskData] = useState(content); + const isInitialLoad = useRef(true); // Manage view state internally - const [view, setView] = useState(() => { - const stored = storageKey ? (localStorage.getItem(`${storageKey}:view`) as ViewMode) || "code" : "code"; - return stored; - }); + const [view, setView] = useState(() => + storageKey ? (localStorage.getItem(`${storageKey}:view`) as ViewMode) || "code" : "code", + ); // Manage resolve URIs state - default OFF to show original storage URIs const [resolveUrls, setResolveUrls] = useState(() => @@ -99,24 +94,9 @@ export const TaskSourceViewer: FC = ({ onTaskLoad, sdkTyp [storageKey], ); - const handleResolveUrlsChange = useCallback( - (newResolveUrls: boolean) => { - setResolveUrls(newResolveUrls); - - // Save to localStorage - if (storageKey) { - localStorage.setItem(`${storageKey}:resolveUrls`, String(newResolveUrls)); - } - }, - [storageKey], - ); - - /** - * Format raw API response into display format. - * Strips annotations/predictions for Data Explorer mode. - */ - const formatTaskData = useCallback( - (response: any) => { + // Load full task data + useEffect(() => { + onTaskLoad({ resolveUri: resolveUrls }).then((response) => { const formatted: any = { id: response.id, data: response.data, @@ -132,79 +112,32 @@ export const TaskSourceViewer: FC = ({ onTaskLoad, sdkTyp formatted.state = response.state; } - return formatted; - }, - [sdkType], - ); - - /** - * Load task data from API with current resolveUrls setting. - */ - const loadTaskData = useCallback(async () => { - setIsLoading(true); - try { - const response = await onTaskLoad({ resolveUri: resolveUrls }); - setTaskData(formatTaskData(response)); - } catch (error) { - console.error("Failed to load task data:", error); - } finally { - setIsLoading(false); - } - }, [onTaskLoad, resolveUrls, formatTaskData]); - - // Initial load - useEffect(() => { - if (isInitialLoadRef.current) { - isInitialLoadRef.current = false; - loadTaskData(); - } - }, [loadTaskData]); + setTaskData(formatted); + isInitialLoad.current = false; + }); + }, [onTaskLoad, sdkType, resolveUrls]); - // Reload when resolveUrls changes (but not on initial load) - useEffect(() => { - if (!isInitialLoadRef.current) { - loadTaskData(); - } - }, [resolveUrls]); // eslint-disable-line react-hooks/exhaustive-deps - - // Handler for the resolve URIs toggle - const onResolveToggleChange = useCallback( + // Handle resolve URIs toggle change + const handleResolveUrlsChange = useCallback( (e: ChangeEvent) => { - handleResolveUrlsChange(e.target.checked); + const newValue = e.target.checked; + setResolveUrls(newValue); + if (storageKey) { + localStorage.setItem(`${storageKey}:resolveUrls`, String(newValue)); + } }, - [handleResolveUrlsChange], - ); - - // Resolve URIs toggle element for toolbar (right-aligned) - const resolveUriToggle = useMemo( - () => ( -
- -
- ), - [resolveUrls, onResolveToggleChange], + [storageKey], ); - // Show loading state while fetching data - if (isLoading || !taskData) { - return ( -
-
- -
-
- ); - } + // Provide toggle to external render location (e.g., modal header) + useEffect(() => { + if (renderToggle && isInteractiveViewerEnabled) { + renderToggle(); + } + }, [renderToggle, view, handleViewChange, isInteractiveViewerEnabled]); return (
- {/* View mode toggle - shown inside component to avoid modal update issues */} - {isInteractiveViewerEnabled && ( -
- -
- )} -
{view === "code" ? ( @@ -219,7 +152,11 @@ export const TaskSourceViewer: FC = ({ onTaskLoad, sdkTyp maxHeight={560} readerViewThreshold={100} storageKey={storageKey} - toolbarExtra={resolveUriToggle} + toolbarExtra={ +
+ +
+ } /> )}
diff --git a/web/libs/ui/src/lib/modal/ModalPopup.tsx b/web/libs/ui/src/lib/modal/ModalPopup.tsx index 3f1b7a90137f..2136caa216df 100644 --- a/web/libs/ui/src/lib/modal/ModalPopup.tsx +++ b/web/libs/ui/src/lib/modal/ModalPopup.tsx @@ -57,6 +57,7 @@ export class Modal extends Component, ModalState> { static CloseButton = ModalCloseButton; modalRef = createRef(); + mouseDownTarget: HTMLElement | null = null; constructor(props: ModalProps) { super(props); @@ -155,6 +156,7 @@ export class Modal extends Component, ModalState> { ref={(el) => setRef(this.modalRef, el)} mod={mods} mix={mixes} + onMouseDown={this.onMouseDown} onClick={this.onClickOutside} data-testid={this.props["data-testid"]} style={styles} @@ -180,6 +182,10 @@ export class Modal extends Component, ModalState> { return createPortal(modalContent, document.body); } + onMouseDown = (e: React.MouseEvent) => { + this.mouseDownTarget = e.target as HTMLElement; + }; + onClickOutside = (e: React.MouseEvent) => { if (!this.modalRef.current) return; const { closeOnClickOutside } = this.props; @@ -189,9 +195,17 @@ export class Modal extends Component, ModalState> { const content = cn("modal-ls").elem("content").closest(elem); const close = cn("modal-ls").elem("close").closest(elem); + // Don't close if mousedown started inside content (e.g., text selection dragged outside) + const mouseDownContent = this.mouseDownTarget ? cn("modal-ls").elem("content").closest(this.mouseDownTarget) : null; + if (mouseDownContent && content === null) { + this.mouseDownTarget = null; + return; + } + if (allowClose && ((isInModal && close) || (content === null && closeOnClickOutside !== false))) { this.hide(); } + this.mouseDownTarget = null; }; closeOnEscape = (e: KeyboardEvent) => { From 305f525651d450efd35ea7660312fdf00be655ca Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 13:13:55 +0000 Subject: [PATCH 5/6] Fixes --- .../TaskSourceViewer.test.tsx | 79 +++++++------------ 1 file changed, 27 insertions(+), 52 deletions(-) diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx index a33354a93e73..e4d77277653a 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.test.tsx @@ -202,56 +202,43 @@ describe("TaskSourceViewer Component", () => { }); }); - it("should show view toggle inside component", async () => { - render(); - - await waitFor(() => { - // View toggle tabs should be visible inside the component - expect(screen.getByTestId("tab-code")).toBeInTheDocument(); - expect(screen.getByTestId("tab-interactive")).toBeInTheDocument(); - }); - }); - - it("should save view preference to localStorage when changed", async () => { - const user = userEvent.setup(); + it("should call renderToggle with ViewToggle component", async () => { + const mockRenderToggle = jest.fn(); - render(); + render(); await waitFor(() => { - expect(screen.getByTestId("tab-interactive")).toBeInTheDocument(); + expect(mockRenderToggle).toHaveBeenCalled(); }); - await user.click(screen.getByTestId("tab-interactive")); - - await waitFor(() => { - expect(localStorage.getItem("test:tasksource:view")).toBe("interactive"); - }); + // Should have been called with a ViewToggle element + const toggleArg = mockRenderToggle.mock.calls[0][0]; + expect(toggleArg).toBeTruthy(); }); - it("should switch between code and interactive views", async () => { - const user = userEvent.setup(); - - render(); + it("should update renderToggle when view changes via callback", async () => { + const mockRenderToggle = jest.fn(); + let capturedOnViewChange: ((view: string) => void) | null = null; - // Initially in code view - await waitFor(() => { - expect(screen.getByTestId("code-view")).toBeInTheDocument(); + // Capture the onViewChange callback from the toggle + mockRenderToggle.mockImplementation((toggle: any) => { + if (toggle?.props?.onViewChange) { + capturedOnViewChange = toggle.props.onViewChange; + } }); - // Switch to interactive - await user.click(screen.getByTestId("tab-interactive")); + render(); await waitFor(() => { - expect(screen.getByTestId("json-viewer")).toBeInTheDocument(); - expect(screen.queryByTestId("code-view")).not.toBeInTheDocument(); + expect(capturedOnViewChange).toBeTruthy(); }); - // Switch back to code - await user.click(screen.getByTestId("tab-code")); + // Simulate view change via callback + capturedOnViewChange!("interactive"); await waitFor(() => { - expect(screen.getByTestId("code-view")).toBeInTheDocument(); - expect(screen.queryByTestId("json-viewer")).not.toBeInTheDocument(); + expect(localStorage.getItem("test:tasksource:view")).toBe("interactive"); + expect(screen.getByTestId("json-viewer")).toBeInTheDocument(); }); }); }); @@ -274,19 +261,8 @@ describe("TaskSourceViewer Component", () => { }); }); - describe("Error Handling", () => { - it("should handle API errors gracefully", async () => { - const mockOnTaskLoad = jest.fn().mockRejectedValue(new Error("API Error")); - - // Should not throw - expect(() => { - render(); - }).not.toThrow(); - }); - }); - describe("Loading State", () => { - it("should show spinner while loading", async () => { + it("should show initial content while loading then update", async () => { // Create a promise that doesn't resolve immediately let resolvePromise: (value: any) => void; const mockOnTaskLoad = jest.fn().mockImplementation( @@ -298,16 +274,15 @@ describe("TaskSourceViewer Component", () => { render(); - // Spinner should be visible while loading - expect(screen.getByTestId("spinner")).toBeInTheDocument(); + // Should show initial content from props + expect(screen.getByTestId("code-view")).toBeInTheDocument(); - // Resolve the promise + // Resolve the promise with new data resolvePromise!(mockTaskData); - // Wait for content to appear + // Wait for content to be updated await waitFor(() => { - expect(screen.queryByTestId("spinner")).not.toBeInTheDocument(); - expect(screen.getByTestId("code-view")).toBeInTheDocument(); + expect(screen.getByTestId("code-view")).toHaveTextContent("s3://bucket/image.jpg"); }); }); }); From 3b6e9021a2a52e5912b12a605b729aec3978737b Mon Sep 17 00:00:00 2001 From: makseq Date: Wed, 4 Feb 2026 13:39:51 +0000 Subject: [PATCH 6/6] Fix --- .../components/Common/TaskSourceViewer/TaskSourceViewer.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx index f8ef4d80be8e..59e859efcbd6 100644 --- a/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx +++ b/web/libs/datamanager/src/components/Common/TaskSourceViewer/TaskSourceViewer.tsx @@ -1,4 +1,4 @@ -import { type ChangeEvent, type FC, useEffect, useState, useCallback, useRef } from "react"; +import { type ChangeEvent, type FC, useEffect, useState, useCallback } from "react"; import { JsonViewer, type FilterConfig, Toggle } from "@humansignal/ui"; import { FF_LOPS_E_3, FF_INTERACTIVE_JSON_VIEWER, isFF } from "../../../utils/feature-flags"; import { CodeView } from "./CodeView"; @@ -70,7 +70,6 @@ export const TaskSourceViewer: FC = ({ const isInteractiveViewerEnabled = isFF(FF_INTERACTIVE_JSON_VIEWER); const [taskData, setTaskData] = useState(content); - const isInitialLoad = useRef(true); // Manage view state internally const [view, setView] = useState(() => @@ -113,7 +112,6 @@ export const TaskSourceViewer: FC = ({ } setTaskData(formatted); - isInitialLoad.current = false; }); }, [onTaskLoad, sdkType, resolveUrls]);