Skip to content

Cypress: Code App - Seeding Task - #3994

Open
geodem127 wants to merge 27 commits into
devfrom
test/code-app-data-seeding
Open

Cypress: Code App - Seeding Task#3994
geodem127 wants to merge 27 commits into
devfrom
test/code-app-data-seeding

Conversation

@geodem127

@geodem127 geodem127 commented Feb 9, 2026

Copy link
Copy Markdown
Contributor
  • added code app data seeding task
  • updated plugin
  • updated type decleration
  • fixed missing content-type header on the script fetch request
  • fixed fragile/incorrect getAuthToken() call (now reuses sdk.token)
  • merged latest dev
  • added cleanup:code task and teardown hook for seeded views/scripts/stylesheets
  • fixed ActionButton dropping data-cy in the inactive/disabled render branch

Note: Had to use native fetch for creating script files since the SDK doesn't support that yet

Test fixtures and e2e coverage for this task will be added in the PR that actually consumes seed:code.

@geodem127 geodem127 self-assigned this Feb 9, 2026
@geodem127
geodem127 force-pushed the test/code-app-data-seeding branch from 583c99b to f401960 Compare February 23, 2026 19:06
@geodem127 geodem127 mentioned this pull request Feb 23, 2026
6 tasks
Comment thread cypress/plugins/seeds/code.ts
geodem127 added 5 commits May 7, 2026 02:34
- Use sdk.token instead of a bare getAuthToken() call, which only
  worked by relying on cached auth state from an earlier call
- Add missing content-type header on the script fetch POST, which
  the API needs to parse the JSON body (was silently failing)
- Simplify async flow to match content.ts conventions
- Add cypress/e2e/code/seed.spec.js plus view/stylesheet/script
  fixtures exercising all three seed:code branches

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Keeping the fixtures — other PRs building on this branch will use them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Will be added in the actual PR that consumes them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/index.d.ts Outdated
Comment thread cypress/plugins/seeds/code.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code Review: Cypress Code App Seeding Task

Overall: The structure and intent are solid and follows the established content.ts pattern well. A few correctness issues need fixing before this lands, plus one type mismatch.


Bugs / Correctness

1. Date.now() instead of UUID for unique names (see inline comment on line 17 of code.ts)
CLAUDE.md explicitly mandates uuidv4 over timestamps for test data uniqueness, because timestamps can collide across the 5 parallel CI runners that share the same COMMIT_ID. See how content.ts handles this.

2. No res.ok guard on the native fetch call (see inline comment on lines 21-26 of code.ts)
A 4xx/5xx from the scripts endpoint will not throw. It silently flows through res.json() (which may itself throw on a non-JSON error body) and returns null. The seed task will appear to succeed, but the calling spec will receive null with no indication of what went wrong. Check res.ok and throw a descriptive error, consistent with how content.ts surfaces failures.

3. Type mismatch: Partial<> in index.d.ts vs. actual return type (see inline comment on lines 73-76 of index.d.ts)
code.ts declares SeedCodeTask = WebView | Stylesheet | Script | null (full objects), but the index.d.ts overload wraps them in Partial<>, making every field optional. Consuming tests would need to null-guard every field access even though a successful API response always returns complete objects. Remove the Partial<> wrappers.


Minor / Nits

4. path parameter shadows the path module import (see inline comment on line 11 of code.ts)
Not a runtime bug (since only join is used, not the path namespace), but it will confuse readers. Rename the parameter to fixturePath to match the pattern from content.ts.


What is Good

  • Reusing sdk.token directly instead of calling getAuthToken() again is the right fix: avoids a second login round-trip and keeps token state consistent with the SDK instance.
  • The STYLESHEET_TYPES constant cleanly centralises the content-type branching logic.
  • Spreading ...code(config) into the task registry follows the exact same pattern as content, making the plugin easy to extend.
  • The decision to use native fetch for script creation (with a PR note explaining why) is appropriately pragmatic given the SDK gap.

Addresses review: silent null returns turned real API failures into
confusing downstream null-reference errors in consuming tests.
Matches the fail-loud pattern content.ts already uses.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/index.d.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code Review

Overview

Adds a seed:code Cypress plugin task that reads a fixture JSON file and creates a WebView, Stylesheet, or Script on the dev instance. The architecture mirrors seed:content — a function exported from a new cypress/plugins/seeds/code.ts module, registered in cypress/plugins/index.js, and typed in cypress/index.d.ts.

The fix for sdk.token reuse (instead of re-calling getAuthToken()) and adding the missing content-type header are both good corrections.


Issues

Must fix

  • Date.now() for uniqueness (line 17) — CLAUDE.md explicitly requires uuidv4 for test data names, not timestamps, because Date.now() collides across the 5 parallel CI runners. content.ts already imports and uses uuidv4(). See inline comment.

  • json.filename vs json.fileName (line 18)Script, Stylesheet, and WebView in src/shell/services/types.ts all declare fileName (camelCase). If the fixture JSON follows the API response shape and uses fileName, then json.filename is undefined here, silently producing paths like /__e2e__/.../1234567890 | undefined. Needs verification against the actual fixture format. See inline comment.

  • No res.ok check before res.json() (line 30) — The native fetch path calls .json() unconditionally. A 5xx or gateway-level error that returns HTML will throw a SyntaxError with no context instead of a useful message. See inline comment.

Nit

  • Partial<> in type declaration (index.d.ts line 73) — The implementation always returns a full API object or throws; it never returns a partial shape. Partial<> just forces consuming tests to null-check fields that are guaranteed present. WebView | Script | Stylesheet is the accurate type. See inline comment.

Other observations

  • No retry logic, whereas content.ts retries createModel up to 3 times to handle API flakiness under parallel runner load. The fetch-based script path is particularly vulnerable. Worth adding before the consuming spec PR lands.
  • The module.exports + ES import mix is consistent with content.ts, so no change needed there.
  • readFileSync blocking in an async function is also consistent with the existing seed — acceptable for a plugin task context.

@geodem127
geodem127 force-pushed the test/code-app-data-seeding branch from cf17404 to 9b6cad5 Compare July 20, 2026 20:07
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/index.d.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/e2e/code/sidebar.spec.js
@github-actions

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 3 warning(s) — see inline comments

Comment thread cypress/e2e/code/sidebar.spec.js
Comment thread cypress/plugins/seeds/code.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments

Comment thread cypress/e2e/code/sidebar.spec.js
Comment thread cypress/e2e/code/sidebar.spec.js
Comment thread cypress/plugins/seeds/code.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 3 warning(s) — see inline comments

Comment thread cypress/plugins/seeds/code.ts
Comment thread cypress/e2e/code/sidebar.spec.js
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments

Comment thread cypress/e2e/code/sidebar.spec.js
Comment thread cypress/plugins/seeds/code.ts
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments

Comment thread cypress/e2e/code/sidebar.spec.js
Comment thread cypress/plugins/seeds/code.ts
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants