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(media): filter first before getting git blob hash #1060

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Dec 15, 2023

Problem

Sites with a lot of images cannot load their images on the CMS.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Improvements:

  • Perform a filter first before getting the Git blob hash, which was found to be quite expensive for a large number of files.

Bug Fixes:

  • The addedTime attribute was accidentally reverted earlier, fixed that so that the added times are now correct on EFS.
  • Skip checking the .git folder, so we should have less error messages in the logs.

Tests

Smoke test:

  • Navigate to moe-ast-moehc with the repo on GGS (i.e. email-login)
  • Attempt to load the images page
  • The page should load fairly quickly

Deploy Notes

None

@dcshzj dcshzj requested a review from a team December 15, 2023 06:59
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

lgtm. was wondering why we still force-fit the pagination stuff into listDirectoryContents given that they have differing goals (and resulted in listDirectoryContent having a huge API but default arguments help).

if we foresee the pagination stuff to always occur with listing the contents then ok, otherwise more in favour of breaking this up

src/services/db/GitHubService.ts Outdated Show resolved Hide resolved
@dcshzj
Copy link
Contributor Author

dcshzj commented Dec 15, 2023

if we foresee the pagination stuff to always occur with listing the contents then ok, otherwise more in favour of breaking this up

I don't have a good answer to this, so I have split the function up. Reading a media directory is the only time that needs pagination right now I believe, and it is also the only time that requires the Git blob hash.

@dcshzj dcshzj requested a review from seaerchin December 15, 2023 08:24
@dcshzj dcshzj merged commit c5d666a into develop Dec 15, 2023
10 checks passed
@mergify mergify bot deleted the fix/large-image-directories branch December 15, 2023 08:54
This was referenced Dec 18, 2023
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