Repository navigation
Add built-in Brand Bug / Watermark plugin - #355
Conversation
github-actions
Bot
added
documentation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughChangesBrand Bug plugin
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🧹 Nitpick comments (6)
packages/manifest-schema/schema-v11.json (1)
110-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
anyOfaccepts an emptytext, so a contentless mark validates.
required: ["text"]is satisfied by"text": "". Tightening the branch keeps the schema as strict as the server projection.- "anyOf": [{ "required": ["imageAssetId"] }, { "required": ["text"] }], + "anyOf": [ + { "required": ["imageAssetId"] }, + { "required": ["text"], "properties": { "text": { "minLength": 1 } } } + ],🤖 Prompt for 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. In `@packages/manifest-schema/schema-v11.json` around lines 110 - 118, Update the text branch of the mark schema’s anyOf validation to require a non-empty text value, aligning it with the server projection while preserving the imageAssetId branch and existing text length limit.apps/dashboard/src/pages/PluginsPage.tsx (1)
193-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate the three target queries on the selected scope.
All three lists are fetched on every editor mount even when
scope === "all"discards them. The previous countdown-bar code only queried the selected scope.♻️ Proposed refactor
- const screens = useQuery({ queryKey: ["screens"], queryFn: api.screens }); + const screens = useQuery({ + queryKey: ["screens"], + queryFn: api.screens, + enabled: scope === "screens", + }); const groups = useQuery({ queryKey: ["screen-groups"], queryFn: () => api.screenGroups(), + enabled: scope === "sync_groups", }); const locations = useQuery({ queryKey: ["locations"], queryFn: api.locations, + enabled: scope === "locations", });🤖 Prompt for 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. In `@apps/dashboard/src/pages/PluginsPage.tsx` around lines 193 - 212, The useTargetOptions function currently fetches screens, groups, and locations regardless of the selected scope. Configure each useQuery call’s enabled condition so only the query matching scope ("screens", "sync_groups", or "locations") runs, while the "all" scope skips all three and preserves the existing option mapping.apps/dashboard/src/api/types.ts (1)
811-812: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared
PluginTargetScopealias.
targetScope/targetIdsare now repeated verbatim across plugin input types, mirroring the server's sharedvalidateTargeting. A single exported union (or aPluginTargetingfragment) keeps the four scopes defined once.🤖 Prompt for 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. In `@apps/dashboard/src/api/types.ts` around lines 811 - 812, Introduce an exported shared PluginTargetScope union (or PluginTargeting fragment) in the types module and replace the repeated targetScope/targetIds declarations across the plugin input types with it. Ensure all four existing scope values remain unchanged and the shared definition is reused consistently.apps/server/internal/plugins/brandbug_integration_test.go (1)
191-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the catalog entry by ID, not by registry position.
catalog.Items[1]couples this test to the ordering ofcatalogEntries; adding a plugin between the two entries breaks it for no functional reason. Find the item whoseID == "brand_bug"instead.🤖 Prompt for 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. In `@apps/server/internal/plugins/brandbug_integration_test.go` around lines 191 - 197, Update the catalog assertion in the integration test to locate the entry by matching its ID to "brand_bug" rather than indexing catalog.Items[1]. Keep validating the matched entry’s InstanceCount is 2, while preserving the existing catalog length and error checks.apps/server/internal/plugins/brandbug.go (1)
299-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTargeting predicate is now duplicated per plugin.
This
WHEREclause is character-for-charactercountdownBarsForScreen's predicate with the table name swapped. A shared helper that takes the instances/targets table names (both package literals, as withtargetIDsFrom) would keep the four scope semantics in one place as more built-in plugins land.🤖 Prompt for 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. In `@apps/server/internal/plugins/brandbug.go` around lines 299 - 314, The targeting predicate in brandBugsForScreen duplicates countdownBarsForScreen; extract the shared scope-filter SQL into a helper that accepts the instances and targets table names as package-controlled literals, following the existing targetIDsFrom pattern. Update both brandBugsForScreen and countdownBarsForScreen to reuse it while preserving all four targeting scopes and existing query parameters.apps/server/internal/playlists/service.go (1)
1850-1875: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFourth near-duplicate of the "resolve player-compatible image variant" query.
The query/scan in
resolveBrandBugLogoduplicates the sameasset_variants ... player_compatible=TRUE ORDER BY CASE kind ...pattern already present at the widget fallback image lookup (~line 1452), the website fallback image lookup (~line 1486), andprojectWidgetAssets(~line 1807). Worth extracting a sharedresolveImageVariant(ctx, assetID) (ManifestAsset, error)helper so the "player-compatible ready image" selection rule stays in one place as new callers (like this one) are added.♻️ Sketch of a shared helper
+func (s *Service) resolveImageVariant(ctx context.Context, assetID uuid.UUID) (ManifestAsset, error) { + var asset ManifestAsset + err := s.db.QueryRow(ctx, `SELECT v.asset_id,v.id,v.mime_type,encode(v.sha256,'hex'),v.file_size,v.width,v.height,v.duration_seconds + FROM asset_variants v JOIN assets a ON a.id=v.asset_id AND a.type='image' AND a.deleted_at IS NULL + WHERE v.asset_id=$1 AND v.deleted_at IS NULL AND v.player_compatible=TRUE + ORDER BY CASE v.kind WHEN 'playback' THEN 0 WHEN 'original' THEN 1 ELSE 2 END LIMIT 1`, assetID). + Scan(&asset.AssetID, &asset.VariantID, &asset.MIMEType, &asset.SHA256, &asset.FileSize, &asset.Width, &asset.Height, &asset.DurationSeconds) + if err == nil { + asset.DownloadPath = "/api/v1/player/assets/" + asset.AssetID.String() + "/variants/" + asset.VariantID.String() + } + return asset, err +} func (s *Service) resolveBrandBugLogo(ctx context.Context, manifest *Manifest, config *plugins.ManifestBrandBugConfig, seen map[uuid.UUID]bool) error { if config.ImageAssetID == nil { return nil } - var asset ManifestAsset - err := s.db.QueryRow(ctx, `SELECT ...`, *config.ImageAssetID).Scan(...) + asset, err := s.resolveImageVariant(ctx, *config.ImageAssetID) if err != nil { if !errors.Is(err, pgx.ErrNoRows) { return err } config.ImageAssetID = nil return nil } - asset.DownloadPath = "/api/v1/player/assets/" + asset.AssetID.String() + "/variants/" + asset.VariantID.String() variantID := asset.VariantID config.ImageVariantID = &variantID ... }🤖 Prompt for 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. In `@apps/server/internal/playlists/service.go` around lines 1850 - 1875, Extract the repeated player-compatible image variant query and scan into a shared resolveImageVariant helper returning ManifestAsset and error. Update the widget fallback lookup, website fallback lookup, projectWidgetAssets, and resolveBrandBugLogo to call this helper while preserving the existing asset selection, error handling, download-path construction, and deduplication behavior.
🤖 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 `@apps/server/internal/database/migrations/00064_brand_bug_plugin.sql`:
- Around line 10-28: Remove the row-level CHECK requiring image_asset_id or
non-empty text from the migration’s table definition, while preserving the ON
DELETE SET NULL behavior and all other constraints. The non-empty invariant
should remain enforced by the plugin service.
In `@packages/manifest-schema/README.md`:
- Line 15: Update the Brand Bug documentation to preserve its optional-logo
contract: in packages/manifest-schema/README.md at line 15, describe the logo
asset and variant as optional; in docs/plugins.md at lines 45-47, qualify
required download and activation behavior so it applies only when a logo is
configured and resolved.
---
Nitpick comments:
In `@apps/dashboard/src/api/types.ts`:
- Around line 811-812: Introduce an exported shared PluginTargetScope union (or
PluginTargeting fragment) in the types module and replace the repeated
targetScope/targetIds declarations across the plugin input types with it. Ensure
all four existing scope values remain unchanged and the shared definition is
reused consistently.
In `@apps/dashboard/src/pages/PluginsPage.tsx`:
- Around line 193-212: The useTargetOptions function currently fetches screens,
groups, and locations regardless of the selected scope. Configure each useQuery
call’s enabled condition so only the query matching scope ("screens",
"sync_groups", or "locations") runs, while the "all" scope skips all three and
preserves the existing option mapping.
In `@apps/server/internal/playlists/service.go`:
- Around line 1850-1875: Extract the repeated player-compatible image variant
query and scan into a shared resolveImageVariant helper returning ManifestAsset
and error. Update the widget fallback lookup, website fallback lookup,
projectWidgetAssets, and resolveBrandBugLogo to call this helper while
preserving the existing asset selection, error handling, download-path
construction, and deduplication behavior.
In `@apps/server/internal/plugins/brandbug_integration_test.go`:
- Around line 191-197: Update the catalog assertion in the integration test to
locate the entry by matching its ID to "brand_bug" rather than indexing
catalog.Items[1]. Keep validating the matched entry’s InstanceCount is 2, while
preserving the existing catalog length and error checks.
In `@apps/server/internal/plugins/brandbug.go`:
- Around line 299-314: The targeting predicate in brandBugsForScreen duplicates
countdownBarsForScreen; extract the shared scope-filter SQL into a helper that
accepts the instances and targets table names as package-controlled literals,
following the existing targetIDsFrom pattern. Update both brandBugsForScreen and
countdownBarsForScreen to reuse it while preserving all four targeting scopes
and existing query parameters.
In `@packages/manifest-schema/schema-v11.json`:
- Around line 110-118: Update the text branch of the mark schema’s anyOf
validation to require a non-empty text value, aligning it with the server
projection while preserving the imageAssetId branch and existing text length
limit.
🪄 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: 9d3c50b7-73a1-4176-b93b-bffec60baea9
📒 Files selected for processing (29)
README.mdapps/dashboard/src/App.tsxapps/dashboard/src/api/client.tsapps/dashboard/src/api/types.tsapps/dashboard/src/pages/PluginsPage.test.tsxapps/dashboard/src/pages/PluginsPage.tsxapps/player-linux/src/core/manifest.test.tsapps/player-linux/src/core/manifest.tsapps/player-linux/src/core/types.tsapps/player-linux/src/renderer/brand-bug-resolver.test.tsapps/player-linux/src/renderer/brand-bug-resolver.tsapps/player-linux/src/renderer/renderer.tsapps/player-linux/static/index.htmlapps/server/internal/database/migrations/00064_brand_bug_plugin.sqlapps/server/internal/httpapi/plugins.goapps/server/internal/httpapi/routes.goapps/server/internal/playlists/plugin_assets_integration_test.goapps/server/internal/playlists/service.goapps/server/internal/plugins/brandbug.goapps/server/internal/plugins/brandbug_integration_test.goapps/server/internal/plugins/brandbug_test.goapps/server/internal/plugins/service.goapps/server/internal/plugins/service_integration_test.godocs/api.mddocs/architecture.mddocs/openapi.yamldocs/plugins.mdpackages/manifest-schema/README.mdpackages/manifest-schema/schema-v11.json
| image_asset_id UUID REFERENCES assets(id) ON DELETE SET NULL, | ||
| text TEXT NOT NULL DEFAULT '' CHECK (char_length(text) <= 180), | ||
| width_percent INTEGER NOT NULL CHECK (width_percent BETWEEN 2 AND 40), | ||
| text_size_percent INTEGER NOT NULL CHECK (text_size_percent BETWEEN 1 AND 12), | ||
| opacity_percent INTEGER NOT NULL CHECK (opacity_percent BETWEEN 10 AND 100), | ||
| margin_percent INTEGER NOT NULL CHECK (margin_percent BETWEEN 0 AND 20), | ||
| text_color TEXT NOT NULL CHECK (text_color ~ '^#[0-9a-fA-F]{6}$'), | ||
| background_style TEXT NOT NULL CHECK (background_style IN ('none', 'scrim')), | ||
| starts_at TIMESTAMPTZ, | ||
| ends_at TIMESTAMPTZ, | ||
| enabled BOOLEAN NOT NULL DEFAULT TRUE, | ||
| priority INTEGER NOT NULL DEFAULT 0 CHECK (priority BETWEEN -1000 AND 1000), | ||
| target_scope TEXT NOT NULL CHECK (target_scope IN ('all', 'screens', 'sync_groups', 'locations')), | ||
| created_by UUID NOT NULL REFERENCES users(id), | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| -- A mark with neither a logo nor a caption would occupy a corner invisibly. | ||
| CHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0), | ||
| CHECK (starts_at IS NULL OR ends_at IS NULL OR ends_at > starts_at) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the service-side invariant exists and that nothing else relies on the row CHECK.
rg -n --type=go -C3 'image_asset_id' apps/server/internal/plugins
rg -n --type=go -C3 'logo|text' apps/server/internal/plugins/brandbug.go | head -60
# Manifest projection behavior for a cleared logo with empty text.
rg -n -C4 'brand_bug|brandBug' apps/server/internal/playlists/service.go | head -80Repository: Gibsonmb71/tilecast
Length of output: 5684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- migration ---'
sed -n '1,120p' apps/server/internal/database/migrations/00064_brand_bug_plugin.sql
echo '--- brandbug service validation and update/create path ---'
sed -n '1,240p' apps/server/internal/plugins/brandbug.go
echo '--- brandbug projection path ---'
sed -n '240,380p' apps/server/internal/plugins/brandbug.goRepository: Gibsonmb71/tilecast
Length of output: 15549
ON DELETE SET NULL conflicts with the row-level CHECK.
Deleting a referenced logo from a logo-only mark will null image_asset_id, trip CHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0), and abort the asset delete. The non-empty invariant already lives in the plugin service, so drop this table CHECK.
🤖 Prompt for 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.
In `@apps/server/internal/database/migrations/00064_brand_bug_plugin.sql` around
lines 10 - 28, Remove the row-level CHECK requiring image_asset_id or non-empty
text from the migration’s table definition, while preserving the ON DELETE SET
NULL behavior and all other constraints. The non-empty invariant should remain
enforced by the plugin service.
| Manifest v14 adds the optional `crossfade` playlist transition. Capable Players composite overlapping images, videos, websites, Widgets, and Layouts during the visual transition. The server projects `crossfade` as `fade` for older Players, and v1-v13 remain unchanged. | ||
|
|
||
| Manifests v11-v14 may also carry the optional typed `plugins` projection. Built-in plugins are independent of playlist and Layout selection. The first discriminator is `countdown_bar@1`; its recurrence, target, display mode, height, and priority are cached with the rest of the manifest so Players can evaluate it locally while offline. | ||
| Manifests v11-v14 may also carry the optional typed `plugins` projection. Built-in plugins are independent of playlist and Layout selection. The discriminators are `countdown_bar@1` and `brand_bug@1`. Countdown Bar's recurrence, target, display mode, height, and priority are cached with the rest of the manifest so Players can evaluate it locally while offline; Brand Bug carries its corner, resolved logo asset and variant, screen-relative sizing, and optional date window, and its logo appears in `assets` like any other required image. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the optional-logo contract consistent across documentation.
Brand Bug supports text-only instances, but both descriptions imply that every mark has a required/resolved logo.
packages/manifest-schema/README.md#L15-L15: describe the logo asset and variant as optional.docs/plugins.md#L45-L47: qualify required download and activation behavior to apply only when a logo is configured and resolved.
📍 Affects 2 files
packages/manifest-schema/README.md#L15-L15(this comment)docs/plugins.md#L45-L47
🤖 Prompt for 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.
In `@packages/manifest-schema/README.md` at line 15, Update the Brand Bug
documentation to preserve its optional-logo contract: in
packages/manifest-schema/README.md at line 15, describe the logo asset and
variant as optional; in docs/plugins.md at lines 45-47, qualify required
download and activation behavior so it applies only when a logo is configured
and resolved.
e46e0ac to
3296826
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/internal/database/migrations/00066_brand_bug_plugin.sql (1)
8-28: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
ON DELETE SET NULLstill conflicts with the row CHECK — unresolved from prior review.Deleting a referenced logo from a logo-only mark (empty
text) nullsimage_asset_idvia the FK action, which then tripsCHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0). Per Postgres semantics, referential actions likeSET NULLmust still satisfy row CHECK constraints — the implicitUPDATEfails and the whole asset delete aborts, contradicting the comment on lines 8-9 that the instance should survive as text-only. The non-empty invariant is already enforced invalidateBrandBug(Go), so this table-level CHECK is both redundant and unsafe.🐛 Proposed fix
- -- A mark with no logo and no caption would hold a corner invisibly. - CHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0), CHECK (starts_at IS NULL OR ends_at IS NULL OR ends_at > starts_at)🤖 Prompt for 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. In `@apps/server/internal/database/migrations/00066_brand_bug_plugin.sql` around lines 8 - 28, Remove the table-level CHECK requiring image_asset_id or non-empty text from the migration, while retaining the image_asset_id foreign key with ON DELETE SET NULL. Keep the other column constraints unchanged so deleting a logo can leave an empty-text, text-only row as intended; validation remains handled by validateBrandBug.
🧹 Nitpick comments (3)
apps/dashboard/src/pages/PluginsPage.tsx (2)
1063-1075: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogo picker silently caps at the first 100 ready images.
An installation with more than 100 ready images cannot select the later ones, and nothing in the UI says the list is truncated. Consider a searchable/typeahead picker or at least a note when
images.data.totalexceeds the fetched page.🤖 Prompt for 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. In `@apps/dashboard/src/pages/PluginsPage.tsx` around lines 1063 - 1075, The logo picker’s images query in the PluginsPage component only fetches the first 100 ready assets. Update the picker flow to support searching or pagination so users can access assets beyond the first page, or explicitly indicate truncation when images.data.total exceeds the fetched results; preserve the existing image-selection behavior for the returned assets.
167-176: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThree target lists are fetched even when the scope is
all.The hook returns
nullforall, yet still issues the screens, sync-group, and location queries on every editor mount. Gating each query on the scope avoids the unused round trips (cached keys keep later scope switches instant).♻️ Proposed change
- const screens = useQuery({ queryKey: ["screens"], queryFn: api.screens }); + const screens = useQuery({ + queryKey: ["screens"], + queryFn: api.screens, + enabled: scope === "screens", + }); const groups = useQuery({ queryKey: ["screen-groups"], queryFn: () => api.screenGroups(), + enabled: scope === "sync_groups", }); const locations = useQuery({ queryKey: ["locations"], queryFn: api.locations, + enabled: scope === "locations", });🤖 Prompt for 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. In `@apps/dashboard/src/pages/PluginsPage.tsx` around lines 167 - 176, Update useTargetSource so the screens, groups, and locations queries are enabled only when their corresponding target scope is active, while preserving their existing query keys for caching and later scope switches. Keep returning null for the all scope and retain each query’s current fetcher.packages/manifest-schema/schema-v11.json (1)
110-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
anyOfaccepts an emptytextas a valid mark.
{ "required": ["text"] }is satisfied by"text": "", so a config with noimageAssetIdand an empty string passes schema validation while rendering nothing. Constraining the branch keeps the "logo, text, or both" invariant enforceable at the schema level.♻️ Suggested tightening
- "anyOf": [{ "required": ["imageAssetId"] }, { "required": ["text"] }], + "anyOf": [ + { "required": ["imageAssetId"] }, + { + "required": ["text"], + "properties": { "text": { "minLength": 1 } } + } + ],🤖 Prompt for 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. In `@packages/manifest-schema/schema-v11.json` around lines 110 - 118, Update the mark schema’s anyOf branch requiring text so it also enforces a non-empty string, reusing the existing text property constraints. Preserve acceptance of imageAssetId-only marks and marks containing both imageAssetId and valid text.
🤖 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 `@docs/openapi.yaml`:
- Around line 2262-2274: Update the affected OpenAPI schema properties in the
documented section to use the OpenAPI 3.1 union form type: [string, "null"]
instead of nullable: true, preserving their existing string and format
constraints.
---
Duplicate comments:
In `@apps/server/internal/database/migrations/00066_brand_bug_plugin.sql`:
- Around line 8-28: Remove the table-level CHECK requiring image_asset_id or
non-empty text from the migration, while retaining the image_asset_id foreign
key with ON DELETE SET NULL. Keep the other column constraints unchanged so
deleting a logo can leave an empty-text, text-only row as intended; validation
remains handled by validateBrandBug.
---
Nitpick comments:
In `@apps/dashboard/src/pages/PluginsPage.tsx`:
- Around line 1063-1075: The logo picker’s images query in the PluginsPage
component only fetches the first 100 ready assets. Update the picker flow to
support searching or pagination so users can access assets beyond the first
page, or explicitly indicate truncation when images.data.total exceeds the
fetched results; preserve the existing image-selection behavior for the returned
assets.
- Around line 167-176: Update useTargetSource so the screens, groups, and
locations queries are enabled only when their corresponding target scope is
active, while preserving their existing query keys for caching and later scope
switches. Keep returning null for the all scope and retain each query’s current
fetcher.
In `@packages/manifest-schema/schema-v11.json`:
- Around line 110-118: Update the mark schema’s anyOf branch requiring text so
it also enforces a non-empty string, reusing the existing text property
constraints. Preserve acceptance of imageAssetId-only marks and marks containing
both imageAssetId and valid text.
🪄 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: 1465db50-1408-4ad3-9267-8fb10c9d7c10
📒 Files selected for processing (29)
README.mdapps/dashboard/src/App.tsxapps/dashboard/src/api/client.tsapps/dashboard/src/api/types.tsapps/dashboard/src/pages/PluginsPage.test.tsxapps/dashboard/src/pages/PluginsPage.tsxapps/player-linux/src/core/manifest.test.tsapps/player-linux/src/core/manifest.tsapps/player-linux/src/core/types.tsapps/player-linux/src/renderer/brand-bug-resolver.test.tsapps/player-linux/src/renderer/brand-bug-resolver.tsapps/player-linux/src/renderer/renderer.tsapps/player-linux/static/index.htmlapps/server/internal/database/migrations/00066_brand_bug_plugin.sqlapps/server/internal/httpapi/plugins.goapps/server/internal/httpapi/routes.goapps/server/internal/playlists/plugin_assets_integration_test.goapps/server/internal/playlists/service.goapps/server/internal/plugins/brandbug.goapps/server/internal/plugins/brandbug_integration_test.goapps/server/internal/plugins/brandbug_test.goapps/server/internal/plugins/service.goapps/server/internal/plugins/service_integration_test.godocs/api.mddocs/architecture.mddocs/openapi.yamldocs/plugins.mdpackages/manifest-schema/README.mdpackages/manifest-schema/schema-v11.json
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- docs/api.md
- docs/plugins.md
| imageAssetId: | ||
| type: [string, "null"] | ||
| format: uuid | ||
| description: A ready image in the Media library. | ||
| text: { type: string, maxLength: 180 } | ||
| widthPercent: { type: integer, minimum: 2, maximum: 40 } | ||
| textSizePercent: { type: integer, minimum: 1, maximum: 12 } | ||
| opacityPercent: { type: integer, minimum: 10, maximum: 100 } | ||
| marginPercent: { type: integer, minimum: 0, maximum: 20 } | ||
| textColor: { type: string, pattern: "^#[0-9a-fA-F]{6}$" } | ||
| backgroundStyle: { enum: [none, scrim] } | ||
| startsAt: { type: [string, "null"], format: date-time } | ||
| endsAt: { type: [string, "null"], format: date-time } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which OpenAPI version does the document declare, and which nullable styles appear?
rg -n '^openapi:' docs/openapi.yaml
rg -c 'nullable: true' docs/openapi.yaml
rg -n 'type: \[' docs/openapi.yaml | head -30Repository: Gibsonmb71/tilecast
Length of output: 423
🌐 Web query:
OpenAPI 3.1 nullable type array null supported deprecated nullable valid
💡 Result:
In OpenAPI 3.1, the nullable keyword is removed [1]. It is not merely deprecated; it is no longer part of the specification [2][3]. To specify that a value can be null, you should use the JSON Schema standard approach by providing an array of types [4][1]. The type keyword now accepts an array of strings, where "null" is included as one of the allowed types [4][5]. Example usage for a string that can be null: type: - "string" - "null" Alternatively, you can use anyOf to combine a schema with a null type, which is also a valid and standard JSON Schema approach [6][2]: anyOf: - type: "string" - type: "null" While some older tooling or legacy configurations might still react to nullable: true because it was standard in OpenAPI 3.0, it is not recognized as a valid keyword in the OpenAPI 3.1 specification, and reliance on it is discouraged or ineffective in strictly compliant environments [2][3].
Citations:
- 1: https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0
- 2: openapi 3.1 - support null types microsoft/typespec#5010
- 3: Remove "nullable" entirely OAI/OpenAPI-Specification#2246
- 4: https://learn.openapis.org/upgrading/v3.0-to-v3.1.html
- 5: https://spec.openapis.org/oas/v3.1
- 6: Clarification on nullable properties, OpenAPI 3.1 OAI/OpenAPI-Specification#3148
Replace nullable: true with type: [string, "null"] here. docs/openapi.yaml declares OpenAPI 3.1.0, so the union form is valid and nullable is the out-of-spec variant that will trip 3.1 validators.
🤖 Prompt for 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.
In `@docs/openapi.yaml` around lines 2262 - 2274, Update the affected OpenAPI
schema properties in the documented section to use the OpenAPI 3.1 union form
type: [string, "null"] instead of nullable: true, preserving their existing
string and format constraints.
Places a logo, sponsor mark, legal notice, campaign badge, or location label in any corner over all normal content. An instance carries a corner, an optional Media-library logo, optional text, screen-relative sizing, opacity, margin, text color, backing, an optional date window, enabled state, priority, and the same four targeting scopes as Countdown Bar. Sizes are percentages of the screen rather than pixels, so one instance reads the same on a 1080p panel and a 4K one: logo width against screen width, text size against screen height, margin against the shorter edge. A mark needs a logo or text; one with neither would hold a corner invisibly and is rejected at save time, in the database, and in the manifest schema. At most one mark occupies a corner — highest priority, then the stable instance ID — while different corners coexist, so a logo can hold the top right with a legal notice in the bottom left. Marks always overlay and never reflow the content stage. When a Countdown Bar is visible, bottom-corner marks are lifted by the bar's height instead of being covered by it. The plugin registry needed generalizing to hold two plugins. ManifestPlugin.Config is now the discriminated payload rather than the countdown config, the catalog is a closed table of compiled-in entries with per-plugin counts, and targeting validation is shared instead of copied. Studio's card grid maps each plugin id to its own route and icon, and both editors drive one set of target fields. Logos are real media: manifest assembly resolves the playback variant, publishes it as an ordinary manifest asset, and the Player treats it like playlist media — verified and cached before activation, so the mark keeps drawing while the network is gone. Only a ready image can be chosen, and a logo that becomes unavailable between saving and manifest build degrades to the instance's text, or is omitted if it had none; neither case fails a screen's whole manifest. Renderer corner elements are created once and updated in place, because rebuilding them on the one-second plugin tick would re-decode the logo and flicker a watermark that should look painted on.
3296826 to
cda1b41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/api/types.ts (1)
5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
contributorpermissions explicitly. Non-viewer checks expose contributor controls for deleting assets, layouts, and playlists, while contributors may author but cannot delete or publish layouts. Split these capability checks instead of usingrole !== "viewer"; server rejection alone leaves incorrect UI actions.🤖 Prompt for 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. In `@apps/dashboard/src/api/types.ts` at line 5, Update the capability checks using the role union in the permissions types so contributor access is handled explicitly rather than through role !== "viewer". Allow contributors only the authoring controls they support, and exclude them from delete and publish actions for assets, layouts, and playlists; preserve the existing permissions for owners, administrators, editors, and viewers.
♻️ Duplicate comments (1)
apps/server/internal/database/migrations/00076_brand_bug_plugin.sql (1)
10-28: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
ON DELETE SET NULLstill conflicts with the row-level CHECK.Hard-deleting an asset referenced by a logo-only mark nulls
image_asset_idand then tripsCHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0), aborting the delete. The non-empty invariant is already enforced invalidateBrandBug, so the table CHECK can be dropped.🗄️ Proposed change
- -- A mark with neither a logo nor a caption would occupy a corner invisibly. - CHECK (image_asset_id IS NOT NULL OR char_length(btrim(text)) > 0), CHECK (starts_at IS NULL OR ends_at IS NULL OR ends_at > starts_at)🤖 Prompt for 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. In `@apps/server/internal/database/migrations/00076_brand_bug_plugin.sql` around lines 10 - 28, Remove the table-level CHECK requiring image_asset_id or non-empty text from the migration, since ON DELETE SET NULL must allow asset deletion and validateBrandBug already enforces the non-empty invariant. Preserve the separate starts_at/ends_at ordering constraint and all other column constraints.
🧹 Nitpick comments (5)
apps/server/internal/plugins/service.go (2)
258-275: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
targetIDsFrominterpolates a table name. The comment is accurate today (both call sites pass package literals), but nothing enforces it. A small allowlist keeps the invariant checkable rather than conventional.♻️ Optional guard
func (s *Service) targetIDsFrom(ctx context.Context, table string, id uuid.UUID) ([]uuid.UUID, error) { + switch table { + case "countdown_bar_targets", "brand_bug_targets": + default: + return nil, fmt.Errorf("%w: unknown target table", ErrInvalid) + } rows, err := s.db.Query(ctx, `SELECT target_id FROM `+table+` WHERE instance_id=$1 ORDER BY target_id`, id)🤖 Prompt for 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. In `@apps/server/internal/plugins/service.go` around lines 258 - 275, Update targetIDsFrom to validate table against an explicit allowlist of the package’s supported table literals before building the query, returning an error for any unsupported value; preserve the existing query and row-processing behavior for allowed tables.
541-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfig kinds are inconsistent across projections. Countdown and ticker entries carry value configs (
ManifestCountdownConfig,ManifestAlertTickerConfig), whilebrandBugsForScreencarries*ManifestBrandBugConfig. Every downstream consumer must now type-assert both forms per plugin type; the assembly stage inapps/server/internal/playlists/service.goonly mutates the Brand Bug pointer today. Consider standardising on pointers for all three so a future projection stage doesn't silently fail a value assertion.🤖 Prompt for 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. In `@apps/server/internal/plugins/service.go` around lines 541 - 561, The ManifestForScreen projections use inconsistent config shapes: countdowns and tickers return value configs while brand bugs return a pointer. Update the countdown and ticker projection methods, countdownBarsForScreen and alertTickersForScreen, to return pointers to ManifestCountdownConfig and ManifestAlertTickerConfig, matching brandBugsForScreen and preserving the existing config contents and manifest assembly.apps/server/internal/plugins/service_integration_test.go (1)
265-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
firstConfighelper; no callers remain in the package.🤖 Prompt for 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. In `@apps/server/internal/plugins/service_integration_test.go` around lines 265 - 272, Remove the unused firstConfig helper from the test package, including its implementation and associated comment, since no callers remain.apps/server/internal/plugins/brandbug_test.go (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a local zero UUID value in the test case.
uuid.Nilis a mutable package-level variable; usevar zero uuid.UUIDto keep the test isolated.🤖 Prompt for 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. In `@apps/server/internal/plugins/brandbug_test.go` at line 61, Update the "nil asset" test case in the BrandBugInput test to use a local zero UUID variable declared as var zero uuid.UUID, and assign its address to ImageAssetID instead of referencing the mutable package-level uuid.Nil.apps/player-linux/static/index.html (1)
233-241: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBrand Bug overlay shares an identical z-index with the alert ticker.
#brand-bugsand#alert-tickerboth usez-index: 26; today's correct stacking (marks above the ticker) only holds because#brand-bugshappens to come later in the DOM. The nearby comment only documents intent relative to the countdown bar, not the ticker. Making the z-index explicitly higher removes the implicit DOM-order dependency.♻️ Proposed fix
`#brand-bugs` { position: absolute; inset: 0; - z-index: 26; + z-index: 27; pointer-events: none; }🤖 Prompt for 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. In `@apps/player-linux/static/index.html` around lines 233 - 241, Update the `#brand-bugs` stacking rule to use a z-index higher than `#alert-ticker`, preserving its position above the countdown bar and removing reliance on DOM order. Leave its positioning and pointer-events behavior unchanged.
🤖 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 `@apps/dashboard/src/pages/PluginsPage.tsx`:
- Around line 1071-1083: Update the logo-selection flow around the images query
so it does not silently imply that all ready image assets are available: show a
hint when images.data.total exceeds the loaded page size. Ensure the currently
stored imageAssetId is retained as an option even when it is outside the first
100 results, so the Select displays the saved logo instead of falling back to
“No image.”
In `@apps/player-linux/src/core/types.ts`:
- Around line 147-148: Extend the ManifestPlugin union in types.ts to include an
explicit alert_ticker plugin interface/config matching the server-emitted
manifest shape, so player and IPC manifest contracts represent and forward
ticker plugins accurately.
---
Outside diff comments:
In `@apps/dashboard/src/api/types.ts`:
- Line 5: Update the capability checks using the role union in the permissions
types so contributor access is handled explicitly rather than through role !==
"viewer". Allow contributors only the authoring controls they support, and
exclude them from delete and publish actions for assets, layouts, and playlists;
preserve the existing permissions for owners, administrators, editors, and
viewers.
---
Duplicate comments:
In `@apps/server/internal/database/migrations/00076_brand_bug_plugin.sql`:
- Around line 10-28: Remove the table-level CHECK requiring image_asset_id or
non-empty text from the migration, since ON DELETE SET NULL must allow asset
deletion and validateBrandBug already enforces the non-empty invariant. Preserve
the separate starts_at/ends_at ordering constraint and all other column
constraints.
---
Nitpick comments:
In `@apps/player-linux/static/index.html`:
- Around line 233-241: Update the `#brand-bugs` stacking rule to use a z-index
higher than `#alert-ticker`, preserving its position above the countdown bar and
removing reliance on DOM order. Leave its positioning and pointer-events
behavior unchanged.
In `@apps/server/internal/plugins/brandbug_test.go`:
- Line 61: Update the "nil asset" test case in the BrandBugInput test to use a
local zero UUID variable declared as var zero uuid.UUID, and assign its address
to ImageAssetID instead of referencing the mutable package-level uuid.Nil.
In `@apps/server/internal/plugins/service_integration_test.go`:
- Around line 265-272: Remove the unused firstConfig helper from the test
package, including its implementation and associated comment, since no callers
remain.
In `@apps/server/internal/plugins/service.go`:
- Around line 258-275: Update targetIDsFrom to validate table against an
explicit allowlist of the package’s supported table literals before building the
query, returning an error for any unsupported value; preserve the existing query
and row-processing behavior for allowed tables.
- Around line 541-561: The ManifestForScreen projections use inconsistent config
shapes: countdowns and tickers return value configs while brand bugs return a
pointer. Update the countdown and ticker projection methods,
countdownBarsForScreen and alertTickersForScreen, to return pointers to
ManifestCountdownConfig and ManifestAlertTickerConfig, matching
brandBugsForScreen and preserving the existing config contents and manifest
assembly.
🪄 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: 012d1ac9-5744-450e-8024-d62a12197127
📒 Files selected for processing (29)
README.mdapps/dashboard/src/App.tsxapps/dashboard/src/api/client.tsapps/dashboard/src/api/types.tsapps/dashboard/src/pages/PluginsPage.test.tsxapps/dashboard/src/pages/PluginsPage.tsxapps/player-linux/src/core/manifest.test.tsapps/player-linux/src/core/manifest.tsapps/player-linux/src/core/types.tsapps/player-linux/src/renderer/brand-bug-resolver.test.tsapps/player-linux/src/renderer/brand-bug-resolver.tsapps/player-linux/src/renderer/renderer.tsapps/player-linux/static/index.htmlapps/server/internal/database/migrations/00076_brand_bug_plugin.sqlapps/server/internal/httpapi/plugins.goapps/server/internal/httpapi/routes.goapps/server/internal/playlists/plugin_assets_integration_test.goapps/server/internal/playlists/service.goapps/server/internal/plugins/brandbug.goapps/server/internal/plugins/brandbug_integration_test.goapps/server/internal/plugins/brandbug_test.goapps/server/internal/plugins/service.goapps/server/internal/plugins/service_integration_test.godocs/api.mddocs/architecture.mddocs/openapi.yamldocs/plugins.mdpackages/manifest-schema/README.mdpackages/manifest-schema/schema-v11.json
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/plugins.md
| const images = useQuery({ | ||
| queryKey: ["brand-bug-image-assets"], | ||
| queryFn: () => | ||
| api.assets( | ||
| new URLSearchParams({ | ||
| page: "1", | ||
| pageSize: "100", | ||
| type: "image", | ||
| status: "ready", | ||
| sort: "name", | ||
| }), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The logo list silently truncates at 100 images.
pageSize: "100" with no search or paging means a library with more ready images offers only the first page by name. Two consequences: a newer logo cannot be chosen, and when editing an instance whose imageAssetId is outside that page, Select falls back to rendering the first option ("No image") while form state still holds the real id — the field reads as cleared but saves unchanged.
Cheapest mitigation: surface the shortfall and keep the stored option present.
♻️ Sketch
<option value="">No image</option>
+ {imageAssetId &&
+ !(images.data?.items ?? []).some(
+ (item) => item.id === imageAssetId,
+ ) && <option value={imageAssetId}>Current logo</option>}
{(images.data?.items ?? []).map((item) => (plus a hint when images.data.total exceeds the loaded page.
🤖 Prompt for 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.
In `@apps/dashboard/src/pages/PluginsPage.tsx` around lines 1071 - 1083, Update
the logo-selection flow around the images query so it does not silently imply
that all ready image assets are available: show a hint when images.data.total
exceeds the loaded page size. Ensure the currently stored imageAssetId is
retained as an option even when it is outside the first 100 results, so the
Select displays the saved logo instead of falling back to “No image.”
| export type ManifestPlugin = | ||
| ManifestCountdownBarPlugin | ManifestBrandBugPlugin; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 '"alert_ticker"|alert_ticker' apps/player-linux/src
rg -nP --type=ts -C3 'plugin\.type|case "countdown_bar"|case "brand_bug"' apps/player-linux/srcRepository: Gibsonmb71/tilecast
Length of output: 4755
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant type definitions ---'
sed -n '1,220p' apps/player-linux/src/core/types.ts
printf '%s\n' '--- manifest parsing and plugin consumers ---'
rg -n -C4 'ManifestPlugin|PluginEntry|ManifestAlertTicker|parse.*manifest|plugins' apps/player-linux/src/core apps/player-linux/src/renderer
printf '%s\n' '--- server projection ---'
sed -n '560,620p' apps/server/internal/plugins/service.go
printf '%s\n' '--- related player tests ---'
rg -n -C5 'alert_ticker|ManifestPlugin|plugins:' apps/player-linux/src --glob '*test*' --glob '*.spec.*'
printf '%s\n' '--- status ---'
git status --shortRepository: Gibsonmb71/tilecast
Length of output: 35301
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- player manifest ingestion ---'
rg -n -C6 'JSON\.parse|as Manifest|Manifest\b|presentPlugins' apps/player-linux/src/core apps/player-linux/src/main apps/player-linux/src --glob '*.ts' --glob '*.tsx' | head -n 260
printf '%s\n' '--- renderer plugin shapes and bridge implementation ---'
sed -n '120,180p' apps/player-linux/src/renderer/renderer.ts
sed -n '430,510p' apps/player-linux/src/renderer/renderer.ts
rg -n -C6 'presentPlugins|RendererPlugin|onPlugins|plugins.*ManifestPlugin' apps/player-linux/src --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- server manifest plugin declarations ---'
rg -n -C6 'type ManifestPlugin|ManifestAlertTickerConfig|alert_ticker' apps/server/internal --glob '*.go'Repository: Gibsonmb71/tilecast
Length of output: 50375
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json
from pathlib import Path
types = Path("apps/player-linux/src/core/types.ts").read_text()
renderer = Path("apps/player-linux/src/renderer/renderer.ts").read_text()
storage = Path("apps/player-linux/src/core/storage.ts").read_text()
main = Path("apps/player-linux/src/main/index.ts").read_text()
manifest = {"plugins": [{"id": "a", "type": "alert_ticker", "version": 1, "config": {}}]}
round_tripped = json.loads(json.dumps(manifest))
print("alert_ticker survives JSON round-trip:", round_tripped["plugins"][0]["type"] == "alert_ticker")
print("core union includes alert_ticker:", '"alert_ticker"' in types)
print("renderer union includes alert_ticker:", "RendererAlertTickerPlugin" in renderer and '"alert_ticker"' in renderer)
print("manifest storage uses unchecked JSON cast:", "JSON.parse(raw) as T" in storage)
print("main plugin bridge is typed as ManifestPlugin[]:", "plugins: ManifestPlugin[]" in main)
PYRepository: Gibsonmb71/tilecast
Length of output: 386
Add alert_ticker to ManifestPlugin. The server emits it and the player forwards manifests through this type; add an explicit ticker interface/config to keep the core and IPC contracts accurate.
🤖 Prompt for 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.
In `@apps/player-linux/src/core/types.ts` around lines 147 - 148, Extend the
ManifestPlugin union in types.ts to include an explicit alert_ticker plugin
interface/config matching the server-emitted manifest shape, so player and IPC
manifest contracts represent and forward ticker plugins accurately.
1 participant
Summary
Adds Brand Bug / Watermark as a built-in plugin. It places a logo, sponsor mark, legal notice, campaign badge, or location label in any corner over all normal content — playlists, Layouts, websites, and Widgets — without changing what is playing.
Each instance carries a corner, an optional Media-library logo, optional text, screen-relative sizing, opacity, margin, text color, backing, an optional date window, enabled state, priority, and the same four targeting scopes as Countdown Bar.
Design decisions worth a reviewer's attention:
validateBrandBug, as a table CHECK, and as ananyOfin the manifest schema. A mark with neither would hold a corner invisibly.ManifestPlugin.Configbecomes the discriminated payload rather than the countdown config, since more than one plugin type now projects into the manifest. Brand Bug joins the catalog as a fourth entry beside Countdown Bar, Emergency Alerts, and Forms.Migration
00066_brand_bug_plugin.sqladdsbrand_bug_instancesandbrand_bug_targets.image_asset_idisON DELETE SET NULL, so deleting a logo from the Media library leaves the instance intact rather than taking it with it.Rebased onto current main
This branch was rebased over the plugin work that landed in the meantime (Forms and Emergency Alerts moving into Plugins, the countdown progress fill, configurable countdown text metrics, and the restructured target picker). Notable consequences:
00064to00066to sit after00064_countdown_bar_progress_filland00065_countdown_bar_text_metrics.enabledand their counts from different tables and a uniform loop no longer fits.TargetFieldsplususeTargetSource, and both editors drive it. The Brand Bug editor gets that picker for free rather than reintroducing a plainer third copy.Validation
Ran locally after the rebase:
gofmt -l,go vet ./...,go build ./cmd/tilecastgo test ./...withTEST_DATABASE_URLset — full server suite green, including new PostgreSQL integration paths for the Brand Bug lifecycle, invalid-logo rejection, per-corner projection, and logo-to-asset projection in manifest assemblynpm run format:check,npm run lint,npm test(505 tests across 70 files),npm run buildtsc --noEmit,vitest run(267 tests, 13 new for the resolver),npm run buildScope limit: Brand Bug is drawn by Linux Player only. Android Player already renders Countdown Bar but ignores plugin types it does not implement, so a configured mark is inert there rather than an error.
docs/plugins.mdstates this, and drawing it on Android is outstanding work — worth a follow-up now that a second plugin has to be written twice.Summary by CodeRabbit