Repository navigation
Migrate Tribute to Corepack-pinned Yarn 4 - #19
Conversation
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski reviewed 7 files and all commit messages, and made 1 comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on pkaminski).
.travis.yml at r1 (raw file):
Remove the Travis setup inherited from the fork and switch to a GitHub actions build / test job instead.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski+CODX made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on pkaminski).
Previously, pkaminski (Piotr Kaminski) wrote…
Remove the Travis setup inherited from the fork and switch to a GitHub actions build / test job instead.
Removed .travis.yml and added .github/workflows/build.yaml. It runs on pull requests and pushes to reviewable-master/release, selects Node from .nvmrc, installs the Corepack-pinned Yarn, then runs yarn install --immutable, yarn build, and yarn test-single-run.
The job uses the runner's installed Chrome and skips the obsolete Puppeteer Chromium download. Karma now honors an explicit CHROME_BIN, retaining Puppeteer as the local fallback; both paths were checked. Immutable installation and build pass, with generated library files unchanged. Running the actual browser-test script locally gives the same 20 passes and 12 pre-existing failures as before. The workflow leaves those failures visible.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski+CODX made 1 comment.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on pkaminski).
Previously, pkaminski+CODX (Agent CODX for Piotr Kaminski) wrote…
Removed
.travis.ymland added.github/workflows/build.yaml. It runs on pull requests and pushes toreviewable-master/release, selects Node from.nvmrc, installs the Corepack-pinned Yarn, then runsyarn install --immutable,yarn build, andyarn test-single-run.The job uses the runner's installed Chrome and skips the obsolete Puppeteer Chromium download. Karma now honors an explicit
CHROME_BIN, retaining Puppeteer as the local fallback; both paths were checked. Immutable installation and build pass, with generated library files unchanged. Running the actual browser-test script locally gives the same 20 passes and 12 pre-existing failures as before. The workflow leaves those failures visible.
The first GitHub Actions run has now completed. Corepack setup, hardened immutable installation, and build all pass; Chrome starts and executes all 32 tests. Linux reports 19 passes and 13 failures: the same 12 failures reproduced locally, plus the input-menu positioning test (Expected 224 to equal 504). The PR description records this additional runner result; CI remains red and no failures are suppressed.
Security PR #18 is restacked on this commit and still changes only yarn.lock. Its reconstruction proof passes against the updated migration baseline.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski+CODX made 1 comment.
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on pkaminski).
Previously, pkaminski+CODX (Agent CODX for Piotr Kaminski) wrote…
The first GitHub Actions run has now completed. Corepack setup, hardened immutable installation, and build all pass; Chrome starts and executes all 32 tests. Linux reports 19 passes and 13 failures: the same 12 failures reproduced locally, plus the input-menu positioning test (
Expected 224 to equal 504). The PR description records this additional runner result; CI remains red and no failures are suppressed.Security PR #18 is restacked on this commit and still changes only
yarn.lock. Its reconstruction proof passes against the updated migration baseline.
The GitHub Actions build is now green, including all 32 browser tests. The failures came from the test helper creating plain divs for its contenteditable fixtures without setting contentEditable. Adding that attribute fixes all failures locally and on Linux, including the extra positioning failure. No assertions were removed and published library code is unchanged.
Security PR #18 is restacked on the fix and its lockfile replay has been reverified.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
This makes our fork incompatible even more with upstream, but it had already diverged markedly so any pull from upstream will need to resolve a bunch of conflicts anyway.
+r:@snoack
@pkaminski reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on snoack).
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack reviewed 10 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on pkaminski).
.travis.yml at r3 (raw file):
With .travis.yml gone, the build status badge in README.md is now stale.
.github/workflows/build.yaml line 23 at r3 (raw file):
with: node-version-file: .nvmrc package-manager-cache: false
Nit: Without a replacement actions/cache step (like home's deploy workflow has), every run downloads all packages again. Add one keyed on yarn.lock?
test/spec/utils/dom-helpers.js line 10 at r3 (raw file):
let input = document.createElement(elementToCreate); if (element === 'contenteditable') { input.contentEditable = 'true';
Fine for this PR, but the failures point at a library bug: ensureEditable (src/Tribute.js:257) checks !element.contentEditable, which is "inherit" (truthy) for a plain div, so the "not contentEditable" guard never fires. Worth a follow-up switching it to !element.isContentEditable, with a test that attaching to a non-editable div throws?
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski+CODX made 3 comments and resolved 2 discussions.
Reviewable status: 9 of 11 files reviewed, 1 unresolved discussion (waiting on snoack).
.github/workflows/build.yaml line 23 at r3 (raw file):
Previously, snoack (Sebastian Noack) wrote…
Nit: Without a replacement
actions/cachestep (like home's deploy workflow has), every run downloads all packages again. Add one keyed onyarn.lock?
Added an actions/cache@v5 step using Yarn's actual cacheFolder, keyed on the lockfile and Yarn configuration, with an OS-specific restore prefix for partial hits.
test/spec/utils/dom-helpers.js line 10 at r3 (raw file):
Previously, snoack (Sebastian Noack) wrote…
Fine for this PR, but the failures point at a library bug:
ensureEditable(src/Tribute.js:257) checks!element.contentEditable, which is"inherit"(truthy) for a plain div, so the "not contentEditable" guard never fires. Worth a follow-up switching it to!element.isContentEditable, with a test that attaching to a non-editable div throws?
Confirmed. The separate follow-up is now #21, with Jacob requested for review. It uses isContentEditable, fixes the README examples, bumps the library to 5.1.4, and regenerates the distributed bundles.
The six added browser cases cover plain and explicitly non-editable divs, explicit and inherited editability, inputs, and textareas. The two rejection cases fail before the fix; all 38 tests pass afterward. The fix is stacked above security PR #18, keeping this migration's runtime code unchanged.
Previously, snoack (Sebastian Noack) wrote…
With
.travis.ymlgone, the build status badge inREADME.mdis now stale.
Replaced the Travis badge with the fork's GitHub Actions build.yaml badge and workflow link, targeting reviewable-master.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski reviewed 2 files and all commit messages.
Reviewable status: 9 of 11 files reviewed, 1 unresolved discussion (waiting on snoack).
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pkaminski).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Use Corepack-pinned Yarn 4.13.0 with the node-modules linker and a five-day resolution cooldown. Convert the Classic lockfile, ignore local Yarn state, and replace the inherited Travis configuration with a GitHub Actions build/test job. CI uses Node 24, Corepack, immutable installation, and the runner's installed Chrome; Karma honors CHROME_BIN with Puppeteer as its local fallback. The library's runtime requirements, dependency specifications, and version remain unchanged.
This migration is based on reviewable-master and precedes security PR #18. All 877 original lockfile descriptors retain their resolved versions. Yarn adds 73 npm descriptors for metadata and native-build dependencies absent from the Classic lockfile; compatible existing versions are reused where possible. Their 67 distinct versions and the Yarn executable passed a publication-date check against the five-day cooldown. Body Parser remains at the original 1.20.1 here; #18 applies its verified 1.20.9 security backport afterward. Yarn's added node-gyp tree already uses Undici 8.12.3, above the requested 8.12.2 minimum and beyond cooldown.
The browser-test helper now explicitly marks its contenteditable div fixtures editable. Without that attribute, the fixtures could not receive input as intended, causing the previously reported browser-test failures. This changes only test setup; all 32 tests and their assertions remain enabled, and published library code is unchanged.
Validation on Node 24.21.0:
Security PR #18 is restacked on this migration and its lockfile reconstruction proof has been reverified. This package-manager/CI migration is outside lockfile-only automatic merging.
Follow-up validation: Linux CI passes, including all 32 browser tests. The workflow caches Yarn downloads, and the README badge points to this fork's build workflow. The separate
isContentEditablefix is #21, with Jacob requested for review.This change is