Skip to content

chore: sync development into main - #30

Closed
nicomiguelino wants to merge 4 commits into
mainfrom
development
Closed

nicomiguelino wants to merge 4 commits into
mainfrom
development

Conversation

@nicomiguelino

Copy link
Copy Markdown
Contributor

Syncs the @screenly/edge-apps 1.2.1 dependency bump (removes the screen name from the app header) from development into main.

- Removes the screen name from the app header (Screenly/edge-apps-library#56)
- Regenerates screenshots to reflect the updated header
@nicomiguelino
nicomiguelino marked this pull request as ready for review July 23, 2026 03:37
- Set STAGE_EDGE_APP_ID and PRODUCTION_EDGE_APP_ID repo variables and pass them through to the initialize/update actions as edge_app_id
- Drop id from screenly.yml and delete screenly_qc.yml since a single manifest is now used regardless of environment
- Pin edge-apps-actions to a commit for now, since v26.9.0 can't be tagged there until a repo ruleset is adjusted
Copilot AI lite review requested due to automatic review settings September 4, 2026 03:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Removing screenly_qc.yml breaks existing stage workflows that still reference that manifest file.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Syncs changes from development into main, primarily updating the @screenly/edge-apps tooling dependency and aligning deployment workflows/manifests with the newer Edge Apps action usage.

Changes:

  • Bump @screenly/edge-apps from ^1.0.0 to ^1.2.1 (and update bun.lock accordingly).
  • Update GitHub Actions workflows to use a pinned commit SHA for Screenly/edge-apps-actions/* and pass edge_app_id from secrets/vars.
  • Remove the id field from screenly.yml and delete screenly_qc.yml.
File summaries
File Description
screenly.yml Removes the manifest id field.
screenly_qc.yml Deletes the stage/QC manifest file.
package.json Bumps @screenly/edge-apps devDependency to ^1.2.1.
bun.lock Lockfile updates from the dependency bump.
.github/workflows/update-edge-app.yml Pins the update action to a commit SHA and provides edge_app_id.
.github/workflows/initialize-edge-app.yml Pins the initialize action to a commit SHA and provides edge_app_id selection logic.
Review details
  • Files reviewed: 5/16 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread screenly.yml Resolved
…kflows

The multi-app initialize/update-rss-reader-apps workflows target
screenly_qc.yml for stage deploys independently of the standard
initialize/update-edge-app actions. Deleting it broke that path.
Copilot AI review requested due to automatic review settings September 4, 2026 05:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The dependency bump currently results in a Vite↔esbuild peer version mismatch in the lockfile that can break build/dev scripts unless addressed.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

package.json:14

  • @screenly/edge-apps@^1.2.1 pulls in Vite 8 (see bun.lock:958), which declares a peer dependency on esbuild ^0.27.0 || ^0.28.0, but the lock currently resolves esbuild@0.17.19 (bun.lock:630). This version mismatch can break edge-apps-scripts dev/build at runtime; add an explicit esbuild devDependency in the supported range and regenerate bun.lock.
  "devDependencies": {
    "@playwright/test": "^1.58.0",
    "@screenly/edge-apps": "^1.2.1",
    "@types/bun": "^1.3.13",
    "@types/jsdom": "^28.0.1",
    "bun-types": "^1.3.13",
    "jsdom": "^29.1.1",
    "npm-run-all2": "^8.0.4",
    "prettier": "^3.8.3",
    "typescript": "^6.0.3"
  },
  • Files reviewed: 4/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@vpetersson-bot

Copy link
Copy Markdown
Contributor

Heads up: this PR currently has merge conflicts with main and can't be merged as-is. A rebase (or merge from main) will clear it.

Flagged by an automated PR-hygiene sweep — no action needed beyond the rebase, and no reply expected.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vpetersson-bot vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this because the description and the diff don't match, and the gap is on the riskier side.

The body says this syncs the @screenly/edge-apps 1.2.1 bump. The diff also does three other things, none of them mentioned:

1. actions/checkout is downgraded from v7 to v6 — but only in update-edge-app.yml. initialize-edge-app.yml keeps @v7. So main would end up with two workflows in the same repo disagreeing about checkout, and the one that deploys to production is the one going backwards. If that's deliberate (a v7 incompatibility with the pinned action?) it needs a comment saying so; if it's a stray revert picked up in the sync, it should be fixed on development before this lands.

2. The app's id moves out of screenly.yml and into CI configuration. screenly.yml loses its id: line, and the workflows gain edge_app_id: ${{ secrets.EDGE_APP_ID || vars.PRODUCTION_EDGE_APP_ID }} (and the stage equivalent). That's a meaningful operational change: app identity is no longer in the repo, and deploys now depend on repository variables being set correctly per environment. If PRODUCTION_EDGE_APP_ID is unset or wrong, the production job either fails or — worse — targets the wrong app. Please confirm both variables are populated in this repo's environments before merging. This is the part I'd most want checked.

3. Pinning Screenly/edge-apps-actions from @v1 to a commit SHA is a genuine improvement and worth calling out as intentional rather than leaving it to be discovered — a moving @v1 tag in a workflow holding SCREENLY_API_TOKEN is exactly what SHA-pinning is for.

None of this is necessarily wrong — it all presumably landed on development deliberately. But a development → main sync is the moment it reaches production deploys, and a one-line description that mentions only a dependency bump means nobody reviews the parts that matter. Worth expanding the body to list what's actually crossing over.

(Reviewed alongside the matching sync in airtable-app — same three points there.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants