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

"Outline Renders" tool #967

Merged
merged 25 commits into from
Feb 26, 2025
Merged

"Outline Renders" tool #967

merged 25 commits into from
Feb 26, 2025

Conversation

jwajgelt
Copy link
Contributor

@jwajgelt jwajgelt commented Feb 14, 2025

Adds the "Outline Renders" tool which highlights React components when they are rendered. Based on react-scan utility.

  • we reuse the react-scan implementation of drawing the outlines on the canvas. Accessing it from our frontend code required abstracting the renderer code and exporting it from the package. These changes are available and pulled from a public fork the react-scan repository. refactor: abstract outline rendering software-mansion-labs/react-scan#1
  • we use the bippy library for instrumenting the React rendering process. We include the built library code (extracted from the official release) in this repo in the packages/vscode-extension/lib/bippy directory and inject it into the RN Application bundle.
    We do it this way because it needs to be included in the final extension package in order to be injected into the RN application, and only the lib directory is available to Metro.

How Has This Been Tested:

  • Open the "Tools" dropdown and toggle "Outline Renders".
  • Trigger component updates in the application.
  • The updated components should be briefly highlighted, with their names and update counter displayed.
  • Resizing the phone Preview should not break the outlines' positions.
  • The tool's enabled status should persist app, extension and IDE launches/reloads.

@jwajgelt jwajgelt requested a review from kmagiera February 14, 2025 13:42
@jwajgelt jwajgelt self-assigned this Feb 14, 2025
Copy link

vercel bot commented Feb 14, 2025

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

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 26, 2025 8:23am

@jwajgelt jwajgelt force-pushed the @jwajgelt/render-outlines branch from 1543254 to ae7dfe3 Compare February 17, 2025 07:54
jwajgelt added a commit that referenced this pull request Feb 17, 2025
Makes it possible to register tool plugins with no corresponding VSC
panel. This is meant to be used by
#967

### How Has This Been Tested: 
Remove `openTool` method from one of the tool plugins and verify:
- the project still typechecks
- the tool dropdown does not display the "Open Panel" button
@jwajgelt jwajgelt force-pushed the @jwajgelt/render-outlines branch from ae7dfe3 to cf04014 Compare February 17, 2025 10:34
@jwajgelt jwajgelt marked this pull request as ready for review February 17, 2025 11:46
@filip131311 filip131311 self-requested a review February 18, 2025 11:02
Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

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

I left some inline comments. I also have additional question regarding how this works:

If i click multiple tiems on something that changes UI (as in example video) the count will count all the renders of the element that shows up. My question is "when is onCommitFiberRoot triggered? if it counts those multiple changes as one commit? Is it something that is normal for react and i shouldn't be worried about it?

Screen.Recording.2025-02-18.at.14.55.33.mov

height: screenHeight,
};
} else {
const { width: windowWidth, height: windowHeight } = Dimensions.get("window");
Copy link
Collaborator

Choose a reason for hiding this comment

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

in Inspector code we use uniform:

function getInspectorDataForCoordinates(mainContainerRef, x, y, requestStack, callback) {
  const { width: screenWidth, height: screenHeight } = Dimensions.get("screen");

could you write up coments here explaining why it is different for different platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To correct for the status bar on Android.
Dimensions returned from measureInWindow are relative to the window, so we should be using that, but because the canvas we use later to draw the overlays covers the entire screen, we need to translate the measurements from window coordinate space to screen.
On iOS it seems to be the same, but on Android you need to translate everything by the Status Bar height.

function getWindowRect() {
if (Platform.OS === "android") {
const { width: screenWidth, height: screenHeight } = Dimensions.get("screen");
const statusBarHeight = StatusBar.currentHeight || 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The StatusBar.currentHeight variable is not respected if the user uses react-native-screens and probably in more scenarios, I don't know if it wouldn't be better to ignore the misalignment here hoping that the problem will go away when android forces edge-to-edge instead of exposing ourselves to unpredictable bugs.

Alternatively we might try to find a better way to measure StatusBar for example by dumping native component hierarchy using adb (that would also fix inspector problems), but I Think solving that issue is outside of this PR scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work fine Bluesky which, as far as I can tell, uses screens (through native-stack). The app is rendered below the status bar though, so this might be why.
IMO it's better to have this solution, which seems to work for the examples I've checked, than ignore the status bar altogether and have the outlines always offset.

Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

I'd prefer that we actually use the name react-scan instead of "render outlines" given we rely on the tool.

We also want to have a bit more details about this setup with the extra repo. Specifically, we should put in the description how these different repositoreis are structured. What are the changes that we made. Why is bippy source pulled into the repo etc

@jwajgelt
Copy link
Contributor Author

I'd prefer that we actually use the name react-scan instead of "render outlines" given we rely on the tool.

I'm not convinced that's a good idea.

  • The name is not very descriptive. That way the user needs to already be familiar with react-scan to know what our tool is supposed to do. Granted, "Outline Renders" might not be the best name in that metric either, something like "Outline Components when Rendered" might have been better.
  • I think it gives the wrong expectation to users. What we currently implement is a very small part of what the "real" react-scan does. If someone who uses react-scan on web tries to use our tool, they would probably be disappointed with how little they can do compared to it.

@jwajgelt jwajgelt requested a review from kmagiera February 20, 2025 06:43
@jwajgelt jwajgelt force-pushed the @jwajgelt/render-outlines branch from a4f6a9e to d720247 Compare February 21, 2025 09:11
jwajgelt added a commit that referenced this pull request Feb 21, 2025
Allows tools to configure (via the `persist` boolean property) whether
their enabled/disabled status should persist IDE restarts.
This is motivated by the "Outline Renders" tool (PR #967), which
shouldn't be enabled when the IDE starts.

### How Has This Been Tested: 
- change the `persist` value for one of the tool plugins to `false`
- verify that when the extension is restarted:
  - tools which have `persist` set to `true` remember their status
  - tools which have `persist` set to `false` are disabled
@jwajgelt jwajgelt force-pushed the @jwajgelt/render-outlines branch from d720247 to ea4a5f6 Compare February 21, 2025 13:04
Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Looks good. As final thought I think we should add the license to ale be included in the notices file (one that we generate in buiild step).
I think that we can use submodules-NOTICES.json for this reason despite dippy not being in submodule. If we do that, we don't need LICENSE file in the source tree

@jwajgelt jwajgelt force-pushed the @jwajgelt/render-outlines branch from a33dd66 to d2a5525 Compare February 26, 2025 08:23
@jwajgelt jwajgelt merged commit ec88b73 into main Feb 26, 2025
4 checks passed
@jwajgelt jwajgelt deleted the @jwajgelt/render-outlines branch February 26, 2025 08:26
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.

3 participants