Skip to content
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

Refactor common code to use full paths #4393

Merged
merged 3 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions web-common/src/features/charts/ChartMenuItems.svelte
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
<script lang="ts">
import { getFileAPIPathFromNameAndType } from "@rilldata/web-common/features/entity-management/entity-mappers";
import NavigationMenuItem from "@rilldata/web-common/layout/navigation/NavigationMenuItem.svelte";
import { runtime } from "../../runtime-client/runtime-store";
import { deleteFileArtifact } from "../entity-management/actions";
import { EntityType } from "../entity-management/types";
import { useChartFileNames } from "./selectors";
import { useChartRoutes } from "./selectors";

export let chartName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than chartName, this should now be chartPath. Then we can use the path directly in the deleteFileArtifact function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. This PR makes an effort to not touch the components to update such params.


$: chartFileNames = useChartFileNames($runtime.instanceId);
$: chartRoutes = useChartRoutes($runtime.instanceId);

async function handleDeleteChart() {
await deleteFileArtifact(
$runtime.instanceId,
chartName,
getFileAPIPathFromNameAndType(chartName, EntityType.Chart),
EntityType.Chart,
$chartFileNames.data ?? [],
$chartRoutes.data ?? [],
);
}
</script>
Expand Down
8 changes: 8 additions & 0 deletions web-common/src/features/charts/selectors.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import { getRouteFromName } from "@rilldata/web-common/features/entity-management/entity-mappers";
import {
ResourceKind,
useResource,
} from "@rilldata/web-common/features/entity-management/resource-selectors";
import { EntityType } from "@rilldata/web-common/features/entity-management/types";
import { useMainEntityFiles } from "../entity-management/file-selectors";

export function useChartFileNames(instanceId: string) {
return useMainEntityFiles(instanceId, "charts");
}

export function useChartRoutes(instanceId: string) {
return useMainEntityFiles(instanceId, "charts", (name) =>
getRouteFromName(name, EntityType.Chart),
);
}

export const useChart = (instanceId: string, chartName: string) => {
return useResource(instanceId, chartName, ResourceKind.Chart);
};
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
<script lang="ts">
import { getFileAPIPathFromNameAndType } from "@rilldata/web-common/features/entity-management/entity-mappers";
import { runtime } from "../../runtime-client/runtime-store";
import { deleteFileArtifact } from "../entity-management/actions";
import { EntityType } from "../entity-management/types";
import { useCustomDashboardFileNames } from "./selectors";
import { useCustomDashboardRoutes } from "./selectors";
import * as DropdownMenu from "@rilldata/web-common/components/dropdown-menu/";

export let customDashboardName: string;

$: customDashboardFileNames = useCustomDashboardFileNames(
$runtime.instanceId,
);
$: customDashboardRoutes = useCustomDashboardRoutes($runtime.instanceId);

async function handleDeleteCustomDashboard() {
await deleteFileArtifact(
$runtime.instanceId,
customDashboardName,
getFileAPIPathFromNameAndType(customDashboardName, EntityType.Dashboard),
EntityType.Dashboard,
$customDashboardFileNames.data ?? [],
$customDashboardRoutes.data ?? [],
);
}
</script>
Expand Down
8 changes: 8 additions & 0 deletions web-common/src/features/custom-dashboards/selectors.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { getRouteFromName } from "@rilldata/web-common/features/entity-management/entity-mappers";
import { EntityType } from "@rilldata/web-common/features/entity-management/types";
import { useMainEntityFiles } from "../entity-management/file-selectors";

export function useCustomDashboardFileNames(instanceId: string) {
return useMainEntityFiles(instanceId, "custom-dashboards");
}

export function useCustomDashboardRoutes(instanceId: string) {
return useMainEntityFiles(instanceId, "custom-dashboards", (name) =>
getRouteFromName(name, EntityType.Dashboard),
);
}
9 changes: 7 additions & 2 deletions web-common/src/features/dashboards/DashboardAssets.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import Model from "@rilldata/web-common/components/icons/Model.svelte";
import {
useDashboardFileNames,
useDashboardRoutes,
useValidDashboards,
} from "@rilldata/web-common/features/dashboards/selectors";
import { deleteFileArtifact } from "@rilldata/web-common/features/entity-management/actions";
Expand Down Expand Up @@ -53,6 +54,7 @@
$: sourceNames = useSourceFileNames(instanceId);
$: modelNames = useModelFileNames(instanceId);
$: dashboardNames = useDashboardFileNames(instanceId);
$: dashboardRoutes = useDashboardRoutes(instanceId);
$: dashboards = useValidDashboards(instanceId);

const MetricsSourceSelectionError = (
Expand Down Expand Up @@ -168,9 +170,12 @@
);
await deleteFileArtifact(
instanceId,
dashboardName,
getFileAPIPathFromNameAndType(
dashboardName,
EntityType.MetricsDefinition,
),
EntityType.MetricsDefinition,
$dashboardNames?.data ?? [],
$dashboardRoutes?.data ?? [],
);

// redirect to model when metric is deleted
Expand Down
8 changes: 8 additions & 0 deletions web-common/src/features/dashboards/selectors.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { filterExpressions } from "@rilldata/web-common/features/dashboards/stores/filter-utils";
import { getRouteFromName } from "@rilldata/web-common/features/entity-management/entity-mappers";
import { useMainEntityFiles } from "@rilldata/web-common/features/entity-management/file-selectors";
import {
ResourceKind,
useFilteredResourceNames,
useFilteredResources,
useResource,
} from "@rilldata/web-common/features/entity-management/resource-selectors";
import { EntityType } from "@rilldata/web-common/features/entity-management/types";
import {
V1Expression,
V1MetricsViewSpec,
Expand All @@ -26,6 +28,12 @@ export function useDashboardFileNames(instanceId: string) {
return useMainEntityFiles(instanceId, "dashboards");
}

export function useDashboardRoutes(instanceId: string) {
return useMainEntityFiles(instanceId, "dashboards", (name) =>
getRouteFromName(name, EntityType.MetricsDefinition),
);
}

export function useDashboard(instanceId: string, metricViewName: string) {
return useResource(instanceId, metricViewName, ResourceKind.MetricsView);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
import * as yup from "yup";
import { runtime } from "../../runtime-client/runtime-store";
import { renameFileArtifact } from "./actions";
import { getLabel, getRouteFromName } from "./entity-mappers";
import {
getFileAPIPathFromNameAndType,
getLabel,
getRouteFromName,
} from "./entity-mappers";
import {
INVALID_NAME_MESSAGE,
VALID_NAME_PATTERN,
Expand Down Expand Up @@ -50,8 +54,8 @@
try {
await renameFileArtifact(
runtimeInstanceId,
currentAssetName,
values.newName,
getFileAPIPathFromNameAndType(currentAssetName, entityType),
getFileAPIPathFromNameAndType(values.newName, entityType),
Comment on lines +57 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

When we migrate, the modal should take a fileName as prop, not an assetName

entityType,
);
goto(getRouteFromName(values.newName, entityType), {
Expand Down
31 changes: 14 additions & 17 deletions web-common/src/features/entity-management/actions.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,31 @@
import { goto } from "$app/navigation";
import { notifications } from "@rilldata/web-common/components/notifications";
import { extractFileName } from "@rilldata/web-common/features/sources/extract-file-name";
import { appScreen } from "@rilldata/web-common/layout/app-store";
import {
runtimeServiceDeleteFile,
runtimeServiceRenameFile,
} from "@rilldata/web-common/runtime-client";
import { httpRequestQueue } from "@rilldata/web-common/runtime-client/http-client";
import { get } from "svelte/store";
import {
getFileAPIPathFromNameAndType,
getLabel,
getRouteFromName,
removeLeadingSlash,
} from "./entity-mappers";
import { getLabel, removeLeadingSlash } from "./entity-mappers";
import { getNextEntityName } from "./name-utils";
import type { EntityType } from "./types";

export async function renameFileArtifact(
instanceId: string,
fromName: string,
toName: string,
fromPath: string,
toPath: string,
type: EntityType,
) {
await runtimeServiceRenameFile(instanceId, {
fromPath: getFileAPIPathFromNameAndType(fromName, type),
toPath: getFileAPIPathFromNameAndType(toName, type),
fromPath,
toPath,
});

const fromName = extractFileName(fromPath);
const toName = extractFileName(toPath);

httpRequestQueue.removeByName(fromName);
notifications.send({
message: `Renamed ${getLabel(type)} ${fromName} to ${toName}`,
Expand All @@ -35,24 +34,22 @@ export async function renameFileArtifact(

export async function deleteFileArtifact(
instanceId: string,
name: string,
filePath: string,
type: EntityType,
names: Array<string>,
allPaths: Array<string>,
showNotification = true,
) {
const path = getFileAPIPathFromNameAndType(name, type);
const name = extractFileName(filePath);
try {
await runtimeServiceDeleteFile(instanceId, removeLeadingSlash(path));
await runtimeServiceDeleteFile(instanceId, removeLeadingSlash(filePath));

httpRequestQueue.removeByName(name);
if (showNotification) {
notifications.send({ message: `Deleted ${getLabel(type)} ${name}` });
}

if (get(appScreen)?.name === name) {
const route = getRouteFromName(getNextEntityName(names, name), type);

await goto(route);
await goto(getNextEntityName(allPaths, name));
Copy link
Contributor

Choose a reason for hiding this comment

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

When we migrate, navigation to the "next entity name" will be confusing in cases where users do not have a sources/, models/, dashboards/ directory structure. So instead, to avoid too much magic in the code, I think we should navigate to the root / in all cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm true. I thought we could navigate to the next file in the folder. But i havent seen any editor do that.

}
} catch (err) {
console.error(err);
Expand Down
5 changes: 4 additions & 1 deletion web-common/src/features/entity-management/file-selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { createRuntimeServiceListFiles } from "@rilldata/web-common/runtime-clie
export function useMainEntityFiles(
instanceId: string,
prefix: "sources" | "models" | "dashboards" | "charts" | "custom-dashboards",
transform = (name: string) => name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than this transform parameter, can we instead just edit the createRuntimeServiceListFiles selector function to return file paths not file names?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but it is still used in a lot of places without the fill path. Lets do that refactor once we get to different entities.

) {
let extension: string;
switch (prefix) {
Expand Down Expand Up @@ -40,7 +41,9 @@ export function useMainEntityFiles(
})
.map((filePath) => {
// Remove the directory and extension from the filePath to get the file name
return filePath.replace(`/${prefix}/`, "").replace(extension, "");
return transform(
filePath.replace(`/${prefix}/`, "").replace(extension, ""),
);
})
// Sort the file names alphabetically in a case-insensitive manner
.sort((fileNameA, fileNameB) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { goto } from "$app/navigation";
import { notifications } from "@rilldata/web-common/components/notifications";
import { renameFileArtifact } from "@rilldata/web-common/features/entity-management/actions";
import { getFileAPIPathFromNameAndType } from "@rilldata/web-common/features/entity-management/entity-mappers";
import {
INVALID_NAME_MESSAGE,
VALID_NAME_PATTERN,
Expand Down Expand Up @@ -39,7 +40,12 @@
try {
const toName = e.target.value;
const type = EntityType.MetricsDefinition;
await renameFileArtifact(runtimeInstanceId, metricsDefName, toName, type);
await renameFileArtifact(
runtimeInstanceId,
getFileAPIPathFromNameAndType(metricsDefName, type),
getFileAPIPathFromNameAndType(toName, type),
type,
);
goto(`/dashboard/${toName}/edit`, { replaceState: true });
} catch (err) {
console.error(err.response.data.message);
Expand Down
15 changes: 9 additions & 6 deletions web-common/src/features/models/navigation/ModelMenuItems.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
import Cancel from "@rilldata/web-common/components/icons/Cancel.svelte";
import EditIcon from "@rilldata/web-common/components/icons/EditIcon.svelte";
import Explore from "@rilldata/web-common/components/icons/Explore.svelte";
import { getFilePathFromNameAndType } from "@rilldata/web-common/features/entity-management/entity-mappers";
import {
getFileAPIPathFromNameAndType,
getFilePathFromNameAndType,
} from "@rilldata/web-common/features/entity-management/entity-mappers";
import { getFileHasErrors } from "@rilldata/web-common/features/entity-management/resources-store";
import { EntityType } from "@rilldata/web-common/features/entity-management/types";
import { BehaviourEventMedium } from "@rilldata/web-common/metrics/service/BehaviourEventTypes";
Expand All @@ -14,7 +17,7 @@
import { runtime } from "../../../runtime-client/runtime-store";
import { deleteFileArtifact } from "../../entity-management/actions";
import { useCreateDashboardFromTableUIAction } from "../../metrics-views/ai-generation/generateMetricsView";
import { useModel, useModelFileNames } from "../selectors";
import { useModel, useModelRoutes } from "../selectors";
import NavigationMenuItem from "@rilldata/web-common/layout/navigation/NavigationMenuItem.svelte";
import NavigationMenuSeparator from "@rilldata/web-common/layout/navigation/NavigationMenuSeparator.svelte";

Expand All @@ -25,7 +28,7 @@
const queryClient = useQueryClient();
const dispatch = createEventDispatcher();

$: modelNames = useModelFileNames($runtime.instanceId);
$: modelRoutes = useModelRoutes($runtime.instanceId);
$: modelHasError = getFileHasErrors(
queryClient,
$runtime.instanceId,
Expand All @@ -45,12 +48,12 @@
);

const handleDeleteModel = async (modelName: string) => {
if ($modelNames.data) {
if ($modelRoutes.data) {
await deleteFileArtifact(
$runtime.instanceId,
modelName,
getFileAPIPathFromNameAndType(modelName, EntityType.Model),
EntityType.Model,
$modelNames.data,
$modelRoutes.data,
);
}
};
Expand Down
8 changes: 8 additions & 0 deletions web-common/src/features/models/selectors.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { getRouteFromName } from "@rilldata/web-common/features/entity-management/entity-mappers";
import { useMainEntityFiles } from "@rilldata/web-common/features/entity-management/file-selectors";
import {
ResourceKind,
useFilteredResourceNames,
useFilteredResources,
useResource,
} from "@rilldata/web-common/features/entity-management/resource-selectors";
import { EntityType } from "@rilldata/web-common/features/entity-management/types";
import {
V1ListFilesResponse,
createRuntimeServiceGetFile,
Expand All @@ -31,6 +33,12 @@ export function useModelFileNames(instanceId: string) {
return useMainEntityFiles(instanceId, "models");
}

export function useModelRoutes(instanceId: string) {
return useMainEntityFiles(instanceId, "models", (name) =>
getRouteFromName(name, EntityType.Model),
);
}

export function useModel(instanceId: string, name: string) {
return useResource(instanceId, name, ResourceKind.Model);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import { useGetDashboardsForModel } from "../../dashboards/selectors";
import { renameFileArtifact } from "../../entity-management/actions";
import {
getFileAPIPathFromNameAndType,
getFilePathFromNameAndType,
getRouteFromName,
} from "../../entity-management/entity-mappers";
Expand Down Expand Up @@ -86,8 +87,8 @@
const entityType = EntityType.Model;
await renameFileArtifact(
runtimeInstanceId,
modelName,
toName,
getFileAPIPathFromNameAndType(modelName, entityType),
getFileAPIPathFromNameAndType(toName, entityType),
entityType,
);
await goto(getRouteFromName(toName, entityType), {
Expand All @@ -114,7 +115,7 @@
</svelte:fragment>
</IconButton>
</svelte:fragment>
<svelte:fragment slot="cta" let:width>
<svelte:fragment let:width slot="cta">
{@const collapse = width < 800}
<PanelCTA side="right">
<ModelWorkspaceCTAs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ const FILE_PATH_SPLIT_REGEX = /\//;
export const INVALID_CHARS = /[^a-zA-Z_\d]/g;

export function getTableNameFromFile(filePath: string, name?: string) {
return name ?? sanitizeEntityName(extractTableName(filePath));
return name ?? sanitizeEntityName(extractFileName(filePath));
}

export function extractTableName(filePath: string): string {
export function extractFileName(filePath: string): string {
let fileName = filePath.split(FILE_PATH_SPLIT_REGEX).slice(-1)[0];
const lastIndexOfDot = fileName.lastIndexOf(".");
fileName =
Expand Down
Loading
Loading