Skip to content

Feature/add prov to aics fms#869

Open
SeanDuHare wants to merge 16 commits into
mainfrom
feature/add-prov-to-aics-fms
Open

Feature/add prov to aics fms#869
SeanDuHare wants to merge 16 commits into
mainfrom
feature/add-prov-to-aics-fms

Conversation

@SeanDuHare

@SeanDuHare SeanDuHare commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Context

Provenance is available for use in the application, however for internal users their query defaults to AICS FMS which won't include provenance at the moment.

Changes

This adds a provenance source to the default AICS FMS query. While doing so I encountered 3 other things:

  1. The origin would be out of sight after the graph loads for large graphs, so I added a zoom to NetworkGraph/index.tsx
  2. "File Path" is not a good column to query AICS FMS by, so this changes the column to search to "File ID" when querying AICS FMS
  3. Fixed a bug that happens on large graphs when the node searching ends early. If an edge exists without both nodes existing already the graph would generate a placeholder node that would cause the graph.node(<node id>) to bubble up an error (Graph/index.ts)

Testing

Tested using this provenance source file

Example

Screenshot 2026-07-08 at 10 21 43 AM

This comment was marked as resolved.

return createFileNode(file);
}
} catch {
// Backup while moving between provenance versions: Try looking by "File ID", this should be removed in the future

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aswallace lmk if you think we should still have a fallback

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine with me! I don't think we have enough people using this right now for it to be an issue. I only sent the old template to Chantelle, so we can check in with her directly

@SeanDuHare SeanDuHare marked this pull request as ready for review July 8, 2026 17:59
lastCenteredRefreshKeyRef.current = refreshKey;
const centerX = originNode.position.x + (originNode.width ?? 0) / 2;
const centerY = originNode.position.y + (originNode.height ?? 0) / 2;
setCenter(centerX, centerY, { zoom: getZoom(), duration: 800 });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whats duration here? 800?

@SeanDuHare SeanDuHare Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Time to pan, its in milliseconds. Just makes it animate a little rather than jump suddenly.

@aswallace aswallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall lgtm! A couple bugs(?) that I wasn't sure where to comment about:

I'm still seeing some floating files, e.g., if you scroll to the right for this query

Image

I'm also running into an annotationName.split is not a function error for opening queries related to certain edge connection types, I think because it's expecting "is derived from" to have a "pointer" type

Screen.Recording.2026-07-09.at.9.43.35.AM.mov

lastCenteredRefreshKeyRef.current = refreshKey;
const centerX = originNode.position.x + (originNode.width ?? 0) / 2;
const centerY = originNode.position.y + (originNode.height ?? 0) / 2;
setCenter(centerX, centerY, { zoom: getZoom(), duration: 800 });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is really nice on initial node and when resetting the view! but it's slightly inconvenient when working elsewhere in the graph, e.g., if changing the organization structure

Screen.Recording.2026-07-09.at.8.52.19.AM.mov

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh good point! I'll make sure it just functions on the initial load (maybe the reset too?).

return createFileNode(file);
}
} catch {
// Backup while moving between provenance versions: Try looking by "File ID", this should be removed in the future

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine with me! I don't think we have enough people using this right now for it to be an issue. I only sent the old template to Chantelle, so we can check in with her directly

Comment thread packages/core/services/FileService/DatabaseFileService/index.ts Outdated
private static readonly CSV_ENDPOINT_VERSION = "2.0";
public static readonly BASE_CSV_DOWNLOAD_URL = `file-explorer-service/${HttpFileService.CSV_ENDPOINT_VERSION}/files/selection/manifest`;
private readonly downloadService: FileDownloadService;
public readonly provenanceIdColumn = "file_id";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Making sure I'm on the same page-- For "Input File Path", we just render an entity instead of looking up the file path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we're talking in Slack about this causing issues for Brian's work potentially & it does seem like we could add a way for each edge to define how to look up their file's ID column

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rolling with this for now (7c52510) but it does feel increasingly like the edges should define their lookup column - lmk if anyone has thoughts or ideas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants