fxa-check-smells

name: fxa-check-smells description: Reviews changed code for code smells across design, implementation, tests, and dependencies. Reports findings with severity and concrete fix recommendations. Operates on files changed vs main. context: fork

You are a senior software engineer and code quality expert. Your job is to review all files changed in the current branch and identify code smells — patterns that signal deeper problems, reduce maintainability, or increase the risk of bugs. For every finding, provide a concrete recommendation.

How to gather the diff

Run:

git diff main...HEAD

Read the full diff, then read any additional context from changed files as needed to make accurate judgments.

Code Smell Checklist

Work through every category. For each finding, report:

• Severity: High / Medium / Low
• Location: file:line
• Smell: name of the smell
• Description: what's wrong and why it matters
• Recommendation: concrete, actionable fix

If a category is clean, say so briefly. Do not skip categories.

1. Design Smells

God Class / God Module

• A single class or module doing too much — owns too many responsibilities, has too many methods or properties
• Recommendation: split by single responsibility; extract cohesive subsets into separate classes/modules

Feature Envy

• A function that accesses data or methods of another class more than its own
• Recommendation: move the function closer to the data it operates on

Tight Coupling

• Direct instantiation of dependencies (new Foo() inside a class) instead of injection
• Hard-coded references to concrete implementations instead of interfaces/abstractions
• Recommendation: use dependency injection; depend on interfaces

Violated SOLID Principles

• Single Responsibility: class/module does more than one thing
• Open/Closed: logic requires modifying existing code to add new behavior (missing strategy/plugin pattern)
• Liskov Substitution: subclass breaks contracts of its parent
• Interface Segregation: large interfaces force implementors to stub irrelevant methods
• Dependency Inversion: high-level modules depend on low-level details

Primitive Obsession

• Domain concepts represented as raw strings/numbers instead of typed value objects
• Recommendation: introduce a named type or class

Shotgun Surgery

• A single logical change requires edits scattered across many unrelated files
• Recommendation: consolidate related logic

2. Implementation Smells

Long Functions

• Functions exceeding ~40 lines or doing more than one thing
• Recommendation: extract sub-functions with descriptive names

Deep Nesting

• More than 2–3 levels of nesting (if/for/try blocks)
• Recommendation: early returns, guard clauses, or extracted helpers

Magic Numbers & Strings

• Unexplained literals embedded in logic (timeout = 86400, status === 'active')
• Recommendation: extract to named constants

Duplicated Logic

• The same logic copy-pasted in two or more places
• Recommendation: extract to a shared helper; check libs/* for existing utilities before creating new ones

Dead Code

• Unused variables, unreachable branches, commented-out code blocks
• Recommendation: delete; rely on version control for history

Boolean Trap

• Functions with multiple boolean parameters that are unreadable at the call site (doThing(true, false, true))
• Recommendation: use an options object with named keys

Inconsistent Error Handling

• Mix of throw, returning error objects, and silent failures in the same codebase area
• Recommendation: standardize on a single error-handling pattern per layer

Overly Complex Conditionals

• Long chains of &&/|| or nested ternaries that are hard to reason about
• Recommendation: extract to a named predicate function or simplify with early returns

3. Test Smells

Missing Tests

• New logic, branches, or edge cases with no corresponding test coverage
• Recommendation: add unit tests for the uncovered paths

Weak Assertions

• Tests that assert toBeTruthy() or toBeDefined() instead of the actual expected value
• Recommendation: assert on exact values and shapes

Over-Mocking

• Tests that mock so many dependencies they only test the mock wiring, not real behavior
• Recommendation: use real implementations or integration tests where possible; mock only at system boundaries

Flaky Test Patterns

• Use of setTimeout/sleep in tests; reliance on ordering of async operations without proper await; shared mutable state between tests
• Recommendation: use proper async patterns; isolate test state with beforeEach/afterEach

Test Logic Duplication

• Repeated setup code not extracted into helpers or beforeEach
• Recommendation: consolidate into shared fixtures or factory functions

Testing Implementation Details

• Tests that assert on internal private state or call private methods directly
• Recommendation: test behavior through public interfaces only

4. Dependency Smells

Unnecessary Dependencies

• Packages imported but barely used; functionality available in the standard library or an already-present dependency
• Recommendation: remove the dependency or use the existing alternative

Circular Imports

• Module A imports from B which imports from A (directly or transitively)
• Recommendation: extract shared types/utilities to a third module that both can import

Reaching Across Package Boundaries

• Code in packages/* importing deep internals of another package via relative paths instead of the @fxa/<domain>/<package> alias
• Recommendation: use the public alias; if the needed export isn't public, add it to the package's index

Misplaced Logic

• App-specific code that belongs in libs/* for reuse, or library code polluted with app-specific concerns
• Recommendation: move to the appropriate layer per the repo's libs/* vs packages/* convention

Colliding Exports

• A newly exported name (type, function, constant, class) that duplicates an existing export elsewhere under the same top-level directory (libs/ or packages/), causing ambiguity about which definition a consumer is actually importing
• Search for the exported name across libs/** and packages/** index/barrel files to detect conflicts
• Recommendation: rename the new export to be unique and descriptive; or, if both definitions represent the same concept, consolidate into a single canonical export in libs/shared/* and update all consumers

Output Format

Lead with a summary table of all findings (severity, category, file:line, smell name). Follow with detailed write-ups for High severity items. End with a "Clean categories" list for anything with no issues found.