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

Fix activity target picker #8812

Merged
merged 1 commit into from
Nov 30, 2024
Merged

Fix activity target picker #8812

merged 1 commit into from
Nov 30, 2024

Conversation

charlesBochet
Copy link
Member

This PR is fixing the Task/Note Target picker.

Issue

A few weeks ago, we have simplified the recordPicker logic. During this refacto, we stopped making sure the selected records were fetched.
However, the optimistic update of the activity supposed that current activityTargets are present in the selection in the picker in order to filter out the removed items.

Fix

In case we don't find the record in the picker selection, we don't filter it out (it means the user cannot have removed it)

Next step

@ijreilly I think we should put it back, as the selected records do not appear on top anymore

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 fixes a bug in the activity target picker where activity targets were being incorrectly filtered out during updates due to incomplete record fetching.

  • Modified filtering logic in /packages/twenty-front/src/modules/activities/inline-cell/components/ActivityTargetInlineCellEditMode.tsx to preserve activity targets not found in selection
  • Fixed optimistic update behavior to prevent accidental removal of valid activity targets
  • Ensures backward compatibility with existing activity target data by maintaining unfetched records

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +110 to +112
return recordSelectedInMultiSelect
? recordSelectedInMultiSelect.selected
: true;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: This ternary could be simplified to return !recordSelectedInMultiSelect || recordSelectedInMultiSelect.selected

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM

@charlesBochet charlesBochet merged commit 69f052e into main Nov 30, 2024
19 checks passed
@charlesBochet charlesBochet deleted the fix-activity-target-picker branch November 30, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants