Skip to content

Feat/status page pagespeed section#3452

Open
akashmannil wants to merge 6 commits into
developfrom
feat/status-page-pagespeed-section
Open

Feat/status page pagespeed section#3452
akashmannil wants to merge 6 commits into
developfrom
feat/status-page-pagespeed-section

Conversation

@akashmannil

Copy link
Copy Markdown
Collaborator

Adding PageSpeed section to the Status page.

accidently closed before force pushing to fix rebase issue
#3443

All reviewed changes have been made

  • (Do not skip this or your PR will be closed) I deployed the application locally.
  • (Do not skip this or your PR will be closed) I have performed a self-review and testing of my code.
  • I have included the issue # in the PR.
  • I have added i18n support to visible strings (instead of <div>Add</div>, use):
const { t } = useTranslation();
<div>{t('add')}</div>
  • I have not included any files that are not related to my pull request, including package-lock and package-json if dependencies have not changed
  • I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
  • I made sure font sizes, color choices etc are all referenced from the theme. I don't have any hardcoded dimensions.
  • My PR is granular and targeted to one specific feature.
  • I ran npm run format in server and client directories, which automatically formats your code.
  • I took a screenshot or a video and attached to this PR if there is a UI change.

@akashmannil akashmannil force-pushed the feat/status-page-pagespeed-section branch from 6427262 to c7d9331 Compare March 30, 2026 12:26

@ajhollid ajhollid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's getting there! Some more mostly maintenance related changes, please have a look at my comments. Thank you!

Comment thread client/src/Pages/StatusPage/Status/Components/PageSpeedMetrics.tsx Outdated
Comment thread client/src/Pages/StatusPage/Status/Components/PageSpeedMetrics.tsx Outdated
Comment thread client/src/Pages/StatusPage/Status/Components/PageSpeedMetrics.tsx Outdated
Comment thread client/src/Pages/StatusPage/Status/Components/PageSpeedMetrics.tsx Outdated
Comment thread client/src/Validation/statusPage.ts Outdated

@ajhollid ajhollid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks pretty good, I think just a naming convention issue and we should be good here.

Comment thread client/src/Types/StatusPage.ts Outdated
@akashmannil

Copy link
Copy Markdown
Collaborator Author

Thanks for the suggestions. made changes to reflect naming convention changes. Also,

  • Moved flex layout props from sx to native Grid props
  • Removed nullish coalescing — introduced a PageSpeedCheck interface with required fields to guarantee the type
  • Cast latestCheck as PageSpeedCheck since pagespeed monitors always have these values

@ajhollid ajhollid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly good, major issue with the TimescaleDB implementation though, please be sure to review and actually test that.

We should avoid the cast to PageSpeedCheck as well since it's type is narrower than Check. Casting to PageSpeedCheck type makes invalid assumptions about what data is present in a Check object.

Comment thread client/src/Pages/StatusPage/Status/Components/PageSpeedMetrics.tsx Outdated
@akashmannil

Copy link
Copy Markdown
Collaborator Author

@ajhollid made changes based on comments. Please do let me know if it needs changes

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