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

Improve performance of demo workspace #6201

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Improve performance of demo workspace #6201

merged 3 commits into from
Jul 12, 2024

Conversation

gitstart-twenty
Copy link
Contributor

@gitstart-twenty gitstart-twenty commented Jul 10, 2024

Description

  1. We've created another PR here here that adds the placeholder images to the placeholder-images repo

  2. For now, pages is deploying to the TWNTY-3514 on placeholder-images. If the PR is merged, we'll need to update pages to deploy from main

Refs

Demo

demo-workspace.mp4

Fixes #3514

@gitstart-twenty
Copy link
Contributor Author

How to set up the demo workspace locally

  1. Go to the .env file in the twenty-server package and look for the line # DEMO_WORKSPACE_IDS=REPLACE_ME_WITH_A_RANDOM_UUID
  2. Uncomment the line and replace REPLACE_ME_WITH_A_RANDOM_UUID with a random UUID4 string
  3. Run the command yarn nx run twenty-server:command workspace:seed:demo
  4. You should now be able to log in with any of the seed demo users
    1. Refer to packages/twenty-server/src/database/typeorm-seeds/core/demo/users.ts for the user's credentials

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

  • Replaced base64 encoded avatar images with URLs to GitHub-hosted placeholder images in packages/twenty-server/src/engine/workspace-manager/standard-objects-prefill-data/person.ts
  • Reduced data size for database inserts to improve performance

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

@FelixMalfait
Copy link
Member

Thanks!

  1. For the 6 prefilled objects since those are famous people, could you please keep the original base64 image? (but fetch it from the url / host them on the repo you created). Note, we need to make sure never to delete this url since now these urls will be hardcoded for each new workspace creation @charlesBochet

  2. Could you please look into why it's not displayed in chips?

Screenshot 2024-07-10 at 14 11 30 Screenshot 2024-07-10 at 14 12 01
  1. Since we want to discourage using base64 coming from the database, I'd advocate renaming getImageAbsoluteURIOrBase64 to getImageAbsoluteURI and removing the possibility to load it via base64 (cc @charlesBochet let us know if you disagree)

@gitstart-twenty
Copy link
Contributor Author

Thanks!

  1. For the 6 prefilled objects since those are famous people, could you please keep the original base64 image? (but fetch it from the url / host them on the repo you created). Note, we need to make sure never to delete this url since now these urls will be hardcoded for each new workspace creation @charlesBochet

On it!

  1. Could you please look into why it's not displayed in chips?
Screenshot 2024-07-10 at 14 11 30 Screenshot 2024-07-10 at 14 12 01

This same issue exists on https://demo.twenty.com, we're looking into it though!

  1. Since we want to discourage using base64 coming from the database, I'd advocate renaming getImageAbsoluteURIOrBase64 to getImageAbsoluteURI and removing the possibility to load it via base64 (cc @charlesBochet let us know if you disagree)

On it!

@gitstart-twenty
Copy link
Contributor Author

Hello @FelixMalfait 👋

The problem with the chips happens with these files, here is the trace:
useLoadRecordIndexTable => useRecordTableRecordGqlFields => useRecordTableRecordGqlFields => getObjectMetadataIdentifierFields

Inside getObjectMetadataIdentifierFields the imageIdentifierFieldMetadataItem is null, because none of the fields pass the .find check, and this happens because objectMetadataItem.imageIdentifierFieldMetadataId is null. and this causes the Graphql query not to add the avatarUrl key on the request.

It's seems this property is should be added in the backend when these fields are created, or when the data model itself is created, but in the case of the demo data, it doesn't happen since they have their own creation flow in the database. considering this only happens in the demo workspace, is this also happens on the production app?
Could you give us more context about how objectMetadataItem.imageIdentifierFieldMetadataId is handled by the server?

About renaming the function, there are 3 images that still use base64 (company logo images), and they are also passed to the function, if we remove the possibility of loading it via base64, it will not be possible to load these images, should we add these images to the placeholder repo too?
image

@charlesBochet
Copy link
Member

I have taken a look to this one:

  • if we add the right imageIdentifierFieldMetadataId to person object, it does display the image:
image ==> so I let's wait for @Weiko to fill the imageIdentifierFieldMetadataId and it will fix the issue. LGTM for this PR
  • Regarding the new worskpace data prefill (not the demo one), let's do the same (and I see you have done it in your PR). LGTM

  • Regarding the last issue which to deprecate the getImageAbsoluteURIOrBase64, let's also upload it on our new placeholder pages and agree with @FelixMalfait that we can remove the base64 possibility which does not seem to be a good pattern

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM, merging
@gitstart-twenty could you take care of the workspace logo base64 in a follow-up PR?

@charlesBochet charlesBochet merged commit 12c68fd into main Jul 12, 2024
6 checks passed
@charlesBochet charlesBochet deleted the TWNTY-3514 branch July 12, 2024 19:34
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

(updates since last review)

  • Added comprehensive Kubernetes and Terraform deployment configurations for TwentyCRM (packages/twenty-docker/k8s/README.md, packages/twenty-docker/k8s/manifests/*, packages/twenty-docker/k8s/terraform/*)
  • Updated address fields to more granular components across multiple files for better data structure and performance (packages/twenty-server/src/database/typeorm-seeds/workspace/companies.ts, packages/twenty-server/src/modules/company/standard-objects/company.workspace-entity.ts)
  • Introduced new GraphQL error handling utilities and hooks for improved error tracking (packages/twenty-server/src/engine/core-modules/graphql/hooks/use-graphql-error-handler.hook.ts, packages/twenty-server/src/engine/core-modules/graphql/utils/*)
  • Refactored and modularized contact creation management (packages/twenty-server/src/modules/contact-creation-manager/*)
  • Removed deprecated fields and updated sync status enums for message channels (packages/twenty-server/src/modules/messaging/common/standard-objects/message-channel.workspace-entity.ts)

184 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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

(updates since last review)

  • Deleted packages/twenty-server/src/engine/workspace-manager/demo-objects-prefill-data/companies-demo.json.ts
  • Modified column name from 'address' to 'addressAddressCity' in packages/twenty-server/src/engine/workspace-manager/demo-objects-prefill-data/company.ts
  • Ensure column name change is consistent across the codebase to avoid data issues

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

lucasbordeau pushed a commit that referenced this pull request Jul 29, 2024
…rBase64` function (#6282)

### Description

1. This PR is a continuation of a previous PR:
#6201 (review)

2. One test case was removed here:
`packages/twenty-front/src/utils/image/__tests__/getImageAbsoluteURI.test.ts`
because since we are not handling base64 images anymore, the result is
the same of the last test case. Would you rather we update the test
instead?


### Refs

- #3514
- #6201

### Demo


https://www.loom.com/share/4f32b535c77a4d418e319b095d09452c?sid=df34adf8-b013-44ef-b794-d54846f52d2d

Fixes #3514

---------

Co-authored-by: gitstart-twenty <[email protected]>
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.

Improve performance of demo workspace
3 participants