refactor(ktl-api): unify bulk batch execution + standardize progress and logging

[CSWinnall]

rename child record APIs to getChildRecords and getAllChildRecords extract shared bulk helpers (_buildBatchContext, _runBatchWorkers, _logBatchSummary, _logBatchFailures) refactor createRecords, updateRecords, and deleteRecords to use bounded worker scheduling with early stop behavior improve processAutomatedBulkOps promise wiring in concurrent PUT path split bulk error codes for clearer triage (KEC_1028 create, KEC_1029 update, KEC_1030 delete, KEC_1031 tags) make bulk edit and copy progress both count up (X of total) for UX consistency

[cortexrd]

PR #587 — refactor(ktl-api): unify bulk batch execution + standardize progress and logging

What it does

• $.ajax → fetch — Core _request transport migrated from jQuery Ajax to native fetch with AbortController timeouts
• Bulk ops refactored — createRecords, updateRecords, deleteRecords now use bounded worker concurrency (_runBatchWorkers) instead of unbounded Promise.all/Promise.allSettled
• API renames — getRecordChildren → getChildRecords, getAllRecordChildren → getAllChildRecords, param connectionFieldKey → connectionSlug
• New features — uploadAsset(), createRecordWithAsset(), auto-upload via autoUploadAssets option on createRecord
• Error code split — Old shared KEC_1027 split into KEC_1028 (create), KEC_1029 (update), KEC_1030 (delete), KEC_1031 (tags)
• UX — Bulk progress now counts up ("X of total") instead of counting down
• Bulk edit safety — Null checks for missing source rows/records with user-facing error popups
• ktl.api exposed — Added api: this.api to the public KTL object

---

Issues I see

1. Breaking rename with no backward-compat shim

getRecordChildren / getAllRecordChildren are renamed to getChildRecords / getAllChildRecords. Any external app code calling the old names will silently fail. I didn't find callers in your repos, but Craig or other KTL users might have code using them. Consider adding deprecated aliases:

getRecordChildren: function (...args) { return apiInstance.getChildRecords(...args); },

2. Stagger delay scales with absolute index — will be very slow on large batches

In _runBatchWorkers, the delay is staggerMs * index where index is the absolute item number (0–99, etc.). With bounded workers, by the time a worker picks up item 60, it has already waited for ~10 previous items to complete. Then it adds 60 * 40 = 2400ms of stagger on top. Item 99 would add ~4 seconds of pure waiting. The old unbounded code had the same formula but all delays ran concurrently from time zero. With bounded workers, this delay accumulates on top of actual processing time, making large batches significantly slower. The fix would be a flat await delay(staggerMs) per iteration instead of staggerMs * index.

3. debugger statements in production code (2 places)

At the new null-check guards for bulk edit source rows (~line 31358, 31373):

if (typeof ktl?.account?.isDeveloper === 'function' && ktl.account.isDeveloper()) {
    debugger;
}

Even gated behind isDeveloper(), this will freeze the browser tab if DevTools is open. Probably fine for debugging but should be removed before release.

4. _buildRequestError receives fetch-shaped objects but reads .responseText

The current _buildRequestError at line 5288 reads jqXHR?.responseText. The new fetch callers pass { status, statusText, responseText, headers } — this works because the property name matches. But if an actual fetch Response object ever gets passed directly (it has .text() method, not .responseText property), it would silently produce body: null. The current call sites are correct, but it's fragile.

5. KEC_1027 still exists on dev in tags code (lines 14253, 14349)

The PR changes these to KEC_1031, but the current dev branch still has 5 places using KEC_1027. The PR correctly splits all of them — just verify the merge won't leave orphaned KEC_1027 references if there have been other changes to those lines.

6. Minor: Double error handling in _request

When a non-retryable HTTP error occurs, it's thrown inside the try block, caught by the outer catch, re-checked, and re-thrown. Functionally correct but unnecessarily redundant — the inner throw and outer catch do the same retryable check twice.

---

What to test

High priority (core transport change):

• Any page that loads data via KTL API views — confirm records still load
• Single record create, update, delete — verify basic CRUD still works
• Trigger a bulk edit operation (paste multiple values) — verify progress shows "X of Y" counting up
• Trigger a bulk copy operation — verify progress shows "X of Y"
• Test with a bad/expired session to verify error handling (should get clean error, not crash)
• If you can trigger a 429 rate limit, verify retry/backoff still works

Bulk operations:

• Bulk update ~20+ records — watch progress and verify all records update
• Bulk delete — same thing
• Bulk create (copy) — verify records are created and progress is accurate
• Test with continueOnError: false (default) — one failure should stop the batch
• Click a bulk edit/copy button before the view fully loads — should get the new "source row not found" popup instead of crashing

New features (if you use them):

• ktl.api.uploadAsset(file) — upload a file and verify it returns an asset ID
• ktl.api.createRecordWithAsset(viewId, fieldId, file, data) — create a record with a file attachment

Renames (if any external code uses the old names):

• Search your Knack app customization code for getRecordChildren or getAllRecordChildren

---

Bottom line: The refactor is solid architecturally — bounded concurrency is a real improvement over unbounded Promise.all. The $.ajax → fetch migration looks correct. The stagger delay issue (#2) is the most concerning functional problem — it won't break anything, but large batches will be noticeably slower than before. I'd flag that to Craig before merging.

[cortexrd]

Does this PR address this issue?
#588
Just asking because I was about to apply a fix and didn't want to create a duplicate.

[cortexrd]

Updated Review — Post-Copilot Fixes (PR #590)

Re-reviewed after the Copilot fix commit (ddd65bb) and subsequent commits were merged into this branch.

Original Issues — Status

#	Issue	Status
1	No backward-compat shim for getRecordChildren / getAllRecordChildren	STILL OPEN
2	Stagger delay accumulation (staggerMs * index)	FIXED — now flat await delay(staggerMs) per iteration
3	debugger statements in production code	FIXED — removed
4	_buildRequestError reads .responseText	IMPROVED — now checks multiple shapes (responseText, body, detects Response-like objects)
5	KEC_1027 split	FIXED — correctly split into KEC_1028–KEC_1031
6	Double error handling in _request	FIXED — fast rethrow for already-structured errors

Issue 1 is the only original concern that remains unresolved. External code using getRecordChildren or getAllRecordChildren will get undefined with no error. Suggest adding deprecated aliases in the facade object.

---

New Issues Found

Medium Priority

A. findRecords filter merge discards caller's match: 'or'

When callers pass { match: 'or', rules: [...] } as opts.filters, the merge logic flattens the rules into a top-level 'and' group:

mergedFilters = { match: 'and', rules: [baseRule, ...opts.filters.rules] };

This turns the caller's OR conditions into AND conditions. Fix: always nest the caller's filter as a single sub-rule instead of spreading:

mergedFilters = { match: 'and', rules: [baseRule, opts.filters] };

(The else branch already does this correctly — just the if branch that spreads is wrong.)

B. Promise.all in parallel page fetch has no error recovery

In getAllRecords and getAllChildRecords, the parallel page batches use Promise.all:

const batchResults = await Promise.all(
    pageNumbers.map(page => this.getRecords(viewId, { ...pageOpts, page }))
);

If any single page fails, the entire accumulated result is lost — records from previously successful batches are discarded. The remaining in-flight fetches in that batch continue running (consuming rate limit) but their results are thrown away. Consider Promise.allSettled for partial recovery, or at minimum a try/catch to return what's been collected so far.

Low Priority

C. uploadAsset bypasses retry/backoff logic

uploadAsset has its own inline fetch call instead of routing through _requestInner. This means no automatic retry on 429/500/502/503/504 and no exponential backoff. Transient failures during file upload will fail immediately.

D. Dead hasResponseLikeShape detection in _buildRequestError

The check typeof jqXHR.text === 'function' can never be true because no call site passes a raw Response object — they all construct plain { status, statusText, responseText, headers } objects. Harmless but misleading dead code.

E. updateRecords per-record shape detection only checks first element

The check typeof recordIds[0] === 'object' && 'id' in recordIds[0] only inspects the first array element. A mixed array like [{id, data}, 'string_id', ...] would be detected as "per-record" mode, then fail on later elements during destructuring.

F. No file size validation in uploadAsset

Any Blob is accepted regardless of size. Knack will reject oversized files, but the error will be opaque. A basic guard (e.g., warn above 50MB) would improve DX.

---

What Looks Good

• All new code is vanilla JS — no new jQuery, no alert()/confirm()
• _inflightGets dedup cleanup is correct (.finally() always removes entries, error and success paths both clean up)
• _runBatchWorkers concurrency model is safe (JS single-threaded, index grab is atomic)
• _fieldsWithKwCache session-lifetime design is reasonable (keywords are static after startup)
• Error code split is clean and well-organized
• Progress UX counting up is a nice improvement

---

Recommended before merge: Fix Issue 1 (backward-compat shim) and Issue A (filter merge). The rest can be addressed post-merge.

[cortexrd]

Correction on Issue A (findRecords filter merge):

Knack's filter API only supports a single match operator — all AND or all OR, no nested groups. Given that constraint, the current behavior of forcing match: 'and' is correct: findRecords is a "find by field value" helper, so the base rule must always apply. Preserving match: 'or' would make the lookup field optional, which defeats the purpose.

Downgrading from medium to not an issue. The combination of findRecords + OR filters simply doesn't make sense given Knack's limitations.

[cortexrd]

Correction on Issue 1 (backward-compat shim for renamed methods):

Dropping this — no external consumers use getRecordChildren / getAllRecordChildren at this time. Craig already updated his code. No shim needed.

Updated summary of remaining items:

• Issue 1 — backward-compat shim — Not needed
• Issue A — findRecords filter merge — Not a bug (Knack limitation)
• Issue B — Promise.all in parallel page fetch — Still the main concern (partial results lost on single page failure)
• Issues C–F — Low priority, can address post-merge