Conversation
📝 WalkthroughWalkthroughThe change adds surface appearance schemas and validation, surface model declarations, generated schema coverage, plugin documentation, and host handling for model discovery and appearance data. ChangesSurface capabilities
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Existing plugins can fail to compile against the updated API, and a plugin-provided model can prevent initialization. These compatibility and availability issues should be resolved before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/base/src/__tests__/surface-appearance.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/base/src/surface-appearance-schema.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). packages/host/src/__tests__/plugin.models.spec.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Shapes bloom where surfaces meet 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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: fda5bb2a-de03-40b8-8fa3-bf800b21d485
📒 Files selected for processing (23)
packages/base/README.mdpackages/base/assets/surface-appearance.schema.jsonpackages/base/src/__tests__/schema-generation.spec.tspackages/base/src/__tests__/surface-appearance.spec.tspackages/base/src/__tests__/surface-model.spec.tspackages/base/src/main.tspackages/base/src/manifest.tspackages/base/src/schema-error.tspackages/base/src/surface-api/index.tspackages/base/src/surface-api/models.tspackages/base/src/surface-api/plugin.tspackages/base/src/surface-api/types.tspackages/base/src/surface-appearance-schema.tspackages/base/src/surface-appearance.tspackages/base/src/surface-layout.tspackages/base/src/surface-model.tspackages/base/tools/generate-schemas.mjspackages/host/src/__tests__/plugin.appearance.spec.tspackages/host/src/__tests__/plugin.models.spec.tspackages/host/src/context.tspackages/host/src/main.tspackages/host/src/plugin.tspackages/host/src/types.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| // Clean up | ||
| } | ||
|
|
||
| async getSurfaceModels(): Promise<SurfaceModelDefinition[]> { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Import SurfaceModelDefinition in this example.
The added method references a type that the example does not import. Copying this example causes Cannot find name 'SurfaceModelDefinition'.
Suggested correction
-import { SurfacePlugin, SurfaceContext } from '`@companion-surface/base`'
+import { SurfacePlugin, SurfaceContext, type SurfaceModelDefinition } from '`@companion-surface/base`'| * | ||
| * @param context Information about the host, for building the definitions against | ||
| */ | ||
| getSurfaceModels(context: SurfaceModelsContext): Promise<SurfaceModelDefinition[]> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep getSurfaceModels optional for existing plugins.
SurfacePlugin is an externally implemented interface. A required method breaks compilation for plugins that implement the previous contract.
Please declare getSurfaceModels? and treat an absent method as an empty model list in PluginWrapper. This preserves the stated compatibility with older modules and avoids logging their expected absence as a failure.
Based on learnings: introduce new plugin capabilities through an opt-in method so third-party implementations of the original contract remain compatible.
Proposed interface change
- getSurfaceModels(context: SurfaceModelsContext): Promise<SurfaceModelDefinition[]>
+ getSurfaceModels?(context: SurfaceModelsContext): Promise<SurfaceModelDefinition[]>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getSurfaceModels(context: SurfaceModelsContext): Promise<SurfaceModelDefinition[]> | |
| getSurfaceModels?(context: SurfaceModelsContext): Promise<SurfaceModelDefinition[]> |
Source: Learnings
| this.#surfaceModels = valid | ||
|
|
||
| // Cloned so that a plugin cannot mutate what the host is holding, as with configFields | ||
| this.#host.surfaceModelsChanged?.(structuredClone(valid)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep model cloning inside the non-fatal boundary.
validateSurfaceModelDefinition validates the original object and discards the parsed schema result. An enumerable function-valued property therefore remains on a model with valid required fields. structuredClone(valid) then throws DataCloneError.
The clone runs outside the per-model try block, and PluginWrapper.init() awaits #loadSurfaceModels() without catching this error. Initialization can reject.
Clone or canonicalize each model inside its per-model try block, then add the cloned model to valid. Discard only models that cannot be cloned.
6d591e0 to
56a07e3
Compare
The same formatValidationError was copied into both manifest.ts and surface-layout.ts. Move it to schema-error.ts so a third validator has one to use rather than a third copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161mZMVRSUR82w2mTXfpnM7
56a07e3 to
ca83a90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 15a0df2b-6238-49d1-b090-a2ddf8c3b39c
📒 Files selected for processing (16)
packages/base/README.mdpackages/base/assets/surface-appearance.schema.jsonpackages/base/src/__tests__/surface-appearance.spec.tspackages/base/src/__tests__/surface-model.spec.tspackages/base/src/surface-api/models.tspackages/base/src/surface-api/types.tspackages/base/src/surface-appearance-schema.tspackages/base/src/surface-appearance.tspackages/base/src/surface-model.tspackages/host/src/__tests__/plugin.appearance.spec.tspackages/host/src/__tests__/plugin.models.spec.tspackages/host/src/__tests__/surfaceProxy.leds.spec.tspackages/host/src/__tests__/surfaceProxy.rotate.spec.tspackages/host/src/main.tspackages/host/src/plugin.tspackages/host/src/types.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| * describing every one of them. Set to null if this surface has no face to describe, in which | ||
| * case it is drawn from geometry derived out of its layout instead. | ||
| */ | ||
| surfaceAppearance: SurfaceAppearanceDefinition | null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep surfaceAppearance optional for existing plugins.
This required member breaks existing SurfaceRegisterProps object literals at compile time. The host already treats an omitted value as null with surfaceAppearance ?? null.
Please make the property optional. This preserves source compatibility while new plugins can provide an appearance.
Proposed compatibility fix
- surfaceAppearance: SurfaceAppearanceDefinition | null
+ surfaceAppearance?: SurfaceAppearanceDefinition | null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| surfaceAppearance: SurfaceAppearanceDefinition | null | |
| surfaceAppearance?: SurfaceAppearanceDefinition | null |
ca83a90 to
c122238
Compare
A layout says what controls a surface has and what to draw on them; it says nothing about where they are, so anything drawing a picture of the device has to guess. An appearance says it: a bounding box, artwork, and a rectangle and shape per control, keyed by the same control ids. bodyColor is required because it is the one thing that is useful with or without artwork - it fills the face when there is none, and says what the device looks like when there is, so a black device can be told apart from a black page behind it. Nothing consumes it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161mZMVRSUR82w2mTXfpnM7
Required with an explicit null rather than optional, so that a module author has to decide their surface has no face to describe instead of never noticing they could describe one. A malformed appearance, or one which misses a control the layout has, is dropped with a warning and the surface still opens: how a device looks may not stop it working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161mZMVRSUR82w2mTXfpnM7
c122238 to
be6653c
Compare
A layout is only learnt when a surface connects, so a device cannot be laid out before it is plugged in. getSurfaceModels asks the plugin what it drives, once, after init. A method rather than a callback because a method is the version a module author has to confront when they upgrade. The wrapper holds what it gets, for its own caller to relay onwards. No callback to the host: the list is fetched once and cannot change, so anything named for a change would be advertising something that never happens. A model which does not validate is dropped on its own, and a plugin which throws when asked still finishes initialising. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161mZMVRSUR82w2mTXfpnM7
be6653c to
baa91b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 0ad842ce-7099-423d-939b-1753bc321dc6
📒 Files selected for processing (5)
packages/base/assets/surface-appearance.schema.jsonpackages/base/src/__tests__/surface-appearance.spec.tspackages/base/src/surface-appearance-schema.tspackages/host/src/__tests__/plugin.models.spec.tspackages/host/src/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host/src/plugin.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| export const MAX_BODY_IMAGE_LENGTH = 512 * 1024 | ||
|
|
||
| /** Inline only - nothing downstream shares a filesystem with the module. */ | ||
| const BODY_IMAGE_REGEX = /^data:image\/(svg\+xml|png|webp);base64,[A-Za-z0-9+/]+={0,2}$/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,125p' packages/base/src/surface-appearance-schema.ts
sed -n '1,120p' packages/base/assets/surface-appearance.schema.json
sed -n '150,205p' packages/base/src/__tests__/surface-appearance.spec.ts
sed -n '300,350p' packages/host/src/plugin.tsRepository: bitfocus/companion-surface-api
Length of output: 14163
🏁 Script executed:
set -eu
rg -n -C 3 "validateSurfaceAppearance|BODY_IMAGE_REGEX|surface-appearance.schema.json|A=" packages/base packages/hostRepository: bitfocus/companion-surface-api
Length of output: 22415
Validate Base64 quantum grouping. An otherwise valid appearance containing data:image/png;base64,A= passes BODY_IMAGE_REGEX and validateSurfaceAppearance, so the host can retain it instead of applying the invalid-appearance fallback. The generated JSON schema has the same gap. Tighten the pattern, regenerate the schema, and add a regression test for this input.
-const BODY_IMAGE_REGEX = /^data:image\/(svg\+xml|png|webp);base64,[A-Za-z0-9+/]+={0,2}$/
+const BODY_IMAGE_REGEX =
+ /^data:image\/(svg\+xml|png|webp);base64,(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)$/2 participants
The intention here is to allow Companion to draw a fairly accurate representation of the surface, including overlaying any interactive controls.
This includes expecting modules to list their supported models and their visual representations, so that users can test previewing in different surface types
Summary by CodeRabbit