packages/shared/src/common/__tests__/retry.test.ts (2)

167-187: Order-sensitive verification suggestion.

Consider adding a test to ensure onError executes before shouldRetry (this is part of the documented contract), which would guard against future regressions.

Apply this diff to add the test:

@@
     describe("onError", () => {
+      it("calls onError before shouldRetry", async () => {
+        const order: string[] = [];
+        const onError = jest.fn(() => order.push("onError"));
+        const shouldRetry = jest.fn((_r: unknown, _e: unknown) => {
+          order.push("shouldRetry");
+          return false;
+        });
+        const error = new Error("test error");
+        fn.mockImplementation(() => {
+          throw error;
+        });
+        await expect(async () =>
+          executeWithRetries(fn, {
+            initialDelay: 1,
+            maxAttempts: 1,
+            onError,
+            shouldRetry,
+          })
+        ).rejects.toThrow();
+        expect(order).toEqual(["onError", "shouldRetry"]);
+      });

---

119-188: Add resilience test if we decide to make onError non-fatal.

If we adopt the production change to isolate onError exceptions (see comment in retry.ts), add this test to prevent regressions. It will fail until the production change is made.

Apply this diff to add the test:

@@
     describe("onError", () => {
+      it("still retries and surfaces the original error even if onError throws", async () => {
+        const original = new Error("original");
+        const onError = jest.fn(() => {
+          throw new Error("onError failed");
+        });
+        fn.mockImplementation(() => {
+          throw original;
+        });
+        await expect(async () =>
+          executeWithRetries(fn, {
+            initialDelay: 1,
+            maxAttempts: 2,
+            onError,
+          })
+        ).rejects.toThrow(original);
+        expect(onError).toHaveBeenCalledTimes(2);
+      });

packages/shared/src/common/retry.ts (3)

42-47: Clarify the onError contract; ensure it cannot affect outcomes.

The comment promises that onError “doesn't change” outcomes. To uphold this contract even if a caller’s callback throws, either: (a) explicitly document that onError must not throw, or (b) make the implementation resilient (preferred; see next comment). If sticking with documentation-only, tighten the wording.

Apply this diff to clarify the doc:

   /**
-   * Function to be called when an error occurs. It doesn't change whether shouldRetry is called
-   * or not, nor does it change the result of that function.
-   * It's called before shouldRetry.
+   * Callback invoked on each caught error, before `shouldRetry`.
+   * It must be side-effect-only; its own failure MUST NOT influence retry decisions or the final
+   * outcome. The implementation ignores any exception raised by this callback.
    */

---

117-117: Make onError non-fatal to preserve retry semantics if the callback throws.

Right now, if a user-supplied onError throws, it aborts the retry flow and violates the stated contract. Wrap it defensively and log.

Apply this diff:

-      onError?.(error);
+      try {
+        onError?.(error);
+      } catch (onErrorErr) {
+        // Ensure callback failures do not affect retry flow.
+        log(`[${context}] onError handler threw: ${errorToString(onErrorErr)}`);
+      }

---

90-93: Optional: expose attempt to onError for richer diagnostics.

Consider evolving the signature to onError?: (error: unknown, attempt: number) => void. This provides context without requiring consumers to track state externally. Would be a minor breaking change; consider a follow-up with deprecation path.

packages/core/src/command/feature-flags/types.ts (1)

45-45: Nit: align flag name with prevailing “cxsWith…Enabled” pattern

Most CX-scoped flags here read as “cxsWithXEnabled” (e.g., cxsWithEpicEnabled, cxsWithDemoAugEnabled). Consider renaming to improve consistency and grep-ability:

Apply within this file:

-  cxsEnabledForCommonwellV2: ffStringValuesSchema,
+  cxsWithCommonwellV2Enabled: ffStringValuesSchema,

And propagate the same key rename to:

• packages/core/src/command/feature-flags/ffs-on-dynamodb.ts (initialFeatureFlags)
• packages/core/src/command/feature-flags/domain-ffs.ts (string key used by getCxsWithFeatureFlagEnabled)

I can provide a full repo-safe rename plan if you want.

packages/core/src/command/feature-flags/ffs-on-dynamodb.ts (1)

68-69: Nit (only if you adopt the naming tweak in types.ts)

If you choose to rename to cxsWithCommonwellV2Enabled, mirror it here:

-  cxsEnabledForCommonwellV2: { enabled: false, values: [] },
+  cxsWithCommonwellV2Enabled: { enabled: false, values: [] },

packages/core/src/command/feature-flags/domain-ffs.ts (3)

306-309: Use includes for brevity and consistency

Elsewhere in this file (e.g., Lines 115–118) we use includes. Align here for consistency.

 export async function isQuestFeatureFlagEnabledForCx(cxId: string): Promise<boolean> {
   const cxIdsWithQuestEnabled = await getCxsWithQuestFeatureFlag();
-  return cxIdsWithQuestEnabled.some(i => i === cxId);
+  return cxIdsWithQuestEnabled.includes(cxId);
 }

---

315-317: Also prefer includes here

Minor readability/style tweak to match other helpers that already use includes.

 export async function isCommonwellV2EnabledForCx(cxId: string): Promise<boolean> {
   const cxIdsWithCommonwellV2Enabled = await getCxsEnabledForCommonwellV2();
-  return cxIdsWithCommonwellV2Enabled.some(i => i === cxId);
+  return cxIdsWithCommonwellV2Enabled.includes(cxId);
 }

---

311-318: Optional: align helper/key naming with “With…Enabled” convention

If you accept the schema key rename suggestion, consider aligning these names for consistency:

-export async function getCxsEnabledForCommonwellV2(): Promise<string[]> {
-  return getCxsWithFeatureFlagEnabled("cxsEnabledForCommonwellV2");
-}
+export async function getCxsWithCommonwellV2Enabled(): Promise<string[]> {
+  return getCxsWithFeatureFlagEnabled("cxsWithCommonwellV2Enabled");
+}

-export async function isCommonwellV2EnabledForCx(cxId: string): Promise<boolean> {
-  const cxIdsWithCommonwellV2Enabled = await getCxsEnabledForCommonwellV2();
+export async function isCommonwellV2EnabledForCx(cxId: string): Promise<boolean> {
+  const cxIdsWithCommonwellV2Enabled = await getCxsWithCommonwellV2Enabled();
   return cxIdsWithCommonwellV2Enabled.includes(cxId);
 }

I can generate a repo-wide rename script to keep everything in sync if desired.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.

Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T19:59:22.396Z
Learning: The file at `packages/api/src/aws/app-config.ts` contains domain feature flags (not AWS AppConfig specific functionality) and is planned to be moved to the domain folder in packages/core in a future PR.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from app-config.ts to the domain folder in packages/core.

packages/shared/src/common/__tests__/retry.test.ts (3)

119-134: Good coverage: validates baseline without onError provided.

This confirms the core retry behavior remains unchanged when no callback is supplied. No changes needed.

---

136-153: Strong behavioral check: onError is invoked on every failure.

The assertions correctly verify call count and payload for each attempt. Nice.

---

155-165: Positive-path assertion is appropriate.

Verifies that a successful first attempt avoids onError invocation. Looks good.

packages/shared/src/common/retry.ts (2)

18-21: Sane default for onError.

Defaulting to emptyFunction avoids undefined checks at call sites and keeps behavior predictable when users omit the callback.

---

4-4: emptyFunction export verified

• packages/shared/src/common/general.ts exports emptyFunction (line 4).
• packages/shared/src/index.ts re-exports emptyFunction (line 7).
• No circular import from general.ts back into retry.ts detected.

No further action required.

packages/core/src/command/feature-flags/types.ts (1)

45-46: CW v2 FF added to schema — LGTM

The new entry cxsEnabledForCommonwellV2 correctly uses ffStringValuesSchema and will flow into StringValueFeatureFlags via the merge. Matches the usage introduced in domain helpers.

packages/core/src/command/feature-flags/ffs-on-dynamodb.ts (2)

68-69: Initial FF entry added — default-off with empty values is correct

The addition of cxsEnabledForCommonwellV2: { enabled: false, values: [] } is consistent with other CX-scoped flags and safe for rollout.

---

68-69: Verify feature flag integration & test coverage

• The new flag key cxsEnabledForCommonwellV2 is correctly referenced in:
  • packages/core/src/command/feature-flags/types.ts (schema definition)
  • packages/core/src/command/feature-flags/ffs-on-dynamodb.ts (DynamoDB defaults)
  • packages/core/src/command/feature-flags/domain-ffs.ts (helpers getCxsEnabledForCommonwellV2 & isCommonwellV2EnabledForCx)
• No existing test files mention or exercise these new helper functions. Consider adding unit tests for:
  • getCxsEnabledForCommonwellV2
  • isCommonwellV2EnabledForCx
• Manually verify that the FF admin UI and any export endpoints surface the cxsEnabledForCommonwellV2 key as expected.

packages/core/src/command/feature-flags/domain-ffs.ts (1)

311-318: CW v2 domain helpers — LGTM

Both helpers follow the established pattern (thin wrappers on getCxsWithFeatureFlagEnabled). Types line up with the schema change and the initial FF entry.