Merge pull request #47 from atlassian-labs/safer-task-ids

.nvmrc

@@ -1 +1 @@
-10.15.3
+12.18.3

src/decorator/task-harness.tsx

@@ -66,10 +66,10 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro
         },
         {
           eventName: eventNames.START_ONE,
-          fn: async function onStartOne({ taskId, copies, samples }: RunOne['Params']) {
-            const task = tasks[taskId];
+          fn: async function onStartOne({ taskName, copies, samples }: RunOne['Params']) {
+            const task = tasks[taskName];
             if (task == null) {
-              throw new Error(`Could not find task with id: ${taskId}`);
+              throw new Error(`Could not find task with id: ${taskName}`);
             }
 
             if (task.type === 'timed' || task.type === 'interaction') {
@@ -79,7 +79,7 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro
                 samples,
                 copies,
               });
-              safeEmit(eventNames.FINISH_ONE, { taskId, result });
+              safeEmit(eventNames.FINISH_ONE, { taskName, result });
               return;
             }
             if (task.type === 'static') {
@@ -88,7 +88,7 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro
                 getNode,
                 copies,
               });
-              safeEmit(eventNames.FINISH_ONE, { taskId, result });
+              safeEmit(eventNames.FINISH_ONE, { taskName, result });
               return;
             }
           },

src/events.ts

@@ -8,8 +8,8 @@ export default {
 };
 
 export type RunOne = {
-  Params: { taskId: string; copies: number; samples: number };
-  Result: { taskId: string; result: TimedResult | StaticResult };
+  Params: { taskName: string; copies: number; samples: number };
+  Result: { taskName: string; result: TimedResult | StaticResult };
 };
 
 export type RunAll = {

src/panel/machine.ts

@@ -5,7 +5,7 @@ export type MachineEvents =
   | { type: 'WAIT' }
   | { type: 'LOADED'; storyName: string; pinned: Nullable<RunContext> }
   | { type: 'START_ALL' }
-  | { type: 'START_ONE'; taskId: string }
+  | { type: 'START_ONE'; taskName: string }
   | { type: 'FINISH'; results: TaskGroupResult[] }
   | { type: 'PIN' }
   | { type: 'UNPIN' }

src/panel/pinned-storage.ts

@@ -1,27 +1,47 @@
 import { RunContext } from './machine';
 import { packageName } from '../addon-constants';
-import { Nullable, TaskGroupResult, Combine, TaskGroup } from '../types';
+import { Nullable, TaskGroupResult, Combine, TaskGroup, ResultMap } from '../types';
 
-function upgrade(context: RunContext): Nullable<RunContext> {
-  if (!context.results) {
-    return context;
+function hasProperty(value: Record<string, any>, key: string): boolean {
+  return Object.prototype.hasOwnProperty.call(value, key);
+}
+
+function isValidContext(value: unknown): value is RunContext {
+  if (typeof value !== 'object') {
+    return false;
+  }
+  if (value == null) {
+    return false;
+  }
+  if (Array.isArray(value)) {
+    return false;
+  }
+
+  // Duck typing:
+  const hasAllProperties: boolean = ['results', 'samples', 'copies'].every((key) =>
+    hasProperty(value, key),
+  );
+
+  if (!hasAllProperties) {
+    return false;
   }
 
-  // Changed TaskGroup.groupName to TaskGroup.groupId
-  return {
-    ...context,
-    results: context.results.map(
-      (result: any): TaskGroupResult => {
-        if (result.groupName) {
-          return {
-            groupId: result.groupName,
-            map: result.map,
-          };
-        }
-        return result;
-      },
-    ),
-  };
+  // Now going to get any result and ensure it doesn't have a 'taskId'
+  // if it does have a taskId then the entry is out of date
+  const map: ResultMap | undefined =
+    // @ts-ignore
+    value && value.results && value.results[0] ? value.results[0].map : undefined;
+
+  if (map == null) {
+    return false;
+  }
+
+  const hasTaskId: boolean = Object.keys(map).some((key) => {
+    const entry = map[key];
+    return hasProperty(entry, 'taskId');
+  });
+
+  return hasTaskId ? false : true;
 }
 
 function getKey(storyName: string) {
@@ -43,5 +63,13 @@ export function getPinned(storyName: string): Nullable<RunContext> {
     return null;
   }
 
-  return upgrade(JSON.parse(raw));
+  const value: any = JSON.parse(raw);
+  if (!isValidContext(value)) {
+    // eslint-disable-next-line no-console
+    console.warn('Unsupported value found in localStorage. Value cleared');
+    clearPinned(storyName);
+    return null;
+  }
+
+  return value;
 }

src/panel/task-group.tsx

@@ -51,10 +51,10 @@ export default React.memo(function TaskGroup({ group, result, pinned }: Props) {
       {group.tasks.map((task: Task) => {
         return (
           <TaskResult
-            key={task.taskId}
+            key={task.name}
             task={task}
-            result={result.map[task.taskId] || null}
-            pinned={pinned?.map[task.taskId] || null}
+            result={result.map[task.name] || null}
+            pinned={pinned?.map[task.name] || null}
           />
         );
       })}

src/panel/task-result/error-result.tsx

@@ -45,7 +45,6 @@ export default function ErrorResultView({ task, result }: { task: Task; result:
 
   return (
     <ExpandingResult
-      taskId={task.taskId}
       name={task.name}
       result={resultNode}
       getExpanded={({ isExpanded }) =>

src/panel/task-result/expanding-result.tsx

@@ -13,7 +13,6 @@ export type ExpandedArgs = {
 };
 
 type Props = {
-  taskId: string;
   name: string;
   result: React.ReactNode;
   getExpanded: (args: ExpandedArgs) => React.ReactNode;
@@ -77,7 +76,7 @@ function ExpandIcon({ isExpanded }: ExpandedArgs) {
   );
 }
 
-export function ExpandingResult({ taskId, name, result, getExpanded }: Props) {
+export function ExpandingResult({ name, result, getExpanded }: Props) {
   const [isExpanded, setIsExpanded] = useState<boolean>(false);
   const toggle = useCallback(() => setIsExpanded((value) => !value), [setIsExpanded]);
   const service = useRequiredContext(ServiceContext);
@@ -101,7 +100,7 @@ export function ExpandingResult({ taskId, name, result, getExpanded }: Props) {
               secondary
               small
               disabled={!nextEventsInclude('START_ONE', state.nextEvents)}
-              onClick={() => send({ type: 'START_ONE', taskId })}
+              onClick={() => send({ type: 'START_ONE', taskName: name })}
             >
               Run task{' '}
               <small>

src/panel/task-result/index.tsx

@@ -19,25 +19,24 @@ export default function TaskResult({ task, result, pinned }: ResultProps) {
   }
 
   if (result.type === 'error') {
-    return <ErrorResultView key={task.taskId} task={task} result={result} />;
+    return <ErrorResultView key={task.name} task={task} result={result} />;
   }
 
   if (result.type === 'static') {
     invariant(task.type === 'static', `Unexpected task type: ${task.type}`);
     // Sometimes a pinned value can be an error. We don't want to compare against that
     const pin: Nullable<StaticResult> = pinned && pinned.type === 'static' ? pinned : null;
-    return <StaticResultView key={task.taskId} task={task} result={result} pinned={pin} />;
+    return <StaticResultView key={task.name} task={task} result={result} pinned={pin} />;
   }
 
   if (result.type === 'timed') {
     invariant(
       task.type === 'timed' || task.type === 'interaction',
-      `Mismatched task -> result type
-      (Task [${task.taskId}:${task.type}] : Result [${result.taskId}:${result.type}]`,
+      `Unexpected task type: ${task.type}`,
     );
     // Sometimes a pinned value can be an error. We don't want to compare against that
     const pin: Nullable<TimedResult> = pinned && pinned.type === 'timed' ? pinned : null;
-    return <TimedResultView key={task.taskId} task={task} result={result} pinned={pin} />;
+    return <TimedResultView key={task.name} task={task} result={result} pinned={pin} />;
   }
 
   // eslint-disable-next-line no-console

src/panel/task-result/static-result.tsx

@@ -89,7 +89,6 @@ export default function StaticResultView({
 
   return (
     <ExpandingResult
-      taskId={task.taskId}
       name={task.name}
       result={resultNode}
       getExpanded={({ isExpanded }) =>

src/panel/task-result/timed-result.tsx

@@ -165,7 +165,6 @@ export default function TimedResultView({ task, pinned, result }: TimedProps) {
 
   return (
     <ExpandingResult
-      taskId={task.taskId}
       name={task.name}
       result={resultNode}
       getExpanded={({ isExpanded }) =>

src/panel/use-panel-machine.tsx

@@ -10,17 +10,16 @@ import { clearPinned, getPinned, savePinned } from './pinned-storage';
 
 type MergeArgs = {
   existing: TaskGroupResult[];
-  taskId: string;
   result: TimedResult | StaticResult;
 };
 
-function mergeWithResults({ existing, taskId, result }: MergeArgs): TaskGroupResult[] {
+function mergeWithResults({ existing, result }: MergeArgs): TaskGroupResult[] {
   return existing.map((groupResult: TaskGroupResult) => {
     return {
       ...groupResult,
       map: {
         ...groupResult.map,
-        [taskId]: result,
+        [result.taskName]: result,
       },
     };
   });
@@ -54,12 +53,11 @@ export default function usePanelMachine(machine: MachineType, channel: Channel)
       service.send('FINISH', { results });
     }
 
-    function finishOne({ taskId, result }: RunOne['Result']) {
+    function finishOne({ result }: RunOne['Result']) {
       const results: TaskGroupResult[] = mergeWithResults({
         // we are using a state machine guard to prevent this
         existing: service.state.context.current.results!,
         result,
-        taskId,
       });
       service.send('FINISH', { results });
     }
@@ -104,7 +102,7 @@ export default function usePanelMachine(machine: MachineType, channel: Channel)
 
           if (event.type === 'START_ONE') {
             channel.emit(eventNames.START_ONE, {
-              taskId: event.taskId,
+              taskName: event.taskName,
               samples,
               copies,
             });

src/task-runner/get-error-result.ts

@@ -5,14 +5,14 @@ export default function getErrorResult({ task, error }: { task: Task; error: any
   if (error instanceof UnsupportedError) {
     return {
       type: 'error',
-      taskId: task.taskId,
+      taskName: task.name,
       reason: 'unsupported',
       message: error.message,
     };
   }
   return {
     type: 'error',
-    taskId: task.taskId,
+    taskName: task.name,
     reason: 'unhandled',
     message: null,
   };

src/task-runner/run-group.ts

@@ -10,7 +10,7 @@ import {
   TimedTask,
   ErrorResult,
 } from '../types';
-import toResultMap from '../util/to-result-map';
+import getResultMap from '../util/get-result-map';
 import { asyncMap } from './async';
 import { getResultForStaticTask } from './run-static-task';
 import { getResultForTimedTask } from './run-timed-task';
@@ -58,7 +58,7 @@ export default async function runGroup({
 
   const results: TaskGroupResult = {
     groupId: group.groupId,
-    map: toResultMap([...timedResults, ...staticResults, ...interactionResults]),
+    map: getResultMap([...timedResults, ...staticResults, ...interactionResults]),
   };
 
   return results;

src/task-runner/run-static-task.ts

@@ -20,7 +20,7 @@ export async function getResultForStaticTask({
     const value: string = await runStaticTask({ task, getElement });
     const result: StaticResult = {
       type: 'static',
-      taskId: task.taskId,
+      taskName: task.name,
       value,
     };
     return result;

src/task-runner/run-timed-task.ts

@@ -89,7 +89,7 @@ export async function getResultForTimedTask({
 
     const result: TimedResult = {
       type: 'timed',
-      taskId: task.taskId,
+      taskName: task.name,
       averageMs: average,
       samples,
       variance: {

src/tasks/get-groups.ts

@@ -1,7 +1,22 @@
-import { PublicInteractionTask, AllowedGroup, TaskGroup } from '../types';
+import { PublicInteractionTask, AllowedGroup, TaskGroup, Task } from '../types';
 import client from './preset/client';
 import server from './preset/server';
 import { getInteractionGroup } from './get-interaction-group';
+import flatten from '../util/flatten';
+import invariant from 'tiny-invariant';
+
+function getDuplicateTaskNames(groups: TaskGroup[]): string[] {
+  const tasks: Task[] = flatten(groups.map((group) => group.tasks));
+  const allNames: string[] = tasks.map((task) => task.name);
+
+  const duplicates: string[] = allNames.filter((name: string) => {
+    return allNames.filter((value) => value === name).length > 1;
+  });
+
+  // duplicates will include two entries for every duplicate
+  // using a Set to trip out the duplicates
+  return [...new Set(duplicates)];
+}
 
 export function getGroups({
   allowedGroups,
@@ -21,5 +36,12 @@ export function getGroups({
 
   result.push(getInteractionGroup(interactions));
 
+  const duplicateNames: string[] = getDuplicateTaskNames(result);
+
+  invariant(
+    !duplicateNames.length,
+    `Tasks found with duplicate names: [${duplicateNames.join(',')}]`,
+  );
+
   return result;
 }

src/tasks/get-interaction-group.ts

@@ -8,7 +8,7 @@ export function getInteractionGroup(interactions: PublicInteractionTask[]): Task
       return {
         ...item,
         type: 'interaction',
-        taskId: `interaction::(index:${index})(name:${item.name})`,
+        name: item.name,
         description: item.description || '(None provided)',
       };
     },

src/tasks/get-tasks-map.ts

@@ -2,8 +2,8 @@ import { Task, TaskGroup, TaskMap } from '../types';
 import { flatten } from 'xstate/lib/utils';
 
 export default function getTaskMap(groups: TaskGroup[]): TaskMap {
-  return flatten(groups.map((group) => group.tasks)).reduce((acc: TaskMap, item: Task) => {
-    acc[item.taskId] = item;
+  return flatten(groups.map((group) => group.tasks)).reduce((acc: TaskMap, task: Task) => {
+    acc[task.name] = task;
     return acc;
   }, {});
 }