Conversation
🦋 Changeset detectedLatest commit: 670c595 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
![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 the publicData feature for plugins, allowing developers to expose specific plugin data fields as reactive properties through getters or lenses. The implementation includes updates to the SDK model, factory, and builder classes to support the new PublicData generic, along with logic in the Vue integration to handle reactive state reconciliation. A review comment identifies an opportunity to improve type safety in the PluginModelBuilder by removing redundant type assertions and avoiding the use of any.
| return new PluginModelBuilder<Data, Params, Outputs, Config, Versions, NewPD>({ | ||
| name: this.name, | ||
| dataFn: this.dataFn, | ||
| getDefaultDataFn: this.getDefaultDataFn, | ||
| featureFlags: this.featureFlags, | ||
| outputs: this.outputs as any, | ||
| outputFlags: this.outputFlags, | ||
| publicDataDef: def as PublicDataDef<Data, NewPD>, | ||
| }); |
There was a problem hiding this comment.
The outputs property is being cast to any, which reduces type safety. Since the Outputs generic parameter isn't changing when creating the new PluginModelBuilder instance, this.outputs should be assignable directly without a cast. If a cast is truly necessary due to a subtle type inference issue, a more specific assertion should be used instead of any.
Additionally, the cast on publicDataDef is redundant as the type of def already matches PublicDataDef<Data, NewPD> and can be removed.
return new PluginModelBuilder<Data, Params, Outputs, Config, Versions, NewPD>({
name: this.name,
dataFn: this.dataFn,
getDefaultDataFn: this.getDefaultDataFn,
featureFlags: this.featureFlags,
outputs: this.outputs,
outputFlags: this.outputFlags,
publicDataDef: def,
});
There was a problem hiding this comment.
outputs: this.outputs as any, - please fix
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const publicData = reactive( | ||
| Object.fromEntries( | ||
| Object.entries(rawDef).map(([key, fieldDef]) => { | ||
| const isLens = typeof fieldDef === "object"; | ||
| const getter = isLens | ||
| ? (fieldDef as { get: (d: unknown) => unknown }).get | ||
| : (fieldDef as (d: unknown) => unknown); | ||
| const setter = isLens | ||
| ? (fieldDef as { set?: (d: unknown, v: unknown) => void }).set | ||
| : undefined; | ||
| const ref = setter | ||
| ? computed({ | ||
| get: () => { | ||
| const data = pluginModel.data; | ||
| return data != null ? getter(data) : undefined; | ||
| }, | ||
| set: (v: unknown) => { | ||
| const data = pluginModel.data; | ||
| if (data != null) setter(data, v); | ||
| }, | ||
| }) | ||
| : computed(() => { | ||
| const data = pluginModel.data; | ||
| return data != null ? getter(data) : undefined; | ||
| }); | ||
| return [key, ref]; | ||
| }), | ||
| ), | ||
| ) as InferFactoryPublicData<F>; |
There was a problem hiding this comment.
reactive() wraps computed refs – setter silently discarded on null data
When pluginModel.data is null (plugin not yet initialized), calling a writable publicData setter does nothing (if (data != null) setter(data, v) silently no-ops). This is intentional in the getter branch, but for the setter the mutation is lost forever — Vue won't retry it and no error is raised. Callers have no way to know the write was dropped.
Consider throwing or queuing the write instead of silently dropping it:
set: (v: unknown) => {
const data = pluginModel.data;
if (data == null) throw new Error(`publicData setter called before plugin data is initialized`);
setter(data, v);
},Or at minimum document the behaviour so callers know a write before data is ready is silently discarded.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/ui-vue/src/internal/createAppV3.ts
Line: 395-423
Comment:
**`reactive()` wraps computed refs – setter silently discarded on null data**
When `pluginModel.data` is `null` (plugin not yet initialized), calling a writable `publicData` setter does nothing (`if (data != null) setter(data, v)` silently no-ops). This is intentional in the getter branch, but for the setter the mutation is lost forever — Vue won't retry it and no error is raised. Callers have no way to know the write was dropped.
Consider throwing or queuing the write instead of silently dropping it:
```ts
set: (v: unknown) => {
const data = pluginModel.data;
if (data == null) throw new Error(`publicData setter called before plugin data is initialized`);
setter(data, v);
},
```
Or at minimum document the behaviour so callers know a write before data is ready is silently discarded.
How can I resolve this? If you propose a fix, please make it concise.| const plugins = Object.fromEntries( | ||
| platforma.blockModelInfo.pluginIds.map((id) => [id, { handle: id }]), | ||
| platforma.blockModelInfo.pluginIds.map((id) => { | ||
| const { publicData } = pluginAccess.getOrCreatePluginState(id as PluginHandle); | ||
| return [id, { handle: id as PluginHandle, publicData }]; | ||
| }), | ||
| ) as InferPluginHandles<Plugins>; |
There was a problem hiding this comment.
Previously, the plugins map was built with just { handle: id } — plugin states (including watchIgnorable watchers and computed refs) were created lazily on first call to usePlugin(). This PR changes that: every plugin in pluginIds now has its full internal state (reactive model, deep watcher, computed publicData refs) created during createAppV3 initialization, even if the plugin is never accessed.
The deep watcher on pluginModel.data will now fire on data changes for all plugins, not only those the block's UI actually uses. In a block with many plugins this could cause unnecessary updatePluginData round-trips.
Consider keeping the lazy path and instead computing publicData only when accessed, or at least documenting that state is now created eagerly so future maintainers are aware of the change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/ui-vue/src/internal/createAppV3.ts
Line: 449-454
Comment:
**Eager plugin state initialization changes prior lazy behaviour**
Previously, the `plugins` map was built with just `{ handle: id }` — plugin states (including `watchIgnorable` watchers and computed refs) were created lazily on first call to `usePlugin()`. This PR changes that: every plugin in `pluginIds` now has its full internal state (reactive model, deep watcher, computed `publicData` refs) created during `createAppV3` initialization, even if the plugin is never accessed.
The deep watcher on `pluginModel.data` will now fire on data changes for all plugins, not only those the block's UI actually uses. In a block with many plugins this could cause unnecessary `updatePluginData` round-trips.
Consider keeping the lazy path and instead computing `publicData` only when accessed, or at least documenting that state is now created eagerly so future maintainers are aware of the change.
How can I resolve this? If you propose a fix, please make it concise.| const publicData = reactive( | ||
| Object.fromEntries( | ||
| Object.entries(rawDef).map(([key, fieldDef]) => { | ||
| const isLens = typeof fieldDef === "object"; |
There was a problem hiding this comment.
typeof null === "object" can cause a silent runtime error
typeof fieldDef === "object" returns true for null in JavaScript. If null somehow ends up as a value in rawDef (e.g. JSON-deserialized pluginPublicData over a network or a bad cast), getter would be assigned null.get, causing a "Cannot read properties of null" runtime error with no helpful message.
| const isLens = typeof fieldDef === "object"; | |
| const isLens = fieldDef !== null && typeof fieldDef === "object"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/ui-vue/src/internal/createAppV3.ts
Line: 398
Comment:
**`typeof null === "object"` can cause a silent runtime error**
`typeof fieldDef === "object"` returns `true` for `null` in JavaScript. If `null` somehow ends up as a value in `rawDef` (e.g. JSON-deserialized `pluginPublicData` over a network or a bad cast), `getter` would be assigned `null.get`, causing a "Cannot read properties of null" runtime error with no helpful message.
```suggestion
const isLens = fieldDef !== null && typeof fieldDef === "object";
```
How can I resolve this? If you propose a fix, please make it concise.
| * zoom: { get: (d) => d.zoom, set: (d, v) => { d.zoom = v } }, // primitive — lens | ||
| * }) | ||
| */ | ||
| publicData<NewPD extends Record<string, unknown>>(def: { |
There was a problem hiding this comment.
don't use one letter short names
| { deep: true }, | ||
| ); | ||
|
|
||
| const rawDef = platforma.blockModelInfo.pluginPublicData[handle as string] ?? {}; |
There was a problem hiding this comment.
pluginPublicData have incorrect type, it's reason why u need a cast for handle
| ); | ||
|
|
||
| const rawDef = platforma.blockModelInfo.pluginPublicData[handle as string] ?? {}; | ||
| const publicData = reactive( |
There was a problem hiding this comment.
reactive is unnecessary construction
| ? (fieldDef as { set?: (d: unknown, v: unknown) => void }).set | ||
| : undefined; | ||
| const ref = setter | ||
| ? computed({ |
There was a problem hiding this comment.
I strictly against versus public data setter
| Outputs extends PluginOutputs, | ||
| Config extends PluginConfig, | ||
| Versions extends Record<string, unknown> = {}, | ||
| PublicData extends Record<string, unknown> = Record<string, unknown>, |
There was a problem hiding this comment.
add PluginPublicData and reuse it here
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1534 +/- ##
==========================================
- Coverage 54.59% 54.46% -0.14%
==========================================
Files 250 250
Lines 14463 14467 +4
Branches 2998 2998
==========================================
- Hits 7896 7879 -17
- Misses 5564 5588 +24
+ Partials 1003 1000 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const existing = pluginStates.get(handle); | ||
| if (existing) { | ||
| return existing as unknown as PluginState<InferFactoryData<F>, InferFactoryOutputs<F>>; | ||
| return existing as unknown as PluginState< |
There was a problem hiding this comment.
why we need this cast?
There was a problem hiding this comment.
Add tests for you case, it would be great to see, how this api will work
3 participants
Greptile Summary
This PR introduces a
publicDatafeature for plugins, allowing plugin authors to declaratively expose fields from their internaldataobject as a typed, reactive surface accessible onapp.plugins[id].publicDataand throughusePlugin(). The implementation threads a newPublicDatageneric through the entire plugin type stack (PluginFactoryLike→PluginFactory→PluginModel→PluginRecord→InferPluginHandles) and wires the runtime increateAppV3using Vue computed refs wrapped insidereactive().Key changes:
sdk/model: NewPublicFieldDef/PublicDataDeftypes; fluent.publicData(def)builder step onPluginModelBuilder;publicDataDefpropagated throughPluginModelFactoryand serialized intoBlockModelInfo.pluginPublicData.sdk/ui-vue:createAppV3readsblockModelInfo.pluginPublicData, creates a getter-only or getter/setter computed ref per field, and wraps them in areactive()object so Vue auto-unwraps them at access sites. The result is placed onPluginState.publicDatareturned byusePlugin()and pre-populated in theapp.pluginsmap.pluginPublicData: {}— no new tests exercise the getter/setter/lens paths or the reactive update flow for the new feature.Confidence Score: 5/5
Safe to merge; all findings are P2 quality-of-life concerns that do not block correctness on the primary path.
The core type machinery is carefully constructed and consistent throughout the stack. The Vue reactive pattern (computed refs inside
reactive()) is idiomatic Vue 3 and correct. The three flagged issues are all P2: a defensivenullguard on a code path that TypeScript already prevents at compile time, a silent no-op setter that matches how the getter already behaves, and an eager-vs-lazy initialization change that is benign in practice since the watcher won't fire unless data actually mutates.sdk/ui-vue/src/internal/createAppV3.ts — contains all three flagged points; the eager plugin-state initialization deserves a deliberate sign-off before merge.
Important Files Changed
publicDatavia computed refs wrapped inreactive(); three issues flagged — null-safety oftypeof fieldDef === "object"check, silent no-op setter when data is null, and eager state creation for all plugins changing previous lazy behaviour.PublicFieldDef/PublicDataDeftypes and a fluent.publicData()builder step toPluginModelBuilder; propagatesPublicDatageneric throughPluginModel,PluginInstance, andPluginModelFactory. Changes look correct and consistent.pluginPublicDatafield toBlockModelInfo; extendsInferPluginHandlesto includereadonly publicData: PublicDataand forwards the new generic inInferPluginData/InferPluginHandles. Clean and correct.PublicDatatoPluginFactoryLikephantom and exportsInferFactoryPublicDataextractor. Straightforward addition, consistent with existing pattern forData/Params/Outputs.PublicDatageneric toPluginStateand exposesreadonly publicData: PublicDatafield. Minimal and correct.PPublicDatageneric toplugin()method andPluginRecord; populatespluginPublicDatainBlockModelInfousingpublicDataDef ?? {}. Logic is correct.pluginPublicData: {}to the legacy V2 block'sBlockModelInfo. Correct empty placeholder since V2 has no plugins.PublicFieldDefandPublicDataDeffrom the package.pluginPublicData: {}to all existing test fixtures; no new tests exercise the actualpublicDatareactive behaviour or the lens/getter distinction.pluginPublicData: {}to V2 test fixtures to keep them compatible with the updated type signature.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "add changeset" | Re-trigger Greptile
(4/5) You can add custom instructions or style guidelines for the agent here!