Repository navigation
Conversation
6575c19 to
7c0bb11
Compare
Windows runs package scripts through cmd.exe, which keeps single quotes literally, so --filter './packages/*' matched no workspace project and the package build, typecheck and test scripts exited 0 without running. Use double quotes, which both sh and cmd strip. Also: - judge packages/*/src TypeScript in the changed-file length gate, matching the lint target roots - stop pnpm build from emitting package dist output nothing consumes; CI still runs build:packages - export only the replay-core values the application imports; types used by public signatures stay exported - put the jsdom docblock back at the top of ShellReplayOutput.test.ts and merge the component's duplicate package import - drop the stale findIndexAtTime ownership entry from the engine docs
|
Review of The extraction itself holds up: moved code, bundler resolution, Vitest projects and the boundary checks all worked in local runs. Findings, all fixed in
The description said the cycle checker reports three existing cycles. It reports four, identical on develop and this branch (the fourth is Verification
#1762 no longer depends on this PR: stack #1783 was dissolved and #1762 now targets develop directly. |
|
Closing this one — not because the work is wrong, but because the cost/benefit doesn't hold up. The extraction itself is clean and my earlier review confirmed that; this is a scope judgment, not a quality one. What the diff delivers: 7 source files moved into two private, never-published packages, and 7 genuinely new test cases ( Why the premise doesn't survive contact with the files. The stated problem is that these algorithms' dependency boundaries can't be validated independently. But the extracted files are already dependency-free:
Why the cost is real rather than theoretical. There's no The part worth keeping. The 7 new test cases need none of the extraction. Happy to port them onto develop against today's paths as a small Sorry to land this after the follow-up round you did; the boundary question is worth raising separately if you think the invariant needs stronger enforcement than a cruiser rule. |
Problem
Replay algorithms and terminal shell integration live inside the application, so their dependency boundaries and tests cannot be validated independently. Existing build, lint and test discovery assumes all frontend sources belong to the app.
Solution
Extract private source-exported replay-core and terminal-shell-integration workspace packages, migrate consumers, and teach both bundlers and quality checks to discover workspace sources. Keep the mutable shell replay cache as one application-owned instance. Configure Vitest 4 projects with isolated package tests and package build/typecheck commands. Add regression coverage for replay boundaries and terminal shell events alongside the extracted implementations.
Potential risks
Workspace resolution, bundler watching and CI discovery change; pnpm is required to install the two local links. The lockfile only adds workspace importers/links and retains upstream dependency versions. These packages are private and have no public release or persistence migration; rollback reverts the extraction and reinstalls the prior lockfile. Full application/native UI suites and packaged desktop startup were not run. The cycle checker still reports four existing app-only cycles (identical on develop) involving the icon registry, session input barrels and HoverCard; no package cycle was found. Runtime CPU/RSS was not measured and no speedup is claimed. Consumer UI changes are imports and cache ownership wiring, so screenshots would not demonstrate this change.
Verification
pnpm install --frozen-lockfile: passed with isolated dependencies matching the current lockfile.pnpm build:packages,pnpm typecheck:packages,pnpm test:packages: passed; 64 package tests.pnpm exec vitest run --config config/vitest.config.ts --project app --maxWorkers=2 src/engines/SessionCore/replay/components/ShellReplayOutput/ShellReplayOutput.test.ts: four passed.pnpm exec vitest list --config config/vitest.config.ts --project '@orgii/*' --json: discovered all 64 package tests.node --test scripts/quality/workspace/*.test.cjs scripts/quality/dependency-boundaries/*.test.mjs scripts/ci/select-lint-targets.test.cjs: 25 passed, including actual Webpack/Rspack build and watch behavior.pnpm typecheck:fast: passed, rerun after integration with latest develop.node scripts/quality/dependency-boundaries/check.mjs: zero new boundary violations; three baseline edges retained.node scripts/quality/check-test-placement.mjs: passed.node scripts/quality/check-circular-dependencies.mjs --json: reported four existing app-only cycles, the same set as develop; unrelated cycle cleanup is excluded.Review follow-up verification
Follow-up commit
88fed4851faddresses review findings. It uses Windows-safe package filter quoting (cmd.exekept'./packages/*'literally, so package build/typecheck/test scripts matched no project and exited 0). It extends the changed-file length gate topackages/*/src, removes the unused packagedistemit frompnpm build(CI still runsbuild:packages), and trims unconsumed replay-core value exports. It also restores theShellReplayOutputtest docblock order, merges a duplicate import, and removes a stale engine-doc entry.node --test scripts/ci/*.test.cjs: 53 passed, including new package-source cases for the file-length gate.pnpm test:packages: 64 passed (35 replay-core, 29 terminal-shell-integration);pnpm test:workspace: 5 passed.pnpm exec vitest run --config config/vitest.config.ts --project app src/engines/SessionCore/replay/components/ShellReplayOutput/ShellReplayOutput.test.ts: 4 passed.pnpm typecheck:fast, ESLint and oxlint on the changed TypeScript files,node scripts/quality/check-test-placement.mjs, and the file-length gate on the diff: passed. Commit hooks (lint-staged, tsgo, commitlint) passed.cmd.exeitself (double quotes are stripped by bothshandcmd), the full app suite, and a production app build.Audit
Architecture L1 isolated builds/tests; L2 package inventory and discovery; L3 exported types; L4 application cache ownership; L5 replay bounds and event regressions; L6 source exports and dependency direction; L7 compatibility with current Vitest APIs; L8 no wire change; L9 app and bundler consumers; L10 one implementation and one mutable cache owner. Pure import changes do not introduce action controls. Resource verdict: no new timers or subscriptions in the extraction; native lifecycle measurements remain unverified.