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.
  • SQL numeric ids: getSyncRequest accepts string | number. The before hook stores the id through getRelationshipId, so an unpopulated SQL relationship (42) and a populated { id: 42 } both sync. Job input stringifies the id (type: 'text').

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 (fixed in pass 5): convertTabs was called without the caller's isLocalized, so unnamed tab fields were marked localized: true using the default predicate.

Pass 4 — item 1

  • Item 1: traverseFields and getBlockFields live in utilities/traverseFields.ts. restoreOrder, buildPayloadUpdateObject, buildCrowdinJsonObject and buildCrowdinHtmlObject dispatch through the visitor. Nested walks still call the builders so getLocalizedFields type/locale filtering is unchanged.
  • Tests: traverseFields.spec.ts was committed first (failing). The existing builder specs and the book-demo snapshot are the regression suite.

Pass 5 — convertTabs isLocalized and nested block restore

  • convertTabs: flattenContainerFields passes the caller's isLocalized into convertTabs, so unnamed tab fields are marked localized: true with the same predicate as collapsible and row flattening. Covered by getLocalizedFields.structure.spec.ts and getLocalizedFieldsRelocalized.spec.ts.
  • Nested id / blockType: restoreOrder (used by buildPayloadUpdateObject when a source document is passed) already restores nested blocks. That is pinned in the buildPayloadUpdateObject blocks tests. The unused top-level-only restoreIdAndBlockType helper on the translations API was removed.

Pass 6 — extract Payload document walk

  • Vendor-neutral field walk is the payload-localized-fields package (document/): traverseFields, getLocalizedFields (include predicate, no Crowdin json/html type), restoreOrder, findField, isLocalizedField (Payload localization only). Crowdin exclude rules (custom.crowdinSync.disable / admin description) stay on the plugin utilities wrappers.
  • Crowdin builders (buildCrowdinJsonObject, buildCrowdinHtmlObject, buildPayloadUpdateObject) stay in plugin/src/lib/utilities/ and call payload-localized-fields. getLocalizedFields({ type: 'json' | 'html' }) remains as a Crowdin convenience that maps to include.
  • Tests: document/src/*.spec.ts were committed first (failing). Existing plugin utilities specs are the regression suite.

Pass 7 — extract json/html document slice

  • buildJsonObject, buildHtmlObject, and buildUpdateObject live in payload-localized-fields. JSON leaves are non-richText; HTML leaves are richText keyed by dotted path. Arrays/blocks are keyed by id (and blockType) so round-trip does not depend on order. Locale maps from locale: 'all' are skipped.
  • Crowdin names (buildCrowdinJsonObject, buildCrowdinHtmlObject, buildPayloadUpdateObject) stay on the plugin as wrappers that pass the Crowdin isLocalizedField exclude. Rich-text HTML conversion stays in the plugin.
  • Tests: document/src/documentSlice.spec.ts was committed first (failing). Existing Crowdin builder specs are the regression suite.

Pass 8 — localization adapters

  • SourceWriter / syncDocumentSource and TranslationReader / applyDocumentSlice live in payload-localization-adapters (adapters/). They consume the json/html document slice. Crowdin afterChange and payloadCrowdinSyncTranslationsApi implement the writer/reader. File CRUD and Lexical/Slate HTML conversion stay Crowdin-specific. Phrase and Lokalise plugins in this monorepo will depend on the same package.
  • Tests: adapters/src/syncSource.spec.ts and adapters/src/applySlice.spec.ts were committed first (failing). Existing afterChange and translations specs are the regression suite.

Notes on approach​

  • Each item must have a failing (or green-baseline) test committed before any implementation changes.
  • Item 1 is done. The four builder specs and the book-demo snapshot are the regression suite; traverseFields.spec.ts covers dispatch, skip, unnamed layout fields, and block slug lookup.