Clean up duplicated code used for recursively flattening graphic types#3836
Clean up duplicated code used for recursively flattening graphic types#3836
Conversation
Summary of ChangesHello @Keavon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the core logic for flattening graphic tables by introducing a generic approach. Previously, separate functions existed for flattening different graphic types (vector, raster, color, gradient), leading to significant code duplication. The changes consolidate this logic into a single, reusable function and a new trait, making the codebase more maintainable and extensible for future graphic types. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
80584c8 to
f1d87e0
Compare
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that significantly cleans up the code for recursively flattening graphic types. By introducing a generic flatten_graphic_table function and a TryFromGraphic trait, you've eliminated a large amount of duplicated code. This makes the implementation much cleaner, more maintainable, and easier to extend with new graphic types in the future. The changes in flatten_path are particularly nice, simplifying the logic considerably. I have one suggestion to further improve performance by avoiding an unnecessary clone.
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/graphic-types/src/graphic.rs">
<violation number="1" location="node-graph/libraries/graphic-types/src/graphic.rs:108">
P2: Doc comment says "Recursion through `Graphic::Graphic` sub-tables composes transforms and opacity" but the `Graphic::Graphic` recursive branch only composes transforms — opacity from intermediate `Graphic::Graphic` layers is silently dropped. The opacity composition only happens at the leaf level, meaning deeply nested `Graphic::Graphic` wrappers lose intermediate opacity. Consider either fixing the recursion to also compose opacity (by mutating `graphic.alpha_blending.opacity` alongside `graphic.transform` in the loop), or updating the doc comment to accurately reflect current behavior.
(Based on your team's feedback about aligning behavior with comments and TODO expectations.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
No description provided.