Skip to content

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Nov 3, 2023
Merged

chore(site): remove template version editor xservice #10490

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c290855
Refactor initial loading
BrunoQuaresma Oct 31, 2023
93df525
Remove non used events
BrunoQuaresma Oct 31, 2023
b3c712c
Move cancel build to create build service
BrunoQuaresma Oct 31, 2023
10d6b4e
Extract upload
BrunoQuaresma Oct 31, 2023
7eb5c91
Extract create version
BrunoQuaresma Nov 1, 2023
c024a1d
Extract logs
BrunoQuaresma Nov 1, 2023
e7713fe
Refactor version fetching and refetch
BrunoQuaresma Nov 1, 2023
f782c7d
Extract resources
BrunoQuaresma Nov 1, 2023
8bf23af
Extract missing variables
BrunoQuaresma Nov 1, 2023
5502077
Extract publish from the machine and remove it
BrunoQuaresma Nov 1, 2023
20d6100
Remove unused data module
BrunoQuaresma Nov 1, 2023
9d1ca61
Fix loading state and tabs code to make it clear
BrunoQuaresma Nov 1, 2023
c73f976
Fix disable preview state
BrunoQuaresma Nov 1, 2023
86735c9
Optimistic update logs
BrunoQuaresma Nov 1, 2023
86d7a6d
Encapsulate a few logics in hooks
BrunoQuaresma Nov 1, 2023
e748fe8
Extract start/end build logic
BrunoQuaresma Nov 1, 2023
71e8920
Fix tests
BrunoQuaresma Nov 2, 2023
8a7fae6
Remove unused comment
BrunoQuaresma Nov 2, 2023
2769cfe
Show initialize error on snackbar
BrunoQuaresma Nov 2, 2023
4893156
Update tarFile in a single state call
BrunoQuaresma Nov 2, 2023
32934ed
Simplify onDone call
BrunoQuaresma Nov 2, 2023
209e364
Make tar manipulation more clear
BrunoQuaresma Nov 2, 2023
66ea796
Improve publish promise construction
BrunoQuaresma Nov 2, 2023
353f325
Fix storybook
BrunoQuaresma Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Encapsulate a few logics in hooks
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Nov 1, 2023
commit 86d7a6d4f1502ac03077253065a99163ba553950
101 changes: 59 additions & 42 deletions site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
View file
Open in desktop
Copy link
Collaborator Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,7 @@ export const TemplateVersionEditorPage: FC = () => {
...templateVersionOptions,
keepPreviousData: true,
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly!

});
const fileQuery = useQuery({
...file(templateVersionQuery.data?.job.file_id ?? ""),
enabled: templateVersionQuery.isSuccess,
});
const [fileTree, setFileTree] = useState<FileTree>();
const uploadFileMutation = useMutation(uploadFile());
const currentTarFileRef = useRef<TarReader | null>(null);
const createTemplateVersionMutation = useMutation(
createTemplateVersion(orgId),
);
Expand All @@ -70,38 +64,12 @@ export const TemplateVersionEditorPage: FC = () => {
const { logs, setLogs } = useVersionLogs(templateVersionQuery.data, {
onDone: templateVersionQuery.refetch,
});

// Initialize file tree
useEffect(() => {
const initializeFileTree = async (file: ArrayBuffer) => {
const tarReader = new TarReader();
await tarReader.readFile(file);
currentTarFileRef.current = tarReader;
const fileTree = await createTemplateVersionFileTree(tarReader);
setFileTree(fileTree);
};

if (fileQuery.data) {
initializeFileTree(fileQuery.data).catch(() => {
console.error("Error on initializing the editor");
});
}
}, [fileQuery.data]);

// Handling missing variables
const missingVariablesQuery = useQuery({
...templateVersionVariables(templateVersionQuery.data?.id ?? ""),
enabled:
templateVersionQuery.data?.job.error_code ===
"REQUIRED_TEMPLATE_VARIABLES",
});
const [isMissingVariablesDialogOpen, setIsMissingVariablesDialogOpen] =
useState(false);
useEffect(() => {
if (missingVariablesQuery.data) {
setIsMissingVariablesDialogOpen(true);
}
}, [missingVariablesQuery.data]);
const { fileTree, tarFile } = useFileTree(templateVersionQuery.data);
const {
missingVariables,
setIsMissingVariablesDialogOpen,
isMissingVariablesDialogOpen,
} = useMissingVariables(templateVersionQuery.data);

// Handling publishing
const [isPublishingDialogOpen, setIsPublishingDialogOpen] = useState(false);
Expand All @@ -123,12 +91,12 @@ export const TemplateVersionEditorPage: FC = () => {
templateVersion={templateVersionQuery.data}
defaultFileTree={fileTree}
onPreview={async (newFileTree) => {
if (!currentTarFileRef.current) {
if (!tarFile) {
return;
}
setLogs([]);
const newVersionFile = await generateVersionFiles(
currentTarFileRef.current,
tarFile,
newFileTree,
);
const serverFile = await uploadFileMutation.mutateAsync(
Expand Down Expand Up @@ -191,7 +159,7 @@ export const TemplateVersionEditorPage: FC = () => {
resources={resourcesQuery.data}
buildLogs={logs}
isPromptingMissingVariables={isMissingVariablesDialogOpen}
missingVariables={missingVariablesQuery.data}
missingVariables={missingVariables}
onSubmitMissingVariableValues={async (values) => {
if (!uploadFileMutation.data) {
return;
Expand Down Expand Up @@ -221,11 +189,39 @@ export const TemplateVersionEditorPage: FC = () => {
);
};

const useFileTree = (templateVersion: TemplateVersion | undefined) => {
const tarFileRef = useRef<TarReader | null>(null);
Copy link
Member

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 with useRef 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 update tarFileRef when they want a re-render, but not realize why the screen isn't changing
  • There is the tarFileRef.current write before createTemplateVersionFileTree gets called, but it doesn't seem that delaying the state change until after initializeFileTree resolves would affect anything?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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!

const fileQuery = useQuery({
...file(templateVersion?.job.file_id ?? ""),
enabled: templateVersion !== undefined,
});
const [fileTree, setFileTree] = useState<FileTree>();
useEffect(() => {
const initializeFileTree = async (file: ArrayBuffer) => {
const tarReader = new TarReader();
await tarReader.readFile(file);
tarFileRef.current = tarReader;
const fileTree = await createTemplateVersionFileTree(tarReader);
setFileTree(fileTree);
};

if (fileQuery.data) {
initializeFileTree(fileQuery.data).catch(() => {
console.error("Error on initializing the editor");
});
}
}, [fileQuery.data]);

return {
fileTree,
tarFile: tarFileRef.current,
};
};

const useVersionLogs = (
templateVersion: TemplateVersion | undefined,
options: { onDone: () => Promise<unknown> },
) => {
// Watch version logs
const [logs, setLogs] = useState<ProvisionerJobLog[]>();
const templateVersionId = templateVersion?.id;
const refetchTemplateVersion = options.onDone;
Expand Down Expand Up @@ -263,6 +259,27 @@ const useVersionLogs = (
};
};

const useMissingVariables = (templateVersion: TemplateVersion | undefined) => {
const { data: missingVariables } = useQuery({
...templateVersionVariables(templateVersion?.id ?? ""),
enabled: templateVersion?.job.error_code === "REQUIRED_TEMPLATE_VARIABLES",
});
const [isMissingVariablesDialogOpen, setIsMissingVariablesDialogOpen] =
useState(false);

useEffect(() => {
if (missingVariables) {
setIsMissingVariablesDialogOpen(true);
}
}, [missingVariables]);

return {
missingVariables,
isMissingVariablesDialogOpen,
setIsMissingVariablesDialogOpen,
};
};

const generateVersionFiles = async (
tarReader: TarReader,
fileTree: FileTree,
Expand Down