P1b/refactor - #185
Open
gavinlin-cloud wants to merge 2 commits into
Open
Conversation
…table, also Extract three concerns out of assertPortable into named module-scope functions: astChecksPortable , and register @opencode-ai/httpapi-codegen#test in turbo.json so this package's tests run in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P1B: Refactor (packages/httpapi-codegen/src/index.ts:810): Function with high complexity (count = 31): assertPortable
Use this pull request template to briefly answer the questions below in one to two sentences each.
Feel free to delete this text at the top after filling out the template.
1. Issue
Link to the associated GitHub issue:
#51
Full path to the refactored file:
packages/httpapi-codegen/src/index.tsWhat do you think this file does?
(Your answer does not have to be 100% correct; give a reasonable, evidence‑based guess.)
It builds standalone TypeScript API clients from an Effect
HttpApidefinition. The generated client is its own file and can't use server runtime objects, so the compiler checks first that every schema is "portable", meaning it can be written using onlyeffecttypes.What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
assertPortable()at line 810, plus thevisitCurrenthelper nested inside it.Which Qlty‑reported issue did you address?
(Name the rule/metric and include the BEFORE value; e.g., “Cognitive Complexity 18 in render()”.)
Function with high complexity (count = 31) in
assertPortable. Qlty also flagged many returns (count = 14) on it, and the nestedvisitCurrentwas separately flagged at complexity 20 with 10 returns.2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The function was 47 lines doing three different jobs: tracking visited nodes with a cache and cycle guard, recursing into child nodes based on node kind, and applying a stricter rule for tagged error classes. Changing one job meant reading all three. The helpers were also nested functions, which pushed every branch deeper and inflated the score.
What changes did you make to resolve the issue?
I pulled out three helper functions to module scope:
astChecksPortable,portableChildren, andtaggedErrorUnportable.How do your changes improve maintainability? Did you consider alternatives?
Each job is now its own function, and the list of node kinds lives in one place instead of being spread through an if-chain.
assertPortablewent from complexity 31 to no complexity smell, returns dropped from 14 to 9, andvisitCurrent's smells are gone. I looked at also movingvisitandvisitCurrentout to module scope, which would cut the number further. I decided against it because it means passing thevisitingset andportablecache around as arguments, and that felt risky on recursive code with a cycle guard when the smell was already fixed.3. Validation
How did you validate that the change is correct?

The tests in
packages/httpapi-codegen/test/generate.test.tsalready cover this code. They check exact error text and exact pass/fail results, so any change in behavior would break them. All 66 tests pass, same as before the refactor.Attach a screenshot of the test coverage showing the lines were executed by the tests.
Attach a screenshot showing the tests that cover the change passing during CI
Attach a screenshot of


qlty smells --no-snippets <full/path/to/file.ts>showing fewer reported issues after the changes.local test passing

