fix(common-settings-bridge): clear lint and harden message validation#368
fix(common-settings-bridge): clear lint and harden message validation#368rezk2ll wants to merge 3 commits into
Conversation
Removes `any` casts from the AMQP bridge tests and the `as unknown as CommonSettingsMessage` cast in the message handler. `validateMessage` now accepts `Record<string, unknown>` and narrows fields at runtime, matching the actual shape the lib hands us.
`typeof x === "number"` lets NaN and Infinity through, both of which JSON producers can reach via `1e1000` / `-1e1000`. An Infinity timestamp crashed `new Date(...).toISOString()` inside the info log, escaping the validation try/catch and turning the intended ack-and-drop into a retry-then-DLQ storm. A NaN/-Infinity version silently lost legitimate updates because every comparison in shouldApplyUpdate returns false. Switch both guards to `Number.isFinite` and split the payload-not-object error from UserIdNotProvidedError so the two failure modes are distinguishable in logs.
📝 WalkthroughFundamental flaw Systemic data-flow and core logic changes
Deprecated APIs / removed legacy code
Test and typing hardening
Explicitly ignored technical debt
WalkthroughThis PR adds runtime validation for AMQP messages in CommonSettingsBridge, replaces positional persistence args with a single exported SaveSettingsMeta, and tightens Jest mock typings and test fixtures across bridge, repository, index, and profile-updater tests. ChangesMessage Validation Type Safety
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
|
View your CI Pipeline Execution ↗ for commit 3bed56c
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02895298-0dff-4074-b94c-e411ccd9287f
📒 Files selected for processing (3)
packages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test / Test Affected Packages
- GitHub Check: Docs / Update Documentation
🧰 Additional context used
📓 Path-based instructions (5)
packages/common-settings-bridge/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/common-settings-bridge/AGENTS.md)
Use
expressfor the HTTP server implementation of the Application Service endpoint
Files:
packages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/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/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/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#privatefields 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 blockersEnforce 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
anytype is forbidden without exception
as unknown as Tdouble casting is forbidden without exception
Useunknownoveranyfor data from external sources (HTTP, JSON.parse, databases)
Prefertypefor unions and intersections,interfacefor object shapes
Avoid TypeScriptenum; 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 usinginstanceofchecks (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-ignoreand@ts-expect-errormust have a written explanation
Files:
packages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/index.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ES modules (type: module) throughout the ToM-Server project
Files:
packages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/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)
Usefunctiondeclarations 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
Avoidelseafter areturn
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)
finallyis 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
Preferasync/awaitover.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/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/index.test.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/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/index.test.ts
🔇 Additional comments (1)
packages/common-settings-bridge/src/index.test.ts (1)
2-4: LGTM!Also applies to: 6-6, 16-19, 94-94, 96-101, 143-148, 175-203, 209-209, 226-226, 245-245, 265-265, 289-289, 308-308, 337-337, 353-353, 366-366, 389-389, 417-417, 453-453, 466-466, 478-478
| if (!Number.isFinite(timestamp)) { | ||
| throw new MessageParseError("Message missing required timestamp field"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, what exact millisecond range is valid for new Date(ms).toISOString()before it throwsRangeError?
💡 Result:
In JavaScript/TypeScript, new Date(ms).toISOString() throws a RangeError when the Date’s internal time value is not a finite millisecond time (e.g., NaN/Infinity) or when it is outside the supported time-value range. Exact millisecond bounds for the supported Date time value are: - Valid range: -8,640,000,000,000,000 ms through +8,640,000,000,000,000 ms (inclusive) - RangeError occurs for any ms strictly less than -8,640,000,000,000,000 or strictly greater than +8,640,000,000,000,000 (and for non-finite timestamps). [1][2] So, the “valid ms range” before toISOString() throws is: - -8640000000000000 <= ms <= 8640000000000000 [2][1] Notes: - toISOString() throws when the Date object is invalid (MDN describes this as “Thrown if the date is invalid”). [3] - The ECMAScript spec states the time value range supported by a Date is -8,640,000,000,000,000 to +8,640,000,000,000,000 milliseconds. [1]
Citations:
- 1: https://tc39.es/ecma262/multipage/numbers-and-dates.html
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="$(pwd)"
file="packages/common-settings-bridge/src/bridge.ts"
# Show relevant sections without dumping the whole file
echo "=== validateMessage area (lines ~1-120) ==="
nl -ba "$file" | sed -n '1,140p'
echo "=== other relevant areas (~230-320) ==="
nl -ba "$file" | sed -n '220,330p'
echo "=== formatTimestamp / usage area (~450-560) ==="
nl -ba "$file" | sed -n '440,560p'Repository: linagora/ToM-server
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/common-settings-bridge/src/bridge.ts"
echo "=== validateMessage area (lines ~1-180) ==="
cat -n "$file" | sed -n '1,180p'
echo "=== other relevant areas (~180-360) ==="
cat -n "$file" | sed -n '180,360p'
echo "=== formatTimestamp / usage area (~360-620) ==="
cat -n "$file" | sed -n '360,620p'Repository: linagora/ToM-server
Length of output: 26029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "saveSettings\\(" packages/common-settings-bridge/src
rg -n "timestamp" packages/common-settings-bridge/src/types.ts packages/common-settings-bridge/src/settings-repository.ts packages/common-settings-bridge/src -SRepository: linagora/ToM-server
Length of output: 11976
Guard timestamp against JS Date out-of-range, not just finiteness.
Number.isFinite still lets finite millisecond values outside [-8_640_000_000_000_000, +8_640_000_000_000_000] through; new Date(ms).toISOString() will throw RangeError, and your formatTimestamp(timestamp) calls happen after validation (outside the validateMessage try/catch), so this can still cascade into retries/DLQ.
Suggested hardening
+const MAX_JS_DATE_MS = 8_640_000_000_000_000;
+
+function isValidTimestamp(value: unknown): value is number {
+ return (
+ typeof value === "number" &&
+ Number.isFinite(value) &&
+ Number.isInteger(value) &&
+ Math.abs(value) <= MAX_JS_DATE_MS
+ );
+}
+
function validateMessage(message: Record<string, unknown>): ParsedMessage {
const requestId = message.request_id;
if (typeof requestId !== "string" || requestId.length === 0) {
throw new MessageParseError("Message missing required request_id field");
}
const timestamp = message.timestamp;
- if (!Number.isFinite(timestamp)) {
+ if (!isValidTimestamp(timestamp)) {
throw new MessageParseError("Message missing required timestamp field");
}
const payload = message.payload;
@@
return {
@@
- timestamp: timestamp as number,
+ timestamp,
@@
};
}| mockUploadContent.mockResolvedValue("mxc://example.com/uploaded123"); | ||
| // Mock global fetch for avatar downloads | ||
| global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as any; | ||
| global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as unknown as typeof fetch; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no forbidden double-casts remain in this test file (and optionally repo-wide).
rg -n --type=ts '\bas\s+unknown\s+as\b' packages/common-settings-bridge/src/index.test.ts
rg -n --type=ts '\bas\s+unknown\s+as\b' packages/common-settings-bridge/srcRepository: linagora/ToM-server
Length of output: 1135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/common-settings-bridge/src/index.test.ts"
# Print around the flagged line
sed -n '120,170p' "$FILE" | cat -n
# Also locate and print createMockFetchResponse definition and its return type
rg -n "createMockFetchResponse" "$FILE"
python3 - <<'PY'
import re,sys,subprocess,os,shlex, textwrap, json
file="packages/common-settings-bridge/src/index.test.ts"
# Find line numbers for the function/const definition
import pathlib
p=pathlib.Path(file)
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if "createMockFetchResponse" in l and ("function" in l or "const" in l or "let" in l or "async" in l):
print("DEF_CANDIDATE", i, l.strip())
PYRepository: linagora/ToM-server
Length of output: 2471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/common-settings-bridge/src/index.test.ts"
# show top of file to see environment and imports
sed -n '1,80p' "$FILE" | cat -n
# show around flagged line
sed -n '100,160p' "$FILE" | cat -nRepository: linagora/ToM-server
Length of output: 5435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/common-settings-bridge/src/index.test.ts"
# show top of file to see environment and imports
sed -n '1,80p' "$FILE" | cat -n
# show around flagged line
sed -n '100,160p' "$FILE" | cat -nRepository: linagora/ToM-server
Length of output: 5435
Kill the forbidden as unknown as typeof fetch in packages/common-settings-bridge/src/index.test.ts
The global.fetch mock still uses a forbidden as unknown as T double-cast; replace it with a single-type assertion on the mock function.
Suggested fix
- global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as unknown as typeof fetch;
+ global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as jest.MockedFunction<typeof fetch>;Also, packages/common-settings-bridge/src/index.ts and packages/common-settings-bridge/src/bridge.test.ts still contain as unknown as casts and need the same treatment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as unknown as typeof fetch; | |
| global.fetch = jest.fn(() => Promise.resolve(createMockFetchResponse())) as jest.MockedFunction<typeof fetch>; |
Replaces `any` and `(err as any)?.code` with proper `unknown` narrowing in the profile updater and settings repository, bundles `saveSettings` extra parameters into a `SaveSettingsMeta` object to satisfy `useMaxParams`, and swaps `(MockLogger as any).configure` for `Object.assign` in the logger mock. `biome ci` now exits 0 across `packages/common-settings-bridge`.
rezk2ll
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79122871-8351-432b-9756-3b863a727bb3
📒 Files selected for processing (7)
packages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test / Test Affected Packages
- GitHub Check: Docs / Update Documentation
🧰 Additional context used
📓 Path-based instructions (7)
packages/common-settings-bridge/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/common-settings-bridge/AGENTS.md)
Use
expressfor the HTTP server implementation of the Application Service endpoint
Files:
packages/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.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/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.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#privatefields 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 blockersEnforce 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
anytype is forbidden without exception
as unknown as Tdouble casting is forbidden without exception
Useunknownoveranyfor data from external sources (HTTP, JSON.parse, databases)
Prefertypefor unions and intersections,interfacefor object shapes
Avoid TypeScriptenum; 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 usinginstanceofchecks (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-ignoreand@ts-expect-errormust have a written explanation
Files:
packages/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ES modules (type: module) throughout the ToM-Server project
Files:
packages/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.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)
Usefunctiondeclarations 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
Avoidelseafter areturn
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)
finallyis 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
Preferasync/awaitover.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/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.ts
packages/common-settings-bridge/src/index.ts
📄 CodeRabbit inference engine (packages/common-settings-bridge/AGENTS.md)
The
src/index.tsfile is the bridge entry point and Application Service handler
Files:
packages/common-settings-bridge/src/index.ts
**/index.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STYLE.md)
Do not use barrel re-exports of entire directories
Files:
packages/common-settings-bridge/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/common-settings-bridge/src/matrix-profile-updater.tspackages/common-settings-bridge/src/matrix-profile-updater.test.tspackages/common-settings-bridge/src/settings-repository.test.tspackages/common-settings-bridge/src/settings-repository.tspackages/common-settings-bridge/src/bridge.test.tspackages/common-settings-bridge/src/bridge.tspackages/common-settings-bridge/src/index.ts
🔇 Additional comments (9)
packages/common-settings-bridge/src/bridge.ts (1)
347-352: LGTM!packages/common-settings-bridge/src/settings-repository.ts (3)
7-17: LGTM!
246-247: LGTM!
264-267: LGTM!packages/common-settings-bridge/src/matrix-profile-updater.test.ts (1)
6-9: LGTM!Also applies to: 15-15
packages/common-settings-bridge/src/matrix-profile-updater.ts (1)
5-19: LGTM!Also applies to: 113-121, 255-260, 300-307
packages/common-settings-bridge/src/settings-repository.test.ts (1)
15-18: LGTM!Also applies to: 25-27, 176-177, 195-196, 219-221, 233-233, 259-259, 263-263, 273-275, 289-289
packages/common-settings-bridge/src/bridge.test.ts (1)
363-368: LGTM!Also applies to: 453-458, 473-478
packages/common-settings-bridge/src/index.ts (1)
17-17: LGTM!
| setAvatarUrl: jest.fn(), | ||
| matrixClient: mockMatrixClient, | ||
| } as any; | ||
| } as unknown as jest.Mocked<Intent>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check for forbidden double casts in the touched test file
rg -nP 'as\s+unknown\s+as' packages/common-settings-bridge/src/matrix-profile-updater.test.tsRepository: linagora/ToM-server
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba packages/common-settings-bridge/src/matrix-profile-updater.test.ts | sed -n '15,60p'Repository: linagora/ToM-server
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/common-settings-bridge/src/matrix-profile-updater.test.ts | sed -n '20,55p'Repository: linagora/ToM-server
Length of output: 1296
Kill the as unknown as ... double-casts in matrix-profile-updater.test.ts (type theater).
Those as unknown as jest.Mocked<Intent> and as unknown as jest.Mocked<Logger> assertions are still present at lines 29 and 44; replace them with narrow mock shapes (only the methods/properties this test uses).
Suggested refactor
- let mockIntent: jest.Mocked<Intent>;
- let mockLogger: jest.Mocked<Logger>;
+ type MockIntent = {
+ setDisplayName: jest.MockedFunction<(displayName: string) => Promise<void>>;
+ setAvatarUrl: jest.MockedFunction<(avatarUrl: string) => Promise<void>>;
+ matrixClient: MockMatrixClient;
+ };
+
+ type MockLogger = Pick<jest.Mocked<Logger>, "debug" | "info" | "warn" | "error">;
+
+ let mockIntent: MockIntent;
+ let mockLogger: MockLogger;
@@
- mockIntent = {
+ mockIntent = {
setDisplayName: jest.fn(),
setAvatarUrl: jest.fn(),
matrixClient: mockMatrixClient,
- } as unknown as jest.Mocked<Intent>;
+ };
@@
- mockLogger = {
+ mockLogger = {
debug: jest.fn(),
info: jest.fn(),
warn: jest.fn(),
error: jest.fn(),
- } as unknown as jest.Mocked<Logger>;
+ };| repository = new SettingsRepository( | ||
| mockDb as unknown as RealDatabase<UserSettingsTableName>, | ||
| mockLogger as unknown as RealLogger, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no forbidden double-casts remain in this touched test file.
rg -n --type=ts '\bas unknown as\b' packages/common-settings-bridge/src/settings-repository.test.ts
# Expected after fix: no output.Repository: linagora/ToM-server
Length of output: 176
Remove the remaining as unknown as double-casts in SettingsRepository test setup
packages/common-settings-bridge/src/settings-repository.test.ts (lines 83-84) still uses mockDb as unknown as RealDatabase<UserSettingsTableName> and mockLogger as unknown as RealLogger; replace the casts with properly typed constructor-argument types so there’s no forbidden as unknown as T chain.
2 participants
Summary
The whole
packages/common-settings-bridgepackage now passesbiome cicleanly. #367 merged with a "we'll clean it up later" compromise; this is the cleanup. Along the way, replacing theas anycasts in the message handler with real runtime narrowing uncovered two latent bugs that the type-erasure had been hiding.The handler typed timestamp and version as numbers but relied on a falsy/null check, so any JSON producer could send
1e1000(parses toInfinity) and slip past validation. AnInfinitytimestamp then crashednew Date(...).toISOString()inside the info log, escaped the validation try/catch, and turned the intended ack-and-drop into a retry-then-DLQ storm. A non-finite version made every comparison inshouldApplyUpdatereturn false and silently discarded legitimate updates as "stale". Both guards now useNumber.isFinite.Also splits the "payload is not an object" case out of
UserIdNotProvidedErrorso the two failure modes show up differently in logs, and refactorssaveSettingsto take a meta object since six positional parameters trippeduseMaxParams.The lint CI is still red overall because the rest of the monorepo (apps, other packages) has its own pre-existing errors, but the package this PR touches is clean.