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

feat: react scan #445

Merged
merged 29 commits into from
Dec 19, 2024
Merged

feat: react scan #445

merged 29 commits into from
Dec 19, 2024

Conversation

JBR90
Copy link
Contributor

@JBR90 JBR90 commented Dec 12, 2024

Description

  • Introduced react-scan for performance monitoring

  • Added useReactScan hooks that can be dropped into component to track render of all or passed in component. Will console table of all components or log of individual component.

  • Integrated an example Playwright tests to validate unnecessary renders. (unfortunately only works locally , so have removed for now)

dev:react-scan: Enables React Scan for local performance monitoring.
dev:react-scan-monitor: Tracks performance in the React Scan dashboard.
For dashboard access, ask me for login credentials.
React Scan dashboard URL: React Scan Dashboard

https://github.com/aidenybai/react-scan
image

How to test

Run dev:react-scan and go to lesson chat
Run dev:react-scan-monitor and interact with the site, go to React Scan Dashboard to see render report

Screenshots

react-scan monitoring dashboard

image

logs from useReactScan

image

react-scan overlay with number of renders

image

Checklist

  • Manually tested across browsers / devices
  • Considered impact on accessibility
  • Does this PR update a package with a breaking change

Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 3:39pm

@JBR90 JBR90 changed the title Feat react scan feat: react scan Dec 12, 2024
Copy link

github-actions bot commented Dec 12, 2024

Playwright test results

failed  1 failed
passed  13 passed
flaky  1 flaky
skipped  2 skipped

Details

report  Open report ↗︎
stats  17 tests across 16 suites
duration  10 minutes, 6 seconds
commit  6161178

Failed tests

Common persona - mobile › tests/aila-chat/full-romans.mobile.test.ts › Full aila flow with Romans fixture

Flaky tests

No persona › tests/modifiy-lesson.test.ts › Modify a lesson plan › Modify a lesson resource

Skipped tests

No persona › tests/auth.test.ts › authenticate through Clerk UI
No persona › tests/chat-performance.test.ts › Component renders during lesson chat › There are no unnecessary rerenders across left and right side of chat

(a, b) => b.renderCount - a.renderCount,
);

console.table(sortedReports);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add a table method on the logger? This will probably give us an unexpected use of console linting warning?

};
});
await page.addInitScript(() => {
console.log("Window object:", typeof window);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be using the logger?

Copy link
Contributor Author

@JBR90 JBR90 Dec 18, 2024

Choose a reason for hiding this comment

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

Was having issues with the test so this was for debugging, i've removed the test now for now. It's not working in the preview builds unfortunately.

window.process.env.NEXT_PUBLIC_ENABLE_RENDER_SCAN = "true";
}

// console.log("Window process:", window.process);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these before merge?

@JBR90 JBR90 requested a review from stefl December 18, 2024 10:08
@JBR90 JBR90 marked this pull request as ready for review December 18, 2024 10:08
@JBR90 JBR90 requested a review from a team December 18, 2024 10:13
@@ -31,7 +31,7 @@ type ChildKey =
| "aila:stream"
| "aila:rag"
| "aila:testing"
| "aila:chat"
| "aila:chat-performance"
Copy link
Collaborator

@codeincontext codeincontext Dec 18, 2024

Choose a reason for hiding this comment

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

aila is for the aila package. Maybe you want performance, chat:performance or ui:performance?

package.json Outdated
"dev": "FORCE_COLOR=1 turbo dev --parallel --ui=stream --log-prefix=none --filter=!@oakai/db",
"dev:react-scan": "NEXT_PUBLIC_ENABLE_RENDER_SCAN=true turbo dev --parallel --ui=stream --log-prefix=none --filter=!@oakai/db",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be something along the lines of NEXT_PUBLIC_ENABLE_RENDER_SCAN=true pnpm dev?

Comment on lines 102 to 116
.filter(([componentName]) => {
// Exclude styled components and other library-generated components
const isCustomComponent =
// Check if name exists and doesn't start with known library prefixes
componentName &&
!componentName.startsWith("Styled") &&
!componentName.includes("styled") &&
!componentName.startsWith("_") &&
!componentName.includes("$") &&
componentName !== "div" &&
componentName !== "span";

return isCustomComponent;
})
.map(transformReport);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good candidate for a function to hide these stringy details out of the way and make the hook easier to follow

package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@codeincontext codeincontext left a comment

Choose a reason for hiding this comment

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

Tested locally and it seems to be working well. It would be interesting to count the renders between key points like each streaming state, as different things could be happening within each 10s window? Just a thought really for the future!

Copy link
Collaborator

@codeincontext codeincontext left a comment

Choose a reason for hiding this comment

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

Looking great!

@JBR90 JBR90 merged commit 2f255b3 into main Dec 19, 2024
17 of 18 checks passed
@JBR90 JBR90 deleted the feat-react-scan branch December 19, 2024 08:43
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants