OpenConceptLab/ocl_issues#2773 | Mapper preview - #60
snyaggarwal wants to merge 7 commits into
Conversation
…remaining should not clear existing candidates
…rows in datagrid | automatch dialog is to only consider quota rows
paynejd
left a comment
paynejd
left a comment
There was a problem hiding this comment.
Reviewed against ocl_issues#2773, #2762 and #2781, and cross-checked the contract against the backend branch.
⚠️ Dependency: this PR cannot merge before OpenConceptLab/oclapi2#900
oclapi2#900 (mapper-preview) is a hard dependency of this PR, and it is not mergeable yet — my review there on 2026-09-21 lists four blockers (500 on project re-create after delete, self-serve quota refund via negative units, inverted kill switch, hardcoded pk=16 in the group migration). Every capability name, error code and meter this PR reads is defined there. Merging this first ships a UI that reads fields the API does not yet return.
Two of those backend items are shared contract decisions that have to move in both PRs together — they are not "backend-only":
- What a match operation is. The backend meters
$matchasunits=len(rows); #2762/#2782 specify row–algorithm pairs. This PR's estimate (AutoMatchDialog.jsx#L88) and pre-truncation (MapProject.jsx#L2091-L2094) both multiply byalgorithmCount, i.e. they implement the spec, not the backend. As it stands a 4-algorithm run is estimated at 4x what the server actually charges — the UI will truncate runs the server would have allowed. Whichever definition wins, both sides change. 0 = unlimited. #2782's runbook says "set caps to 0" to kill the feature; in code0means unlimited, so following the runbook uncaps everyone. This PR inherits the same ambiguity in its own form — see theautoMatchRows.jscomment below, where a missing capability is read as unlimited by one helper and as blocked by another.
Please hold this PR until #900's blockers land, then re-point it at the final contract.
The frontend review
The architecture is good: autoMatchRows.js is a clean extraction with real tests, MapperQuotaChip / PreviewLimitDialog are well factored, and all three locales (en/es/zh) are complete and consistent — that last one is genuinely rare and appreciated.
The problems cluster in one place, and it's the same seam as the backend: the UI and the quota disagree about what has been spent and what is left. Four blockers, four should-fixes, all inline.
Blockers
- Bulk matching swallows a preview-limit 403 as a successful empty result — the exact silent failure #2773 asked us to fix.
- A limit error on a plain row click leaves the row spinning forever.
- The quota cache is never refreshed on the ordinary row-click path, which is what makes stale counts (and #1) reachable.
- Existing sessions never see the feature at all —
permissionsis dropped on refresh, sohasCapabilitystays false until re-login. Combined with #900's missing backfill for the 14mapper-approvedusers, deploy day is rough for every current Mapper user.
Not approving yet. Happy to pair on the metering definition — as above, I don't think it should be settled in review comments on two separate PRs.
| const rowsToProcess = getRowsToProcess(rows, rowStatuses, autoMatchScope, selectedRowIndexes) | ||
| const preview = getMapperPreview() | ||
| const previewEligibleRowIndexes = getPreviewEligibleRowIndexes(rows, preview) | ||
| let rowsToProcess = getRowsToProcess(rows, rowStatuses, autoMatchScope, selectedRowIndexes, previewEligibleRowIndexes) |
There was a problem hiding this comment.
Blocker — bulk matching treats a preview-limit 403 as a successful empty match.
This is the bulk entry point, but the fix for #2773 only landed on the per-row path. processBatch (~L1942-L1965) calls service.post(payload, token, headers, query) with raw defaulting to false, and APIService.sendRequest resolves rather than throws in that mode:
// src/services/APIService.js:145
return error.response ? error.response.data : error.message;So a 403 {error_code: 'mapper_match_operations_exceeded', detail: ...} lands in the try, not the catch. The result:
forEach(rowBatch, __row => {
markAlgo(__row.__index, algo.id, 1) // marked SUCCEEDED
log({action: 'algo_finished', ...}, __row.__index) // logged as finished
})
return response.data || []; // error body has no .data -> []Every row in the batch is marked algo_finished with zero candidates, no alert, no PreviewLimitDialog. The user sees a completed run that found nothing — indistinguishable from a genuine no-match, which is precisely the failure mode #2773 exists to eliminate.
processBatch needs the same isPreviewLimitError(response) check the per-row path got, before the success forEach:
const response = await service.post(...);
if(isPreviewLimitError(response)) {
forEach(rowBatch, __row => markAlgo(__row.__index, algo.id, -2))
handlePreviewLimitError(response)
throw new PreviewLimitAbort() // or similar, to stop the remaining queue
}Worth deciding explicitly whether hitting the limit mid-run should abort the remaining queue or let it drain — every subsequent batch will 403 too, so draining means N pointless round trips and N more chances to mismark rows. I'd abort.
| const handlePreviewLimitError = (response, rowId) => { | ||
| if(!isPreviewLimitError(response)) | ||
| return false | ||
| setAlert({message: response?.detail || t('unknown_error'), severity: 'error'}) | ||
| setIsLoadingInDecisionView(false) | ||
| if(isNumber(rowId) && restoreRefreshRowStage(rowId)) | ||
| refreshMapperQuotaCache() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Blocker — a limit error on a plain row click leaves the row spinning forever.
handlePreviewLimitError returns true at L3274, and every caller treats true as "handled, stop here":
if(handlePreviewLimitError(response, __row.__index))
return // L3390 - skips markAlgo(..., -2) on L3394But the only thing that resets the row's stage inside this function is restoreRefreshRowStage(rowId), and that returns false unless a snapshot exists — snapshots are written only by onRefreshClick (L2856). So on the common path (user clicks a row, no Refresh button involved) the row stays at stage 0 — in-flight — with a spinner that never resolves. The alert fires, but the row is stuck until reload.
The same refreshMapperQuotaCache() on L3273 is gated behind that snapshot too, so the row that just burned an operation also doesn't refresh the meter (see my comment on L3455).
Suggested shape — pass the algo through so the stage can always be marked failed:
| const handlePreviewLimitError = (response, rowId) => { | |
| if(!isPreviewLimitError(response)) | |
| return false | |
| setAlert({message: response?.detail || t('unknown_error'), severity: 'error'}) | |
| setIsLoadingInDecisionView(false) | |
| if(isNumber(rowId) && restoreRefreshRowStage(rowId)) | |
| refreshMapperQuotaCache() | |
| return true | |
| } | |
| const handlePreviewLimitError = (response, rowId, algoId) => { | |
| if(!isPreviewLimitError(response)) | |
| return false | |
| setAlert({message: response?.detail || t('unknown_error'), severity: 'error'}) | |
| setIsLoadingInDecisionView(false) | |
| if(isNumber(rowId)) { | |
| if(!restoreRefreshRowStage(rowId) && algoId) | |
| markAlgo(rowId, algoId, -2) | |
| refreshMapperQuotaCache() | |
| } | |
| return true | |
| } |
That needs the three call sites updated to pass the id — L3390 (algoId), L3568 ('ocl-scispacy-loinc'), L4008 (bridgeAlgoId) — all three have it to hand. Applying the suggestion alone is safe (it just no-ops the markAlgo), so please do both.
| if(clearRefreshRowStageSnapshot(__row.__index)) | ||
| refreshMapperQuotaCache() |
There was a problem hiding this comment.
Blocker — the quota cache is never refreshed on the ordinary row-click path.
This is the success path of per-row matching, and the refresh is gated on clearRefreshRowStageSnapshot(__row.__index). That returns true only when a snapshot exists, and snapshots are written in exactly one place:
// L2855-L2859 - onRefreshClick, and nowhere else
const onRefreshClick = () => {
refreshRowStageSnapshotRef.current[rowIndex] = ...So the meter refreshes after the Refresh button, and never after a normal row click — even though both consume the same match operations. A user working down the grid burns their quota against a MapperQuotaChip that keeps showing the count from page load, until something else happens to refresh it.
That stale read is also what makes the other two findings reachable: getMapperPreview() at L2087 sizes the bulk pre-truncation off the same cache, so the truncation is computed from a quota that may be long spent — and then the server 403s mid-run into the swallow-bug above.
The snapshot bookkeeping and the meter refresh are independent concerns; they shouldn't share a condition:
| if(clearRefreshRowStageSnapshot(__row.__index)) | |
| refreshMapperQuotaCache() | |
| clearRefreshRowStageSnapshot(__row.__index) | |
| refreshMapperQuotaCache() |
Same gating at L3392, L3570 and L4012 — all four want the same treatment.
If the concern is request volume, debounce refreshMapperQuotaCache rather than skipping it; a stale quota is worse than an extra GET /user/.
| localStorage.setItem('user', JSON.stringify({ | ||
| ...currentUser, | ||
| capabilities: response.data?.capabilities || [] | ||
| })); |
There was a problem hiding this comment.
Blocker — existing sessions never gain permissions, so nobody with a live session sees the feature.
This merge keeps capabilities and drops permissions. But every gate in the app reads permissions, not capabilities:
// utils.js:1140
export const hasCapability = (user, capability) =>
Boolean((user || getCurrentUser())?.permissions?.includes(capability))getMapperPreview() uses it for hasAccess, hasAIAssistant, hasCustomAlgorithms and hasOrgProjects (L1153-L1156), and MapProject.jsx:527 gates the AI Assistant UI on it. So for any user whose cached user object predates this release — i.e. everyone with a live session on deploy day — permissions stays undefined forever, hasCapability returns false, and the AI Assistant and quota chip simply don't appear. Calling this function makes it look fixed, which is worse than not calling it.
The backend already returns both under the same query param (core/users/serializers.py:219-221 pops permissions and capabilities together on includeCapabilities), so this is a one-line merge:
| localStorage.setItem('user', JSON.stringify({ | |
| ...currentUser, | |
| capabilities: response.data?.capabilities || [] | |
| })); | |
| localStorage.setItem('user', JSON.stringify({ | |
| ...currentUser, | |
| capabilities: response.data?.capabilities || [], | |
| permissions: response.data?.permissions || currentUser.permissions || [] | |
| })); |
Note this interacts with oclapi2#900: there is no backfill for the 14 existing mapper-approved users, so on deploy they lose the underlying permission and the client can't re-read it. Both halves need to be right or current Mapper users are locked out.
| if(rowsPerProject.unlimited || rowsPerProject.limit === null || rowsPerProject.limit === undefined) | ||
| return null |
There was a problem hiding this comment.
A missing capability is read as unlimited here and as blocked everywhere else.
limit === null || limit === undefined returning null means "no restriction, every row eligible". But toMeter in utils.js reads the identical state as zero:
// utils.js:1147-1150
const limit = entry.limit === undefined ? null : entry.limit
const unlimited = limit === 0 // null is NOT unlimited
return {limit, used, unlimited, remaining: unlimited ? null : Math.max((limit || 0) - used, 0)}
// null -> 0 - 0 -> 0 remainingSo when a capability is absent from the payload, AutoMatchDialog computes rowsRemaining = 0 → isPreviewQuotaExhausted → "No more rows are left in your preview quota" with Submit disabled, while this helper returns null and the grid happily enables every row for individual matching. Two surfaces, opposite conclusions, same data.
The backend docstring is explicit that None means blocked, not unlimited — 0 is the unlimited sentinel. Only unlimited should open the gate:
| if(rowsPerProject.unlimited || rowsPerProject.limit === null || rowsPerProject.limit === undefined) | |
| return null | |
| if(rowsPerProject.unlimited) | |
| return null | |
| if(rowsPerProject.limit === null || rowsPerProject.limit === undefined) | |
| return [] |
Worth adding a case to __tests__/autoMatchRows.test.js pinning it — the current suite covers unlimited: true and a numeric limit, but not the missing-capability shape, which is the one that will actually show up if a capability name is ever misspelled or a group is misconfigured.
(This is the client-side face of the 0 = unlimited question on oclapi2#900 — if that sentinel changes there, this changes too.)
| const algorithmCount = Math.max(algosSelected.length, 1) | ||
| const estimatedOperations = rowsToMatchCount * algorithmCount | ||
| const operationsRemaining = preview.matchOperations.unlimited ? null : preview.matchOperations.remaining | ||
| const rowsRemaining = preview.rowsPerProject.unlimited ? null : preview.rowsPerProject.remaining | ||
| const rowsCapByOperations = operationsRemaining === null ? null : Math.floor(operationsRemaining / algorithmCount) | ||
| const effectiveRowCap = [rowsRemaining, rowsCapByOperations].filter(n => n !== null).reduce( | ||
| (min, n) => min === null ? n : Math.min(min, n), null | ||
| ) | ||
| const willTruncate = effectiveRowCap !== null && rowsToMatchCount > effectiveRowCap | ||
| const isPreviewQuotaExhausted = willTruncate && effectiveRowCap <= 0 |
There was a problem hiding this comment.
An AI-only run is costed against the match-operations meter, and blocked by it.
algorithmCount is derived from algosSelected unconditionally, but the dialog has an algos toggle (L54, "retrieve candidates") that the user can switch off to run AI analysis alone. With algos === false the run issues no $match calls and consumes no match operations — yet:
estimatedOperationsstill readsrows x algorithmCount,effectiveRowCapis still capped byrowsCapByOperations,- so with 0 operations left,
isPreviewQuotaExhaustedistrueand Submit is disabled (L153-L158) for a run that would have cost nothing.
MapProject.jsx:2091-2102 has the same bug on the execution side — it truncates rowsToProcess to 0 and runs nothing.
| const algorithmCount = Math.max(algosSelected.length, 1) | |
| const estimatedOperations = rowsToMatchCount * algorithmCount | |
| const operationsRemaining = preview.matchOperations.unlimited ? null : preview.matchOperations.remaining | |
| const rowsRemaining = preview.rowsPerProject.unlimited ? null : preview.rowsPerProject.remaining | |
| const rowsCapByOperations = operationsRemaining === null ? null : Math.floor(operationsRemaining / algorithmCount) | |
| const effectiveRowCap = [rowsRemaining, rowsCapByOperations].filter(n => n !== null).reduce( | |
| (min, n) => min === null ? n : Math.min(min, n), null | |
| ) | |
| const willTruncate = effectiveRowCap !== null && rowsToMatchCount > effectiveRowCap | |
| const isPreviewQuotaExhausted = willTruncate && effectiveRowCap <= 0 | |
| const algorithmCount = algos ? Math.max(algosSelected.length, 1) : 0 | |
| const estimatedOperations = rowsToMatchCount * algorithmCount | |
| const operationsRemaining = preview.matchOperations.unlimited ? null : preview.matchOperations.remaining | |
| const rowsRemaining = preview.rowsPerProject.unlimited ? null : preview.rowsPerProject.remaining | |
| const rowsCapByOperations = (operationsRemaining === null || algorithmCount === 0) ? | |
| null : Math.floor(operationsRemaining / algorithmCount) | |
| const effectiveRowCap = [rowsRemaining, rowsCapByOperations].filter(n => n !== null).reduce( | |
| (min, n) => min === null ? n : Math.min(min, n), null | |
| ) | |
| const willTruncate = effectiveRowCap !== null && rowsToMatchCount > effectiveRowCap | |
| const isPreviewQuotaExhausted = willTruncate && effectiveRowCap <= 0 |
The algorithmCount === 0 guard matters — without it the division yields Infinity and the cap silently disappears.
Two open questions this exposes, both of which need the oclapi2#900 answer:
- Does an AI-only run still consume
mapper.rows_per_project? The suggestion above keepsrowsRemainingin the cap, which is the conservative reading. If AI analysis only metersai_assistant.calls, the row cap shouldn't apply either. ai_assistant.callsisn't checked here at all.getMapperPreview()exposes it, but nothing in this dialog gates on it, so an AI-only run with an exhausted AI quota submits and fails server-side. Follow-up slice, or should it be in this one?
| t('map_project.preview_estimate_note', { | ||
| used: estimatedOperations.toLocaleString(), | ||
| remaining: operationsRemaining !== null ? operationsRemaining.toLocaleString() : rowsRemaining.toLocaleString() | ||
| }) |
There was a problem hiding this comment.
Wrong unit in the estimate sentence when operations are unlimited.
The fallback substitutes rowsRemaining into preview_estimate_note, whose text is:
"This run will use {{used}} of your {{remaining}} remaining match operations."
When operationsRemaining === null (operations unlimited) but rowsRemaining is finite, the sentence reports a row count labelled as operations — and pairs it with used: estimatedOperations, which is an operation count. So the two halves of one sentence are in different units, and on a multi-algorithm run the "used" number can exceed the "remaining" number while nothing is actually wrong.
| t('map_project.preview_estimate_note', { | |
| used: estimatedOperations.toLocaleString(), | |
| remaining: operationsRemaining !== null ? operationsRemaining.toLocaleString() : rowsRemaining.toLocaleString() | |
| }) | |
| operationsRemaining !== null ? | |
| t('map_project.preview_estimate_note', { | |
| used: estimatedOperations.toLocaleString(), | |
| remaining: operationsRemaining.toLocaleString() | |
| }) : | |
| t('map_project.preview_estimate_rows_note', { | |
| used: rowsToMatchCount.toLocaleString(), | |
| remaining: rowsRemaining.toLocaleString() | |
| }) |
This needs a new key in all three locales alongside preview_estimate_note (en/es/zh translations.json) — suggested en copy:
"preview_estimate_rows_note": "This run will use {{used}} of your {{remaining}} remaining preview rows.",| APIService.new().overrideURL(response.data.url).appendToUrl('logs/').post({logs: {row_logs: rowLogsForSave, project_logs: savedProjectLogs}}).then(() => {}) | ||
| } else if(status === 403 && errorData?.error_code) { | ||
| setPreviewLimit({errorCode: errorData.error_code, limit: errorData.limit, used: errorData.used}) | ||
| } else { |
There was a problem hiding this comment.
This catch-all now toasts on throttling and on logout.
Switching the save to raw=true means handleAPIError hands back the raw error instead of a pending request, so responses that were previously absorbed by APIService now fall through to this else:
- 429 —
handleAPIErrorcallsnotifyThrottlingListeners(error.response)and returns{handled: true, result: raw ? error : createPendingRequest()}(APIService.js:72-74). The throttle UI already tells the user what happened; this adds a second, less informative error toast on top. - 401 — the 401 branch triggers logout/redirect; an 8-second error toast rides along with the redirect.
Both are worse on autosave, where the user didn't initiate anything and gets an unexplained error toast mid-typing.
| } else { | |
| } else if(status !== 429 && status !== 401) { |
Separately: should isAutoSave suppress this toast entirely? An autosave failure is worth surfacing somehow, but an 8s error alert during editing is heavy — the !isAutoSave guard on the success path (L1550) suggests the intent was for autosave to stay quiet in both directions.
paynejd
left a comment
paynejd
left a comment
There was a problem hiding this comment.
Cross-PR alignment pass — #60 / oclapi2#900 / ocl-ai-assistant#195
Follow-up to my review above, after reading all three mapper-preview PRs together. One correction to the dependency statement, and one new finding.
Correction: the dependency is three-way, not two-way
Deploy sequencing — all three must land together, in this order. My original review here named only oclapi2#900; the full chain is:
- oclapi2#900 + the ocl_issues#2781 backfill — defines the capabilities, permissions, error codes and
/capabilities/consume/. - ocl-ai-assistant#195 — enforces
ai_assistant.callsand readspermissions/capabilitiesfrom oclapi2. Against an oclapi2 that doesn't serve them it fails closed: total ai-assistant outage. - oclmap#60 (this PR) — reads the meters and error codes both of the above produce.
Merging any of these independently produces a broken environment, not a partially-working one.
Concretely for this PR: ocl-ai-assistant#195 is what makes ai_assistant_calls_limit_reached real. MapProject.jsx:4886 and :4913 already branch on that code, but today the meter behind it never increments — #195's CapabilitiesService.consume raises AttributeError on every call (self._headers vs _get_headers) and a catch-all except Exception swallows it. So this PR's AI-quota handling is dead code until #195 lands with that fixed. Worth knowing when testing: an AI quota that never appears to exhaust is currently a backend bug, not a bug in this diff.
New finding: the error-code registry is incomplete
CAP_COPY_KEY maps 5 codes; oclapi2#900 can emit more than that, and ai-assistant emits none at all. Detail inline on PreviewLimitDialog.jsx.
Still open, and now confirmed as cross-repo decisions
- Metering unit — I've confirmed the direction: oclapi2 charges
units=len(rows), this PR estimatesrows x algorithmCount. This PR matches #2762/#2782; the backend doesn't. Raised on #900 too. 0 = unlimited/None = blocked— three implementations (toMeterhere,get_capability_limitin oclapi2,HasAIAssistantCapacityin ai-assistant). MyautoMatchRows.jscomment above stands: this PR hasNonebackwards in that one helper.limit: nullmis-messaging — raised on #900, where the root cause is. It surfaces here, inPreviewLimitDialog's_no_countcopy: an unentitled user is shown "Your free preview's match-operation limit has been reached". Once #900 emits a distinct not-entitled code, this dialog needs the matching entry and copy.
| const CAP_COPY_KEY = { | ||
| mapper_rows_per_project_limit_reached: 'rows', | ||
| mapper_match_operations_limit_reached: 'match_operations', | ||
| mapper_projects_limit_reached: 'projects', | ||
| mapper_org_projects_denied: 'org_projects', | ||
| mapper_access_denied: 'access', | ||
| } |
There was a problem hiding this comment.
This registry is missing codes the backends can actually emit, so they fall back to generic copy.
oclapi2#900 defines seven mapper-facing codes. This map has five:
error_code |
source | mapped here |
|---|---|---|
mapper_projects_limit_reached |
capabilities/constants.py:20 |
✅ |
mapper_rows_per_project_limit_reached |
:21 |
✅ |
mapper_match_operations_limit_reached |
:22 |
✅ |
mapper_access_denied |
common/permissions.py:115 |
✅ |
mapper_org_projects_denied |
:171 |
✅ |
mapper_custom_algorithms_denied |
:144 |
❌ → generic |
mapper_ai_assistant_denied |
:132 |
❌ → generic |
capability_limit_reached (fallback) |
capabilities/views.py:59 |
❌ → generic |
mapper_custom_algorithms_denied is reachable from this PR's own save path: CanUseCustomMapperAlgorithms guards map-project create/update (map_projects/views.py:35-39), fires whenever algorithms contains a type: 'custom' entry, and a 403 from onSave lands at MapProject.jsx:1554-1555 → setPreviewLimit → this dialog. A preview user who adds a custom algorithm and saves currently gets:
You've reached a preview limit
You've reached a preview limit for the Mapper.
instead of the backend's actual, much more useful message — "Externally hosted algorithms are not available in preview." No crash (the generic keys exist), just the wrong copy on a path that will be hit, since custom algorithms are exactly what preview users don't have.
| const CAP_COPY_KEY = { | |
| mapper_rows_per_project_limit_reached: 'rows', | |
| mapper_match_operations_limit_reached: 'match_operations', | |
| mapper_projects_limit_reached: 'projects', | |
| mapper_org_projects_denied: 'org_projects', | |
| mapper_access_denied: 'access', | |
| } | |
| const CAP_COPY_KEY = { | |
| mapper_rows_per_project_limit_reached: 'rows', | |
| mapper_match_operations_limit_reached: 'match_operations', | |
| mapper_projects_limit_reached: 'projects', | |
| mapper_org_projects_denied: 'org_projects', | |
| mapper_custom_algorithms_denied: 'custom_algorithms', | |
| mapper_ai_assistant_denied: 'ai_assistant', | |
| mapper_access_denied: 'access', | |
| } |
That needs four new keys per locale (en/es/zh), mirroring the backend's denied_message wording:
"preview_limit_title_custom_algorithms": "Externally hosted algorithms aren't in the preview",
"preview_limit_body_custom_algorithms_no_count": "Externally hosted algorithms aren't part of the preview yet. Use OCL's built-in algorithms, or ask about more access below.",
"preview_limit_title_ai_assistant": "The AI Assistant isn't available yet",
"preview_limit_body_ai_assistant_no_count": "The AI Assistant isn't available for your account. Ask about more access below and we'll sort it out.",Two related items, both tracked on the other PRs:
mapper_ai_assistant_deniedis defined in oclapi2 but wired to no view, and ocl-ai-assistant raises a bareHttp403()with noerror_codefor that same denial — so today an unentitled AI user reachest('unknown_error'), not this dialog. Raised on oclapi2#900 and ocl-ai-assistant#195; mapping it here is the third piece.- A not-entitled code is still needed. Once #900 stops reporting
limit: nullas "limit reached", the new code wants an entry here too — otherwise the_no_countcopy keeps telling unentitled users they spent an allowance they never had.
2 participants
Linked Issue
Refs:
OpenConceptLab/ocl_issues#2773
OpenConceptLab/ocl_issues#2781