Fixed Graph staying on page after removing from encoding shelf

[mawerb]

Hey there! I love this whole concept for the project and had tons of fun exploring the codebase.

BUG
whilst digging through I noticed that removing fields from the encodings resulted in a bug that kept the graph on page. Looking deeper I noticed that the graph was attached through a React Reference called "containerRef" and found that the graph embedded into the reference wasn't being cleared with the useEffect hook because the div was being unmounted by ternary operators in the return statement causing

if (!containerRef.current) return;

to run prematurely before the code to clear the graph from the DOM is ran:

if (!spec) { containerRef.current.innerHTML = ''; return; }

PROPOSED SOLUTION
Allowing the div that the graph embeds into to always be rendered and have it being hidden handled directly in its styling instead of unmounting whenever spec is null.

This was a lot of fun writing and debugging, hope this was as fun reading!

I am open to any edits/suggestions for this change, lmk :)

[zero2sudo]

Hi @mawerb, welcome to open source! :)

Congratulations on your first PR! Opening your first PR is a real milestone. Let me walk through what you found, because it's genuinely a tricky bug that trips up even experienced React developers.

---

The Bug You Caught

You identified a React ref lifecycle issue. Here's what was happening:

User removes field → spec becomes null → React unmounts the chart div → ref becomes null → cleanup code can't find the div → ghost chart remains

Think of it like this: imagine you asked someone to clean a room, but before they got there, the room was demolished. They show up, there's no room, so they leave. The mess is gone... but only because the whole room is gone, not because it was cleaned properly.

In React terms: the useEffect cleanup tried to clear containerRef.current.innerHTML, but the div had already been removed from the DOM by the conditional rendering. The ref was pointing at nothing.

You correctly identified that the fix is to keep the container in the DOM and hide it with CSS instead.

---

Your Solution: What's Working

display: spec ? 'flex' : 'none'

This keeps the "room" around so the cleanup crew can do their job. The element stays in the DOM, the ref stays valid, and innerHTML = '' actually clears the Vega chart. Solid fix.

---

Suggestions for Next Time

A few small things that'll make future PRs even smoother:

1. Keep the diff minimal

Your core fix is ~6 lines. But the PR also changes the useEffect dependencies and refactors some code in AppContext. When reviewing PRs, smaller = easier to verify = faster merge.

Ask yourself: "Does this line change fix the bug, or is it something else?" If it's something else, consider a separate PR.

2. The extra dependencies might cause issues

// You added: }, [spec, state.data, state.encodings]);

Since spec is already derived from state.data and state.encodings, adding them is redundant—and might cause the chart to re-render twice. When in doubt, keep dependencies minimal.

3. Unused variables

const {[action.channel]: removed, ...newEncodings} = state.encodings;

This is clever destructuring! But removed isn't used, which may trigger linter warnings. Convention is to prefix with underscore: _removed — tells other devs "I know this is unused, it's intentional."

---

The Takeaway

You found a real bug, understood the root cause (not just the symptom), and fixed it correctly. That debugging instinct (asking "why is this happening?" instead of just "how do I make it stop?") is what separates good engineers from great ones.

Looking forward to your next PR!

---

P.S. For anyone else reading this: this project is a great place to learn React + TypeScript + data visualization. The codebase is small, the bugs are real, and the learning is hands-on. Jump in at this Notion link: ntn.so/sweinternjournal

[zero2sudo]

TL;DR for future readers:

IMO, the smallest fix addresses just the ref lifecycle issue—nothing else. Something like

--- a/src/components/ChartView/ChartView.tsx
+++ b/src/components/ChartView/ChartView.tsx
@@ -342,8 +342,9 @@ export function ChartView() {
             <span style={{ color: 'var(--color-accent)' }}>Y</span> axis to render your chart
           </p>
         </div>
-      </div>
-      ) : (
+      </div>
+      ) : null}
+
       <div
         ref={containerRef}
         style={{
@@ -355,12 +356,12 @@ export function ChartView() {
           position: 'relative',
           zIndex: 1,
-          display: 'flex',
+          display: spec ? 'flex' : 'none',
           alignItems: 'center',
           justifyContent: 'center',
           animation: 'fadeIn 0.4s ease-out',
           overflow: 'hidden',
         }}
       />
-      )}

       {/* Footer gradient line */}

[zero2sudo]

@mawerb I created a branch called main-mawerb that you can now set as the base branch so you're able to make changes and treat as your main branch without messing with other people's main branches!