refactor(worker-javascript): extract Phase 1 helpers from Core.js

AGENTS.md

@@ -1,51 +1,184 @@
 # AGENTS.md
 
-## Testing
+Canonical guide for agents (and humans) working in this repository. Covers architecture, build commands, testing, coding conventions, commit hygiene, PR creation, and changesets.
+
+## High-Level Architecture
+
+DoenetML is a semantic markup language for building interactive web activities. The system has three layers:
+
+### Layer 1: Parsing (Input)
+The **parser** (`packages/parser`) converts DoenetML XML into a **DAST** (Document Abstract Syntax Tree). It handles XML parsing, validation, and normalization. Key exports: `stringToLezer()`, `lezerToDast()`, `normalizeDocumentDast()`.
+
+### Layer 2: Computation (Worker)
+The **worker** (`packages/doenetml-worker`) runs in a Web Worker and manages document state and computation. It combines:
+- **JavaScript logic** (`packages/doenetml-worker-javascript`) for component evaluation, dependency tracking, and state updates
+- **Rust/WASM logic** (`packages/doenetml-worker-rust/lib-js-wasm-binding`). Reference-resolution paths run in Rust today; the rest of the worker is slowly being transitioned over.
+
+Communication between main thread and worker uses structured messages. The worker is responsible for evaluating components, tracking dependencies (DAG), and managing variants.
+
+### Layer 3: UI (Viewer/Editor)
+The **main component** (`packages/doenetml/src/doenetml.tsx`) exports two top-level React components:
+- **`DoenetViewer`** — read-only rendering of DoenetML
+- **`DoenetEditor`** — editor UI with live preview
+
+Both use Redux for state management (`packages/doenetml/src/state/`) and share a Web Worker instance.
+
+### Connected Packages
+- **`@doenet/prefigure`** — backend for executing Python/computation-heavy activities
+- **`@doenet/standalone`, `@doenet/doenetml-iframe`** — bundled variants of the main library for different hosting scenarios
+- **`@doenet/codemirror`** — code editor integration
+- **`@doenet/ui-components`** — reusable UI components (used across doenetml, prefigure, etc.)
+- **`packages/vscode-extension`** — VS Code extension with LSP support (`packages/lsp`)
+
+## Monorepo Structure
+
+This is an npm workspace monorepo. Key points:
+- All packages build via **Vite** and **Wireit** (a task orchestration tool that manages build dependencies)
+- **Wireit** is configured in each `package.json`'s `wireit` field; it automatically rebuilds dependencies when inputs change
+- Each package can be built/tested independently with `-w <package-name>` or `-w @scope/package-name` flags
+
+## Build & Development Commands
 
-Agents working in this repository should read [TEST_RUN_INSTRUCTIONS_FOR_AGENTS.md](TEST_RUN_INSTRUCTIONS_FOR_AGENTS.md) before running tests.
+### Daily Development
 
+```bash
+# Start dev server (port 8012, builds doenetml and dependencies)
+npm run dev
+
+# Build a single package (rebuilds dependencies automatically via wireit)
+npm run build -w @doenet/doenetml
+
+# Build all packages (one-shot, no watch)
+npm run build:all
+
+# Format code with Prettier (required before commits)
+npm run prettier:format
+
+# Check formatting
+npm run prettier:check
+```
+
+### Run docs locally
+
+```bash
+npm run docs
+```
+
+Builds prerequisites and serves docs (Nextra-based) at `http://localhost:3000`.
+
+## Testing
+
+Read [TEST_RUN_INSTRUCTIONS_FOR_AGENTS.md](TEST_RUN_INSTRUCTIONS_FOR_AGENTS.md) before running tests. Highlights:
 - For `@doenet/test-cypress`, rebuild before Cypress runs after code changes.
 - Follow the required sequence: `build -> preview -> cypress run`.
 - Do not rely on an old build or an already-running preview server after source edits.
 - The runbook includes non-interactive test commands, Cypress preview-server workflow, fail-fast Cypress commands, and stale-asset troubleshooting.
 
-## Coding Conventions for Agents
+### Test Tooling
+
+- **Vitest** for unit tests, component logic, and utility functions (files: `*.test.ts`, `*.test.tsx`)
+- **Cypress** for e2e tests, user interactions, and full rendering (files: `cypress/e2e/*.cy.js`)
+- Tests are grouped; run by group number to parallelize CI
 
-- Avoid `private` class fields and methods; use an underscore prefix for internal members.
-- Avoid fire-and-forget calls via `void`; if a Promise is intentionally not awaited, attach an explicit `.catch(...)` handler.
-- Prefer function declarations over function-valued variables, unless reassignment or dynamic replacement is required.
-- Prefer `async`/`await` over old-style Promise chains (`.then(...)` / `.catch(...)`) when writing asynchronous code.
+### Common Test Commands
 
-## Commit Hygiene Requirements
+```bash
+# Run all tests (Vitest only, very slow)
+npm run test
 
-- Format changed files with Prettier before committing.
-- Ignore changes to `testCode.doenet` files. Do not commit them.
-- Ignore changes to `packages/doenetml/dev/main.tsx`. Do not commit them.
-- Ignore changes to any untracked `*.md` files in the repository root. Do not commit them.
+# Run Vitest only (all packages except `doenetml-worker-javascript`)
+npm run test:all-no-worker-js
+
+# Run targeted Vitest (e.g., prefigure package)
+npm run test -w @doenet/prefigure -- --run test/index-api.test.ts
+
+# Run Cypress e2e tests in groups (recommended)
+npm run test:e2e-group1
+npm run test:e2e-group2
+npm run test:e2e-group3
+npm run test:e2e-group4
+npm run test:e2e-group5
+npm run test:codemirror-cypress
+
+# Run a single Cypress spec (fast-fail mode)
+npm run test-cypress-fast-fail -w @doenet/test-cypress -- --config specPattern=cypress/e2e/tagSpecific/choiceinput.cy.js
+```
+
+## Coding Conventions
+
+- **No `private` class fields or methods.** Use an underscore prefix for internal members (`_field`).
+- **No fire-and-forget promises.** Always attach an explicit `.catch(...)` handler to intentionally unawaited Promises, or use `async`/`await`.
+- **Prefer function declarations** over function-valued variables (`function foo() {}` over `const foo = () => {}`), unless reassignment or dynamic replacement is required.
+- **Prefer `async`/`await`** over `.then(...)` / `.catch(...)` chains.
+
+## Commit Hygiene
+
+- Format changed files with Prettier before committing: `npm run prettier:format`
+- Files that should never be staged or committed (local development / planning notes):
+  - `packages/doenetml/dev/testCode.doenet`
+  - `packages/doenetml/dev/main.tsx`
+  - Untracked `*.md` files in the repository root
+
+If you edit these during development they will show as modified, but should not be staged.
 
 ## PR Creation
 
-- This checkout may use a personal fork as `origin` and the canonical repository as `upstream`.
-- Do not assume `origin/main` matches the PR base. Base pull requests on `upstream/main`.
-- Push the branch to your fork, then create the PR in `Doenet/DoenetML` with your fork branch as the head.
-- **Preferred method: Use GitHub CLI (`gh`).** The `mcp_gitkraken_pull_request_create` tool requires authentication that may not be available.
+This checkout may use a personal fork as `origin` and the canonical `Doenet/DoenetML` as `upstream`.
+
+- **Always base PRs on `upstream/main`**, not `origin/main`.
+- Push your branch to your fork (`origin`), then create the PR targeting `Doenet/DoenetML:main`.
+- **Preferred method: GitHub CLI (`gh`).** The `mcp_gitkraken_pull_request_create` tool requires authentication that may not be available.
 - Command format: `gh pr create --repo Doenet/DoenetML --base main --head <fork-owner>:<branch>`. Replace `<fork-owner>` with your GitHub username (e.g., `dqnykamp:my-branch`).
-- Before pushing, run `prettier` to format modified files and ensure formatting compliance.
-- Before creating the PR, confirm that only intended files are staged and committed, since this repository often has unrelated local work in the tree.
+- Before pushing, run `npm run prettier:format` on modified files.
+- Before creating the PR, confirm only intended files are staged — this repository often has unrelated local work in the tree.
 - After creating the PR, verify the branch has been pushed to `origin` and the PR links to the correct target branch (`Doenet/DoenetML:main`).
 
 ## Changesets
 
-- Changesets are configured in `.changeset/config.json`.
-- The repo currently uses one `fixed` group containing six packages:
-  - `@doenet/doenetml`
-  - `@doenet/standalone`
-  - `@doenet/doenetml-iframe`
-  - `@doenet/v06-to-v07`
-  - `@doenet/vscode-extension`
-  - `doenet-vscode-extension`
-- Additionally, `@doenet/prefigure` is published but with an independent version (not part of the fixed group).
-- User-facing changes in `@doenet/doenetml` are also apparent in `@doenet/standalone`, `@doenet/doenetml-iframe`, `@doenet/vscode-extension`, and `doenet-vscode-extension`.
-- User-facing changes in `@doenet/standalone` are also apparent in `@doenet/doenetml-iframe`.
-- When writing a changeset, include the published package or packages where the user-facing change is apparent, not just the package where the implementation lives.
-- The fixed-group configuration coordinates versioning across all six fixed-group packages.
+The repo uses Changesets for version management. Configuration is in `.changeset/config.json`.
+
+### Fixed Group (synchronized versioning)
+Six packages version together:
+- `@doenet/doenetml`, `@doenet/standalone`, `@doenet/doenetml-iframe`
+- `@doenet/v06-to-v07`, `@doenet/vscode-extension`, `doenet-vscode-extension`
+
+### Independent Versioning
+- `@doenet/prefigure` versions independently
+
+**Rule**: When creating a changeset for a user-facing change, include **all packages** where the change is visible to end users, not just where the implementation lives.
+
+Example: A change to `packages/doenetml/src/Viewer` affects `@doenet/doenetml`, `@doenet/standalone`, `@doenet/doenetml-iframe`, and downstream variants. Include all in the changeset.
+
+User-facing changes in `@doenet/standalone` are also apparent in `@doenet/doenetml-iframe`.
+
+## Key State & Data Flow
+
+### Redux Store Structure
+Located in `packages/doenetml/src/state/`:
+- **`main` slice** — document state, component data, update queue
+- **`keyboard` slice** — virtual keyboard focus tracking
+
+Components dispatch actions to update UI state; the worker listens for changes and updates document computation.
+
+### Worker Communication
+The worker receives serialized updates and returns rendered component states. Redux selectors provide derived state to UI components.
+
+## Common Tasks
+
+### Add a new component type
+1. Implement the **worker logic** in `packages/doenetml-worker-javascript` (component class, attributes, state variables, actions)
+2. Implement the **UI renderer** in `packages/doenetml/src/Viewer/renderers` or similar
+3. Register the component in `componentInfoObjects` so the worker knows about it; if the component appears in the DAST/normalized-DAST schema, update the relevant schema definitions too
+4. Add **tests** in both Vitest and Cypress
+5. Add a **changeset** if user-facing
+
+### Debug a rendering issue
+1. Start `npm run dev` and inspect the browser console
+2. Use Redux DevTools to inspect state changes
+3. Run `npm run test -w @doenet/test-cypress` to verify e2e tests still pass
+
+## Performance & Notes
+
+- The worker runs heavy computations off the main thread; avoid blocking UI updates
+- Large documents or deeply nested variants can cause lag; consider profiling with DevTools
+- Rust/WASM changes require a full rebuild of `packages/doenetml-worker-rust`; Wireit handles this but can be slow the first time

CLAUDE.md

@@ -0,0 +1,3 @@
+# CLAUDE.md
+
+See [AGENTS.md](AGENTS.md) for project conventions, architecture, build commands, and agent guidelines.

CORE_REFACTOR_DEFERRED.md

@@ -0,0 +1,83 @@
+# Core.js refactor — deferred findings
+
+These items came out of the PR review for `core-refactor-1` (Phase 1: extracting helpers from `packages/doenetml-worker-javascript/src/Core.js`). They were intentionally out of scope for that PR but are good candidates for the next refactor pass.
+
+The implementation in this branch covers the in-scope items from the PR review.
+
+## Deferred items
+
+### Type the `core: any` back-reference in extracted managers
+
+Every new manager (`DiagnosticsManager`, `VisibilityTracker`, `StatePersistence`, `AutoSubmitManager`, `NavigationHandler`, `ResolverAdapter`) declares `core: any;`. That defeats the point of converting to TypeScript — typos and accidental property reads through `core` go unchecked.
+
+Since `Core.js` is still JavaScript, defining a real `Core` interface is awkward. Two practical paths:
+
+1. **A minimal `CoreBackref` interface** in `packages/doenetml-worker-javascript/src/types/coreBackref.ts` listing only the fields each manager actually reads. This is documentary and catches typos. Each manager can narrow further with `Pick<CoreBackref, ...>`.
+2. **Convert `Core.js` to `Core.ts`** in a follow-up phase. Requires real type definitions for `_components`, `flags`, `componentInfoObjects`, etc. — a much larger lift but lets the managers drop their back-reference typing concerns entirely.
+
+(1) is the cheaper near-term win.
+
+### Reduce stateless managers to plain functions
+
+`NavigationHandler` and `ResolverAdapter` hold no state of their own — only a `core` back-reference. The pure-function shape used in `StateVariableNameResolver.ts` is more honest for these:
+
+```ts
+// NavigationHandler.ts
+export async function handleNavigatingToComponent({
+    core, componentIdx, hash,
+}: { core: CoreBackref; componentIdx: number; hash: string }) { ... }
+
+export function navigateToTarget({ core, args }: { core: CoreBackref; args: any }) { ... }
+```
+
+Core's wrappers shrink by one line each. If we keep the class form for symmetry with the other (genuinely stateful) managers, that's defensible — but the current PR has both shapes coexisting, so the inconsistency is real.
+
+### Move `getSourceLocationForComponent` out of `DiagnosticsManager`
+
+It currently lives at `DiagnosticsManager.ts:150-170`. It walks `_components` ancestors to find a position; nothing about it is diagnostic-specific. It's only there because diagnostics are its primary caller (`Core.js:7530`). Better home: `packages/doenetml-worker-javascript/src/utils/descendants.js` alongside `ancestorsIncludingComposites`.
+
+### `TimerLabels` constants for `reportTimerError`
+
+Each call site to `reportTimerError(...)` writes a hand-typed label (`"auto-submit answers"`, `"scheduled saveState"`, `"throttled saveChangesToDatabase"`, `"visibility periodic send"`, `"visibility resume send"`, `"visibility auto-suspend"`). A shared object would catch typos and centralize the namespace:
+
+```ts
+// packages/doenetml-worker-javascript/src/utils/timerErrors.ts
+export const TimerLabels = {
+    autoSubmit: "auto-submit answers",
+    scheduledSaveState: "scheduled saveState",
+    throttledSaveChanges: "throttled saveChangesToDatabase",
+    visibilityPeriodicSend: "visibility periodic send",
+    visibilityResumeSend: "visibility resume send",
+    visibilityAutoSuspend: "visibility auto-suspend",
+} as const;
+```
+
+Skip if the next phase introduces a broader logging/instrumentation layer that supersedes this helper.
+
+### Refactor `calcStartEndIdx` out of `determineParentAndIndexResolutionForResolver`
+
+`ResolverAdapter.determineParentAndIndexResolutionForResolver` contains a nested `async function calcStartEndIdx(replacements)` that mutates three variables from the outer scope (`start_idx`, `end_idx`, and reads `update_start`, `update_end`, `copyComponent`) as side effects. The function is recursive and returns a value, but callers discard the return and rely entirely on the side-effect mutations. This was carried over verbatim from Core.js.
+
+A cleaner shape: make `calcStartEndIdx` a standalone helper (or a private method on `ResolverAdapter`) that returns `{ start_idx, end_idx }` directly, eliminating the closure mutation. This untangles the logic and makes the function independently testable.
+
+### Pre-existing fire-and-forget calls still in `Core.js`
+
+Three intentionally unawaited Promise calls remain in `Core.js`. They pre-date this PR (this refactor did not introduce them, and adding `.catch` handlers was out of scope for a behavior-preserving extraction), but they violate the AGENTS.md convention that fire-and-forget Promises must attach an explicit `.catch(...)` handler. They should be picked up in the next phase that touches Core directly.
+
+- **`Core.js:410`** — `this.saveState();` inside the `generateDast` epilogue (`if (!this.receivedStateVariableChanges) { this.saveState(); }`). Returns a Promise; rejection is unhandled.
+- **`Core.js:11160`** — `this.saveState(true, true);` inside `processNewStateVariableValues` after recording component submissions. Same shape as above.
+- **`Core.js:449-452`** — `setTimeout(this.sendVisibilityChangedEvents.bind(this), this.visibilityInfo.saveDelay)` inside `onDocumentFirstVisible()`. The bound function returns `Promise<void> | undefined`; setTimeout discards the return, so any rejection is unhandled.
+
+Suggested fix in each case: wrap with `.catch(reportTimerError("<label>"))` using the helper added in this PR (`src/utils/timerErrors.ts`). For the setTimeout case, use a thin arrow wrapper: `setTimeout(() => { this.sendVisibilityChangedEvents()?.catch(reportTimerError("first-visible visibility send")); }, ...)`.
+
+### Regression test for the `.primitive.number` → `.primitive.value` fix
+
+Commit `aed7910c` fixed a latent bug at `ResolverAdapter.ts:84` (was reading `component.attributes.createComponentIdx.primitive.number`, should be `.value` after a `primitive.type === "number"` guard). The fix matches the pattern at `utils/resolver.ts:220-224` and `utils/componentIndices.ts:725`.
+
+This codepath fires when a copy component (created via `extend`) has a `source:sequence` attribute and a numeric `createComponentIdx`. A targeted Vitest case constructing that specific shape and asserting the resolver receives a `parentSourceSequence` with the correct `parent: <number>` field would lock the fix in. Without it, regressions could re-introduce the silent `undefined`-parent bug.
+
+## Notes for the next agent
+
+- The applied items in this PR are self-contained — see the diff against the previous commit. The structural deferrals above don't depend on each other except that #1 (typing) makes #2 (stateless→plain) cleaner since the back-reference type would already exist.
+- AGENTS.md is the source of truth for project conventions; CLAUDE.md is now a one-line pointer.
+- Cypress runs in CI; do not include local Cypress steps in any verification plan.