Extensions: Compatibility guarantee - #1732
Conversation
Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesMeshery extension contract
Lazy date-picker loading
Package declaration export
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actors/mesheryExtensionContract.ts`:
- Around line 167-170: Update isMesheryExtensionEvent to validate each known
event’s data payload in addition to its type discriminant before returning true,
rejecting malformed payloads such as null data while preserving valid event
narrowing. Add tests covering malformed known-event payloads and incompatible
schema shapes.
- Around line 248-308: Update InjectedCapabilityReport and
reportInjectedCapabilities to track a contract-version mismatch when a supplied
contractVersion differs from the extension’s declared version, while treating an
absent version as the supported legacy case. Use the existing contract-version
symbol and ensure isInjectedCapabilityReportSatisfied plus
describeInjectedCapabilityReport include mismatches, preserving the satisfied
result for matching and absent legacy values. Add coverage for matching,
mismatched, and absent versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8341478-f0fb-4839-828a-27402c8642e7
📒 Files selected for processing (6)
src/__testing__/mesheryExtensionContract.test.tssrc/actors/index.tssrc/actors/mesheryExtensionContract.tssrc/base/DateTimePicker/DateTimePicker.tsxsrc/custom/permissions.tsxsrc/index.tsx
|
@rishiraj38, please note the changes pertaining to permissions. |
…tract version isMesheryExtensionEvent narrowed on the type discriminant alone, so a known literal carrying a renamed or absent payload passed the guard and subscribers read undefined off it - the same silent skew the literal map exists to prevent. It now checks each event's required payload fields via a table the union completeness-guards, while tolerating extra fields so additive changes stay compatible. contractVersion was documented as a loud handshake but only ever excluded from 'unrecognized', never compared. reportInjectedCapabilities now reports a contractVersionMismatch, which fails satisfaction and is named in the diagnostic. An absent version stays the supported legacy case: hosts deployed before the handshake predate the field rather than disagreeing about it. Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
| * were built against and surface a loud, actionable mismatch instead of failing | ||
| * feature-by-feature at runtime. | ||
| */ | ||
| export const MESHERY_EXTENSION_CONTRACT_VERSION = 1; |
There was a problem hiding this comment.
Alignment question: this contract is declared in the design system, not in meshery/schemas.
This is the core of the review question. The module is a genuine improvement over hand-duplicated literals, but it establishes sistent as the source of truth for a cross-repo service contract, which cuts against the stated architecture:
meshery/schemasalready owns permission keys end to end:build/permissions.csv→build/generate-permissions-ts.js→typescript/permissions.ts(+models/permissions/permissions.go), fingerprinted byPERMISSIONS_INDEX_IDand versioned inpermissions_history/. That is a generated, hashed, multi-language artifact.- It also already has a
capabilityconstruct (schemas/constructs/v1beta1/capability/capability.yaml, with generated Go and TS). - Sistent's role in that split is the design system - UX principles and UI components - and it already models this correctly elsewhere: every file under
src/schemas/is a one-line re-export with a "source-of-truth" comment (export { ModelImportRjsfSchemaV1Beta2 as default } from '@meshery/schemas'), andpermissions.tsxitself sourcesKeyfrom@meshery/schemas/permissions.
MESHERY_EXTENSION_CONTRACT_VERSION is a hand-incremented integer in a TS file; MESHERY_EXTENSION_CAPABILITIES is a hand-maintained array. I diffed the array against Meshery's actual injectProps in ui/components/layout/Navigator/NavigatorExtension.tsx and it is exactly right today (all 30 keys, plus resolver and CapabilitiesRegistryClass correctly in the deprecated table). But nothing keeps it right: the truth lives in a useMemo in another repo, and drift in the direction that matters most - Meshery adds a capability - is silent here.
Concrete direction, which would let this same module survive unchanged as a re-export:
- Land the event/capability contract as a construct in
meshery/schemas(constructs/v1beta1/extension-contract/), generating TS and Go so Meshery Server and Cloud can validate the same names. - Have the generator emit
MESHERY_EXTENSION_CAPABILITIES/MESHERY_EXTENSION_EVENTand the contract version, so a bump is a schema change with a history entry rather than an edit anyone can forget. - Reduce this file to
export { ... } from '@meshery/schemas/extensionContract', matching thesrc/schemas/pattern already established in this repo.
Not blocking the PR - the compile-time safety here is real and worth having now. But it should land as a follow-up before a second consumer starts deriving from sistent, because the migration cost grows with every repo that imports from here instead of from schemas.
|
|
||
| export type MissingPermissionReason = { | ||
| type: 'MISSING_PERMISSION'; | ||
| data: { keyId: string }; |
There was a problem hiding this comment.
keyId is typed string, dropping the branded PermissionKey that meshery/schemas exists to provide.
@meshery/schemas/permissions defines PermissionKey as string & { readonly __brand: 'PermissionKey' } precisely so a permission identifier cannot be confused with an arbitrary string, and Key.id is typed as it. This file - which is where the permission event is now declared - flattens it back to string.
Failure it allows: a publisher emits { type: 'MISSING_PERMISSION', data: { keyId: 'create-design' } } (a human-readable slug rather than the key UUID). It compiles on both sides. The subscriber in Meshery/Kanvas looks it up in the generated Keys table, finds nothing, and renders a shield with no key metadata - the same silent-no-op class of bug this module was written to eliminate, one level down.
Directly relevant to the schemas-as-single-source-of-truth objective: an event payload that carries a permission key should be typed by the package that generates permission keys.
Not auto-fixed - typing it strictly is a small cascade, not a one-liner, and it changes an exported type:
import type { PermissionKey } from '@meshery/schemas/permissions';
export type MissingPermissionReason = {
type: 'MISSING_PERMISSION';
data: { keyId: PermissionKey };
};createCanShow currently builds keyId from Key?.id || Key?.action || '', which widens to string, so the legacy action/subject fallbacks in HasKeyProps need to be retired (or narrowed) in the same change. Worth doing deliberately rather than as a review edit.
| * heard of, which is exactly the host/extension skew that used to present as a | ||
| * silently dead feature. | ||
| */ | ||
| export const isMesheryExtensionEvent = (event: unknown): event is MesheryExtensionEvent => |
There was a problem hiding this comment.
isMesheryExtensionEvent narrows to MesheryExtensionEvent while only checking type - the data guarantee is fabricated.
Every member of the union carries a data object, so narrowing to the union promises the caller that event.data exists. The predicate never looks at it.
Failure: a host built against a newer contract publishes { type: 'MERGE_DESIGN' } with a renamed payload field, or any malformed event with a valid type. The guard returns true, TypeScript narrows to MergeDesignEvent, and the subscriber's event.data.id throws TypeError: Cannot read properties of undefined. That is strictly worse than no guard: the crash now happens inside the branch the developer believed was validated, and the doc above tells subscribers to treat false as the defect signal - so the one case that does reach them unvalidated is the one they were told was safe.
Fixed - the predicate now also requires data to be a non-null object, with tests for { type: MERGE_DESIGN } and data: null. Note this is still a shallow check; per-member payload validation would need a schema, which is another argument for generating this contract from meshery/schemas.
| * outright missing capability is a broken contract. | ||
| */ | ||
| export const isInjectedCapabilityReportSatisfied = (report: InjectedCapabilityReport): boolean => | ||
| report.missing.length === 0 && report.missingHooks.length === 0; |
There was a problem hiding this comment.
Confirming @coderabbitai's finding, and it runs deeper than the version comparison.
contractVersion is declared in MESHERY_EXTENSION_HANDSHAKE_FIELDS, excluded from unrecognized, and then never read. So the module's own promise - "extensions compare it against the value they were built against and surface a loud, actionable mismatch" - is not implemented anywhere: a host advertising contractVersion: 7 produces a fully "satisfied" report and describeInjectedCapabilityReport() returns null.
The second half is more damaging. The header says additive changes need no version bump, but reportInjectedCapabilities reports a newly-added capability as missing, isInjectedCapabilityReportSatisfied returns false, and the message then asserts:
The host and this extension bundle were built against different contract versions
In the additive case both sides report the same version. The most common real failure sends the maintainer to diff two identical numbers, and the message is confidently wrong about the cause.
Fixed:
InjectedCapabilityReportgainshostContractVersionandcontractVersionMismatch: { host, extension } | null.- Absent version = the legacy case (hosts predating the handshake, which today is all of them - Meshery's
NavigatorExtensioninjects nocontractVersion) and is not faulted. - A differing version, or a non-numeric one, is a mismatch and fails
isInjectedCapabilityReportSatisfied. describeInjectedCapabilityReportnow states the cause it actually knows: "renamed or removed between those versions" on a real mismatch, "additive key the host has not shipped yet" when the versions agree.
Covered by five new tests: matching, absent/legacy, mismatched, malformed ('v1'), and the same-version-but-missing-key case that used to be misreported.
There was a problem hiding this comment.
@marblom007, agreed—this addresses both failure modes cleanly. In particular, distinguishing actual version skew from same-version missing additive capabilities makes the diagnostic actionable, while treating an absent version as a legacy host case preserves compatibility.
The added matching, legacy, mismatched, malformed, and same-version/missing-key tests cover the important contract paths. Thanks for the thorough fix.
🐇🔎
✏️ Learnings added
Learnt from: marblom007
Repo: layer5io/sistent PR: 1732
File: src/actors/mesheryExtensionContract.ts:308-308
Timestamp: 2026-07-22T04:19:28.907Z
Learning: In `src/actors/mesheryExtensionContract.ts`, `reportInjectedCapabilities` must treat an absent `injectProps.contractVersion` as a supported legacy-host case. A present non-numeric version or a numeric version differing from `MESHERY_EXTENSION_CONTRACT_VERSION` is a contract-version mismatch and must cause `isInjectedCapabilityReportSatisfied` to return false. Diagnostics must distinguish rename/removal version mismatches from missing capabilities introduced additively when host and extension versions match.
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.
| MergeDesign: 'MERGE_DESIGN', | ||
| DispatchToMesheryStore: 'DISPATCH_TO_MESHERY_STORE', | ||
| FeatureRequiresUserAccount: 'FeatureRequiresUserAccount', | ||
| MissingPermission: 'MISSING_PERMISSION', |
There was a problem hiding this comment.
Sistent's own publish site still used the raw string literal this map exists to replace.
MESHERY_EXTENSION_EVENT is introduced so "a rename here breaks compilation at every call site in every repo" - but the single place sistent publishes onto this bus, createCanShow in src/custom/permissions.tsx, still built the event with an inline literal:
const reason = predicateRes?.[1] || {
type: 'MISSING_PERMISSION', // <- not derived from the map
data: { keyId: actionString }
};Renaming MissingPermission in the map would leave this publishing the old literal, compiling cleanly, and every subscriber's event.type === MESHERY_EXTENSION_EVENT.MissingPermission would quietly stop matching - the exact OPEN_DESIGN_IN_KANVAS failure mode, reintroduced by the module meant to prevent it. The object literal was also untyped, so nothing forced it to be a ReasonEvent at all.
Fixed: annotated const reason: ReasonEvent and switched to MESHERY_EXTENSION_EVENT.MissingPermission.
| MesheryExtensionEventType, | ||
| (typeof MESHERY_EXTENSION_EVENT)[keyof typeof MESHERY_EXTENSION_EVENT] | ||
| >; | ||
| const _everyEventIsMapped: UnmappedEventType extends never ? true : never = true; |
There was a problem hiding this comment.
The completeness guard ships dead code to every consumer, and does not actually assert what it looks like it asserts.
const _everyEventIsMapped: UnmappedEventType extends never ? true : never = true;
void _everyEventIsMapped;Two problems:
- It emits at runtime. A
constplus avoidexpression survive intodist/index.jsanddist/index.mjsfor a check the compiler finished before emit. Type-level assertions should cost nothing. - The conditional form is unsound as a guard.
neveris assignable to every type, soX extends never ? true : neveryieldsneveron failure - andconst x: never = truedoes error, so this one happens to work; but the idiom is a foot-gun that silently passes in the very similarExpect<T extends true>shape people reach for next.
Fixed with a type-only assertion that fails on the constraint instead:
type AssertNever<T extends never> = T;
export type EveryEventIsMapped = AssertNever<UnmappedEventType>;Adding a member to MesheryExtensionEvent without adding it to MESHERY_EXTENSION_EVENT now fails to compile, with zero runtime output.
| * The host is expected to keep supplying these until the corresponding bundles | ||
| * age out; dropping one is a breaking contract change. | ||
| */ | ||
| export const MESHERY_EXTENSION_DEPRECATED_CAPABILITIES = { |
There was a problem hiding this comment.
The deprecation table's doc comment is factually wrong about the current host, which will mislead the next person to read it.
The host is expected to keep supplying these until the corresponding bundles age out; dropping one is a breaking contract change.
Checked against Meshery's actual injection site (ui/components/layout/Navigator/NavigatorExtension.tsx): the host supplies CapabilitiesRegistryClass and resolver, but it does not supply capabilitiesRegistry - that key is already gone. So the comment asserts an invariant the host is currently violating, which is how a maintainer ends up "fixing" Meshery to re-add a key nobody wants back.
Fixed - the comment now describes what the table is for (naming the replacement when a host does still supply a retired key, and preventing a retired key from being re-added to MESHERY_EXTENSION_CAPABILITIES) and notes explicitly which of the three the host still injects.
| deprecated: Object.entries(MESHERY_EXTENSION_DEPRECATED_CAPABILITIES) | ||
| .filter(([key]) => provided[key] !== undefined) | ||
| .map(([provided_, replacement]) => ({ provided: provided_, replacement })), | ||
| unrecognized: providedKeys.filter(key => !known.has(key)) |
There was a problem hiding this comment.
Drift detection is asymmetric: unknown top-level keys are reported, unknown nested hooks are not.
unrecognized is computed from Object.keys(provided), so a newer host adding injectProps.someNewThing is surfaced. But three of the host's capabilities live inside injectProps.hooks (CAN, useDynamicComponent, useFilterK8sContexts), and a newer host adding hooks.useSomethingNew produces an empty report.
That is the wrong way round for the stated purpose. The doc says an unrecognized key "is the signal that this bundle predates the host and may be missing behaviour the host expects it to have" - and a new hook is exactly the kind of thing a host adds and then starts depending on. The report goes quiet precisely where it is most needed.
Fixed - added unrecognizedHooks to InjectedCapabilityReport, populated only when the hooks bag is present (mirroring the existing, correct missingHooks behaviour so an absent bag is not double-reported). Informational, like unrecognized, so it does not fail the satisfaction check. Two tests added.
| * Importing it at module scope contradicted that: this module is reached from the | ||
| * package barrel, so merely writing `import { anything } from '@sistent/sistent'` | ||
| * executed a top-level require of the optional package and threw | ||
| * `Cannot find module '@mui/x-date-pickers/AdapterDateFns'` for anyone without |
There was a problem hiding this comment.
This fix is correct, but it is only half the problem - date-fns is also an optional peer and is still imported eagerly.
The diagnosis in this comment is exactly right, and I verified the @mui/x-date-pickers half is genuinely fixed (the built bundle now contains only import("@mui/x-date-pickers/..."), no top-level require). But src/custom/UniversalFilter.tsx:3 does:
import { subDays, subMonths, subYears } from 'date-fns';date-fns is declared optional in peerDependenciesMeta alongside @mui/x-date-pickers, and UniversalFilter is reached from the barrel via export * from './custom'. So the failure described here survives, through the other peer. Verified against the artifacts built from this branch:
$ grep -c 'require("date-fns")' dist/index.js # 1 (top-level, CJS)
$ grep -c 'from"date-fns"' dist/index.mjs # 1 (top-level, ESM)
and end-to-end, blocking both optional peers from module resolution and requiring the built barrel:
PR HEAD: FAIL barrel threw: Cannot find module 'date-fns'
with fix: OK barrel imported without the optional peers
So on this branch import { Button } from '@sistent/sistent' still dies before anything renders, in exactly the Kanvas clean-install scenario described above.
Fixed - the three calls are trivial calendar arithmetic, so they now come from a new dependency-free leaf module src/utils/date.utils.ts, matching date-fns semantics including end-of-month clamping (subtractMonths(May 31, 3) is Feb 28, not Mar 3 - the naive setMonth() gets this wrong and would silently widen a "last 3 months" filter by three days). That removes date-fns from the runtime graph entirely rather than deferring it, which is better than lazy-loading here since the arithmetic is ~15 lines.
Guarded against recurrence by src/__testing__/optionalPeerDependencies.test.ts, which walks every non-test module under src/ and fails on any eager import of either optional peer. Verified it distinguishes the bug from the fix: it flags import ... from 'date-fns' and require('date-fns'), and allows await import(...) and type-only imports. It would have caught both halves of this.
| import('@mui/x-date-pickers/AdapterDateFns'), | ||
| import('@mui/x-date-pickers/LocalizationProvider'), | ||
| import('@mui/x-date-pickers/DateTimePicker') | ||
| ]); |
There was a problem hiding this comment.
Deferring the import also defers the failure, into a place with no actionable message.
The stated motivation is that the old error pointed at sistent rather than at the missing peer. But moving it into React.lazy does not fix that - it makes it worse in one respect: the module-not-found now surfaces mid-render from inside a rejected promise, propagating to whatever error boundary happens to be above <UniversalFilter>, with no import statement anywhere near the stack. A consumer who genuinely did not install the optional peer gets an async component crash instead of a load-time error, and still no sentence telling them what to install.
Fixed - the Promise.all now has a .catch that rethrows with the remediation, preserving the original via cause:
<DateTimePicker> requires the optional peer dependencies @mui/x-date-pickers and date-fns.
Install them, or do not render <DateTimePicker> / <UniversalFilter dateRange>.
Two things worth flagging that I did not change, since both are judgement calls for you:
fallback={null}in a form field. The reasoning in the comment is sound for a warm chunk, but on a cold first open of theUniversalFilterdate range the two pickers pop in after a network round-trip, shifting the panel. Worth a fixed-height placeholder rather thannullif you have seen it flash.- Dynamic
import()in the CJS build. esbuild emits a nativeimport()insidedist/index.js. Node handles it, bundlers handle it, but Jest's default (non---experimental-vm-modules) runtime cannot execute a dynamic import in a CJS module - so a consumer test that actually renders aDateTimePickermay now fail where it previously passed. Since the motivating bug was Kanvas Jest suites, that trade is worth confirming against a real Kanvas run before merge.
| // extension contract and are declared there, so the host's typed bus and every | ||
| // extension's subscriber derive them from one place. Re-exported here to keep the | ||
| // long-standing `@sistent/sistent` import paths working. | ||
| export type { MissingCapabilityReason, MissingPermissionReason }; |
There was a problem hiding this comment.
Adopting the contract's prescribed bus type breaks createCanShow at compile time - the one integration point this PR calls out.
The module doc instructs every host to declare its bus as EventBus<MesheryExtensionEvent>, and src/index.tsx now exports createCanShow explicitly so that "its eventBus argument stops being variance-checked - the one place a host hands its event bus to sistent."
Once it is variance-checked, it does not compile. EventBus<T> is invariant in T (it accepts T in publish and yields T from on/onAny), so EventBus<MesheryExtensionEvent> is not assignable to EventBus<ReasonEvent>. Reproduced with tsc against this branch:
error TS2322: Type 'EventBus<MesheryExtensionEvent>' is not assignable to type 'EventBus<ReasonEvent>'.
Types of parameters 'value' and 'value' are incompatible.
Type 'MesheryExtensionEvent' is not assignable to type 'ReasonEvent'.
Type 'K8sContextsUpdatedEvent' is not assignable to type 'MissingCapabilityReason'.
So the moment Meshery follows the instruction and types mesheryEventBus, createCanShow(getRegistry, CAN, () => mesheryEventBus) fails to build. Today it only works because the bare new EventBus() collapses to its constraint - i.e. it compiles because of the bug this PR is fixing. This would have landed as a green sistent CI and a red Meshery build.
Fixed - createCanShow now takes the narrowest thing it actually uses, which is variance-correct by construction:
export type ReasonEventPublisher = { publish: (event: ReasonEvent) => void };A publisher that accepts the whole contract union trivially accepts a reason event, so EventBus<MesheryExtensionEvent> satisfies it. Verified the full host call site compiles after the change. ReasonEventPublisher is exported from the barrel.
| // worse than a missing type: consumers still resolve it at runtime, so the import | ||
| // silently degrades to `any` and its `eventBus` argument stops being | ||
| // variance-checked - the one place a host hands its event bus to sistent. | ||
| export { createCanShow } from './custom/permissions'; |
There was a problem hiding this comment.
createCanShow is re-exported without its parameter and return types, so the any this comment is fixing comes straight back.
The reasoning is right, but the fix is incomplete: createCanShow returns a component taking HasKeyProps<ReasonEvent>, and takes an event-bus accessor. None of HasKeyProps, ReasonEvent, or InvertAction are re-exported here, and they reach the entry only through the nested ./custom barrel - the same rollup-plugin-dts drop this block exists to work around. A consumer that wants to wrap the returned component, or type an invert_action array, cannot name any of those types and falls back to any at the boundary.
Fixed:
export {
createCanShow,
type HasKeyProps,
type InvertAction,
type ReasonEvent,
type ReasonEventPublisher
} from './custom/permissions';Confirmed present in the built dist/index.d.ts.
Broader point, since this is now the fifth copy of this comment block in one file: the workaround is being applied per-symbol as each one is discovered by a downstream consumer, which means the next one is also found in production. Worth either pinning the rollup-plugin-dts bug and fixing it once, or switching the declaration build to a mode that does not drop nested re-exports - the current approach guarantees the failure keeps recurring.
| hooks: Object.fromEntries(MESHERY_EXTENSION_HOOKS.map(hook => [hook, () => null])) | ||
| }); | ||
|
|
||
| describe('Meshery extension contract — event literals', () => { |
There was a problem hiding this comment.
The type-only contract gets 175 lines of tests; the actual runtime bug fix in this PR gets none.
Per the repo's own standard that tests ship with the change: this suite is genuinely good, but it exercises a module that is almost entirely compile-time. The DateTimePicker change is the part of this PR that fixes a real, reported, runtime failure - "in Kanvas that took out four unrelated Jest suites on a clean install" - and nothing here would catch its regression, or catch the same bug appearing in a different module.
That is not hypothetical: the same bug is live in this PR via date-fns in UniversalFilter (see the DateTimePicker.tsx thread). A test asserting the property rather than the component would have caught both.
Added src/__testing__/optionalPeerDependencies.test.ts, which walks every non-test module under src/ and fails if any of them eagerly imports an optional peer, listing the offending paths. It correctly ignores await import(...) and type-only imports, and separately asserts that both peers are still marked optional in package.json so the test's premise cannot silently change.
Also added src/__testing__/date.utils.test.ts for the replacement date helpers, covering the end-of-month and leap-year clamping that a naive implementation gets wrong.
|
@pontusringblom @rishiraj38 - to be precise about what this PR does and does not do to permissions, since #1731 touches Behaviour is unchanged. They now live in The reason they moved: One thing worth knowing for #1731: No change to |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Review: alignment with the permissions refactoring and schemas-as-single-source-of-truthReviewed at extra-high effort. 15 findings, 13 posted inline; the two below have no anchor in the diff. Direct answer to the questionWhere it aligns. The diagnosis is correct and valuable. Hand-duplicated string literals across sistent / meshery / meshery-extensions are exactly how Where it does not. It makes sistent the source of truth for a cross-repo service contract, which is the schemas repo's job:
The tell is that sistent already models this correctly: every file under I diffed Suggested direction (follow-up, not a blocker): land the contract as a construct in Two findings without an inline anchor1. console.log('cant perform action : reason', reason, eventBus);Fires in production on every click of any permission-gated control the user lacks, and logs the event-bus accessor alongside it. Pre-existing, but in a file this PR touches. Fixed - removed; the event is published on the bus, which is the actual reporting channel. 2. import { Key } from '@meshery/schemas/permissions';
export { cloudApi } from '@meshery/schemas/cloudApi';A consumer without Fixes applied
Not fixed, deliberately
|
…nostic The closing sentence asserted host and bundle were built against different contract revisions even when the only finding was a missing capability at a matching advertised version, sending the reader to compare two numbers that agree. A known version mismatch is already named in the listed parts. Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
`@mui/x-date-pickers` and `date-fns` are declared optional under
`peerDependenciesMeta`, so a consumer is entitled to skip them. But
`src/index.tsx` re-exports nearly everything, which means a module-scope
import of an optional peer in ANY barrel-reachable file breaks that promise
for everyone: `import { Button } from '@sistent/sistent'` throws
`Cannot find module 'date-fns'` before a single component renders, and the
message names sistent rather than the peer the consumer deliberately did not
install.
layer5io#1732 fixed one instance of this class by deferring `@mui/x-date-pickers` to
`React.lazy`. `UniversalFilter` was still importing `subDays`/`subMonths`/
`subYears` from `date-fns` at module scope for its quick-range presets, so
the barrel remained unusable on a clean install (layer5io#1735). Meshery UI happens
to have `date-fns` installed and never saw it; consumers that do not, do.
The presets now use `src/utils/date.utils.ts`, a new dependency-free leaf
module. It is deliberately NOT folded into `time.utils.tsx`, which drags in
`moment` and a React component - pure date arithmetic should be reachable
without a rendering stack behind it. Semantics match `date-fns` exactly,
including end-of-month clamping (`subtractMonths(May 31, 3)` is Feb 28, not
Mar 3) and local-time day arithmetic so a range spanning a DST boundary does
not drift by an hour.
`optionalPeerDependencies.test.ts` guards the whole bug class rather than
this one component: it walks the source tree and fails on any eager import
of an optional peer, listing the offending paths so the failure message is
also the remediation. It also asserts the peers are still marked optional,
so if that premise ever changes the assertions get revisited rather than
silently kept.
Also carried in this change:
- `DateTimePicker` now wraps its lazy peer import so a missing peer surfaces
as a message naming the peer, instead of a raw module-not-found thrown
mid-render from inside a promise. Deferring the resolution deferred the
failure too; an unactionable async crash is no improvement on an
unactionable sync one.
- `time.utils.tsx` imports `CustomTooltip` from its leaf path instead of the
`../custom` barrel, breaking a `custom/X -> utils -> custom/index -> X`
cycle and no longer forcing consumers of a date helper to load the entire
component library.
- `permissions.tsx` types `createCanShow`'s bus as a structural
`ReasonEventPublisher`. `EventBus<T>` is invariant in `T`, so the host's
`EventBus<MesheryExtensionEvent>` - the bus the contract tells every host
to declare - was not assignable to `EventBus<ReasonEvent>` and the
integration failed to compile. It also publishes via the named
`MESHERY_EXTENSION_EVENT.MissingPermission` handle so a contract rename
breaks this publish site rather than silently ceasing to match the
subscriber, and drops a stray `console.log` from the click path.
- The barrel exports `HasKeyProps`, `InvertAction`, `ReasonEvent` and
`ReasonEventPublisher` alongside `createCanShow`; without them a consumer
cannot name the types of the wrapper it builds and falls back to `any`.
Fixes layer5io#1735
Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
What this is
The single declaration of the Meshery host <-> extension contract, which Meshery UI and Kanvas both derive from instead of hand-duplicating string literals.
This is the keystone of a three-repo change:
layer5io/sistentmeshery/mesherycontractVersionlayer5labs/meshery-extensionsBoth consumers need a published
@sistent/sistentcarryingsrc/actors/mesheryExtensionContract.ts, so this lands and releases first.Why
Three things cross the host/extension boundary: the bundle's export shape, a bag of injected capabilities handed over as
injectProps, and an RxJS event bus. The last two were duplicated string literals in each repo, and the host bus was declared as a barenew EventBus()- whose type parameter collapses to its constraint, sopublish()accepted any{ type: string }.A rename on either side therefore compiled cleanly on both and failed silently at runtime: the subscriber's
if (event.type === ...)simply stopped matching and the feature became a no-op with no error, no warning, and no failing test. That is howOPEN_DESIGN_IN_KANVAS->OPEN_DESIGN_IN_EXTENSIONshipped broken, and howcapabilitiesRegistry->providerCapabilitiesshipped before it.Deriving every literal from one declaration turns that class of rename into a compile error in every consumer.
What it declares
MesheryExtensionEvent- the event union, andMesheryExtensionEventBusas the bus alias both sides must useMESHERY_EXTENSION_EVENT- named handles for every literal, with a compile-time guard so the map cannot drift behind the unionMESHERY_EXTENSION_CAPABILITIES/MESHERY_EXTENSION_HOOKS- keys the host is expected to put oninjectPropsMESHERY_EXTENSION_DEPRECATED_CAPABILITIES- keys the host still injects so already-published bundles keep mountingMESHERY_EXTENSION_CONTRACT_VERSION- the handshake valueCompile-time agreement is necessary but not sufficient
Extension bundles are published artifacts loaded into whichever host version is deployed, so host and extension are never guaranteed to be built from the same commit. No CI gate can observe that skew. The runtime helpers exist to make it loud:
isMesheryExtensionEvent- narrows a bus event, checking both the discriminant and the payload it promises to carry, so a renamed payload field is caught rather than handed to a subscriber that readsundefinedoff it. Extra fields are tolerated, so additive changes stay compatible.reportInjectedCapabilities/isInjectedCapabilityReportSatisfied/describeInjectedCapabilityReport- compare a host'sinjectPropsagainst the contract and render one actionable message naming the offending keys. Side-effect free, so the host asserts on it in a unit test (catching a prop rename before merge) and extensions run it at mount (catching deploy-time skew).A
contractVersionthe host advertises that differs from this build's fails the check. An absent version is the supported legacy case - hosts deployed before the handshake existed predate the field rather than disagreeing about it, and flagging them all at once would train maintainers to ignore the report.Why the DateTimePicker change is in this PR
Not a drive-by.
@mui/x-date-pickersis declared an optional peer dependency, butDateTimePicker.tsximported it at module scope - and that module is reachable from the package barrel. So merely writingimport { anything } from '@sistent/sistent'executed a top-level require of a package most consumers never install, throwingCannot find module '@mui/x-date-pickers/AdapterDateFns'with a message pointing at sistent rather than at the missing peer. On a clean install it took out four unrelated Kanvas Jest suites.That is precisely the import Kanvas has to make to consume the contract, so this fix is what makes the contract consumable at all. Resolving the picker on first render via
React.lazykeeps the optional dependency genuinely optional: the cost is paid only by code that actually renders one, which is whatpeerDependenciesMeta.optionalalready advertises.Verified at the bundle level - the built CJS/ESM output contains only dynamic
import()of the three picker entrypoints and no top-levelrequire.Also in here
createCanShowis now exported explicitly from the package entrypoint. It was reachable at runtime but dropped from the generated.d.tsby a nested-barrel quirk, so consumers silently gotany- which meant itseventBusargument was not variance-checked at the one place a host hands its bus to sistent. Same quirk asFeedbackButtonabove it.MissingPermissionReason/MissingCapabilityReasonmoved fromcustom/permissions.tsxinto the contract and are re-exported, so every existing import path keeps working. They moved becausePermissionShieldpublishes them onto whichever bus the caller supplies, which in Meshery is the host <-> extension bus - making them part of the contract whether or not we had declared them so.Compatibility
Additive. No existing export was removed or renamed, and no runtime behaviour changed outside
DateTimePicker's resolution timing. Per the file's own rule, additive changes need noMESHERY_EXTENSION_CONTRACT_VERSIONbump, so it stays at1.Testing
23 Jest cases in
src/__testing__/mesheryExtensionContract.test.ts, covering the wire literals (pinned by hand on purpose - a rename is only safe alongside a version bump and a deprecation entry, and this test is what forces that conversation), malformed and renamed payloads for known literals, a valid payload for every literal, extra-field tolerance, missing capabilities and hooks, deprecated aliases, unrecognized keys, and the version handshake across matching / mismatched / non-numeric / absent.Full suite green (411 tests, 22 suites),
eslint .clean,npm run buildclean.Signed-off-by: Ahmed Jamil 197998703+CodeAhmedJamil@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests