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: notifications #299

Merged
merged 1 commit into from
Mar 3, 2025
Merged

feat: notifications #299

merged 1 commit into from
Mar 3, 2025

Conversation

RobPruzan
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Feb 28, 2025

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

Name Status Preview Comments Updated (UTC)
react-scan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 1, 2025 2:02am

@@ -23,7 +23,7 @@
"enabled": true
},
"linter": {
"enabled": true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo




export const not_globally_unique_generateId = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changing name

listeners.forEach((listener) => listener(event));

const events = [...get().state.events, event];
const applyOverlapCheckToLongRenderEvent = (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this handles a "frame drop" event overlapping with an interaction event, which will always be the case when there's an interaction that caused the frame drop

this is a suboptimal implementation because it may cause a flicker, forcing us to read a lagged version of this downstream

the correct solution is to check if an interaction is ongoing during a frame drop event, and kill it if so


let framesDrawnInTheLastSecond: Array<number> = [];

export function startLongPipelineTracking() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you imagine a pipeline to drawing frames

| start-pipeline, js , dom stuff, paint , draw |

this tracks

| timing-start , start-pipeline, js, dom stuff, paint , draw | timing-end , timing-start , start-pipeline ...

so this correctly tracks "dropped frames" as opposed to tracking requestAnimationFrame times (wrong) and long tasks (not full picture)


type Subscriber<T> = (data: T) => void;

export class Store<T> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should use our zustand store clone over this

return this.currentValue;
}
}
export const MAX_INTERACTION_BATCH = 150;
Copy link
Collaborator Author

@RobPruzan RobPruzan Mar 1, 2025

Choose a reason for hiding this comment

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

bounded array makes sure we never cause a memory leak

});

let currFrame: ReturnType<typeof requestAnimationFrame> | null = null;
export const drawHighlights = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a state machine to handle transitions between drawing states

https://million-js.slack.com/archives/C06FBU2AY7Q/p1739766745053689

// a set of entities communicate to each other through channels
// the state in the channel is persisted until the receiving end consumes it
// multiple subscribes to the same channel will likely lead to unintended behavior if the subscribers are separate entities
class PerformanceEntryChannels<T> implements PerformanceEntryChannelsType<T> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is needed if we want to map our custom interaction tracking to performance entry tracking- makes bidirectional communication and persisted state trivial. Inspired from go channels

}
>;

export const createChildrenAdjacencyList = (root: Fiber, limit: number) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sometimes a user will click a button that has some content inside of it. If there's an SVG inside of it, the name of the SVG's composite fiber ancestor is normally pretty descriptive, so we can use this in the future

const isProduction: boolean = process.env.NODE_ENV === "production";
const prefix: string = "Invariant failed";

// FIX ME THIS IS PRODUCTION INVARIANT LOL
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uh gonna fix

skipBoundaries: true,
};

const FILTER_PATTERNS = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo: dedupe with monitoring

>;

/**
* we need to fix:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo: delete this comment its just my internal reasoning

};

type ShouldContinue = boolean;
const trackDetailedTiming = ({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice util for chaining callbacks into something we can track times for

}
if (
info.target instanceof HTMLInputElement ||
info.target instanceof HTMLSelectElement
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

click:change runs after click, but only will fire for HTMLInputElement and HTMLSelectElement, so we need to make sure we track this, unless we may miss javascript time from these handlers

) {
return;
}
if (Date.now() - lastInteractionRef.current.stageStart > 2000) {
Copy link
Collaborator Author

@RobPruzan RobPruzan Mar 1, 2025

Choose a reason for hiding this comment

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

idk if this is needed anymore, fool proof validation

once: true,
});

// this is an edge case where a click event is not fired after a pointerdown
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo: remove comment, old internal reasoning

framePresentTime: number | null;
formattedReactData: string;
}) => {
return `I will provide you with a set of high level, and low level performance data about an interaction in a React App:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i wanted to cry writing these prompts

options.preserveSymlinks = true;
},
},
// {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remindme: add back

@RobPruzan RobPruzan mentioned this pull request Mar 1, 2025
@RobPruzan RobPruzan marked this pull request as ready for review March 1, 2025 01:50
Copy link

pkg-pr-new bot commented Mar 1, 2025

Open in Stackblitz

npm i https://pkg.pr.new/aidenybai/react-scan@299

commit: 8413c9d

@@ -0,0 +1,115 @@
import { Fiber } from 'bippy';
export const getChildrenFromFiberLL = (fiber: Fiber) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

term: what's "LL"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

linked list

const queue: Array<[node: Fiber, parent: Fiber | null]> = [];
const visited = new Set<Fiber>();

queue.push([root, root.return]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

an actual linked list would probably be less memory, but whatever, this is fine. I'll just add a note to myself.

Comment on lines +545 to +546
startTimingTracking();
createNotificationsOutlineCanvas();
Copy link
Collaborator

Choose a reason for hiding this comment

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

if toolbar is recreated (browser extension on/off mode) these should not be re-executed maybe ... at least createNotificationsOutlineCanvas

@@ -521,6 +556,21 @@ const initToolbar = (showToolbar: boolean) => {
createToolbar(shadowRoot);
};

const createNotificationsOutlineCanvas = () => {
try {
const highlightRoot = document.documentElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe is a good idea to keep everything inside the react-scan web component to be able to create/destroy evertything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes will add todo

Comment on lines -3 to +10
width: 480,
height: 36,
initialHeight: 36 * 10,
width: 550,
height: 350,
initialHeight: 400,
} as const;

export const MIN_CONTAINER_WIDTH = 240;

export const LOCALSTORAGE_KEY = 'react-scan-widget-settings';
export const LOCALSTORAGE_KEY = 'react-scan-widget-settings-v2';
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should fix this ... migrate the old settings ... or at least remove/override them
TODO: @pivanov - fix it with the UI changes

@RobPruzan
Copy link
Collaborator Author

Going to merge, leaving any todos in code

any patches we can send directly to main, this is long overdue

@RobPruzan RobPruzan merged commit e77959a into main Mar 3, 2025
5 checks passed
@lxsmnsyc lxsmnsyc deleted the notifications branch March 3, 2025 05:15
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