Skip to content

P1B: Refactor (packages/core/src/integration.ts): Function with many returns (count = 7): draft - #82

Open
smalinka wants to merge 2 commits into
CMU-17313Q:mainfrom
smalinka:refactor/integration-draft
Open

smalinka wants to merge 2 commits into
CMU-17313Q:mainfrom
smalinka:refactor/integration-draft

Conversation

@smalinka

@smalinka smalinka commented Sep 5, 2026

Copy link
Copy Markdown

P1B: Starter Task: Refactoring PR

1. Issue

Link to the associated GitHub issue:

Closes #70 - #70

Full path to the refactored file:

packages/core/src/integration.ts

What do you think this file does?

It defines opencode's integration registry: the service that tracks third-party integrations (their IDs, names, and available auth methods), stores credentials against them, and drives OAuth attempts through their lifecycle. The Draft object it exposes is the mutation API that other code uses to register, update, and remove integrations and their methods.

What is the scope of your refactoring within that file?

The draft factory passed to State.create (line 230), specifically the two findIndex comparator callbacks inside draft.method.update and draft.method.remove. One module-level helper, isSameMethod, was added just above the Draft type declaration.

Which Qlty-reported issue did you address?

Function with many returns (count = 7): draft at line 230. The same function also reported Function with high complexity (count = 39), and the file reported High total complexity (count = 90).

packages/core/src/integration.ts
 230  Function with many returns (count = 7): draft
   1  High total complexity (count = 90)
 230  Function with high complexity (count = 39): draft

2. Refactoring

How did the specific issue you chose impact the codebase's maintainability?

The same three-branch method-equality rule was written out twice — once in method.update and once in method.remove — with the operands in opposite order, so any future change to how two methods are considered "the same" would have to be found and applied in two places. Those two inline callbacks contributed six of the seven returns that pushed draft over the many-returns threshold, adding noise to an already large factory function.

What changes did you make to resolve the issue?

I extracted the shared comparison into a single module-level function, isSameMethod(a: Method, b: Method): boolean, and replaced both inline callback bodies with calls to it. The comparison logic itself is unchanged — same three branches, same semantics — only its location changed.

How do your changes improve maintainability? Did you consider alternatives?

Method equality now has one definition and one name, so the rule is discoverable and changeable in one place, and draft drops from 7 returns to 1 with its complexity falling from 39 to 22 (file total 90 → 76). I considered inlining the comparison as a single boolean expression instead, but that would have kept the logic duplicated and made the OAuth-only ID check harder to read; a named helper also documents the intent that non-OAuth methods match on type alone.

3. Validation

How did you validate that the change is correct?

The existing suite in packages/core/test/integration.test.ts passes 9/9 after the change, and it exercises the refactored comparator directly. "registers and overrides methods independently" and "reveals the previous registration when an override closes" both depend on method.update correctly finding — or failing to find — a matching existing method, which is exactly the branch isSameMethod now decides; if the extraction had changed the comparison semantics, those tests would fail. Coverage for packages/core/src/integration.ts is 87.90% of lines, and the uncovered ranges (276-280, 317-318, 385-401, 421, 458-463, 466-471) do not include the new helper, confirming the tests execute the changed code.

bun run typecheck (tsgo --noEmit) is clean. The full packages/core suite reports 1079 pass / 2 fail both before and after this change; the two failures (util.effect-flock > fails on unwritable lock roots and util.flock > fails clearly on unwritable lock roots) are pre-existing and environmental — they chmod a directory to 0o500 and expect a permission error, which never occurs because the devcontainer runs as root. bun lint (oxlint) reports 698 warnings and 2 errors across the monorepo, identical before and after, none of them in this file.

Screenshot 2026-09-05 at 19 19 52 Screenshot 2026-09-05 at 19 32 24 Screenshot 2026-09-06 at 22 02 27 Screenshot 2026-09-06 at 22 36 14

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.

P1B: Refactor (packages/core/src/integration.ts:230): Function with many returns (count = 7)

1 participant