Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

eliasylonen
Copy link
Contributor

@eliasylonen eliasylonen commented Dec 9, 2024

Issue: #8016

Almost ready to be merged.

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

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@FelixMalfait
Copy link
Member

FelixMalfait commented Dec 9, 2024

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
Copy link
Member

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
Copy link
Member

FelixMalfait commented Dec 9, 2024

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

@lucasbordeau
Copy link
Contributor

Please fix the tests

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Dec 9, 2024

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.

Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

throw new Error('Current workspace member is not defined');
}

return recordIdOrSpecialId === 'CURRENT_WORKSPACE_MEMBER'
Copy link
Contributor

@lucasbordeau lucasbordeau Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eliasylonen eliasylonen force-pushed the feat/8016-workspace-member-current-user-filter branch 2 times, most recently from db7ac12 to 01ae9a7 Compare December 21, 2024 19:31
@eliasylonen
Copy link
Contributor Author

Continuing in PR #9182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants