Skip to content

chore(config-parser): pass biome lint#373

Open
rezk2ll wants to merge 1 commit into
devfrom
chore/config-parser-biome-pass
Open

chore(config-parser): pass biome lint#373
rezk2ll wants to merge 1 commit into
devfrom
chore/config-parser-biome-pass

Conversation

@rezk2ll
Copy link
Copy Markdown
Member

@rezk2ll rezk2ll commented May 26, 2026

Summary

Switches any to unknown in the public ConfigProperty.default, OldConfigDescription value union, Configuration map, and the internal coerceValue return; replaces (error as any).code = "ENOENT" with NodeJS.ErrnoException; converts the readFile mock from async to explicit Promise.resolve()/reject(); and drops the unused async keyword from tests whose bodies do not await.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6ec54212-3f4a-4481-905a-3d0c8893471d

📥 Commits

Reviewing files that changed from the base of the PR and between 93e11b7 and efebccf.

📒 Files selected for processing (3)
  • packages/config-parser/src/index.test.ts
  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (7)
packages/config-parser/**/*.{ts,tsx}

📄 CodeRabbit inference engine (packages/config-parser/AGENTS.md)

packages/config-parser/**/*.{ts,tsx}: Use twakeConfig(desc, defaultConfigFile?, useEnv?, useOldParser?) as the sole public API for configuration loading and validation in @twake/config-parser
Define each configuration key's type, default, and required status using the ConfigDescription type in @twake/config-parser
Supported configuration types in @twake/config-parser are: number, boolean, array, json, object, string

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
packages/config-parser/src/**/*.ts

📄 CodeRabbit inference engine (packages/config-parser/AGENTS.md)

packages/config-parser/src/**/*.ts: Unknown keys in the config file must throw UnacceptedKeyError in @twake/config-parser
Missing required configuration keys must throw MissingRequiredConfigError in @twake/config-parser

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{js,ts,jsx,tsx}: Code must follow the philosophy of simplicity over cleverness - junior developers should understand code in 30 seconds, avoid metaprogramming, deep generics, decorator magic, and prefer readable for loops over complex chains like .reduce().flatMap().filter()
Code must follow the philosophy of explicit over implicit - dependencies must be injected not imported globally, errors must be typed not caught-and-rethrown, data flows must be traceable through function signatures
Code comments must explain why, never what - the code itself explains what
Do not use // comments to disable code - delete dead code instead, as Git has history

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{ts,tsx}: Code must follow the philosophy of boundaries over conventions - use module facades enforced by lint rules instead of comments, prefer #private fields over naming conventions, prefer TypeScript types over JSDoc comments
Do not introduce new any types in TypeScript - warnings are existing tech debt, new ones are blockers

Enforce TypeScript strict mode across all packages in ToM-Server

**/*.{ts,tsx}: Use PascalCase for types, interfaces, classes, and enums
Return types must be explicit on all non-trivial functions
Every function must return a meaningful value (void is forbidden)
Use ActionResult type for functions that perform actions with no natural data return
Use Result<T, E> type for functions that produce data, making failure first-class
any type is forbidden without exception
as unknown as T double casting is forbidden without exception
Use unknown over any for data from external sources (HTTP, JSON.parse, databases)
Prefer type for unions and intersections, interface for object shapes
Avoid TypeScript enum; use string union types for internal values
Provide type guards and validation helpers for string unions that cross system boundaries
Use Result or ActionResult for expected, domain-meaningful failures (not exceptions)
Use Error.cause when wrapping or rethrowing errors to preserve the original error chain
Type caught errors correctly using instanceof checks (not casting unknown to Error)
Prefer functions and plain objects over classes; use classes only for genuine encapsulation with private mutable state and lifecycle
Do not use static-only classes to group related functions; use named exports instead
Classes must have a single responsibility
@ts-ignore and @ts-expect-error must have a written explanation

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use ES modules (type: module) throughout the ToM-Server project

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
**/*.{js,ts,tsx,jsx}

📄 CodeRabbit inference engine (CODING_STYLE.md)

**/*.{js,ts,tsx,jsx}: Use 2 spaces for indentation (not 4, not tabs)
Opening braces must go on the same line (never on a new line)
Use trailing commas in multi-line structures
Semicolons are required on all statements
Maximum line length is 120 characters (hard limit)
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE only for module-level primitive constants that never change
Boolean variables must use is/has/can prefix (e.g., isLoading, hasPermission, canRetry)
Do not abbreviate variable names beyond accepted list (i, j, e, err, ctx, req, res)
Functions must have a single responsibility (no 'and' in function names)
Use function declarations for named, standalone, exported units; use arrow functions for callbacks and inline helpers
Maximum 5 function arguments (use options object for more parameters)
Keep functions short (25-40 lines maximum, fit on one screen without scrolling)
Recursion must be tail-call only, or use an iterative loop instead
Maximum 2 levels of nesting (no level 3)
Use early returns to reduce nesting and establish preconditions
Avoid else after a return
Throw exceptions only for invariant violations and programming errors
Catch errors at system boundaries (HTTP handlers, job runners, event listeners), not deep in business logic
Never swallow errors silently (no empty catch blocks)
finally is for cleanup only, not for conditional logic
Import from specific files, not barrel exports
Organize imports in order: Node built-ins, external packages, internal absolute paths, internal relative paths (with blank lines between groups)
Comments must explain why, not what
Document function contracts in JSDoc, not implementation mechanics
TODO comments must have an owner name and ticket reference
Prefer async/await over .then() chains
Run independent async operations in parallel using Promise.all
Never fire-and-forget async operations without a .catch() handler
Use === instead of == (never use loose equality)
Do not use mutabl...

Files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
**/index.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CODING_STYLE.md)

Do not use barrel re-exports of entire directories

Files:

  • packages/config-parser/src/index.ts
🧠 Learnings (1)
📚 Learning: 2026-03-31T07:26:27.898Z
Learnt from: pm-McFly
Repo: linagora/ToM-server PR: 355
File: packages/matrix-identity-server/src/db/index.ts:413-413
Timestamp: 2026-03-31T07:26:27.898Z
Learning: When reviewing TypeScript code in this repo, follow Biome’s `noDoubleEquals` rule: do not use loose equality (`== null` / `!= null`) as a shorthand. For nullish checks, use explicit strict comparisons instead (e.g., `value === null || value === undefined` or `value !== null && value !== undefined`).

Applied to files:

  • packages/config-parser/src/index.ts
  • packages/config-parser/src/types.ts
  • packages/config-parser/src/index.test.ts
🔇 Additional comments (6)
packages/config-parser/src/types.ts (1)

18-18: Solid type-safety tightening.

Systematically eliminating any in favor of unknown for external configuration data is exactly the right move. Config values come from files and environment variables—external sources that deserve proper type guards, not blind trust. This forces callers to narrow types properly instead of letting type-unsound any leak through the codebase.

Also applies to: 36-39, 61-61

packages/config-parser/src/index.ts (1)

30-30: Implementation signature aligns with tightened contracts.

The coerceValue return type update mirrors the type contract changes in types.ts. Since this helper returns genuinely heterogeneous types based on the switch cases, unknown is the honest signature. Callers already handle the uncertainty by storing into Configuration[string]: unknown.

packages/config-parser/src/index.test.ts (4)

71-71: Proper error typing eliminates forbidden any.

Using NodeJS.ErrnoException for filesystem errors with .code properties is the correct play. This is the built-in type for exactly this pattern. Beats the hell out of casting to any and hoping for the best.

Also applies to: 82-82, 485-485


67-75: Mock refactor is explicit and correct.

Replacing the async wrapper with explicit Promise.resolve() / Promise.reject() is semantically equivalent but clearer about what the mock returns. No need for the async sugar when you're directly controlling the Promise outcomes. Good move.


96-96: Correctly stripped unnecessary async keywords.

None of these test callbacks contain await statements—they're synchronous tests. Removing the async cruft is correct cleanup. If it doesn't await, it shouldn't be async.

Also applies to: 112-112, 138-138, 159-159, 188-188, 219-219, 255-255, 261-261, 267-267, 273-273, 279-279, 289-289, 307-307, 319-319, 339-339, 353-353, 373-373, 380-380


505-543: Expanded edge-case coverage.

New tests properly verify null/undefined default handling and required-key validation across environment and default value sources. Good coverage expansion—these are the exact edge cases that bite you in production if untested.


📝 Walkthrough

Fundamental Issue Fixed

TypeScript type safety was being subverted by loose any types in public APIs and error handling. The config-parser module exposed its internal weak typing to consumers, and test code was casting errors as any to force-set properties—both patterns that defeat static type checking entirely.

Changes Made

Type System Hardening:

  • ConfigProperty.default, Configuration, and OldConfigDescription all shifted from any to unknown. This forces consumers to perform explicit type narrowing rather than treating values as magically typed.
  • Error code assignment in mocks changed from (error as any).code = "ENOENT" to proper NodeJS.ErrnoException typing, eliminating unsafe casts.

Test Cleanup:

  • Removed async keywords from test callbacks that don't use await. These were semantic noise; the tests work identically as synchronous functions.
  • Expanded test coverage for null/undefined defaults and missing required config behavior.
  • Adjusted the fs.promises.readFile mock from an async function body to explicit Promise.resolve()/Promise.reject() calls.

No Logic Changes:
The runtime behavior is identical. This is purely a type-checking pass to satisfy Biome's linter rules—no algorithms modified, no APIs deprecated, no legacy code removed.

Walkthrough

This PR tightens TypeScript type safety in the config-parser module by systematically replacing any with unknown across exported type definitions and implementation signatures. The test suite is refactored to use synchronous callbacks and enhanced with improved mock error typing and expanded edge-case coverage for default handling and missing-key scenarios.

Changes

Type Safety Tightening

Layer / File(s) Summary
Type contract updates
packages/config-parser/src/types.ts
ConfigProperty.default, OldConfigDescription, and Configuration exports change from any to unknown, tightening the type contract throughout the module.
Implementation type signature alignment
packages/config-parser/src/index.ts
coerceValue return type updates from any to unknown to align with the tightened type contracts.
Test mock infrastructure and error typing
packages/config-parser/src/index.test.ts
Mock filesystem helper refactored to use Promise.reject/Promise.resolve and proper NodeJS.ErrnoException error code typing; removes loose any typing from error setup.
Test callback refactoring and coverage expansion
packages/config-parser/src/index.test.ts
Async test callbacks converted to synchronous style across all test suites; edge-case coverage expanded for null/undefined defaults, MissingRequiredConfigError with environment/default satisfaction, and boolean coercion mixed-case parsing.

Suggested labels

chore, package::configuration, priority::normal

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description lacks the required 'What', 'Why', 'How', and 'Checklist' sections from the template. It provides only a technical summary without addressing the template structure or linking an issue. Restructure the description using the template: add 'What' section (1 paragraph), 'Why' (with issue link), 'How' (if needed), and complete the 'Checklist' with relevant items checked off.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'chore(config-parser): pass biome lint' directly describes the main objectives: replacing 'any' with 'unknown', fixing typing issues, and removing unused async keywords to satisfy linter requirements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented May 26, 2026

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

To disable these notifications, a workspace admin can disable them in workspace settings.


View your CI Pipeline Execution ↗ for commit efebccf

Command Status Duration Result
nx affected -t build ❌ Failed 16s View ↗

☁️ Nx Cloud last updated this comment at 2026-05-26 17:58:15 UTC

@rezk2ll rezk2ll requested a review from pm-McFly May 26, 2026 19:04
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.

1 participant