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.

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

bugfix/empty-local-path #458

Closed
wants to merge 2 commits into from
Closed

bugfix/empty-local-path #458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd9bcd3
bugfix/empty-local-path
BrianWhitneyAI Feb 28, 2025
b965a9a
Merge branch 'main' into bugfix/empty-local-path
BrianWhitneyAI Mar 13, 2025
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
42 changes: 15 additions & 27 deletions packages/core/components/FileDetails/FileAnnotationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,19 @@ export default function FileAnnotationList(props: FileAnnotationListProps) {
const annotations = useSelector(metadata.selectors.getSortedAnnotations);
const { executionEnvService } = useSelector(interaction.selectors.getPlatformDependentServices);

// The path to this file on the host this application is running on
// may not match the path to this file stored in the database.
// Determine this local path.
const [localPath, setLocalPath] = React.useState<string | null>(null);
// Format path for mounted users
const [formattedLocalPath, setFormattedLocalPath] = React.useState<string | null>(null);
React.useEffect(() => {
let active = true;

async function formatPathForHost() {
if (!fileDetails || !active) return;

const localPath = fileDetails.localPath;
const path = localPath ? await executionEnvService.formatPathForHost(localPath) : null;

setLocalPath(path);
async function formatLocalPath() {
const local = fileDetails?.localPath?.trim();
if (!local) {
setFormattedLocalPath(null);
return;
}
const formatted = await executionEnvService.formatPathForHost(local);
setFormattedLocalPath(formatted);
}

formatPathForHost();

return () => {
active = false;
};
formatLocalPath();
}, [fileDetails, executionEnvService]);

const content: JSX.Element | JSX.Element[] | null = React.useMemo(() => {
Expand All @@ -66,15 +58,11 @@ export default function FileAnnotationList(props: FileAnnotationListProps) {
let fmsStateIndicator = false;

if (annotation.name === AnnotationName.LOCAL_FILE_PATH) {
if (fileDetails && fileDetails.downloadInProgress) {
annotationValue = "Copying to VAST in progress…";
if (fileDetails.downloadInProgress) {
annotationValue = fileDetails.localPath || Annotation.MISSING_VALUE;
fmsStateIndicator = true;
} else if (localPath === null) {
// localPath hasn't loaded yet or there is no local path annotation
return accum;
} else {
// Use the user's /allen mount point, if known
annotationValue = localPath;
annotationValue = formattedLocalPath || Annotation.MISSING_VALUE;
}
}

Expand All @@ -99,7 +87,7 @@ export default function FileAnnotationList(props: FileAnnotationListProps) {
/>,
];
}, [] as JSX.Element[]);
}, [annotations, fileDetails, isLoading, localPath]);
}, [annotations, fileDetails, formattedLocalPath, isLoading]);

return <div className={classNames(styles.list, className)}>{content}</div>;
}
2 changes: 1 addition & 1 deletion packages/core/entity/Annotation/AnnotationName.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default {
FILE_NAME: "file_name", // a file attribute (top-level prop on file documents in MongoDb)
FILE_SIZE: "file_size", // a file attribute (top-level prop on file documents in MongoDb)
FILE_PATH: "file_path", // a file attribute (top-level prop on file documents in MongoDb)
LOCAL_FILE_PATH: "File Path (Local VAST)", // (optional) annotation for FMS files on the local NAS
LOCAL_FILE_PATH: "file_local_path", // (optional) annotation for FMS files on the local NAS
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the need for this? When does "File Path (Local VAST)" come into play?

PLATE_BARCODE: "Plate Barcode",
SHOULD_BE_IN_LOCAL: "Should Be in Local Cache",
THUMBNAIL_PATH: "thumbnail", // (optional) file attribute (top-level prop on the file documents in MongoDb)
Expand Down
38 changes: 8 additions & 30 deletions packages/core/entity/FileDetail/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ const NAS_HOST_PREFIXES: Record<Environment, string> = {
export interface FmsFile {
annotations: FmsFileAnnotation[];
file_id?: string;
file_local_path?: string | null;
file_name?: string;
file_path?: string;
file_size?: number;
Expand All @@ -92,16 +93,6 @@ export default class FileDetail {
private readonly env: Environment;
private readonly uniqueId?: string;

// REMOVE THIS FUNCITON (BACKWARDS COMPAT)
public convertAicsDrivePathToAicsS3Path(path: string): string {
const pathWithoutDrive = path.replace(NAS_HOST_PREFIXES[this.env], "");
// Should probably record this somewhere we can dynamically adjust to, or perhaps just in the file
// document itself, alas for now this will do.
return FileDetail.convertAicsS3PathToHttpUrl(
`${AICS_FMS_S3_BUCKETS[this.env]}${pathWithoutDrive}`
);
}

private static convertAicsS3PathToHttpUrl(path: string): string {
return `${AICS_FMS_S3_URL_PREFIX}${path}`;
}
Expand All @@ -110,6 +101,7 @@ export default class FileDetail {
this.fileDetail = fileDetail;
this.env = env;
this.uniqueId = uniqueId;
this.fileDetail.file_local_path = this.localPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the inner fileDetail here?

}

public get details() {
Expand Down Expand Up @@ -157,23 +149,14 @@ export default class FileDetail {
}

public get cloudPath(): string {
//// REMOVE THIS (BACKWARDS COMPAT)
if (this.path.startsWith("/allen")) {
return this.convertAicsDrivePathToAicsS3Path(this.path);
}
////

// AICS FMS files' paths are cloud paths
return this.path;
}

public get localPath(): string | null {
//// REMOVE THIS (BACKWARDS COMPAT)
if (this.path.startsWith("/allen")) {
return this.path;
if (this.downloadInProgress) {
return "Copying to VAST in progress…";
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this

}
////

if (this.getAnnotation("Cache Eviction Date")) {
return this.getLocalPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just move the body of the getLocalPath function in here? Feels funky that there are two localPath functions.

Like so:

const cloudPrefix = `${AICS_FMS_S3_URL_PREFIX}${AICS_FMS_S3_BUCKETS[this.env]}`;
if (this.getAnnotation("Cache Eviction Date") && this.path.startsWith(cloudPrefix)) {
    const localPrefix = NAS_HOST_PREFIXES[this.env];
    const relativePath = this.path.replace(`${cloudPrefix}`, "");
    return `${localPrefix}${relativePath}`;
}

return null;

}
Expand All @@ -193,12 +176,6 @@ export default class FileDetail {
}

public get downloadPath(): string {
//// REMOVE THIS (BACKWARDS COMPAT)
if (this.path.startsWith("/allen")) {
return `http://aics.corp.alleninstitute.org/labkey/fmsfiles/image${this.path}`;
}
////

const localPath = this.getLocalPath();
// For AICS files that are available on the Vast, users can use the cloud path, but the
// download will be faster and not incur egress fees if we download via the local network.
Expand All @@ -211,7 +188,7 @@ export default class FileDetail {

public get downloadInProgress(): boolean {
const shouldBeInLocal = this.getFirstAnnotationValue(AnnotationName.SHOULD_BE_IN_LOCAL);
return Boolean(shouldBeInLocal) && !this.getLocalPath();
return Boolean(shouldBeInLocal) && !this.getAnnotation("Cache Eviction Date");
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like now that it is used a couple places "Cache Eviction Date" should be added to AnnotationName and used like a constant

}

public get size(): number | undefined {
Expand Down Expand Up @@ -239,9 +216,10 @@ export default class FileDetail {
public getFirstAnnotationValue(annotationName: string): string | number | boolean | undefined {
return this.getAnnotation(annotationName)?.values[0];
}

public getAnnotation(annotationName: string): FmsFileAnnotation | undefined {
return this.fileDetail.annotations.find((annotation) => annotation.name === annotationName);
return this.fileDetail.annotations?.find(
(annotation) => annotation.name === annotationName
);
}

public async getPathToThumbnail(): Promise<string | undefined> {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/entity/FileSelection/test/FileSelection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ describe("FileSelection", () => {
const fileExplorerServiceBaseUrl = FESBaseUrl.TEST;
const queryResult = [];
for (let i = 0; i < 31; i++) {
queryResult.push(i);
queryResult.push({ file_id: `${i}` });
}
// Due to overfetching the result set we desire is a subsection of query results
const expectedDetails = queryResult
Expand Down
1 change: 1 addition & 0 deletions packages/core/services/FileService/DatabaseFileService/test/DatabaseFileService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ describe("DatabaseFileService", () => {
values: ["6"],
},
],
file_local_path: null,
});
});
});
Expand Down
3 changes: 2 additions & 1 deletion packages/desktop/src/services/ExecutionEnvServiceElectron.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ export default class ExecutionEnvServiceElectron implements ExecutionEnvService
public async formatPathForHost(posixPath: string): Promise<string> {
const fmsPath = new FmsFilePath(posixPath);

if (this.getOS() === "Darwin") {
// Only attempt to probe for a mount point if fileShare is defined.
if (this.getOS() === "Darwin" && fmsPath.fileShare) {
const mountPoint = await this.probeForMountPoint(fmsPath.fileShare);
if (mountPoint) {
return fmsPath.withMountPoint(mountPoint).formatForOs(this.getOS());
Expand Down