Project

General

Profile

Actions

Bug #22143

closed

Process page crash

Added by Peter Amstutz 3 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Story points:
-
Release:
Release relationship:
Auto

Files

mapFn_is_not_a_function.png (300 KB) mapFn_is_not_a_function.png Peter Amstutz, 09/25/2024 02:48 PM

Subtasks 1 (0 open1 closed)

Task #22145: Review 22143-process-page-crashResolvedStephen Smith10/09/2024Actions
Actions #1

Updated by Peter Amstutz 3 months ago

https://workbench.tordo.arvadosapi.com/processes/tordo-xvhdp-xe0kp4vd0qjyx6f

I think I was seeing this on the #13327 or #22132 branch, those branches don't don't do anything related to the collection panel file listing, so I'm not sure what's going on.

Actions #2

Updated by Peter Amstutz 3 months ago

  • Assigned To set to Stephen Smith
Actions #3

Updated by Peter Amstutz 3 months ago

  • Assigned To changed from Stephen Smith to Lisa Knox
Actions #4

Updated by Lisa Knox 2 months ago

22143-process-page-crash @ 62c20a4bdb2f56ead82573ff7590ada4614b92da

developer-run-tests-services-workbench2: #1235

  • ✅ All agreed upon points are implemented / addressed.
  • ✅ Anything not implemented (discovered or discussed during work) has a follow-up story.
  • ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
  • - Documentation has been updated.
  • - Behaves appropriately at the intended scale (describe intended scale).
  • ✅ Considered backwards and forwards compatibility issues between client and server.
    no changes
  • ✅ Follows our coding standards and GUI style guidelines.
Notes:
  • I was able to reproduce the bug at one point but then I was unable to reproduce it again. I believe when I reproduced it, I had switched from one branch to another with a different package.json without re-installing, but I am unable to confirm this. I addressed what I could by adding a null check where the stack trace points, but until we can reproduce it we can't be 100% sure that it's fixed.
Actions #5

Updated by Lisa Knox 2 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Stephen Smith 2 months ago

I think this is ok as a workaround, but I'm also wondering about one of the uses of mapNodeValue. setNodeValue and all uses of setNodeValueWith explicitly pass in anonymous functions for mapFn, and the last use, mapTreeValues, is mostly called passing in an anonymous function, the only exception being 2 uses where it uses services.collectionService.extendFileURL as the mapFn. I noticed that one of these in collection-panel-files-actions.ts references the collectionService using servicesProvider.getServices().collectionService.extendFileURL instead of using a thunk to access services, which results in extendFileURL being inferred as an any type, and I'm wondering if maybe there isn't a guarantee that getServices() is reliable. So I would suggest trying to change that action into a thunk which seems to satisfy the type system better and makes mapped be a proper tree type instead of any. I have no clue if this will fix the issue but I think it would be good to do as long as the collection file browser seems unaffected, which from what I can tell should be fine.

--- a/services/workbench2/src/components/collection-panel-files/collection-panel-files.tsx
+++ b/services/workbench2/src/components/collection-panel-files/collection-panel-files.tsx
@@ -335,12 +335,12 @@ export const CollectionPanelFiles = withStyles(styles)(
                 fetchData([leftKey, rightKey], true);
             }
             // eslint-disable-next-line react-hooks/exhaustive-deps
         }, [currentPDH]); 

         React.useEffect(() => {
             if (rightData) {
                 const filtered = rightData.filter(({ name }) => name.indexOf(rightSearch) > -1);
-                setCollectionFiles(filtered, false)(dispatch);
+                dispatch(setCollectionFiles(filtered, false));
             }
         }, [rightData, dispatch, rightSearch]);
diff --git a/services/workbench2/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts b/services/workbench2/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts
index 547f1534d1..4e943e42f7 100644
--- a/services/workbench2/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts
+++ b/services/workbench2/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts
@@ -29,10 +29,10 @@ export type CollectionPanelFilesAction = UnionOf<typeof collectionPanelFilesActi

 export const COLLECTION_PANEL_LOAD_FILES = 'collectionPanelLoadFiles';

-export const setCollectionFiles = (files, joinParents = true) => (dispatch: any) => {
+export const setCollectionFiles = (files, joinParents = true) => (dispatch: Dispatch, getState: () => RootState, services: ServiceRepository) => {
     const tree = createCollectionFilesTree(files, joinParents);
     const sorted = sortFilesTree(tree);
-    const mapped = mapTreeValues(servicesProvider.getServices().collectionService.extendFileURL)(sorted);
+    const mapped = mapTreeValues(services.collectionService.extendFileURL)(sorted);
     dispatch(collectionPanelFilesAction.SET_COLLECTION_FILES(mapped));
 };
Actions #7

Updated by Lisa Knox 2 months ago

22143-process-page-crash @ commit: 89d4d8b32e7783855ed8ab3b2ea3861a3e60b28f

developer-run-tests-services-workbench2: #1238

I took your suggestion and also added typing for the `pathData` variable while I was there.

Actions #8

Updated by Peter Amstutz 2 months ago

  • Target version changed from Development 2024-10-09 sprint to Development 2024-10-23 sprint
Actions #9

Updated by Stephen Smith 2 months ago

This lgtm!

Actions #10

Updated by Lisa Knox 2 months ago

  • Status changed from In Progress to Resolved
Actions #11

Updated by Peter Amstutz about 2 months ago

  • Release set to 70
Actions

Also available in: Atom PDF