Feature/show precomputed results tweaks #146

meganrm · 2025-11-06T01:24:20Z

Problem

I made some changes to how you were storing things in the store.

The main things I was trying to address is

don't store things in more than once place, this is so your state is a single source of truth that can't be out of sync with itself.
use selectors in combination so you only access the data in one way each time
Move more things into the store and out of App state

github-actions · 2025-11-06T01:24:39Z

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	20.38%	366 / 1795
🔵	Statements	20.38%	366 / 1795
🔵	Functions	41.37%	24 / 58
🔵	Branches	71.68%	81 / 113

File Coverage

File	Stmts	% Branch	% Funcs	% Lines	Uncovered Lines
Changed Files
src/App.tsx	0%	0%	0%	0%	1-7, 16-20, 22, 24-25, 27-35, 37, 39-41, 43-45, 47-53, 55-69, 71-115, 117-125, 127-162, 164-168, 170-174, 176-195, 197, 199
src/components/Dropdown/index.tsx	0%	0%	0%	0%	1-2, 12-17, 19-26, 28, 30
src/components/Viewer/index.tsx	0%	0%	0%	0%	1-3, 5-13, 15, 17
src/state/constants.ts	0%	100%	100%	0%	4-11
src/state/store.ts	0%	0%	0%	0%	1, 55, 57-64, 66-68, 70-78, 80-95, 97-98, 100-120, 122-123, 125-126, 128-130, 132-135, 137-139, 141-148, 150-157, 159-174, 176-178, 180-181, 183-190, 192-194, 196-199, 201-215, 217-229, 231-247, 250-254, 256-264, 268-277, 279-282, 284-287, 289-292, 294-297, 299-310, 313-329
src/types/index.ts	100%	100%	100%	100%
src/utils/firebase.ts	27.22%	62.5%	26.66%	27.22%	33-34, 39-40, 74-76, 79-89, 92-96, 99-107, 111-124, 127-132, 135-137, 140-164, 167-190, 204-205, 208-218, 220-229, 231, 233-237, 239-244

Generated in workflow #128

github-actions · 2025-11-06T01:24:54Z

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-11-11 21:49 UTC

Copilot

Pull Request Overview

This PR refactors the state management to ensure a single source of truth by consolidating data storage and introducing composed selectors. The main focus is on renaming PackingInputs to RecipeManifest, restructuring how packing results are stored, and preventing redundant state storage.

Key changes:

Renamed PackingInputs type to RecipeManifest and result_path field to defaultResultPath
Introduced PackingResults type and consolidated result-related state into a single object
Refactored useResultUrl selector to derive the result URL from either packing results or default paths

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/utils/firebase.ts	Updated type references from `PackingInputs` to `RecipeManifest` and changed field name to `defaultResultPath`
src/types/index.ts	Renamed `PackingInputs` to `RecipeManifest`, added new `PackingResults` type
src/state/store.ts	Replaced `setResultUrl` with `setPackingResults`, added compound selectors for deriving result URLs
src/components/Viewer/index.tsx	Removed props, now uses `useResultUrl` hook and constructs full URL internally
src/components/Dropdown/index.tsx	Updated type reference from `PackingInputs` to `RecipeManifest`
src/App.tsx	Updated to use `setPackingResults` instead of `setResultUrl`, constructs `PackingResults` objects

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/state/store.ts

src/App.tsx

…computed-results-tweaks

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/state/constants.ts

src/state/store.ts

ascibisz · 2025-11-07T21:20:59Z

src/state/store.ts

-    inputOptions: Record<string, PackingInputs>;
+    inputOptions: Record<string, RecipeManifest>;
    recipes: Record<string, RecipeData>;
+    packingResults: PackingResults;


I think ideally we want packingResults to be a Record<string, PackingResults>, with recipeId as the key, so that when a user navigates back to a recipe that they previously ran, the results of that last run are displayed rather than the pre-computed results

yeah I was thinking about that, maybe good to have in a separate PR

Copilot

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/state/store.ts

src/state/constants.ts

src/state/store.ts

src/App.tsx

Co-authored-by: Copilot <[email protected]>

…hub.com/AllenCell/cellpack-client into feature/show-precomputed-results-tweaks

…nto feature/show-precomputed-results-tweaks

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-10T18:23:45Z

src/App.tsx

+            setPackingResults({
+                jobId: id,
+                jobLogs: `Packing job failed: ${localJobStatus.error_message}`,
+                resultUrl: "",
+                runTime: range,
+                outputDir: "",
+            });


The jobLogs field is being set in the FAILED case but left empty in the DONE case (line 148). This inconsistency means successful jobs lose their logs. Consider preserving the existing jobLogs value or fetching/storing logs for successful jobs as well.

interim17 · 2025-11-10T19:11:44Z

@meganrm I'm going to review this now and FYI I made a new branch, pointing at this one to handle the single source of turth issue.

The old branch was too different from this, and from main, and was doing too much given how many active lines of development there are. It is here: #150 That is what I hope to merge into this.

I will delete: #134 once it isn't relevant for context.

interim17

Looking good.

I appreciate bringing in some of the changes from the original SSOT branch.

I think https://github.com/AllenCell/cellpack-client/pull/150/files will be a more targeted next PR and in subsequent PRs we can address things like

final shape of RecipeManifest/RecipeData
when to load recipes vs recipe metadata
testing

interim17 · 2025-11-10T19:30:37Z

src/state/store.ts

        },

        selectRecipe: async (recipeId) => {
+            get().setPackingResults({ ...EMPTY_PACKING_RESULTS });


Nit: part of why I froze this object was so avoid triggering re-renders if we use EMPTY_PACKING_RESULTS when that's already in state.

Won't be a big deal either way, but in a case like this the spread creates a new object, so subscribers will re-render, even if packing results were already empty before calling selectRecipe. At least I think so.

interim17 · 2025-11-10T19:31:32Z

src/types/index.ts

    recipe: string;
-    result_path?: string;
+    defaultResultPath?: string;
    editable_fields?: EditableField[];


We should use camel case here?

ascibisz and others added 5 commits November 5, 2025 15:38

use result_path from firebase to display result in viewer upon selection

bd571c5

working but many updates

fdba1c7

reduce rerenders

c05b6f9

remove log

efa879f

clean up selectors

cd11abb

meganrm requested review from ascibisz, Copilot and interim17 November 6, 2025 01:28

Copilot AI reviewed Nov 6, 2025

View reviewed changes

src/state/store.ts Show resolved Hide resolved

src/App.tsx Outdated Show resolved Hide resolved

src/App.tsx Outdated Show resolved Hide resolved

src/App.tsx Show resolved Hide resolved

meganrm and others added 7 commits November 5, 2025 17:33

use runtime from results

a9b3864

reduce redundant state

3aa354c

move more things to the store

dae5a6a

remove unnecessary additional field from store

2353947

Merge branch 'feature/show-precomputed-results' into feature/show-pre…

1a595f1

…computed-results-tweaks

remove merge resolve error

774e287

use empty result obj

dd22a51

meganrm requested a review from Copilot November 6, 2025 23:55

Copilot AI reviewed Nov 6, 2025

View reviewed changes

src/state/constants.ts Outdated Show resolved Hide resolved

src/state/store.ts Show resolved Hide resolved

meganrm added 2 commits November 6, 2025 15:56

fix included param

9517f3f

move hooks to top of function

893bb5a

ascibisz reviewed Nov 7, 2025

View reviewed changes

Base automatically changed from feature/show-precomputed-results to main November 7, 2025 21:23

meganrm requested a review from Copilot November 10, 2025 17:58

Copilot AI reviewed Nov 10, 2025

View reviewed changes

meganrm and others added 4 commits November 10, 2025 10:01

Update src/state/store.ts

3e50510

Co-authored-by: Copilot <[email protected]>

use empty packing result object

a157b63

Merge branch 'feature/show-precomputed-results-tweaks' of https://git…

369421c

…hub.com/AllenCell/cellpack-client into feature/show-precomputed-results-tweaks

use zero

72092e3

meganrm added 3 commits November 10, 2025 10:19

Merge branch 'main' of https://github.com/AllenCell/cellpack-client i…

29f22d1

…nto feature/show-precomputed-results-tweaks

remove dup typing

dc5b4b2

better error message

0b84b49

meganrm requested a review from Copilot November 10, 2025 18:23

Copilot AI reviewed Nov 10, 2025

View reviewed changes

interim17 approved these changes Nov 10, 2025

View reviewed changes

ascibisz approved these changes Nov 11, 2025

View reviewed changes

meganrm merged commit f092c36 into main Nov 11, 2025
2 checks passed

meganrm deleted the feature/show-precomputed-results-tweaks branch November 11, 2025 21:49

Feature/show precomputed results tweaks #146

Feature/show precomputed results tweaks #146

Uh oh!

Conversation

meganrm commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Uh oh!

github-actions bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

github-actions bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

ascibisz Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meganrm Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

meganrm Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

interim17 commented Nov 10, 2025

Uh oh!

interim17 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interim17 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

interim17 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

meganrm commented Nov 6, 2025 •

edited

Loading

github-actions bot commented Nov 6, 2025 •

edited

Loading

github-actions bot commented Nov 6, 2025 •

edited

Loading

ascibisz Nov 7, 2025 •

edited

Loading

interim17 left a comment •

edited

Loading