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(seo): fix seo score #331

Merged
merged 6 commits into from
Jul 18, 2023
Merged

fix(seo): fix seo score #331

merged 6 commits into from
Jul 18, 2023

Conversation

AntonLantukh
Copy link
Collaborator

Description

Fix seo score

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Visit the preview URL for this PR (updated for commit 466e8e3):

https://ottwebapp--pr331-fix-seo-score-e42qzimy.web.app

(expires Sat, 12 Aug 2023 13:40:08 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 3 times, most recently from c32d163 to 95a55fe Compare July 6, 2023 11:25
@AntonLantukh AntonLantukh marked this pull request as ready for review July 6, 2023 12:01
@AntonLantukh
Copy link
Collaborator Author

Looks like firebase adds a no-index header for PR envs:

Screenshot 2023-07-06 at 14 10 37

Copy link
Contributor

@dbudzins dbudzins left a comment

Choose a reason for hiding this comment

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

It's probably good to fix some things on the demo config dialog, but what we really want to test is the home page with all the shelves. Can you also try to get the lhci action working appropriately with that?

@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 8 times, most recently from e5be802 to c2af33f Compare July 12, 2023 09:22
@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 6 times, most recently from b2d0e69 to 0013e7c Compare July 12, 2023 11:59
@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 10 times, most recently from 83efb84 to c14af24 Compare July 12, 2023 14:34
@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 2 times, most recently from 0c3bc15 to 814e3ac Compare July 13, 2023 09:01
@AntonLantukh AntonLantukh force-pushed the fix/seo-score branch 9 times, most recently from 4bc519b to 0e0f2e9 Compare July 13, 2023 13:30
@AntonLantukh
Copy link
Collaborator Author

AntonLantukh commented Jul 13, 2023

@dbudzins I have added Lighthouse workflow to the Preview workflow as a separate job with only one (SEO) required check.

I have also increased numberOfRuns to 3 to improve the accuracy. One more thing I spotted is that a server created 'on the go' within Lighthouse workflow is quite slow and doesn't reflect actual performance results.

I decided to use a separate job in the same workflow to simplify the way we can pass the link. I tried several approaches:

  1. I used workflow_dispatch in the Lighthouse workflow and triggered it from the Preview workflow manually using gh cli. It worked, but the check didn't appear in the PR.

  2. I used workflow_run, but according to the docs "This event will only trigger a workflow run if the workflow file is on the default branch."

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

"Default" one is a develop branch and it adds some complexity to debug and develop such workflow as the version of the default branch is taken. And I am not sure that the PR check will be present in this case.

  workflow_run:
    workflows: [Firebase Preview]
    types:
      - completed
  1. I used lewagon/[email protected] package to wait for Preview job in the Lighthouse workflow and continue when preview link is present. I uploaded an artifact in Preview job and wanted to get acces to in Lighthouse job. However it didn't work out of the box. By default, you can only upload / download an artifact within one workflow. There is a custom job to make it possible within different workflows but it already adds too much complexity.
    steps:
      - name: Wait for preview link to be created
        uses: lewagon/[email protected]
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          check-name: build_and_preview
          repo-token: ${{ secrets.GITHUB_TOKEN }}
          wait-interval: 10
          allowed-conclusions: success,skipped,cancelled
          verbose: true
      - name: Grab preview link
        uses: actions/download-artifact@v3
        with:
          name: preview_link
      - name: Set env
        run: echo "PREVIEW_LINK=$(cat preview_link.txt)" >> $GITHUB_ENV
      - uses: actions/checkout@v2
      - name: Install Lighthouse CI
        run: sudo yarn global add @lhci/[email protected]
      - name: Run Lighthouse CI
        run: lhci autorun --collect.url=${{ env.PREVIEW_LINK }}?app-config=gnnuzabk --config=./lighthouserc.js

@dbudzins
Copy link
Contributor

@AntonLantukh OK, so basically to summarize you are running the lighthouse SEO check on the preview build, right? That's a really clever way to do this. Nice work figuring it out and implementing it!

Copy link
Contributor

@dbudzins dbudzins left a comment

Choose a reason for hiding this comment

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

Nice work and clever solution to the lighthouse runs.

@AntonLantukh AntonLantukh merged commit 1f1332e into develop Jul 18, 2023
9 checks passed
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