Skip to content

refactor: simplify ActionResult loading and Diff API (plan 015)#87

Open
DavertMik wants to merge 1 commit into
mainfrom
refactor/action-result-cleanup
Open

refactor: simplify ActionResult loading and Diff API (plan 015)#87
DavertMik wants to merge 1 commit into
mainfrom
refactor/action-result-cleanup

Conversation

@DavertMik

Copy link
Copy Markdown
Contributor

Implements plan 015 — a behavior-preserving cleanup of the core ActionResult/Diff. Independent PR off main (depends only on 014's already-merged Step 1). Fable-reviewed APPROVE; existing action-result-diff/action-result-memo suites + integration are the net.

Changes

  • One file loader. A shared private loadStateFile(file): string|null; the four state-file loaders become thin wrappers (screenshot keeps its own Buffer body). One readFileSync path for state files.
  • Diff.create — the footgun fix. static async create() constructs and awaits calculate(); ActionResult.diff() returns it. No consumer calls diff.calculate() separately anymore (navigator, deep-analysis ×3, toToolResult, and the diff tests updated). .calculate() now appears only inside Diff.create. Store only _isSameUrl (urlHasChanged() = !_isSameUrl); drop the ariaDiff getter (alias of ariaChanged), keep htmlDiff.
  • Shared aria counter. countAriaChanges() + LARGE_ARIA_CHANGE_THRESHOLD = 50 in utils/aria.ts; the inline block in action-result and the local copy in tools.ts now call it.
  • Small cleanups. this.url && this.url !== '' → truthiness; notes: anystring[]; getStateHash heading loop → direct pushes; removed the write-only _browserLogs field (orphaned when 014 removed browserLogsContent).
  • New test: ActionResult.diff() is pre-calculated — hasChanges() is truthful without a manual calculate().

Deviation (post-014 drift)

Step 2 (lazy fromState) skipped. html loads at construction for headings anyway, and post-014 there is no lazy browser-logs getter (browserLogs is a plain field pilot reads off a fromState result) — lazying it would change behavior. The eager browser-logs path is deliberately left intact.

Verification

bun run lint exit 0; bunx tsc --noEmit728 = baseline; bun test tests/unit765/0; bun test tests/integration63/0. Fable-reviewed: loadStateFile edge paths identical, all 5 Diff.create consumers read only post-calculation, countAriaChanges byte-identical, cleanups hash-identical.

🤖 Generated with Claude Code

- One shared private `loadStateFile()`; the four state-file loaders become thin
  wrappers on it (screenshot keeps its own Buffer body).
- Diff gets a `static async create()` that constructs + calculates; ActionResult.diff()
  returns it, so no consumer calls `diff.calculate()` separately (navigator,
  deep-analysis ×3, toToolResult, and the diff tests updated). Store only `_isSameUrl`
  (`urlHasChanged` = `!_isSameUrl`); drop the `ariaDiff` getter (alias of `ariaChanged`),
  keep `htmlDiff` (tests use it).
- Shared `countAriaChanges()` + `LARGE_ARIA_CHANGE_THRESHOLD` in utils/aria.ts; the
  inline block in action-result and the local copy in tools.ts now call it.
- Small cleanups: `this.url && this.url !== ''` → truthiness; `notes: any` → `string[]`;
  `getStateHash` heading loop → direct pushes.
- New test: `ActionResult.diff()` is pre-calculated (hasChanges truthful without a
  manual calculate()).

Deviations from the plan (post-014 drift): Step 2 (lazy fromState) skipped — html
loads at construction for headings anyway, and there is no lazy browser-logs getter
post-014 (pilot reads browserLogs off a fromState result), so lazying it would change
behavior. The cosmetic method-reordering and `_browserLogs` consolidation were left
to avoid touching that eager browser-logs path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Explorbot Self-Regression

Commit 70c2a2c · run

Scenario Result Attempts Duration
basic (native) PASS 1/3 8m
experience: control OK — failed as expected 1/1 45s
experience: seeded PASS 1/3 1m

Attempt details

  • basic (native) attempt 1 — PASS: login evidence: PASS (post-login plan=true, post-login research=true); research: PASS (files=7, wellFormed=true, keywords=7/3); scenarios: PASS (tests=6/5, features=4/3); tests passed: PASS (5 passed, 0 failed (reporter: 5 passed, 0 failed))
  • experience: control attempt 1 — PASS: control: OK — failed as expected (0 passed, 1 failed)
  • experience: seeded attempt 1 — PASS: seeded: PASS (1 passed, 0 failed)

Session analysis — basic (native):

Session Analysis

Explored the Issues list page and its core functionality: creating issues, filtering by status/labels, text search, and viewing issue details. All five core flows completed successfully, with the app responding correctly. Automation required multiple recovery attempts for some interactions.

Coverage

  • Pages: /issues, /issues?new=1, /issues?q=..., /issues?label=..., /issues/5
  • Features: Issue creation, status filtering, label filtering, text search, issue detail view

What works

  • Issue creationET-1 Create New Issue
  • Status filteringET-2 Filter Issues by Status
  • Text searchET-3 Search Issues by Text
  • Label filteringET-4 Filter Issues by Label
  • Issue detail viewET-5 View Issue Detail

Execution Issues

  • ET-3 Search Issues by Text — initial search field fill failed, recovered with correct selector
  • ET-4 Filter Issues by Label — multiple dropdown open attempts failed, recovered via alternative selector
  • ET-5 View Issue Detail — table row click failed, recovered via direct URL navigation

@DenysKuchma DenysKuchma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge only after plan 014

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants