Conversation
- Replace BlockModel with BlockModelV3 + DataModelBuilder - Migrate args/uiState to unified data object - Replace GraphMaker component with GraphMakerPlugin - Register umapPlugin for UMAP chart - Bump graph-maker to 1.2.6, add pf-plots 1.1.67
There was a problem hiding this comment.
Code Review
This pull request refactors the "Clonotype Space" block model to utilize BlockModelV3, involving data structure migration and integration of a GraphMakerPlugin for UMAP visualization. Key changes include updating package dependencies and configurations. Critical feedback points to hardcoded local paths in pnpm.overrides which will break builds, and a bug in sequence selection validation where object comparisons are incorrectly performed. Further improvements are suggested for maintainability, such as extracting hardcoded default values into named constants and simplifying the .args() function using object destructuring. Additionally, the hardcoded 'Main' label in the UI should be addressed for better internationalization.
| "pnpm": { | ||
| "overrides": { | ||
| "@milaboratories/graph-maker": "/Users/elenaerokhina/projects/mictx/core/visualizations/packages/graph-maker/package.tgz", | ||
| "@platforma-sdk/model": "/Users/elenaerokhina/projects/mictx/core/platforma/sdk/model/package.tgz", | ||
| "@platforma-sdk/ui-vue": "/Users/elenaerokhina/projects/mictx/core/platforma/sdk/ui-vue/package.tgz" | ||
| } | ||
| }, |
There was a problem hiding this comment.
The pnpm.overrides section contains hardcoded local paths specific to a user's machine. This will break the build for other developers and in CI/CD environments. This section must be removed before merging. If you need to use local packages for development, consider using pnpm link or a similar mechanism that doesn't involve checking in user-specific paths into version control.
| const validValues = new Set(filteredOptions.map((option: any) => option.value)); | ||
|
|
||
| // Check if current selection contains invalid values (from previous dataset) | ||
| const currentSelection = app.model.args.sequencesRef; | ||
| const currentSelection = app.model.data.sequencesRef; | ||
| const hasInvalidValues = currentSelection.some((value: string) => !validValues.has(value)); |
There was a problem hiding this comment.
There appears to be a bug in how invalid sequence selections are checked. currentSelection is of type SUniversalPColumnId[] (an array of objects), but in the .some() callback, value is incorrectly typed as string. Furthermore, validValues is a Set of objects, and using has() will perform a reference check, which is not the intended behavior. To correctly check for value equality, you should compare stringified versions of the objects.
const validValues = new Set(filteredOptions.map((option: any) => JSON.stringify(option.value)));
// Check if current selection contains invalid values (from previous dataset)
const currentSelection = app.model.data.sequencesRef;
const hasInvalidValues = currentSelection.some((value) => !validValues.has(JSON.stringify(value)));
| .init(() => ({ | ||
| defaultBlockLabel: getDefaultBlockLabel({ | ||
| sequenceLabels: [], | ||
| umap_neighbors: 15, | ||
| umap_min_dist: 0.5, | ||
| }), | ||
| customBlockLabel: '', | ||
| sequenceType: 'aminoacid', | ||
| sequencesRef: [], | ||
| umap_neighbors: 15, | ||
| umap_min_dist: 0.5, | ||
| mem: 64, | ||
| cpu: 8, | ||
| alignmentModel: {}, | ||
| })); |
There was a problem hiding this comment.
The init function for the data model contains several hardcoded default values (e.g., for umap_neighbors, umap_min_dist, mem, cpu). To improve maintainability and readability, it's recommended to extract these magic numbers into named constants. This makes it easier to understand their purpose and to update them in one place if needed.
For example:
const DEFAULT_UMAP_NEIGHBORS = 15;
const DEFAULT_UMAP_MIN_DIST = 0.5;
const DEFAULT_MEM = 64;
const DEFAULT_CPU = 8;
// ... in dataModel builder
.init(() => ({
defaultBlockLabel: getDefaultBlockLabel({
sequenceLabels: [],
umap_neighbors: DEFAULT_UMAP_NEIGHBORS,
umap_min_dist: DEFAULT_UMAP_MIN_DIST,
}),
// ...
umap_neighbors: DEFAULT_UMAP_NEIGHBORS,
umap_min_dist: DEFAULT_UMAP_MIN_DIST,
mem: DEFAULT_MEM,
cpu: DEFAULT_CPU,
// ...
}));| .args((data) => { | ||
| if (!data.inputAnchor) throw new Error('Dataset is required'); | ||
| if (!data.sequencesRef.length) throw new Error('Sequence columns are required'); | ||
| if (data.umap_neighbors === undefined) throw new Error('Neighbors is required'); | ||
| if (data.umap_min_dist === undefined) throw new Error('Minimum distance is required'); | ||
| if (data.mem === undefined) throw new Error('Memory is required'); | ||
| if (data.cpu === undefined) throw new Error('CPU is required'); | ||
| return { | ||
| inputAnchor: data.inputAnchor, | ||
| sequencesRef: data.sequencesRef, | ||
| sequenceType: data.sequenceType, | ||
| umap_neighbors: data.umap_neighbors, | ||
| umap_min_dist: data.umap_min_dist, | ||
| cpu: data.cpu, | ||
| mem: data.mem, | ||
| defaultBlockLabel: data.defaultBlockLabel, | ||
| customBlockLabel: data.customBlockLabel, | ||
| }; | ||
| }) |
There was a problem hiding this comment.
The .args() function can be simplified. The validation checks are fine, but the returned object is unnecessarily verbose. Since you are returning almost all properties from data except for alignmentModel, you can use object destructuring with rest properties to make it more concise.
.args((data) => {
if (!data.inputAnchor) throw new Error('Dataset is required');
if (!data.sequencesRef.length) throw new Error('Sequence columns are required');
if (data.umap_neighbors === undefined) throw new Error('Neighbors is required');
if (data.umap_min_dist === undefined) throw new Error('Minimum distance is required');
if (data.mem === undefined) throw new Error('Memory is required');
if (data.cpu === undefined) throw new Error('CPU is required');
const { alignmentModel: _, ...args } = data;
return args;
})|
|
||
| .sections((_ctx) => ([ | ||
| { type: 'link', href: '/', label: strings.titles.main }, | ||
| { type: 'link', href: '/', label: 'Main' }, |
There was a problem hiding this comment.
The label for the section link is hardcoded as 'Main'. Previously, this was sourced from @milaboratories/strings (strings.titles.main), which is better for maintainability and internationalization (i18n). While the strings dependency has been removed, hardcoding UI strings directly in the model is not ideal. Consider using a centralized place for UI strings if one exists, or re-introducing a simple mechanism for it.
| import { getDefaultBlockLabel } from './label'; | ||
|
|
||
| export type BlockArgs = { | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
please remove, every time AI generates separators in different format, better just do not use them at all
No description provided.