DOC-2235: Document the Visual pipeline editor for Redpanda Connect - #651
DOC-2235: Document the Visual pipeline editor for Redpanda Connect#651JakeSCahill wants to merge 24 commits into
Conversation
Adds a new "Explore the Visual editor" section to the Connect quickstart and a July 2026 changelog entry, plus a Doc Detective test embedded inline in the quickstart that verifies the documented procedure against the real Cloud console and regenerates the canvas screenshot in the same run. Also fixes a sharp/@img platform-binary version mismatch in package-lock.json (would have broken the daily screenshot-test cron job on GitHub's real linux-x64 runners) and adds the GitHub Actions workflow that runs the test daily against preprod and prod. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Deploy Preview for rp-cloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Visual editor documentation for Redpanda Connect pipelines, including an embedded UI test and screenshot. Introduces Doc Detective scripts for inline testing, screenshot-size enforcement, cleanup, and screenshot standards linting. Adds package configuration, environment templates, and operational documentation. Adds GitHub Actions for scheduled/manual cloud screenshot tests and pull-request screenshot linting. Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…path A future inline test on a new page would otherwise require remembering to add it to a manually maintained input array. Doc Detective already skips pages with no inline test comments cheaply, so scanning the whole tree costs nothing extra and needs no registration step per new test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 @.github/workflows/doc-detective-screenshots.yml:
- Around line 57-66: Replace the hardcoded CLOUD_LOGIN_EMAIL values in the
preprod and production branches with environment-specific GitHub Actions
secrets, and write those secret values into tests/doc-detective/.env. Keep the
existing environment selection and password handling unchanged, using distinct
secrets for each test account.
- Around line 41-42: Apply least-privilege checkout settings in both workflow
files: add top-level permissions granting only contents read and configure each
actions/checkout@v4 step with persist-credentials disabled. Update
.github/workflows/doc-detective-screenshots.yml lines 41-42 and
.github/workflows/screenshot-standards-lint.yml lines 17-18; both sites require
the same change.
In `@modules/develop/pages/connect/connect-quickstart.adoc`:
- Line 387: Update the pipeline setup instruction near the Visual tab step to
explicitly direct users to create a new pipeline or open an existing one before
selecting the Visual tab. Align the documentation with the test flow that
navigates to the pipeline creation page, while preserving the existing
YAML/Visual tab guidance.
- Around line 434-443: Correct the screenshot fixture’s typed YAML keys by
changing the literal ppipeline: and ooutput: entries to pipeline: and output:.
Preserve the surrounding SHIFT and wait steps so the keys remain at the intended
top-level indentation and the input/processors/output structure is represented
correctly.
- Line 416: Update the Doc Detective step identified by the find configuration
for the “Save” element so it performs a click by enabling the click option,
matching the interaction behavior of the surrounding steps.
In `@tests/doc-detective/.env.example`:
- Around line 1-4: Update the comments in .env.example to state that login is
performed by embedded steps in the AsciiDoc tests, specifically
connect-quickstart.adoc, and remove references to login-inline.json,
config-level beforeAny, and run-inline.js.
In `@tests/doc-detective/lint-screenshots.js`:
- Around line 38-49: Update resolveImagePath to accept the current filePath and
resolve module-local image references such as image::filename.png[] against the
current module’s images directory, deriving the module from filePath. Preserve
existing remote-image handling and cross-module module:filename resolution, and
update its callers to pass filePath so existence and size checks run for local
references.
In `@tests/doc-detective/README.md`:
- Around line 95-101: Update the README description of lint-screenshots.js to
remove the claim that it validates screenshot naming, unless a corresponding
naming check is implemented. Keep the documented checks for alt text and
resolvable image existence/size accurate.
In `@tests/doc-detective/run-inline.js`:
- Around line 108-111: Redact resolved credentials and other sensitive step data
before serializing report in tests/doc-detective/run-inline.js lines 108-111,
then persist only the sanitized report. Update
.github/workflows/doc-detective-screenshots.yml lines 74-82 to upload that
sanitized report, or remove the report artifact upload entirely.
- Around line 115-116: The exit-code logic in run-inline.js must treat skipped
validation contexts as failures, not only explicit test failures. Update the
condition around report.summary.tests.fail so any requested inline test/context
with a non-PASS result, including SKIPPED, sets process.exitCode to 1 while
preserving the withinSizeLimit failure behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5e369c3-c49c-46b8-9d5a-891ba9b99959
⛔ Files ignored due to path filters (2)
modules/shared/images/rpcn-visual-editor-canvas-v26-2.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/doc-detective-screenshots.yml.github/workflows/screenshot-standards-lint.yml.gitignoremodules/develop/pages/connect/connect-quickstart.adocmodules/get-started/pages/whats-new-cloud.adocpackage.jsontests/doc-detective/.env.exampletests/doc-detective/README.mdtests/doc-detective/lint-screenshots.jstests/doc-detective/run-inline.js
- Redact resolved credentials before persisting last-inline-report.json, and stop treating SKIPPED test contexts as an implicit pass — a broken CI environment that never launches a browser was previously reported as a successful run. - Add least-privilege permissions and persist-credentials: false to both workflows; move test-account emails to secrets alongside their passwords (PREPROD_EMAIL, EMAIL). - Fix the inline test: "Open a pipeline" -> "Open an existing pipeline, or create a new one" to match what the test itself does; add the missing click:true on the Save step so it actually verifies Save works instead of only locating the button. - lint-screenshots.js now resolves same-module image:: references (no "module:" prefix) instead of silently skipping them — surfaced 5 more pre-existing oversized images alongside the 9 already known, all out of scope for this PR. - Fix a stale .env.example comment left over from an earlier login-flow design, and a README claim that the linter checks image naming (it doesn't). Not applying the "ppipeline:"/"ooutput:" suggestion: those are an intentional, tested workaround for a real WebDriver/Monaco bug where the character typed immediately after a Shift+Home selection gets dropped, documented in tests/doc-detective/README.md's Gotchas section. Replying on the PR with that context.
Compresses the 13 pre-existing oversized PNGs (5 newly caught by the lint-screenshots.js module-local-image fix, 9 already known) down to the 100KB ceiling: resize to the standard's own spec (960px display width at 2x retina = 1920px file width, the actual root cause for most of these — several were captured well past that) plus palette PNG quantization where resizing alone wasn't enough. Verified visually, no discernible quality loss. Also fixes cloud-mcp.gif (2236KB, 143 frames) via gifsicle: reduced frame count, lossy compression, and resize, down to 80KB. Adds tests/doc-detective/fix-oversized-image.js so the next oversized PNG doesn't need this figured out from scratch, and documents the GIF approach (which needs visual judgment calls a script shouldn't make silently) in tests/doc-detective/README.md. Adds gifsicle as a devDependency for portable GIF tooling.
- Step 3's "*+*" (bold-wrapped bare plus sign) hit an AsciiDoc landmine: a "+" with no adjacent whitespace on either side is the delimiter for the +...+ passthrough macro. With two such spans on the same line, they paired up and swallowed everything between them, rendering as stray literal asterisks with the button description missing. Backslash-escaping was tried and found unreliable (Asciidoctor rendered the first occurrence clean but literally showed the backslash on the second). Fixed by rephrasing to "plus-sign icon" — also more correct per the style guide, since an icon with no visible label isn't "UI text" that bold is meant to indicate. - The screenshot crop bled into the tab bar (top) and a toolbar row (bottom) by a partial, cut-off sliver of each — not from the padding being too small, but too large: `.react-flow`'s own bounding box starts right at the canvas edge, so any padding at all pulls in whatever sits just outside it. Zeroed top/bottom padding, kept it on left/right where there was no bleed. - The example pipeline's "generate" input had no `mapping` field, i.e. no actual logic for what data to generate — the linter's cascading "expected object value, got !!null" / "field X is invalid" errors on generate were a real, confusing symptom of that real gap, not just cosmetic noise. Added one. (Tried the same investigation for the remaining `log` processor errors and a click-to-refresh workaround for the Problems-count display, but the latter proved flaky/ non-deterministic run to run — reverted rather than ship something that only sometimes reduces the count. The Problems/Unsaved chips showing a nonzero count is exactly the UI behavior this section of the docs describes, so it's left as whatever the real state is.)
Verified live against preprod: introducing a YAML syntax error does NOT make the Visual tab keep showing the last valid pipeline. It shows an explicit "Unable to visualize this pipeline — fix the YAML in the YAML tab" banner with skeleton placeholders instead. The "keeps showing last valid" behavior is real, but belongs to the YAML tab's own structure tree sidebar (a separate feature, confirmed showing "Showing the last valid outline — the current YAML is invalid"), not the Visual tab itself — the original text conflated the two. Also confirmed the other half of that paragraph is accurate: editing a node's config and closing its panel (not just clicking Save or selecting another node) does write the edit into the YAML immediately, verified by editing a field, closing without saving, and checking the YAML tab directly.
Previously the workflow only ran our own run-inline.js wrapper and uploaded artifacts for a human to check by hand — the official action's create_pr_on_change/create_issue_on_fail capability was documented as "future scope" but never wired up. Switches the CI workflow to run the tests via doc-detective/github-action directly: - create_pr_on_change: true — a screenshot diff beyond the accepted threshold now opens a PR automatically instead of silently updating a build artifact nobody looks at. - create_issue_on_fail: true — a genuine step failure (not a screenshot diff) files an issue instead. Deliberately does NOT use the action's default issue_body, which embeds the raw $RESULTS object — that contains the resolved CLOUD_LOGIN_PASSWORD substituted into this test's login steps, posted via a direct API call that bypasses GitHub's log secret-masking. Overridden to link the (correctly masked) run logs instead. - Documents the action's built-in `integrations` input (including `claude`) as the ready-to-enable hook for the "point an LLM at the filed issue" future work — already built into the action we're now using, just not turned on until a claude-mentions-responding integration exists for this repo. Extracted the test config from an inline object in run-inline.js into tests/doc-detective/config.json so local runs and the action's own `config` input use identical settings instead of two copies that can drift apart. Known gap, called out in both the workflow comments and the README: the action installs its own copy of doc-detective rather than using this repo's node_modules, so it doesn't inherit the sharp/@img version-pinning fix in package.json's overrides. Needs a manual workflow_dispatch run to confirm it doesn't hit that same upstream conflict before trusting the daily cron on it.
The previous guidance assumed act defaults to a safe fake GITHUB_TOKEN. It doesn't: with no token explicitly provided, act silently pulls a real, authenticated one from the local gh CLI session. A local test run of the new doc-detective/github-action integration hit this and filed a real issue (#652, closed once caught) against this repo from what was supposed to be a local dry run. Verified the fix (-s GITHUB_TOKEN=<garbage>) actually prevents any real API mutation, but it's blunt: act uses the same token to `git clone` the marketplace action itself, so a bad token breaks that too, before ever reaching create_pr_on_change/create_issue_on_fail. There's no way to give act a token valid enough to fetch a public action but denied write access to this repo specifically. Net effect documented plainly: this workflow's PR/issue-creation behavior can't be safely dry-run locally at all. act is only useful here to sanity-check job structure/secrets wiring (expect it to fail at the action-clone step with the safe token). Login/browser/screenshot behavior should be validated via run-inline.js directly instead, and the actual create_pr_on_change/create_issue_on_fail behavior needs a real workflow_dispatch run once this workflow is reachable on GitHub.
micheleRP
left a comment
micheleRP
left a comment
There was a problem hiding this comment.
Review: DOC-2235 Document the Visual pipeline editor
Files reviewed: 2 .adoc pages, 2 workflows, 3 Node scripts, config.json, .env.example, the 425-line README, package.json/package-lock.json, .gitignore, and all 15 changed images. Branch is 10 commits behind origin/main.
Overall assessment: The framework around this is unusually well built, and the inline test genuinely passing 70/70 against preprod is real evidence. But the documented procedure doesn't work as written: it sends the reader to a read-only canvas, and three specific behavioral claims contradict the shipped source. Those need fixing before merge, along with the screenshot and one badly degraded GIF.
What this PR does
Adds an "Explore the Visual editor" section to connect-quickstart.adoc covering the new node-based canvas alongside the YAML editor, plus a What's New entry. It also introduces a Doc Detective screenshot-testing framework: test steps embedded inline in the page, a daily cron that replays the procedure against preprod and prod and auto-opens a PR when the canvas screenshot drifts, and a PR-time screenshot-standards lint.
Jira ticket alignment
DOC-2235 ("Document feature: Visual pipeline editor for Redpanda Connect", labels 26.2-Feature, P2-Feature, RP-Connect, status In Review). The ticket body only points at ENG-1217, so there are no explicit acceptance criteria. The PR covers the feature and the changelog, so it satisfies the ticket.
How I verified the behavioral claims
Findings 1 through 3 started from opening a real pipeline on prod, where the tabs were Monitor and YAML with no Visual tab, which didn't match this PR. Tracing that back gave the rest.
Sources: redpanda-data/console @ master (frontend/src/components/pages/rp-connect/pipeline/) and redpanda-data/cloudv2 @ origin/main (457feaa64b), both fetched 2026-08-05. The Visual editor UI lives in console, not cloudv2: cloudv2's rp-connect/create route is just an <Outlet />, and the flags are passed through to the embedded Console.
Critical: the procedure doesn't work as written
1. Step 1 lands the reader on a read-only canvas. Step 1 says "Open an existing pipeline, or create a new one, and click the Visual tab (next to YAML)", and steps 2 through 5 then add components, edit node config, and click Save. None of that is possible from an opened pipeline. The tab sets differ by mode (index.tsx:1144-1165):
| Mode | Tabs |
|---|---|
| view | Monitor, YAML (internal value configuration), plus Visual when enabled |
| create / edit | YAML, Visual, and no tabs at all when the flag is off |
In view mode the panel is rendered with mode="view" (index.tsx:1254-1263), and visual-editor-panel.tsx:389 sets isEditing = mode !== 'view' with readOnly={!isEditing} at line 919. The reader has to click Edit pipeline first.
Fix: "Open a pipeline and click Edit pipeline, or create a new pipeline, then click the Visual tab (next to YAML)." Worth one sentence noting that opening a pipeline without editing gives a read-only canvas, since that is what a reader will hit first.
2. Step 5 has the Save semantics backwards. Step 5 says "Click Save. Edits you made on the Visual tab are written to the pipeline's YAML at this point", while the paragraph after the TIP says edits are written into the YAML immediately without needing Save. The second one is right. From node-inspector.tsx:109-111: "The selected node's pending-edit hooks; the panel flushes them on node-leave / save, no per-node Apply button" and "Flush pending edits into the YAML immediately (field-blur / Apply commits, not just node-leave)". Save persists the pipeline to the server; index.tsx:280 flushes any in-progress edit first, then reads fresh YAML for the request.
Fix: step 5 becomes "Click Save to persist the pipeline." Drop "at this point", and the trailing paragraph can stay as the explanation.
3. The structure-tree TIP is wrong on both of its claims. It says "a structure tree replaces the sidebar's connector shortcuts once your pipeline has at least one node".
- There is no node-count condition:
index.tsx:662isconst showStructureTree = isPipelineDiagramsEnabled. Empty sections render an italic "empty" note (pipeline-structure-tree.tsx:380-384), so the tree is there from the start. - It does not replace the connector shortcuts. The tree renders above the connector card, and in edit mode that card stays with only its Input/Output shortcuts hidden (
hideInputOutput={isPipelineDiagramsEnabled},index.tsx:787).
The test asserting [role='tree'] passes either way, so it can't catch this.
Fix: "On the YAML tab, a structure tree in the sidebar outlines your pipeline, so you can jump to any node. Adding it hides the sidebar's Input and Output shortcuts; the Processor, Cache, and Buffer shortcuts stay."
Minor, same area: the chips render as "7 problems" and "3 unsaved", a count plus a lowercase word (pipeline-unsaved-panel.tsx:37), not Problems and Unsaved.
Critical: availability
4. The feature is behind two flags that both default to off, and the docs say nothing about it.
// console index.tsx:849-850
const isVisualEditorFeatureEnabled = (): boolean =>
isFeatureFlagEnabled('enableRpcnVisualEditor') && isFeatureFlagEnabled('enablePipelineDiagrams') && isEmbedded();cloudv2 app-constants.ts:71 defines enable-rpcn-visual-editor with default false, and :85 defines enable-pipeline-diagrams with default false. The code does ship in the pinned build (install-pack/26.2.yml:19 pins console_image_tag: master-64821f2, and visual-editor-panel.tsx exists at that commit), so this is purely flag rollout.
Concretely: on prod today, an org without those flags sees Monitor and YAML only, with no Visual tab anywhere. Both the quickstart section and the changelog entry describe the tab as simply present, so readers will look for something they don't have. Worth an availability note in both places. I'm deliberately not proposing a maturity label; that needs a call from whoever owns the ENG-1217 rollout.
Critical: images
5. The one screenshot of the feature shows a broken pipeline, and hides the context the reader needs. rpcn-visual-editor-canvas-v26-2.png shows a red "7 problems" chip, a "3 unsaved" chip, and the input and processor nodes outlined in red with error badges. It is also cropped to .react-flow, so the YAML | Visual tab strip is not visible, which matters a lot given finding 1: the image can't show which mode the reader must be in. And the alt text says "a generate input node" while the image shows three nodes (generate, log, drop). The output node's right edge is clipped too.
Fix: capture in edit mode, on a fixture that lints clean, with the tab strip included. That resolves the error state, the missing mode context, and the clipping in one pass, and the alt text can then describe all three nodes.
6. modules/shared/images/cloud-mcp.gif is now roughly 10x too fast and half size. Measured: it was 740x324, 143 frames at a uniform 200 ms delay (28.6 s total); it is now 400x175, 21 frames at a uniform 140 ms delay (2.94 s total). Keeping every 7th frame is fine, but the per-frame delay needed to be 7x the original, so 140 centiseconds, not --delay=14. It renders on one page, develop/cloud-mcp/overview.adoc:21, so the deploy preview can be compared against production directly.
The README bakes the same error in, so the next person repeats it. tests/doc-detective/README.md:168-172 says "delay 14 (2x the original ~7)", but the original delay is 20 centiseconds and the factor is 7x:
npx gifsicle -O3 --lossy=180 --colors 48 --resize-width 400 --delay=140 ...At 400 px the terminal text is likely too small regardless, so consider keeping more frames at a larger width, or replacing the GIF with a static image plus a link to a recording.
Suggestions
7. Section placement. == Explore the Visual editor sits after == Clean up, which deletes both pipelines and the dad-jokes topic. The reader tears everything down, then is told to open or create a pipeline. Move it before == Clean up, or split it onto its own page under develop:connect/ and link from the quickstart. Given findings 1 and 4, a separate page may be the better home: it needs a mode explanation and an availability note that don't really belong mid-quickstart.
8. The test never exercises the path the prose describes first. It navigates straight to /rp-connect/create, so create mode only. That's also why [data-value='yaml'] works: in view mode the YAML tab's value is configuration. If step 1 is going to mention opening an existing pipeline, that path deserves coverage, or the prose should stop implying it.
9. Terminology drifts between "view" and "tab". The top TIP says "two views: YAML and Visual" and "This quickstart uses the YAML view", while the new section, its TIP, and the What's New entry all say "tab". Standardize on "tab", which matches the UI.
10. Same-page link. The TIP links within its own page using a full resource ID: xref:develop:connect/connect-quickstart.adoc#visual-editor[...]. The repo has 106 internal anchor links; <<visual-editor,Explore the Visual editor>> is the local convention and survives a rename.
11. Versioned image filename. rpcn-visual-editor-canvas-v26-2.png is the only versioned filename in modules/shared/images/. The daily job overwrites this same file in place, so the v26-2 tracks nothing and will go stale. Suggest rpcn-visual-editor-canvas.png.
12. Optional additions to the section. The canvas also supports Escape to deselect, Delete to remove a node, and Cmd+Z / Cmd+Y for undo and redo (visual-editor-panel.tsx, canvasKeyAction), with undo and redo buttons in the top left. Also worth knowing that the / palette on the canvas is independent of the separate enableConnectSlashMenu flag, which governs / inside the Monaco YAML editor (index.tsx:869, :917), so the claim as written is correct.
13. screenshot-standards-lint.yml and the metadata lint should be settled together.
- Its
paths:filter means it cannot be a required status check without leaving PRs stuck pending, the same trap documented inlint-metadata.yml. If it is meant to be required, drop the filter. - The filter covers
modules/shared/images/**only, but the script scans every module, so an oversized PNG added undermodules/reference/images/with no.adocchange won't trigger it. - The script scans all 946 pages with no allowlist, which is why this PR had to rewrite 14 pre-existing images. The allowlist plus non-blocking ratchet in
lint-metadata.ymlwould let the gate land without that churn.
14. 14 unrelated images were recompressed to satisfy the new lint (commit 137cc5cd): the 5 SQL venn diagrams, byoc_apply.png, cloud_metrics.png, connect_ui.png, control-flows.png, control_d_plane.png, data-flows.png, metadata-flows.png, redpanda_connect_dashboard.png, and cloud-mcp.gif. I compared connect_ui.png (3434x2380 to 1920x1331, 866 KB to 76 KB) and join-venn.png before and after; both hold up. Only the GIF is damaged. Still, this is scope beyond DOC-2235 and it makes the diff hard to read. A separate compression PR would let the venn diagrams and the GIF get individual attention.
15. The CI path skips the post-processing the local runner does. CI runs doc-detective/github-action, not run-inline.js, so enforceScreenshotSizeLimit() (lossless recompression plus the 100 KB check) and cleanUpScreenshotTempFiles() never run there. An auto-PR from the daily cron can therefore commit a screenshot over the ceiling, which this repo's own new lint would then fail on that very PR, or a leftover <uuid>_<timestamp>.png. Either run run-inline.js in CI or replicate that pass after the action step.
16. The prod cron will file an issue every day. create_issue_on_fail: true plus a daily schedule plus flags that default off equals one new issue per day, indefinitely. Given finding 4, restricting the schedule to preprod until the flags are on, then adding prod, avoids the noise entirely.
17. Hardcoded cluster IDs in a public repo. d9f1nf0d6v8fd1orsa00 (preprod) and cnk4fdktluj4mbfiarq0 (prod) are committed in doc-detective-screenshots.yml. Credentials are correctly in secrets. Moving the IDs to repo variables keeps org resource identifiers out of a public repo and makes a recreated cluster a variable edit.
18. The lockfile doubles: 1,172 to 2,292 packages. doc-detective-core brings the Appium and WebDriver tree, and netlify.toml sets no NPM_FLAGS, so every deploy preview for this repo installs it. NPM_FLAGS = "--omit=dev" should be safe since Antora is a regular dependency, or the test tooling could live in its own package.json under tests/.
19. Two stale comments. run-inline.js's redactSecrets comment says the report "is uploaded as a CI artifact for debugging failures", but the merged workflow has no artifact upload. And cleanUpScreenshotTempFiles() sweeps only modules/shared/images while its comment reads as a general sweep, so a screenshot written to another module's images/ dir would leave temp files behind.
20. Unverified: the workflow uses $RUN_URL in pr_body, but its own comments only document that variable for issue_body. If the action doesn't substitute it there, the auto-PR body shows a literal $RUN_URL. Worth checking on the first manual workflow_dispatch run.
21. Question, not a defect: roughly 50 inline // (step ...) lines now interleave with the prose steps. That colocation is the point of inline tests and keeps steps next to what they verify, but it also means writers editing this quickstart edit around WebDriver selectors. Worth a team decision on whether pages we expect frequent prose edits on should use a separate spec file.
Impact on other files
nav.adoc: no change needed; this adds a section to an existing page. If the section moves to its own page (finding 7), it needs a nav entry.- Cross-references: nothing renamed or moved, so no stale xrefs.
develop/connect/configuration/resource-management.adocis the only other page mentioning the pipeline editor. Probably no change needed since it covers compute units, but worth a look.
CodeRabbit findings
All 10 are already addressed on the branch, and I verified each rather than trusting the thread: checkout credentials (persist-credentials: false plus top-level contents: read on both workflows), test-account emails moved to secrets, the missing "click":true on the Save step, module-local image resolution in lint-screenshots.js, the .env.example login-flow comment, the README's lint description, and credential redaction in run-inline.js. CodeRabbit withdrew the ppipeline:/ooutput: finding after your explanation, correctly: the doubled leading character absorbs the keystroke Monaco drops after $SHIFT$+Home, and the rendered canvas confirms the YAML comes out structurally right. The only one not fully closed is the "create a pipeline first" comment, which findings 1 and 7 supersede.
What works well
- The
issue_bodycomment explaining why$RESULTSis deliberately omitted, since it would leakCLOUD_LOGIN_PASSWORDin plaintext through an API call that bypasses log masking. That reasoning is what stops a future regression. redactSecrets()does what its comment claims: a global string replace, so a credential nested anywhere in the report is caught.- Failing the run on
skipped > 0, so a broken browser context can't report success while verifying nothing. - The "Gotchas" section is real, hard-won knowledge. The
$HOME$token colliding with a real env var throughreplaceEnvsis a genuinely nasty upstream bug to have found and written down. fix-oversized-image.jsresizing to the standard's own spec before reaching for lossy quantization, and refusing to judge visual quality itself.- Two claims I tried to break and couldn't: nodes are indeed not draggable (
pipeline-flow-canvas.tsx:1325-1330setsnodesDraggable,nodesConnectable, andelementsSelectableall tofalse), and Problems does list lint errors and missing secrets (pipeline-problems-panel.tsx:80-101). - The lint passes cleanly on the branch: 946 files, no violations, which I ran rather than trusting CI.
|
@coderabbitai full review |
|
|
@JakeSCahill when you push fixes for the review above, could you trigger a fresh CodeRabbit full review? |
- Document the Visual tab in connect-quickstart.adoc, sharing the producer pipeline YAML with its test via an Antora example file (modules/develop/examples/connect/producer-pipeline.yaml) so the doc and test can't drift apart. - Add a standalone visual-editor-quickstart.json spec: exercises the documented connector-wizard flow, pastes the pipeline into the editor and captures the Visual-tab screenshot, then cleans up every resource it creates via five independent per-resource cleanup tests (so one missing resource can't block cleanup of the others). - Add a Linux CI variant of the paste test (visual-editor-quickstart-linux) using xclip/Ctrl+V under Xvfb, alongside the existing macOS/local-dev variant (pbcopy/Cmd+V) — headless Chrome's synthetic paste doesn't reliably reach the real OS clipboard, but a real (non-headless) Chrome under a virtual display does, which is what GitHub's ubuntu-latest runners now get via new steps in both doc-detective workflows. - Add select-tests-for-changed-files.js and doc-detective-pr-check.yml so a PR touching a tested doc or spec runs just the associated test(s) against preprod before merge. - Fix a config.json regression that forced headless:false for every platform (would break headless Chrome entirely on a real display-less runner without Xvfb). - Fix a heredoc in doc-detective-screenshots.yml's Claude-investigation step that silently broke the whole file's YAML parsing; move the prompt to its own template file substituted with sed instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
--exit-on-fail was missing from the CLI invocation: without it, doc-detective always exits 0 regardless of test results, so the "Doc Detective PR check" job reported success on PR #651's first real run even though 7/7 tests failed. Also add --auto-screenshot and upload the full results/screenshots as an artifact on failure, so a real failure is diagnosable from the run itself instead of a bare step ID. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real CI run (PR #651) showed the requested 1280x1024 viewport actually rendering as ~1050x861 under Xvfb — Chrome's own window chrome ate into the fixed-size Xvfb screen. The narrower-than-expected width dropped below the Cloud console's responsive breakpoint, silently changing the rendered layout enough that several selectors stopped matching anything. Every resulting failure looked like a generic "element not found," with nothing pointing at viewport size as the actual cause. Fix: size the Xvfb screen (1920x1200) well above the requested browser viewport (1600x1000) instead of matching them. Documented both this and the earlier --exit-on-fail finding in the README so they're not re-discovered from scratch next time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The viewport fix didn't resolve the failures — same exact steps, same step IDs, fail identically regardless of viewport size (945x1037, 1050x861 tried so far), which undermines the responsive-breakpoint theory. Since login succeeds every time but every authenticated, cluster-specific page fails identically right after navigation, this may be a connectivity/permission gate specific to GitHub's runner network rather than a rendering issue. Adding explicit screenshot steps right before the two failing finds, uploaded as an artifact, to see the actual page instead of continuing to guess blindly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mirrors doc-detective-screenshots.yml's existing "Investigate failure with Claude" step, adapted for the PR-check context: on a real test failure, Claude diagnoses it using the artifacts already on disk in the same job (no download needed) and either opens a separate PR against main (never the original PR's own branch) with a fix, plus a comment linking to it, or posts a comment on the original PR stating the diagnosis if it's transient or out of scope. The check itself still just pass/fails on the PR; this only changes what happens after a failure, saving a manual debugging round-trip. Also includes the regenerated Visual-tab baseline screenshot, now that the credentials/cluster ID issue from PR #651's first real runs is fixed (viewport bumped to 1600x1000 earlier changed its aspect ratio). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real runs on PR #651 each ran for 100+ minutes before being manually cancelled: the CLI actually finished all tests and wrote its full results (confirmed from the log timestamps — the "Thanks for using Doc Detective" summary printed, then nothing else happened for over an hour), but the Node process itself never exited. --exit-on-fail sets process.exitCode, which only takes effect once the event loop drains naturally — it can't help if something keeps the process alive indefinitely after the real work is done. Fix: stop trusting the CLI's own process exit entirely. Run it in the background, poll for the results file it reliably writes, force-kill the process (and anything doc-detective/Chrome-related still running) once that file appears or a generous cap elapses, then derive pass/fail directly from the file's own `summary`. Also added a 30-minute job-level timeout-minutes as a second line of defense. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The hang fix let a run actually complete for the first time: 172/177 steps passed, revealing two new, real, unrelated bugs instead of the credential/hang issues masking everything before: - xclip forks into a background process to keep serving the X11 clipboard selection; without redirecting its stdout/stderr away, the runShell step waited on those inherited pipes and timed out after 60s even though the actual clipboard write was instant. Fixed with `> /dev/null 2>&1`. - The wizard test's "Add topic" dialog only got 1000ms to close before the next step clicked "User" — not enough on this environment/ viewport, so the click intercepted on the still-present dialog footer. Bumped to 2500ms. Also corrected the README's earlier viewport-mismatch writeup, which turned out not to explain the failures it was blamed for at the time (the real cause was the stale credentials/cluster pairing, only found once debug screenshots were added). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…er click The Visual-tab baseline was captured locally and its aspect ratio doesn't match what CI actually renders, causing a hard "Images have different aspect ratios" comparison error rather than the usual variance-based overwrite. Deleting it lets the next CI run write a fresh baseline with no prior file to compare against; will commit whatever that run produces once it lands. Also add a screenshot right before the wizard test's "User" click, since bumping the preceding wait to 2500ms didn't change anything about the click-intercept failure at all (same coordinates, same intercepting dialog-footer element) — the dialog may genuinely never be closing, not just closing slowly. Need to see the actual page to tell which. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Full run on real CI: the wizard and paste tests both passed completely (previous bugs fixed), leaving only cleanup-topic-dad-jokes and cleanup-user-connect failing — each right after its target resource had genuinely just been created moments earlier in the same run. 4s wait + 10s find timeout wasn't enough; looks like real backend propagation lag rather than page-render slowness, not flakiness (both failed instantly and deterministically). Bumped both to 8s wait + 15s find timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Failed once on real CI even with the 8s/15s propagation-delay fix already in place, then passed immediately on an unmodified retry — genuinely intermittent, not deterministic like the earlier topic-already-exists bug. Idempotent either way, so documented as a known, low-stakes residual rather than chased further. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two structural fixes so a partial/interrupted run can never permanently wedge the suite, addressing the real remaining gap between "mostly passes" and "works end-to-end with cleanup": - precleanup-topic-dad-jokes and precleanup-user-connect run before connect-quickstart-build-with-wizard, not just after it. The wizard test's Add topic/Add user dialogs hard-error on a duplicate name (confirmed by hand — an earlier claim in this file that this was idempotent was wrong) and leave the dialog open, cascading into failing every later step in that test. A missed prior cleanup used to wedge every subsequent run the same way; precleanup running first breaks that cycle. Verified by hand: ran the wizard test twice in a row with zero cleanup in between — precleanup correctly no-ops on the first pass (nothing to clean yet) then the build test creates the resources and passes; on the second pass, precleanup finds and clears the first pass's leftovers, and the build test passes again. - cleanup-user-connect-retry and cleanup-acl-connect-retry duplicate their respective tests immediately after them. Both originals have a real, intermittent, previously-unresolved click flake on the same Base UI dropdown component. Since the underlying action is idempotent, a retry test is a clean no-op if the first attempt succeeded (its own first find fails cleanly, nothing else runs) and an independent second chance if it didn't. Also corrected an inaccurate claim in connect-quickstart-build-with-wizard's own description that topic/user creation was idempotent on a duplicate name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real CI run surfaced a genuine UI/docs mismatch, not a test bug: the tested preprod cluster's button reads "Create a pipeline" (with the article), not "Create pipeline" — the label this project's earlier live reconnaissance verified against what was apparently a different cluster/build. Rather than keep chasing which wording is "currently correct" across environments this repo can't fully see, the wizard test's selector now matches on 'Create' + 'pipeline' both being present (contains, not exact text), and the doc's matching instruction is updated to what's actually live now, so both variants of this wording keep working. Also fixes a real blind spot in doc-detective-pr-check.yml's own pass/fail derivation, found by running the full local suite twice in a row: the new -retry tests (added to make cleanup genuinely self-healing) are DESIGNED to fail cleanly whenever the primary test they retry already succeeded — the common case — which a naive `summary.tests.fail > 0` check would read as a permanent failure even on a fully successful run. The gating logic now filters out `-retry` test IDs before counting real failures. Same audit caught a second, independent gating bug: `summary.tests.skipped > 0` was also treated as a failure, but visual-editor-quickstart-mac/-linux are platform-scoped via runOn, so exactly one of them is *always*, correctly, skipped on any given runner — that would have made the Linux CI job permanently red for a reason unrelated to test outcomes. Fixed by checking whether at least one non-retry test reached a real verdict at all, instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real CI run (with the previous -retry gating fix already in place) surfaced the same class of mistake applied to a different test category: precleanup-topic-dad-jokes and precleanup-user-connect are ALSO designed to fail cleanly with "element not found" whenever there's nothing left over to pre-clean — the common case — but the gating logic only excluded '*-retry' test IDs, not 'precleanup-*' ones, so it still reported failure on an otherwise fully green run. Fixed by excluding both patterns consistently. That same run also reproduced cleanup-topic-dad-jokes' backend- propagation-delay flake a second time (first seen and documented two commits ago). Added cleanup-topic-dad-jokes-retry, matching the existing cleanup-user-connect-retry/cleanup-acl-connect-retry pattern, since it's the same class of "idempotent action, occasionally needs one more try" problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
precleanup-topic-dad-jokes, cleanup-topic-dad-jokes, and its -retry sibling all still hit "Element not found within timeout" on real CI even after the previous 4s/10s -> 8s/15s bump, and once with the retry missing too in the same run. Diagnostics ruled out pagination and a selector bug, leaving a longer backend-propagation delay as the only explanation, so wait/timeout is bumped again to 15s wait / 20s find timeout. Updated descriptions and README to match.
… auto-investigate New evidence disproves the "just a propagation delay" theory for cleanup-topic-dad-jokes: checked 3 consecutive real-CI runs and it failed every single time, and a debug screenshot from the wizard test's own "Topic already exists" error proved the topic really did exist while neither precleanup-topic-dad-jokes nor cleanup-topic-dad-jokes could find it in the topics list. Added debug-precleanup-topics-list.png and debug-cleanup-topics-list.png screenshots right before the failing find so the next CI run shows exactly what's rendered. Also fixed both "Investigate failure with Claude" steps (doc-detective-pr-check.yml and doc-detective-screenshots.yml), which were failing with "claude: command not found" since the CLI was never installed on the runner.
2 participants
Jira: https://redpandadata.atlassian.net/browse/DOC-2235
Summary
connect-quickstart.adocand a July 2026 changelog entry inwhats-new-cloud.adoctests/doc-detective/README.mdfor how this works).github/workflows/doc-detective-screenshots.yml(daily cron + manual dispatch against preprod/prod/both) and a fast PR-time screenshot-standards lint workflowsharp/@imgplatform-binary version mismatch inpackage-lock.jsonthat would have broken the daily screenshot-test cron job the first time it ran on GitHub's real linux-x64 runners (filed upstream: appium/appium#22554)Preview pages
Test plan
require("sharp")smoke test passes with the corrected lockfileact(seetests/doc-detective/README.md)🤖 Generated with Claude Code