ci: cover the web e2e write path and seed the cargo caches from main - #1246
ci: cover the web e2e write path and seed the cargo caches from main#1246FSM1 wants to merge 6 commits into
Conversation
The smoke slice gains an `ipfs/kubo` service, `KUBO_API_URL` and a `VITE_READ_ACCELERATOR_URL` pointed at that stack's gateway, which is what makes a write assertion possible: without a pin store the API refuses every hosted upload with a 503, and without a content source the drain cannot read back the state it publishes onto. The gateway is addressed as 127.0.0.1 because Kubo serves `localhost` as a subdomain gateway and 301s every path request to a host that resolves nowhere. On top of that: folder create, rename, move and delete specs plus an upload read back byte for byte, all bound to shipped test ids. Each case ends on a drained queue with no dead letter, and queues a probe folder behind the write under test so a delete or a move out cannot settle vacuously. The e2e project now takes every spec but the bundle-shape one, so a new spec is in the gate the day it lands. Separately, a `Cargo Cache Seed` workflow writes two cargo cache entries on main, and the PR cargo jobs gain a trailing restore-key that reaches them. `ci.yml` runs on `pull_request` only and GitHub scopes a cache entry to the ref that wrote it, so every PR's first cargo run was cold.
WalkthroughThe PR seeds native and WASM Cargo caches from ChangesCI cache and web E2E write-path
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant FilesPage
participant WebAPI
participant Kubo
participant VaultPage
Playwright->>FilesPage: perform folder or upload action
FilesPage->>WebAPI: submit file operation
WebAPI->>Kubo: store hosted content
VaultPage->>WebAPI: settle and read uploaded content
VaultPage-->>Playwright: return decoded file bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/web-e2e/page-objects/files.page.ts`:
- Around line 91-97: The FilesPage.preview() path only validates decoded text,
so add a download helper in tests/web-e2e/page-objects/files.page.ts that
returns the raw facade.download() bytes as Uint8Array. In
tests/web-e2e/tests/write-path.spec.ts, replace the upload round-trip text
assertion with byte-for-byte comparison against a Uint8Array fixture, retaining
Buffer conversion only where Playwright upload requires it.
- Around line 84-89: Update FilesPage.upload() in
tests/web-e2e/page-objects/files.page.ts to accept Uint8Array and pass
Buffer.from(bytes) as the Playwright buffer. Update its caller in
tests/web-e2e/tests/write-path.spec.ts to provide a Uint8Array, preserving the
existing upload behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9df815b7-e29e-40b9-9f2d-64fae54f7fa8
📒 Files selected for processing (8)
.github/workflows/cargo-cache-seed.yml.github/workflows/ci.yml.github/workflows/web-e2e.ymltests/web-e2e/README.mdtests/web-e2e/page-objects/files.page.tstests/web-e2e/page-objects/vault.page.tstests/web-e2e/playwright.config.tstests/web-e2e/tests/write-path.spec.ts
The round-trip test is named for bytes and asserted decoded text. The preview renders through a fatal UTF-8 decode that folds a BOM, so a byte the round trip changed and the decoder swallowed left the assertion green. FilesPage.download saves the file the way a member would and returns what reached disk, and the test compares that with the uploaded bytes. The preview assertion stays: it proves the read path renders, which the saved bytes do not. upload takes Uint8Array now, converting to Playwright's Buffer payload at the boundary rather than making every caller hold one.
The byte assertion timed out: preview leaves its dialog open, and the modal backdrop intercepts every click meant for a row, so the download action was never reached and no download event fired. preview closes the dialog it opened and waits for it to go, which is what a caller reading a file back then acting on the listing needs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/engine/introspection.test.ts (1)
77-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd invalid-input coverage for
nodeHex.The test covers only successful conversion. Add cases for odd-length and non-hex values. Assert that the bridge rejects with
TypeErrorbeforeclient.facade.downloadis called.As per path instructions, tests under
**/*.{test,itest}.tsmust cover edge cases and assert behavior. The suppliedfromHexcontract defines these failure cases.🤖 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/web/src/engine/introspection.test.ts` around lines 77 - 87, Add invalid-input tests for the `window.__CIPHERBOX_ENGINE__?.download` bridge covering odd-length and non-hex `nodeHex` values. Assert each call rejects with `TypeError` and verify `client.facade.download` is not called, while preserving the existing successful conversion test.Source: Path instructions
🤖 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.
Nitpick comments:
In `@apps/web/src/engine/introspection.test.ts`:
- Around line 77-87: Add invalid-input tests for the
`window.__CIPHERBOX_ENGINE__?.download` bridge covering odd-length and non-hex
`nodeHex` values. Assert each call rejects with `TypeError` and verify
`client.facade.download` is not called, while preserving the existing successful
conversion test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0306476-6a56-4e99-b630-86090d9b2524
📒 Files selected for processing (5)
apps/web/src/engine/introspection.test.tsapps/web/src/engine/introspection.tstests/web-e2e/page-objects/files.page.tstests/web-e2e/page-objects/vault.page.tstests/web-e2e/tests/write-path.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/web-e2e/tests/write-path.spec.ts
- tests/web-e2e/page-objects/files.page.ts
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
introspection.test.ts covers only the successful nodeHex conversion — add odd-length and non-hex cases, asserting rejection before facade.download is called |
Taken. The contract checks out: fromHex throws TypeError on an odd length and again on a non-hex character, and the tap is async, so both surface as rejections. |
it.each(['010', 'zz'])('refuses %s as a node id, before the engine is asked', async (bad) => {The expect(download).not.toHaveBeenCalled() half is the part worth having. It pins the ordering — decode first, ask the engine second — which is the only thing about this bridge a reader could get wrong. 'zz' rather than an uppercase pair, because fromHex accepts A-F.
Negative-controlled: moving client.facade.download(…) ahead of the fromHex call fails all three download cases. Reverted; the tap is byte-identical to 8ed3d76.
Earlier findings
Both actionable findings from the f927e01 review are resolved, and the second one is resolved differently from what my first reply said — see that thread. The saved-bytes route failed Web E2E three times; the third failure showed the browser saving the app's own index.html, because the /stream/<ticket> request reached the origin server instead of the Service Worker. The byte assertion now runs through an engine tap, and whether that Service Worker bypass reaches real browsers is left open rather than settled here.
Gates
| Gate | Result |
|---|---|
pnpm typecheck |
0 |
pnpm lint |
0 |
pnpm lint:tracker-refs |
0 |
apps/web unit suite |
12/12 in introspection.test.ts |
Web E2E Smoke on 8ed3d76 |
success |
Web E2E is the gate that matters for this PR and it is green on the head this nitpick was raised against; 71e1d14 adds a unit test only.
Generated by Claude Code
|
@coderabbitai review
Generated by Claude Code |
|
✅ Action performedReview finished.
|
|
Held as a draft: blocked on UAT, which is blocked on #1253. CI is green and CodeRabbit's verdict on What is unverifiedWhether an ordinary browser saves the file's own bytes or the app shell. It cannot be settled inside Playwright, because Playwright's own CDP download capture is the suspect. It needs a real browser, a real session, DevTools open, and a check that the worker is controlling the page before the click — if it is not, Why it is blockedLogin is down on every method. Google returns What this PR does not depend onNothing here rests on that verification. The round trip asserts through the preview path (buffered Flip back to ready once a session can be created and the check is run. |
2 participants
Closes #1096
Closes #1153
Two CI-owned slices in one PR: they share no file.
The web e2e write path
web-e2e.ymlgains anipfs/kuboservice mirroring the contract job, plus thetwo variables the write path needs:
KUBO_API_URL— without itKuboPinStorerefuses every hosted upload with a503, and since PR fix(engine): settle a refused upload and isolate an unreadable sweep node #1217 the drain charges that as a spent attempt, so a write
spec would dead-letter rather than fail on an assertion.
VITE_READ_ACCELERATOR_URLpointed at the same stack's gateway. This one isnot optional either: the drain's rebase leg reads the record head block back
before it publishes onto it, and with no content source configured the queue
never drains at all. It is addressed as
127.0.0.1, notlocalhost, becauseKubo serves
localhostas a subdomain gateway and 301s every path requestto
<cid>.ipfs.localhost, which resolves nowhere in a browser.Five new specs in
tests/web-e2e/tests/write-path.spec.ts, bound to shippeddata-testids and shipped accessible names — folder create, rename, move anddelete, and an upload read back byte for byte through the preview. Two
deliberate shapes:
screen" is the optimistic overlay and would pass over a write that never
published.
queue is strict FIFO, so the probe's pending mark clears only once everything
ahead of it published — otherwise a delete or a move out takes its own row off
the root and leaves the settle nothing to wait on.
The
e2eproject now takes every spec except the bundle-shape one, so a newspec joins the gate the day it lands instead of when someone remembers to widen
a list.
retries: 0is untouched: the suite ran 78/78 green locally across sixrepeats of the full file.
tests/web-e2e/README.mdloses the "does not cover yet" section, which is nowfalse, and its local recipe gains Kubo and both variables.
Seeding the cargo caches from main
ci.ymltriggers onpull_requestonly, and GitHub scopes a cache entry to theref that wrote it, so every PR's first cargo run was cold on every cargo job. A
run can read entries scoped to the default branch, so a new
Cargo Cache Seedworkflow writes two of them on main-push and the PR jobs gain a trailing
restore-key that reaches them:
main-cargo-linux-cargo-,core-kats-cargo-,client-browser-cargo-main-wasm-cargo-wasm-engine-cargo-,web-e2e-cargo-Two entries rather than one per job prefix, because the repo's cache budget is
already over the limit and these have to earn their bytes against the PR-scoped
entries LRU evicts. The keys hash
Cargo.lockplusrust-toolchain.toml— thetwo inputs that invalidate a dependency tree — so a merge that changes neither
hits its own key and writes nothing at all; the trigger is narrowed to the same
two files. A weekly cron and
workflow_dispatchcover an entry evicted betweenlockfile changes. macOS, Windows and the desktop matrix are deliberately not
seeded: they would add four more multi-GB entries on the most expensive runners.
Verification
pnpm lint:tracker-refs,pnpm typecheck,pnpm lint— all clean.zizmor --no-online-audits .github/workflows/ .github/actions/(the pinned1.25.2) — no findings; the new workflow is not added to any ignore list.
pnpm --filter @cipherbox/web-e2e test:e2e --repeat-each=6against a livelocal stack — 78 passed.
Found on the way, not fixed here
Two runtime findings outside this PR's files, both reproducible:
useFileDownloadmints a
/stream/<ticket>URL and clicks an anchor at it; the Service Workerdoes not intercept that request, so the origin answers with the SPA fallback
and the user gets a 394-byte
index.htmlunder the file's name, with no errorsurfaced. Reproduced 3/3. This is why the round trip asserts through the
preview — the buffered read — rather than through the save.
queue drains on neither; polling the footer's refresh control at 1 Hz kept a
single queued folder create unpublished for over 60 s, while one click settles
it in ~1.8 s. The suite works around it by forcing exactly one pass and then
waiting passively.
Also worth wiring by whoever owns
apps/web:engineHostConfignever setsprofile, so aVITE_ENVIRONMENT=cibundle runs the production 30 s pollcadence rather than the CI profile's 1 s. That is what makes the forced-pass
workaround necessary at all.
Note
Add web E2E write-path tests and seed Cargo caches from main branch
mainand weekly, populatingmain-cargo-andmain-wasm-cargo-cache keys as fallbacks for PR builds.KUBO_API_URLandVITE_READ_ACCELERATOR_URLenv vars wired for tests.window.__CIPHERBOX_ENGINE__.downloadintrospection hook.smoke.spec.tsto all tests exceptrelease-bundle.spec.ts.Macroscope summarized 71e1d14.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation