Multi text groups

[jlamanskygitt]

Implemented multi-text-groups changes for the theme

[cairocoder01]

@jlamanskygitt It looks like you created a new field type for this component. It should actually be used for the existing link field type and just replace that existing UI.

[cairocoder01]

Please do a search in the project for link-input to find all of the javascript that is run based on that class. We need to test that we can remove that outdated js code without affected functionality.

[github-actions]

Code Review

Thanks for this refactor! Moving the link field rendering into the DT_Components class and using the dt-multi-text-groups web component is the right direction and aligns well with how other field types (e.g., communication_channel, key_select, multi_select) are already handled. The overall structure is clean and consistent with existing patterns.

Bug: Deleted options not filtered

The original code explicitly skipped options marked as deleted:

if ( isset( $option_value['deleted'] ) && $option_value['deleted'] === true ) {
    continue;
}

The new render_link() method's array_map does not replicate this filter, so deleted link types will appear in the groups attribute passed to dt-multi-text-groups. This should be fixed, for example by filtering before mapping:

$filtered_options = array_filter($default_options, function ($value) {
    return \!(isset($value['deleted']) && $value['deleted'] === true);
});
$options_array = array_map(function ($key, $value) {
    return [
        'id'    => (string) $key,
        'label' => $value['label'] ?? $key,
    ];
}, array_keys($filtered_options), $filtered_options);

Missing icon data in options

The original dropdown rendered per-option icons via dt_render_field_icon( $option_value ). The new $options_array only includes id and label. If the dt-multi-text-groups component supports an icon property per group, it should be included here to maintain visual parity with the previous UI:

'icon' => $value['icon'] ?? null,

Minor: array_map closure style

The other render_* methods in DT_Components (e.g., render_key_select, render_multi_select) use the same two-arg array_map pattern for building options arrays with array_keys / values. This is consistent — no change needed.

Summary

Architecture	✅ Correct approach — moves link rendering into DT_Components alongside other field types
Deleted options filter	❌ Missing — will expose deleted link types to users
Icon support	⚠️ Likely regression — per-option icons not passed through
Code consistency	✅ Matches existing patterns in the class

The deleted-options bug should be fixed before merge. The icon question depends on whether dt-multi-text-groups supports per-group icons, but worth verifying.

[cairocoder01]

Functionally, this looks great. Really nice work! Just waiting on the cleanup of the old code.

[github-actions]

Code Review

Thanks for working on migrating the link field to the web components pattern. The approach of using DT_Components::render_link() is consistent with how other field types (communication_channel, connection, etc.) have been migrated. However, there are several issues to address before this is ready to merge.

---

Critical: Missing web component

The new render_link() method renders a <dt-multi-text-groups> component, but this component does not exist anywhere in the codebase or in @disciple.tools/web-components. A search across all JS, PHP, and TS files returns zero matches for dt-multi-text-groups.

This means link fields will not render at all after this PR is merged. Is this component being developed in a separate PR against disciple-tools-web-components? If so, this theme PR should not merge until that dependency is available and the component is included in the build.

---

Bug: Deleted link options not filtered

The original code explicitly skipped options marked as deleted:

if ( isset( $option_value['deleted'] ) && $option_value['deleted'] === true ) {
    continue;
}

The new render_link() passes all options to the component without filtering:

$options_array = array_map(function ( $key, $value ) {
    return [
        'id' => (string) $key,
        'label' => $value['label'] ?? $key
    ];
}, array_keys( $default_options ), $default_options);

Deleted link types will appear as options in the UI unless dt-multi-text-groups handles this filtering internally — if so, it needs to receive the deleted flag in the options array so it can act on it.

---

Bug: Missing null check on default key

$default_options = $fields[$field_key]['default'];

If the field definition does not have a default key this will produce a PHP notice/warning. The safer approach:

$default_options = $fields[$field_key]['default'] ?? [];

---

Orphaned JavaScript code

The PR removes the link cases from merge/new-record logic, but several dead code blocks remain and should be cleaned up in this PR:

shared-functions.js — SHAREDFUNCTIONS.addLink() (around line 942) targets .link-list-* and #link-template-* DOM elements that no longer exist after the PHP rendering changes.

details.js — Event handlers for .add-link-dropdown[data-only-one-option] and .add-link__option (lines ~314-322) reference the old HTML structure.

new-record.js — The fieldType === 'link' branch (lines ~84-100) that shows/hides .add-link-${field} and binds addLink handlers references the old DOM structure.

new-bulk-record.js — Same pattern as new-record.js (lines ~57-62, ~79-87).

merge-post-details.js — is_link_field_value_already_in_primary() (around line 1199) is no longer called anywhere after the case 'link' blocks are removed, but the function definition remains.

---

Orphaned SCSS

The link field styles in dt-assets/scss/_details.scss — .add-link-dropdown, .add-link-dropdown__content, .add-link__option, .link-section, .link-section .section-subheader, etc. — are no longer needed and should be removed.

---

Minor: Icon not forwarded in options array

The original code rendered per-type icons. The new $options_array only includes id and label. If dt-multi-text-groups renders per-type icons, the icon key should be forwarded:

return [
    'id' => (string) $key,
    'label' => $value['label'] ?? $key,
    'icon' => $value['icon'] ?? null,
];

---

Summary

Severity	Issue
Critical	dt-multi-text-groups web component does not exist in the codebase
Bug	Deleted options not filtered from groups array
Bug	Missing ?? [] null check on default key
Cleanup	Orphaned JS: addLink, add-link-dropdown handlers, is_link_field_value_already_in_primary
Cleanup	Orphaned SCSS for link field styles
Minor	icon not forwarded in options array

The architectural direction is good and consistent with the existing component pattern. Once the dt-multi-text-groups component exists and the above issues are resolved, this will be a clean migration.

[cairocoder01]

• When attempting to merge two contacts, if one has no value for a multi-text-group and the second does have a value, when I check the box next to the second, the value does not show up in the third column that shows the updated merged value

Otherwise it seems to be looking good. There are branch conflicts, so make sure you merge the develop branch back into this branch again to get it up-to-date.

[github-actions]

Code Review

Existing issues (missing dt-multi-text-groups component, deleted options filter, null check, orphaned JS/SCSS) were already raised in prior reviews — not repeating those.

---

High

new-record.js:293–332 — Record created but redirect blocked on upload failure, enabling duplicate creation

When a record is created successfully but a file upload fails:

1. uploadError is thrown (line 315)
2. The outer .catch at line 322 re-enables the submit button and shows an error message
3. The page never redirects to the created record — the user sees "Upload failed" on the new-record form with the submit button active again

Clicking submit again creates a second record with the same data. The first record exists (without the file) and is now an orphan the user does not know about.

The fix is to redirect to the created record even when uploads fail, and surface the upload error on the detail page instead of leaving the user on the new-record form.

---

Medium

modular-list.js:1332 — Hardcoded English strings not translatable

The strings 'file' and 'files' in the file_upload column display are user-facing but not passed through the translation system. Per project conventions, all user-facing strings must use the localised JS translations object (e.g. list_settings.translations). These should be added to the backend-localised data passed to the script.

---

modular-list-bulk.js:260–277 — Silent failure gives no user feedback on conditional-removal errors

When window.API.get_post fails for a record during conditional removal (key_select / user_select), the error is caught and swallowed. do_done() always reloads the page regardless. If the fetch fails for some records in the queue, those records are silently skipped with no indication to the user. The comment "user will see error via UI feedback" does not appear to be backed by any actual feedback path.

---

Summary

The file-upload-failure / duplicate-record bug in new-record.js is the main blocker. The other two are medium-severity and should be addressed before merge.

[github-actions]

Code Review

Prior reviews already flagged: missing dt-multi-text-groups component, deleted options not filtered, ?? [] null check, orphaned JS/SCSS. Not repeating those.

---

Medium

merge-post-details.js — Duplicate entries in mergedValue when mergeContact is true

In the new case 'link' select path (introduced by this PR):

if (valIdx < 0 || mergeContact) {
  mergedValue.push(sourceItem);
  if (mergeContact) {
    sourceItem.meta_id = "";
  }
}

The || mergeContact condition means items are always pushed when processing the dupe contact, regardless of whether they are already present (valIdx >= 0). This differs from the communication_channel pattern it mirrors (line 646), which only pushes if (valIdx < 0).

Concretely: if the select event fires more than once for the same checkbox (e.g., rapid clicks, programmatic re-triggers, or a page state issue), the same link entry will be duplicated in mergedValue. Since sourceItem.meta_id is cleared to "" on the first pass, the findIndex on the second pass finds x.meta_id === "" but the condition still pushes due to || mergeContact.

The fix is to drop the || mergeContact and guard the meta_id clearing with a separate check:

if (valIdx < 0) {
  if (mergeContact) {
    sourceItem.meta_id = "";
  }
  mergedValue.push(sourceItem);
}

---

merge-post-details.js — splice(-1, 1) removes wrong element when valIdx === -1

In the new deselect path:

} else {
  if (!is_field_value_still_selected(update_field_id, field_type, sourceItem)) {
    if (mergeContact) {
      mergedValue.splice(valIdx, 1);  // valIdx can be -1
    }
  }
}

If valIdx === -1 (the source item is not found in mergedValue), Array.prototype.splice(-1, 1) silently removes the last element of the array instead of being a no-op. This can happen if the merge contact's link field is deselected when mergedValue doesn't contain the expected item (e.g., on page-state inconsistency or re-render).

Note: the communication_channel case at line 658 has the same unguarded pattern, so this is inherited from the existing code rather than newly introduced. But since this PR is introducing new link field merge logic, it's worth addressing here:

if (mergeContact && valIdx > -1) {
  mergedValue.splice(valIdx, 1);
}

---

Summary

The architecture is correct and consistent with other web component migrations. The two issues above are in the new merge-contact selection logic and can cause data corruption (duplicated or dropped link values) during the merge workflow. Both are straightforward to fix before merge.

[github-actions]

Code Review

Prior reviews already raised: missing dt-multi-text-groups component, deleted options not filtered, ?? [] null check, orphaned JS/SCSS, || mergeContact duplicate push, and splice(-1, 1) when valIdx === -1. Not repeating those.

---

High

merge-post-details.js — mergeContact is always false due to broken URL parsing

The new case 'link' block determines whether the selected checkbox belongs to the merge contact by parsing baseURI:

let dupeID = baseURI.substring(
  baseURI.indexOf('dupeid=') + 'dupeid='.length,
);
let mergeContact = currentID === dupeID;

But the merge page URL is always constructed as (from contacts.js:253):

?dupeid=456&currentid=789

dupeid is the first query param, so substring(indexOf('dupeid=') + 7) captures "456&currentid=789" not "456". The comparison currentID === dupeID compares "456" against "456&currentid=789", which is always false.

With mergeContact always false:

• The select path's || mergeContact bypass never fires — sourceItem.meta_id is never cleared to ''
• The deselect if (mergeContact) { mergedValue.splice(valIdx, 1) } never runs — deselecting the merge contact never removes values from mergedValue
• The delete-flag section (if (!mergeContact)) runs unconditionally, setting delete = true/false on primary contact items regardless of which checkbox was clicked

Fix: use URLSearchParams instead of substring:

let dupeID = new URL(baseURI).searchParams.get('dupeid') ?? '';

---

Medium

merge-post-details.js — is_field_value_still_selected has no case 'link' handler

The new deselect path calls is_field_value_still_selected(update_field_id, field_type, sourceItem) for link fields. But is_field_value_still_selected (line 978) handles communication_channel, connection, multi_select, tags, location, and location_meta — there is no case 'link'. The function returns false for unknown types, so !is_field_value_still_selected(...) is always true for link fields.

Consequence: if the same link value is selected from both contacts and the user deselects one, the value is removed from mergedValue without checking whether it is still selected on the other side.

Add a case 'link' to is_field_value_still_selected matching by meta_id (or value when meta_id is empty), consistent with how communication_channel is handled at line 1059.

---

Summary

The URL parsing bug means the merge-contact/primary-contact split logic never activates — link field merge-checkbox deselection is effectively broken. Both issues should be fixed before merge.

[cairocoder01]

@jlamanskygitt This is almost there. It's merging the values together well. Here are some kinda edge cases I found that we should handle.

• Can't removed values from archived record
  • Selected both values for a link field. Value merged correctly into third column.
  • Removed a value from primary record. Success.
  • Removed value from archived record. Did not get removed. (Do we need temp ID values added to them?)
• Handle newly added values
  • Selected both values for a link field. Value merged correctly into third column.
  • Added new value to first group.
  • Unchecked archived value. Just the archived values removed (as expected).
  • Rechecked archived value. Values added back correctly.
  • Unchecked primary record. Primary values removed along with the new value I just added (not expected)

Other than these, the merge seems to be working correctly and saves the updated value as expected.

[github-actions]

Code Review

Prior reviews already raised: missing dt-multi-text-groups component, deleted options not filtered, ?? [] null check on default key, orphaned JS/SCSS, || mergeContact duplicate push, splice(-1, 1) when valIdx === -1, URL parsing bug causing mergeContact to always be false, and missing case 'link' in is_field_value_still_selected. Not repeating those.

---

Medium

merge-post-details.js — is_present check does not handle empty JSON array

The new case 'link' in the is_present switch:

case 'link': {
  return !window.lodash.isEmpty(
    $(td_field_input).find('dt-multi-text-groups').not('[value=""]'),
  );
}

not('[value=""]') only excludes elements whose value attribute is exactly "". A dt-multi-text-groups element rendered for a contact with no link values (where $post[$field_key] is an empty array) will receive value="[]" from the PHP renderer, not value="". That element passes the not('[value=""]) filter, causing is_present to return true for an empty field — the merge checkbox appears for a field that has no selectable values.

The location_meta case (line 241) shows the correct pattern for a JSON-valued web component:

case 'link': {
  const fieldElement = $(td_field_input).find('dt-multi-text-groups')[0];
  if (!fieldElement) return false;
  const valueStr = fieldElement.getAttribute('value') || '[]';
  try {
    const arr = JSON.parse(valueStr);
    return Array.isArray(arr) && arr.length > 0;
  } catch (e) {
    return false;
  }
}

---

Summary

No new high-severity issues in this iteration. The one medium issue above is straightforward to fix. The blocking items flagged by earlier reviews (missing component, URL parsing, is_field_value_still_selected) still need to be addressed before merge.

[github-actions]

Code Review

Prior reviews already raised: missing dt-multi-text-groups component, deleted options not filtered, ?? [] null check on default key, orphaned JS/SCSS handlers, splice(-1, 1) when valIdx === -1, URL parsing bug causing mergeContact to always be false, missing case 'link' in is_field_value_still_selected, and is_present empty JSON array check. Not repeating those.

---

High

new-bulk-record.js — No save path for link values after old code is removed

The PR removes the .link-input collection block from new-bulk-record.js:

// REMOVED:
$('.link-input').each((index, entry) => { ... });

Unlike new-record.js (which has a generic web component loop at lines 217–251 that collects values from all DT-* elements), new-bulk-record.js's submit handler (line 107) only contains explicit class-based collectors (.select-field, .text-input, .dt_textarea, etc.) with no equivalent DT-* loop. After this PR, bulk record creation will silently omit link field values — they will not be included in the new_post payload sent to the API.

A generic web component loop equivalent to the one in new-record.js must be added to new-bulk-record.js's submit handler.

---

Medium

merge-post-details.js — sourceItem mutation corrupts data on deselect + reselect

In the new case 'link' select handler, when processing a merge contact's checkbox:

if (mergeContact) {
  sourceItem.tempKey = sourceItem.meta_id;  // mutates source object
  sourceItem.meta_id = null;                // mutates source object
  mergedValue.push(sourceItem);
}

sourceItem is a reference into sourceValue (from sourceField.val()). If the web component's value getter returns its internal array by reference (rather than deep copies), mutating sourceItem corrupts the source element's data structure.

Sequence that breaks:

1. User selects merge contact checkbox → sourceItem.meta_id = null, sourceItem.tempKey = originalId
2. User deselects → item removed from mergedValue
3. User re-selects → sourceItem.meta_id is already null, so sourceItem.tempKey = null (original ID is lost)
4. mergedValue now contains { ..., meta_id: null, tempKey: null } — the backend has no way to identify this as a new item vs. an existing one

The fix is to clone before mutating:

if (mergeContact) {
  const newItem = { ...sourceItem, tempKey: sourceItem.meta_id, meta_id: null };
  mergedValue.push(newItem);
}

---

Summary

The new-bulk-record.js save gap is a silent data-loss regression that affects bulk record creation with link fields. The sourceItem mutation is a correctness bug on re-selection in the merge flow. Both should be fixed before merge; the existing blocking items from prior reviews (missing component, URL parsing) also remain unresolved.