Conversation
🦋 Changeset detectedLatest commit: 6b68910 The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 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, 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 presents a detailed architectural plan for implementing a unified service provider within the application. The plan introduces a declarative system for defining and consuming versioned services, allowing blocks and plugins to specify their dependencies explicitly. It outlines the creation of dedicated service registries for both model and UI components, incorporating lazy loading for UI services and a robust IPC mechanism for remote Node.js services. The design also includes comprehensive compatibility checks for service availability and a clear migration path, ensuring a smooth transition from the current unconditional driver injection model to a more modular and maintainable service-oriented architecture. Highlights
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. 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 comprehensive design plan for a unified service provider, aiming to replace the current unconditional driver injection with a structured, versioned service definition and registry system. The plan details how services will be defined, declared as requirements by blocks and plugins, provisioned on both model and UI sides (with lazy loading for UI services), and how compatibility will be handled. Key feedback includes suggestions to improve the robustness of the _registeredIds set against HMR, enhance the lazyMap proxy with additional traps for standard object operations, clarify the wrapResult function in the service router, provide a concrete implementation sketch for ServiceRegistry.get, and detail the mechanism for tracking and rejecting in-flight IPC calls from evicted UI components.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
1 similar comment
vadimpiven
force-pushed
the
feat/services
branch
from
01747e5 to
475ef76
Compare
1 similar comment
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1528 +/- ##
==========================================
- Coverage 54.55% 54.32% -0.24%
==========================================
Files 250 254 +4
Lines 14467 14525 +58
Branches 2999 3004 +5
==========================================
- Hits 7893 7891 -2
- Misses 5572 5633 +61
+ Partials 1002 1001 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Address review findings: abort listener cleanup, lazy proxy getOwnPropertyDescriptor, ServiceCallAbortedError definition, block:dispose ordering, test reset utility, terminology pass.
ServiceId (branded string, PluginHandle pattern), ServiceRegistry, ServiceInjector for VM injection, BlockServiceContext for Ui-side dispose, lazyMap for lazy instantiation, full model-side and Ui-side injection paths, backward compatibility, per-file changes.
- Split service code into focused modules: service_declarations, service_core, service_registry, service_capabilities, block_services, service_resolve, service_bridge - Extract service factory registrations to dedicated files (service_factories.ts) - Remove legacy SpecDriver from computable_context, migrate to service system - Restore accidentally removed createPFrame/createPTable/createPTableV2 VM exports - Add unsupported valueType filtering in SpecDriver.createSpecFrame - Rename ModelServiceDrivers/UiServiceDrivers to ModelServices/UiServices - Tighten UiServices type constraint to Partial<UiServices> - Add BlockDefaultUiServices with proper type derivation through PlatformaV3 - Add block-level service tests (model outputs + UI indicators) - Add registerServiceCapabilities helper for runtime capability registration - Add unit tests for service infrastructure (19 tests) - Add compile-time type assertions for service access restrictions - Fix createAppV3.test.ts mock to include serviceDispatch - Pass blockServiceNames to BlockRenderCtx in all block output lambdas
…OD_MAP Extract createUiServiceInjectors, resolveUiInjector, SERVICE_METHOD_MAP, and buildServiceInfo from desktop app into pl-model-common. Method names are auto-derived at module load from the injector factory shape, eliminating the need for runtime worker calls or caching.
…e, this binding - Add Disposable to ColumnCollectionImpl and AnchoredColumnCollectionImpl to dispose spec frames and prevent WASM resource leaks - Warn on unsupported valueType in SpecDriver instead of silent drop - Use Map.has() instead of truthiness check in ServiceRegistryBase - Preserve this binding in render/api.ts service dispatch
- Add dispose() method and Disposable interface to ColumnCollection and AnchoredColumnCollection to prevent spec frame WASM resource leaks - Use 'using' in createPlDataTableV3 for automatic cleanup - Revert console.warn on unsupported valueType (filtering is by design)
The test doesn't run the workflow, so calculationStatus is NotCalculated. Use awaitStableValue() on block state instead of awaitBlockDone() which requires calculationStatus === Done. Accept createPlDataTable error (no result pool data in test environment).
Logging now happens before the actual operation so that the log entry is visible even when the call crashes.
…lias, remove dead import
Both RefCountPoolBase and RefCountManualPoolBase iterated envelopes but checked isDisposable on the envelope object rather than envelope.resource, so inner resources were never disposed on pool teardown.
Old blocks use the pl7.app/parents annotation (axis names) to express parent-child relationships, but the Rust/WASM engine only reads numeric parentAxes indices. This caused linker columns like pl7.app/sc/cellLinker to fail with "must have exactly 2 connected components, got 3". Add resolveAnnotationParents() helper in pl-model-common that pipes axesSpec through getNormalizedAxesList + getDenormalizedAxesList to convert annotation-based parents to numeric indices. Apply it in: - pf-spec-driver createSpecFrame (spec-only WASM path) - pf-driver createPFrame (data WASM path) - pf-driver createPTableV2 (spec frame for query evaluation)
- createSpecFrame returns PoolEntry<SpecFrameHandle> with idempotent
unref instead of bare handle; disposeSpecFrame removed
- Injector returns VM object {key, unref} and registers addOnDestroy
for automatic cleanup on computable destruction
- Add PoolEntry/PoolEntryGuard to pl-model-common for cross-package
pool lifecycle management
- Add requireComputableCtx getter, migrate all computableCtx guards
- Migrate createPFrame/createPTable/createPTableV2 to PoolEntryGuard
- Add ServiceRegistryBase.dispose() for proper service cleanup
- Migrate PFramePool from RefCountManualPoolBase to RefCountPoolBase;
delete RefCountManualPoolBase (no remaining consumers)
- SpecDriver implements AsyncDisposable (pool is async)
oxfmt extends does not merge ignorePatterns — local overrides replace the base config entirely. Add coverage/ to all .oxfmtrc.json files so test coverage artifacts are never checked for formatting.
vadimpiven
force-pushed
the
feat/services
branch
from
10eb815 to
62e5287
Compare
Iterator helpers are an ES2024 feature not available in QuickJS. Revert to Array.from() with mapping function.
| .map((key) => Services[key]); | ||
| } | ||
|
|
||
| export type KnownServiceName = (typeof Services)[keyof typeof Services] & string; |
There was a problem hiding this comment.
Let's do ServiceName = (typeof Services)[keyof typeof Services] & string;
and remove brand type and KnownServiceName
3 participants
Greptile Summary
This PR introduces a services subsystem for Platforma V3 blocks, providing typed access to WASM services (e.g.,
PFrameSpecviaSpecDriver) and node services (e.g.,PFrameproxied over IPC). The implementation is well-architected across several layers: service declaration (service_declarations.ts), a shared type and registry layer (pl-model-common), QuickJS VM injection for model scripts (service_injectors.ts,computable_context.ts), and UI-side lazy service composition (service_bridge.ts,createAppV3.ts).Key design decisions implemented cleanly relative to the previously-reviewed plan:
Servicesobject inservice_declarations.tsdrives all auto-derived types (ServiceRequireFlags,SERVICE_CAPABILITY_FLAGS,SERVICE_METHOD_MAP), eliminating the string-literal duplication risks noted in plan-phase reviews.ServiceInjectorMap(model/QuickJS side) andUiServiceInjectorMap(UI side) are compile-time exhaustive — missing a service after adding it toServicesis a type error.ServiceRegistryBase.getById()usesMap.has()(not=== undefined) to cache singleton instances, and the UI-sidebuildServicesuses an explicitfetchedflag set before the factory call to prevent retry on error.getMethodNamesusesObject.getOwnPropertyDescriptorto walk the prototype chain without triggering getters.satisfiesused correctly:BLOCK_SERVICE_FLAGSusessatisfiesrather than a direct annotation, preserving the literal type needed byResolveModelServices.One issue found:
resolveUiInjectorinlib/model/common/src/services/service_injectors.tsreturnsundefinedinstead of the declarednullfor WASM services (see inline comment).Confidence Score: 5/5
Safe to merge — implementation is solid, well-tested, and all plan-phase concerns have been addressed in the actual code.
The only finding is a P2 type-contract mismatch in resolveUiInjector (returns undefined for WASM services instead of null). All prior P0/P1 concerns from the design review have been correctly addressed: getters are not triggered during introspection, Map.has() is used for singleton caching, satisfies is used instead of widening annotations, and the injector maps are compile-time exhaustive. No runtime bugs or data-integrity issues remain.
lib/model/common/src/services/service_injectors.ts — resolveUiInjector return type mismatch for WASM services.
Important Files Changed
Comments Outside Diff (3)
sdk/model/src/columns/column_collection_builder.ts, line 232-243 (link)ColumnCollectionImplcallsthis.specDriver.createSpecFrame(...)directly on thePFrameSpecDriver. The oldComputableContextHelper.createSpecFrame()wrapper registered anaddOnDestroycleanup callback; that wrapper is removed. EverycreatePlDataTableV3call accumulates aPFrameWasmV2in the long-livedSpecDriver.framesMap without ever freeing it.Prompt To Fix With AI
lib/node/ts-helpers/src/ref_count_pool.ts, line 123-135 (link)dispose()iterates envelopes, never disposes inner resourcesthis.resources.values()yieldsRefCountEnvelope<R>objects ({ refCount, resource }), not the actualRinstances. BothisDisposable(resource)andisAsyncDisposable(resource)check the envelope, which never hasSymbol.disposeorSymbol.asyncDispose, so neither branch is ever taken. Every resource managed by the pool silently leaks on disposal.lib/node/ts-helpers/src/ref_count_manual_pool.ts, line 70-81 (link)dispose()checks the envelope, not the inner resourcethis.resources.values()yieldsRefCountEnvelope<R>objects. The variable nameresourceis misleading — it is the envelope ({ refCount, resource }), not the contained resource.isDisposable(resource)will never be true because envelopes don't implementSymbol.dispose, so all managed resources leak on pool disposal.The
release()method gets this right by doingconst resource = envelope.resourcebefore checking;dispose()should follow the same pattern:Prompt To Fix All With AI
Reviews (14): Last reviewed commit: "Add coverage to oxfmt ignorePatterns acr..." | Re-trigger Greptile