Conversation
…llname package. Added author field to schema3.
📝 WalkthroughWalkthroughAdds author tracking: a new public Changes
Sequence Diagram(s)sequenceDiagram
participant ElectronApp as Electron App (renderer/main)
participant CommandsMgr as CommandsManager
participant Fullname as fullname package
participant MinskyBackend as minsky backend
ElectronApp->>CommandsMgr: onReady (no file)
CommandsMgr->>Fullname: await fullname()
Fullname-->>CommandsMgr: full name string
CommandsMgr->>MinskyBackend: minsky.author(full name)
CommandsMgr->>MinskyBackend: createNewSystem / reset view
sequenceDiagram
participant AppMenu as Application Menu
participant PopupWindow as Headless Popup (Author modal)
participant AuthorComp as AuthorComponent
participant MinskyBackend as minsky backend
participant WindowUtil as WindowUtilityService
AppMenu->>PopupWindow: open /headless/menu/edit/author?author=...
PopupWindow->>AuthorComp: route with author param
AuthorComp->>MinskyBackend: minsky.author(editedAuthor) (if Electron)
AuthorComp->>WindowUtil: close window (if non-main)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request implements feature #1784 to add author tracking to Minsky models. The feature allows users to associate an author name with each model, which is stored in the model file and automatically populated with the system user's full name when creating a new model.
Changes:
- Added
authorfield to C++ model and schema3 classes to store model author information - Integrated
fullnamenpm package to automatically detect the system user's full name - Added UI menu item and routing for an Author dialog component (though the component implementation is missing from this PR)
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/schema3.h | Added author field declaration to schema3::Minsky class and initialization in constructor from minsky::Minsky |
| schema/schema3.cc | Added author field assignment when populating minsky::Minsky from schema3 |
| model/minsky.h | Declared author field as std::string member variable with documentation |
| model/minsky.cc | Initialize author to empty string in clearAllMaps() |
| gui-js/package.json | Added fullname dependency and fixed formatting |
| gui-js/package-lock.json | Updated lock file with fullname package and its dependencies |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Added author() method to TypeScript API wrapper |
| gui-js/libs/menu/src/lib/edit/edit.module.ts | Added AuthorComponent to module imports |
| gui-js/libs/menu/src/lib/edit/edit-routing.module.ts | Added author route configuration |
| gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | Set author to fullname() when creating new system |
| gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts | Added "Author" menu item in Edit menu |
| gui-js/apps/minsky-electron/src/app/app.ts | Call createNewSystem() when starting without command-line file argument |
Files not reviewed (1)
- gui-js/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { NgModule } from '@angular/core'; | ||
| import { RouterModule, Routes } from '@angular/router'; | ||
| import { DimensionsComponent } from './dimensions/dimensions.component'; | ||
| import {AuthorComponent } from './author/author'; |
There was a problem hiding this comment.
There is an inconsistent space after the opening brace in this import statement. The import should be formatted as "import { AuthorComponent }" (with a space after the opening brace) to match the formatting of other imports in the file (e.g., line 1, line 2). This maintains consistency with the formatting conventions throughout the codebase.
| import {AuthorComponent } from './author/author'; | |
| import { AuthorComponent } from './author/author'; |
| @@ -1,11 +1,12 @@ | |||
| import { CommonModule } from '@angular/common'; | |||
| import { NgModule } from '@angular/core'; | |||
| import {AuthorComponent } from './author/author'; | |||
There was a problem hiding this comment.
There is an inconsistent space after the opening brace in this import statement. The import should be formatted as "import { AuthorComponent }" (with a space after the opening brace) to match the formatting of other imports in this file and throughout the codebase.
| import {AuthorComponent } from './author/author'; | |
| import { AuthorComponent } from './author/author'; |
| @@ -1,4 +1,4 @@ | |||
| import { | |||
| import { | |||
There was a problem hiding this comment.
There is an extra space after "import" in this line. The import should be "import {" instead of "import {" to maintain consistency with the standard formatting used throughout the codebase.
| import { | |
| import { |
| // Macs sometimes put extra guff on the command line, so ignore errors | ||
| } | ||
| } else { // start an empty model | ||
| CommandsManager.createNewSystem(); |
There was a problem hiding this comment.
The call to CommandsManager.createNewSystem() is not awaited, but this is an async function that returns a Promise. Since this function can return false if the user cancels, and it performs asynchronous operations (including calling fullname()), the await keyword should be added here to ensure the operation completes properly and to handle any potential errors.
| CommandsManager.createNewSystem(); | |
| await CommandsManager.createNewSystem(); |
| minsky.clearAllMaps(); | ||
| minsky.pushFlags(); | ||
| minsky.clearHistory(); | ||
| minsky.author(await fullname()); |
There was a problem hiding this comment.
The fullname() function call may fail or return an empty string if it cannot determine the user's full name (e.g., on systems where the user information is not available or accessible). Consider adding error handling here to gracefully handle cases where fullname() fails or returns an empty/undefined value. For example, wrapping it in a try-catch and providing a default value like an empty string on failure.
| minsky.author(await fullname()); | |
| let authorName = ''; | |
| try { | |
| const fullName = await fullname(); | |
| if (typeof fullName === 'string' && fullName.trim().length > 0) { | |
| authorName = fullName; | |
| } | |
| } catch { | |
| // If fullname() fails, fall back to an empty author name | |
| authorName = ''; | |
| } | |
| minsky.author(authorName); |
| import { NgModule } from '@angular/core'; | ||
| import { RouterModule, Routes } from '@angular/router'; | ||
| import { DimensionsComponent } from './dimensions/dimensions.component'; | ||
| import {AuthorComponent } from './author/author'; |
There was a problem hiding this comment.
The AuthorComponent is being imported and referenced in both the edit.module.ts and edit-routing.module.ts files, but the component file itself (./author/author.ts or ./author/author.component.ts) is not included in this pull request. This will cause a build failure as the import cannot be resolved. The AuthorComponent implementation needs to be added to gui-js/libs/menu/src/lib/edit/author/.
| import {AuthorComponent } from './author/author'; | |
| import {AuthorComponent } from './author/author.component'; |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@gui-js/apps/minsky-electron/src/app/app.ts`:
- Around line 78-80: The call to CommandsManager.createNewSystem() is missing an
await so its Promise<boolean> can reject without being propagated through the
surrounding async onReady flow; update the branch that starts an empty model to
await CommandsManager.createNewSystem() (i.e., change the invocation in the else
branch to await CommandsManager.createNewSystem()) so any rejection is handled
by the async caller’s control flow.
In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts`:
- Line 20: The call to minsky.author() and fullname() must be awaited and
protected so rejections don't become unhandled promises; wrap both calls in a
try/catch, await minsky.author() and await fullname(), and on any error set the
author/name fallback to an empty string before proceeding with system creation
(references: minsky.author() and fullname()).
In `@gui-js/libs/menu/src/lib/edit/edit-routing.module.ts`:
- Around line 4-9: The build will fail because AuthorComponent is imported but
not implemented; either add a concrete AuthorComponent (exported class
AuthorComponent, with a selector/template and declared in the EditModule) or
remove the author route/imports. Specifically, implement and export
AuthorComponent (matching the import used in edit-routing.module.ts and
referenced by the { path: 'author', component: AuthorComponent } route) and
declare it in the EditModule, or delete the import and the 'author' route entry
in the routes array (and remove the same import from edit.module.ts) so the
project compiles.
🧹 Nitpick comments (1)
gui-js/package.json (1)
3-3: Update@nx/workspaceto align with other Nx package versions.Nx packages should be kept at the same version to avoid tooling mismatches. Your repository currently has
@nx/workspaceat^21.0.0while@nx/jest,@nx/js, andnxare at21.2.1. Align@nx/workspaceto^21.2.1usingnx migrate nx@21.2.1.The
fullname@5.0.0package (requires Node ≥18) is fully compatible with your Node.js 20.11.1+ requirement and requires no changes.Proposed change
- "@nx/workspace": "^21.0.0", + "@nx/workspace": "^21.2.1",
| } else { // start an empty model | ||
| CommandsManager.createNewSystem(); | ||
| } |
There was a problem hiding this comment.
Missing await on async createNewSystem() call.
CommandsManager.createNewSystem() returns a Promise<boolean> and is awaited elsewhere (e.g., line 134). Without await, any rejection becomes an unhandled promise rejection instead of propagating through the onReady async flow.
Proposed fix
} else { // start an empty model
- CommandsManager.createNewSystem();
+ await CommandsManager.createNewSystem();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { // start an empty model | |
| CommandsManager.createNewSystem(); | |
| } | |
| } else { // start an empty model | |
| await CommandsManager.createNewSystem(); | |
| } |
🤖 Prompt for AI Agents
In `@gui-js/apps/minsky-electron/src/app/app.ts` around lines 78 - 80, The call to
CommandsManager.createNewSystem() is missing an await so its Promise<boolean>
can reject without being propagated through the surrounding async onReady flow;
update the branch that starts an empty model to await
CommandsManager.createNewSystem() (i.e., change the invocation in the else
branch to await CommandsManager.createNewSystem()) so any rejection is handled
by the async caller’s control flow.
There was a problem hiding this comment.
For my edification, what is the issue with an unhandled promise rejection?
There was a problem hiding this comment.
@highperformancecoder Great question! Here are the key issues with unhandled promise rejections:
1. Error Visibility & Debugging
When a promise rejects without being caught (via await, .catch(), or try/catch), the error doesn't propagate through your normal error-handling flow. In this case, if createNewSystem() fails, the error won't be caught by any surrounding try/catch blocks in onReady(), making it harder to diagnose why the system didn't initialize properly.
2. Node.js Runtime Behavior
- Node.js emits an
unhandledRejectionevent and logs a warning to the console - In newer Node.js versions (15+), unhandled promise rejections terminate the process by default (crash the app)
- Even in older versions, this behavior was planned and warnings were issued
3. Silent Failures
Without await, your code continues executing immediately rather than waiting for the operation to complete. So:
- The empty model might not be fully initialized before subsequent code runs
- Users could see an inconsistent UI state
- Race conditions could occur if other code depends on the system being ready
4. In Your Code Specifically
You do have an uncaughtException handler (line 265), but that only catches synchronous errors. Unhandled promise rejections are in a separate category and would bypass that handler unless you also add an unhandledRejection handler.
Bottom line: Using await ensures errors are caught by your async control flow, the operation completes before continuing, and failures are visible/debuggable rather than silent or causing unexpected crashes.
| import JSON5 from 'json5'; | ||
| import { extname, join, dirname } from 'path'; | ||
| import { tmpdir } from 'os'; | ||
| import fullname from 'fullname'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CommandsManager.ts" -type fRepository: highperformancecoder/minsky
Length of output: 136
🏁 Script executed:
fd -t f "CommandsManager.ts"Repository: highperformancecoder/minsky
Length of output: 134
🏁 Script executed:
# Check if fullname is in package.json
rg -t json '"fullname"' --max-count 5Repository: highperformancecoder/minsky
Length of output: 171
🏁 Script executed:
# Search for fullname() calls in the codebase
rg 'fullname\(\)' --max-count 20Repository: highperformancecoder/minsky
Length of output: 171
🏁 Script executed:
wc -l ./gui-js/apps/minsky-electron/src/app/managers/CommandsManager.tsRepository: highperformancecoder/minsky
Length of output: 141
🏁 Script executed:
cat -n ./gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | head -50Repository: highperformancecoder/minsky
Length of output: 2131
🏁 Script executed:
cat -n ./gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | sed -n '470,485p'Repository: highperformancecoder/minsky
Length of output: 796
🌐 Web query:
fullname npm package v5.0.0 API error handling unsupported platforms
💡 Result:
fullname (npm) v5.0.0 — API
- What it does: gets the current OS user’s “full name”. Supported OSes: macOS, Linux, Windows. [1]
- How to call it (ESM):
[1]
import fullName from "fullname"; const name = await fullName(); // string
Error handling (recommended)
fullName() is async, so treat failures as Promise rejections and wrap with try/catch:
import fullName from "fullname";
import username from "username";
let name;
try {
name = await fullName();
} catch (error) {
// Fallback recommended by the author when a name can’t be found
name = await username();
}The README explicitly suggests falling back to username “in the rare case a name cannot be found”. [1]
“Unsupported platform” behavior
If you run on an OS outside macOS/Linux/Windows, you should expect it to fail (commonly surfaced as an “unsupported platform” error) because those are the only platforms the package claims to work on. [1]
Sources: [1] (npm package README snippet), [2] (Skypack page repeating “Tested on macOS, Linux, and Windows”). [1][2]
🏁 Script executed:
# Search for minsky.author definition and its signature
rg -A 3 'author\s*\(' gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | head -20Repository: highperformancecoder/minsky
Length of output: 189
🏁 Script executed:
# Check if minsky.author() is async by searching in shared module
fd -t f "minsky" gui-js/ --type f | xargs rg -l "author" 2>/dev/null | head -5Repository: highperformancecoder/minsky
Length of output: 115
🏁 Script executed:
# Look for the minsky object definition/type
rg 'from.*@minsky/shared' gui-js/apps/minsky-electron/src/app/managers/CommandsManager.tsRepository: highperformancecoder/minsky
Length of output: 95
🏁 Script executed:
# Check if there are other calls to minsky.author to understand expected usage
rg 'minsky\.author' gui-js/ --max-count 10Repository: highperformancecoder/minsky
Length of output: 308
🏁 Script executed:
# Read the minsky.ts file to see author() definition
wc -l gui-js/libs/shared/src/lib/backend/minsky.tsRepository: highperformancecoder/minsky
Length of output: 120
🏁 Script executed:
# Search for author method in minsky.ts
rg -B 2 -A 5 'author\s*\(' gui-js/libs/shared/src/lib/backend/minsky.tsRepository: highperformancecoder/minsky
Length of output: 852
🏁 Script executed:
# Read ApplicationMenuManager line with author to understand context
rg -B 5 -A 5 'let author=encodeURIComponent' gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.tsRepository: highperformancecoder/minsky
Length of output: 476
Await and handle fullname() rejection to prevent unhandled promise in system creation.
minsky.author() is async (returns Promise<string>) but is not awaited at line 479, leaving any fullname() rejection unhandled. The fullname package (v5.0.0) rejects on unsupported platforms rather than returning an empty string. Add try-catch wrapping both calls and fall back to empty string to ensure system creation completes.
Line 479:
- minsky.author(await fullname());
+ try {
+ const fullName = await fullname();
+ await minsky.author(fullName ?? '');
+ } catch (err) {
+ await minsky.author('');
+ }🤖 Prompt for AI Agents
In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` at line 20,
The call to minsky.author() and fullname() must be awaited and protected so
rejections don't become unhandled promises; wrap both calls in a try/catch,
await minsky.author() and await fullname(), and on any error set the author/name
fallback to an empty string before proceeding with system creation (references:
minsky.author() and fullname()).
| @@ -0,0 +1,42 @@ | |||
| import { Component, OnDestroy, OnInit } from '@angular/core'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 15 hours ago
To fix the issue, remove the unused OnInit symbol from the import statement. This avoids confusion and keeps the file free of unused imports.
Concretely, in gui-js/libs/menu/src/lib/edit/author/author.ts, edit the import on line 1 to only import Component and OnDestroy. No other code changes are needed because the class does not implement OnInit and does not define ngOnInit. No new methods, imports, or definitions are required.
| @@ -1,4 +1,4 @@ | ||
| import { Component, OnDestroy, OnInit } from '@angular/core'; | ||
| import { Component, OnDestroy } from '@angular/core'; | ||
| import { ActivatedRoute } from '@angular/router'; | ||
| import { ElectronService, WindowUtilityService } from '@minsky/core'; | ||
| import { GodleyIcon} from '@minsky/shared'; |
| import { Component, OnDestroy, OnInit } from '@angular/core'; | ||
| import { ActivatedRoute } from '@angular/router'; | ||
| import { ElectronService, WindowUtilityService } from '@minsky/core'; | ||
| import { GodleyIcon} from '@minsky/shared'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 15 hours ago
In general, to fix an unused import, you either remove it or start using it meaningfully. Since GodleyIcon is not referenced anywhere in AuthorComponent, and there is no indication that it should be used, the safest and simplest fix is to remove the import statement for GodleyIcon.
Concretely, in gui-js/libs/menu/src/lib/edit/author/author.ts, delete line 4 (import { GodleyIcon} from '@minsky/shared';). No other code changes are required because nothing in this file refers to GodleyIcon, and the Angular component metadata also does not depend on it. No new imports, methods, or definitions are needed.
| @@ -1,7 +1,6 @@ | ||
| import { Component, OnDestroy, OnInit } from '@angular/core'; | ||
| import { ActivatedRoute } from '@angular/router'; | ||
| import { ElectronService, WindowUtilityService } from '@minsky/core'; | ||
| import { GodleyIcon} from '@minsky/shared'; | ||
| import { Subject, takeUntil } from 'rxjs'; | ||
| import { InputModalComponent} from '@minsky/ui-components'; | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@gui-js/libs/menu/src/lib/edit/author/author.ts`:
- Around line 1-6: Remove the unused imports OnInit and GodleyIcon: in the
import statement that currently reads "import { Component, OnDestroy, OnInit }
from '@angular/core';" drop OnInit, and remove the entire import "import {
GodleyIcon } from '@minsky/shared';" so the file only imports symbols actually
used by the Author component/class (which implements OnDestroy). This keeps the
class signature (implements OnDestroy) unchanged and eliminates unused imports.
- Around line 31-36: handleEditAuthor currently calls
electronService.minsky.author(author) without awaiting its Promise, allowing
windowUtilityService.closeCurrentWindowIfNotMain() to run before the backend
persists the author; change the call to await
electronService.minsky.author(author) and only call
windowUtilityService.closeCurrentWindowIfNotMain() after the awaited call
completes, and add a try/catch around the await to handle/log any errors from
minsky.author(author) so failures don't silently drop the update.
🧹 Nitpick comments (1)
gui-js/libs/menu/src/lib/edit/author/author.ts (1)
16-19: Consider initializingauthorand usingSubject<void>for the destroy signal.Two small improvements:
author(line 17) is uninitialized and will beundefineduntil the first query-param emission, which means the template receivesundefinedas the initial input binding. Defaulting to''is safer.Subject<{}>(line 19) is unconventional for a destroy notifier —Subject<void>is the idiomatic choice, paired withthis.destroy$.next()inngOnDestroy.♻️ Proposed tweak
export class AuthorComponent implements OnDestroy { - author: string; + author = ''; - destroy$ = new Subject<{}>(); + destroy$ = new Subject<void>();ngOnDestroy() { - this.destroy$.next(undefined); + this.destroy$.next(); this.destroy$.complete(); }
| import { Component, OnDestroy, OnInit } from '@angular/core'; | ||
| import { ActivatedRoute } from '@angular/router'; | ||
| import { ElectronService, WindowUtilityService } from '@minsky/core'; | ||
| import { GodleyIcon} from '@minsky/shared'; | ||
| import { Subject, takeUntil } from 'rxjs'; | ||
| import { InputModalComponent} from '@minsky/ui-components'; |
There was a problem hiding this comment.
Remove unused imports OnInit and GodleyIcon.
OnInit (line 1) and GodleyIcon (line 4) are imported but never used. The class implements OnDestroy, not OnInit.
🧹 Proposed fix
-import { Component, OnDestroy, OnInit } from '@angular/core';
+import { Component, OnDestroy } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { ElectronService, WindowUtilityService } from '@minsky/core';
-import { GodleyIcon} from '@minsky/shared';
import { Subject, takeUntil } from 'rxjs';
import { InputModalComponent} from '@minsky/ui-components';🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused import OnInit.
[notice] 4-4: Unused variable, import, function or class
Unused import GodleyIcon.
🤖 Prompt for AI Agents
In `@gui-js/libs/menu/src/lib/edit/author/author.ts` around lines 1 - 6, Remove
the unused imports OnInit and GodleyIcon: in the import statement that currently
reads "import { Component, OnDestroy, OnInit } from '@angular/core';" drop
OnInit, and remove the entire import "import { GodleyIcon } from
'@minsky/shared';" so the file only imports symbols actually used by the Author
component/class (which implements OnDestroy). This keeps the class signature
(implements OnDestroy) unchanged and eliminates unused imports.
| async handleEditAuthor(author: string) { | ||
| if (this.electronService.isElectron) { | ||
| this.electronService.minsky.author(author); | ||
| } | ||
| this.windowUtilityService.closeCurrentWindowIfNotMain(); | ||
| } |
There was a problem hiding this comment.
Missing await on minsky.author() — window may close before the author is persisted.
minsky.author(author) returns Promise<string> (see minsky.ts:1306), but the result is not awaited. Since closeCurrentWindowIfNotMain() on line 35 executes immediately after, the window can close before the backend call completes, silently dropping the update.
🐛 Proposed fix
async handleEditAuthor(author: string) {
if (this.electronService.isElectron) {
- this.electronService.minsky.author(author);
+ await this.electronService.minsky.author(author);
}
this.windowUtilityService.closeCurrentWindowIfNotMain();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async handleEditAuthor(author: string) { | |
| if (this.electronService.isElectron) { | |
| this.electronService.minsky.author(author); | |
| } | |
| this.windowUtilityService.closeCurrentWindowIfNotMain(); | |
| } | |
| async handleEditAuthor(author: string) { | |
| if (this.electronService.isElectron) { | |
| await this.electronService.minsky.author(author); | |
| } | |
| this.windowUtilityService.closeCurrentWindowIfNotMain(); | |
| } |
🤖 Prompt for AI Agents
In `@gui-js/libs/menu/src/lib/edit/author/author.ts` around lines 31 - 36,
handleEditAuthor currently calls electronService.minsky.author(author) without
awaiting its Promise, allowing
windowUtilityService.closeCurrentWindowIfNotMain() to run before the backend
persists the author; change the call to await
electronService.minsky.author(author) and only call
windowUtilityService.closeCurrentWindowIfNotMain() after the awaited call
completes, and add a try/catch around the await to handle/log any errors from
minsky.author(author) so failures don't silently drop the update.
This change is
Summary by CodeRabbit