Skip to content

feat(bulkProcessing): near limit alert DEV-1399#7232

Open
magicznyleszek wants to merge 17 commits into
mainfrom
leszek/dev-1399-near-limit-alert
Open

feat(bulkProcessing): near limit alert DEV-1399#7232
magicznyleszek wants to merge 17 commits into
mainfrom
leszek/dev-1399-near-limit-alert

Conversation

@magicznyleszek

@magicznyleszek magicznyleszek commented Jul 9, 2026

Copy link
Copy Markdown
Member

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. act on any greptile review below a 5/5 score or leave comment explaining why you won't
  10. delete this checklist section from the final squash commit before merging

📣 Summary

Adds a new Near Limit error alert to bulk transcription and bulk translation modals, shown when a user still has quota left but not enough to process all selected submissions.

💭 Notes

Changes here:

  • Near-limit evaluator is now implemented in the bulk alerts pipeline.
  • Near-limit is treated as an error (blocking), so users cannot continue until they reduce selection or increase quota.
  • Both modals now provide near-limit required amounts into the shared alert hook.
  • Unit coverage was extended for near-limit scenarios (show/hide behavior and computed message values).

👀 Preview steps

  1. ℹ️ Have a project with audio/transcript data.
  2. Go to Project → Data → Table
  3. Select submissions and open either:
    • Bulk transcription modal, or
    • Bulk translation modal.
  4. If your account naturally has low remaining quota, use that directly and continue to step 6.
  5. Optional local-testing shortcut (temporary code tweak to force near-limit):
    • In BulkTranscriptionModal.tsx, temporarily multiply required seconds:
      • set near-limit required value (const nearLimitRequiredSeconds) to something like totalSelectedAudioDuration * 1000.
    • In BulkTranslationModal.tsx, temporarily multiply required characters:
      • set near-limit required value to something like nearLimitRequiredCharacters * 1000.
  6. Reload the page and reopen the modal with selected submissions.
  7. 🔴 [on main] Near-limit alert is not appearing.
  8. 🟢 [on PR] Near-limit error alert appears with remaining amount.

@magicznyleszek magicznyleszek requested a review from duvld July 9, 2026 12:55
@magicznyleszek magicznyleszek self-assigned this Jul 9, 2026
@magicznyleszek

Copy link
Copy Markdown
Member Author

@greptile-apps review please

@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements the previously stubbed evaluateNearLimit alert evaluator, surfacing a blocking error in the bulk transcription and translation modals when a user has remaining quota but not enough to cover all selected submissions.

  • Near-limit evaluator implemented: evaluateNearLimit now compares the pre-computed required amount against the API's balance_value, firing only when 0 < remaining < required. Results are stored as raw seconds/characters and formatted via formatTimeFromSeconds for human-readable display, resolving the earlier Math.floor-to-minutes precision issue noted in previous review threads.
  • Pre-filtering for accuracy: Both modals now compute nearLimitRequiredAmount from a filtered submission set (excluding already-transcribed/already-translated rows) so the quota estimate aligns with what the API will actually process — addressing the false-positive blocking issue identified in prior reviews.
  • Removed legacy alerts: The inline fallback Alert components and their hasExceededLimit && activeAlerts.length === 0 guard conditions are removed now that the evaluator pipeline is complete, consolidating all quota-related feedback through BulkProcessingAlerts.

Confidence Score: 5/5

Safe to merge — the change is well-scoped to the alert pipeline and both modals, previous review concerns have been addressed, and no new defects were found.

The near-limit evaluator is correctly implemented with proper guards (missing data, zero required amount, non-positive balance), is mutually exclusive with the reached-limit path, and the pre-filtering in both modals aligns the quota estimate with the submissions that will actually consume quota. All three issues raised in the previous review cycle have been resolved. Unit test coverage for the new evaluator is thorough.

No files require special attention. The two modal files carry the most logic but both are internally consistent.

Reviews (6): Last reviewed commit: "cr fix" | Re-trigger Greptile

@magicznyleszek

Copy link
Copy Markdown
Member Author

@greptile-apps review again

@magicznyleszek magicznyleszek marked this pull request as ready for review July 9, 2026 15:17

@duvld duvld left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions

{t('Start Transcription')}
</ButtonNew>
)}
{hasExceededLimit && (

@duvld duvld Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change the logic here to also display this button if the user is requesting more than their ASR allowance? Or say something like "your selection will surpass the amount of ASR time you have left, please purchase an add-on or select fewer submissions"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When user has not enough minutes left there will be an alert stating what are the options. Ignacio confirmed we don't want to display "Purchase add-on" button here - mu guess is that since user has some minutes left after all, we don't want to be forcing a purchase on them just yet
Screenshot 2026-07-13 at 11 21 49

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the BulkTranscriptionModal. Should we remove this comment and update the logic here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed both legacy alerts, since BulkProcessingAlerts are handling those cases already :)


const remainingAmount = balance.balance_value

if (remainingAmount <= 0 || remainingAmount >= nearLimitRequiredAmount) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'm reading this right. Why do we return an error if the user's remaining amount of time is greater than the audio duration time?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually correct, I have added a comment to explain it :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will also simplify the code to not return that weird createInactiveResult but instead simple null

selectedRegion?: string
actionType: BulkActionType
/** Required amount for full job in base units: seconds (transcription) or characters (translation). */
nearLimitRequiredAmount?: number

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this variable, at least it's a bit confusing for me. When we say "required amount for full job" we mean the return of the duration form useCalculateAudioDuration right? I think "near limit" adds some ambiguity when this is the actual duration number

assetUid: props.assetUid,
})

const nearLimitRequiredSeconds =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about the variable name, I think it reads better if we drop the "near limit" prefix

assetUid: props.assetUid,
})

const nearLimitRequiredSeconds =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand in the preview steps, we want this number to be big to simulate not having enough ASR minutes left(?) I tried setting this to very large numbers (1,000,000,000,000) and Number.POSITIVE_INFINITY and the alert still doesn't show.

@magicznyleszek

Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

…bed-alert' into leszek/dev-1399-near-limit-alert

# Conflicts:
#	jsapp/js/components/submissions/BulkProcessingModals/alerts/alertEvaluators.tests.ts

Copilot AI commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved in f27a5da by merging the latest leszek/dev-1410-already-transcribed-alert changes into this branch and fixing the conflict in alertEvaluators.tests.ts.

Base automatically changed from leszek/dev-1410-already-transcribed-alert to main July 13, 2026 09:04
# Conflicts:
#	jsapp/js/components/submissions/BulkProcessingModals/BulkTranscriptionModal/BulkTranscriptionModal.tsx
#	jsapp/js/components/submissions/BulkProcessingModals/alerts/BulkProcessingAlerts.stories.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants