feat(plugin-dev): add watch mode, readiness polling, and phased start UX - #222
Conversation
… UX (RHIDP-16673)
plugin dev update --watch
----------------------
- Add chokidar file watcher on src/ and package.json with 500ms debounce
- Extract runUpdateCycle() so one-shot and watch share the same implementation
- Serialize cycles: a change arriving during an active cycle queues exactly
one follow-up rather than running concurrently
- Add waitForContainerEvent(tool, service, action, timeoutMs): subscribe to
container runtime event stream via `podman|docker events --stream`, resolve
when the target service emits the target action. Shared primitive used by
both cleanup settling and start phase progress.
- Add waitForContainerCleanup(tool, timeoutMs): waits for both rhdh and
install-dynamic-plugins to emit their terminal event before the next
back-to-back cycle starts, preventing the crun exec.fifo race:
Podman: cleanup (after crun tears down exec.fifo)
Docker: die (terminal event; no equivalent cleanup step)
- Print '[watch] Refresh your browser at <url>' when RHDH is ready
- Fix isIgnoredWatchPath: chokidar v4+ dropped glob-string support for
`ignored` (function/regex/literal path only). The previous glob array
(e.g. double-star dist/node_modules patterns) silently matched nothing,
verified by reproduction against the vendored chokidar. Replaced with a
path-segment predicate that actually excludes output directories.
plugin dev start — phased UX and --watch
-----------------------------------------
- Show four labeled phases: build/export, start runtime, install plugins,
wait for readiness
- [3/4] Installing dynamic plugins uses waitForContainerEvent to watch for
the installer container's died event rather than blocking on a compose call
- waitForInstallerToFinish checks the installer's current Compose state
before subscribing to the events stream: compose up -d won't recreate an
already-exited one-shot container, so a re-entrant `start` against an
already-running runtime previously stalled for the full 60s
waitForContainerEvent timeout waiting on a `died` event that already fired
in a prior invocation. Skips straight through when already exited.
- [4/4] polls HTTP until RHDH responds, then prints the URL
- Add --watch to `start`: after readiness, fall straight into watchUpdate()
so a user can go from a cold start into continuous watch mode in one
command. --configure stays independent — no implicit behavior change.
plugin dev update / restart — runtime pre-flight checks
---------------------------------------------------------
- Add ensureRuntimeRunning(tool, runtimeDir): inspects Compose ps output for
the rhdh service and fails fast with an actionable message
("RHDH Local is not running. Run `rhdh-cli plugin dev start` first.")
instead of a raw compose/container subprocess error. Applied to both
runUpdateCycle (covers update and update --watch) and restart, the only
two subcommands with a real "runtime must already exist" precondition —
confirmed via a full precondition/action/postcondition pass over all six
plugin dev subcommands (start, update, restart, stop, logs, status).
- Refactored formatRuntimeStatus's inline service-lookup closures into
shared findComposeService/composeServiceState/composeServiceExitCode
helpers, reused by ensureRuntimeRunning, waitForInstallerToFinish, and
formatRuntimeStatus itself.
- Fixed ensureGeneratedConfigIncluded's error message: it told users to
"rerun with --configure", but that flag only exists on `start`, not on
`update` — a dead end for a user hitting this on `update --watch`. Now
points at `rhdh-cli plugin dev start --configure` directly.
Shared helpers
--------------
- resolveRhdhUrl(runtimeDir): reads BASE_URL from default.env then .env
(optional override), falls back to http://localhost:7007
- waitForRhdhReady(url, timeoutMs, pollIntervalMs): fetch-polls with dots,
resolves with the URL, throws with an actionable message on timeout
Testing
-------
- waitForContainerEvent: resolves on match, ignores wrong service, times out,
passes correct filter to spawn
- waitForContainerCleanup: podman uses cleanup, docker uses die
- resolveRhdhUrl: fallback, default.env, .env override, commented lines
- waitForRhdhReady: resolves on 200, retries on ECONNREFUSED, throws on timeout
- watchUpdate: cycle fires, debounce coalesces, recovery after failure,
SIGINT, fails cleanly and keeps watching when RHDH Local is not running
- isIgnoredWatchPath: excludes output/dependency directories, does not
exclude ordinary watched paths (regression test for the chokidar v4 glob
behavior change — exercises the real predicate, not a mocked watcher)
- start: enters watch mode on --watch, does not enter it otherwise, skips
the installer event wait when it already exited (re-entrant start)
- update / restart: reject with an actionable message when RHDH Local is
not running or stopped; restart succeeds and issues both compose calls
when running
Signed-off-by: Stan Lewis <stlewis@redhat.com>
Assisted-by: claude-sonnet-4-6@default
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Stan Lewis <gashcrumb@gmail.com> Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
- Simplify the BASE_URL regex in resolveRhdhUrl to remove a superlinear
backtracking pattern (typescript:S8786): capture the rest of the line
greedily and trim() afterward instead of combining a lazy capture with
a trailing \s*$ anchor.
- Add describeWatchError(err) to stringify caught unknown values in
watchUpdate's cycle-failure and watcher-error handlers, instead of
String(err), which falls back to Object.prototype.toString()
('[object Object]') for plain objects (typescript:S6551 x2).
Signed-off-by: Stan Lewis <gashcrumb@gmail.com>
Assisted-By: opencode
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
The prior fix still combined two adjacent quantifiers over overlapping
character classes (\s* then .*), which SonarCloud continued to flag as
superlinear (typescript:S8786). Replace the whole BASE_URL match with a
plain indexOf('=')/slice() split — no regex needed for the key/value
parse, only the existing anchored quote-strip regex remains.
Signed-off-by: Stan Lewis <gashcrumb@gmail.com>
Assisted-By: opencode
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
The watchUpdate and start suites each carried a near-identical block that mkdtemp'd a runtime/plugin dir, wrote the RHDH Local marker files, and wrote a minimal frontend plugin package.json. Extract scaffoldPluginDevRuntime() and have both beforeEach hooks call it. Also collapse the repeated fs.writeFile(path.join(dir, name), content) calls in the resolveRhdhUrl suite into a small writeEnvFile() closure. Pulled new_duplicated_lines_density back under the 3% quality-gate threshold (was 7.7%, all in this file); no behavior change, same 49 tests still pass. Signed-off-by: Stan Lewis <gashcrumb@gmail.com> Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:56 AM UTC · Completed 12:17 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $5.84 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk driven by large change size (1345 lines, large blast radius) and two dependency files changed, partially offset by no security-sensitive paths, no CI workflow changes, a healthy test-file ratio, and no churn or revert history on the newly-introduced dev command files. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk driven by large change size (1297 lines, large blast radius) and two dependency files changed, partially offset by no security-sensitive paths, no CI changes, and low churn on the newly-introduced dev command files. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix waitForInstallerToFinish hardcoding Podman's 'died' event action unconditionally: on Docker (--container-tool docker), the terminal container-death event is 'die', not 'died', so the --filter event=died argument to 'docker events' never matched anything and phase 3 of plugin dev start stalled for the full 60s waitForContainerEvent timeout on every Docker-backed run. Branch on containerTool the same way waitForContainerCleanup already does. Add a regression test that asserts the docker branch subscribes with event=die, not event=died. - Trim src/commands/dev/index.ts back down to the six CLI action handlers (start, update, restart, stop, logs, status) per this repo's barrel-file convention (AGENTS.md). watchUpdate, waitForContainerEvent, waitForContainerCleanup, resolveRhdhUrl, and waitForRhdhReady have no consumer outside command.ts/command.test.ts (tests already import them directly from ./command) and were widening the module's public surface without a driving need. - Fix stale watchUpdate JSDoc claiming it watches tsconfig*.json and *.config.* files; watchPaths only ever contained src/ and package.json. - README: document --watch on start/update, start's phased [1/4]-[4/4] output and blocking-until-ready behavior, update's readiness-poll blocking (up to 2 minutes) and URL output, and that update/restart require the runtime to already be running. - AGENTS.md: document --watch, the readiness-poll blocking behavior on start/update, and the ensureRuntimeRunning pre-flight check on update/restart. Addresses fullsend-ai-review findings on PR redhat-developer#222 (inline comments on command.ts:239, dev/index.ts:20, command.ts:459, and the PR-level summary comment covering README.md/AGENTS.md staleness). The 'authorization-non-github' note and the 'ssrf' note on waitForRhdhReady (informational only, no remediation suggested; the poll target is local developer configuration, not an external input) require no code change. Signed-off-by: Stan Lewis <gashcrumb@gmail.com> Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
Addressed in dbc1748:
All 50 unit tests, lint, prettier, and |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:55 PM UTC · Completed 1:15 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $5.30 |
- Fix api-contract-violation: waitForContainerEvent passed '--stream'
unconditionally, but Docker's 'events' command has no such flag (it
always streams) and rejects unknown flags, exiting immediately.
child.on('close') then resolved the promise before any event was
seen, making phase 3 of 'plugin dev start' (waitForInstallerToFinish)
a silent no-op on Docker. Only pass --stream for Podman. Added a
regression test asserting docker args omit --stream and podman args
include it; confirmed it fails against the unfixed code.
- Fix edge-case: 'update --watch' entered watchUpdate without calling
ensureRuntimeRunning first, so a stopped runtime only surfaced an
error on the first change-triggered cycle instead of failing fast
like the one-shot 'update' path. Call ensureRuntimeRunning before
entering watch mode. Added a regression test.
- Fix logic-error: the readiness check
'res.ok || (res.status >= 200 && res.status < 400)' was a tautology
(res.ok's [200,300) is already covered by [200,400)), obscuring the
intent to also accept 3xx redirects. Simplified to the range check
alone with a clarifying comment.
- Fix api-shape: runUpdateCycle and watchUpdate led with runtimeDir,
while every other multi-param helper in the file
(getComposeServices, ensureRuntimeRunning, compose,
waitForInstallerToFinish, waitForContainerCleanup) leads with
containerTool. Reordered both to (containerTool, runtimeDir, ...)
and updated all call sites (command.ts and command.test.ts).
- Fix code-organization: watchUpdate registered two separate
watcher.on('error', ...) listeners (one logging, one rejecting).
Consolidated into a single handler that does both.
Addresses fullsend-ai-review findings on PR redhat-developer#222 at commit dbc1748.
The 'protected-path' note on AGENTS.md is acknowledged — that edit
was made in the prior commit at the PR author's explicit direction
to document the RHIDP-16673 behaviors landing in this same PR.
Signed-off-by: Stan Lewis <gashcrumb@gmail.com>
Assisted-By: opencode
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
|
Second review round addressed in f4ae23e:
All 52 unit tests, lint, prettier, and |
PatAKnight
left a comment
PatAKnight
left a comment
There was a problem hiding this comment.
A couple of comments and some nits
Seven findings from PatAKnight's review round: - Hold running through the settle-wait instead of clearing it in a finally block right after the cycle. A change arriving during the wait previously saw running === false and started a second, concurrent drainCycles() loop instead of being coalesced into the active one. Also move the settle-wait's event subscription to the top of each cycle (before runUpdateCycle's own compose stop/start calls run), since subscribing afterward almost always misses the event and burns the full settleTimeoutMs. - Fix Docker's events JSON shape: the compose-service label lives under Actor.Attributes for Docker, not the top-level Attributes field Podman uses (verified against real podman events --format json output and Docker's documented schema). Reading only the top-level field meant the filter never matched on Docker, and the events-shape bug was masked by a prior regression test that emitted Podman's shape for a Docker scenario -- fixed that test too. - Watch tsconfig.json in addition to src/ and package.json -- RHIDP-16673's acceptance criteria require watching "relevant build configuration files"; tsconfig.json is the only one plugin new scaffolds. - Scope isIgnoredWatchPath's segment matching to the plugin root (chokidar always passes absolute paths) instead of checking every segment of the full absolute path, which ignored every file when a checkout merely lived under an ancestor directory named e.g. dist or node_modules. - Kill in-flight container-events child processes on shutdown. spawn() shares the parent's process group by default, so a terminal Ctrl+C's whole-group SIGINT takes them along for free, but a direct SIGTERM to just this PID does not, and process.exit() prevents waitForContainerEvent's own JS-side timeout from ever running to kill them itself. Track spawned children in a module-level set; shutdown() kills everything still in it. Added a SIGTERM counterpart to the existing SIGINT test. - Log a warning instead of silently proceeding when the events subprocess errors or exits with an unexpected non-zero code, instead of treating it the same as a successful match. - Run update --watch's first cycle immediately instead of leaving the current tree undeployed until the first save, matching start --watch's build-then-watch structure. This also subsumes the previous round's standalone ensureRuntimeRunning pre-flight call. Manually verified end to end: scaffolded a backend plugin with plugin new, ran plugin dev start and plugin dev update --watch against a real RHDH Local checkout (podman). Confirmed the initial --watch deploy happens before entering watch mode, a source edit triggers a full re-export/re-stage/restart cycle, tsconfig.json is listed in the watch log, and SIGTERM sent mid-cycle kills the in-flight settle-wait's podman events children with no orphans left behind (verified via ps). The new close-code warning also fired for real during this session against a live podman events subprocess, confirming it surfaces genuine environment hiccups rather than only synthetic test scenarios. Signed-off-by: Stan Lewis <gashcrumb@gmail.com> Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
- typescript:S6544: `if (settlePromise) await settlePromise;` used a Promise value directly as a boolean condition. Changed to an explicit `!== undefined` check. - Duplication gate: the two new settle-wait tests (holds running through the settle wait / kills an in-flight settle-wait subscription on shutdown) shared a near-identical block that put watchUpdate into the settle-wait phase (capture spawned children, hold cycle 1 open, inject a second change, release, wait for both events subscriptions to spawn). Extracted enterSettleWait(), resolveSettleWait(), and restoreDefaultSpawnMock() helpers; both tests now call them instead of repeating the setup. Signed-off-by: Stan Lewis <gashcrumb@gmail.com> Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
All 7 findings from this review round addressed in 650e2a4 (SonarCloud follow-up in e8b503f):
Manual end-to-end verification: scaffolded a backend plugin with A SonarCloud follow-up (e8b503f) fixed a Promise-in-boolean-conditional finding and de-duplicated two new tests that shared setup — quality gate is green with zero open issues. All 64 unit tests, lint, prettier, and |
PatAKnight
left a comment
PatAKnight
left a comment
There was a problem hiding this comment.
Some incredibly minor nits, up to you if you want to include or not. Otherwise, LGTM
…fallback - Support lowercase status field on container events as an action fallback in waitForContainerEvent and add unit test coverage. - Update comment in scheduleUpdate to reflect the while-loop cycle draining mechanism instead of referring to a stale finally block. - Include tsconfig.json in watched files lists across AGENTS.md, CHANGELOG.md, and README.md. Assisted-By: opencode Signed-off-by: Stan Lewis <gashcrumb@gmail.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…2.1 profile Transitive resolution of @backstage/cli-defaults 0.1.6+ by @backstage/cli pulls in @backstage/cli-module-package-manager-yarn, which contains an unresolved Yarn patch locator (got@patch:got@npm%3A11.8.2#~/.yarn/patches/got-npm-11.8.2-c1eb105458.patch) causing yarn install to fail with ENOENT in scaffolded standalone plugin projects. Pinning @backstage/cli-defaults to 0.1.5 in resolutions prevents this transitive resolution and restores clean yarn install in generated projects. Assisted-By: opencode Signed-off-by: Stan Lewis <gashcrumb@gmail.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
2 participants
Summary
Adds continuous watch mode to
plugin dev, readiness polling, and a phased progress UX forstart(RHIDP-16673).plugin dev start --watch/plugin dev update --watch--watchflag onstart: after the runtime is up and ready, falls straight into watch mode instead of exitingrhdh-cli plugin dev update --watch: watchessrc/andpackage.json(chokidar, 500ms debounce) and re-runs the same export/stage/restart cycle as a one-shotupdateon every changecleanupon Podman,dieon Docker) before starting the next cycle, avoiding acrun exec.fiforace on back-to-back restartsRefresh your browser at <url>once RHDH responds to a readiness pollplugin dev start— phased UX[1/4]–[4/4]): build/export, start runtime, install plugins, wait for readiness[3/4]now watches the installer container'sdiedevent instead of blocking on a Compose call, and skips straight through on a re-entrantstartif the installer already exited in a prior run (previously stalled for the full 60s event-wait timeout)[4/4]polls HTTP until RHDH responds, then prints the URLplugin dev update/restart— runtime pre-flightRHDH Local is not running. Run \rhdh-cli plugin dev start` first.` instead of a raw Compose/container error, when the runtime isn't up yetAlso fixed
isIgnoredWatchPath: chokidar v4+ dropped glob-string support for theignoredoption (function/regex/literal path only) — the previous glob array silently matched nothing. Replaced with a path-segment predicate that actually excludesnode_modules/,dist/,dist-dynamic/, anddist-types/from the watcher.Testing
waitForContainerEvent/waitForContainerCleanup,resolveRhdhUrl,waitForRhdhReady,watchUpdate(cycle trigger, debounce, recovery, SIGINT, not-running guard),isIgnoredWatchPath,start --watchentry/skip,update/restartpre-flight guardsCloses #RHIDP-16673