fix: fix for react strict mode #107
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nce of react layer via react ref.
Reviewer's GuideStandardizes event subscriptions with automatic cleanup, refactors resize and unmount logic across layers, hooks, and services for React Strict Mode compatibility, introduces styling props ( Sequence diagram for updated useLayer hook lifecyclesequenceDiagram
actor ReactComponent
participant useLayer
participant Graph
participant LayerInstance
ReactComponent->>useLayer: mount
useLayer->>Graph: addLayer(layerCtor, props)
Graph-->>useLayer: LayerInstance
useLayer->>useLayer: setLayer(LayerInstance)
ReactComponent->>useLayer: unmount
useLayer->>Graph: detachLayer(LayerInstance)
Note over useLayer,Graph: Ensures cleanup on unmount
ER diagram for updated event subscription and cleanup in Graph, Layers, and LayererDiagram
GRAPH ||--o{ LAYERS : manages
LAYERS ||--o{ LAYER : contains
LAYER }o--|| REACTLAYER : extends
LAYERS ||--o{ RESIZEOBSERVER : uses
LAYERS ||--o{ WINDOW : listens_to
LAYER ||--o{ EVENT : subscribes
LAYER ||--o{ ROOT_ELEMENT : listens_to
GRAPH ||--o{ CAMERA_SERVICE : updates
LAYER ||--o{ HTML_ELEMENT : applies_class
Class diagram for updated Layer and ReactLayer structureclassDiagram
class Layer {
+unmountLayer()
+afterInit()
+detachLayer()
+getDRP()
+unmount()
+propsChanged(nextProps)
}
class ReactLayer {
+afterInit()
+propsChanged(nextProps)
+applyBlockListClassName(oldClassName, newClassName)
+blockListClassName: string
}
Layer <|-- ReactLayer
Class diagram for new parseClassNames utilityclassDiagram
class parseClassNames {
<<function>>
+parseClassNames(className: string): string[]
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @draedful - I've reviewed your changes - here's some feedback:
- Emitter: by removing the destroyed flag guards in off/emit/destroy, callbacks may still fire on a destroyed emitter—consider reintroducing a guard to prevent post-destroy emissions.
- useLayer hook now creates and tears down layers inside a standard useEffect, which can delay initial props assignment and cause render mismatches in Strict Mode—consider using useLayoutEffect or synchronously setting props on mount.
- LayersService switched from throttling+change-check to a 16ms debounce without verifying size changes, which may trigger redundant renders on every resize—consider restoring the change detection or fine-tuning the debounce parameters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Emitter: by removing the destroyed flag guards in off/emit/destroy, callbacks may still fire on a destroyed emitter—consider reintroducing a guard to prevent post-destroy emissions.
- useLayer hook now creates and tears down layers inside a standard useEffect, which can delay initial props assignment and cause render mismatches in Strict Mode—consider using useLayoutEffect or synchronously setting props on mount.
- LayersService switched from throttling+change-check to a 16ms debounce without verifying size changes, which may trigger redundant renders on every resize—consider restoring the change detection or fine-tuning the debounce parameters.
## Individual Comments
### Comment 1
<location> `src/utils/Emitter.ts:10` </location>
<code_context>
const getTime = () => performance.now();
type EmitterEventsDefinition = Record<string, (...args: unknown[]) => void>;
export class Emitter<T extends EmitterEventsDefinition = EmitterEventsDefinition> {
- private destroyed: boolean;
-
private gcLaunched: boolean;
</code_context>
<issue_to_address>
The destroyed flag and related checks have been removed from Emitter.
This change allows methods like off, emit, and _walkGC to be called on a destroyed Emitter, which could cause unexpected behavior or memory leaks if handlers aren't cleaned up. Please confirm this is intentional and safe for all use cases.
</issue_to_address>
### Comment 2
<location> `src/react-components/hooks/useLayer.ts:38` </location>
<code_context>
? Omit<Props, "root" | "camera" | "graph" | "emitter"> & { root?: Props["root"] }
: never
) {
- const layer = useMemo(() => (graph ? graph.addLayer(layerCtor, props) : null), [graph]);
- const prevProps = usePrevious(props);
+ const [layer, setLayer] = useState<InstanceType<T> | null>(null);
- useLayoutEffect(() => {
</code_context>
<issue_to_address>
Layer instance is now managed via useState and useEffect instead of useMemo.
Using useState/useEffect may recreate the layer more frequently, potentially causing unnecessary detach/attach cycles. Please confirm this is intentional and won't impact performance or cause flicker.
</issue_to_address>
### Comment 3
<location> `src/react-components/layer/ReactLayer.tsx:35` </location>
<code_context>
- * This is where we set up event subscriptions to ensure they work properly
- * after the layer is unmounted and reattached.
- */
- protected afterInit(): void {
- // Register event listener with the onGraphEvent method for automatic cleanup when unmounted
- this.onGraphEvent("camera-change", this.performRender);
</code_context>
<issue_to_address>
Consider removing the manual class-toggling methods and letting React handle className updates in the render method.
```suggestion
You can remove the entire imperative class‐toggling logic (`afterInit` / `propsChanged` / `applyBlockListClassName`) and instead let React manage your `blockListClassName`. For example, wrap your `<BlocksList>` in a `<div>` with a dynamic `className` in the portal:
```ts
// 1) Delete these methods entirely:
// protected afterInit() { … }
// protected propsChanged() { … }
// private applyBlockListClassName() { … }
// 2) Update renderPortal to delegate className to React:
public renderPortal(
renderBlock: <T extends TBlock>(graphObject: Graph, block: T) => React.JSX.Element
) {
const container = this.getHTML();
if (!container) return null;
return createPortal(
<div className={this.props.blockListClassName}>
<BlocksList
graph={this.context.graph}
renderBlock={renderBlock}
/* keep other props here */
/>
</div>,
container
);
}
```
This preserves all behavior—new `blockListClassName` will be applied/updated by React—while removing ~30 lines of manual DOM work.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const getTime = () => performance.now(); | ||
| type EmitterEventsDefinition = Record<string, (...args: unknown[]) => void>; | ||
| export class Emitter<T extends EmitterEventsDefinition = EmitterEventsDefinition> { | ||
| private destroyed: boolean; |
There was a problem hiding this comment.
question (bug_risk): The destroyed flag and related checks have been removed from Emitter.
This change allows methods like off, emit, and _walkGC to be called on a destroyed Emitter, which could cause unexpected behavior or memory leaks if handlers aren't cleaned up. Please confirm this is intentional and safe for all use cases.
| ? Omit<Props, "root" | "camera" | "graph" | "emitter"> & { root?: Props["root"] } | ||
| : never | ||
| ) { | ||
| const layer = useMemo(() => (graph ? graph.addLayer(layerCtor, props) : null), [graph]); |
There was a problem hiding this comment.
suggestion (performance): Layer instance is now managed via useState and useEffect instead of useMemo.
Using useState/useEffect may recreate the layer more frequently, potentially causing unnecessary detach/attach cycles. Please confirm this is intentional and won't impact performance or cause flicker.
| }); | ||
| } | ||
|
|
||
| protected afterInit(): void { |
There was a problem hiding this comment.
issue (complexity): Consider removing the manual class-toggling methods and letting React handle className updates in the render method.
Suggested change
| protected afterInit(): void { | |
| You can remove the entire imperative class‐toggling logic (`afterInit` / `propsChanged` / `applyBlockListClassName`) and instead let React manage your `blockListClassName`. For example, wrap your `<BlocksList>` in a `<div>` with a dynamic `className` in the portal: | |
| ```ts | |
| // 1) Delete these methods entirely: | |
| // protected afterInit() { … } | |
| // protected propsChanged() { … } | |
| // private applyBlockListClassName() { … } | |
| // 2) Update renderPortal to delegate className to React: | |
| public renderPortal( | |
| renderBlock: <T extends TBlock>(graphObject: Graph, block: T) => React.JSX.Element | |
| ) { | |
| const container = this.getHTML(); | |
| if (!container) return null; | |
| return createPortal( | |
| <div className={this.props.blockListClassName}> | |
| <BlocksList | |
| graph={this.context.graph} | |
| renderBlock={renderBlock} | |
| /* keep other props here */ | |
| /> | |
| </div>, | |
| container | |
| ); | |
| } |
This preserves all behavior—new blockListClassName will be applied/updated by React—while removing ~30 lines of manual DOM work.
draedful
force-pushed
the
fix-react-strict-mode
branch
2 times, most recently
from
42b5773 to
79d279b
Compare
…onsistency - Moved from useEffect to useLayoutEffect in useLayer for immediate updates. - Replaced window.devicePixelRatio with globalThis.devicePixelRatio for broader compatibility. - Enhanced updateSize method in Layer to ensure proper rendering. - Changed rootSize in LayersService to use signals for reactive updates. - Switched debounce to throttle in handleRootResize for improved performance.
draedful
force-pushed
the
fix-react-strict-mode
branch
from
79d279b to
6ce5804
Compare
This was referenced Jul 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve React strict mode compliance by cleaning up event listeners and subscriptions across layers and services, add customizable block list styling via new props, refactor event handling for clarity, and introduce a utility for parsing CSS class names with corresponding tests.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: