[WIP] Fix dependency issues in effect-selection #93

aganders3 · 2024-06-01T01:54:34Z

This is some code for discussion on dealing with selection in the effects.

Some ideas here may be useful, but I don't love passing trackmanager to the reducer. I'll probably take another crack at this.

vercel · 2024-06-01T01:54:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
points-web-viewer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 3, 2024 8:38pm

aganders3 · 2024-06-01T01:59:14Z

src/components/App.tsx

+                // pass in the dispatcher to trigger a refresh, is this a hack?
+                dispatcher: dispatchCanvas,


This is pretty weird. What this is doing is passing in the dispatcher, so it can be called from the dispatcher (yo, dog).

This is because the dispatch (here) is instantaneous, but the code in the reducer is async/await. We want to refresh when tracks are actually fetched so the cell counter goes up.

We don't need a separate effect for that because the cell counter uses canvas.tracks.size in props, so if it changes it will trigger a re-render there.

This issue is gone now, right?

Yeah I took this out because the loading updates cause enough rerendering to get the counter updated.

aganders3 · 2024-06-01T02:00:20Z

src/hooks/usePointCanvas.ts

@@ -154,6 +192,9 @@ function reducer(canvas: PointCanvas, action: PointCanvasAction): PointCanvas {
            newCanvas.pointBrightness = 1.0;
            newCanvas.resetPointColors();
            break;
+        case ActionType.SELECTION:
+            newCanvas.selector.selection = action.selection;


This might need a new Map(action.selection) sometimes? Otherwise we just need to be careful (that's a big "just") not to merely modify the selection and then dispatch this.

src/components/App.tsx

aganders3 · 2024-06-01T02:05:56Z

This still has issues with curTime mismatch fetching the wrong tracks 😞. Maybe the way to solve this is for the selection to hold the actual point IDs? Then the curTime is really captured at selection time. I can try to add that here.

… more semantic

aganders3 · 2024-06-03T14:36:05Z

This still has issues with curTime mismatch fetching the wrong tracks 😞. Maybe the way to solve this is for the selection to hold the actual point IDs? Then the curTime is really captured at selection time. I can try to add that here.

I thought 1e5e457 would fix this, but it doesn't quite. I can't cause an issue anymore by selecting a bunch of points and moving the slider, but it still causes issues if you select while playing.

aganders3 · 2024-06-03T14:37:22Z

src/lib/PointSelector.ts

 import { BoxPointSelector } from "./BoxPointSelector";
 import { SpherePointSelector } from "./SpherePointSelector";

+export type PointSelection = Set<number>;


This is bad naming - now there is PointSelection and PointsCollection. That can definitely be cleaned up.

This code is mostly vendored, so I think it's fine. Plus we might even consider getting rid of this code given that it's fairly complicated for something that is probably not too useful.

aganders3 · 2024-06-03T14:38:51Z

src/hooks/usePointCanvas.ts

+            const newSelection = new Set([...selection].map((p) => canvas.curTime * canvas.maxPointsPerTimepoint + p));
+            dispatchCanvas({ type: ActionType.SELECTION, selection: newSelection });


This changes the IDs of the points from indices within the timeframe to the calculated pointID. As noted there are still possible mismatch problems, so maybe we need to do this conversion even earlier?

I'm thinking about this issue now. What are the remaining possible mismatch problems? This will be executed immediately after the selection is made, so curTime and maxPointsPerTimepoint will not change. Are their values assumed not to change in a later side-effect related to this?

I think this may be sufficient. I need to re-test systematically, my above comment may be inaccurate.

andy-sweet · 2024-06-03T21:30:24Z

src/lib/PointCanvas.ts

@@ -38,6 +37,8 @@ export class PointCanvas {
    readonly selector: PointSelector;

    readonly tracks: Tracks = new Map();
+    // set of track IDs that have had their lineage fetched
+    readonly rootTracks: Set<number> = new Set();


This would also be useful over on #91: https://github.com/aganders3/points-web-viewer/pull/91/files#diff-86963609bec6fda3d34233676d5ae33f6aa4b9bbe2758a10cd4863b704b37093R219

andy-sweet · 2024-06-03T21:58:02Z

src/hooks/usePointCanvas.ts

@@ -168,6 +189,30 @@ function reducer(canvas: PointCanvas, action: PointCanvasAction): PointCanvas {
        case ActionType.SIZE:
            newCanvas.setSize(action.width, action.height);
            break;
+        case ActionType.SYNC_TRACKS: {
+            const { trackManager, pointId, adding, setNumLoadingTracks } = action;
+            const fetchAndAddTrack = async (p: number) => {


I can't find much official documentation on whether calling an async function in a react reducer is recommended. I found some posts on SO/Medium, but nothing too authoritative.

Given that reducers should be pure, my guess is that it's probably not recommended.

I guess the reduction of all dispatched actions will be the same even with async because those will not have started yet. But the instance will be modified in the future when the async functions run. And each dispatch will mutate different instances, which makes this feel fragile.

Yeah, you're definitely right. Fragile is a kind word for it.

I'm trying to think of how we can do this without having an effect depend on canvas. One idea is to split the canvas back into state + ref, but that's another pretty big change at this point.

Another idea is maybe we do want an effect that runs whenever canvas changes to sync the state. Maybe that's fine as long as it's somewhat smart about skipping unnecessary work?

aganders3 · 2024-06-06T15:21:05Z

Closing as #95 covers the desired behavior changes, and I think this is the wrong direction for refactoring.

Closes #49 Closes #62 (by making it no longer relevant) This stores selection state in the URL hash. In order to make this work, we need some state that reflects the selected points/cells/tracks independent of `PointCanvas.curTime` because the ordering of the React effects when re-hydrating from the entire state can be different compared to when the user is interacting with the application. Previously, the canvas stored the last selected point indices, which is transient state that depends on `PointCanvas.curTime`. Here we stored the all the point IDs that were selected by the user (inspired from #93). Alternatively, we considered storing the track IDs that were selected in #91 , but that doesn't work as well with the rest of the application logic. This PR also updates the URL hash to store some simpler state that not been added like point brightness. It also refactors that code a little to make it easier to keep new state in sync. Though there is still a lot of duplication that could be improved. It does not fix #30 , which is related, but can be solved independently.

Make hook dependencies errors

f8b61f1

Move fetching logic into dispatcher, dispatch to set selection

1c9431e

vercel bot deployed to Preview June 1, 2024 01:56 View deployment

aganders3 commented Jun 1, 2024

View reviewed changes

src/components/App.tsx Outdated Show resolved Hide resolved

Use newCanvas in reducer - not sure this makes a difference but feels…

ee85b24

… more semantic

vercel bot deployed to Preview June 1, 2024 02:09 View deployment

Trying to make track adding idempotent

1e5e457

vercel bot deployed to Preview June 3, 2024 14:32 View deployment

aganders3 commented Jun 3, 2024

View reviewed changes

Fix highlighting points on selection

f1aab72

vercel bot deployed to Preview June 3, 2024 20:38 View deployment

andy-sweet reviewed Jun 3, 2024

View reviewed changes

andy-sweet mentioned this pull request Jun 5, 2024

Store selection state in URL hash #95

Merged

aganders3 closed this Jun 6, 2024

aganders3 deleted the effect-selectedpoints-hook-deps branch June 6, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix dependency issues in effect-selection #93

[WIP] Fix dependency issues in effect-selection #93

aganders3 commented Jun 1, 2024 •

edited

Loading

vercel bot commented Jun 1, 2024 •

edited

Loading

aganders3 Jun 1, 2024

andy-sweet Jun 3, 2024

aganders3 Jun 3, 2024

aganders3 Jun 1, 2024 •

edited

Loading

aganders3 commented Jun 1, 2024

aganders3 commented Jun 3, 2024 •

edited

Loading

aganders3 Jun 3, 2024

andy-sweet Jun 3, 2024

aganders3 Jun 3, 2024

andy-sweet Jun 4, 2024

aganders3 Jun 4, 2024

andy-sweet Jun 3, 2024 •

edited

Loading

andy-sweet Jun 3, 2024 •

edited

Loading

aganders3 Jun 3, 2024

aganders3 commented Jun 6, 2024

		// pass in the dispatcher to trigger a refresh, is this a hack?
		dispatcher: dispatchCanvas,

		const newSelection = new Set([...selection].map((p) => canvas.curTime * canvas.maxPointsPerTimepoint + p));
		dispatchCanvas({ type: ActionType.SELECTION, selection: newSelection });

[WIP] Fix dependency issues in effect-selection #93

[WIP] Fix dependency issues in effect-selection #93

Conversation

aganders3 commented Jun 1, 2024 • edited Loading

vercel bot commented Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

aganders3 commented Jun 1, 2024

aganders3 commented Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

andy-sweet Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 commented Jun 6, 2024

aganders3 commented Jun 1, 2024 •

edited

Loading

vercel bot commented Jun 1, 2024 •

edited

Loading

aganders3 Jun 1, 2024 •

edited

Loading

aganders3 commented Jun 3, 2024 •

edited

Loading

andy-sweet Jun 3, 2024 •

edited

Loading

andy-sweet Jun 3, 2024 •

edited

Loading