EAGLE-1556: Script to update old graphs#1002
EAGLE-1556: Script to update old graphs#1002james-strauss-uwa wants to merge 19 commits intomasterfrom
Conversation
…read them, write them as V4 JSON
…r in parsing a single graph
…at using keys instead of ids
…rom EAGLE client.
…rchy. Code cleanup.
Reviewer's GuideAdd 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 typesclassDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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
dataeven though it will be undefined when the error occurs; use the caughterror(and/or the HTTP response body from Utils.httpPostJSONString) when constructing the message to avoid logging and showingundefined.
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const srcRepoIndex = parseInt($('#graphUpdaterModalSourceRepositorySelect').val() as string); | ||
| const srcRepo = Repositories.repositories()[srcRepoIndex]; | ||
| if (srcRepo === null){ |
There was a problem hiding this comment.
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.
| const destRepoIndex = parseInt($('#graphUpdaterModalDestinationRepositorySelect').val() as string); | ||
| const destRepo = Repositories.repositories()[destRepoIndex]; | ||
| if (destRepo === null){ |
There was a problem hiding this comment.
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 { | |||
| }); | |||
There was a problem hiding this comment.
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:
- Remove the closing
});that matches thenew Promise(call at the end ofsaveFilesToRemote. - Replace any
resolve()calls withreturn;(or just allow the function to complete if no value is needed). - Replace any
reject(error)(orreject(someError)) calls withthrow error;so that theasyncfunction rejects its promise naturally. - If there's a
try/catchthat only exists to callreject, you can either remove it (and let errors bubble) or keep it andthrowfrom thecatchinstead of callingreject.
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
GraphUpdater.push, the function is declared as returningPromise<void>but in the error path itreturnserrorJSON.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.pushassumeserroris a JSON string (JSON.parse(error)), butsaveFilesToRemoterejects 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 avoidingJSON.parseon non-strings. - In
GraphUpdater.fetchLogicalGraphsandGraphUpdater.push, the selected repository index is taken viaparseInt(...val() as string)with no validation; it would be safer to guard againstNaN/out-of-range indices before indexing intoRepositories.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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
| } 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; |
There was a problem hiding this comment.
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.
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:
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:
Enhancements:
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:
Enhancements: