6 Commits

Include renames

Author	SHA1	Message	Date
Neil	231e805b1e	fix(lint): enable anti-slop/no-shape-in-symbol-names (#20785) ... Flip `anti-slop/no-shape-in-symbol-names` from "off" to "error" and clear every violation under src, config, tests and mobile. What the rule bans ------------------ The case-insensitive substring "shape" in any JS/TS identifier: variables, functions, parameters, types, type parameters, class members, private names, object-literal keys and JSX identifiers. The one exemption is a statically accessed member read owned by another value (`zodObject.shape` is fine), so third-party APIs stay readable without a suppression. "Shape" names a value's structure rather than its domain role. `UserShape`, `validateArgShape` and `errorShape` all tell you the symbol is "an object with some fields" -- which is already what a type says -- while saying nothing about what the value is for or who owns it. The rule forces the name to carry the domain instead. Violations fixed ---------------- 689 violations across 109 files at baseline (verified by re-running the audit against the pre-change tree with the rule set to "error"). Fix pattern ----------- Rename for the domain role, not the structure: -type FieldShape = 'list' | 'map' | 'whole' -const FIELD_SHAPES = { ... } satisfies Record<keyof Observation, FieldShape> +type FieldEncoding = 'list' | 'map' | 'whole' +const FIELD_ENCODINGS = { ... } satisfies Record<keyof Observation, FieldEncoding> -function assertGitPushTargetShape(target: unknown): void +function assertValidGitPushTarget(target: unknown): void -function describeReadDirPathShape(p: string): ReadDirPathKind +function classifyReadDirPath(p: string): ReadDirPathKind Predicates became statements about the value (`isDeltaShapedProviderFrameKind` -> `isDeltaProviderFrameKind`, `isDeleteShapedDiscardEntry` -> `discardDeletesEntryFile`, `isSkillsCliAgentKeyShaped` -> `isUsableSkillsCliAgentKey`). Type aliases dropped the suffix where the remaining name was already unambiguous (`GhGraphqlErrorShape` -> `GhGraphqlError`). No wire-visible name was renamed: no IPC or RPC channel, stream opcode, request/response param, persisted field, or i18n key. The `--shape=symlink|copy` CLI flag read by .github/workflows/skill-update-roundtrip.yml is unchanged -- only the local variable holding it was renamed. Exemptions ---------- They are file-scoped entries in config/oxlint-anti-slop.json, not inline `oxlint-disable` comments. An inline directive naming an anti-slop rule reads back as an UNUSED directive under the root lint scan, which does not load this plugin -- the changed-code quality gate counts that warning, so the comment form cannot be used for a rule that lives only in this config. * src/renderer/src/components/browser-pane/annotate/**: in the screenshot annotator a "shape" is the drawn geometry -- pen, arrow, rect, ellipse, highlight. That is a genuine domain noun, and it pervades every symbol in the module. * repo-icon.tsx, repo-header-project-actions.tsx, mobile MobileRepoIcon.tsx: lucide exports the icon component as `Shapes`. The name is theirs, and the matching REPO_LUCIDE_ICONS key is the persisted icon name shared with the desktop picker -- renaming it would orphan saved repo icons. * src/shared/onboarding-state-types.ts, src/shared/constants.ts: `shapedSidebar` is a persisted onboarding-checklist field and a telemetry enum member; renaming it would orphan saved state. * src/shared/rpc-contract/rpc-send-params.ts: matching zod's own literal `shape` property is what selects the ZodObject branch of the conditional type. No exemption was added merely to avoid a rename. Eight symbols initially suppressed as "a cross-module refactor outside this change" were proven to have zero non-TypeScript references repo-wide and renamed instead. Zod's `ZodRawShape` needed no exemption at all: `Readonly<Record<string, z.ZodType>>` is its definition, so repo-update-params.ts and ui-update-value-tolerance-params.ts spell it out instead. Likewise telemetry-event-classification.ts now reads `.shape` through an `in` narrowing, which also retires two pre-existing type assertions; three more assertions the rename had dragged onto changed lines (two `JSON.parse` sites, one node:sqlite row read) became annotations and an explicit row mapping. Verified -------- * Audit reports zero violations; confirmed the rule genuinely fires by planting a probe violation. * node config/scripts/run-typecheck-projects-in-parallel.mjs exits 0. * Vitest over src/shared, src/main/github/project-view, the annotate module, the repo-icon components and the Chromium SameSite electron spec: all green. * All 66 removed "shape" identifiers grepped repo-wide across every file type; none survive. * node config/scripts/generate-rpc-params-catalog.mjs --check exits 0. * node --check on every changed .mjs; oxfmt clean on all changed files. * `pnpm run check:code-quality:changed` reports 0 findings. Not machine-verified: the 3 mobile/ files (its Vitest run cannot resolve `expo/tsconfig.base.json` in this worktree), and the WSL- and Playwright-gated specs. All are rename- or comment-only hunks, read in full.	2026-09-15 02:00:27 -07:00
Neil	5a626dcdf4	refactor(git): share push-target resolution between local and the SSH relay (#18406) ... `src/relay/git-handler-push-target.ts` and `src/main/git/remote.ts` carried identical ~160-line copies of the resolver that decides which remote a plain `git push` hits. Identical today is exactly when to share it: the cost of a future divergence is pushing to the wrong remote, which retrying does not undo. Move the resolver to src/shared/git-push-target-resolution.ts, parameterized on a `(args) => Promise<{ stdout }>` runner — the only thing the two hosts actually differ in — and delete both copies. The relay entry point keeps only the work that is genuinely relay-side: re-validating an explicit target that arrived over the wire and running `check-ref-format` on it. No behavior change on either path, and nothing new or different is published, so this engages no rule in remote-wire-compatibility. No git command changes. src/relay/git-push-target-local-parity.test.ts scripts one repository's config and requires `git.push` over the real relay dispatcher and the desktop's `gitPush` to emit the same push argv, plus the argv each case should produce.	2026-09-03 14:42:58 -07:00
Neil	104f9655e4	perf(git): answer remote-URL questions from one subprocess, not one per remote (#18158) ... Four copies of the same loop ran `git remote` and then a serial `git remote get-url <name>` per remote to answer "which remote has this URL". On a repo with 58 remotes that is 59 subprocesses -- measured at 1083 ms -- for one question, and worktree create asks it several times. `git remote -v` answers for every remote from one child, reporting the same insteadOf-expanded first fetch URL `get-url` prints. The batched `cat-file --batch-check` branch-conflict probe decides from stdout, but its WSL route was unfenced, so a login-shell fallback printed the distro banner onto the stream it parses. That broke the one-line-per-ref contract, made every batch undecided, and fell straight back to one `show-ref` per remote -- the cost the batch exists to remove. Measured at 58 remotes / 4346 branches, spawns and wall time: push-target remote scan 59 -> 1 (1083 ms -> 8 ms) branch-conflict probe 60 -> 3 (984 ms -> 43 ms) configured push target 123 -> 6 (2707 ms -> 157 ms)	2026-09-02 12:53:48 -07:00
Neil	77f23b013f	refactor(shared): drop the shared/types barrel and import from the real modules (#14447) ... #14397 split `shared/types.ts` into 46 per-domain modules but kept the path as a re-export barrel so the import sites did not have to change. This removes the barrel: every consumer now imports from the module that actually declares the type, and `src/shared/types.ts` is deleted. Barrels hide where a type lives, make every consumer look like it depends on the whole domain, and let an unrelated edit invalidate a module that ~2,000 files transitively import. 2,323 import declarations across 2,321 files. Rewritten mechanically: each specifier was resolved to an absolute path via the TypeScript AST and recomputed, rather than string-substituted, so alias forms (`@/../../shared/ types`) and per-specifier `type` modifiers survive. Four cases the mechanical pass had to handle, each found by a gate rather than by reading the diff: - Modules inside `src/shared` import the barrel as `./types`, not `shared/types`. A pre-filter on the latter string skipped 176 of them and left imports dangling at a deleted file, which surfaced as confusing `Property 'x' is optional in type 'Repo' but required in Pick<Repo, ...>` errors rather than "module not found". - The barrel RENAMED one type on the way through (`WorkspaceSource as WorkspaceCreateTelemetrySource`), so the original name in the owning module has to be re-aliased at each consumer. - Three test files put `;(globalThis as ...)` on the line after the import. TypeScript parses that `;` as the import statement's terminator, so replacing through `statement.getEnd()` deletes it and breaks ASI. The rewrite now stops at the module specifier. - A file that already imported directly from a module got a SECOND import from it, because the barrel re-exported those same names — which trips `import/no-duplicates` under `--deny-warnings`. A post-pass merges declarations sharing a specifier and type-only-ness; the `import type` plus `import` pair from one module is left alone, since that form is allowed. Splitting one barrel import into several genuinely adds lines, which pushed `terminal-layout-pty-ownership.ts` to 301 counted lines: its 107-character import must wrap, and neither local type collapses onto one line (101 and 116 characters). Rather than contort a type declaration to fit a line budget, `collectLeafIds` and `pruneLeaves` move to `terminal-pane-layout-tree.ts` — they are pure structural operations on the layout tree and independent of PTY ownership. `visible-worktrees.ts` similarly loses its own mini-barrel re-export of `isDefaultBranchWorkspace`, with the four real consumers repointed at the declaring module. No `max-lines` bypass added. Verified: cold `tsc --noEmit` green on node, cli, and web (buildinfo deleted first — these projects are `composite: true` and reuse stale caches); the full `pnpm lint` green, not just bare oxlint — the narrower local check is what let the duplicate imports reach CI; max-lines ratchet OK at 344.	2026-08-13 22:48:24 -07:00
Jinjing	692e0b0ab9	Push existing fork review branches without republishing (#5531) ... * Enable pushing to configured push targets for existing fork reviews * Resolve push targets using branch.pushRemote, remote.pushDefault, and URL-valued remotes normalized to matching named remotes. * Support this push target resolution on both local and relay/SSH git handlers to prevent drift in SSH worktrees. * Offer "Push" instead of "Publish Branch" in the Source Control UI when a linked review exists and a valid push target is available. * Prevent pushing a feature branch's base branch (e.g. main) directly to a fork via remote.pushDefault. * Keep fork push target when contributor branch matches base branch name Ensure that a fork push target is not incorrectly discarded when its branch name matches the base branch name on another remote (for example, targeting fork/main while the base is origin/main). Previously, the matching logic only compared the branch leaf name, which treated different remotes as identical and disabled the push target. We now qualify the ref comparison with the remote name to differentiate them.	2026-06-16 18:10:01 -07:00
Jinjing	5a6b2994ea	fix: address review findings (#1770)	2026-05-13 16:14:50 -07:00