Refactor (packages/console/app/src/routes/zen/util/handler.ts): Function with many parameters - #107
Open
iibrohim-hash wants to merge 7 commits into
Open
iibrohim-hash wants to merge 7 commits into
iibrohim-hash wants to merge 7 commits into
Conversation
Reduces selectProvider's 11 positional parameters to a single SelectProviderParams object, resolving the Qlty 'many parameters' smell. Pure signature change — no behavioral changes. Updated the single call site in retriableRequest accordingly.
Reduces selectProvider's 11 positional parameters to a single SelectProviderParams object, resolving the Qlty 'many parameters' smell. Moved selectProvider to its own module (selectProvider.ts) so it can be unit tested without pulling in Database/SST-linked dependencies from handler.ts. Added selectProvider.test.ts covering provider selection, exclusion, sticky provider preference, and the no-provider-available error path. Updated the single call site in handler.ts's retriableRequest accordingly.
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
1 participant
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: Starter Task: Refactoring PR
1. Issue
Link to the associated GitHub issue:
#90
Full path to the refactored file:
packages/console/app/src/routes/zen/util/handler.ts
What do you think this file does?
Handles incoming API requests to the "zen" LLM proxy endpoint: authenticates the caller, validates the requested model, selects an upstream provider, forwards the request, and tracks usage/billing.
What is the scope of your refactoring within that file?
The
selectProviderfunction and its single call site insideretriableRequest. I also movedselectProviderinto its own module,selectProvider.ts, so it could be unit tested independently ofhandler.ts's database/config dependencies.Which Qlty-reported issue did you address?
Qlty "Function with many parameters" —
selectProviderhad 11 positional parameters (BEFORE: count = 11).2. Refactoring
How did the specific issue you chose impact the codebase's maintainability?
11 positional parameters made call sites error-prone (easy to mix up argument order) and made the function signature hard to read or safely extend.
What changes did you make to resolve the issue?
Introduced a
SelectProviderParamsinterface bundling all 11 parameters into a single object, changedselectProviderto accept that one object, destructured it inside the function, and updated the single call site inretriableRequestto pass an object literal instead of positional arguments.How do your changes improve maintainability? Did you consider alternatives?
A single named-parameter object makes the call site self-documenting and order-independent, and makes it easy to add new options later without breaking existing callers. I considered splitting the function into two smaller ones instead, but that would have added indirection without addressing the actual smell.
3. Validation
How did you validate that the change is correct?
Ran
qlty smellsbefore and after confirming the "many parameters" smell is resolved; ranbun lint(no new issues introduced in changed files); ran the full existing test suite (bun test, 23/25 pass — the 2 failures are inproviderUsage.test.ts, pre-existing onmainand unrelated to this change, confirmed by reproducing the identical failure with this branch's changes stashed); addedapp/test/selectProvider.test.tswith 4 new tests directly exercisingselectProvider's new object-based signature (provider selection, exclusion via retry, sticky provider preference, and the no-provider-available error path).Attach a screenshot of the test coverage showing the lines were executed by the tests.
bun testbun lintFull repo: 698 warnings, 2 errors, pre-existing on main, unrelated to this PR.
Scoped to this PR's changed files (handler.ts, selectProvider.ts): only 2 pre-existing warnings remain in handler.ts (unused destructured variables reqBody and reasoningTokens, not touched by this refactor). selectProvider.ts introduces zero lint issues.
before
after
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.before
after
