Skip to content

refactor(platform): colocate the HarmonyOS, Linux and web interactors behind lazy factory facades - #3713

Merged
thymikee merged 4 commits into
mainfrom
refactor/platform-interactor-facades
Oct 16, 2026
Merged

thymikee merged 4 commits into
mainfrom
refactor/platform-interactor-facades

Conversation

@thymikee

@thymikee thymikee commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Move the HarmonyOS, Linux and web interactors (and web's unsupported-interactor) with their tests into platform-harmonyos, platform-linux and platform-web; no root implementation remains. Android, Apple and Vega are untouched.
  • Each package's ./interactor export is a lazy factory facade (src/interactor-facade.ts): type-only imports plus a function-scoped await import('./interactor.ts'). The implementation stays private, so the facade's eager closure is one module and platform mechanics load only when the factory is called.
  • register-builtins.ts keeps its dynamic imports, now of the package subpaths. The layering policy admits those subpaths only from that file and only dynamically. Web uses the canonical selectors/interaction-positionals stripAtPrefix.
  • Review fix: the R13 summary printed by check:layering now names the dynamic-only interactor subpath exception (78b06026b1).
  • No behavior change claimed; no native or device validation.

Validation

Head 78b06026b1, merge-base b7c93e3a37 (origin/main is now 87fe3ea83e; no rebase needed):

  • pnpm check:affected --run on this exact head (pnpm 11.17.0, AGENT_DEVICE_VITEST_MAX_WORKERS=1, under the shared gate lock): all runnable checks passed, exit 0. Vitest-related: 1852 files, 15577 tests passed. GitHub-authoritative lanes (coverage, native, smoke) are CI-only.
  • Earlier round (0776567d70) ran the same full gate green; the only change since is the one-line summary string.
  • Cold design review of this head: clean; no notes taken (pre-existing helper duplication and test-scope ideas are out of scope).

View guided diff Turn on auto-fix

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 18 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/layering/platform-package-policy.ts
@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.29 MB 5.30 MB +529 B
Package (unpacked) 5.29 MB 5.29 MB +529 B
Package (download) 1.59 MB 1.59 MB -139 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.6 ms 29.3 ms -0.3 ms
CLI --help 83.7 ms 81.2 ms -2.5 ms

@thymikee

Copy link
Copy Markdown
Member Author

The move itself looks clean at 78b0602, and all 20 checks pass at that commit. Evidence is still pending: I did not rerun the affected checks or the layering scan, and I did not measure the module load graph at runtime, so please show a run of the layering scan on this head. No conflicts are known. Nothing else blocks merge once that evidence is in.

Could each package export its createXInteractor from the root index instead of adding the ./interactor subpath? Then register-builtins can call await import('@agent-device/platform-<family>'), which the existing src/core/interactors/ carve-out in isAllowedPlatformRootImport already allows (https://github.com/callstack/agent-device/blob/78b0602/scripts/layering/platform-package-policy.ts#L400-L411). Exporting a plugin from the root, as platform-vega does, would work too. That would remove the three facade files, the three ./interactor exports, isInteractorSubpathImport, INTERACTOR_COMPOSITION_FILE, the three MECHANICS_FACET_SUBPATHS entries, the summary clause and the matching policy tests. Loading would not change, because each moved interactor.ts statically imports ./index.ts already. Nothing has to change first, only a decision that the interactor factory belongs to each package's root facade.

Not blocking: the new subpath mechanism sits beside the existing root-import carve-out (https://github.com/callstack/agent-device/blob/78b0602/scripts/layering/platform-package-policy.ts#L98), and isInteractorSubpathImport checks both the platform-${family}/interactor template and the enumerated list (https://github.com/callstack/agent-device/blob/78b0602/scripts/layering/platform-package-policy.ts#L110). One source of truth is enough, and you can take or leave both notes.

The only other thread, from cubic-dev-ai, no longer applies, because the summary now names the interactor subpaths, so please resolve it: #3713 (comment)

@thymikee

Copy link
Copy Markdown
Member Author

Thanks. I tried the root-factory design, and fallow's circular-dependency gate blocks it. I left the PR head unchanged at 78b0602.

Layering on 78b0602, run under the shared gate lock with pnpm 11.17.0: pnpm check:layering passed (396 tests, 0 failures, guard OK). pnpm run check:fallow --base origin/main also passed: "No issues in 18 changed files".

What I tried: each package root exports createXInteractor (deferred(...) and plain async function versions), the builtin registry imports the package root, and the three facade files, subpath exports, isInteractorSubpathImport and the matching R13 summary clause and test are gone. Layering, typecheck, production-exports and a root-route test passed (the test failed when I broke the export). A module-load probe showed the root loads only index.ts until the factory is called, then interactor.ts, and no operation modules.

Why it fails: fallow audit --base origin/main reports 3 circular dependencies:
platform-{harmonyos,linux,web}/src/index.ts → interactor.ts → index.ts

The interactors statically import their deferred operations from the root, and fallow counts the factory's dynamic import('./interactor.ts') as an edge. Typing the factory explicitly instead of typeof import(...) made no difference. The separate facade file avoids the cycle because nothing imports it back.

The only ways to keep the root factory are a fallow-ignore-next-line circular-dependency suppression (×3), or making the interactors import operation modules directly, which grows the eager closure. I haven't done either. Which do you prefer: keep the facades, or accept the suppressions?

@thymikee

Copy link
Copy Markdown
Member Author

Keeping the existing lazy facades for this extraction. The root-export experiment introduces three index → interactor → index cycles, including with explicitly typed factories. We will keep the cycle and eager-loading guards intact; no suppressions are needed.

I independently checked the fresh evidence on unchanged head 78b06026b1: pnpm check:layering passed (396 tests, guard OK), and pnpm run check:fallow --base origin/main passed, both under the shared gate lock with pnpm 11.17.0. The earlier exact-head affected gate and CI evidence remain valid. This addresses the requested layering proof; the optional root-export simplification is declined based on the experiment above.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks, that settles it at 78b0602. The root-export experiment shows why the smaller design does not work: it adds three index → interactor → index cycles that fallow blocks, and the only ways around them are suppressions or a larger eager closure. Keeping the lazy facades is the right call. The layering scan and the fallow check both pass on this head, so the evidence I asked for is in. No conflicts, and all checks pass. This is ready for human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 16, 2026
@thymikee
thymikee merged commit 77137b0 into main Oct 16, 2026
20 checks passed
@thymikee
thymikee deleted the refactor/platform-interactor-facades branch October 16, 2026 17:39
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-16 17:54 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant