Project: doctolib-worktrees · Branch: HEAD · Mode:  · Model: claude-opus-4-7

Stats: 1 user messages, 17 tool calls, tools used: Bash, ToolSearch, mcp__plugin_github_github__pull_request_read, mcp__plugin_github_github__get_file_contents.

Conversation

Turn 1 — User

Review code simplification opportunities in PR #7487 from doctolib/doctolib-pro-desktop.

Context
This is a dead-code removal PR. It removes the useConnectorContextFromElectronSettings field from TiConnectSettings. The PR is generally clean, but review the following specific concerns:

Files to review:

1. src/modules/de/kbv-ti-connect/services/configuration-service/__tests__/index.test.ts

The contextFromRequest object { MandantId: 'Mandant1', ClientSystemId: 'ClientID1', WorkplaceId: 'Workplace1', UserId: 'User1' } is now duplicated in 4 places across different describe blocks:
- configuration describe block
- authSignAlgorithm describe block  
- isJwaSelectionEnabled describe block
- isEccSignatureEnforced describe block

Each test creates a local const contextFromRequest = { MandantId: 'Mandant1', ... }.

Also note that debugData hardcodes the same values: "MandantId":"Mandant1","ClientSystemId":"ClientID1","WorkplaceId":"Workplace1","UserId":"User1".

2. src/modules/de/kbv-ti-connect/services/configuration-service/index.ts

After the PR, the obtainConfiguration method has this structure:
if (contextFromRequest) {
  contextToUse = this.normalizeRequestContext(contextFromRequest)
} else {
  kbvTiConnectLogger.error(...)
  throw new Error(i18n.t('ti_connect.error_messages.missing_connector_context'))
}

Is there a simpler way to write this (e.g., guard clause)?

3. src/modules/de/kbv-ti-connect/services/types.ts

The UserEditableTiConnectSetting type is a complex exclusion. After the removal, verify whether the exclusion list is still clean and there are no other obsolete entries.

Questions:
1. Should contextFromRequest be hoisted to the describe('ConfigurationService') level or at minimum a beforeEach-level variable in each relevant describe block?
2. Is the debugData constant coupling to hardcoded values a fragility concern?
3. Is there a cleaner way to write the error guard in obtainConfiguration?

Provide Conventional Comments formatted suggestions with file and line references. Focus only on the changed code, not the surrounding unchanged code.

Turn 1 — Assistant

I'll analyze the PR changes to provide specific simplification suggestions. Let me explore the relevant files first.

Tools used:

- Bash: find /Users/USER/workspace/doctolib/worktrees -type d -name "kbv-ti-connect" 2>/dev/null | head -5

Tool results:

  → result (ok): (Bash completed with no output)

Let me fetch the PR directly via GitHub MCP to inspect the changed code.

Tools used:

- ToolSearch (inputs: query, max_results)

Tool results:

  → result (ok)

Tools used:

- mcp__plugin_github_github__pull_request_read (inputs: method, owner, repo, pullNumber)

Tools used:

- mcp__plugin_github_github__pull_request_read (inputs: method, owner, repo, pullNumber)

Tool results:

  → result (ERROR): failed to get pull request diff: GET https://api.github.com/repos/doctolib/doctolib-pro-desktop/pulls/7487: 404 Not Found []

Tool results:

  → result (ERROR): failed to get pull request files: GET https://api.github.com/repos/doctolib/doctolib-pro-desktop/pulls/7487/files?page=1&per_page=30: 404 Not Found []

Tools used:

- Bash: gh pr view 7487 --repo doctolib/doctolib-pro-desktop --json title,number,headRefName,baseRefName,state 2>&1 | head -50

Tool results:

  → result (ok): {"baseRefName":"master","headRefName":"fhir-2177","number":7487,"state":"OPEN","title":"refactor(FHIR-2177): remove dead useConnectorContextFromElectronSettings flag"}

Tools used:

- Bash: gh pr diff 7487 --repo doctolib/doctolib-pro-desktop 2>&1 | head -400

Tool results:

  → result (ok): diff --git a/src/modules/de/gematik-kim/services/__tests__/account.test.ts b/src/modules/de/gematik-kim/services/__tests__/account.test.ts
index 49af5cbe08..1cea5bf357 100644
--- a/src/modules/de/gematik-kim/services/__tests__/account.test.ts
+++ b/src/modules/de/gematik-kim/services/__tests__/account.test.ts
@@ -26,7 +26,6 @@ const mockTiSettings: TiConnectSettings = {
   mandantId: '',
   workplaceId: '',
   userId: '',
-  useConnectorContextFromElectronSettings: false,
   serverCertificateFin
…(truncated, 12889 more chars)

Now let me view the full obtainConfiguration method and types.ts around the relevant areas to make informed suggestions.

Tools used:

- mcp__plugin_github_github__get_file_contents (inputs: owner, repo, path, ref)

Tools used:

- mcp__plugin_github_github__get_file_contents (inputs: owner, repo, path, ref)

Tools used:

- mcp__plugin_github_github__get_file_contents (inputs: owner, repo, path, ref)

Tool results:

  → result (ERROR): failed to resolve git reference: failed to get final reference for "refs/pull/7487/head": GET https://api.github.com/repos/doctolib/doctolib-pro-desktop/git/ref/pull/7487/head: 404 Not Found []

Tool results:

  → result (ERROR): failed to resolve git reference: failed to get final reference for "refs/pull/7487/head": GET https://api.github.com/repos/doctolib/doctolib-pro-desktop/git/ref/pull/7487/head: 404 Not Found []

Tool results:

  → result (ERROR): failed to resolve git reference: failed to get final reference for "refs/pull/7487/head": GET https://api.github.com/repos/doctolib/doctolib-pro-desktop/git/ref/pull/7487/head: 404 Not Found []

Tools used:

- Bash: gh api repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/index.ts?ref=fhir-2177 --jq '.content' | base64 -d 2>/dev/null | head -120

Tool results:

  → result (ok): (eval):1: no matches found: repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/index.ts?ref=fhir-2177

Tools used:

- Bash: gh api repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/__tests__/index.test.ts?ref=fhir-2177 --jq '.content' | base64 -d 2>/dev/null

Tool results:

  → result (ok): (eval):1: no matches found: repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/__tests__/index.test.ts?ref=fhir-2177

Tools used:

- Bash: gh api repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/types.ts?ref=fhir-2177 --jq '.content' | base64 -d 2>/dev/null

Tool results:

  → result (ok): (eval):1: no matches found: repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/types.ts?ref=fhir-2177

Tools used:

- Bash: gh api 'repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/index.ts?ref=fhir-2177' --jq '.content' | base64 -d

Tool results:

  → result (ok): import i18n from 'i18next'

import {
  ConnectorConfiguration,
  ConnectorContext,
  ConnectorContextInput,
  CryptType,
} from '@module/de/kbv-ti-connect/services/types'
import { extraLogParser, getNestedKeys } from 'src/utils/extraLogParser.ts'

import { kbvTiConnectLogger } from '../logger'
import { TiConnectRepository } from '../ti-connect-repository'

export default class ConfigurationService {
  static readonly configurationService = { name: 'ConfigurationService' }

  // TODO: Remove redu
…(truncated, 4029 more chars)

Tools used:

- Bash: gh api 'repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/configuration-service/__tests__/index.test.ts?ref=fhir-2177' --jq '.content' | base64 -d

Tools used:

- Bash: gh api 'repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/types.ts?ref=fhir-2177' --jq '.content' | base64 -d

Tool results:

  → result (ok): import i18n from 'i18next'
import { MockInstance } from 'vitest'

import { TiConnectSettings } from '@module/de/kbv-ti-connect/services/types'

import ConfigurationService from '../../configuration-service'
import { kbvTiConnectLogger } from '../../logger'
import { testStore } from '../../mock/mockStore'
import { TiConnectRepository } from '../../ti-connect-repository'

vi.mock('../../logger', () => ({
  kbvTiConnectLogger: {
    debug: vi.fn(),
    error: vi.fn(),
  },
  default: {
    debug: v
…(truncated, 7374 more chars)

Tool results:

  → result (ok): export type TiConnectAction =
  | 'startConnection'
  | 'testConnectorConfiguration'
  | 'subscribeToEventService'
  | 'unsubscribeFromEventService'
  | 'readVsd'
  | 'readKvk'
  | 'getSmcbPinStatus'
  | 'changePin'
  | 'handleSmcb'
  | 'unblockPin'
  | 'enableOfflineMode'
  | 'enableOnlineMode'
  | 'verifyCertificate'
  | 'readCardCertificate'
  | 'externalAuthenticate'
  | 'idpAuthenticate'
  | 'persistConfigurationAndRestartEventServer'
  | 'checkCertificateExpiration'
  | 'clearConnectorConf
…(truncated, 5857 more chars)

Now I have all the information I need. Note one important observation: in the current test file (on branch), the debugData string uses escaped-brace regex syntax (\\{) because it appears to be matched against an object. Also, eventSubscriptions is already listed in the exclusion but is NOT a key of TiConnectSettings (it's obsolete). Let me verify.

Tools used:

- Bash: gh api 'repos/doctolib/doctolib-pro-desktop/contents/src/modules/de/kbv-ti-connect/services/types.ts?ref=fhir-2177' --jq '.content' | base64 -d | grep -n 'eventSubscriptions\|TiConnectSettings'

Tool results:

  → result (ok): 146:export interface TiConnectSettings {
169:  TiConnectSettings,
174:  Pick<TiConnectSettings, 'eventServiceHost' | 'eventServicePort'>
204:      keyof TiConnectSettings,
208:      | 'eventSubscriptions'

Tools used:

- Bash: gh search code 'eventSubscriptions repo:doctolib/doctolib-pro-desktop' 2>&1 | head -30

Tool results:

  → result (ok): doctolib/doctolib-pro-desktop:src/modules/de/kbv-ti-connect/services/types.ts: | 'eventSubscriptions'

Confirmed: eventSubscriptions appears only in the Exclude list of UserEditableTiConnectSetting, but it's NOT a key of TiConnectSettings. This is an obsolete entry (a non-member in an Exclude<keyof T, ...> is a silent no-op in TypeScript). This is a good finding for the review.

Let me also verify the current error-flow in obtainConfiguration. The current shape is:

if (!settings) {
  throw new Error(...) // no logging
}
if (contextFromRequest) {
  contextToUse = this.normalizeRequestContext(contextFromRequest)
} else {
  kbvTiConnectLogger.error(...)
  throw new Error(...)
}

After the dead-code removal, the second branch only has two outcomes: set contextToUse or throw. A guard clause fits perfectly. Now I have enough information to present the review.

---

Review: PR #7487 — Code simplification suggestions

Below are Conventional Comments-formatted suggestions scoped strictly to code that the PR modified. I grouped them by file.

---

1. src/modules/de/kbv-ti-connect/services/configuration-service/__tests__/index.test.ts

suggestion (non-blocking): hoist the duplicated contextFromRequest fixture to describe('ConfigurationService') scope

Lines ~87, 116, 140, 169 (four identical literals):

const contextFromRequest = { MandantId: 'Mandant1', ClientSystemId: 'ClientID1', WorkplaceId: 'Workplace1', UserId: 'User1' }

Now that the useConnectorContextFromElectronSettings branch is gone, every remaining describe block in this file (except context, which uses its own '123'/'456'/... variant) needs the same Mandant1/ClientID1/Workplace1/User1 object. Duplicating it four times creates exactly the fragility your question (Q2) flags: debugData is hardcoded against those literals, and if anyone tweaks one fixture, the test silently continues to pass against a stale debugData string.

Recommended: a single const at the top-level describe('ConfigurationService') scope, next to traceId, debugParams, debugData:

describe('ConfigurationService', () => {
  const traceId = 'test-trace-id'
  const contextFromRequest = {
    MandantId: 'Mandant1',
    ClientSystemId: 'ClientID1',
    WorkplaceId: 'Workplace1',
    UserId: 'User1',
  }
  const debugParams = / ... /
  const debugData = / ... /
  // ...
}

Then each inner describe just references the outer constant. beforeEach is overkill here because the fixture is an immutable literal and is only read, never mutated.

Note: the context describe block deliberately uses different values ('123' / '456' / '789' / '101112') to verify the pass-through behavior — keep those as-is. The hoist only consolidates the Mandant1/... fixture used by the four other describes.

issue (non-blocking): debugData is coupled to literal string values

const debugData =
  '{ \\{"connectorHost":"localhost","connectorPort":"445",' +
  '"context":\\{"MandantId":"Mandant1","ClientSystemId":"ClientID1","WorkplaceId":"Workplace1","UserId":"User1"\\}\\} }'

Yes, this is a fragility concern (your Q2). The connectorHost/connectorPort values come from testStore.ti_connect, and the context values now come from the in-line contextFromRequest. If either drifts, the coupling is invisible. After you hoist contextFromRequest, you can derive debugData from the same source — at least for the context portion:

const debugData =
  { \\{"connectorHost":"${testStore.ti_connect.connectorHost}", +
  "connectorPort":"${testStore.ti_connect.connectorPort}", +
  "context":${JSON.stringify(contextFromRequest).replace(/[{}]/g, '\\$&')}\\} }

That is arguably over-engineered for a test, so a lighter alternative is to just leave debugData as a string literal but add a top-of-file comment asserting the invariant, or a small assertion helper. Pick whichever your team prefers — the hoist is the important part; this one is optional.

praise: the context describe block is much cleaner

Collapsing the two nested describes ("when context usage from ddv settings is enabled/disabled") into a single flat describe('context') with beforeEach is a nice readability win. The new structure reads linearly.

---

2. src/modules/de/kbv-ti-connect/services/configuration-service/index.ts

suggestion (non-blocking): replace the if/else in obtainConfiguration with a guard clause

Current shape (lines 66–89):

public static obtainConfiguration(
  traceId: string | number,
  contextFromRequest?: ConnectorContextInput,
): ConfigurationService {
  let contextToUse: ConnectorContext

  const { settings } = TiConnectRepository

  if (!settings) {
    throw new Error(i18n.t('ti_connect.error_messages.missing_connector_context'))
  }

  if (contextFromRequest) {
    contextToUse = this.normalizeRequestContext(contextFromRequest)
  } else {
    kbvTiConnectLogger.error(/ ... /)
    throw new Error(i18n.t('ti_connect.error_messages.missing_connector_context'))
  }
  // ...
}

Answer to your Q3: yes. Now that the happy branch is a single assignment, the else-throws pattern inverts naturally into a guard clause, which also lets you drop the let and the pre-declaration:

public static obtainConfiguration(
  traceId: string | number,
  contextFromRequest?: ConnectorContextInput,
): ConfigurationService {
  const { settings } = TiConnectRepository

  if (!settings) {
    throw new Error(i18n.t('ti_connect.error_messages.missing_connector_context'))
  }

  if (!contextFromRequest) {
    kbvTiConnectLogger.error(
      this.configurationService.name,
      new Error(i18n.t('ti_connect.error_messages.missing_connector_context')),
      {
        extra: extraLogParser({
          traceId,
          functionName: 'obtainConfiguration',
          operation: 'read',
          direction: 'output',
        }),
      },
    )
    throw new Error(i18n.t('ti_connect.error_messages.missing_connector_context'))
  }

  const contextToUse = this.normalizeRequestContext(contextFromRequest)
  // ...
}

Benefits: contextToUse becomes const, no pre-declared mutable binding, and the two error paths sit next to each other at the top of the function — which also makes it more obvious that the first guard is missing the kbvTiConnectLogger.error call that the second one has. (Possibly a pre-existing inconsistency worth a follow-up, but out of scope for this PR.)

nitpick (non-blocking): i18n.t('ti_connect.error_messages.missing_connector_context') is invoked three times in this function

Extracting to a local const errorMessage would reduce duplication and make the translation key easier to update. Minor — feel free to ignore.

---

3. src/modules/de/kbv-ti-connect/services/types.ts

issue (non-blocking): UserEditableTiConnectSetting still contains an obsolete exclusion

export type UserEditableTiConnectSetting =
  | Exclude<
      keyof TiConnectSettings,
      | 'cert'
      | 'privateKey'
      | 'keyStorePath'
      | 'eventSubscriptions'   // <-- not a key of TiConnectSettings
      | 'serviceEndpoints'
      | 'vsd'
    >
  | 'modeOnlineCheck'

Answer to your Q (section 3): no, the exclusion list is not fully clean after this PR. 'eventSubscriptions' is not a member of TiConnectSettings (verified against the current interface on the branch — it only has serviceEndpoints and vsd as optional grouped fields). Exclude<keyof T, 'nonMember'> silently evaluates to keyof T for that entry, so this is a no-op — dead code that misleads readers into thinking the field exists.

Since this PR is specifically about removing dead TiConnectSettings-related flags, this is a natural fit. Recommendation: drop the 'eventSubscriptions' line in the same PR.

export type UserEditableTiConnectSetting =
  | Exclude<
      keyof TiConnectSettings,
      'cert' | 'privateKey' | 'keyStorePath' | 'serviceEndpoints' | 'vsd'
    >
  | 'modeOnlineCheck'

(I also collapsed the exclusion onto a single li
…(truncated, 1478 more chars)