Conversation
…llname package. Added author field to schema3.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a public author field across C++ model and schema, exposes a new backend author(...) API, initializes author on new-system creation using fullname(), adds an Edit → Author menu item and modal, and wires an AuthorComponent and Electron plumbing to edit/persist the author. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant CmdMgr as CommandsManager
participant Fullname as fullname pkg
participant Minsky as minsky backend
App->>CmdMgr: onReady (no file)
CmdMgr->>Fullname: await fullname()
Fullname-->>CmdMgr: full name
CmdMgr->>Minsky: minsky.author(full name)
CmdMgr->>Minsky: createNewSystem() / initialize model
sequenceDiagram
participant Menu as Application Menu
participant Popup as Headless Popup Window
participant AuthorComp as AuthorComponent
participant Minsky as minsky backend
participant WinUtil as WindowUtilityService
Menu->>Popup: open /headless/menu/edit/author?author=...
Popup->>AuthorComp: route with author param
AuthorComp->>Minsky: minsky.author(editedAuthor) (if Electron)
AuthorComp->>WinUtil: close window (if non-main)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
Outdated
Show resolved
Hide resolved
| 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'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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",
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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(); }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/1Free.mky (1)
860-923:⚠️ Potential issue | 🟡 MinorNormalize space character references from
 to the conventional form.Both encodings are valid XML, but
 with leading zeros is non-standard. Current files consistently use (~210 occurrences) while older schema files and the vast majority of space references (~1143) use the conventional . If this was an unintended side-effect of the recent XML validation fix, normalizing to would improve consistency with conventions and prevent potential issues with downstream tooling.test/Makefile (1)
20-28:⚠️ Potential issue | 🟡 MinorMove
-lxcb -lX11to the non-MXE block to avoid linking X11 libraries in Windows cross-builds and eliminate redundancy.
-lxcb -lX11are added unconditionally on line 21, but-lxcbis also added again on line 27 in the non-MXEelseblock. This creates two issues:
- MXE (Windows) builds now link X11 libraries, which don't exist in the Windows build environment.
- Non-MXE builds link
-lxcbtwice, which is redundant.Move
-lxcb -lX11from the unconditional LIBS assignment into theelseblock (non-MXE only), and remove the redundantLIBS+=-lxcbon line 27.Suggested fix
LIBS+=-L../RavelCAPI -lravelCAPI -L../RavelCAPI/civita -lcivita \ -lboost_filesystem -lboost_system -lboost_thread \ - -lboost_date_time -lclipboard -lxcb -lX11 \ + -lboost_date_time -lclipboard \ -lgsl -lssl -lcrypto -lgslcblas -lxml2 -ltiff -ldl ifdef MXE else -LIBS+=-lxcb +LIBS+=-lxcb -lX11 endif
🧹 Nitpick comments (6)
examples/indexing.mky (1)
11-14: Minor coordinate drift from re-saving — consider whether example file churn is intentional.The floating-point coordinate tweaks across Wires 39, 48, and 50 appear to be artifacts of re-saving the file with the updated application version rather than deliberate edits. While harmless, incidental diffs in example/data files can create noise in version history. If these coordinate changes aren't intentional, consider reverting them to keep the diff focused on the
author/version changes only.Also applies to: 64-67, 82-85
examples/fundamentalConstants.mky (1)
751-751: Minor coordinate drift — likely a re-save artifact.The
xvalue shifted by 2 units (320.246 → 318.246). If this wasn't intentional, it may be worth reverting to keep the example file unchanged beyond the schema additions.examples/GoodwinLinear.mky (1)
84-172: Coordinate changes appear to be layout drift from re-serialization.These float adjustments across multiple wires (102, 106, 108, 109, 110) look like they result from re-saving the example in the updated application version rather than intentional layout edits. If this drift is unintentional, consider whether the serialization logic should round-trip coordinates more deterministically to reduce noise in future diffs of
.mkyfiles.examples/Solow.mky (1)
406-406: uses a leading zero — consider or instead.The numeric character reference
 is valid XML, but the leading zero is unconventional. Standard practice is (decimal) or (hex). If this encoding is emitted by the serializer automatically, it's fine — just noting the inconsistency with common XML conventions. Not a correctness issue.examples/LoanableFunds.mky (1)
424-424: Text encoding normalization uses for spaces — verify parser compatibility.The serializer now encodes ASCII spaces as
 (e.g.,Lending and repayment rates). While numeric character references with leading zeros are valid per the XML spec, some parsers or tooling may not expect this form. The more conventional forms are or . If this encoding is intentional for XML validation purposes (as the commit message suggests), this is fine — just ensure all downstream consumers (including third-party tools that might load.mkyfiles) handle it correctly.Also applies to: 5151-5151, 5928-5928, 6187-6187, 6446-6446, 6744-6744
gui-js/libs/menu/src/lib/edit/author/author.ts (1)
25-27:params.authormay beundefinedwhen the query param is absent.If the route is navigated to without an
authorquery parameter,params.authorevaluates toundefined, overwriting the''default. This could surface as the string"undefined"if it later reaches the template or the backend call.🛡️ Suggested defensive default
this.route.queryParams.pipe(takeUntil(this.destroy$)).subscribe((params) => { - this.author=params.author; + this.author = params.author ?? ''; });
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/binaryInterpolation.mky`:
- Around line 812-818: The serialized <slider> blocks for integral variables
(e.g., int1 and int2) contain sentinel/uninitialized values (min ==
1.79769e+308, max == -1.79769e+308, step == 0); update the slider serialization
to either omit the <slider> element when those sentinel defaults are present or
replace them with sensible integral defaults (e.g., set min/max to the type's
bounds and step to 1) before writing; specifically change the code that emits
the <slider> block (detect the sentinel triple: min == DBL_MAX, max == -DBL_MAX,
step == 0) and implement the conditional skip or normalization so example files
no longer contain inverted ranges or zero step for integral variables like
int1/int2.
In `@examples/statistics.mky`:
- Around line 731-737: Slider blocks currently have inverted sentinel min/max
(1.79769e+308 and -1.79769e+308) with step 0 and are marked visible, causing
meaningless UI for integral variables; update the affected <slider> blocks (the
elements containing tags stepRel, visible, min, max, step) to either set
<visible>false</visible> so they don't render, or replace the sentinel values
with sensible integral slider defaults (e.g., <min>-1</min>, <max>1</max>,
<step>1</step>) depending on whether a real slider is appropriate, and ensure
<stepRel> remains correctly set.
🧹 Nitpick comments (2)
examples/BasicGrowthModel.mky (1)
377-377: Incidental positional change — is this intentional?The x-coordinate of item
id=36changed from891to875. This looks like a side-effect of re-saving the example file rather than a deliberate layout adjustment. If unintentional, consider reverting to avoid noise in the diff.examples/1Free.mky (1)
3-4: Version bump and empty author in example file look intentional but worth a quick check.The
minskyVersionjump to3.22.0and the new empty<author></author>element are consistent with re-saving this example under the latest application version with the new author schema field. Two minor points:
- The version appears to be a large jump from the previous value (per the AI summary, previously
3.4.0-beta.3). Confirm this matches the current application version and isn't a typo.- Consider whether shipped example files should have a meaningful author (e.g., the project or original creator) rather than an empty string, since empty author may look like missing metadata to end users.
| <slider> | ||
| <stepRel>false</stepRel> | ||
| <visible>true</visible> | ||
| <min>1.79769e+308</min> | ||
| <max>-1.79769e+308</max> | ||
| <step>0</step> | ||
| </slider> |
There was a problem hiding this comment.
Slider blocks for integral variables have inverted min/max sentinel values with visible: true.
All four new slider blocks (items 58, 64, 70, 76) serialize min as 1.79769e+308 (DBL_MAX) and max as -1.79769e+308 (−DBL_MAX), with step of 0. These appear to be uninitialized defaults. Based on learnings, the runtime normalizes these (e.g., substituting [-1, 1] when the value is zero, and handling zero step elsewhere), so this should work correctly at runtime.
That said, these sliders are marked <visible>true</visible> — if a user opens this example file, the sliders will render for these integral variables. You may want to confirm that the runtime normalization produces a usable slider experience for these items, or alternatively set <visible>false</visible> if sliders aren't meaningful for these integrals.
🤖 Prompt for AI Agents
In `@examples/statistics.mky` around lines 731 - 737, Slider blocks currently have
inverted sentinel min/max (1.79769e+308 and -1.79769e+308) with step 0 and are
marked visible, causing meaningless UI for integral variables; update the
affected <slider> blocks (the elements containing tags stepRel, visible, min,
max, step) to either set <visible>false</visible> so they don't render, or
replace the sentinel values with sensible integral slider defaults (e.g.,
<min>-1</min>, <max>1</max>, <step>1</step>) depending on whether a real slider
is appropriate, and ensure <stepRel> remains correctly set.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
necessary, as author should never reject.
…/minsky into feature-1784-author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gui-js/package.json`:
- Line 183: The package.json bump to "fullname" ^5.0.0 is problematic because v5
is ESM-only; update CommandsManager.ts to avoid a static CommonJS-style require
or import by replacing the static import with a dynamic import at runtime (e.g.,
use await import('fullname') inside the async initialization function or in the
method that needs it) or alternatively pin the dependency to fullname@4.x in
package.json and run install; modify wherever CommandsManager.ts references
fullname (search for the import line and usages in CommandsManager) to use the
dynamic import pattern if you choose the ESM v5 path so the Electron main
(CommonJS) process does not throw a "require() of ES Module not supported"
error.
🧹 Nitpick comments (1)
gui-js/package.json (1)
150-164: Inconsistent version pinning across Nx packages.
@nx/jestand@nx/jsare pinned to exact"21.2.1", whilenxand@nx/workspaceuse caret ranges"^21.2.1". Nx packages are designed to work together at identical versions — mixing pinning strategies can cause subtle incompatibilities if the caret-ranged packages resolve to a newer minor/patch.Consider aligning all
@nx/*andnxpackages to the same pinning strategy.
| "electron-log": "^5.4.1", | ||
| "electron-progressbar": "^2.2.1", | ||
| "electron-store": "^8.2.0", | ||
| "fullname": "^5.0.0", |
There was a problem hiding this comment.
fullname v5+ is ESM-only — verify it works in the Electron main process.
The fullname package switched to ESM-only in v5. Electron's main process typically runs in a CommonJS context, which means a standard import fullname from 'fullname' or require('fullname') may fail at runtime with an error like "require() of ES Module not supported".
Ensure the import in CommandsManager.ts uses a dynamic await import('fullname') pattern, or consider using fullname@4.x which still provides a CommonJS build.
#!/bin/bash
# Check how fullname is imported in CommandsManager.ts
fd "CommandsManager.ts" --exec cat {}
# Check if the project has "type": "module" in any package.json that would affect the electron app
fd "package.json" --max-depth 4 --exec grep -l '"type": "module"' {} 2>/dev/null
# Check tsconfig for module resolution settings in the electron app
fd "tsconfig" -e json --path-separator / | xargs grep -l "module" 2>/dev/null | head -5 | xargs -I{} sh -c 'echo "=== {} ===" && cat {}'Is fullname npm package v5 ESM only?
🤖 Prompt for AI Agents
In `@gui-js/package.json` at line 183, The package.json bump to "fullname" ^5.0.0
is problematic because v5 is ESM-only; update CommandsManager.ts to avoid a
static CommonJS-style require or import by replacing the static import with a
dynamic import at runtime (e.g., use await import('fullname') inside the async
initialization function or in the method that needs it) or alternatively pin the
dependency to fullname@4.x in package.json and run install; modify wherever
CommandsManager.ts references fullname (search for the import line and usages in
CommandsManager) to use the dynamic import pattern if you choose the ESM v5 path
so the Electron main (CommonJS) process does not throw a "require() of ES Module
not supported" error.
There was a problem hiding this comment.
fullname works fine as is, and it is integrated at version 5.
Under what circumstances will the current import statement fail then?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how fullname is imported in CommandsManager.ts
echo "=== CommandsManager.ts fullname import ==="
fd "CommandsManager.ts" --exec grep -n "fullname" {}
echo -e "\n=== Full CommandsManager.ts content around fullname usage ==="
fd "CommandsManager.ts" --exec cat {}
Length of output: 43041
🌐 Web query:
fullname npm package version 5 ESM CommonJS module format
💡 Result:
fullname@5.0.0 is ESM-only (the published usage is import fullName from 'fullname', not require(...)). [1]
How to use it
-
ESM (recommended):
import fullName from 'fullname'; console.log(await fullName());
[1]
-
From a CommonJS project (no
require()): use dynamic import:const {default: fullName} = await import('fullname'); console.log(await fullName());
This works because CommonJS cannot synchronously
require()an ESM-only package. [3]
If you need require()
Stay on an older major that still supports CommonJS (pre-ESM-only), or migrate your project to ESM (for example by using "type": "module" which makes Node treat .js files as ESM). [2][3]
Sources: npm package readme/usage [1], Node.js package/module-type docs [2], “Pure ESM package” guidance [3].
[1] (npmjs.com)
[2] (nodejs.org)
[3] (gist.github.com)
Citations:
- 1: https://www.npmjs.com/package/fullname?utm_source=openai
- 2: https://nodejs.org/download/release/v14.13.1/docs/api/packages.html?utm_source=openai
- 3: https://gist.github.com/Kuingsmile/ff74465c556b13b8ef4ccf888ed79f6b?utm_source=openai
@highperformancecoder You're absolutely right, and I apologize for the confusion.
You're using the correct ES6 import syntax (import fullname from 'fullname';), and your TypeScript build tooling is handling the ESM package properly. The web search confirms that fullname v5.0.0 is ESM-only, but that's exactly what you're importing.
Why it works for you:
- You're using
import(ES6 module syntax), notrequire()(CommonJS) - Your TypeScript/webpack build pipeline transpiles and bundles this correctly for Electron
- The async usage with
await fullname()is the proper pattern
When it would fail:
My concern would only apply if someone tried to use require('fullname') in a plain CommonJS file without proper build tooling. That would produce the "require() of ES Module not supported" error. But that's not what you're doing.
Your implementation is correct as-is. No changes needed.
1 participant
This change is
Summary by CodeRabbit
New Features
Chores