-
Notifications
You must be signed in to change notification settings - Fork 887
chore(site): remove template version editor xservice #10490
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
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 recommend reviewing this file by looking into it without the diff.
Have 7/8 files done – just need to finish up |
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.
Overall, this looks really good. Had a couple questions/suggestions, but the only part I have potential concerns about is how useFileTree
sets up state
site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx
Outdated
Show resolved
Hide resolved
jest | ||
.spyOn(api, "createTemplateVersion") | ||
.mockResolvedValueOnce(MockTemplateVersion); | ||
.mockResolvedValue(newTemplateVersion); |
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.
Just double-checking for my own understanding: is there any real difference between the mock function returning the base MockTemplateVersion
value, versus it returning the updated version?
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.
Yes, because we are going to use this version to optimistically update the UI.
); | ||
const templateVersionQuery = useQuery({ | ||
...templateVersionOptions, | ||
keepPreviousData: true, |
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.
Just making sure: this is for making the UX nicer, right? It just makes sure that if the page needs to load in new version data, the UI doesn't unload while that's happening?
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.
Exactly!
site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
Outdated
Show resolved
Hide resolved
}} | ||
/> | ||
)} | ||
</> | ||
); | ||
}; | ||
|
||
const useFileTree = (templateVersion: TemplateVersion | undefined) => { | ||
const tarFileRef = useRef<TarReader | null>(null); |
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 don't know the full set of problems that this component is trying to solve, but my gut feeling is that it might make sense to combine tarFile
and fileTree
into one single useState
call. Something like this:
type TarInfo = {
reader: TarReader;
tree: FileTree;
};
// Type (TarInfo | undefined)
const [tarState, setTarState] = useState<TarInfo>();
const initializeFileTree = async (file: ArrayBuffer) => {
const tarReader = new TarReader();
await tarReader.readFile(file);
const fileTree = await createTemplateVersionFileTree(tarReader);
setTarState({ reader: tarReader, file: fileTree });
};
- Right now, it looks like they're always supposed to be used together. Combining them would make sure that it's impossible to have one without the other.
tarFile
is set up withuseRef
right now, even though the inner value is referenced directly in render logic. My worry is that if the logic gets more complicated later, someone might updatetarFileRef
when they want a re-render, but not realize why the screen isn't changing- There is the
tarFileRef.current
write beforecreateTemplateVersionFileTree
gets called, but it doesn't seem that delaying the state change until afterinitializeFileTree
resolves would affect anything?
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.
Awesome points.
1 - I think makes sense
2, 3 - This initialization should happen only once
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.
Moved it to a single state call!
site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
Outdated
Show resolved
Hide resolved
group: file.group, | ||
}); | ||
} else { | ||
tar.addFile(file.name, tarReader.getTextFile(file.name) as string, { |
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.
Should there be some error handling in the off chance tarReader.getTextFile
returns undefined
?
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.
For this specific use case, we can just ignore it for now.
site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
Outdated
Show resolved
Hide resolved
|
||
// Optimistically update the template version data job status to make the | ||
// build action feels faster | ||
const onBuildStart = () => { |
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.
Could some of this functionality be done via useMutation
's onMutate
and onSuccess
callbacks? onMutate
also lets you return out a cleanup function
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 don't think so because this is not related to a specific mutation.
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.
My biggest concern got addressed, so I'm going to go ahead and approve, just to make sure you're not blocked
Close #9942