Skip to content

1033 remove dead code echarts#1062

Merged
johbaxter merged 4 commits intodevfrom
1033-remove-dead-code-echarts
May 8, 2025
Merged

1033 remove dead code echarts#1062
johbaxter merged 4 commits intodevfrom
1033-remove-dead-code-echarts

Conversation

@johbaxter
Copy link
Copy Markdown
Contributor

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

@johbaxter johbaxter requested a review from a team as a code owner May 8, 2025 14:20
@johbaxter johbaxter linked an issue May 8, 2025 that may be closed by this pull request
2 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2025

@CodiumAI-Agent /describe

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2025

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link
Copy Markdown

Title

1033 remove dead code echarts


User description

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Enhancement


Description

  • Add listeners.preProcess in block definitions.

  • Trigger preProcess listener on mount.

  • Render preProcess via accordion in FrameOperations.

  • Remove obsolete chart constants and utilities.


Changes walkthrough 📝

Relevant files
Enhancement
8 files
VisualizationBlock.tsx
Support `preProcess` listener in block component                 
+9/-2     
DataTabStyling.tsx
Add material icons and switch imports                                       
+5/-5     
FrameOperations.tsx
Introduce accordion for `preProcess` listeners                     
+75/-4   
LabelsDendrogram.tsx
Add `ChangeEvent` import support                                                 
+1/-2     
LegendDendrogram.tsx
Add `ChangeEvent` to observer component                                   
+2/-5     
Map.tsx
Rework imports and component structure                                     
+4/-4     
MapChartBlockSettings.tsx
Introduce `Sync` and `Stack` UI imports                                   
+4/-4     
MapMarkerSize.tsx
Add `observer` and `computed` imports                                       
+8/-7     
Configuration changes
1 files
config.tsx
Configure default `preProcess` listener property                 
+3/-1     
Formatting
9 files
CustomizeSymbol.tsx
Reorder and streamlines import statements                               
+3/-3     
Gantt.tsx
Clean up unused imports in Gantt component                             
+3/-3     
GanttDisplayValueLabels.tsx
Update import path for `EchartVisualizationBlockDef`         
+1/-1     
GanttToolsSection.tsx
Adjust hooks and import ordering                                                 
+2/-2     
Edit-X-Axis.tsx
Move `Style` import to top                                                             
+1/-1     
Dendrogram.tsx
Reorder imports and add type definitions                                 
+1/-1     
DendrogramFrameOperation.tsx
Relocate `useNotification` import                                               
+1/-1     
LegendToggleMapChart.tsx
Consolidate import ordering and hooks                                       
+4/-4     
MapTool.tsx
Reorder UI import statements                                                         
+2/-2     
Cleanup
2 files
ChartStyling.tsx
Remove unused chart state variable                                             
+0/-3     
DendrogramChartField.tsx
Bring `computed` import to top                                                     
+4/-4     
Additional files
9 files
TooltipMapChart.tsx +1/-1     
chart-utility.ts +0/-15   
Visualization.constants.ts +0/-61   
VisualizationBlock.tsx +0/-148 
VisualizationBlockMenu.tsx +0/-113 
VizBlockContextMenu.tsx +0/-94   
config.tsx +0/-40   
index.ts +0/-2     
default-menu.ts +24/-8   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Listeners Type Mismatch

    The listeners.preProcess property is defined as a boolean in the interface, initialized as an array in the config, and then invoked as a function in useEffect. Ensure the type and default value of preProcess align with its intended use.

        listeners: {
            preProcess: true;
        };
        slots: never;
    }
    
    export const VisualizationBlock: BlockComponent = observer(
        <D extends BlockDef = BlockDef>({ id }) => {
            const { data, attrs, listeners } = useBlock<EchartVisualizationBlockDef>(id);
            const { setData } = useBlockSettings<EchartVisualizationBlockDef>(id);
            const { state } = useBlocks();
    
            const elementRef = useRef<HTMLDivElement>(null);
            const timeoutRef = useRef<ReturnType<typeof setTimeout>>(null);
    
            useEffect(() => {
                if (listeners.preProcess) {
                    listeners.preProcess();
                }
            }, []);
    Missing Effect Dependencies

    The useEffect that calls listeners.preProcess() uses an empty dependency array, so changes to listeners won’t retrigger it. Add appropriate dependencies to ensure the effect runs when needed.

    useEffect(() => {
        if (listeners.preProcess) {
            listeners.preProcess();
        }
    }, []);

    State Mutation
    The code directly mutates the accordionSection state before calling setAccordionSection, which can lead to stale or unexpected UI updates. Use immutable patterns (e.g., cloning) when updating state.

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add effect dependency for listener

    Include listeners.preProcess in the dependency array and guard its type before
    calling to prevent stale closures and runtime errors.

    libs/renderer/src/components/block-defaults/echart-visualization-block/VisualizationBlock.tsx [84-95]

     const { data, attrs, listeners } = useBlock<EchartVisualizationBlockDef>(id);
     
     useEffect(() => {
    -    if (listeners.preProcess) {
    +    if (typeof listeners.preProcess === 'function') {
             listeners.preProcess();
         }
    -}, []);
    +}, [listeners.preProcess]);
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring the useEffect depends on listeners.preProcess avoids stale closures, and guarding its type prevents runtime errors.

    Medium
    Prevent direct state mutation

    Avoid mutating accordionSection directly. Use a functional state update that maps
    and returns a new array and object.

    libs/renderer/src/components/block-defaults/echart-visualization-block/variant/bar-chart/FrameOperations.tsx [947-957]

    -onChange={(e) =>{
    -    let accordionSectionToUp = accordionSection;
    -    let indexToUpdate = accordionSectionToUp.findIndex((accordItem)=>accordItem.hasOwnProperty(accordionList[index]));
    -    accordionSectionToUp[indexToUpdate][accordionList[index]] = {
    -        ...accordionSectionToUp[indexToUpdate][accordionList[index]],
    -        expanded: !accordionSectionToUp[indexToUpdate][accordionList[index]].expanded,
    -    };
    -    setAccordionSection((prevAccordionSection)=>{
    -        return [...accordionSectionToUp];
    -    })
    +onChange={(_, expanded) => {
    +    setAccordionSection(prev =>
    +        prev.map((section, idx) => {
    +            const key = accordionList[idx];
    +            if (idx === index) {
    +                return {
    +                    [key]: {
    +                        ...section[key],
    +                        expanded,
    +                    },
    +                };
    +            }
    +            return section;
    +        })
    +    );
     }}
    Suggestion importance[1-10]: 7

    __

    Why: The current onChange mutates accordionSection directly; using a functional update preserves immutability and ensures reliable React state updates.

    Medium
    General
    Add missing React key

    Add a unique key prop to each <Accordion> in the .map to avoid React list key warnings.

    libs/renderer/src/components/block-defaults/echart-visualization-block/variant/bar-chart/FrameOperations.tsx [944-949]

     {
         accordionSection.map((item,index)=>(
             <Accordion
    +            key={accordionList[index]}
                 expanded={item[accordionList[index]].expanded}
                 onChange={(e) => { /* ... */ }}
                 sx={{ width:'100%' }}
             >
             …
             </Accordion>
         ))
     }
    Suggestion importance[1-10]: 5

    __

    Why: Adding a unique key prop to each <Accordion> avoids React list key warnings without altering logic.

    Low

    @johbaxter johbaxter merged commit 9259f2a into dev May 8, 2025
    3 checks passed
    @johbaxter johbaxter deleted the 1033-remove-dead-code-echarts branch May 8, 2025 15:25
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /update_changelog

    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.

    Remove dead code Echarts

    3 participants