Skip to content

EAGLE-1556: Script to update old graphs#1002

Open
james-strauss-uwa wants to merge 19 commits intomasterfrom
eagle-1556
Open

EAGLE-1556: Script to update old graphs#1002
james-strauss-uwa wants to merge 19 commits intomasterfrom
eagle-1556

Conversation

@james-strauss-uwa
Copy link
Collaborator

@james-strauss-uwa james-strauss-uwa commented Feb 25, 2026

A new feature that uses EAGLE to load old graphs and save them in the latest format. Effectively updating old graphs.

This initially called for a playwright script, but it seemed useful to allow the user to select the source and destination repositories/branches without having to edit the script each time.

The basics of this are working, I just need some feedback on the remaining parts:

  • Is the current menu location good. It is in a new Help > Tools menu. Is there a better place for it? Will we ever have other tools here?
  • Do we need more feedback on success rate? For example, if one or more graphs fails to update, it just displays "Error" in the table row(s). Do we need to make that more obvious?
  • At the moment, the user must have already created a new destination branch in their destination repository. The user must also have added this repository/branch to the list of repos in EAGLE. Creating the repo/branch must be done outside EAGLE. Is this clear to the user? Should we add code to allow the user to specify a new branch and create it at runtime?
  • Any feedback on the UI/UX flow.

Summary by Sourcery

Add a UI-driven tool for updating graphs from old repositories to the latest format and batch-pushing them to remote Git repositories.

New Features:

  • Introduce a Graph Updater modal that lets users select source and destination repositories, fetch all graphs, update them to the latest schema, and push changes in bulk.
  • Add keyboard shortcut and Help > Tools menu entry to launch the Graph Updater workflow from the UI.
  • Support batch-saving multiple graph files to remote GitHub/GitLab repositories via new backend endpoints and a client helper.

Enhancements:

  • Extend repository browsing utilities to expand all folders and collect all graph files, and expose full path names for repository files.
  • Improve GitHub save error handling by catching metadata-setting failures and returning structured errors.

Summary by Sourcery

Introduce a UI-driven Graph Updater workflow to batch-convert legacy graph files to the latest schema and push them to selected Git repositories.

New Features:

  • Add a Graph Updater modal that lets users choose source and destination repositories, fetch all graph files, convert them to the latest format, and push them in bulk.
  • Expose the Graph Updater via a Help > Tools menu entry and a keyboard shortcut.
  • Implement backend endpoints and client support for saving multiple graph files to remote GitHub/GitLab repositories in a single commit.

Enhancements:

  • Extend repository browsing to expand all folders and collect all graph files, and expose full path names for repository files.
  • Improve GitHub save error handling by catching metadata-setting failures and returning structured errors for UI display.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Feb 25, 2026

Reviewer's Guide

Add a UI-driven Graph Updater tool that discovers all graph files in a selected source repository, upgrades them to the latest schema, and batch-pushes them to a selected destination GitHub/GitLab repository via new backend endpoints and client helpers, including improved metadata/error handling.

Updated class diagram for GraphUpdater and related repository types

classDiagram
    class GraphUpdaterFile {
        +string data
        +ko.Observable~RepositoryFile~ file
        +ko.Observable~GraphUpdater.FileStatus~ state
        +GraphUpdaterFile(file RepositoryFile)
    }

    class GraphUpdater {
        <<static>>
        +ko.Observable~boolean~ isFetching
        +ko.Observable~boolean~ hasFetched
        +ko.Observable~boolean~ isUpdating
        +ko.Observable~boolean~ hasUpdated
        +Repository sourceRepository
        +ko.ObservableArray~GraphUpdaterFile~ updatedLogicalGraphs
        +findIndexOfNodeDataArrayWithId(nodeDataArray any[], id NodeId) number
        +usesNodeKeys(graphObject any) boolean
        +updateKeysToIds(graphObject any) void
        +showModal() Promise~void~
        +hideModal() Promise~void~
        +setState(isFetching boolean, hasFetched boolean, isUpdating boolean, hasUpdated boolean) void
        +onSourceRepositoryChange() void
        +fetchLogicalGraphs() Promise~void~
        +update() Promise~void~
        +push() Promise~void~
    }

    class GraphUpdater_FileStatus {
        <<enumeration>>
        No
        Updating
        Error
        Success
    }

    class Repository {
        +RepositoryService service
        +string name
        +string branch
        +files() RepositoryFile[]
        +folders() RepositoryFolder[]
        +select() Promise~void~
        +refresh() Promise~void~
        +findAllGraphs() RepositoryFile[]
        +expandAll() Promise~void~
        +getNameAndBranch() string
    }

    class RepositoryFile {
        +RepositoryService service
        +string path
        +string name
        +Eagle.FileType type
        +ko.PureComputed~string~ htmlId
        +ko.PureComputed~string~ pathAndName
    }

    class Eagle {
        +saveFilesToRemote(repository Repository, jsonString string) Promise~void~
        +changeRightWindowMode(mode RightWindowMode) void
        +getInstance() Eagle
    }

    class Setting {
        <<static>>
        +SchemaVersion enum
        +findValue(key string) string
    }

    class LogicalGraph {
        <<static>>
        +fromOJSJson(graphObject any, fileName string, errorsWarnings Errors.ErrorsWarnings) LogicalGraph
        +fromV4Json(graphObject any, fileName string, errorsWarnings Errors.ErrorsWarnings) LogicalGraph
        +toV4JsonString(logicalGraph LogicalGraph, pretty boolean) string
    }

    class GitHub {
        <<static>>
        +openRemoteFile(service RepositoryService, repositoryName string, repositoryBranch string, filePath string, fileName string) Promise~string~
    }

    class GitLab {
        <<static>>
        +openRemoteFile(service RepositoryService, repositoryName string, repositoryBranch string, filePath string, fileName string) Promise~string~
    }

    class Utils {
        <<static>>
        +determineSchemaVersion(graphObject any) Setting.SchemaVersion
        +httpPostJSONString(url string, jsonString string) Promise~void~
        +showNotification(title string, message string, level string) void
        +showUserMessage(title string, message string) void
    }

    GraphUpdaterFile --> RepositoryFile
    GraphUpdater --> GraphUpdaterFile : manages
    GraphUpdater --> GraphUpdater_FileStatus : uses
    GraphUpdater --> Repository : sourceRepository
    GraphUpdater --> LogicalGraph : upgrades_graphs
    GraphUpdater --> Setting : uses_SchemaVersion
    GraphUpdater --> GitHub : loads_files_from
    GraphUpdater --> GitLab : loads_files_from
    GraphUpdater --> Utils : uses_helpers

    Repository --> RepositoryFile : contains
    Eagle --> Repository : operates_on
    Eagle --> Utils : uses_httpPost
Loading

File-Level Changes

Change Details Files
Introduce Graph Updater workflow, including modal UI, state management, and graph upgrade logic.
  • Add GraphUpdaterFile helper to track per-file data and update state with Knockout observables.
  • Implement GraphUpdater static state flags and updatedLogicalGraphs collection to drive the modal flow.
  • Implement showModal/hideModal to populate source/destination repo dropdowns and control the Bootstrap modal lifecycle.
  • Implement fetchLogicalGraphs to expand the selected source repository, discover all graph files, and populate updatedLogicalGraphs.
  • Implement update to load each graph remotely, detect schema version, convert key-based graphs to id-based, parse using appropriate fromJson, and serialize to V4 JSON while tracking per-file success/error state.
  • Implement push to assemble updated files, build a commit payload with token and commit message, invoke Eagle.saveFilesToRemote, and close the modal on success.
  • Add FileStatus enum to represent per-file update status and drive the table rendering.
src/GraphUpdater.ts
templates/tools/graph_updater.html
static/base.css
src/main.ts
Extend repository model and file metadata to support graph discovery and path handling required by the updater.
  • Add Repository.findAllGraphs to recursively traverse folders/files and collect all graph-type files.
  • Add Repository.expandAll to recursively select the repository and all subfolders to ensure contents are loaded before discovery.
  • Add RepositoryFile.pathAndName computed to provide full relative paths for use when committing files remotely.
src/Repository.ts
src/RepositoryFile.ts
Add backend and client support for batch-saving files to remote Git repositories, with improved GitHub error handling.
  • Wrap set_metadata_for_ingress in save_git_hub_file with try/except and route errors through github_exception_handler.
  • Add /saveFilesToRemoteGithub endpoint that accepts a JSON payload with repository details, token, list of files, and commit message; sets metadata per file, builds a Git tree, and creates a single commit updating all files.
  • Introduce Eagle.saveFilesToRemote client helper to POST a JSON string to either GitHub or GitLab batch-save endpoints, refresh the repository, and show success/error notifications.
  • Preserve GitLab save endpoint behavior while removing trailing whitespace and wiring into the generic client helper.
eagleServer/eagleServer.py
src/Eagle.ts
Expose the Graph Updater as a first-class tool in the UI via menu and keyboard shortcut.
  • Add a Help > Tools > Graph Updater submenu entry that opens the updater modal and shows its shortcut.
  • Register a KeyboardShortcut with id graph_updater (key 9) that calls GraphUpdater.showModal.
  • Expose GraphUpdater on the window object for use by Knockout bindings and templates.
  • Add CSS to constrain the height of the graph table area within the modal and enable scrolling.
templates/navbar.html
src/KeyboardShortcut.ts
src/main.ts
static/base.css

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 4 issues, and left some high level feedback:

  • In GraphUpdater.update you call openRemoteFileFunc with graphFile.file().repository.* but RepositoryFile does not expose a repository property, so this will be undefined at runtime; instead derive service/name/branch from GraphUpdater.sourceRepository (or add a repository reference to RepositoryFile) to avoid failures when fetching files.
  • In Eagle.saveFilesToRemote the catch block references data even though it will be undefined when the error occurs; use the caught error (and/or the HTTP response body from Utils.httpPostJSONString) when constructing the message to avoid logging and showing undefined.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In GraphUpdater.update you call openRemoteFileFunc with graphFile.file().repository.* but RepositoryFile does not expose a repository property, so this will be undefined at runtime; instead derive service/name/branch from GraphUpdater.sourceRepository (or add a repository reference to RepositoryFile) to avoid failures when fetching files.
- In Eagle.saveFilesToRemote the catch block references `data` even though it will be undefined when the error occurs; use the caught `error` (and/or the HTTP response body from Utils.httpPostJSONString) when constructing the message to avoid logging and showing `undefined`.

## Individual Comments

### Comment 1
<location path="src/GraphUpdater.ts" line_range="359-362" />
<code_context>
+            // parse file data as LogicalGraph
+            let lg: LogicalGraph;
+            try {
+                lg = fromJsonFunc(graphObject, graphFile.file.name, {"errors":[], "warnings":[]});
+            }
+            catch (error) {
+                console.error("Error parsing graph file:", graphFile.file.name, error);
+                graphFile.state(GraphUpdater.FileStatus.Error);
+                continue;
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `graphFile.file` as a property instead of calling the observable is likely a runtime bug.

`GraphUpdaterFile.file` is a `ko.Observable<RepositoryFile>`, but it’s being treated as a plain object here. You’re reading `graphFile.file.name`, which is the observable function’s name, not the `RepositoryFile` name. Both the `fromJsonFunc` call and the `console.error` should use `graphFile.file().name` instead.
</issue_to_address>

### Comment 2
<location path="src/Eagle.ts" line_range="1969-1978" />
<code_context>
+    saveFilesToRemote = async (repository: Repository, jsonString : string): Promise<void> => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Error path in `saveFilesToRemote` uses an uninitialised `data` variable and may leave the promise pending for unsupported services.

In the `catch` block, `data` may be `undefined` if `httpPostJSONString` throws before assignment, so the user message and console log will be misleading. Prefer using the caught `error` when building the message/log instead of `data`. Also, in the `default` branch of `switch (repository.service)`, the function `return`s without resolving or rejecting the promise, which can leave callers hanging; reject the promise (or throw) there instead.
</issue_to_address>

### Comment 3
<location path="src/GraphUpdater.ts" line_range="485-486" />
<code_context>
+            await new Promise<void>((resolve, reject) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** New promise wrapper around `openRemoteFileFunc` never handles rejections, which can stall `attemptLoadLogicalGraphTable`.

Because `openRemoteFileFunc` now returns a `Promise<string>`, this `new Promise` only settles in the `.then` path. If `openRemoteFileFunc` rejects, this promise never resolves or rejects, so the surrounding `for`-loop can hang. Either handle errors with a `.catch` that records the failure and settles the promise, or drop the manual `new Promise` and just `await openRemoteFileFunc(...)` inside a try/catch.
</issue_to_address>

### Comment 4
<location path="src/Repository.ts" line_range="187-189" />
<code_context>
         });
     }

+    // expand all the directories
+    expandAll = async () : Promise<void> => {
+        return new Promise(async(resolve) => {
+
+            async function traverseFolder(folder: RepositoryFolder) : Promise<void> {
</code_context>
<issue_to_address>
**suggestion:** `expandAll` uses an unnecessary `new Promise` wrapper around an `async` function.

Because `expandAll` is already `async`, you can remove the `new Promise` wrapper and `await this.select()` and the recursive traversal directly. This avoids the double-async pattern and keeps error handling straightforward.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@james-strauss-uwa james-strauss-uwa removed the request for review from M-Wicenec February 25, 2026 03:01
@james-strauss-uwa
Copy link
Collaborator Author

@sourcery-ai review

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 3 issues, and left some high level feedback:

  • GraphUpdater.push is typed/used as an async void-style operation but returns an error string in the catch path; consider either throwing/rejecting on error or changing the signature so callers can reliably distinguish success/failure without mixing side-effect notifications and return values.
  • The GraphUpdater UI state is tracked by four separate booleans (isFetching/hasFetched/isUpdating/hasUpdated), which makes illegal combinations easy; consider replacing these with a single observable enum or state machine to simplify transitions and avoid inconsistent UI states.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- GraphUpdater.push is typed/used as an async void-style operation but returns an error string in the catch path; consider either throwing/rejecting on error or changing the signature so callers can reliably distinguish success/failure without mixing side-effect notifications and return values.
- The GraphUpdater UI state is tracked by four separate booleans (isFetching/hasFetched/isUpdating/hasUpdated), which makes illegal combinations easy; consider replacing these with a single observable enum or state machine to simplify transitions and avoid inconsistent UI states.

## Individual Comments

### Comment 1
<location path="src/GraphUpdater.ts" line_range="229-231" />
<code_context>
+        this.setState(true, false, false, false);
+
+        // get source repository
+        const srcRepoIndex = parseInt($('#graphUpdaterModalSourceRepositorySelect').val() as string);
+        const srcRepo = Repositories.repositories()[srcRepoIndex];
+        if (srcRepo === null){
+            Utils.showNotification("Error", "Source repository not found", "danger");
+            this.setState(false, false, false, false);
</code_context>
<issue_to_address>
**issue:** Handle invalid/missing source repo selection more robustly (undefined/NaN vs null).

If no option is selected or `val()` is `null`, `parseInt` will return `NaN`, so `Repositories.repositories()[srcRepoIndex]` will be `undefined`, not `null`, and the current `if (srcRepo === null)` check won’t trigger. This can cause runtime errors when calling `this.sourceRepository.expandAll()`. Consider guarding against `Number.isNaN(srcRepoIndex)` and checking for a missing repository (e.g., `srcRepo == null` or validating `srcRepoIndex` is in range) before proceeding.
</issue_to_address>

### Comment 2
<location path="src/GraphUpdater.ts" line_range="332-334" />
<code_context>
+
+    static async push(): Promise<void> {
+        // get destination repository
+        const destRepoIndex = parseInt($('#graphUpdaterModalDestinationRepositorySelect').val() as string);
+        const destRepo = Repositories.repositories()[destRepoIndex];
+        if (destRepo === null){
+            Utils.showNotification("Error", "Destination repository not found", "danger");
+            return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Destination repo selection has the same undefined/NaN handling issue as source.

Here `parseInt` may return `NaN` and the array access may return `undefined`, but the code only checks for `null`. This can still lead to using an `undefined` repository. Please align the validation with the source selection (e.g., `Number.isNaN(destRepoIndex)`, index bounds, and a `destRepo == null` check).
</issue_to_address>

### Comment 3
<location path="src/Eagle.ts" line_range="1969-1959" />
<code_context>
+    saveFilesToRemote = async (repository: Repository, jsonString : string): Promise<void> => {
</code_context>
<issue_to_address>
**suggestion:** The `saveFilesToRemote` implementation wraps an `async` function in a new `Promise` unnecessarily.

Since the method is already `async`, it shouldn’t also return `new Promise(...)`. This double-wrapping adds unnecessary complexity and can obscure how errors propagate. Instead, remove `new Promise` and rely on `return` and `throw` within the async function body.

Suggested implementation:

```typescript
    saveFilesToRemote = async (repository: Repository, jsonString : string): Promise<void> => {
        let url : string;

```

Because only part of the function body is visible, you'll also need to:
1. Remove the closing `});` that matches the `new Promise(` call at the end of `saveFilesToRemote`.
2. Replace any `resolve()` calls with `return;` (or just allow the function to complete if no value is needed).
3. Replace any `reject(error)` (or `reject(someError)`) calls with `throw error;` so that the `async` function rejects its promise naturally.
4. If there's a `try/catch` that only exists to call `reject`, you can either remove it (and let errors bubble) or keep it and `throw` from the `catch` instead of calling `reject`.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +229 to +231
const srcRepoIndex = parseInt($('#graphUpdaterModalSourceRepositorySelect').val() as string);
const srcRepo = Repositories.repositories()[srcRepoIndex];
if (srcRepo === null){
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Handle invalid/missing source repo selection more robustly (undefined/NaN vs null).

If no option is selected or val() is null, parseInt will return NaN, so Repositories.repositories()[srcRepoIndex] will be undefined, not null, and the current if (srcRepo === null) check won’t trigger. This can cause runtime errors when calling this.sourceRepository.expandAll(). Consider guarding against Number.isNaN(srcRepoIndex) and checking for a missing repository (e.g., srcRepo == null or validating srcRepoIndex is in range) before proceeding.

Comment on lines +332 to +334
const destRepoIndex = parseInt($('#graphUpdaterModalDestinationRepositorySelect').val() as string);
const destRepo = Repositories.repositories()[destRepoIndex];
if (destRepo === null){
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Destination repo selection has the same undefined/NaN handling issue as source.

Here parseInt may return NaN and the array access may return undefined, but the code only checks for null. This can still lead to using an undefined repository. Please align the validation with the source selection (e.g., Number.isNaN(destRepoIndex), index bounds, and a destRepo == null check).

@@ -1959,6 +1959,62 @@ export class Eagle {
});
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: The saveFilesToRemote implementation wraps an async function in a new Promise unnecessarily.

Since the method is already async, it shouldn’t also return new Promise(...). This double-wrapping adds unnecessary complexity and can obscure how errors propagate. Instead, remove new Promise and rely on return and throw within the async function body.

Suggested implementation:

    saveFilesToRemote = async (repository: Repository, jsonString : string): Promise<void> => {
        let url : string;

Because only part of the function body is visible, you'll also need to:

  1. Remove the closing }); that matches the new Promise( call at the end of saveFilesToRemote.
  2. Replace any resolve() calls with return; (or just allow the function to complete if no value is needed).
  3. Replace any reject(error) (or reject(someError)) calls with throw error; so that the async function rejects its promise naturally.
  4. If there's a try/catch that only exists to call reject, you can either remove it (and let errors bubble) or keep it and throw from the catch instead of calling reject.

Copy link

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

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

Hey - I've found 3 issues, and left some high level feedback:

  • In GraphUpdater.push, the function is declared as returning Promise<void> but in the error path it returns errorJSON.error; this is inconsistent with the signature and will either cause a type error or silently drop the value—either change the return type or stop returning a value from this method.
  • The error handling in GraphUpdater.push assumes error is a JSON string (JSON.parse(error)), but saveFilesToRemote rejects with an error object; this will itself throw and mask the original failure, so consider standardizing the error shape (e.g. rejecting with a structured object/string) and avoiding JSON.parse on non-strings.
  • In GraphUpdater.fetchLogicalGraphs and GraphUpdater.push, the selected repository index is taken via parseInt(...val() as string) with no validation; it would be safer to guard against NaN/out-of-range indices before indexing into Repositories.repositories() so the UI can show a clear error instead of potentially throwing.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `GraphUpdater.push`, the function is declared as returning `Promise<void>` but in the error path it `return`s `errorJSON.error`; this is inconsistent with the signature and will either cause a type error or silently drop the value—either change the return type or stop returning a value from this method.
- The error handling in `GraphUpdater.push` assumes `error` is a JSON string (`JSON.parse(error)`), but `saveFilesToRemote` rejects with an error object; this will itself throw and mask the original failure, so consider standardizing the error shape (e.g. rejecting with a structured object/string) and avoiding `JSON.parse` on non-strings.
- In `GraphUpdater.fetchLogicalGraphs` and `GraphUpdater.push`, the selected repository index is taken via `parseInt(...val() as string)` with no validation; it would be safer to guard against `NaN`/out-of-range indices before indexing into `Repositories.repositories()` so the UI can show a clear error instead of potentially throwing.

## Individual Comments

### Comment 1
<location path="src/GraphUpdater.ts" line_range="229" />
<code_context>
+        this.setState(true, false, false, false);
+
+        // get source repository
+        const srcRepoIndex = parseInt($('#graphUpdaterModalSourceRepositorySelect').val() as string);
+        const srcRepo = Repositories.repositories()[srcRepoIndex];
+        if (srcRepo === null){
</code_context>
<issue_to_address>
**issue:** Handle invalid/empty repository selection more defensively.

If no option is selected, `$('#graphUpdaterModalSourceRepositorySelect').val()` can be `null`, so `parseInt` returns `NaN` and `Repositories.repositories()[NaN]` is `undefined`. The `if (srcRepo === null)` check won’t catch this. Please guard for `null`/`undefined` and `Number.isNaN(srcRepoIndex)` before indexing into `Repositories.repositories()`. The same check should be applied to the destination repo in `push()`, which also only checks for `null`.
</issue_to_address>

### Comment 2
<location path="src/GraphUpdater.ts" line_range="280" />
<code_context>
+            graphFile.state(GraphUpdater.FileStatus.Updating);
+
+            // fetch the file data
+            const fileData: string = await openRemoteFileFunc(graphFile.file().repository.service, graphFile.file().repository.name, graphFile.file().repository.branch, graphFile.file().path, graphFile.file().name);
+
+            // determine if graph is OJS or V4
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrap remote file load and parsing in a try/catch to avoid leaving the UI in a partially-updating state.

If `openRemoteFileFunc` rejects or `JSON.parse(fileData)` throws, the error will escape `update()` and the `GraphUpdaterFile` never reaches an `Error` state, so the modal spinner may never clear. Please wrap the remote load + parse + conversion for each file in try/catch, set `graphFile.state(FileStatus.Error)` on failure, and continue with remaining files. An outer try/finally to keep the global `setState` flags consistent even when a file fails would also help.
</issue_to_address>

### Comment 3
<location path="src/GraphUpdater.ts" line_range="383-388" />
<code_context>
+        } catch (error) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid assuming a JSON-encoded error and returning a string from a `Promise<void>` function.

Inside `push()`, the `catch` assumes `error` is JSON text (`JSON.parse(error)`), but `Eagle.saveFilesToRemote` rejects with the raw error object, so this will likely throw inside the handler. It also returns `errorJSON.error` (a string) from a `Promise<void>`, which is inconsistent with the signature. Instead, handle `error` as an unknown object (e.g., log / stringify it) and either rethrow or return `void` after showing the message—no return value is needed here.
</issue_to_address>

Hi @james-strauss-uwa! 👋

Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀

Install the sourcery-ai bot to get automatic code reviews on every pull request ✨

Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

this.setState(true, false, false, false);

// get source repository
const srcRepoIndex = parseInt($('#graphUpdaterModalSourceRepositorySelect').val() as string);

Choose a reason for hiding this comment

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

issue: Handle invalid/empty repository selection more defensively.

If no option is selected, $('#graphUpdaterModalSourceRepositorySelect').val() can be null, so parseInt returns NaN and Repositories.repositories()[NaN] is undefined. The if (srcRepo === null) check won’t catch this. Please guard for null/undefined and Number.isNaN(srcRepoIndex) before indexing into Repositories.repositories(). The same check should be applied to the destination repo in push(), which also only checks for null.

graphFile.state(GraphUpdater.FileStatus.Updating);

// fetch the file data
const fileData: string = await openRemoteFileFunc(graphFile.file().repository.service, graphFile.file().repository.name, graphFile.file().repository.branch, graphFile.file().path, graphFile.file().name);

Choose a reason for hiding this comment

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

issue (bug_risk): Wrap remote file load and parsing in a try/catch to avoid leaving the UI in a partially-updating state.

If openRemoteFileFunc rejects or JSON.parse(fileData) throws, the error will escape update() and the GraphUpdaterFile never reaches an Error state, so the modal spinner may never clear. Please wrap the remote load + parse + conversion for each file in try/catch, set graphFile.state(FileStatus.Error) on failure, and continue with remaining files. An outer try/finally to keep the global setState flags consistent even when a file fails would also help.

Comment on lines +383 to +388
} catch (error) {
const errorJSON = JSON.parse(error);

// try to load the file
await new Promise<void>((resolve, reject) => {
openRemoteFileFunc(row.service, row.name, row.branch, row.folder, row.file, (error: string, data: string) => {
// if file fetched successfully
if (error === null){
const errorsWarnings: Errors.ErrorsWarnings = {"errors":[], "warnings":[]};
const file: RepositoryFile = new RepositoryFile(row.service, row.folder, row.file);
const lg: LogicalGraph = LogicalGraph.fromOJSJson(JSON.parse(data), file.name, errorsWarnings);

// record number of errors
row.numLoadWarnings = errorsWarnings.warnings.length;
row.numLoadErrors = errorsWarnings.errors.length;

// use git-related info within file
row.generatorVersion = lg.fileInfo().generatorVersion;
row.lastModifiedBy = lg.fileInfo().lastModifiedName;
row.repositoryUrl = lg.fileInfo().repositoryUrl;
row.commitHash = lg.fileInfo().location.commitHash();
row.downloadUrl = lg.fileInfo().location.downloadUrl();
row.signature = lg.fileInfo().signature;

// convert date from timestamp to date string
const date = new Date(lg.fileInfo().lastModifiedDatetime * 1000);
row.lastModified = date.toLocaleDateString() + " " + date.toLocaleTimeString()

// check the graph once loaded
Utils.checkGraph(eagle);
const results: Errors.ErrorsWarnings = Utils.gatherGraphErrors();
row.numCheckWarnings = results.warnings.length;
row.numCheckErrors = results.errors.length;
}

resolve();
});
});
Utils.showUserMessage("Error", errorJSON.error + "<br/><br/>NOTE: These error messages provided by " + destRepo.service + " are not very helpful. Please contact EAGLE admin to help with further investigation.");
console.error("Error: " + errorJSON.error);
return errorJSON.error;

Choose a reason for hiding this comment

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

issue (bug_risk): Avoid assuming a JSON-encoded error and returning a string from a Promise<void> function.

Inside push(), the catch assumes error is JSON text (JSON.parse(error)), but Eagle.saveFilesToRemote rejects with the raw error object, so this will likely throw inside the handler. It also returns errorJSON.error (a string) from a Promise<void>, which is inconsistent with the signature. Instead, handle error as an unknown object (e.g., log / stringify it) and either rethrow or return void after showing the message—no return value is needed here.

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.

2 participants