Skip to content

Feature 1784 author#605

Open
highperformancecoder wants to merge 3 commits intomasterfrom
feature-1784-author
Open

Feature 1784 author#605
highperformancecoder wants to merge 3 commits intomasterfrom
feature-1784-author

Conversation

@highperformancecoder
Copy link
Owner

@highperformancecoder highperformancecoder commented Feb 6, 2026

This change is Reviewable

Summary by CodeRabbit

  • New Features
    • Added "Author" option to the Edit menu for managing author information.
    • New documents automatically capture the user's full name when created.
    • Author information is persisted with saved models.
    • Opening the app without a file now initializes a new empty document by default.

Copilot AI review requested due to automatic review settings February 6, 2026 05:54
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

📝 Walkthrough

Walkthrough

Adds author tracking: a new public author field in the core Minsky model, propagated through schema and initialization. The Electron app initializes new models with the system fullname and exposes an "Author" Edit menu item and UI to view/edit the author via a routed modal.

Changes

Cohort / File(s) Summary
Core Model
model/minsky.h, model/minsky.cc
Added public std::string author to Minsky and initialize it in clearAllMaps.
Schema Persistence
schema/schema3.h, schema/schema3.cc
Added author field to schema3::Minsky and copy/propagate it from runtime Minsky during populate/copy.
App startup & Commands
gui-js/apps/minsky-electron/src/app/app.ts, gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
When no file is opened on ready, create a new system and set its author using fullname() then minsky.author(...) before resetting view.
Application Menu
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts
Added "Author" item to Edit menu; click opens headless routing popup /headless/menu/edit/author with encoded author query param.
Edit UI routing & module
gui-js/libs/menu/src/lib/edit/edit-routing.module.ts, gui-js/libs/menu/src/lib/edit/edit.module.ts
Added /author route and registered AuthorComponent in the edit module alongside existing components.
Author component & template
gui-js/libs/menu/src/lib/edit/author/author.ts, gui-js/libs/menu/src/lib/edit/author/author.html
New AuthorComponent that reads query param author, provides handleEditAuthor to call minsky.author(author) in Electron and closes non-main windows; template uses minsky-input-modal bound to author.
Backend wrapper
gui-js/libs/shared/src/lib/backend/minsky.ts
Added author(...args: string[]): Promise<string> method to Minsky wrapper to forward author calls to the native backend.
Package deps
gui-js/package.json
Added fullname ^5.0.0 (also into devDependencies) and moved nx / @nx/workspace from dependencies to devDependencies.

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
Loading
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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped in code with whiskers bright,

Gave models names by morning light.
From fullname's call to menu's tune,
The author's tale now hums in tune.
A rabbit's hop — a tiny write. 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Feature 1784 author' is vague and lacks specificity; it references an issue number but provides no meaningful information about what the feature does or what functionality is being added. Consider a more descriptive title that explains the feature, such as 'Add author field to save/load author information' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature-1784-author

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 author field to C++ model and schema3 classes to store model author information
  • Integrated fullname npm 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';
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
import {AuthorComponent } from './author/author';
import { AuthorComponent } from './author/author';

Copilot uses AI. Check for mistakes.
@@ -1,11 +1,12 @@
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import {AuthorComponent } from './author/author';
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
import {AuthorComponent } from './author/author';
import { AuthorComponent } from './author/author';

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
import {
import {
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
import {
import {

Copilot uses AI. Check for mistakes.
// Macs sometimes put extra guff on the command line, so ignore errors
}
} else { // start an empty model
CommandsManager.createNewSystem();
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
CommandsManager.createNewSystem();
await CommandsManager.createNewSystem();

Copilot uses AI. Check for mistakes.
minsky.clearAllMaps();
minsky.pushFlags();
minsky.clearHistory();
minsky.author(await fullname());
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
import { NgModule } from '@angular/core';
import { RouterModule, Routes } from '@angular/router';
import { DimensionsComponent } from './dimensions/dimensions.component';
import {AuthorComponent } from './author/author';
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

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/.

Suggested change
import {AuthorComponent } from './author/author';
import {AuthorComponent } from './author/author.component';

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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/workspace to align with other Nx package versions.

Nx packages should be kept at the same version to avoid tooling mismatches. Your repository currently has @nx/workspace at ^21.0.0 while @nx/jest, @nx/js, and nx are at 21.2.1. Align @nx/workspace to ^21.2.1 using nx migrate nx@21.2.1.

The fullname@5.0.0 package (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",

Comment on lines +78 to 80
} else { // start an empty model
CommandsManager.createNewSystem();
}
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
} 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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

For my edification, what is the issue with an unhandled promise rejection?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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 unhandledRejection event 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "CommandsManager.ts" -type f

Repository: 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 5

Repository: highperformancecoder/minsky

Length of output: 171


🏁 Script executed:

# Search for fullname() calls in the codebase
rg 'fullname\(\)' --max-count 20

Repository: highperformancecoder/minsky

Length of output: 171


🏁 Script executed:

wc -l ./gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts

Repository: highperformancecoder/minsky

Length of output: 141


🏁 Script executed:

cat -n ./gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | head -50

Repository: 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):
    import fullName from "fullname";
    
    const name = await fullName(); // string
    [1]

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 -20

Repository: 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 -5

Repository: 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.ts

Repository: 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 10

Repository: 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.ts

Repository: 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.ts

Repository: 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.ts

Repository: 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

Unused import OnInit.

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.

Suggested changeset 1
gui-js/libs/menu/src/lib/edit/author/author.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/gui-js/libs/menu/src/lib/edit/author/author.ts b/gui-js/libs/menu/src/lib/edit/author/author.ts
--- a/gui-js/libs/menu/src/lib/edit/author/author.ts
+++ b/gui-js/libs/menu/src/lib/edit/author/author.ts
@@ -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';
EOF
@@ -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';
Copilot is powered by AI and may make mistakes. Always verify output.
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

Unused import GodleyIcon.

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.

Suggested changeset 1
gui-js/libs/menu/src/lib/edit/author/author.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/gui-js/libs/menu/src/lib/edit/author/author.ts b/gui-js/libs/menu/src/lib/edit/author/author.ts
--- a/gui-js/libs/menu/src/lib/edit/author/author.ts
+++ b/gui-js/libs/menu/src/lib/edit/author/author.ts
@@ -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';
 
EOF
@@ -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';

Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 initializing author and using Subject<void> for the destroy signal.

Two small improvements:

  1. author (line 17) is uninitialized and will be undefined until the first query-param emission, which means the template receives undefined as the initial input binding. Defaulting to '' is safer.
  2. Subject<{}> (line 19) is unconventional for a destroy notifier — Subject<void> is the idiomatic choice, paired with this.destroy$.next() in ngOnDestroy.
♻️ 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();
   }

Comment on lines +1 to +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';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +31 to +36
async handleEditAuthor(author: string) {
if (this.electronService.isElectron) {
this.electronService.minsky.author(author);
}
this.windowUtilityService.closeCurrentWindowIfNotMain();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

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.

1 participant