Skip to main content

Broad refactoring plan

This document covers refactor opportunities across the plugin codebase outside of plugin/src/lib/api/ (which is tracked separately in refactoring.md). The focus is DRY, single responsibility, separation of concerns, and small methods.

Approach: TDD throughout. Every item requires a failing test that demonstrates the problem (or the desired post-refactor behaviour) before any code changes are made. If something is broken today, the failing test comes first.

Items​

1. Extract field-traversal skeleton from buildPayloadUpdateObject, buildCrowdinJsonObject, buildCrowdinHtmlObject, and restoreOrder​

File: plugin/src/lib/utilities/index.ts:414–745

Problem: All four functions contain nearly-identical group / array / blocks recursive dispatch:

if (field.type === 'group') {
  // recurse into field.fields
} else if (field.type === 'array') {
  // map over items, recurse into field.fields
} else if (field.type === 'blocks') {
  // map over items, look up block by slug, recurse
} else {
  // leaf
}

The traversal skeleton — including the block-slug lookup and the filter(isEmpty) cleanup — is copy-pasted across ~250 lines. The only variation is what each function does at the leaf and container level. Any bug in the traversal logic (e.g. a new field type) must be fixed in four places. There is even a TODO comment in the source acknowledging this ("block, array and group logic is duplicated").

Fix: Extract a single traverseFields utility that accepts a visitor object:

interface FieldTraversalVisitor<T> {
  group(field: GroupField, recurse: () => T): T;
  array(field: ArrayField, items: unknown[], recurse: (item: unknown, subFields: Field[]) => T): T;
  blocks(field: BlocksField, items: unknown[], recurse: (item: unknown, subFields: Field[]) => T): T;
  leaf(field: Field, value: unknown): T;
}

buildCrowdinJsonObject, buildCrowdinHtmlObject, buildPayloadUpdateObject, and restoreOrder become thin visitors passed to traverseFields.

TDD: Write tests for traverseFields directly before extracting. Existing tests for the four builder functions become the regression suite.

2. Consolidate identical type guard bodies in types.ts and utilities/payload.ts ✅​

Files: plugin/src/lib/types.ts:73–83, plugin/src/lib/utilities/payload.ts:3–7

Problem: Three exported functions share an identical body:

// types.ts
export const isCrowdinArticleDirectory = (val): val is CrowdinArticleDirectory =>
  val !== undefined && val !== null && typeof val !== 'string';

export const isCrowdinCollectionDirectory = (val): val is CrowdinCollectionDirectory =>
  val !== undefined && val !== null && typeof val !== 'string';

// utilities/payload.ts
export const isNotString = <T>(val): val is T =>
  val !== undefined && val !== null && typeof val !== 'string';

isNotString is a generic version of the same check. The two typed guards in types.ts could delegate to it, or all three could delegate to a single shared primitive.

Fix: Make isCrowdinArticleDirectory and isCrowdinCollectionDirectory delegate to isNotString:

export const isCrowdinArticleDirectory = (val): val is CrowdinArticleDirectory =>
  isNotString(val);

TDD: Unit tests already exist for the type guards. Add a test asserting the delegation relationship to prevent future divergence.

3. Replace duplicated hook logic in syncTranslations / syncAllTranslations ✅​

File: plugin/src/lib/fields/pluginFields.ts:156–327 (now plugin/src/lib/fields/syncTranslationHooks.ts)

Problem: The two checkbox fields syncTranslations and syncAllTranslations have structurally identical beforeChange and afterChange hooks (~70 lines each, ~140 lines total). Both hooks:

  1. Guard on context.triggerAfterChange === false
  2. Extract draft and articleDirectoryId from siblingData
  3. Store state in req.context
  4. In afterChange, type-check context then call either payload.jobs.queue or updatePayloadTranslation

The only differences are: syncTranslations computes excludeLocales (limiting to the current locale); syncAllTranslations enqueues one job per locale.

Fix: Extract createSyncBeforeChangeHook and createSyncAfterChangeHook factory functions parameterised by the sync mode:

type SyncMode = 'current-locale' | 'all-locales';

const createSyncBeforeChangeHook = (mode: SyncMode, fieldName: string) => ...
const createSyncAfterChangeHook = (mode: SyncMode, pluginOptions: PluginOptions) => ...

TDD: Add unit tests for each hook factory (both modes) before extracting. The existing pluginFields.afterRead.spec.ts covers the afterRead hook; the beforeChange/afterChange hooks currently have no unit tests.

4. Remove unused initFunctions in plugin.ts ✅​

File: plugin/src/lib/plugin.ts:68 and plugin/src/lib/plugin.ts:331

Problem: initFunctions is declared as (() => void)[], nothing is ever pushed into it, and the forEach call at line 331 is a no-op. It's dead code that misleads readers into thinking initialisation side-effects are registered there.

Fix: Delete both the declaration and the forEach call.

TDD: The existing plugin.spec.ts unit tests provide regression coverage. Verify they still pass after removal.

5. Extract documentTabFields builder from plugin.ts ✅​

File: plugin/src/lib/plugin.ts:96–118

Problem: The inline documentTabFields construction uses any[] and lives inside the plugin closure where it is hard to test independently. The logic that decides which fields to register — based on syncedCollectionSlugs and syncedGlobalSlugs — is already tested in plugin.spec.ts but only via the full plugin output, not the field builder itself.

Fix: Extract to plugin/src/lib/fields/documentTabFields.ts:

export function buildDocumentTabFields(
  syncedCollectionSlugs: string[],
  syncedGlobalSlugs: string[],
): Field[]

Type the return as Field[] (not any[]).

TDD: Write focused unit tests for buildDocumentTabFields directly. The existing plugin.spec.ts tests become higher-level regression coverage.

6. Split getLocalizedFields (119 lines, five concerns) ✅​

File: plugin/src/lib/utilities/index.ts:93–212 (line numbers before the refactor)

Problem: getLocalizedFields handles five distinct concerns in a single 119-line function:

  1. Filtering fields by localization status
  2. Filtering fields by type (json / html)
  3. Recursing into nested fields (group, array, blocks)
  4. Flattening container fields (tabs via convertTabs, collapsible via getCollapsibleLocalizedFields, row via getRowLocalizedFields)
  5. Applying a caller-supplied isLocalized predicate

A comment in the source already flags this: "find a better way to do this - block, array and group logic is duplicated".

Fix: Extract named private helpers, keeping getLocalizedFields as a thin orchestrator:

  • filterByType(fields, type) — json / html / undefined filter
  • flattenContainerFields(fields) — tabs / collapsible / row flattening
  • recurseNestedFields(field, options) — group / array / blocks recursion

TDD: Extensive tests already exist in utilities/getLocalizedFields.spec.ts. Add tests for each extracted helper before splitting.

Completed work​

Pass 1 — items 2, 4, 5

  • Item 2: isNotString is replaced by isPopulatedRelationship, which checks for an object instead of "not a string". The old name described one of three things it ruled out, and it misreported numeric ids (unpopulated relationships on SQL adapters) as populated documents. isCrowdinArticleDirectory and isCrowdinCollectionDirectory delegate to it. There were no existing type guard tests; types.spec.ts and utilities/payload.spec.ts now cover the guards, including numeric ids.
  • Item 4: initFunctions removed. onInit still forwards to the host config's onInit, now covered in plugin.spec.ts.
  • Item 5: buildDocumentTabFields lives in fields/documentTabFields.ts, returns Field[], and has focused tests in documentTabFields.spec.ts.

Pass 2 — item 3

  • Item 3: createSyncBeforeChangeHook(mode, pluginOptions) and createSyncAfterChangeHook(mode, pluginOptions) live in fields/syncTranslationHooks.ts, alongside syncFieldNames, which maps each SyncMode to its checkbox name. The before hook takes pluginOptions rather than a field name because the current-locale mode needs localeMap; the field name comes from syncFieldNames. Baseline tests in pluginFields.syncHooks.spec.ts were committed against the old hooks first and pass unchanged against the factories. They cover both modes, inline and job-queue loading, and the triggerAfterChange === false guard.
  • Known limitation, kept as-is: the after hook only syncs when articleDirectoryId is a string, so numeric ids (SQL adapters) never trigger a sync. Fixing it is a behaviour change and belongs in its own PR.

Pass 3 — item 6

  • Item 6: getLocalizedFields is now a short orchestrator over three private helpers in utilities/index.ts: filterByType, recurseNestedFields and flattenContainerFields. Layout fields (tabs, collapsible, row) are excluded up front instead of being carried through every step and filtered out at the end. Group, array and blocks fields are now reduced once and dropped if empty; previously each was traversed twice, once by containsLocalizedFields to decide whether to keep it and again to build the result, and that repeated at every nesting level. The old TODO about duplicated block, array and group logic is resolved.
  • Tests: the helpers are private, so instead of testing them directly, getLocalizedFields.structure.spec.ts was committed first as a baseline against the old implementation. It pins the output shape that the existing tests only covered indirectly: top-level fields before tab, collapsible and row contents; named tabs as groups; blocks reduced to slug and fields; empty containers dropped under a type filter; and custom isLocalized predicates.
  • Known quirk, kept as-is: convertTabs is called without the caller's isLocalized, so unnamed tab fields are marked localized: true using the default predicate. The baseline tests pin this.

Remaining: item 1.

Notes on approach​

  • Each item must have a failing (or green-baseline) test committed before any implementation changes.
  • Items 1 and 3 carry the highest risk (core translation pipeline and UI field hooks respectively) — take extra care with regression coverage before touching them.
  • Items 4 and 5 are safe to tackle first: item 4 is pure deletion, item 5 is a pure extraction with no behaviour change.