Disable activeFill

[AntonJames-Sistence]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A
#267

What is the new behavior?

Possible to add disableActiveFill to the theme

disabled.mov

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[netlify]

✅ Deploy Preview for reagraph ready!

Name	Link
🔨 Latest commit	a5773a4
🔍 Latest deploy log	https://app.netlify.com/projects/reagraph/deploys/68a76e61325698000871f355
😎 Deploy Preview	https://deploy-preview-373--reagraph.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[amcdnl]

@claude - Can you review?

[claude]

Claude finished @amcdnl's task —— View job

---

Code Review for PR #373

Todo List

• Read and analyze changed files
• Review implementation approach
• Check for bugs and edge cases
• Provide feedback

---

Summary

This PR adds a disableActiveFill feature that allows users to disable the active/hover color changes for nodes and edges. The implementation has been moved from the theme to the store based on previous review feedback. Overall, the implementation is solid and functional, though there are a few areas for improvement.

Review Feedback

✅ Strengths

1. Clean API: The disableActiveFill prop on GraphCanvas is intuitive and well-documented in src/GraphCanvas/GraphCanvas.tsx:82-84
2. Consistent application: The flag is consistently checked across all components (Node, Edge, Sphere, Arrow)
3. Store-based approach: Using the store makes the flag accessible throughout the component tree without prop drilling
4. Good test coverage: Added a story demonstrating the feature in stories/demos/Basic.story.tsx:297-308

🔍 Areas for Improvement

1. Prop initialization in store (src/store.ts:48-49, 72, 85, 101-102)

The disableActiveFill is properly initialized in the store, but there's no mechanism to update it when the prop changes on GraphCanvas. The theme has a useEffect to handle updates (src/GraphCanvas/GraphCanvas.tsx:227-229), but disableActiveFill doesn't.

Issue: If someone changes the disableActiveFill prop dynamically, it won't update in the store.

Suggestion: Add a useEffect similar to the theme update:

useEffect(() => {
  store.getState().setDisableActiveFill(disableActiveFill);
}, [disableActiveFill, store]);

2. Missing TypeScript type export (src/GraphCanvas/GraphCanvas.tsx:84)

The disableActiveFill prop is defined on GraphCanvasProps but the type information could be more discoverable for users.

Observation: This is consistent with other props, so not a blocker.

3. Consider naming consistency

The prop is called disableActiveFill which is accurate but verbose. Consider if disableActiveColors or disableHoverEffects might be more intuitive for users, though the current name is clear about what it does.

4. Edge label conditionals (src/symbols/Node.tsx:342-345, 357-361)

The Node component has two separate checks for disableActiveFill in the label section:

• Line 343: combinedActiveState && !disableActiveFill
• Line 358: combinedActiveState && !disableActiveFill

These could be extracted into a helper variable at the start of labelComponent memo to improve readability:

const shouldApplyActiveFill = combinedActiveState && !disableActiveFill;

5. Documentation

Consider adding JSDoc comments to the store properties (src/store.ts:48-49) to explain what disableActiveFill does:

/**
 * When true, prevents nodes/edges from changing color on hover/selection
 */
disableActiveFill: boolean;

6. Default value consistency

The default is false everywhere which is correct (preserves existing behavior), but worth calling out in the PR description that this is non-breaking.

Minor Observations

1. Code consolidation in Node.tsx: As mentioned in a previous review comment, the color logic (lines 245-249) could benefit from consolidation:

const shouldApplyActiveFill = combinedActiveState && !disableActiveFill;
const color = shouldApplyActiveFill
  ? theme.node.activeFill
  : node.fill || theme.node.fill;

2. Package-lock.json changes: The package-lock.json shows version updates (+2/-2) which seem unrelated to this PR. These might be from dependency updates during development.

Testing Recommendations

1. Test dynamic prop changes (changing disableActiveFill from true to false and vice versa)
2. Verify behavior with selections, hovers, and active states
3. Test with both nodes and edges simultaneously
4. Verify the Ring component behavior on selected nodes (src/symbols/nodes/Sphere.tsx:51-60)

Conclusion

This is a well-implemented feature that follows the existing patterns in the codebase. The main issue is the missing useEffect to handle dynamic prop updates. The code quality is good and the feature works as intended.

Recommendation: Request changes to add the useEffect for dynamic updates, then approve.

---

Job Run | Branch: anton/disable-active-fill

[amcdnl]

@AntonJames-Sistence - Wouldn't you just want to make the color the same for active and it would solve this? Just curious, I can't remember the exact scenario for this. Im going to close this for now until we have clearer requirements on this.