Skip to content

chore(app): clear lint warnings in GUI files touched by the Effect upgrade - #53098

Merged
kitlangton merged 2 commits into
v2from
gui-lint-cleanup
Oct 4, 2026
Merged

kitlangton merged 2 commits into
v2from
gui-lint-cleanup

Conversation

@kitlangton

@kitlangton kitlangton commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Why

#52868 added a Lint changed files step to the check workflow. It runs bun run lint:changed HEAD^1, which fails when any GUI package file a PR adds or edits has an oxlint problem, including the warn-level anti-slop rules. #50231 (Effect rc.118) only changes Effect import paths in 31 such files, but those files already had 716 warnings between them, so its check job fails on code it never touched.

What Changes

This PR makes exactly those 31 files lint-clean on v2, so #50231 passes once v2 is merged into it again. It stays on Effect rc.112 and changes no Effect import paths.

Warnings Count Fix
require-readable-spacing 541 oxlint --fix (blank lines only)
no-runtime-typeof 56 Predicate.isString / isNumber / isObject / isFunction with the same semantics
no-unknown-parameters / -returns, no-unsafe-dictionary-type 47 Named types: error handlers take cause: unknown (the repo convention); mock fixtures use MockFixture, MockFields, MockSession, and MockProject; request bodies are decoded with Schema.Json / Schema.Record
require-safety-comment-for-type-assertion, no-chained-type-assertions 33 Casts removed where the type now holds (most of the config.project as … casts, ipc.ts's fake Electron.Event); the rest get a SAFETY: comment
no-manual-tag-comparison / -tagged-construction 26 Predicate.isTagged, Option.isSome, SchemaAST.isObjects, Schema.encodeSync(TestEvent)
no-conditional-empty-object-spread, no-known-value-widening 13 Objects are built in separate statements, and key order is kept

Seven files were already clean and are left untouched. About half the hand edits are in packages/app/e2e/utils/mock-server.ts. Its fixture config is now typed as JSON-like data instead of unknown, and every e2e spec still typechecks against it unchanged.

Targeted suppressions

Following the existing SAFETY: + scoped oxlint-disable pattern (for example gui-extensions/src/sdk/core.ts and app/e2e/regression/review.spec.ts), these cases keep a justified suppression because the honest fix would change behavior:

  • desktop/src/renderer/ipc-client.ts speaks Effect's RPC wire format by hand and deliberately keeps the Effect runtime out of the renderer's startup graph. Its plain _tag messages and comparisons sit in one scoped disable block. The never-read Promise<unknown> dispose callbacks and the trusted reply value also keep suppressions.
  • app/src/runtime/persistence/schema.ts: recover and merge walk untyped persisted trees against a schema AST, and withInitial decodes the result afterwards.
  • Wire-format literals in tests and mocks: the mock server's 401 body, the expected RPC exits in ipc-transport.test.ts, and the two as unknown as Electron fakes in that test.
  • app/e2e/utils/mock-api.ts: jsonValue is the encoder that turns arbitrary handler output into JSON.

Scope

Behavior is unchanged. Production edits only swap equivalent predicates, add comments, or restructure code without changing values (ipc.ts window wiring, ssh/controller.ts config patch, model selection choices). The e2e mock answers the same responses. Merging this into #50231 conflicts only on adjacent import lines and blank lines. #50231's own server.address._tag === "UnixPathAddress" lines need Predicate.isTagged(...) when that merge is resolved, and its new askpass code needs oxlint --fix. A trial merge resolved that way lints and typechecks clean.

Verification

bun run lint:changed HEAD^1                 # 0 problems in 24 changed files
bunx oxlint --format json <the 31 files>    # 0 problems (716 before)
bun run check                               # passes
cd packages/app && bun run test:unit        # 727 pass
cd packages/app && bun run test:browser     # 165 pass
cd packages/app && bun run typecheck:e2e    # only the 2 errors already on v2 (performance probes)
cd packages/app && bun test ./e2e/performance/unit/mock-server.test.ts   # 3 pass
cd packages/gui-extensions && bun run test  # 98 pass, 1 skip
cd packages/desktop && bun run test         # 109 pass, 1 skip
cd packages/app && bunx playwright test      # local dev-server run, see below

CI's production-build e2e passes on Linux and Windows. The only red step is the app component tests, where browser-pane-restore.spec.ts fails as it already does on v2 (fix in #53091). Locally, the full dev-server e2e run left 19 failures (timeline scroll, command palette, composer, and similar). The same 19 also fail with this PR's 24 files reverted to v2, so they come from the local environment, not this change.

# ------------------------ >8 ------------------------
# Do not modify or remove the line above.
# Everything below it will be ignored.
#
# Conflicts:
#	packages/app/e2e/utils/mock-api.ts
#	packages/app/e2e/utils/mock-server.ts
@kitlangton
kitlangton merged commit eaf80d9 into v2 Oct 4, 2026
9 checks passed
@kitlangton
kitlangton deleted the gui-lint-cleanup branch October 4, 2026 19:04
Ichinose-Kazuki pushed a commit to Ichinose-Kazuki/opencode that referenced this pull request Oct 7, 2026
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.

1 participant