fix(seg): Enhance segmentation tools and UI, refactor code, and update dependencies (#4915)

2025-04-02 18:10:47 -04:00
1 parent 1dc00d3030
commit 8432d5f633
@@ -54,7 +54,7 @@
    "@cornerstonejs/codec-libjpeg-turbo-8bit": "^1.2.2",
    "@cornerstonejs/codec-openjpeg": "^1.2.4",
    "@cornerstonejs/codec-openjph": "^2.4.5",
-    "@cornerstonejs/dicom-image-loader": "^3.8.2",
+    "@cornerstonejs/dicom-image-loader": "^3.8.4",
    "@emotion/serialize": "^1.1.3",
    "@ohif/core": "3.10.0-beta.144",
    "@ohif/extension-cornerstone": "3.10.0-beta.144",
@@ -275,6 +275,20 @@ function ViewerViewportGrid(props: withAppTypes) {

  // Store previous isReferenceViewable values to avoid infinite loops
  const prevReferenceViewableMap = useRef(new Map());
+  // Track viewports that need isReferenceViewable updates
+  const viewportsToUpdate = useRef(new Map());
+
+  // Apply isReferenceViewable updates in an effect, not during render
+  useEffect(() => {
+    const updates = viewportsToUpdate.current;
+    if (updates.size > 0) {
+      updates.forEach((isReferenceViewable, viewportId) => {
+        viewportGridService.setIsReferenceViewable(viewportId, isReferenceViewable);
+        prevReferenceViewableMap.current.set(viewportId, isReferenceViewable);
+      });
+      viewportsToUpdate.current.clear();
+    }
+  });

  const getViewportPanes = useCallback(() => {
    const viewportPanes = [];
@@ -313,7 +327,7 @@ function ViewerViewportGrid(props: withAppTypes) {
        uiNotificationService
      );

-      // Only update isReferenceViewable if it's changed to avoid render loops
+      // Only queue isReferenceViewable updates if it's changed to avoid render loops
      // We need to handle both function and non-function values
      if (viewportId) {
        const prevValue = prevReferenceViewableMap.current.get(viewportId);
@@ -323,8 +337,8 @@ function ViewerViewportGrid(props: withAppTypes) {
        // For non-functions, compare directly. For functions, we treat them as always different
        // (this is conservative but safe)
        if (!isSameFunction && prevValue !== isReferenceViewable) {
-          viewportGridService.setIsReferenceViewable(viewportId, isReferenceViewable);
-          prevReferenceViewableMap.current.set(viewportId, isReferenceViewable);
+          // Queue the update instead of doing it during render
+          viewportsToUpdate.current.set(viewportId, isReferenceViewable);
        }
      }

@@ -37,7 +37,7 @@
    "@cornerstonejs/codec-libjpeg-turbo-8bit": "^1.2.2",
    "@cornerstonejs/codec-openjpeg": "^1.2.4",
    "@cornerstonejs/codec-openjph": "^2.4.5",
-    "@cornerstonejs/dicom-image-loader": "^3.8.2",
+    "@cornerstonejs/dicom-image-loader": "^3.8.4",
    "@ohif/ui": "3.10.0-beta.144",
    "cornerstone-math": "0.1.9",
    "dicom-parser": "^1.8.21"
@@ -195,6 +195,38 @@ const bindings = [
    keys: ['n'],
    isEditable: true,
  },
+  {
+    commandName: 'increaseBrushSize',
+    label: 'Increase Brush Size',
+    keys: [']'],
+    isEditable: true,
+  },
+  {
+    commandName: 'decreaseBrushSize',
+    label: 'Decrease Brush Size',
+    keys: ['['],
+    isEditable: true,
+  },
+  {
+    commandName: 'setToolActive',
+    commandOptions: { toolName: 'CircularEraser' },
+    label: 'Eraser',
+    keys: ['e'],
+    isEditable: true,
+  },
+  {
+    commandName: 'setToolActive',
+    commandOptions: { toolName: 'CircularBrush' },
+    label: 'Brush',
+    keys: ['b'],
+    isEditable: true,
+  },
+  {
+    commandName: 'addNewSegment',
+    label: 'Add New Segment',
+    keys: ['a'],
+    isEditable: true,
+  },
 ];

 export default bindings;
@@ -2,18 +2,111 @@ import React, { useState, useEffect } from 'react';
 import { ErrorBoundary as ReactErrorBoundary } from 'react-error-boundary';
 import { useTranslation } from 'react-i18next';
 import { toast } from 'sonner';
-import {
-  Dialog,
-  DialogContent,
-  DialogHeader,
-  DialogTitle,
-  DialogDescription,
-} from '../Dialog/Dialog';
+import { Dialog, DialogContent } from '../Dialog/Dialog';
 import { ScrollArea } from '../ScrollArea/ScrollArea';
-import { Icons } from '../Icons';
+import { Button } from '../Button/Button';

 const isProduction = process.env.NODE_ENV === 'production';

+/**
+ * Parses an error stack trace to extract important information
+ * Extracts the first function name from the stack trace
+ */
+const parseErrorStack = (error: ErrorBoundaryError) => {
+  if (!error.stack) {
+    return { filePath: null, errorTitle: null, code: null, firstFilename: null };
+  }
+
+  const stack = error.stack;
+  const stackLines = stack.split('\n');
+
+  // Extract error message from first line
+  const errorMessage = stackLines[0].trim();
+
+  // Extract first function name from the stack trace
+  let firstFilename = null;
+
+  // Find the first stack line (starts with " at ")
+  for (let i = 1; i < stackLines.length; i++) {
+    const line = stackLines[i].trim();
+    if (line.startsWith('at ')) {
+      // Extract function name pattern
+      const match = line.match(/at\s+([^\s(]+)[\s(]/);
+      if (match && match[1]) {
+        firstFilename = match[1];
+        break;
+      }
+    }
+  }
+
+  // Sanitize stack trace for display - safer approach to avoid ReDoS
+  const sanitizedStack = stackLines
+    .map(line => {
+      // Limit line length to prevent excessive processing
+      const limitedLine = line.substring(0, 2000);
+
+      // Process each part separately to avoid complex regex patterns
+      if (limitedLine.includes('(')) {
+        // Extract filename from paths in parentheses
+        const openParenIndex = limitedLine.indexOf('(');
+        const closeParenIndex = limitedLine.indexOf(')', openParenIndex);
+
+        if (openParenIndex >= 0 && closeParenIndex > openParenIndex) {
+          const pathInParens = limitedLine.substring(openParenIndex + 1, closeParenIndex);
+
+          // Find the last segment after slash or backslash
+          const lastSlashIndex = Math.max(
+            pathInParens.lastIndexOf('/'),
+            pathInParens.lastIndexOf('\\')
+          );
+
+          if (lastSlashIndex >= 0) {
+            const filename = pathInParens.substring(lastSlashIndex + 1);
+            return (
+              limitedLine.substring(0, openParenIndex + 1) +
+              filename +
+              limitedLine.substring(closeParenIndex)
+            );
+          }
+        }
+      }
+
+      // Handle the "at Function path:line:column" format
+      if (limitedLine.includes(' at ')) {
+        const atIndex = limitedLine.indexOf(' at ');
+        const afterAt = limitedLine.substring(atIndex + 4).trim();
+
+        // Split by whitespace to separate function and path
+        const spaceAfterFunc = afterAt.indexOf(' ');
+
+        if (spaceAfterFunc > 0) {
+          const funcName = afterAt.substring(0, spaceAfterFunc);
+          const path = afterAt.substring(spaceAfterFunc + 1);
+
+          // Check if this is a path with line/column numbers
+          if (path.includes(':') && /.*:[0-9]+:[0-9]+/.test(path)) {
+            // Find the last segment after slash or backslash
+            const lastSlashIndex = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\'));
+
+            if (lastSlashIndex >= 0) {
+              const filename = path.substring(lastSlashIndex + 1);
+              return limitedLine.substring(0, atIndex + 4) + funcName + ' ' + filename;
+            }
+          }
+        }
+      }
+
+      return limitedLine;
+    })
+    .join('\n');
+
+  return {
+    errorTitle: errorMessage,
+    code: sanitizedStack,
+    firstFilename: firstFilename,
+  };
+};
+
 interface ErrorBoundaryError extends Error {
  message: string;
  stack?: string;
@@ -45,14 +138,13 @@ const DefaultFallback = ({
  const title = `${t('Something went wrong')}${!isProduction && ` ${t('in')} ${context}`}.`;
  const subtitle = t('Sorry, something went wrong there. Try again.');

-  const copyErrorDetails = () => {
-    const errorDetails = `
-Context: ${context}
-Error Message: ${error.message}
-Stack: ${error.stack}
-    `;
-    navigator.clipboard.writeText(errorDetails);
-    toast.success(t('Copied to clipboard'));
+  const { errorTitle, code, firstFilename } = parseErrorStack(error);
+
+  const copyErrorToClipboard = () => {
+    if (code) {
+      navigator.clipboard.writeText(code);
+      toast.success(t('Error copied to clipboard'));
+    }
  };

  useEffect(() => {
@@ -64,55 +156,76 @@ Stack: ${error.stack}
      },
      duration: 0,
    });
-  }, [error]);
+  }, [error, subtitle, t, title]);

  if (isProduction) {
    return null;
  }

  return (
-    <>
-      <Dialog
-        open={showDetails}
-        onOpenChange={setShowDetails}
+    <Dialog
+      open={showDetails}
+      onOpenChange={setShowDetails}
+    >
+      <DialogContent
+        className="bg-muted max-w-3xl overflow-hidden border-0 p-0"
+        onInteractOutside={e => e.preventDefault()}
      >
-        <DialogContent className="border-input h-[50vh] w-[90vw] border-2 sm:max-w-[900px]">
-          <DialogHeader>
-            <DialogTitle className="text-muted-foreground flex justify-between text-xl">
-              <div className="flex items-center">{title}</div>
-              <button
-                onClick={() => {
-                  copyErrorDetails();
-                  setShowDetails(false);
-                }}
-                className="text-aqua-pale hover:text-aqua-pale/80 flex items-center gap-2 rounded bg-gray-800 px-4 py-2 font-light"
-              >
-                <Icons.Code className="h-4 w-4" />
-                {t('Copy Details')}
-              </button>
-            </DialogTitle>
+        <div className="p-5 pb-4">
+          <div className="flex items-center justify-between">
+            <h2 className="text-highlight text-xl font-normal">
+              {errorTitle || error.message || title}
+            </h2>
+          </div>
+        </div>

-            <DialogDescription className="text-lg">{subtitle}</DialogDescription>
-          </DialogHeader>
-
-          <ScrollArea className="h-[100%]">
-            <div className="space-y-4 pr-4 font-mono text-base">
-              <div className="space-y-4">
-                <p className="text-aqua-pale break-words text-lg">
-                  {t('Context')}: {context}
-                </p>
-                <p className="text-aqua-pale break-words text-lg">
-                  {t('Error Message')}: {error.message}
-                </p>
-                <pre className="text-aqua-pale whitespace-pre-wrap break-words rounded bg-gray-900 p-4">
-                  Stack: {error.stack}
-                </pre>
+        {/* Code block */}
+        {code && (
+          <>
+            <ScrollArea className="bg-background text-foreground mx-6 h-[321px] rounded-b-md">
+              <div className="bg-background border-input flex items-center justify-between rounded-t-md border-b px-4 py-2">
+                <div className="text-muted-foreground text-base">
+                  {firstFilename || 'Error Stack'}
+                </div>
+                <Button
+                  className="w-20"
+                  onClick={copyErrorToClipboard}
+                  title={t('Copy error')}
+                >
+                  Copy
+                </Button>
              </div>
-            </div>
-          </ScrollArea>
-        </DialogContent>
-      </Dialog>
-    </>
+              <div className="p-4 font-mono text-sm">
+                {code.split('\n').map((line, index) => (
+                  <div
+                    key={index}
+                    className="flex"
+                  >
+                    <span className="whitespace-pre">{line}</span>
+                  </div>
+                ))}
+              </div>
+            </ScrollArea>
+          </>
+        )}
+
+        {/* Footer */}
+        <div className="flex items-center justify-end p-6 pt-2">
+          <Button
+            variant="link"
+            className="text-primary p-0"
+            onClick={() =>
+              window.open(
+                'https://github.com/OHIF/Viewers/issues/new?template=bug-report.yml',
+                '_blank'
+              )
+            }
+          >
+            Report Issue
+          </Button>
+        </div>
+      </DialogContent>
+    </Dialog>
  );
 };

@@ -164,9 +277,9 @@ const ErrorBoundary = ({
    };
  }, []);

-  const onErrorHandler = (error: ErrorBoundaryError, componentStack: string) => {
+  const onErrorHandler = (error: ErrorBoundaryError, componentStack: string | null) => {
    console.debug(`${context} Error Boundary`, error, componentStack, context);
-    onError(error, componentStack, context);
+    onError(error, componentStack || '', context);
  };

  return (
@@ -179,7 +292,7 @@ const ErrorBoundary = ({
        />
      )}
      onReset={onResetHandler}
-      onError={onErrorHandler}
+      onError={(error, info) => onErrorHandler(error as ErrorBoundaryError, info.componentStack)}
    >
      <>
        {children}
@@ -124,24 +124,32 @@ const Trigger = ({
 }: TriggerProps) => {
  const { isOpen } = useLayoutSelector();

-  // Style constants matching ToolButton
-  const baseClasses = '!rounded-lg inline-flex items-center justify-center';
-  const defaultClasses =
-    'bg-transparent text-foreground/80 hover:bg-background hover:text-highlight';
-  const activeClasses = 'bg-background text-foreground/80';
-  const disabledClasses =
-    'text-common-bright hover:bg-primary-dark hover:text-primary-light opacity-40 cursor-not-allowed';
-  const buttonSizeClass = 'w-10 h-10';
-  const iconSizeClass = 'h-7 w-7';
-
-  const buttonClasses = cn(
-    baseClasses,
-    buttonSizeClass,
-    disabled ? disabledClasses : isOpen ? activeClasses : defaultClasses
-  );
-
  const hasTooltip = tooltip || (disabled && disabledText);

+  const button = (
+    <Button
+      className={cn(
+        'inline-flex h-10 w-10 items-center justify-center !rounded-lg',
+        disabled
+          ? 'text-common-bright hover:bg-primary-dark hover:text-primary-light cursor-not-allowed opacity-40'
+          : isOpen
+            ? 'bg-background text-foreground/80'
+            : 'text-foreground/80 hover:bg-background hover:text-highlight bg-transparent',
+        className
+      )}
+      variant="ghost"
+      size="icon"
+      aria-label={tooltip}
+      disabled={disabled}
+    >
+      <Icons.ByName
+        name="tool-layout"
+        className="h-7 w-7"
+      />
+    </Button>
+  );
+
+  // If user passed children (custom button), just wrap it directly
  if (children) {
    return (
      <PopoverTrigger
@@ -153,36 +161,26 @@ const Trigger = ({
    );
  }

-  return (
-    <Tooltip>
-      <TooltipTrigger
-        asChild
-        className={cn(disabled && 'cursor-not-allowed')}
-      >
-        <span data-cy="layout-button">
+  if (!isOpen && hasTooltip) {
+    return (
+      <Tooltip>
+        <TooltipTrigger asChild>
          <PopoverTrigger asChild>
-            <Button
-              className={buttonClasses}
-              variant="ghost"
-              size="icon"
-              aria-label={tooltip}
-              disabled={disabled}
-            >
-              <Icons.ByName
-                name="tool-layout"
-                className={iconSizeClass}
-              />
-            </Button>
+            <span data-cy="layout-button">{button}</span>
          </PopoverTrigger>
-        </span>
-      </TooltipTrigger>
-      {hasTooltip && (
+        </TooltipTrigger>
        <TooltipContent side="bottom">
          {tooltip && <div>{tooltip}</div>}
          {disabled && disabledText && <div className="text-muted-foreground">{disabledText}</div>}
        </TooltipContent>
-      )}
-    </Tooltip>
+      </Tooltip>
+    );
+  }
+
+  return (
+    <PopoverTrigger asChild>
+      <span data-cy="layout-button">{button}</span>
+    </PopoverTrigger>
  );
 };

@@ -47,7 +47,7 @@ export const AddSegmentRow: React.FC<{ children?: React.ReactNode }> = ({ childr

  return (
    <div className="my-px flex h-7 w-full items-center justify-between rounded pl-0.5 pr-7">
-      <div className="flex-1">
+      <div className="mt-1 flex-1">
        {allowAddSegment ? (
          <Button
            size="sm"
@@ -13,7 +13,7 @@ function AddSegmentRow({ onClick, onToggleSegmentationVisibility = null, segment
          <div className="grid h-[28px] w-[28px] place-items-center">
            <Icons.Add />
          </div>
-          <span className="text-[13px]">{t('Add segment')}</span>
+          <span className="mt-1 text-[13px]">{t('Add segment')}</span>
        </div>
      </div>
      {segmentation && (