Add current workspace member option to workspaceMember relation field filter (#8016) #8950

eliasylonen · 2024-12-09T01:40:16Z

Almost ready to be merged.

This touches so many files that it could be good to:

merge quickly after I fix the tests
Add the "No Owner" / "No Assignee" button mentioned in I should be able to filter on workspaceMember = me #8016 separately
Fix unselecting values not working (also broken in main) right after merging

greptile-apps

PR Summary

This PR adds a "current user" filter option for workspace member relations, similar to Linear's implementation, by refactoring filter-related functionality into hooks and adding special ID handling.

Key changes:

Moves filter computation from utility functions to hooks (useComputeContextStoreFilters, useResolveFilterValue) for better state management
Adds special CURRENT_WORKSPACE_MEMBER ID handling in useResolveRelationViewFilterValue to support "me" filter option
Introduces safeStringArrayJSONSchema for consistent JSON string array validation
Refactors filter value resolution to use simpler function signatures and improved type safety
Consolidates query variable generation in new useGetQueryVariablesFromView hook

_{💡 (4/5) You can add custom instructions or style guidelines for the bot here!}

_{27 file(s) reviewed, 17 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-front/src/modules/context-store/hooks/useComputeContextStoreFilters.ts

...rc/modules/object-record/object-filter-dropdown/components/ObjectFilterDropdownDateInput.tsx

...ront/src/modules/object-record/record-filter/hooks/useComputeViewRecordGqlOperationFilter.ts

greptile-apps · 2024-12-09T01:41:53Z

...ront/src/modules/object-record/record-filter/hooks/useComputeViewRecordGqlOperationFilter.ts

+              if (recordIds.length === 0) return;
+              return {


logic: redundant check for recordIds.length === 0, already checked on line 317

greptile-apps · 2024-12-09T01:44:04Z

.../twenty-front/src/modules/views/view-filter-value/hooks/useResolveRelationViewFilterValue.ts

+    const recordIdsAndSpecialIds =
+      safeStringArrayJSONSchema.parse(viewFilterValue);


logic: The parse operation could throw if viewFilterValue is not a valid JSON array of strings. Need to handle this error case.

greptile-apps · 2024-12-09T01:44:30Z

packages/twenty-front/src/modules/views/view-filter-value/utils/resolveDateViewFilterValue.ts

+  if (viewFilterOperand === ViewFilterOperand.IsRelative) {
    return resolveVariableDateViewFilterValue(
-      viewFilter.value,
+      viewFilterValue,
    ) as ResolvedDateViewFilterValue<O>;
  }


logic: No validation is performed on viewFilterValue before passing to resolveVariableDateViewFilterValue, which could throw if value is invalid.

greptile-apps · 2024-12-09T01:44:31Z

packages/twenty-front/src/modules/views/view-filter-value/utils/resolveFilterValue.ts

logic: The entire file is being removed but there's no clear replacement shown in the PR. Need to ensure the new implementation maintains all existing filter value resolution functionality.

greptile-apps · 2024-12-09T01:44:39Z

packages/twenty-front/src/modules/views/view-filter-value/utils/resolveNumberViewFilterValue.ts

 ) => {
-  return viewFilter.value === '' ? null : +viewFilter.value;
+  return viewFilterValue === '' ? null : +viewFilterValue;


logic: unary plus operator could produce NaN for invalid number strings - consider adding validation

greptile-apps · 2024-12-09T01:45:08Z

packages/twenty-front/src/utils/validation-schemas/safeStringArrayJSONSchema.ts

+  }, z.array(z.string()))
+  .catch([]);


style: schema allows any string array - may want to add additional validation for specific string formats or lengths

FelixMalfait · 2024-12-09T08:03:38Z

Great work as always! Starting to play around with it, noticed a first bug:

Screen.Recording.2024-12-09.at.09.02.50.mov

Edit: maybe that's what you meant by 'fix unselected values - broken on main'?

FelixMalfait · 2024-12-09T08:04:57Z

In a followup PR / issue, could you maybe add an "assigned to me" view in "tasks-all.view.ts" file (this will only apply to new workspaces). This will make your feature more discoverable

FelixMalfait · 2024-12-09T09:23:05Z

I think we can forget the "No assignee" from the initial issue as there is already "is empty" which is good enough

lucasbordeau · 2024-12-09T14:41:02Z

Please fix the tests

lucasbordeau · 2024-12-09T14:47:04Z

Could you also verify that your IDE is correctly configured ? The lint problem on the tests and the prettier/eslint problems should not be part of your commit since it should be auto-fixed at save or outputing red squiggles in your console.

lucasbordeau

With selectedRecordIdsAndSpecialIds, the naming tells us that there's a code smell, "DoingThisAndThat" indicates that we should split this into two distinc flows that join at the right abstraction level.

Here we should keep the previous flow as it is, with utils instead of hooks.

The changes seem to be only for relations and selection. You should explore a solution where you create recoil atoms for storing the state of what your new UI components are doing, then using those atoms where it's really needed, and copy/paste at multiple place if that's the things to do.

For example here contextStoreFilters seems to be more appropriate to store your additional current workspace member filter, and maybe a specific parallel hook/util could handle turning the click into a record filter and modify this state ?

lucasbordeau · 2024-12-09T14:44:00Z

.../twenty-front/src/modules/views/view-filter-value/hooks/useResolveRelationViewFilterValue.ts

+        throw new Error('Current workspace member is not defined');
+      }
+
+      return recordIdOrSpecialId === 'CURRENT_WORKSPACE_MEMBER'


That doesn't seem like the right way to go, storing magic ids to bypass the normal flow is always an anti-pattern for lack of a better solution.

Also special id is too vague as a naming and feels too "magic" which is something we really want to avoid

The fact that a lot of utils has been turned to hooks to accomodate a edge case for relation type is a code smell.

We can definitely find a solution that leaves those utils as is. I feel like we're not implemeting this on the right abstraction level.

lucasbordeau · 2024-12-09T15:29:42Z

...ages/twenty-front/src/modules/views/view-filter-value/utils/resolveBooleanViewFilterValue.ts

-) => {
-  return viewFilter.value === 'true';
-};
+export const resolveBooleanViewFilterValue = (viewFilterValue: string) =>


We can keep the previous function, we don't strive to compact the code in this codebase, it's okay to keep things explicit and less compact.

lucasbordeau · 2024-12-09T15:30:24Z

packages/twenty-front/src/modules/views/view-filter-value/utils/resolveDateViewFilterValue.ts

-  ViewFilter,
-  'value'
-> & { operand: O };
-
 export const resolveDateViewFilterValue = <O extends ViewFilterOperand>(


Is this refactor needed for this feature ?

lucasbordeau · 2024-12-09T15:30:45Z

packages/twenty-front/src/modules/views/hooks/useResolveFilterValue.ts

+export const useResolveFilterValue = () => {
+  const { resolveRelationViewFilterValue } = useResolveRelationViewFilterValue();
+
+  const resolveFilterValue = <


We should keep this as a util

lucasbordeau · 2024-12-09T15:31:56Z

packages/twenty-front/src/modules/views/view-filter-value/utils/resolveNumberViewFilterValue.ts

@@ -1,7 +1,6 @@
-import { ViewFilter } from '@/views/types/ViewFilter';

 export const resolveNumberViewFilterValue = (


Is this refactor needed ?

lucasbordeau · 2024-12-09T15:32:56Z

packages/twenty-front/src/utils/validation-schemas/safeStringArrayJSONSchema.ts

@@ -0,0 +1,14 @@
+import { z } from 'zod';
+
+export const safeStringArrayJSONSchema = z


Ok to extract this as it was but the validation seems to introduce a silent error that returns [], could we throw or return a wrong validation state instead when the value is really not parsable as an array ?

…s://github.com/eliasylonen/twenty into feat/8016-workspace-member-current-user-filter

ad-elias added 2 commits December 6, 2024 22:34

Add "me" entry to workspaceMember picker

ffd7824

Resolve relation filter value and refactor

1839da5

greptile-apps bot reviewed Dec 9, 2024

View reviewed changes

Fix 'Me' option icon

b4a3c44

eliasylonen requested a review from FelixMalfait December 9, 2024 02:01

Merge branch 'main' into feat/8016-workspace-member-current-user-filter

7b59e42

FelixMalfait assigned lucasbordeau Dec 9, 2024

lucasbordeau requested changes Dec 9, 2024

View reviewed changes

ad-elias added 2 commits December 10, 2024 12:41

Fix typo

c607cbe

Merge branch 'feat/8016-workspace-member-current-user-filter' of http…

01ae9a7

…s://github.com/eliasylonen/twenty into feat/8016-workspace-member-current-user-filter

charlesBochet added the -PR: awaiting author label Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add current workspace member option to workspaceMember relation field filter (#8016) #8950

Add current workspace member option to workspaceMember relation field filter (#8016) #8950

eliasylonen commented Dec 9, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 9, 2024

greptile-apps bot Dec 9, 2024

greptile-apps bot Dec 9, 2024

greptile-apps bot Dec 9, 2024

greptile-apps bot Dec 9, 2024

greptile-apps bot Dec 9, 2024

FelixMalfait commented Dec 9, 2024 •

edited

Loading

FelixMalfait commented Dec 9, 2024

FelixMalfait commented Dec 9, 2024 •

edited

Loading

lucasbordeau commented Dec 9, 2024

lucasbordeau commented Dec 9, 2024 •

edited

Loading

lucasbordeau left a comment •

edited

Loading

lucasbordeau Dec 9, 2024 •

edited

Loading

lucasbordeau Dec 9, 2024

lucasbordeau Dec 9, 2024

lucasbordeau Dec 9, 2024

lucasbordeau Dec 9, 2024

lucasbordeau Dec 9, 2024

		const recordIdsAndSpecialIds =
		safeStringArrayJSONSchema.parse(viewFilterValue);

		@@ -1,7 +1,6 @@
		import { ViewFilter } from '@/views/types/ViewFilter';

		export const resolveNumberViewFilterValue = (

		@@ -0,0 +1,14 @@
		import { z } from 'zod';

		export const safeStringArrayJSONSchema = z

Add current workspace member option to workspaceMember relation field filter (#8016) #8950

Are you sure you want to change the base?

Add current workspace member option to workspaceMember relation field filter (#8016) #8950

Conversation

eliasylonen commented Dec 9, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 9, 2024

Choose a reason for hiding this comment

FelixMalfait commented Dec 9, 2024 • edited Loading

FelixMalfait commented Dec 9, 2024

FelixMalfait commented Dec 9, 2024 • edited Loading

lucasbordeau commented Dec 9, 2024

lucasbordeau commented Dec 9, 2024 • edited Loading

lucasbordeau left a comment • edited Loading

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

eliasylonen commented Dec 9, 2024 •

edited

Loading

FelixMalfait commented Dec 9, 2024 •

edited

Loading

FelixMalfait commented Dec 9, 2024 •

edited

Loading

lucasbordeau commented Dec 9, 2024 •

edited

Loading

lucasbordeau left a comment •

edited

Loading

lucasbordeau Dec 9, 2024 •

edited

Loading