Conversation
🦋 Changeset detectedLatest commit: 059b7d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @gramkin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a major architectural upgrade to the Platforma API, moving towards a more robust and maintainable block development ecosystem. The introduction of a unified, versioned block state with built-in migration capabilities addresses long-standing challenges in managing block data evolution and ensuring backward compatibility. This change significantly enhances the platform's stability and developer experience by centralizing state management and providing declarative migration paths. Furthermore, the new API streamlines argument derivation and optimizes staging execution, contributing to more efficient block processing. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant update with the new Platforma API v3. The changes are extensive, touching upon the core model, middle layer, and UI components to support a new unified state management system with BlockModelV3, state migrations, and argument derivation from state. The introduction of new test blocks and updates to existing ones to align with this new API is a great addition. My review focuses on ensuring the correctness, security, and maintainability of these new features. I've identified a critical issue related to a self-admitted bug in the migration logic, a few high-severity bugs that could lead to runtime errors, and several medium-severity suggestions to improve code quality and remove temporary workarounds.
…platforma into ivanov/MILAB-2913-api-v3-1
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Platforma API v3, which is a significant and well-structured refactoring. It moves from separate args and uiState to a unified state model, which simplifies state management and enables features like state migrations. The changes are extensive, touching the core model, middle layer, and UI components, and include new test blocks for the v3 API. I've provided a few suggestions for code simplification and bug fixes. Overall, this is a great improvement.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1381 +/- ##
==========================================
- Coverage 52.98% 52.82% -0.17%
==========================================
Files 236 238 +2
Lines 12952 13323 +371
Branches 2643 2737 +94
==========================================
+ Hits 6863 7038 +175
- Misses 5216 5380 +164
- Partials 873 905 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ove explicit inputsValid field
…ayload for v3 blocks
|
|
||
| wf.setPreRun(assets.importTemplate(":prerun")) | ||
|
|
||
| mapToPValueData := func(map) { |
There was a problem hiding this comment.
I do not like that we have this as a duplicate in example blocks. we should create a helper for building json pcolumns. I've made a note for myself, will do that in february
| export type MigrationFn<From, To> = (prev: Readonly<From>) => To; | ||
|
|
||
| /** Versioned data wrapper for persistence */ | ||
| export type Versioned<T> = { |
There was a problem hiding this comment.
Composition over inheritance looks better in this case
export type Data<T> = {
data: T;
};
/** Versioned data wrapper for persistence */
export type Version = {
version: number;
};
/** Result of upgrade operation, may include warning if migration failed */
export type UpgradeResult<T> = Data<T> & Version & {
warning?: string;
};
getDefaultData(): Data<State> & Version
...
upgrade(versioned: Data<State> & Version): UpgradeResult<State>
...
tryRegisterCallback('upgrade', (versioned: Data<State> & Version) => this.upgrade(versioned)); // TODO: rename
I tried it in a code, looks pretty good and more readable
| const startIndex = fromVersion - 1; | ||
| const migrationsToApply = this.steps.slice(startIndex); | ||
|
|
||
| let currentData: unknown = data; | ||
| for (let i = 0; i < migrationsToApply.length; i++) { | ||
| const stepIndex = startIndex + i; | ||
| const fromVer = stepIndex + 1; | ||
| const toVer = stepIndex + 2; |
There was a problem hiding this comment.
let currentData: unknown = data;
for (let i = fromVersion - 1; i < this.steps.length; i++) {
const fromVer = i + 1;
const toVer = i + 2;
| export type Expect<T extends true> = T; | ||
|
|
||
| export type Equal<X, Y> = | ||
| (<T>() => T extends X ? 1 : 2) extends (<T>() => T extends Y ? 1 : 2) ? true : false; | ||
|
|
||
| export type Merge<A, B> = { | ||
| [K in keyof A | keyof B]: K extends keyof B ? B[K] : K extends keyof A ? A[K] : never; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Useful generics, let's move it move suitable place (lib util, something like that)
There was a problem hiding this comment.
I’ve wanted this for a long time. Let’s discuss naming for these packages
| */ | ||
| export function createBlockStorage<TState = unknown>( | ||
| initialData: TState = {} as TState, | ||
| version: number = 1, |
There was a problem hiding this comment.
lets remove default version. In future it can help use this method correctly
| return raw as BlockStorage<TState>; | ||
| } | ||
| // Legacy format: raw is the state directly | ||
| return createBlockStorage(raw as TState, 1); |
There was a problem hiding this comment.
const INITIAL_VERSION = 1
There was a problem hiding this comment.
About that: I'll create a key-based migration (we discussed it with Vadim) in a separate pr, numbers are really not very comprehensible
| * @param data - The new data value | ||
| * @returns A new BlockStorage with updated data | ||
| */ | ||
| export function updateStorageData<TState>( |
There was a problem hiding this comment.
this is not update, more likely its createUpdatedStorage
| * @param version - The new version number | ||
| * @returns A new BlockStorage with updated version | ||
| */ | ||
| export function updateStorageDataVersion<TState>( |
There was a problem hiding this comment.
same, it's not updating, it's creating new entity
| /** Version of the block data, used for migrations */ | ||
| __dataVersion: number; | ||
| /** The block's user-facing data (state) */ | ||
| __data: TState; |
There was a problem hiding this comment.
what for we use double underscore in fields naming?
| /** Version of the block data, used for migrations */ | ||
| __dataVersion: number; | ||
| /** The block's user-facing data (state) */ | ||
| __data: TState; |
There was a problem hiding this comment.
I strongly believe that default data storage should be implemented as a plugin.
It generalize common approach and will be good example/first touch for plugins.
…nArgs to prerunArgs
| /** The normalized BlockStorage object */ | ||
| storage: BlockStorage; | ||
| /** The extracted data (what developers see) */ | ||
| data: unknown; |
There was a problem hiding this comment.
Why data field exist? data always equal storage.__data
| return `${blockId}-${fieldName}`; | ||
| } | ||
|
|
||
| export async function awaitBlockDone(prj: Project, blockId: string, timeout: number = 5000) { |
There was a problem hiding this comment.
please use existing helper if possible
platforma/sdk/test/src/test-block.ts
Line 138 in d2e5604
|
|
||
| // Normalize storage to get current data and version | ||
| const { storage: currentStorage, data: currentData } = normalizeStorage(currentStorageJson); | ||
| const currentVersion = currentStorage.__dataVersion; |
There was a problem hiding this comment.
getStorageDataVersion
| ...currentStorage, | ||
| __dataVersion: version, | ||
| __data: data, | ||
| }); |
There was a problem hiding this comment.
Let's wrap any modification of storage to pub methods(you already have updateStorageDataVersion/updateStorageData)
| }; | ||
|
|
||
| // Get the upgrade callback (registered by Migrator.registerCallbacks()) | ||
| const upgradeCallback = ctx.callbackRegistry['upgrade'] as ((v: { version: number; data: unknown }) => UpgradeResult) | undefined; |
There was a problem hiding this comment.
ctx.callbackRegistry['upgrade'] - let's we use enum or const for callbacks names, It will make transparent where we are using and registering callbacks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
No description provided.