Feature librarian #558
Feature librarian #558
Conversation
DatabaseIngestor stuff moved into Ravel.
Merge branch 'master' into feature-librarian
Fix multiple progress bar deadlock
…SV from parameters.
…s CSV files imported to assist opening the file dialogs. For #1791.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces substantial changes across the codebase to support database-backed CSV import and integration with the Ravel system. It adds new components for database ingestion, updates CSV parsing to use shared utilities, and refactors UI and backend logic to centralize CSV import functionality. The interface for handling mouse events is unified to support reset signaling, and new Angular components are added for database connection and configuration. Numerous methods and properties are refactored or removed, especially around CSVDialog and VariableValue, to streamline data import, improve modularity, and support new database workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Electron/Angular)
participant WindowManager
participant CSVDialog/DatabaseIngestor
participant VariableValue
participant Database
participant Ravel
User->>UI (Electron/Angular): Initiate CSV Import (to parameter or database)
UI (Electron/Angular)->>WindowManager: Show OpenDialog/SaveDialog
WindowManager->>UI (Electron/Angular): Return selected file(s)
UI (Electron/Angular)->>CSVDialog/DatabaseIngestor: importFromCSV(files)
alt Import to parameter
CSVDialog/DatabaseIngestor->>VariableValue: importFromCSV(files)
VariableValue->>VariableValue: Load CSV, validate, populate dimensions
else Import to database
CSVDialog/DatabaseIngestor->>Database: createTable(files[0], spec)
CSVDialog/DatabaseIngestor->>Database: loadDatabase(files, spec, progressCallback)
end
UI (Electron/Angular)->>Ravel: (optional) Connect/init from database
Ravel->>Database: Query/init state as needed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Complexity: Extensive, high-complexity refactor and feature addition affecting core data import, UI, backend, and serialization logic. Changes span many files, introduce new abstractions, update event handling, and require careful review for interface and behavioral consistency. Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@highperformancecoder I'll conduct a manual review of PR #558. Let me first examine the changes to understand what's being modified. 🏁 Script executed: Length of output: 806 Let me try with the correct base branch and get the changed files: 🏁 Script executed: Length of output: 1164 Let me try different approaches to see the actual changes: 🏁 Script executed: Length of output: 109328 Let me look for any actual feature changes beyond this large merge: 🏁 Script executed: Length of output: 786 @highperformancecoder I've reviewed PR #558 and here's what I found: SummaryThis PR appears to be a foundational merge rather than a feature implementation. It contains:
What This MeansThis PR represents the initial setup of the feature branch where the latest RecommendationsSince this is just a foundational merge with no functional changes:
NoteIf you were expecting to see librarian feature changes in this PR, they may be coming in future commits to this Would you like me to help review the actual librarian feature implementation once those commits are added to this branch? |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
✅ Actions performedFull review triggered. |
|
🎯 Unit Test Generation Post-Process Complete The unit test generation and CI/CD fixing process has completed after reaching the maximum number of iterations. Please review the generated tests and any remaining issues manually. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (1)
engine/minskyTensorOps.cc (1)
514-518: Address the const_cast design issue.While the simplification to use
ravel.createChain(a)is good, theconst_castindicates a design problem. Consider one of these approaches:
- Make the
ravelmember mutable if it needs to be modified during tensor operations- Redesign the API to accept const references where appropriate
- Change the calling context to work with non-const objects
void setArgument(const TensorPtr& a,const Args&) override { arg=a; - // not sure how to avoid this const cast here - chain=const_cast<Ravel&>(ravel).createChain(a); + // TODO: Fix const-correctness - consider making ravel mutable or redesigning API + chain=const_cast<Ravel&>(ravel).createChain(a); }
🧹 Nitpick comments (18)
engine/saver.cc (1)
22-23: Unnecessary direct inclusion of.xcdheaders hereNeither
CSVToolsnordynamicRavelCAPIsymbols are referenced in this compilation unit. Pulling in their heavy.xcdheaders widens the dependency surface, slows compilation, and risks ODR/static-init surprises. Prefer one of:-#include "CSVTools.xcd" -#include "dynamicRavelCAPI.xcd" +// Keep these includes in the TU(s) that actually use CSVTools / Ravel C API, +// or aggregate them in a dedicated umbrella header (eg. minsky_epilogue.h) +// if their static registrars must be pulled in globally.Please verify they are really required here and relocate or drop accordingly.
gui-js/libs/shared/src/lib/constants/constants.ts (1)
41-43: Event naming consistency
IMPORT_CSV_TO_DBfollows a different prefix order thanCSV_IMPORT_REFRESH/IMPORT_CSV. Harmonising verbs first (IMPORT_*) or nouns first (CSV_*) across the object will make autocomplete & grepping easier.gui-js/libs/ui-components/src/lib/connect-database/connect-database.scss (1)
1-13: Scope class names to avoid global clashes
.rowand.containerare very generic and may collide with other libraries (e.g. Bootstrap). Consider a component-specific prefix (e.g..db-row,.db-container) or the Angular:hostselector to keep styles encapsulated.gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.scss (1)
8-12: Review the height calculation and remove commented code.The
height: calc(100vh + 100px)could cause content to overflow beyond the viewport. Consider whether this extra 100px is necessary, or if it should usemax-heightinstead.Also, the commented padding rule should be removed if it's not needed.
.selectors { - height: calc(100vh + 100px); + max-height: calc(100vh - 20px); // Account for margins/padding overflow-y: auto; - //padding: -10px; }loadDb.py (1)
2-2: Remove unused imports.The static analysis correctly identifies unused imports that should be cleaned up.
-import json sys.path.insert(0,'.') -from pyminsky import minsky, DataSpec +from pyminsky import minskyAlso applies to: 4-4
engine/databaseIngestor.cc (1)
16-30: Clever but potentially fragile progress callback design.The static pointer approach for bridging C-style callbacks with C++ objects is understandable given the integration constraints. The type checking to conditionally set callbacks for derived Minsky classes is appropriate.
However, consider potential thread safety implications of the static
ingestorProgresspointer, especially if multiple database operations could run concurrently.Could you verify if database import operations can run concurrently, and if so, consider thread-local storage or a different callback mechanism?
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (3)
11-11: Remove unused import.The
JSON5import is not used anywhere in this component.-import JSON5 from 'json5';
38-41: Remove empty route params subscription.The subscription to route params doesn't perform any action and creates a potential memory leak.
ngOnInit(): void { - this.route.queryParams.subscribe((params) => { - }); }
43-46: Type event parameters properly.Event parameters should use proper TypeScript event types instead of any.
- setDbType(event) { + setDbType(event: Event) { const target = event.target as HTMLSelectElement; this.dbType=target.value; } - setConnection(event) { + setConnection(event: Event) { const target = event.target as HTMLSelectElement; this.connection=target.value; } - setTable(event) { + setTable(event: Event) { const target = event.target as HTMLSelectElement; this.table=target.value; }Also applies to: 48-51, 59-62
gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts (3)
9-9: Remove unused import.The
JSON5import is not used in this component.-import JSON5 from 'json5';
36-39: Remove empty route params subscription.The subscription doesn't perform any action.
ngOnInit(): void { - this.route.queryParams.subscribe((params) => { - }); }
41-44: Type event parameters properly.Event parameters should use proper TypeScript types.
- setDbType(event) { + setDbType(event: Event) { const target = event.target as HTMLSelectElement; this.dbType=target.value; } - setTable(event) { + setTable(event: Event) { const target = event.target as HTMLSelectElement; this.table=target.value; } - setConnection(event) { + setConnection(event: Event) { const target = event.target as HTMLSelectElement; this.connection=target.value; }Also applies to: 52-55, 57-60
model/ravelWrap.h (2)
89-90: Consider encapsulating the database member.The public
dbmember violates encapsulation principles. Consider providing getter/setter methods or making it private with friend access if needed for performance.+private: ravelCAPI::Database db; ///< backing database +public: + ravelCAPI::Database& database() { return db; } + const ravelCAPI::Database& database() const { return db; }
144-150: Clarify return type and nullptr behavior in documentation.The documentation refers to "the returned expression" (singular) but the method returns a vector. Also, the nullptr behavior for database extraction should be more prominent.
- /// make a tensor expression corresponding to the state of this - /// Ravel, applied to \a arg. If \a arg is nullptr, then the - /// returned expression is extracted from the database. + /// make tensor expression(s) corresponding to the state of this + /// Ravel, applied to \a arg. + /// @param arg tensor to apply state to, or nullptr to extract from database + /// @return vector of tensor expressions std::vector<civita::TensorPtr> createChain(const TensorPtr& arg);gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts (1)
916-952: Well-refactored CSV import method.The changes properly support both parameter and database CSV imports with appropriate cleanup logic.
Consider adding JSDoc comments to document the new parameters:
+ /** + * Opens CSV import dialog + * @param csvDialog - CSV dialog configuration object + * @param isInvokedUsingToolbar - Whether invoked from toolbar (affects cleanup) + * @param dropTable - Whether to drop existing table (for database imports) + */ static async importCSV(csvDialog: CSVDialog, isInvokedUsingToolbar = false, dropTable=false) {gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts (1)
208-232: Well-structured import menu with proper feature gating.The submenu properly separates parameter and database import options, with appropriate feature flag checking for the database option.
Consider extracting the importCSVvariableName to a constant at the top of the method to improve readability:
+ const CSV_IMPORT_VAR_NAME = importCSVvariableName; { label: 'to parameter', async click() { - minsky.canvas.addVariable(importCSVvariableName, 'parameter'); + minsky.canvas.addVariable(CSV_IMPORT_VAR_NAME, 'parameter');gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts (2)
99-99: Add type annotation fornewTablepropertyThe
newTableproperty should have an explicit type annotation for better code clarity and type safety.- newTable: boolean; + newTable: boolean;
196-197: Consider using a more robust boolean parsing approachThe current string comparison could be improved to handle edge cases better.
- this.isInvokedUsingToolbar = params.isInvokedUsingToolbar==="true"; - this.newTable = params.dropTable==="true"; + this.isInvokedUsingToolbar = params.isInvokedUsingToolbar?.toLowerCase() === "true"; + this.newTable = params.dropTable?.toLowerCase() === "true";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gui-js/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
Doxyversion(1 hunks)Makefile(2 hunks)RESTService/addon.cc(2 hunks)RESTService/pyminsky.cc(1 hunks)RESTService/typescriptAPI.cc(4 hunks)RavelCAPI(1 hunks)doc/version.tex(1 hunks)engine/CSVParser.cc(3 hunks)engine/CSVParser.h(1 hunks)engine/databaseIngestor.cc(1 hunks)engine/databaseIngestor.h(1 hunks)engine/equations.cc(1 hunks)engine/minskyTensorOps.cc(1 hunks)engine/saver.cc(1 hunks)engine/variableValue.cc(1 hunks)engine/variableValue.h(2 hunks)gui-js/apps/minsky-electron/src/app/backend-init.ts(2 hunks)gui-js/apps/minsky-electron/src/app/events/electron.events.ts(4 hunks)gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts(4 hunks)gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts(13 hunks)gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts(3 hunks)gui-js/apps/minsky-electron/src/app/managers/RecordingManager.ts(3 hunks)gui-js/apps/minsky-electron/src/app/managers/StoreManager.ts(3 hunks)gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts(2 hunks)gui-js/apps/minsky-web/src/app/app-routing.module.ts(3 hunks)gui-js/libs/core/src/lib/services/communication/communication.service.ts(1 hunks)gui-js/libs/shared/src/lib/backend/minsky.ts(21 hunks)gui-js/libs/shared/src/lib/constants/constants.ts(1 hunks)gui-js/libs/shared/src/lib/constants/version.ts(1 hunks)gui-js/libs/ui-components/src/index.ts(1 hunks)gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts(1 hunks)gui-js/libs/ui-components/src/lib/connect-database/connect-database.html(1 hunks)gui-js/libs/ui-components/src/lib/connect-database/connect-database.scss(1 hunks)gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.html(1 hunks)gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts(15 hunks)gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts(1 hunks)gui-js/libs/ui-components/src/lib/new-database/new-database.html(1 hunks)gui-js/libs/ui-components/src/lib/new-database/new-database.scss(1 hunks)gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts(1 hunks)gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.html(1 hunks)gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.scss(1 hunks)gui-js/package.json(1 hunks)loadDb.py(1 hunks)minskyVersion.h(1 hunks)model/CSVDialog.cc(0 hunks)model/CSVDialog.h(2 hunks)model/canvas.cc(1 hunks)model/godleyIcon.cc(1 hunks)model/godleyIcon.h(1 hunks)model/item.h(1 hunks)model/minsky.cc(3 hunks)model/minsky.h(2 hunks)model/ravelWrap.cc(2 hunks)model/ravelWrap.h(3 hunks)model/variable.cc(1 hunks)model/variable.h(1 hunks)schema/dataSpecSchema.h(1 hunks)schema/schema3.cc(4 hunks)schema/schema3.h(2 hunks)test/00/RESTService.sh(1 hunks)test/00/importCSV.sh(1 hunks)test/00/t0022a.sh(1 hunks)test/Makefile(1 hunks)test/common-test.sh(1 hunks)test/testCSVParser.cc(2 hunks)test/testCSVParserGemini.cc(1 hunks)test/testSaver.cc(1 hunks)
💤 Files with no reviewable changes (1)
- model/CSVDialog.cc
🧰 Additional context used
🧠 Learnings (13)
engine/CSVParser.h (1)
Learnt from: highperformancecoder
PR: #541
File: engine/CSVParser.cc:652-656
Timestamp: 2025-02-06T01:34:05.110Z
Learning: In the CSVParser, bad input files are handled gracefully by treating them as empty files, without throwing explicit errors. This is by design to make the parser more robust.
engine/databaseIngestor.cc (1)
Learnt from: highperformancecoder
PR: #541
File: gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts:556-556
Timestamp: 2025-02-06T02:56:42.081Z
Learning: In multi-file CSV imports, error messages are prepended with the filename of the faulty file, providing context for troubleshooting. While error reports can be generated for individual files, the report functionality could be enhanced by adding a filename column and supporting reports over multiple files.
engine/CSVParser.cc (2)
Learnt from: highperformancecoder
PR: #541
File: engine/CSVParser.cc:652-656
Timestamp: 2025-02-06T01:34:05.110Z
Learning: In the CSVParser, bad input files are handled gracefully by treating them as empty files, without throwing explicit errors. This is by design to make the parser more robust.
Learnt from: highperformancecoder
PR: #538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
Doxyversion (1)
Learnt from: highperformancecoder
PR: #551
File: Makefile:0-0
Timestamp: 2025-05-09T01:46:43.485Z
Learning: In the Minsky project's Makefile, the behavior of := variable assignments varies across systems. While some systems automatically export these variables to subshells without explicit directives, others don't. For maximum portability, explicit export LD_LIBRARY_PATH statements should be used when environment variables need to be visible to subshells and commands run from the Makefile.
test/Makefile (1)
Learnt from: highperformancecoder
PR: #551
File: Makefile:0-0
Timestamp: 2025-05-09T01:46:43.485Z
Learning: In the Minsky project's Makefile, the behavior of := variable assignments varies across systems. While some systems automatically export these variables to subshells without explicit directives, others don't. For maximum portability, explicit export LD_LIBRARY_PATH statements should be used when environment variables need to be visible to subshells and commands run from the Makefile.
engine/saver.cc (1)
Learnt from: highperformancecoder
PR: #538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
Makefile (1)
Learnt from: highperformancecoder
PR: #551
File: Makefile:0-0
Timestamp: 2025-05-09T01:46:43.485Z
Learning: In the Minsky project's Makefile, the behavior of := variable assignments varies across systems. While some systems automatically export these variables to subshells without explicit directives, others don't. For maximum portability, explicit export LD_LIBRARY_PATH statements should be used when environment variables need to be visible to subshells and commands run from the Makefile.
engine/variableValue.h (1)
Learnt from: highperformancecoder
PR: #538
File: model/variable.h:249-260
Timestamp: 2024-12-30T09:44:42.105Z
Learning: The user clarified that the newly introduced slider parameter methods in VariableBase (sliderMin, sliderMax, sliderStep, etc.) are required for delegation from Variable to VariableValue, maintaining JS compatibility. Negative or zero sliderStep values are addressed in other parts of the codebase.
engine/variableValue.cc (2)
Learnt from: highperformancecoder
PR: #541
File: gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts:556-556
Timestamp: 2025-02-06T02:56:42.081Z
Learning: In multi-file CSV imports, error messages are prepended with the filename of the faulty file, providing context for troubleshooting. While error reports can be generated for individual files, the report functionality could be enhanced by adding a filename column and supporting reports over multiple files.
Learnt from: highperformancecoder
PR: #538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in variableValue.cc rather than throwing an exception if a dimension is not found.
model/variable.h (1)
Learnt from: highperformancecoder
PR: #538
File: model/variable.h:249-260
Timestamp: 2024-12-30T09:44:42.105Z
Learning: The user clarified that the newly introduced slider parameter methods in VariableBase (sliderMin, sliderMax, sliderStep, etc.) are required for delegation from Variable to VariableValue, maintaining JS compatibility. Negative or zero sliderStep values are addressed in other parts of the codebase.
model/minsky.cc (2)
Learnt from: highperformancecoder
PR: #551
File: Makefile:0-0
Timestamp: 2025-05-09T01:46:43.485Z
Learning: In the Minsky project's Makefile, the behavior of := variable assignments varies across systems. While some systems automatically export these variables to subshells without explicit directives, others don't. For maximum portability, explicit export LD_LIBRARY_PATH statements should be used when environment variables need to be visible to subshells and commands run from the Makefile.
Learnt from: highperformancecoder
PR: #538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
test/common-test.sh (1)
Learnt from: highperformancecoder
PR: #551
File: Makefile:0-0
Timestamp: 2025-05-09T01:46:43.485Z
Learning: In the Minsky project's Makefile, the behavior of := variable assignments varies across systems. While some systems automatically export these variables to subshells without explicit directives, others don't. For maximum portability, explicit export LD_LIBRARY_PATH statements should be used when environment variables need to be visible to subshells and commands run from the Makefile.
gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts (1)
Learnt from: highperformancecoder
PR: #541
File: gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts:556-556
Timestamp: 2025-02-06T02:56:42.081Z
Learning: In multi-file CSV imports, error messages are prepended with the filename of the faulty file, providing context for troubleshooting. While error reports can be generated for individual files, the report functionality could be enhanced by adding a filename column and supporting reports over multiple files.
🧬 Code Graph Analysis (13)
model/godleyIcon.h (3)
model/ravelWrap.cc (2)
onMouseUp(162-169)onMouseUp(162-162)gui-js/libs/shared/src/lib/backend/minsky.ts (7)
onMouseUp(96-96)onMouseUp(269-269)onMouseUp(729-729)onMouseUp(1097-1097)onMouseUp(1800-1800)onMouseUp(2020-2020)onMouseUp(2547-2547)model/godleyIcon.cc (2)
onMouseUp(612-616)onMouseUp(612-612)
engine/CSVParser.h (1)
gui-js/libs/shared/src/lib/backend/minsky.ts (19)
DataSpec(497-535)r(150-150)r(2482-2482)dataRowOffset(513-513)dataRowOffset(553-553)dataRowOffset(2583-2583)headerRow(520-520)headerRow(558-558)headerRow(2587-2587)mergeDelimiters(523-523)mergeDelimiters(560-560)mergeDelimiters(2588-2588)counter(511-511)counter(551-551)counter(2582-2582)dontFail(515-515)dontFail(555-555)dontFail(2585-2585)dimensions(138-138)
gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts (2)
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (1)
Component(13-110)gui-js/libs/shared/src/lib/backend/minsky.ts (1)
Ravel(1746-1822)
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts (5)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
WindowManager(23-372)gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts (2)
type(168-170)dropTable(138-140)gui-js/libs/shared/src/lib/backend/minsky.ts (8)
type(154-154)type(302-302)type(1533-1533)type(2293-2293)type(2362-2362)defaultExtension(1028-1028)defaultExtension(1947-1947)CSVDialog(347-365)model/group.cc (2)
defaultExtension(1251-1256)defaultExtension(1251-1251)gui-js/libs/shared/src/lib/interfaces/Interfaces.ts (1)
CanvasItem(117-121)
gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts (1)
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
Ravel(1746-1822)
gui-js/libs/shared/src/lib/constants/version.ts (1)
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
version(2569-2569)
gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
gui-js/libs/shared/src/lib/constants/constants.ts (1)
events(18-68)
model/godleyIcon.cc (2)
model/ravelWrap.cc (2)
onMouseUp(162-169)onMouseUp(162-162)model/godleyIcon.h (1)
editor(82-182)
engine/databaseIngestor.h (3)
model/minsky.h (3)
minsky(61-102)string(327-327)string(328-335)gui-js/libs/shared/src/lib/backend/minsky.ts (7)
DatabaseIngestor(567-588)createTable(577-577)createTable(2503-2503)importFromCSV(241-241)importFromCSV(356-356)importFromCSV(579-579)importFromCSV(2260-2260)engine/databaseIngestor.cc (2)
importFromCSV(24-40)importFromCSV(24-24)
gui-js/apps/minsky-electron/src/app/managers/RecordingManager.ts (1)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
WindowManager(23-372)
schema/dataSpecSchema.h (2)
model/CSVDialog.h (1)
minsky(33-71)engine/CSVParser.h (1)
minsky(33-141)
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts (3)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
WindowManager(23-372)gui-js/libs/shared/src/lib/constants/constants.ts (1)
importCSVvariableName(119-119)gui-js/libs/shared/src/lib/backend/minsky.ts (1)
VariableBase(187-319)
gui-js/libs/shared/src/lib/backend/minsky.ts (3)
engine/databaseIngestor.h (1)
DatabaseIngestor(29-36)gui-js/libs/shared/src/lib/backend/backend.ts (3)
CppClass(3-27)Container(54-67)Sequence(69-72)engine/CSVParser.h (1)
DataSpec(37-130)
🪛 Ruff (0.12.2)
loadDb.py
2-2: json imported but unused
Remove unused import: json
(F401)
4-4: pyminsky.DataSpec imported but unused
Remove unused import: pyminsky.DataSpec
(F401)
🪛 Biome (2.1.2)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts
[error] 171-171: Shouldn't redeclare 'res'. Consider to delete it or rename it.
'res' is defined here:
(lint/suspicious/noRedeclare)
[error] 188-188: Shouldn't redeclare 'res'. Consider to delete it or rename it.
'res' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (106)
gui-js/package.json (1)
3-3: Version bump looks good.The version increment from 3.19.0-beta.1 to 3.19.0-beta.2 is consistent with the coordinated version updates across the project.
test/00/importCSV.sh (1)
10-10: API simplification looks good.The removal of the
.csvDialogproperty access reflects the architectural refactoring where CSV dialog functionality is now directly integrated into the VariableValue object, eliminating unnecessary indirection.gui-js/apps/minsky-electron/src/app/managers/StoreManager.ts (3)
3-3: Good addition of homedir import.Importing
homedirfromnode:osis the correct approach for cross-platform home directory access.
19-21: Well-designed directory persistence feature.Adding
defaultModelDirectoryanddefaultDataDirectoryto the store interface provides good user experience by remembering the last accessed directories for file dialogs.
30-31: Sensible default initialization.Using
homedir()as the initial default for both directory properties is appropriate and provides a good starting point for users.Doxyversion (1)
1-1: Consistent version update.The version bump to 3.19.0-beta.2 in the Doxygen project name maintains consistency with the coordinated version updates across the project.
doc/version.tex (1)
1-1: Documentation version updated consistently.The version update to 3.19.0-beta.2 in the LaTeX documentation maintains consistency with the project-wide version bump.
RavelCAPI (1)
1-1: RavelCAPI submodule pointer unchanged – no action neededThe diff shows no changes to the RavelCAPI gitlink or .gitmodules; all modifications are in engine/CSVParser.cc and GUI code. No downstream compatibility checks against RavelCAPI are required.
Likely an incorrect or invalid review comment.
minskyVersion.h (1)
1-1: Consistent version bump – looks good
The macro now reflects3.19.0-beta.2, keeping native code in sync with the rest of the repo. No further action required.gui-js/libs/shared/src/lib/constants/version.ts (1)
1-1: TS constant updated correctly
The exportedversionconstant matches the C++ macro and other artefacts. 👍test/testSaver.cc (1)
22-23: Added headers appear unused – please verify necessity
CSVTools.xcdanddynamicRavelCAPI.xcdare included but the test doesn’t reference any symbols from them. Extra includes slow builds and widen dependency surfaces. If they were added pre-emptively, consider removing until genuinely required.test/Makefile (1)
25-25: Ensure SOCI core library and headers are available for testingI wasn’t able to verify
-lsoci_corein the sandbox (noldconfig, nosoci.hfound). Please confirm in your environment that:
- The SOCI core library is installed and registered with the linker (e.g. via
ldconfigor appropriate-Lpaths).- The SOCI headers (
soci.h, etc.) are installed and discoverable by the compiler (standard include paths or-Iflags).If you’re relying on pkg-config, you could replace the hard-coded
-lsoci_corewith something like:PKGCONFIG := pkg-config CXXFLAGS += $(shell $(PKGCONFIG) --cflags soci_core) LIBS += $(shell $(PKGCONFIG) --libs soci_core)Otherwise, ensure your README or CI setup installs the system package (e.g.
lib-soci-core-dev) so that tests link and compile successfully.Makefile (2)
165-165: LGTM! Properly integrates the new database ingestion object.The addition of
databaseIngestor.oto theENGINE_OBJSlist correctly includes the new database ingestion functionality in the build process.
308-308: LGTM! SOCI library properly linked for database functionality.The addition of
-lsoci_coreto the LIBS variable enables linking against the SOCI core library, which is required for the new database ingestion features.model/godleyIcon.h (1)
154-154: LGTM! Coordinated interface change for reset signaling.The change from
voidtoboolreturn type foronMouseUpis part of a coordinated update across multiple classes (Item,VariableBase,GodleyIcon,Group,Ravel, andSelection) to support reset signaling after mouse events. The implementation correctly returnsm_editorModeto indicate when a reset may be needed.test/common-test.sh (1)
30-30: LGTM! Improves robustness of environment variable handling.Adding quotes around the
PYTHONPATHassignment ensures proper handling of paths containing spaces or special characters, which is a shell scripting best practice.model/minsky.h (3)
29-29: LGTM! Properly includes the database ingestion header.The inclusion of
databaseIngestor.his appropriately placed and necessary for the new database ingestion functionality being integrated into the Minsky class.
31-31: LGTM! Includes dynamic Ravel CAPI support.The inclusion of
dynamicRavelCAPI.hsupports the enhanced Ravel functionality with database-backed capabilities mentioned in the AI summary.
147-148: LGTM! Integrates database ingestion into core Minsky class.The addition of the
databaseIngestormember variable properly integrates the new database ingestion functionality into the core Minsky model. The placement alongside other similar members likepublicationTabsandphillipsDiagramis appropriate.RESTService/pyminsky.cc (1)
100-101: LGTM! Clean addition of DataSpec type declaration.The
usingdirective andCLASSDESC_DECLARE_TYPEdeclaration follow the established pattern in this file and properly exposeDataSpecfor serialization/reflection in the Python module.model/godleyIcon.cc (1)
612-616: Good implementation of the new mouse event handling contract.The method signature change from
voidtoboolis consistent with the broader refactoring of mouse event handling. Returningm_editorModeappropriately signals whether a reset is needed, which aligns with similar changes in other classes likeRavel.test/00/RESTService.sh (1)
12-12: Test correctly updated to reflect new JSON structure.The JSON reference has been properly updated to match the refactored data model where
csvDialogfunctionality has been integrated directly intoVariableValue, promotingspecto the top level and movingurlto the end.gui-js/libs/ui-components/src/lib/new-database/new-database.scss (1)
1-13: Well-structured stylesheet with clean flexbox layout.The CSS classes are appropriately designed for responsive layout:
.rowprovides horizontal flex layout with centered alignment.containerprovides vertical flex layout with centered alignment- Proper import of shared theme stylesheet
The implementation follows modern CSS best practices and provides reusable layout utilities.
schema/dataSpecSchema.h (2)
22-23: Appropriate includes for ravel integration.The new includes for
CSVTools.handravelState.hare necessary to support the inheritance fromravel::CSVSpecand the broader integration with the ravel CSV parsing system.
30-30: Good refactoring to use ravel CSV specification.Inheriting from
ravel::CSVSpeceliminates code duplication and centralizes CSV parsing logic around the established ravel system. This is a clean architectural improvement that consolidates CSV specification handling.gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.html (1)
389-396: LGTM! Clean addition of Drop table checkbox.The new "Drop table" checkbox is properly integrated with the existing form structure and includes a helpful tooltip explaining its purpose. The reformatting of the "Counter" checkbox maintains consistency in the layout.
test/testCSVParserGemini.cc (2)
45-48: Good test design pattern for abstract base class.The
TestCSVDialogstruct properly provides a minimal concrete implementation of the abstractCSVDialogclass for testing purposes. The emptyimportFromCSVoverride is appropriate since the tests focus on parsing logic rather than import execution.
53-53: Proper adaptation to refactored CSVDialog interface.The change from
CSVDialogtoTestCSVDialogcorrectly adapts the test fixture to work with the new abstract base class design.model/canvas.cc (1)
149-151: Excellent improvement to event handling precision.The change to capture the boolean return value from
onMouseUpand conditionally triggerrequestReset()provides much better control over when system resets occur. This prevents unnecessary resets and allows individual items to determine when their mouse interactions require a system reset.gui-js/apps/minsky-electron/src/app/managers/RecordingManager.ts (3)
26-27: Good standardization of dialog handling.Replacing direct
dialog.showOpenDialogwithWindowManager.showOpenDialogand using the:modelsdefault path enables directory persistence, improving user experience by remembering the last used model directory.
51-51: Consistent application of WindowManager pattern.The change maintains consistency with the dialog handling refactoring being applied across the application.
132-138: Enhanced UX with directory persistence and better default naming.Using
WindowManager.showSaveDialogwith:models/recording.jsonas the default path provides both directory persistence and a sensible default filename for recording files.engine/equations.cc (1)
211-211: LGTM! Clean integration with Ravel tensor operations.This change correctly treats
raveloperations as tensor operations by short-circuiting the evaluation totruebefore the classification switch statement. The placement is optimal for performance and aligns well with the broader Ravel integration enhancements described in the AI summary.test/00/t0022a.sh (1)
4-4: Good refactoring to standardize test infrastructure.Replacing custom setup/teardown logic with the sourced
test/common-test.shimproves maintainability and consistency across test scripts. This centralizes common test utilities while preserving the core test functionality.engine/variableValue.h (2)
44-44: Architectural improvement: inheritance over composition for CSV functionality.The change from having
CSVDialogas a member to inheriting from it directly aligns well with the broader refactoring that centralizes CSV import functionality. This makes CSV import a core interface concern rather than a nested capability.
176-176: Consistent method override for centralized CSV import.The
importFromCSVoverride declaration correctly implements the pure virtual method from the newCSVDialogbase class, integrating CSV import functionality directly intoVariableValueas part of the centralized CSV architecture.model/item.h (1)
261-262: Well-designed interface enhancement for conditional model resets.The change from
voidtoboolreturn type allows derived classes to signal when a model reset is needed after mouse up events. The defaultfalsereturn maintains backward compatibility while enabling more precise control over expensive reset operations in classes likeGodleyIconandRavel.model/variable.h (1)
222-225: Appropriate deprecation notice for architectural migration.The deprecation comment properly communicates the planned removal in version 4, aligning with the broader refactoring that centralizes CSV import functionality into dedicated abstractions like
CSVDialogandDatabaseIngestor. This gives users clear guidance on the API evolution timeline.gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
431-431: LGTM: Simplified CSV import invocation.The removal of the payload parameter aligns well with the backend refactoring to centralize CSV import logic. The backend now handles creating the VariableBase from canvas focus internally, making this API cleaner.
engine/variableValue.cc (1)
89-97: LGTM: Well-implemented CSV import method.This method properly integrates CSV import functionality into the VariableValue class with appropriate error handling:
- Safely handles empty filename vectors
- Leverages existing
loadValueFromCSVFilefor the core functionality- Ensures missing dimensions are populated
- Validates dimension uniqueness with a clear error message
The implementation aligns well with the broader CSV import refactoring across the codebase.
model/minsky.cc (3)
37-42: LGTM: Includes support new database functionality.The new includes for CSV tools, database ingestion, and dynamic Ravel C API properly support the broader database integration features being added to the system.
212-212: Good defensive programming with null check.Adding the null check for
modelprevents potential crashes whendefiningVaris called in contexts where the model hasn't been initialized yet.
1838-1838: LGTM: Template instantiation for Database class.The explicit template instantiation for
ravelCAPI::Databaseis needed to support serialization/reflection of the new database class being integrated.gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.html (1)
3-4: Consider adding two-way binding for text input.The text input uses one-way binding
[ngModel]but lacks(ngModelChange)for two-way binding. If users should be able to edit the horizontal dimension name, consider using[(ngModel)]instead.If this input should be editable, apply this diff:
<input type="text" id="horizontalDimName" - [ngModel]="horizontalDimName"/> + [(ngModel)]="horizontalDimName"/>Otherwise, if it's intended to be read-only, consider adding the
readonlyattribute for clarity.gui-js/apps/minsky-electron/src/app/backend-init.ts (2)
64-69: Improved error handling for CSV import operations.The change correctly broadens the error suppression condition from an exact match to checking if the command ends with
'importFromCSV'. This appropriately handles multiple CSV import command variants introduced in the database integration feature while maintaining error dialog suppression for these operations.
150-153: Good defensive programming for progress bar management.The addition of cleanup logic to finalize existing progress bars before creating new ones prevents UI conflicts and resource leaks. This ensures clean transitions between different progress operations.
RESTService/addon.cc (2)
245-245: Proper integration of Ravel cancellation mechanism.The addition of
ravelCAPI::Ravel::cancel(false)alongside the existing tensor cancellation correctly extends the cancellation state management to include Ravel database operations. This ensures both tensor and database operations start with a clean cancellation state.
598-598: Complete cancellation support for database operations.The addition of
ravelCAPI::Ravel::cancel(true)ensures that cancellation requests properly propagate to Ravel database operations, maintaining consistency with the existing tensor cancellation mechanism.loadDb.py (1)
5-53: Well-structured database ingestion example.The script effectively demonstrates the new database ingestion functionality with proper configuration:
- Appropriate DataSpec setup for Citibike data format
- Correct column mappings and mixed data types (value, time, string)
- Error tolerance enabled with
dontFail(True)for robust processing- Clean workflow: connect → configure → create table → import data
This serves as a good reference implementation for users of the database ingestion feature.
gui-js/libs/ui-components/src/lib/new-database/new-database.html (1)
1-31: Well-structured database connection UI template.The template demonstrates good Angular practices with:
- Appropriate conditional rendering based on database type
- Clean separation between SQLite file selection and connection string input
- Well-integrated Material Design autocomplete for table selection
- Proper event binding and accessible form structure
The user experience flow is intuitive and the implementation follows Angular best practices.
engine/databaseIngestor.cc (1)
24-40: Well-implemented batch CSV import with proper progress tracking.The implementation demonstrates good C++ practices:
- Appropriate two-level progress tracking (batch and individual file)
- Clean RAII-style resource management with ProgressUpdater
- Proper static pointer cleanup ensuring no dangling references
- Good use of filesystem utilities for user-friendly progress messages
The code is robust and follows established patterns for progress reporting in the codebase.
engine/databaseIngestor.h (4)
1-19: Copyright and license section looks good.Standard GPL v3 boilerplate is properly formatted and complete.
21-26: Include structure is well-organized.Clean includes with proper include guard naming convention.
29-36: Class design follows appropriate inheritance pattern.The public inheritance from
CSVDialogand the override ofimportFromCSVcorrectly implement the abstract base class interface. The inlinecreateTablemethod provides clean delegation to the underlying database object.
37-40: File structure properly closed.Namespace and include guard are correctly terminated.
test/testCSVParser.cc (3)
200-204: Appropriate test fixture for abstract CSVDialog.The
TestCSVDialogcorrectly provides the requiredimportFromCSVoverride with an empty implementation, which is suitable for testing CSV parsing and classification functionality without needing actual import logic.
205-205: Correct adaptation to abstract base class.Using
TestCSVDialogas the fixture is the appropriate way to test the abstractCSVDialogfunctionality.
563-563: Updated to use new CSV loading interface.The change from using an
ifstreamto passing a vector of filenames aligns with the refactored CSV import functionality that now supports multiple file imports.model/variable.cc (2)
467-468: Clean delegation to VariableValue's CSV import functionality.The refactored approach correctly sets the data specification and delegates import to the
VariableValue's ownimportFromCSVmethod, aligning with the new architecture whereVariableValueinherits fromCSVDialog.
476-477: Simplified reload logic using updated CSV interface.The refactored code correctly uses the
VariableValue's ownurlandspecproperties with the updatedloadValueFromCSVFileinterface that accepts filename vectors.engine/minskyTensorOps.cc (1)
510-512: LGTM! Database initialization in constructor looks correct.The conditional initialization of the chain when a database connection exists follows a logical pattern for database-backed tensor operations.
gui-js/libs/ui-components/src/index.ts (1)
1-34: LGTM! Export reordering improves organization.The reordering of export statements better groups related database connection and CSV import components, which aligns with the new functionality being added and improves maintainability.
engine/CSVParser.h (1)
86-99: LGTM! Conversion operator is well-implemented.The conversion from
minsky::DataSpectoravel::DataSpecis correctly implemented:
- Safe base class assignment via static_cast
- Comprehensive field copying
- Proper bounds checking with
std::minwhen pairing dimensions- The TODO comment appropriately flags this as potentially temporary code
The implementation facilitates the integration between Minsky's CSV specifications and the Ravel database API.
schema/schema3.cc (4)
21-22: LGTM! Include directives support database integration.The new includes for
CSVTools.xcdanddynamicRavelCAPI.xcdare appropriate for supporting the database connection and CSV import functionality being integrated.
197-199: LGTM! Database connection serialization is correctly implemented.The logic properly:
- Retrieves connection information from the ravel database
- Only serializes when a connection exists (non-empty dbType)
- Stores the connection details in the schema appropriately
507-511: LGTM! Database connection deserialization is correctly implemented.The deserialization logic properly:
- Checks for the presence of database connection info
- Connects using the stored connection parameters
- Initializes the Ravel object from the database
- Maintains consistency with the serialization process
830-832: LGTM! Direct assignments simplify the data model.The direct assignment of
csvDataSpecandurlfromVariableValue(instead of throughcsvDialog) is a beneficial simplification that:
- Reduces indirection and complexity
- Maintains equivalent functionality
- Aligns with the broader refactoring to remove the intermediate
csvDialogmemberschema/schema3.h (2)
118-118: LGTM! Good use of Optional for backward compatibility.The addition of the optional
dbConnectionfield is well-placed and properly typed for storing database connection details with schema items.
151-153: Good refactoring to remove csvDialog intermediary.The direct access to
vv->specandvv->urlsimplifies the data flow and aligns well with the broader refactoring to centralize CSV import functionality.model/CSVDialog.h (2)
35-35: Excellent separation of concerns.Removing the
RenderNativeWindowinheritance properly decouples CSV data handling from UI rendering responsibilities.
66-67: Well-designed abstract interface for CSV import.The pure virtual
importFromCSVmethod provides a clean contract for derived classes while supporting batch file imports.RESTService/typescriptAPI.cc (3)
10-25: LGTM! Includes properly organized for new features.The new type descriptor includes are well-organized and align with the database integration and CSV handling features.
251-269: Proper API exposure for new database and CSV features.The new class registrations correctly expose the database integration and CSV handling classes to the TypeScript API.
313-313: Verify the purpose of the empty CAPIRenderer class.This appears to be a placeholder. Consider adding a TODO comment to clarify future implementation plans.
Is this dummy class intended for future renderer functionality? If so, consider adding a TODO comment to document the planned implementation.
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (3)
903-926: Well-implemented database integration menu items.Good use of feature flag checking with
ravelPro()to conditionally enable database features. The popup window configurations are appropriate for their respective tasks.
1191-1192: Good refactoring of CSV import handler.The change to use
variableValuesdirectly and make the handler async properly aligns with the new CSV dialog abstraction.
1397-1441: Consistent refactoring throughout CSV import menu.All CSV import operations have been properly updated to use the
valueobject directly, removing the unnecessarycsvDialogintermediary layer.engine/CSVParser.cc (1)
230-247: Good addition of schema synchronization.The
toSchema()call ensures the schema stays synchronized when the data area is modified.gui-js/apps/minsky-electron/src/app/events/electron.events.ts (2)
72-84: Good refactoring to centralize dialog management.Using
WindowManager.showOpenDialogandWindowManager.showSaveDialogcentralizes dialog handling and directory persistence.
217-231: Well-structured CSV import handlers.The separation of variable CSV import and database CSV import with appropriate parameters is clean and follows good design patterns.
model/ravelWrap.cc (3)
162-169: Good implementation of conditional reset logic.The change to return whether the input port has wires enables proper conditional reset behavior - only resetting when sourced from variables, not from databases.
279-286: Well-designed database initialization.The
initRavelFromDbmethod properly checks for database existence and empty input wires before initialization, and correctly disables reset until user interaction.
288-302: Efficient caching implementation.The
createChainmethod efficiently caches database results and only queries when the state changes. Good optimization.model/ravelWrap.h (2)
81-84: LGTM! Well-structured caching mechanism.The addition of
lastStatefor serialized state caching andcachedDbResultfor database query results follows good caching patterns. The private visibility ensures proper encapsulation.
116-116: Good API design improvement.Changing
onMouseUpto return bool allows the handler to signal whether a reset/redraw is needed, improving efficiency by avoiding unnecessary redraws.gui-js/apps/minsky-web/src/app/app-routing.module.ts (2)
4-4: LGTM! New components properly imported.The database-related components are correctly imported from the shared UI components library, maintaining consistency with the existing import pattern.
Also applies to: 17-17, 23-23
57-59: Well-structured route configuration for database features.The new routes follow the established 'headless/' pattern and kebab-case naming convention, properly integrating the database-related components into the routing structure.
Also applies to: 113-115, 133-135
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
14-15: LGTM! Required imports for dialog functionality.The added imports properly support the new dialog wrapper methods and path manipulation functionality.
Also applies to: 18-18
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts (5)
4-4: LGTM! Required imports for enhanced CSV functionality.The CSVDialog and VariableBase imports properly support the refactored CSV import workflow.
Also applies to: 6-6
121-123: Good dialog centralization.Properly uses WindowManager wrapper with ':models' prefix for directory persistence.
174-176: Consistent dialog usage.Properly migrated to WindowManager with appropriate default path.
328-329: Comprehensive dialog migration completed successfully.All file dialogs consistently use WindowManager wrappers with appropriate ':models' prefixes, enabling directory persistence across the application.
Also applies to: 371-371, 385-387, 491-499, 897-899, 1006-1006
980-995: Clean helper method for save options.Good abstraction that centralizes the save dialog configuration with proper default path handling.
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts (2)
6-6: Required import for CSV parameter import.VariableBase is properly imported to support the new import data workflow.
146-148: Consistent dialog migration throughout menu system.All file dialogs properly use WindowManager wrappers with contextually appropriate default paths (':models' for general files, ':models/plot' for plot exports).
Also applies to: 237-238, 577-579
gui-js/libs/ui-components/src/lib/import-csv/import-csv.component.ts (9)
138-141: LGTM!The
dropTablegetter is correctly implemented and follows the established pattern for form control accessors.
297-305: LGTM!The default path handling with ':data' prefix and examples directory support is correctly implemented.
328-342: LGTM!The refactoring to use
csvDialogfor URL management and file operations is correctly implemented.
361-364: LGTM!The method correctly uses the
csvDialoginstance for retrieving dialog specifications.
386-386: LGTM!The
parseLinesmethod correctly delegates to thecsvDialoginstance.
415-415: LGTM!The spec update correctly uses the
csvDialoginstance.
579-588: LGTM!The variable renaming logic correctly checks if the focused item matches the CSV dialog's value ID before updating properties.
598-598: LGTM!The report generation correctly uses the
:data/prefix for the default path and delegates tocsvDialogfor report generation.Also applies to: 605-605
612-612: LGTM!The context menu correctly uses
csvDialogfor spec updates and command prefixes.Also applies to: 617-617
gui-js/libs/shared/src/lib/backend/minsky.ts (2)
356-356: Inconsistent parameter types forimportFromCSVmethodsThe
importFromCSVmethod inCSVDialog(line 356) takesa1: string[]while the same method inVariableValue(line 2260) also takesa1: string[]. However,VariableBasehas a different signature at line 241:importFromCSV(a1: string[], a2: DataSpecSchema).Consider whether these inconsistencies are intentional or if they should be aligned for consistency.
Also applies to: 2260-2260
96-96: ConfirmonMouseUpreturn type handlingI ran a global search for
.onMouseUp((excludingminsky.ts) and found no references. Absence of matches isn’t evidence of absence—please manually verify that any existing callers (in UI components, event handlers, etc.) have been updated to handle the newPromise<boolean>return value (e.g. checking the boolean to decide event propagation).
| /// If options contains defaultPath that has starts with ':model/' | ||
| /// or ':data/', then the last directory visited with that type is | ||
| /// substituted. | ||
| /// returns the directory key for the StoreManager. | ||
| static processDefaultDirectory(options: OpenDialogOptions|SaveDialogOptions) { | ||
| let splitDefaultPath=/([^\/]*)\/?(.*)/.exec(options.defaultPath); | ||
| let defaultType=splitDefaultPath[0]; | ||
| let defaultDirectoryKey=""; | ||
|
|
||
| switch (defaultType) { | ||
| case ':models': | ||
| defaultDirectoryKey='defaultModelDirectory'; | ||
| break; | ||
| case ':data': | ||
| defaultDirectoryKey='defaultDataDirectory'; | ||
| break; | ||
| } | ||
|
|
||
| if (defaultDirectoryKey) { | ||
| let defaultDirectory=StoreManager.store.get(defaultDirectoryKey) as string; | ||
| if (defaultDirectory) | ||
| options['defaultPath']=defaultDirectory; | ||
| } | ||
| return defaultDirectoryKey; | ||
| } |
There was a problem hiding this comment.
Fix regex match group access.
The regex result indexing is incorrect. splitDefaultPath[0] contains the full match, not the first capture group. Also needs null checking.
static processDefaultDirectory(options: OpenDialogOptions|SaveDialogOptions) {
let splitDefaultPath=/([^\/]*)\/?(.*)/.exec(options.defaultPath);
- let defaultType=splitDefaultPath[0];
+ if (!splitDefaultPath) return "";
+ let defaultType=splitDefaultPath[1];
let defaultDirectoryKey="";📝 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.
| /// If options contains defaultPath that has starts with ':model/' | |
| /// or ':data/', then the last directory visited with that type is | |
| /// substituted. | |
| /// returns the directory key for the StoreManager. | |
| static processDefaultDirectory(options: OpenDialogOptions|SaveDialogOptions) { | |
| let splitDefaultPath=/([^\/]*)\/?(.*)/.exec(options.defaultPath); | |
| let defaultType=splitDefaultPath[0]; | |
| let defaultDirectoryKey=""; | |
| switch (defaultType) { | |
| case ':models': | |
| defaultDirectoryKey='defaultModelDirectory'; | |
| break; | |
| case ':data': | |
| defaultDirectoryKey='defaultDataDirectory'; | |
| break; | |
| } | |
| if (defaultDirectoryKey) { | |
| let defaultDirectory=StoreManager.store.get(defaultDirectoryKey) as string; | |
| if (defaultDirectory) | |
| options['defaultPath']=defaultDirectory; | |
| } | |
| return defaultDirectoryKey; | |
| } | |
| static processDefaultDirectory(options: OpenDialogOptions|SaveDialogOptions) { | |
| let splitDefaultPath=/([^\/]*)\/?(.*)/.exec(options.defaultPath); | |
| if (!splitDefaultPath) return ""; | |
| let defaultType=splitDefaultPath[1]; | |
| let defaultDirectoryKey=""; | |
| switch (defaultType) { | |
| case ':models': | |
| defaultDirectoryKey='defaultModelDirectory'; | |
| break; | |
| case ':data': | |
| defaultDirectoryKey='defaultDataDirectory'; | |
| break; | |
| } | |
| if (defaultDirectoryKey) { | |
| let defaultDirectory=StoreManager.store.get(defaultDirectoryKey) as string; | |
| if (defaultDirectory) | |
| options['defaultPath']=defaultDirectory; | |
| } | |
| return defaultDirectoryKey; | |
| } |
🤖 Prompt for AI Agents
In gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts between lines
135 and 159, the regex match result is accessed incorrectly by using
splitDefaultPath[0] which contains the full match instead of the first capture
group. Fix this by accessing splitDefaultPath[1] for the defaultType and add a
null check to ensure splitDefaultPath is not null before accessing its groups to
avoid runtime errors.
gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts
Outdated
Show resolved
Hide resolved
| async getTables() { | ||
| this.ravel.db.connect(this.dbType,this.connection,""); | ||
| this.tables=await this.ravel.db.tableNames(); | ||
| } |
There was a problem hiding this comment.
Add error handling for database operations.
Database operations can fail and should be wrapped in try-catch.
async getTables() {
- this.ravel.db.connect(this.dbType,this.connection,"");
- this.tables=await this.ravel.db.tableNames();
+ try {
+ this.ravel.db.connect(this.dbType,this.connection,"");
+ this.tables=await this.ravel.db.tableNames();
+ } catch (error) {
+ this.electronService.showMessageBoxSync({message: `Failed to get tables: ${error.message}`});
+ this.tables = [];
+ }
}📝 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 getTables() { | |
| this.ravel.db.connect(this.dbType,this.connection,""); | |
| this.tables=await this.ravel.db.tableNames(); | |
| } | |
| async getTables() { | |
| try { | |
| this.ravel.db.connect(this.dbType,this.connection,""); | |
| this.tables=await this.ravel.db.tableNames(); | |
| } catch (error) { | |
| this.electronService.showMessageBoxSync({ | |
| message: `Failed to get tables: ${error.message}` | |
| }); | |
| this.tables = []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts
around lines 47 to 50, the database operations lack error handling. Wrap the
calls to this.ravel.db.connect and this.ravel.db.tableNames inside a try-catch
block to catch any exceptions. In the catch block, handle or log the error
appropriately to prevent unhandled promise rejections and improve robustness.
There was a problem hiding this comment.
These are calls to the backend. The backend calls are all try/catch wrapped, and in the event of an exceptiuon being thrown, a error dialog is shown to the user.
There was a problem hiding this comment.
@highperformancecoder Thank you for the clarification! I understand now that the backend infrastructure already handles error handling for these database operations, including showing error dialogs to users when exceptions occur. This centralized approach makes sense and eliminates the need for additional try-catch blocks in the frontend components.
(\ /)
( ._. )
o_(")(")
Understanding the architecture better now!
✏️ Learnings added
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#558
File: gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts:47-50
Timestamp: 2025-07-24T23:00:53.032Z
Learning: In the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
gui-js/libs/ui-components/src/lib/new-database/new-database.html
Outdated
Show resolved
Hide resolved
| export class RavelSelectHorizontalDimComponent implements OnInit { | ||
| dataCols=[]; | ||
| horizontalDimCols=new Set<string>; | ||
| horizontalDimName="?"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for dimension name.
The default value "?" might not be a valid dimension name. Consider adding validation before allowing submission.
Add validation to ensure horizontalDimName is not empty or contains only the placeholder value before calling setHorizontalNames().
🤖 Prompt for AI Agents
In
gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts
at line 29, the default value for horizontalDimName is set to "?", which may be
invalid. Add validation logic before calling setHorizontalNames() to check that
horizontalDimName is neither empty nor equal to the placeholder "?". Prevent
submission or further processing if the validation fails.
There was a problem hiding this comment.
Even the empty string is valid, but not especially useful.
There was a problem hiding this comment.
@highperformancecoder Thank you for the clarification! I understand now that empty strings and even placeholder values like "?" are valid dimension names in the Ravel context, even if they're not particularly useful. I appreciate you correcting my assumption about the validation requirements - my suggestion was overly restrictive based on general UI principles rather than the specific domain constraints of Minsky's dimension handling.
✏️ Learnings added
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#558
File: gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts:29-29
Timestamp: 2025-07-24T23:03:59.815Z
Learning: In Minsky's Ravel components, empty strings and placeholder values like "?" are valid dimension names, even though they may not be particularly useful. Validation should not prevent users from submitting these values as they are technically acceptable in the domain.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in `variableValue.cc` rather than throwing an exception if a dimension is not found.
| async ngOnInit() { | ||
| this.dataCols=await this.ravel.db.numericalColumnNames(); | ||
| } |
There was a problem hiding this comment.
Add error handling for async database operation.
The numericalColumnNames() call should include error handling in case the database connection fails or returns an error.
async ngOnInit() {
- this.dataCols=await this.ravel.db.numericalColumnNames();
+ try {
+ this.dataCols = await this.ravel.db.numericalColumnNames();
+ } catch (error) {
+ console.error('Failed to load numerical columns:', error);
+ this.dataCols = [];
+ // Consider showing user-friendly error message
+ }
}📝 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 ngOnInit() { | |
| this.dataCols=await this.ravel.db.numericalColumnNames(); | |
| } | |
| async ngOnInit() { | |
| try { | |
| this.dataCols = await this.ravel.db.numericalColumnNames(); | |
| } catch (error) { | |
| console.error('Failed to load numerical columns:', error); | |
| this.dataCols = []; | |
| // Consider showing a user-friendly error message here | |
| } | |
| } |
🤖 Prompt for AI Agents
In
gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts
around lines 40 to 42, the async call to this.ravel.db.numericalColumnNames()
lacks error handling. Wrap the await call in a try-catch block to catch any
errors from the database operation and handle them appropriately, such as
logging the error or setting a fallback state.
There was a problem hiding this comment.
See my previous comment about backend calls.
There was a problem hiding this comment.
@highperformancecoder You're absolutely right! Based on your previous explanation, I understand that in the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. This means the suggested error handling around this.ravel.db.numericalColumnNames() is not necessary since the backend already handles these scenarios appropriately.
The frontend components don't need additional error handling for these backend operations as the error management is centralized at the backend level.
🧠 Learnings used
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#558
File: gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts:47-50
Timestamp: 2025-07-24T23:00:53.032Z
Learning: In the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
...s/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts
Outdated
Show resolved
Hide resolved
| setHorizontalNames() { | ||
| this.ravel.db.setAxisNames([...this.horizontalDimCols],this.horizontalDimName); | ||
| this.ravel.initRavelFromDb(); | ||
| this.closeWindow(); | ||
| } |
There was a problem hiding this comment.
Handle async operations properly with error handling.
The method should await initRavelFromDb() and include error handling for both database operations.
-setHorizontalNames() {
- this.ravel.db.setAxisNames([...this.horizontalDimCols],this.horizontalDimName);
- this.ravel.initRavelFromDb();
- this.closeWindow();
+async setHorizontalNames() {
+ try {
+ await this.ravel.db.setAxisNames([...this.horizontalDimCols], this.horizontalDimName);
+ await this.ravel.initRavelFromDb();
+ this.closeWindow();
+ } catch (error) {
+ console.error('Failed to set horizontal names:', error);
+ // Consider showing user-friendly error message
+ }
}📝 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.
| setHorizontalNames() { | |
| this.ravel.db.setAxisNames([...this.horizontalDimCols],this.horizontalDimName); | |
| this.ravel.initRavelFromDb(); | |
| this.closeWindow(); | |
| } | |
| async setHorizontalNames() { | |
| try { | |
| await this.ravel.db.setAxisNames([...this.horizontalDimCols], this.horizontalDimName); | |
| await this.ravel.initRavelFromDb(); | |
| this.closeWindow(); | |
| } catch (error) { | |
| console.error('Failed to set horizontal names:', error); | |
| // Consider showing user-friendly error message | |
| } | |
| } |
🤖 Prompt for AI Agents
In
gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts
around lines 52 to 56, the method setHorizontalNames calls asynchronous
operations without awaiting them or handling errors. Modify the method to be
async, await the call to this.ravel.initRavelFromDb(), and wrap the database
operations in a try-catch block to properly handle and log any errors that occur
during these async calls.
There was a problem hiding this comment.
Again see my comment on backend error handling.
There was a problem hiding this comment.
@highperformancecoder You're absolutely right! As noted in the learning from this PR, backend operations in the Minsky application are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
The method should simply be made async and await the operations without the unnecessary try-catch wrapper:
async setHorizontalNames() {
await this.ravel.db.setAxisNames([...this.horizontalDimCols], this.horizontalDimName);
await this.ravel.initRavelFromDb();
this.closeWindow();
}This follows the established pattern where the backend handles errors appropriately without requiring frontend error handling code.
🧠 Learnings used
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#558
File: gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts:47-50
Timestamp: 2025-07-24T23:00:53.032Z
Learning: In the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (4)
27-30: Add explicit types to class properties.The class properties lack explicit TypeScript type annotations, which reduces type safety and code maintainability.
Apply this diff to add explicit types:
- dbType="sqlite3"; - connection: string; - table=""; - tables=[]; + dbType: string = "sqlite3"; + connection: string = ""; + table: string = ""; + tables: string[] = [];
64-67: Avoid direct DOM manipulation in Angular.Direct DOM manipulation via
document.getElementByIdviolates Angular best practices and should be replaced with template reference variables or reactive forms.Use template reference variables instead:
- setTableInput(event) { - let input=document.getElementById("table") as HTMLInputElement; - this.table=input.value=event?.option?.value; - } + setTableInput(event) { + this.table = event?.option?.value; + }Then update the template to use two-way binding:
[(ngModel)]="table"
93-93: Add await to async method call.The
getTables()method is async but not being awaited, which could cause timing issues.- this.getTables(); + await this.getTables();
104-104: Fix critical logic error in dropTable condition.The condition incorrectly checks if the tables array includes itself, which is always false. It should check if the selected table exists in the tables array.
- let dropTable=!this.tables.includes(this.tables); + let dropTable=!this.tables.includes(this.table);
🧹 Nitpick comments (2)
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (2)
38-41: Remove unused query parameter subscription or implement the logic.The component subscribes to route query parameters but doesn't use them, which creates an unnecessary subscription.
Either implement the parameter handling logic or remove the unused subscription:
- ngOnInit(): void { - this.route.queryParams.subscribe((params) => { - }); - } + ngOnInit(): void { + // Remove if no query params are needed, or implement param handling + }
64-64: Add type annotation to event parameter.The event parameter lacks a proper type annotation, reducing type safety.
- setTableInput(event) { + setTableInput(event: any) {Or better yet, use a more specific type if the exact event type is known from the Material autocomplete component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts(2 hunks)gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts(1 hunks)gui-js/libs/ui-components/src/lib/connect-database/connect-database.html(1 hunks)gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts(1 hunks)gui-js/libs/ui-components/src/lib/new-database/new-database.html(1 hunks)gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- gui-js/libs/ui-components/src/lib/ravel-select-horizontal-dim/ravel-select-horizontal-dim.component.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- gui-js/libs/ui-components/src/lib/new-database/new-database.html
- gui-js/libs/ui-components/src/lib/connect-database/connect-database.html
- gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts
- gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts
🧰 Additional context used
🧠 Learnings (1)
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (1)
Learnt from: highperformancecoder
PR: #558
File: gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts:47-50
Timestamp: 2025-07-24T23:00:53.032Z
Learning: In the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (1)
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts (1)
54-57: Backend error handling is already implemented.Based on the retrieved learnings from this codebase, backend database operations are already wrapped in try-catch blocks at the backend level with automatic error dialogs. No additional frontend error handling is needed here.
gui-js/libs/ui-components/src/lib/new-database/new-database.component.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This change is
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Tests
Chores