Repository navigation
Fix screen workspace review findings - #388
Merged
Merged
Conversation
CodeRabbit findings from #386 and the two PRs before it. Screen detail tabs: ScreenDetailWithPreviewPage passed an unrecognized ?tab= value through unchanged while ScreenDetailPage clamped it to Overview, so ?tab=bogus rendered the Overview panel without its preview layout. Both now share normalizeScreenDetailTab. Android gates: players report "android-tv" or "fire-tv" and the server stores those strings unchanged, so platform === "android" hid every Android-only field and control on every current Android player. One predicate in playerPlatform.ts now covers the family, replacing the local copy in SettingsOperations. Maintenance workspace: a Viewer or Editor selecting Maintenance with no command history saw an empty pane with no explanation. It now says who can send commands. Release cache progress: the native progress element had no accessible name; the visible size text does not label it. Brand Bug logo: the worker marks a variant player-compatible while the asset is still processing, so resolveImageVariant could publish a logo the Player cannot download yet. It now requires processing_status 'ready', covered by the integration path. The manifest schema also accepted whitespace-only text that the server trims and rejects, and the README overstated when a configured logo reaches assets. Migration 00077 installed its CHECK while holding ACCESS EXCLUSIVE over player_releases; it is now NOT VALID there and validated in 00082, which is a no-op on databases that already applied the original.
github-actions
Bot
added
documentation
Contributor
📝 WalkthroughWalkthroughThe dashboard centralizes tab and platform handling, expands maintenance states, and adds tests. Database migrations defer and then validate a download-progress constraint. Manifest generation now excludes processing assets and supports documented text fallback behavior. ChangesDashboard screen management
Database constraint validation
Manifest asset fallback
Estimated code review effort: 3 (Moderate) | ~25 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/server/internal/playlists/plugin_assets_integration_test.go (1)
164-170: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the text fallback explicitly.
This test checks that the plugin remains and both image identifiers are removed. It does not check that
Textremains available. Add an assertion for the expected non-whitespace fallback text.As per coding guidelines, protocol changes require unit tests and a complete PostgreSQL integration path.
Proposed assertion
- if !ok || config.ImageAssetID != nil || config.ImageVariantID != nil { + if !ok || config.ImageAssetID != nil || config.ImageVariantID != nil || config.Text != "Presented by Example" {🤖 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/plugin_assets_integration_test.go` around lines 164 - 170, Extend the fallback assertions in the plugin asset integration test after the ManifestBrandBugConfig type check to verify that config.Text contains the expected non-whitespace fallback text, while preserving the existing plugin-count and image-identifier checks.Source: Coding guidelines
🤖 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/ScreensPage.tsx`:
- Around line 3395-3421: Update the command-history conditional rendering in the
operations section to check commands.isError before the empty-list condition,
and render an error notice when api.screenCommands fails. Preserve the existing
loading, empty-history, and command-list states for successful requests.
In `@packages/manifest-schema/schema-v11.json`:
- Around line 126-129: Revert the change in schema-v11.json so the released v11
validation behavior remains unchanged, including acceptance of whitespace-only
text under the prior rule. Enforce the non-whitespace fallback in server output
instead, or introduce a new schema version only with an explicit Player rollout
contract.
---
Nitpick comments:
In `@apps/server/internal/playlists/plugin_assets_integration_test.go`:
- Around line 164-170: Extend the fallback assertions in the plugin asset
integration test after the ManifestBrandBugConfig type check to verify that
config.Text contains the expected non-whitespace fallback text, while preserving
the existing plugin-count and image-identifier checks.
🪄 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: 72532bb5-e4f2-4cf6-97e6-b92986e6afce
📒 Files selected for processing (11)
apps/dashboard/src/pages/ScreenDetailActivityTab.test.tsxapps/dashboard/src/pages/ScreenDetailWithPreviewPage.tsxapps/dashboard/src/pages/ScreensPage.tsxapps/dashboard/src/playerPlatform.tsapps/dashboard/src/settings/SettingsOperations.tsxapps/server/internal/database/migrations/00077_player_release_download_progress.sqlapps/server/internal/database/migrations/00082_validate_player_release_download_progress.sqlapps/server/internal/playlists/plugin_assets_integration_test.goapps/server/internal/playlists/service.gopackages/manifest-schema/README.mdpackages/manifest-schema/schema-v11.json
Comment on lines
+3395
to
+3421
| !canManageScreens(auth.status?.user) && ( | ||
| <section className="operations"> | ||
| <h3>Recent operations</h3> | ||
| <div className="command-history"> | ||
| {commands.data?.items?.map((c) => ( | ||
| <div key={c.id}> | ||
| <strong> | ||
| {c.type?.replaceAll("_", " ") ?? "Unknown command"} | ||
| </strong> | ||
| <span> | ||
| {c.state} · {new Date(c.createdAt).toLocaleString()} | ||
| </span> | ||
| <small> | ||
| {c.resultCode?.replaceAll("_", " ") ?? "No result yet"} | ||
| </small> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| {commands.isLoading ? ( | ||
| <div className="table-loading">Loading operations…</div> | ||
| ) : (commands.data?.items?.length ?? 0) === 0 ? ( | ||
| <p> | ||
| No maintenance commands have been sent to this screen. An Owner | ||
| or Administrator can send them. | ||
| </p> | ||
| ) : ( | ||
| <div className="command-history"> | ||
| {commands.data?.items?.map((c) => ( | ||
| <div key={c.id}> | ||
| <strong> | ||
| {c.type?.replaceAll("_", " ") ?? "Unknown command"} | ||
| </strong> | ||
| <span> | ||
| {c.state} · {new Date(c.createdAt).toLocaleString()} | ||
| </span> | ||
| <small> | ||
| {c.resultCode?.replaceAll("_", " ") ?? "No result yet"} | ||
| </small> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show a command-history error state.
If api.screenCommands fails, this branch evaluates the missing item list as empty. The page then states that no maintenance commands exist when the history could not be loaded.
Check commands.isError before the empty-list condition and render an error notice.
Proposed fix
{commands.isLoading ? (
<div className="table-loading">Loading operations…</div>
+ ) : commands.isError ? (
+ <div className="notice notice--error" role="alert">
+ Recent operations could not be loaded. {commands.error.message}
+ </div>
) : (commands.data?.items?.length ?? 0) === 0 ? (📝 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.
Suggested change
| !canManageScreens(auth.status?.user) && ( | |
| <section className="operations"> | |
| <h3>Recent operations</h3> | |
| <div className="command-history"> | |
| {commands.data?.items?.map((c) => ( | |
| <div key={c.id}> | |
| <strong> | |
| {c.type?.replaceAll("_", " ") ?? "Unknown command"} | |
| </strong> | |
| <span> | |
| {c.state} · {new Date(c.createdAt).toLocaleString()} | |
| </span> | |
| <small> | |
| {c.resultCode?.replaceAll("_", " ") ?? "No result yet"} | |
| </small> | |
| </div> | |
| ))} | |
| </div> | |
| {commands.isLoading ? ( | |
| <div className="table-loading">Loading operations…</div> | |
| ) : (commands.data?.items?.length ?? 0) === 0 ? ( | |
| <p> | |
| No maintenance commands have been sent to this screen. An Owner | |
| or Administrator can send them. | |
| </p> | |
| ) : ( | |
| <div className="command-history"> | |
| {commands.data?.items?.map((c) => ( | |
| <div key={c.id}> | |
| <strong> | |
| {c.type?.replaceAll("_", " ") ?? "Unknown command"} | |
| </strong> | |
| <span> | |
| {c.state} · {new Date(c.createdAt).toLocaleString()} | |
| </span> | |
| <small> | |
| {c.resultCode?.replaceAll("_", " ") ?? "No result yet"} | |
| </small> | |
| </div> | |
| ))} | |
| </div> | |
| )} | |
| !canManageScreens(auth.status?.user) && ( | |
| <section className="operations"> | |
| <h3>Recent operations</h3> | |
| {commands.isLoading ? ( | |
| <div className="table-loading">Loading operations…</div> | |
| ) : commands.isError ? ( | |
| <div className="notice notice--error" role="alert"> | |
| Recent operations could not be loaded. {commands.error.message} | |
| </div> | |
| ) : (commands.data?.items?.length ?? 0) === 0 ? ( | |
| <p> | |
| No maintenance commands have been sent to this screen. An Owner | |
| or Administrator can send them. | |
| </p> | |
| ) : ( | |
| <div className="command-history"> | |
| {commands.data?.items?.map((c) => ( | |
| <div key={c.id}> | |
| <strong> | |
| {c.type?.replaceAll("_", " ") ?? "Unknown command"} | |
| </strong> | |
| <span> | |
| {c.state} · {new Date(c.createdAt).toLocaleString()} | |
| </span> | |
| <small> | |
| {c.resultCode?.replaceAll("_", " ") ?? "No result yet"} | |
| </small> | |
| </div> | |
| ))} | |
| </div> | |
| )} |
🤖 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/ScreensPage.tsx` around lines 3395 - 3421, Update
the command-history conditional rendering in the operations section to check
commands.isError before the empty-list condition, and render an error notice
when api.screenCommands fails. Preserve the existing loading, empty-history, and
command-list states for successful requests.
Comment on lines
+126
to
+129
| { | ||
| "required": ["text"], | ||
| "properties": { "text": { "pattern": "\\S" } } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep the released v11 schema immutable.
The manifest README states that released versions are not reinterpreted. This change makes a previously valid v11 payload invalid: text: " " with no imageAssetId passed the previous minLength: 1 rule but fails pattern: "\\S".
Keep schema-v11.json unchanged. Enforce the non-whitespace fallback in server output, or add a new schema version with an explicit Player rollout contract.
🤖 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 126 - 129, Revert the
change in schema-v11.json so the released v11 validation behavior remains
unchanged, including acceptance of whitespace-only text under the prior rule.
Enforce the non-whitespace fallback in server output instead, or introduce a new
schema version only with an explicit Player rollout contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the CodeRabbit findings on #386 and the two PRs before it.
Dashboard
?tab=values.ScreenDetailWithPreviewPagepassed anything it did not recognize straight through whileScreenDetailPageclamped it to Overview, so?tab=bogusrendered the Overview panel without the preview layout orLivePreviewPanel. Both now call one exportednormalizeScreenDetailTab, which also removes the duplicated legacy-tab list.android-tvorfire-tvand the server stores those strings unchanged, soplatform === "android"hid the install-permission, Managed Kiosk, Power Assist, and device fields on every current Android player. Newsrc/playerPlatform.tsholds one family predicate, used by all five gates and bySettingsOperationsin place of its local copy. The Platform row now reports the string the device actually sent alongside the Android version.<progress>had no accessible name; the visible size text does not label a native control. It now carries anaria-labelwith the release and both sizes.Server
player_compatible = TRUEwhile the asset is still processing, soresolveImageVariantcould hand a Player a logo it cannot yet fetch. It now requiresprocessing_status = 'ready'. The integration path covers the new state: a processing logo falls back to the mark's text and is not projected as an asset.ADD CONSTRAINTscannedplayer_releasesinside Goose's transaction, holding the lock against cache-progress and release writes until commit. The constraint is now installedNOT VALID, and 00082 validates it in its own transaction under SHARE UPDATE EXCLUSIVE. Databases that already applied the original hold a valid constraint, soVALIDATE CONSTRAINTis a no-op there — confirmed against a database already at the old 00077.Manifest schema
minLength: 1accepted whitespace-only Brand Bugtextthat the server trims and rejects; the text-only branch now requires a non-space character. v12-v14$refinto v11, so the one change covers them.assets; an unavailable logo drops the reference and falls back to text. Reworded to match runtime behavior.Verification
npx tsc --noEmit,prettier --check, and the dashboard suite (321 tests in the touched areas, 584 overall) pass.gofmt,go vet ./...,go build ./cmd/tilecast, andgo test ./...withTEST_DATABASE_URLset all pass on this branch.One thing to watch
The new migration is numbered 00082 because 00078-00081 are taken by other work not yet on
main. If those land after this PR, renumber accordingly — Goose rejects out-of-order migrations on an existing database.Summary by CodeRabbit
New Features
Bug Fixes
Documentation