feature/persistant-annotations #81

BrianWhitneyAI · 2024-05-02T23:11:35Z

Description of Changes

This PR proposes prioritizing recently used Groups, Filters, and Sorts in the ordering of their respective selection dropdowns ( resolves #57 ). This PR is a continuation of #76 feature/recent-hierarchy-annotations , but is based on the new generalize-ui #79 branch.

Testing

Monitored state through various selections and deselections of Groups, Filters, and Sorts including those with repeated no-file-found options.

I did experience some issues with reloading the application where it would disconnect on the first reload but succeed on the second reload. I believe this is an inherent issue to the base branch or my development environment as is still present without these changes.

This PR also passes tests locally with the exception of

  9) @renderer FileDownloadServiceElectron
       promptForDownloadDirectory
         complains about non-writeable directory when given:
     Error: Trying to stub property 'invoke' of undefined

I believe that this is unique to my local environment since I had to use WSL to run the full test suite. In reviewing this PR it would be super helpful to run tests locally with your dev env just to make sure everything is Ok.

Possible Changes/Adjustments

Limiting the length of recent annotations to a smaller number, I foresee a situation where a user has a lot of interaction and the recent list becomes very long (and unalphabetized) and this feature becomes a hindrance
A adjustment to ‘On Start” state to not include undefined elements.

BrianWhitneyAI · 2024-05-02T23:14:11Z

packages/desktop/src/renderer/index.tsx

@@ -102,6 +104,10 @@ store.subscribe(() => {
        {}
    );
    if (JSON.stringify(appState) !== JSON.stringify(oldAppState)) {
+        if (appState.RECENT_ANNOTATIONS[0] == undefined) {


This is the part I do not like. For some reason when a new user opens the app appstate.RECENT_ANNOTATIONS defaults to a list with one defined element rather than an empty list. This breaks when trying to persist the state because the typing does not match. While resetting on the first persist does work its hacky and would be better to fix at the source.

I still think we should find a fix for this rather than special case it if at all possible. My immediate hunch is this is because of the way you are grabbing the values from the persisted storage on load based on your description.
I believe we discussed this, but did you check to see if this:

const recentAnnotations = persistedConfig && persistedConfig[PersistedConfigKeys.RecentAnnotations];

Could be causing an array like so [undefined]? Probably worth trying to see if:

const recentAnnotations = persistedConfig?.[PersistedConfigKeys.RecentAnnotations]?.length ? persistedConfig?.[PersistedConfigKeys.RecentAnnotations] : [];

fixes it?

kmitcham · 2024-05-07T16:29:11Z

packages/core/state/selection/selectors.ts

+        );
+
+        // combine all annotation lists
+        const combinedAnnotations = fileNameAnnotation.concat(


Making sure I understand -this results in: FileName, (recent annotations if any, in original order), (all other annotations, sorted)

SeanLeRoy

I think something this is missing is a visual indicator to the user that describes why these annotations are at the top of the list.
I think making it so the AnnotationSelector component does the special sort for recent annotations rather than the getSortedAnnotations would make it so that AnnotationSelector could then add some icon or something that would inform the user. Something I've seen before is an icon that looks like a redo icon on the list item, that could work well here for the recent annotations.

SeanLeRoy · 2024-05-07T19:14:44Z

packages/core/state/selection/reducer.ts

+                    ...state.recentAnnotations,
+                ])
+            );
+            //let recentAnnotations: string[] = [];


SeanLeRoy · 2024-05-07T19:18:25Z

packages/core/state/selection/reducer.ts

-            ...state,
-            filters: action.payload,
+        [SET_FILE_FILTERS]: (state, action) => {
+            const recentAnnotations = Array.from(


This could be simpler, also removes unnecessary castArray:

[SET_FILE_FILTERS]: (state, action) => ({ ...state, filters: action.payload, recentAnnotations: uniq([ ...action.payload.map((filter: any) => filter.annotationName), ...state.recentAnnotations, ]), // Reset file selections when file filters change fileSelection: new FileSelection(), }),

SeanLeRoy · 2024-05-07T19:21:01Z

packages/core/state/selection/reducer.ts

-            ...state,
-            sortColumn: action.payload,
-        }),
+        [SET_SORT_COLUMN]: (state, action) => {


A bit simpler

[SET_SORT_COLUMN]: (state, action) => ({ ...state, recentAnnotations: uniq([...castArray(action.payload?.annotationName), ...state.recentAnnotations]), sortColumn: action.payload, }),

Also I think you'll need the optional chaining check that this adds because action.payload could be undefined

SeanLeRoy · 2024-05-07T19:22:28Z

packages/core/state/selection/reducer.ts

@@ -175,14 +194,22 @@ export default makeReducer<SelectionStateBranch>(
            ...state,
            fileSelection: action.payload,
        }),
-        [SET_ANNOTATION_HIERARCHY]: (state, action) => ({


A bit simpler (also removes unnecessary castArray)

[SET_ANNOTATION_HIERARCHY]: (state, action) => ({ ...state, annotationHierarchy: action.payload, availableAnnotationsForHierarchyLoading: true, recentAnnotations: uniq([...action.payload, ...state.recentAnnotations]), // Reset file selections when annotation hierarchy changes fileSelection: new FileSelection(), }),

SeanLeRoy · 2024-05-07T19:25:29Z

packages/core/state/selection/selectors.ts

+// COMPOSED SELECTORS
+export const getSortedAnnotations = createSelector(
+    [getAnnotations, getRecentAnnotations],
+    (annotations: Annotation[], recentAnnotationNames: string[]) => {


FWIW These typings are implicit and can be removed

SeanLeRoy · 2024-05-07T19:28:23Z

packages/core/state/selection/selectors.ts

+    [getAnnotations, getRecentAnnotations],
+    (annotations: Annotation[], recentAnnotationNames: string[]) => {
+        // Create Array of annotations from recentAnnotationNames
+        const recentAnnotations = recentAnnotationNames.flatMap((name) =>


This could be simpler by having this inner callback return an individual element rather than an array like so:

const recentAnnotations = recentAnnotationNames.flatMap((name) => annotations.find(annotation => annotation.name === name) ) .filter(annotation => !!annotation);

SeanLeRoy · 2024-05-07T19:28:59Z

packages/core/state/selection/selectors.ts

+        );
+
+        // get the File name annotaion in a list (if Present)
+        const fileNameAnnotation = annotations.filter(


I think you are looking for find here which returns the first element that matches the condition found or undefined if none are found

This should make sure this isn't undefined

SeanLeRoy · 2024-05-07T19:30:46Z

packages/core/state/selection/selectors.ts

+        );
+
+        // create map for filtering duplicate annotations.
+        const combinedAnnotationsMap = new Map();


Could use reduce or similar function to build this map up FWIW

SeanLeRoy · 2024-05-07T19:35:36Z

packages/desktop/src/renderer/index.tsx

@@ -102,6 +104,10 @@ store.subscribe(() => {
        {}
    );
    if (JSON.stringify(appState) !== JSON.stringify(oldAppState)) {
+        if (appState.RECENT_ANNOTATIONS[0] == undefined) {


I still think we should find a fix for this rather than special case it if at all possible. My immediate hunch is this is because of the way you are grabbing the values from the persisted storage on load based on your description.
I believe we discussed this, but did you check to see if this:

const recentAnnotations = persistedConfig && persistedConfig[PersistedConfigKeys.RecentAnnotations];

Could be causing an array like so [undefined]? Probably worth trying to see if:

const recentAnnotations = persistedConfig?.[PersistedConfigKeys.RecentAnnotations]?.length ? persistedConfig?.[PersistedConfigKeys.RecentAnnotations] : [];

fixes it?

SeanLeRoy · 2024-05-07T19:36:03Z

packages/core/state/selection/selectors.ts

@@ -76,3 +77,37 @@ export const getUnavailableAnnotationsForHierarchy = createSelector(
            allAnnotations.filter((annotation) => !availableAnnotations.includes(annotation.name))
        )
 );
+
+// COMPOSED SELECTORS
+export const getSortedAnnotations = createSelector(


Tests around this selector would be good

feature/persistant-annotations

fe33d2e

BrianWhitneyAI commented May 2, 2024

View reviewed changes

maintain recent ordering

289829a

BrianWhitneyAI mentioned this pull request May 6, 2024

feature/recent-hierarchy-annotations #76

Closed

update tests to match feature

0f3d65b

BrianWhitneyAI requested review from SeanLeRoy, pgarrison, aswallace and kmitcham May 6, 2024 21:58

kmitcham reviewed May 7, 2024

View reviewed changes

kmitcham approved these changes May 7, 2024

View reviewed changes

BrianWhitneyAI marked this pull request as ready for review May 7, 2024 17:36

SeanLeRoy reviewed May 7, 2024

View reviewed changes

SeanLeRoy deleted the branch feature/generalize-ui May 7, 2024 19:40

SeanLeRoy closed this May 7, 2024

BrianWhitneyAI mentioned this pull request May 8, 2024

Feature/persistant annotations #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/persistant-annotations #81

feature/persistant-annotations #81

BrianWhitneyAI commented May 2, 2024 •

edited

Loading

BrianWhitneyAI May 2, 2024

SeanLeRoy May 7, 2024

kmitcham May 7, 2024

BrianWhitneyAI May 7, 2024

SeanLeRoy left a comment

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

SeanLeRoy May 7, 2024

feature/persistant-annotations #81

feature/persistant-annotations #81

Conversation

BrianWhitneyAI commented May 2, 2024 • edited Loading

Description of Changes

Testing

Possible Changes/Adjustments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanLeRoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianWhitneyAI commented May 2, 2024 •

edited

Loading