-
Notifications
You must be signed in to change notification settings - Fork 0
RecipeMetadata and prefetching optimizations #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…hub.com/mesoscope/cellpack-client into feature/ssot-refactor
|
| {!recipeObj ? ( | ||
| loadingText | ||
| ) : ( | ||
| <Tabs defaultActiveKey="1" className="recipe-content"> | ||
| <Tabs.TabPane tab="Edit" key="1"> | ||
| <RecipeForm onStartPacking={handleStartPacking} /> | ||
| </Tabs.TabPane> | ||
| <Tabs.TabPane tab="Full Recipe" key="2"> | ||
| <JSONViewer title="Recipe" content={recipeObj} /> | ||
| </Tabs.TabPane> | ||
| </Tabs> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a good reason to prevent this panel from loading once the first recipe has been fetched, but I figured showing the loading marker was in line with our overall design pattern, and it should only be visible very briefly if the user selects a recipe right after the dropdown populates.
src/state/store.ts
Outdated
| const { inputOptions, loadRecipe } = get(); | ||
|
|
||
| const optionList = Object.values(inputOptions || {}); | ||
| if (optionList.length === 0) return; | ||
|
|
||
| const recipeIds = optionList | ||
| .map(o => o?.recipeId) | ||
| .filter(id => id && !get().recipes[id]); | ||
|
|
||
| // Make sure our default initial is in the options we queried | ||
| const initialIdToLoad = | ||
| recipeIds.includes(INITIAL_RECIPE_ID) ? INITIAL_RECIPE_ID : recipeIds[0]; | ||
|
|
||
| // Ensure the bootstrap recipe is loaded & selected | ||
| if (!get().recipes[initialIdToLoad]) { | ||
| await loadRecipe(initialIdToLoad); | ||
| } | ||
| get().selectRecipe(recipeToLoad); | ||
|
|
||
| // Load remaining recipes in the background (don’t block) | ||
| const remainingRecipesToLoad = recipeIds.filter( | ||
| id => id !== initialIdToLoad && !get().recipes[id] | ||
| ); | ||
| await Promise.all(remainingRecipesToLoad.map((id) => loadRecipe(id))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for churn here, but I do think this is better.
Hopefully reads well and it loads the initial recipe before moving on to the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the recipe data loading architecture by splitting RecipeManifest into two distinct types: RecipeMetadata (fast-loading fields available immediately from the collection query) and RecipeData (slower-loading fields like recipe content and editable fields). The refactoring optimizes page load performance by fetching metadata in bulk while deferring individual recipe details until needed.
Key changes:
- Split data types to differentiate between quickly-available metadata and slower recipe details
- Removed
isLoadingstate in favor of deriving loading status from data presence - Implemented throttling to prevent result viewer from loading before recipe panel
- Updated bootstrap logic to load initial recipe first, then remaining recipes in background
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/firebase.ts | Refactored data fetching functions to separate metadata batch loading from individual recipe data loading |
| src/types/index.ts | Split RecipeManifest into RecipeMetadata and reorganized RecipeData interface |
| src/state/store.ts | Removed isLoading state, updated selectors and loading logic to work with new data types |
| src/components/PackingInput/index.tsx | Replaced isLoading check with derived loading state based on data presence |
| src/components/Dropdown/index.tsx | Updated type reference from RecipeManifest to RecipeMetadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ascibisz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the distinction you came up with of RecipeMetadata and RecipeData. Everything looks good and works well for me!
src/components/Dropdown/index.tsx
Outdated
| placeholder: string; | ||
| defaultValue?: string; | ||
| options: Dictionary<RecipeManifest>; | ||
| options: Dictionary<RecipeMetadata>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally I still like the term manifest but I'm not super attached to it. it's what we use in other apps like CFE for this same type of data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sure I like manifest too, for some reason I thought you said at our 1:1 that you wanted it called metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I think I was just using that as a category term, sorry!
src/types/index.ts
Outdated
| recipeId: string; | ||
| configId: string; | ||
| displayName: string; | ||
| defaultRecipeData: ViewableRecipe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you're calling this whole object RecipeData, having defaultRecipeData inside it is a little unclear. I think I would just call this defaultRecipe. or originalRecipe (and then the edits are spicy 😅🍗)
…nto feature/ssot-refactor
…nto fix/prefetch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 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/utils/firebase.ts
Outdated
| */ | ||
| const getRecipeDataFromFirebase = async (recipeId: string, editableFieldIds: string[]): Promise<RecipeData> => { | ||
| const defaultRecipe = await getFirebaseRecipe(recipeId); | ||
| const editableFields = await getEditableFieldsList(editableFieldIds) || []; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The || [] fallback is unnecessary because getEditableFieldsList already returns undefined when the input array is empty, and the function signature is Promise<EditableField[] | undefined>. The || [] will not catch the undefined case. Instead, handle the undefined explicitly: const editableFields = await getEditableFieldsList(editableFieldIds); if (!editableFields) return ...; or change the return type expectation.
| const editableFields = await getEditableFieldsList(editableFieldIds) || []; | |
| const editableFields = await getEditableFieldsList(editableFieldIds); | |
| if (!editableFields) { | |
| return { | |
| recipeId, | |
| defaultRecipe, | |
| editableFields: [], | |
| edits: {} | |
| } | |
| } |
| const editedValue = lodashGet(rec.edits, path); | ||
| if (editedValue !== undefined) { | ||
| if (typeof editedValue === "string" || typeof editedValue === "number") { | ||
| return editedValue; | ||
| } | ||
| return undefined; | ||
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using lodashGet on rec.edits is incorrect because edits is a flat Record<string, string | number> where keys are paths, not a nested object. The path should be used directly as a key: const editedValue = rec.edits[path];
…nto fix/prefetch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Problem
Changes discussed with @meganrm for state management and some optimizations to prefetching of input options and recipe data.
Basically part 2 of #150 and should address the TODOs I added there.
Solution
Instead of
RecipeManifestorRecipeManifest/RecipeData===> we use:RecipeMetadataandRecipeDataRecipeMetadatais whatever we can get on our first trip to server when we callgetAllDocsFromCollection.That means no actual recipe and no unpacked
editableFields.Regardless how we would like to shape this data up front, it makes sense to differentiate these data since they load at very different rates.
Splitting the firebase utils into a batch meta data loader, and a per-ID utility that gets the details (it's slower), makes loading the page smoother.
We can also get rid of
isLoadingstate for now and derive it from the presence of either the input options, or a given recipe object.Default result URLs are in the metadata, but I think we should prevent loading them beacuse it looks weird to have the viewer populate before the recipe panel does, so I throttled them to go together.